maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Rucha!

On Sep 08, Rucha Deodhar wrote:
> Hi, Sergei !
> Thanks for the review.
> 
> > > diff --git a/extra/my_print_defaults.c b/extra/my_print_defaults.c
> > > index b7f52382721..e2ec0b00923 100644
> > > --- a/extra/my_print_defaults.c
> > > +++ b/extra/my_print_defaults.c
> > > @@ -48,20 +48,6 @@ static struct my_option my_long_options[] =
> >
> > you forgot the "config-file", which is here, just about "debug"
> 
> I am sorry, I don't understand.

Oops. Sorry. I looked at my_print_defaults.c in the checked out
branch and I saw:

  {"config-file", 'c', "Deprecated, please use --defaults-file instead. "
   "Name of config file to read; if no extension is given, default "
   "extension (e.g., .ini or .cnf) will be added",
   (char**) &config_file, (char**) &config_file, 0, GET_STR, REQUIRED_ARG,
   0, 0, 0, 0, 0, 0},
#ifdef DBUG_OFF
  {"debug", '#', "This is a non-debug version. Catch this and exit",
   0,0, 0, GET_DISABLED, OPT_ARG, 0, 0, 0, 0, 0, 0},
#else

so I wrote that "config-file" should also be removed.
But now I see that it's present in 10.2, but not in 10.4, it was
already removed.

So, ignore that my comment, you didn't forget config-file, it was
removed in 10.4, I looked at the wrong branch.

> > >    {"debug", '#', "Output debug log", (char**) &default_dbug_option,
> > >     (char**) &default_dbug_option, 0, GET_STR, OPT_ARG, 0, 0, 0, 0, 0, 0},
> > >  #endif
> > > -  {"defaults-file", 'c',
> > > -   "Read this file only, do not read global or per-user config "
> > > -   "files; should be the first option",
> > > -   (char**) &config_file, (char*) &config_file, 0, GET_STR, REQUIRED_ARG,
> > > @@ -141,49 +124,44 @@ static int get_options(int *argc,char ***argv)
> > >    return 0;
> > >  }
> > >
> > > -static char *make_args(const char *s1, const char *s2)
> > > -{
> > > -  char *s= malloc(strlen(s1) + strlen(s2) + 1);
> > > -  strmov(strmov(s, s1), s2);
> > > -  return s;
> > > -}
> > > -
> > >  int main(int argc, char **argv)
> > >  {
> > > -  int count= 0, error, no_defaults= 0;
> > > +  int count, error, args_used;
> > >    char **load_default_groups= 0, *tmp_arguments[6];
> > > -  char **argument, **arguments, **org_argv;
> > > -  int nargs, i= 0;
> > > +  char **argument, **arguments, **org_argv, **new_argv, **pos;
> > > +  int nargs, i= 0, index=0;
> > >    MY_INIT(argv[0]);
> > >
> > >    org_argv= argv;
> > > -  if (*argv && !strcmp(*argv, "--no-defaults"))
> > > -  {
> > > -    argv++;
> > > -    ++count;
> > > -    no_defaults= 1;
> > > -  }
> > > -  /* Copy program name and --no-defaults if present*/
> > > +  args_used= get_defaults_options(argv);
> > > +
> > > +  /* Copy defaults-xxx arguments & program name */
> > > +  count= args_used;
> > >    arguments= tmp_arguments;
> > > -  memcpy((char*) arguments, (char*) org_argv, (++count)*sizeof(*org_argv));
> > > +  memcpy((char*) arguments, (char*) org_argv, count*sizeof(*org_argv));
> > >    arguments[count]= 0;
> > >
> > > -  /* Check out the args */
> > > -  if (get_options(&argc,&argv))
> > > -    cleanup_and_exit(1);
> > > -
> > > -  if (!no_defaults)
> > > +  /*
> > > +     We already process --defaults* options at the beginning in
> > > +     get_defaults_options(). So, now we we make a new commandline which
> > > +     has all options except --defaults* options and pass it to handle_options()
> > > +     instead of original commandline.
> > > +  */
> > > +  new_argv= (char**)malloc(argc*sizeof(*org_argv));
> > > +  for (pos= org_argv; *pos; pos++)
> > >    {
> > > +    if (!is_prefix(*pos, "--defaults-extra-file=") &&
> > > +       !is_prefix(*pos, "--defaults-file=") &&
> > > +       !is_prefix(*pos, "--defaults-group-suffix="))
> > > +      new_argv[index++]= *(pos);
> >
> > Why are you doing it?
> > After get_defaults_options, you know how many arguments were default,
> > just skip args_used, and invoke handle_options on the rest.
> 
> I did it to filter all the --defaults* options, anywhere on the command
> line.
> 
> get_defaults_options() counts the program name in addition to --defaults*
> options, so args_used will at least be 1. So skipping argv using agrs_used
> will at least skip the program name even before it is passed to
> handle_options(). But argv is also skipped in handle_options() to skip the
> program name.
> So one extra argument will get skipped too.

correct, you need to skip args_used-1, good point.

> Say --defaults* options are somewhere at the end, they will not get counted
> in get_defaults_options(). So --defaults* will be passed to
> handle_options() without getting skipped. And handle_options() will give an
> error even if the options are valid because
> it will not find --defaults* options in my_long_options[].

This is what we want, right? That's why you're doing it.
Try `mariadb` client or `mariadb-dump` or `mariadbd` server, anything,
and put --defaults* option not at the beginning. It'll be "unknown
option". The idea is to make my_print_defaults *not special*, to behave
like any other executable.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx