List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:August 26 2010 12:15pm
Subject:Re: bzr commit into mysql-5.5-bugfixing branch (marc.alff:3194) Bug#55873
View as plain text  
Bonjour Marc,

Marc Alff a écrit, Le 26.08.2010 02:59:
> #At file:///home/malff/BZR_TREE/mysql-5.5-bugfixing-55873/ based on
> revid:jon.hauglid@stripped
> 
>  3194 Marc Alff	2010-08-25
>       Bug#55873 short startup options do not work in 5.5
>       
>       Before this fix, the server did not recognize 'short' (as in -a)
>       options but only 'long' (as in --ansi) options
>       in the startup command line, due to earlier changes in 5.5
>       introduced for the performance schema.
>       
>       The root cause is that handle_options() did not honor the
>       my_getopt_skip_unknown flag when parsing 'short' options.
>       
>       The fix changes handle_options(), so that my_getopt_skip_unknown is
>       honored in all cases.
>       
>       Note that there are limitations to this,
>       see the added doxygen documentation in handle_options().
>       
>       The current usage of handle_options() by the server to
>       parse early performance schema options fits within the limitations.
>       This has been enforced by an assert for PARSE_EARLY options, for safety.

> === modified file 'mysys/my_getopt.c'
> --- a/mysys/my_getopt.c	2010-08-05 12:34:19 +0000
> +++ b/mysys/my_getopt.c	2010-08-26 00:59:28 +0000
> @@ -98,6 +98,49 @@ void my_getopt_register_get_addr(my_geto
>    matches with one of the options in struct 'my_option'.
>    Check that option was given an argument if it requires one
>    Call the optional 'get_one_option()' function once for each option.
> +
> +  Note that handle_options() can be invoked multiple times to
> +  parse a command line in several steps.
> +  In this case, use the global flag @c my_getopt_skip_unknown to indicate
> +  that options unknown in the current step should be preserved in the
> +  command line for later parsing in subsequent steps.
> +
> +  For 'long' options (--a_long_option), @c my_getopt_skip_unknown is
> +  fully supported. Command line parameters such as:
> +  - "--a_long_option"
> +  - "--a_long_option=value"
> +  - "--a_long_option value"
> +  will be preserved as is when the option is not known.
> +
> +  For 'short' options (-S), support for @c my_getopt_skip_unknown
> +  comes with some limitation, because several short options
> +  can also be specified together in the same command line argument,
> +  as in "-XYZ".
> +
> +  The first use case supported is: all short options are declared.
> +  handle_options() will be able to interpret "-XYZ" as one of:
> +  - an unknown X option
> +  - "-X -Y -Z", three short options with no arguments
> +  - "-X -YZ", where Y is a short option with argument Z
> +  - "-XYZ", where X is a short option with argument YZ
> +  based on the full short options specifications.
> +
> +  The second use case supported is: no short option is declared.
> +  handle_options() will reject "-XYZ" as unknown, to be parsed later.
> +
> +  The use case that is explicitly not supported is to provide
> +  only a partial list of short options to handle_options().
> +  This function can not be expected to extract some option Y
> +  in the middle of the string "-XYZ" in these conditions,
> +  without knowing if X will be declared an option later.
> +
> +  Note that this limitation only impacts parsing of several
> +  short options from the same command line argument,
> +  as in "mysqld -anW5".
> +  When each short option is properly separated out in the command line
> +  argument, for example in "mysqld -a -n -w5", the code would actually
> +  work even with partial options specs given at each stage.
> +
>    @param [in, out] argc      command line options (count)
>    @param [in, out] argv      command line options (values)
>    @param [in] longopts       descriptor of all valid options
> @@ -464,14 +507,57 @@ int handle_options(int *argc, char ***ar
>  	  }
>  	  if (!opt_found)
>  	  {
> -	    if (my_getopt_print_errors)
> -              my_getopt_error_reporter(ERROR_LEVEL,
> -                                       "%s: unknown option '-%c'", 
> -                                       my_progname, *optend);
> -	    return EXIT_UNKNOWN_OPTION;
> +            if (my_getopt_skip_unknown)
> +            {
> +              /*
> +                We are currently parsing a single argv[] argument
> +                of the form "-XYZ", and parsing is done in multiple phases.

I don't know what "parsing is done in multiple phases" means here.

> +                One or the argument found is not an option.
> +              */
> +              if (optend == cur_arg)
> +              {
> +                /*
> +                  The first argument, "-X", is not an option
> +                  In this case, the entire argument "-XYZ" is rejected
> +                  from this phase, and preserved as is for later parsing.
> +                */
> +                (*argv)[argvpos++]= *pos;
> +              }
> +              else
> +              {
> +                /*
> +                  We are in the middle of an "-XYZ" string already,
> +                  "-X" has already been parsed, and "Y" (pointed by optend)
> +                  is not an option.
> +                  Hack the string "-XYZ" to make a "-YZ" substring in it,
> +                  and push that to the next parsing phase.
> +                */
> +                DBUG_ASSERT(optend > *pos);
> +                DBUG_ASSERT(optend > cur_arg);
> +                DBUG_ASSERT(optend <= *pos + strlen(*pos));
> +                DBUG_ASSERT(*optend);
> +                optend--;
> +                optend[0]= '-'; /* replace 'X' by '-' */
> +                (*argv)[argvpos++]= optend;
> +              }

you could have collapsed if()/else() into a single path (the one of 
else(), if you relaxed some assertions). When optend==curarg, the else() 
code boils down to replacing '-' with '-'.
But that is at your option.

> +              /*
> +                Do not continue to parse at the current "-XYZ" argument,
> +                skip to the next argv[] argument instead.
> +              */
> +              optend= (char*) " ";
> +            }
> +            else
> +            {
> +              if (my_getopt_print_errors)
> +                my_getopt_error_reporter(ERROR_LEVEL,
> +                                         "%s: unknown option '-%c'",
> +                                         my_progname, *optend);
> +              return EXIT_UNKNOWN_OPTION;
> +            }
>  	  }
>  	}
> -	(*argc)--; /* option handled (short), decrease argument count */
> +        if (opt_found)
> +          (*argc)--; /* option handled (short), decrease argument count */
>  	continue;
>        }
>        if ((error= setval(optp, value, argument, set_maximum_value)))
> @@ -479,7 +565,7 @@ int handle_options(int *argc, char ***ar
>        if (get_one_option && get_one_option(optp->id, optp, argument))
>          return EXIT_UNSPECIFIED_ERROR;
>  
> -      (*argc)--; /* option handled (short or long), decrease argument count */
> +      (*argc)--; /* option handled (long), decrease argument count */
>      }
>      else /* non-option found */
>        (*argv)[argvpos++]= cur_arg;
> 
> === modified file 'sql/set_var.cc'
> --- a/sql/set_var.cc	2010-07-27 10:25:53 +0000
> +++ b/sql/set_var.cc	2010-08-26 00:59:28 +0000
> @@ -153,6 +153,17 @@ sys_var::sys_var(sys_var_chain *chain, c
>    guard(lock), offset(off), on_check(on_check_func), on_update(on_update_func),
>    is_os_charset(FALSE)
>  {
> +  /*
> +    There is a limitation in handle_options() related to short options:
> +    - either all short options should be declared when parsing in multiple stages,
> +    - or none should be declared.
> +    Because a lot of short options are used in the normal parsing phase
> +    for mysqld, we enforce here that no short option is present
> +    in the first (PARSE_EARLY) stage.
> +    See handle_options() for details.
> +  */
> +  DBUG_ASSERT(parse_flag == PARSE_NORMAL || getopt_id <= 0 || getopt_id >=
> 255);

I would have preferred the limitation to be checked in handle_options() 
(as sketched in my previous mail), I find it would be:
1) more logical (as it's handle_options() which has the limitations, it 
should enforce it by itself)
2) safer; the assertion above is only about system variables; there are 
short options which are not tied to system variables, like "--ansi/-a".

Why not have handle_options() enforce that its "longopts" can have short 
options only once in the process' lifetime, in debug binaries?

This isn't a showstopper for pushing the bugfix; if there is a hurry for 
pushing now we can rediscuss the assertion later, after the push.
So, ok to push.

>    name.str= name_arg;
>    name.length= strlen(name_arg);
>    DBUG_ASSERT(name.length <= NAME_CHAR_LEN);

-- 
Mr. Guilhem Bichot <guilhem.bichot@stripped>
Oracle / MySQL / Optimizer team, Lead Software Engineer
Bordeaux, France
www.oracle.com / www.mysql.com
Thread
bzr commit into mysql-5.5-bugfixing branch (marc.alff:3194) Bug#55873Marc Alff26 Aug
  • Re: bzr commit into mysql-5.5-bugfixing branch (marc.alff:3194) Bug#55873Guilhem Bichot26 Aug