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@mail.ru> committer: Vlad Lesin <vlad_lesin@mail.ru> 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@mariadb.org