maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #12300
Re: 9bdf35e90f3: MDEV-18215: mariabackup does not report unknown command line options
Hi, Vlad!
Some comments about your command-line option handling code
On Jun 25, Vlad Lesin wrote:
> revision-id: 9bdf35e90f3 (mariadb-10.4.11-242-g9bdf35e90f3)
> parent(s): ceaa8b647a1
> author: Vlad Lesin <vlad_lesin@xxxxxxx>
> committer: Vlad Lesin <vlad_lesin@xxxxxxx>
> timestamp: 2020-06-14 13:23:07 +0300
> message:
>
> MDEV-18215: mariabackup does not report unknown command line options
> MDEV-21298: mariabackup doesn't read from the [mariadbd] and [mariadbd-X.Y]
> server option groups from configuration files
> MDEV-21301: mariabackup doesn't read [mariadb-backup] option group in
> configuration file
>
> All three issues require to change the same code, that is why their
> fixes are joined in one commit.
>
> The fix is in invoking load_defaults_or_exit() and handle_options() for
> backup-specific groups separately from client-server groups to let the last
> handle_options() call fail on unknown backup-specific options.
>
> The order of options procesing is the following:
> 1) Load server groups and process server options, ignore unknown
> options
> 2) Load client groups and process client options, ignore unknown
> options
> 3) Load backup groups and process client-server options, exit on
> unknown option
> 4) Process --mysqld-args command line options, ignore unknown options
>
> New global flag my_handle_options_init_variables was added to have
> ability to invoke handle_options() for the same allowed options set
> several times without re-initialising previously set option values.
>
> --password value destroying is moved from option processing callback to
> mariabackup's handle_options() function to have ability to invoke server's
> handle_options() several times for the same possible allowed options
> set.
>
> Galera invokes wsrep_sst_mariabackup.sh with mysqld command line
> options to configure mariabackup as close to the server as possible.
> It is not known what server options are supported by mariabackup when the
> script is invoked. That is why new mariabackup option "--mysqld-args" is added,
> all unknown options that follow this option will be silently ignored.
>
> wsrep_sst_mariabackup.sh was also changed to:
> - use "--mysqld-args" mariabackup option to pass mysqld options,
> - remove deprecated innobackupex mode,
> - remove unsupported mariabackup options:
> --encrypt
> --encrypt-key
> --rebuild-indexes
> --rebuild-threads
>
> diff --git a/extra/mariabackup/xtrabackup.cc b/extra/mariabackup/xtrabackup.cc
> index 852ef4efe56..19058398258 100644
> --- a/extra/mariabackup/xtrabackup.cc
> +++ b/extra/mariabackup/xtrabackup.cc
> @@ -5990,11 +5977,23 @@ void setup_error_messages()
> die("could not initialize error messages");
> }
>
> -void
> -handle_options(int argc, char **argv, char ***argv_client, char ***argv_server)
> +/** Handle mariabackup options. The options are handled with the following
> +order:
> +
> +1) Load server groups and process server options, ignore unknown options
> +2) Load client groups and process client options, ignore unknown options
> +3) Load backup groups and process client-server options, exit on unknown option
> +4) Process --mysqld-args options, ignore unknown options
> +
> +@param[in] argc arguments count
> +@param[in] argv arguments array
> +@param[out] argv_server server options including loaded from server groups
> +@param[out] argv_client client options including loaded from client groups
> +@param[out] argv_backup backup options including loaded from backup groups */
> +void handle_options(int argc, char **argv, char ***argv_server,
> + char ***argv_client, char ***argv_backup)
could you please rename this function? handle_options() is part of
my_getopt api, the main function to parse the argv array of options.
this one has a different prototype, so C++ can distinguish between them,
but it's very confusing, like having your own strcmp(int, char*, void*).
> {
> /* Setup some variables for Innodb.*/
> srv_operation = SRV_OPERATION_RESTORE;
>
> files_charset_info = &my_charset_utf8_general_ci;
> @@ -6021,49 +6020,64 @@ handle_options(int argc, char **argv, char ***argv_client, char ***argv_server)
> bool prepare = false;
>
> char conf_file[FN_REFLEN];
> - int argc_client = argc;
> - int argc_server = argc;
> -
> - /* scan options for group and config file to load defaults from */
> - for (i = 1; i < argc; i++) {
> -
> - char *optend = strcend(argv[i], '=');
> -
> - if (strncmp(argv[i], "--defaults-group",
> - optend - argv[i]) == 0) {
> - defaults_group = optend + 1;
> - append_defaults_group(defaults_group,
> - xb_server_default_groups,
> - array_elements(xb_server_default_groups));
> - }
>
> - if (strncmp(argv[i], "--login-path",
> - optend - argv[i]) == 0) {
> - append_defaults_group(optend + 1,
> - xb_client_default_groups,
> - array_elements(xb_client_default_groups));
> - }
> + // array_elements() will not work for load_defaults, as it is defined
> + // as external symbol, so let's use dynamic array to have ability to
> + // add new server default groups
> + std::vector<const char *> server_default_groups;
>
> - if (!strncmp(argv[i], "--prepare",
> - optend - argv[i])) {
> - prepare = true;
> - }
> + for (const char **default_group= load_default_groups; *default_group;
> + ++default_group)
> + server_default_groups.push_back(*default_group);
>
> - if (!strncmp(argv[i], "--apply-log",
> - optend - argv[i])) {
> - prepare = true;
> - }
> + std::vector<char *> mysqld_args;
> + std::vector<char *> mariabackup_args;
> + mysqld_args.push_back(argv[0]);
> + mariabackup_args.push_back(argv[0]);
>
> - if (!strncmp(argv[i], "--target-dir",
> - optend - argv[i]) && *optend) {
> - target_dir = optend + 1;
> - }
> + /* scan options for group and config file to load defaults from */
> + for (i= 1; i < argc; i++)
> + {
> + char *optend= strcend(argv[i], '=');
> + if (mysqld_args.size() > 1 ||
> + strncmp(argv[i], "--mysqld-args", optend - argv[i]) == 0)
> + {
> + mysqld_args.push_back(argv[i]);
> + continue;
> + }
> + else
> + mariabackup_args.push_back(argv[i]);
>
> - if (!*optend && argv[i][0] != '-') {
> - target_dir = argv[i];
> - }
> - }
> + if (strncmp(argv[i], "--defaults-group", optend - argv[i]) == 0)
> + {
> + defaults_group= optend + 1;
> + server_default_groups.push_back(defaults_group);
> + }
> + else if (strncmp(argv[i], "--login-path", optend - argv[i]) == 0)
> + {
> + append_defaults_group(optend + 1, xb_client_default_groups,
> + array_elements(xb_client_default_groups));
> + }
> + else if (!strncmp(argv[i], "--prepare", optend - argv[i]))
> + {
> + prepare= true;
> + }
> + else if (!strncmp(argv[i], "--apply-log", optend - argv[i]))
> + {
> + prepare= true;
> + }
> + else if (!strncmp(argv[i], "--target-dir", optend - argv[i]) &&
> + *optend)
> + {
> + target_dir= optend + 1;
> + }
I don't understand. Why is this manual parsing of options if you use
my_getopt anyway?
> + else if (!*optend && argv[i][0] != '-')
> + {
> + target_dir= argv[i];
> + }
> + }
>
> + server_default_groups.push_back(NULL);
> snprintf(conf_file, sizeof(conf_file), "my");
>
> if (prepare && target_dir) {
> @@ -6102,7 +6122,6 @@ handle_options(int argc, char **argv, char ***argv_client, char ***argv_server)
> optp->u_max_value = (G_PTR *) &global_max_value;
what is that???
> }
>
> -
> /* Throw a descriptive error if --defaults-file or --defaults-extra-file
> is not the first command line argument */
and what is that???
You know, if there are ~10 programs using load_defaults/my_getopt and
not a single one of them is doing this, perhaps mariabackup shouldn't be
doing it either?
> for (int i = 2 ; i < argc ; i++) {
> @@ -6143,18 +6163,76 @@ handle_options(int argc, char **argv, char ***argv_client, char ***argv_server)
> xb_client_options, xb_get_one_option)))
> exit(ho_error);
>
> + /* 3) Load backup groups and process client-server options, exit on
> + unknown option */
> +
> + load_defaults_or_exit(conf_file, backup_default_groups, &argc_backup,
> + argv_backup);
> + for (n= 0; (*argv_backup)[n]; n++)
> + {
> + };
> + argc_backup= n;
> +
> + my_handle_options_init_variables = FALSE;
> +
> + if (argc_backup > 0 &&
> + (ho_error= handle_options(&argc_backup, argv_backup,
> + xb_server_options, xb_get_one_option)))
> + exit(ho_error);
I think this whole approach is wrong.
And my_handle_options_init_variables highlights it.
There should be only one invocation of handle_options(), otherwise you
won't get the correct behavior of what options override what.
You can call handle_options() separately for server and client options,
because those are disjoint sets of options. But you cannot call it twice
for xb_server_options.
Now you call handle_options 5 times. I suggest to rewrite the code to do
it only twice. As:
read *all* groups with load_defaults_or_exit(), and read *all* options
with one handle_options() call.
This will be the actual parsing and setting values of variables,
ignoring unknown options. But you still want to issue an error for
unknown options, so
truncate the list of command-line options before --mysqld_args,
read only mariabackup groups, call handle_options again, erroring
out on unknown options.
this should not set any values, it's just for an error, so before that
you reset all pointers to variables - just like you do now for
--maximum.
Because this approach parses all options at once, you'll get the same
behavior regarding what options override what, as any other tool that
uses my_getopt.
> +
> + /* Add back the program name handle_options removes */
> + ++argc_backup;
> + --(*argv_backup);
> +
> + if (innobackupex_mode && argc_backup > 0 &&
> + !ibx_handle_options(&argc_backup, argv_backup))
> + exit(EXIT_FAILURE);
> +
> + my_getopt_skip_unknown = FALSE;
> +
> + if (argc_backup > 0 &&
> + (ho_error= handle_options(&argc_backup, argv_backup,
> + xb_client_options, xb_get_one_option)))
> + exit(ho_error);
> +
> + if (opt_password)
> + {
> + char *argument= opt_password;
> + char *start= argument;
> + opt_password= my_strdup(opt_password, MYF(MY_FAE));
> + while (*argument)
> + *argument++= 'x'; // Destroy argument
> + if (*start)
> + start[1]= 0;
> + }
> +
> + /* 4) Process --mysqld-args options, ignore unknown options */
> +
> + my_getopt_skip_unknown = TRUE;
> +
> + int argc_mysqld = static_cast<int>(mysqld_args.size());
> + if (argc_mysqld > 1)
> + {
> + char **argv_mysqld= &mysqld_args[0];
> + if ((ho_error= handle_options(&argc_mysqld, &argv_mysqld,
> + xb_server_options, xb_get_one_option)))
> + exit(ho_error);
> + }
> +
> + my_handle_options_init_variables = TRUE;
> +
> /* Reject command line arguments that don't look like options, i.e. are
> not of the form '-X' (single-character options) or '--option' (long
> options) */
> - for (int i = 0 ; i < argc_client ; i++) {
> - const char * const opt = (*argv_client)[i];
> + for (int i = 0 ; i < argc_backup ; i++) {
> + const char * const opt = (*argv_backup)[i];
>
> if (strncmp(opt, "--", 2) &&
> !(strlen(opt) == 2 && opt[0] == '-')) {
> bool server_option = true;
>
> - for (int j = 0; j < argc_server; j++) {
> - if (opt == (*argv_server)[j]) {
> + for (int j = 0; j < argc_backup; j++) {
> + if (opt == (*argv_backup)[j]) {
> server_option = false;
> break;
> }
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx
