maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

Thanks!

On Jun 17, Sergey Vojtovich wrote:
> Hi Sergei,
> 
> ok to push. A few minor questions inline.
> 
> On Mon, Jun 16, 2014 at 06:51:53PM +0200, serg@xxxxxxxxxxx wrote:
> > 
> > ---
> >  mysys/my_getopt.c                   | 81 +++++++++++++++++++++++++++------
> >  plugin/server_audit/server_audit.c  |  2 +-
> >  sql/log.cc                          |  3 +-
> >  sql/mysqld.cc                       |  8 ++--
> >  sql/sql_plugin.cc                   |  2 +-
> >  sql/sys_vars.cc                     | 90 ++++++++-----------------------------
> >  storage/maria/ha_maria.cc           | 14 ++----
> >  storage/myisam/ha_myisam.cc         |  7 ++-
> >  storage/xtradb/handler/ha_innodb.cc | 35 +++++++--------
> >  9 files changed, 116 insertions(+), 126 deletions(-)
> Hmmm... no test case affected?

mysqld--help test has it. Hm, I've actually updated it, but it was after
this email, sorry.

> > diff --git a/mysys/my_getopt.c b/mysys/my_getopt.c
> > index 2e97417..3662757 100644
> > --- a/mysys/my_getopt.c
> > +++ b/mysys/my_getopt.c
> > @@ -1300,6 +1300,48 @@ static uint print_name(const struct my_option *optp)
> >    return s - optp->name;
> >  }
> >  
> > +/** prints option comment with indentation and wrapping.
> > +
> > +  The comment column starts at startpos, and has width of width
> > +  Current cursor position is curpos, returns new cursor position
> > +
> > +Note: can print one character beyond width!
> The above line, is it properly indented?

Fixed

> > +*/
> > +static uint print_comment(const char *comment,
> > +                          int curpos, int startpos, int width)
> Probably make width local to this function, it doesn't seem to be used by the
> caller anyway?

I'll try to move both startpos and width, it that'll be to difficult,
I'll keep both in the caller - they're tightly related to each other.

> > +{
> > +  const char *end= strend(comment);
> > +  int endpos= startpos + width;
> > +
> > +  for (; curpos < startpos; curpos++)
> > +    putchar(' ');
> > +
> > +  if (*comment == '.' || *comment == ',')
> > +  {
> > +    putchar(*comment);
> > +    comment++;
> > +    curpos++;
> > +  }
> > +
> > +  while (end - comment > endpos - curpos)
> > +  {
> > +    const char *line_end;
> > +    for (line_end= comment + endpos - curpos;
> > +         line_end > comment && *line_end != ' ';
> > +         line_end--);
> > +    for (; comment < line_end; comment++)
> > +      putchar(*comment);
> I would probably replace it with fwrite(), but fine with this form too.

That was the old code, I only moved it to a separate function.

> > +    while (*comment == ' ')
> > @@ -1359,25 +1401,36 @@ void my_print_help(const struct my_option *options)
> >  	col= 0;
> >        }
> >      }
> > -    for (; col < name_space; col++)
> > -      putchar(' ');
> >      if (optp->comment && *optp->comment)
> Cleanup suggestion: optp->comment can't be NULL here.

fixed.

> >      {
> > -      const char *comment= optp->comment, *end= strend(comment);
> > +      col= print_comment(optp->comment, col, name_space, comment_space);
> >  
> > -      while ((uint) (end - comment) > comment_space)
> > +      switch (optp->var_type & GET_TYPE_MASK) {
> > +      case GET_ENUM:
> > +        typelib_help= ". One of: ";
> > +        break;
> > +      case GET_SET: 
> > +        typelib_help= ". Any combination of: ";
> > +        break;
> > +      case GET_FLAGSET:
> > +        typelib_help= ". Takes a comma-separated list of option=value pairs, "
> > +          "where value is on or off, and options are: ";
> > +        break;
> > +      }
> > +      if (typelib_help &&
> > +          strstr(optp->comment, optp->typelib->type_names[0]) == NULL)
> Should we handle case when optp->typelib == NULL ||
> optp->typelib->type_names[0] == NULL? Probably not.

Agree. I don't think we should.

> I believe this condition is fragile: comment string may mention type_names[0],
> but not list possible values. OTOH since this condition is for very limited data
> set it should cause problems only in rare cases.

Right. My first version didn't have strstr, it always printed the list
of values. But some of sysvars have useful help, like

  static MYSQL_SYSVAR_ENUM(group_commit, maria_group_commit,
         PLUGIN_VAR_RQCMDARG,
         "Specifies Aria group commit mode. "
         "Possible values are \"none\" (no group commit), "
         "\"hard\" (with waiting to actual commit), "
         "\"soft\" (no wait for commit (DANGEROUS!!!))",
         NULL, update_maria_group_commit,
         TRANSLOG_GCOMMIT_NONE, &maria_group_commit_typelib);

I did not want to remove this helpful message, and it wouldn't make any
sense to append the list of values to that. So, as a symple heuristic
I've added this strstr().

> >        }
> > -      printf("%s", comment);
> > +      putchar('\n');
> >      }
> > -    putchar('\n');
> Does this mean if comment is empty there won't be a new line?

Good catch, thanks.

> >      if ((optp->var_type & GET_TYPE_MASK) == GET_BOOL)
> >      {
> >        if (optp->def_value != 0)
> ...skip...
> 
> > @@ -2785,7 +2745,7 @@ static Sys_var_mybool Sys_master_verify_checksum(
> >  
> >  /* These names must match RPL_SKIP_XXX #defines in slave.h. */
> >  static const char *replicate_events_marked_for_skip_names[]= {
> > -  "replicate", "filter_on_slave", "filter_on_master", 0
> > +  "REPLICATE", "FILTER_ON_SLAVE", "FILTER_ON_MASTER", 0
> >  };
> Does this mean there is a convention that values must be in upper case?

No, the convention is that values must be in the same letter case as in
the help message (for strstr() to find them). These values (above) are
not user-visible, my_getopt compares them case-insensitively anyway.
But the help message is user visible, and for this option it mentioned
allowed values in upper case.

I could have used strcasestr() and compare with the help text
case-insensitively too, but decided against it to reduce the number of
false positives (think of values like "ON" - it can match tons of words,
"tons" itself included).

Regards,
Sergei