List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:August 23 2010 12:34pm
Subject:Re: bzr commit into mysql-trunk-bugfixing branch (marc.alff:3197)
Bug#55873
View as plain text  
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
Thread
bzr commit into mysql-trunk-bugfixing branch (marc.alff:3197) Bug#55873Marc Alff11 Aug
  • Re: bzr commit into mysql-trunk-bugfixing branch (marc.alff:3197)Bug#55873Guilhem Bichot23 Aug
    • Re: bzr commit into mysql-trunk-bugfixing branch (marc.alff:3197)Bug#55873Marc Alff24 Aug
      • Re: bzr commit into mysql-trunk-bugfixing branch (marc.alff:3197)Bug#55873Guilhem Bichot25 Aug