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
