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

Content reproduced on this site is the property of the respective copyright holders. It is not reviewed in advance by Oracle and does not necessarily represent the opinion of Oracle or any other party.