From: Marc Alff
Date: August 24 2010 9:25pm
Subject: Re: bzr commit into mysql-trunk-bugfixing branch (marc.alff:3197)
 Bug#55873
List-Archive: http://lists.mysql.com/commits/116696
Message-Id: <4C7438CB.4020004@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit


Hi Guilhelm

On 8/23/10 6:34 AM, Guilhem Bichot wrote:
> 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?

Yes, it works as is. I will use this in later patches.

> 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".

I agree.

>
> 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.

This is also my understanding of the code.
The loop is useless, but does not seem to cause any harm,
so I avoided touching it to avoid introducing more risk.

> 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.
>

Yes, you are correct.
Overall, handle_option() just feels like spaghetti code, and the fix was 
incomplete.

While analysing this code again after a week of rest, I think there is 
much more to it, and that the issue is not that easy to resolve.

In particular, consider the following loop:

       else  /* must be short option */
       {
         for (optend= cur_arg; *optend; optend++)
         {
...
         }
       }

My current understanding of this loop is that the code attempts to find 
*several* short options in the *same* argv[i] command line argument.

For example, "mysqld -alw5" is interpreted as "mysqld -a -l -w5"

The problem here is that I don't see how the server can parse "-XYZ" 
without knowing if X, Y and Z are options of not, that takes argument or 
not, because "-XYZ" could mean:
-X -Y -Z, where X Y and Z are short options with no arguments
-X -YZ, where Y is a short option with a value of Z
-XYZ, where X is a short option with a value of YZ

My conclusion then is that handle_option() can _NOT_ honor the skip 
unknown options flag and support this syntax at the same time, for short 
options all concatenated in the same argv[] argument.

Please confirm whether this is also your analysis, or if you find 
something missing.

At this point, I think the solutions to resolve the bug are:

1) Get rid of short options all together in 5.5
This is what the bug reporter actually suggested.

2) Continue to support short options in 5.5, but only with the explicit 
"-X -Y -Z" syntax, and not with the "-XYZ" syntax.
In that case, supporting my_getopt_skip_unknown in handle_options() for 
short options seems possible and not too intrusive.

This will be an incompatible change, forcing users to replace
"-anCutf8" with "-a -n -Cutf8",
or better with "--ansi --new --character_set_server=utf8".

I actually see this as a good thing.

Another option I explored was:

3) declare every options that has a "short" name as PARSE_EARLY, and 
reorder the initialization code accordingly, so that every short option 
is known to handle_options() the first time, so the code does not need 
to support my_getopt_skip_unknown.
This is however creating chaos in main_mysqld, and is a fix that I 
consider way to risky and way to intrusive.
The problem here is that:
- reordering options (between PARSE_EARLY and PARSE_NORMAL) based on the 
option *semantic* is ok: very low levels (performance schema, logging, 
...) are ok to parse first.
- reordering options based on whether there is a short hand notation or 
not, regardless of the option semantic and what it controls, just 
completely destabilize this otherwise very fragile initialization 
sequence. The embedded server initialization is deeply impacted too.

In short, I don't think 3) is viable.

Before going further, please let me know if you see another solution to 
this. My preference so far is 2).

Regards,
-- Marc