Hello Marc,
Marc Alff a écrit, Le 11.08.2010 19:00:
> #At file:///Users/malff/BZR_TREE/mysql-trunk-bugfixing-55873/ based on
> revid:jonathan.perkin@stripped
>
> 3197 Marc Alff 2010-08-11
> 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.
>
> 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.
> === added file 'mysql-test/suite/perfschema/t/bad_option_4.test'
> +
> +let $outfile= $MYSQLTEST_VARDIR/tmp/bad_option_4.txt;
> +--error 0,1
> +--remove_file $outfile
> +--error 1
> +--exec $MYSQLD_BOOTSTRAP_CMD --loose-console -a -h bad_option_h_param > $outfile
> 2>&1
Here the test expects that mysqld will fail because there is no
directory named bad_option_h_param. A failure proves that
bad_option_h_param properly passes the first call to handle_options()
(which is restricted to perfschema options), thanks to
my_getopt_skip_unknown, and then lands into the second call to
handle_options where -h is handled and the argument is detected as a
non-existent directory. All fine.
To make the test more precise, how about making it grep for
Can't change dir to.*bad_option_h_param
instead of just
bad_option_h_param ,
if technically possible?
The latter pattern is less selective, it could match even if the server
was buggy and rejected bad_option_h_param in the first handle_options()
call with something like "unknown option bad_option_h_param".
Your many tests are very good.
> === modified file 'mysys/my_getopt.c'
> --- a/mysys/my_getopt.c 2010-07-16 21:00:50 +0000
> +++ b/mysys/my_getopt.c 2010-08-11 17:00:26 +0000
> @@ -464,6 +464,17 @@ int handle_options(int *argc, char ***ar
> }
> if (!opt_found)
> {
> + if (my_getopt_skip_unknown)
> + {
> + /*
> + Preserve this unknown short option.
> + If there are any parameters associated with this short option,
> + as in "-X p1 p2 p3", parameters like p1 p2 p3 will be copied
> + in the result by the outer loop.
> + */
> + (*argv)[argvpos++]= *pos;
> + break;
> + }
The corresponding code for long options has a do...while() instead.
After reading handle_options() I cannot see a case where the do...while
will do more than one iteration (i.e. where pos!=first). I'm not asking
you to change do...while to be a single iteration (like your new code
above), it's a detail and I don't want to introduce a subtle bug, just
would like to know whether you agree on the observation or I'm missing
something.
There is a problem with the new "break;" above: it will break out of
for (optend= cur_arg etc)
and come to
if ((error= setval(optp, value, argument, set_maximum_value)))
below, which will do a setval() for a non-identified option.
> if (my_getopt_print_errors)
> my_getopt_error_reporter(ERROR_LEVEL,
> "%s: unknown option '-%c'",
> @@ -471,15 +482,21 @@ int handle_options(int *argc, char ***ar
> return EXIT_UNKNOWN_OPTION;
> }
> }
> - (*argc)--; /* option handled (short), decrease argument count */
> - continue;
> + if (opt_found)
> + {
> + (*argc)--; /* option handled (short), decrease argument count */
> + continue;
> + }
> }
"continue" is in the if() now: so if !opt_found, we don't "continue"
anymore, and go into setval() below, that is a problem (we don't know
what option this is, we shouldn't try to set anything); short options
have their own setval() a few lines earlier.
I think that
if (opt_found)
(*argc)--; /* option handled (short), decrease argument count */
continue;
would do.
> if ((error= setval(optp, value, argument, set_maximum_value)))
> return error;
> if (get_one_option && get_one_option(optp->id, optp, argument))
> return EXIT_UNSPECIFIED_ERROR;
>
> - (*argc)--; /* option handled (short or long), decrease argument count */
> + if (opt_found)
Here opt_found should always be true, or we set an unknown option :-)
When reading the function, it seems that if !opt_found, we either go to
the next command-line argument, or return with some error code, so never
come here.
> + {
> + (*argc)--; /* option handled (short or long), decrease argument count */
> + }
> }
> else /* non-option found */
> (*argv)[argvpos++]= cur_arg;
--
Mr. Guilhem Bichot <guilhem.bichot@stripped>
Oracle / MySQL / Optimizer team, Lead Software Engineer
Bordeaux, France
www.oracle.com / www.mysql.com
