May 2021 - developers - lists.mariadb.org

Re: [Maria-developers] e7762b3a725: MDEV-8334: Rename utf8 to utf8mb3
by Sergei Golubchik 05 May '21

05 May '21

Hi, Rucha! Looks great! Just one question below re. using global vs session old_behavior variable. On May 05, Rucha Deodhar wrote: > revision-id: e7762b3a725 (mariadb-10.5.2-583-ge7762b3a725) > parent(s): bfedf1eb4b6 > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-04-20 12:50:32 +0530 > message: > > MDEV-8334: Rename utf8 to utf8mb3 > > This patch changes the main name of 3 byte character set from utf8 to > utf8mb3. New old_mode UTF8_IS_UTF8MB3 is added and set TRUE by default, > so that utf8 would mean utf8mb3. If not set, utf8 would mean utf8mb4. > diff --git a/libmariadb b/libmariadb > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit b6f8883d9687936a50a7ed79bd9e5af2340efccd > +Subproject commit 03d983b287f8a1fe855cb5ed479a3f7ab4f922ab when rebasing and pushing into 10.6 take care not to rollback C/C changes. That is, only update C/C submodule reference if it's earlier than your 03d983b287f8a1fe855cb5ed479a3f7ab4f922ab > Binary files a/mysql-test/suite/sys_vars/r/character_set_results_basic.result and b/mysql-test/suite/sys_vars/r/character_set_results_basic.result differ > diff --git a/sql/item.cc b/sql/item.cc > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -2359,6 +2359,9 @@ left_is_superset(const DTCollation *left, const DTCollation *right) > > bool DTCollation::aggregate(const DTCollation &dt, uint flags) > { > + > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; if old_behavior is a session variable, then you should use session value here. > if (!my_charset_same(collation, dt.collation)) > { > /* > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -4099,6 +4098,8 @@ static int init_common_variables() > test purposes, to be able to start "mysqld" even if > the requested character set is not available (see bug#18743). > */ > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; ok, here there's no "session" yet, so global value is correct > for (;;) > { > char *next_character_set_name= strchr(default_character_set_name, ','); > diff --git a/sql/set_var.cc b/sql/set_var.cc > --- a/sql/set_var.cc > +++ b/sql/set_var.cc > @@ -533,11 +533,12 @@ static my_old_conv old_conv[]= > CHARSET_INFO *get_old_charset_by_name(const char *name) > { > my_old_conv *conv; > - > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; technically, you should use a session value here too. but see the old_conv array, it doesn't have "utf8" anywhere, so it doesn't matter what the flag is, you can use 0 there. > for (conv= old_conv; conv->old_name; conv++) > { > if (!my_strcasecmp(&my_charset_latin1, name, conv->old_name)) > - return get_charset_by_csname(conv->new_name, MY_CS_PRIMARY, MYF(0)); > + return get_charset_by_csname(conv->new_name, MY_CS_PRIMARY, MYF(utf8_flag)); > } > return NULL; > } > diff --git a/sql/sp.cc b/sql/sp.cc > --- a/sql/sp.cc > +++ b/sql/sp.cc > @@ -291,7 +291,8 @@ bool load_charset(MEM_ROOT *mem_root, > CHARSET_INFO **cs) > { > LEX_CSTRING cs_name; > - > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; but this needs a session value, I suppose > if (field->val_str_nopad(mem_root, &cs_name)) > { > *cs= dflt_cs; > @@ -324,9 +325,10 @@ bool load_collation(MEM_ROOT *mem_root, > *cl= dflt_cl; > return TRUE; > } > + myf utf8_flag= thd->get_utf8_flag(); Hmm, here you do use a session value. So, I suppose your using global value above was not an oversight, but you intentionally did it. What were your reasons? > > DBUG_ASSERT(cl_name.str[cl_name.length] == 0); > - *cl= get_charset_by_name(cl_name.str, MYF(0)); > + *cl= get_charset_by_name(cl_name.str, MYF(utf8_flag)); > > if (*cl == NULL) > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1051,14 +1052,16 @@ static inline void update_global_memory_status(int64 size) > @retval NULL on error > @retval Pointter to CHARSET_INFO with the given name on success > */ > -static inline CHARSET_INFO * > -mysqld_collation_get_by_name(const char *name, > +inline CHARSET_INFO * static inline, please > +mysqld_collation_get_by_name(const char *name, bool utf8_is_utf8mb3, up to you, but wouldn't it be more convenient to pass the flag here? Then you can invoke it with thd->utf8_flag() > CHARSET_INFO *name_cs= system_charset_info) > { > CHARSET_INFO *cs; > MY_CHARSET_LOADER loader; > + myf utf8_flag= utf8_is_utf8mb3 ? MY_UTF8_IS_UTF8MB3 : 0; > my_charset_loader_init_mysys(&loader); > - if (!(cs= my_collation_get_by_name(&loader, name, MYF(0)))) > + > + if (!(cs= my_collation_get_by_name(&loader, name, MYF(utf8_flag)))) > { > ErrConvString err(name, name_cs); > my_error(ER_UNKNOWN_COLLATION, MYF(0), err.ptr()); > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -10449,7 +10449,10 @@ merge_charset_and_collation(CHARSET_INFO *cs, CHARSET_INFO *cl) > CHARSET_INFO *find_bin_collation(CHARSET_INFO *cs) > { > const char *csname= cs->csname; > - cs= get_charset_by_csname(csname, MY_CS_BINSORT, MYF(0)); > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MY_UTF8_IS_UTF8MB3 : 0; Why not a session value here? > + cs= get_charset_by_csname(csname, MY_CS_BINSORT, MYF(utf8_flag)); > if (!cs) > { > char tmp[65]; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2bceae199bb: MDEV-14974: --port ignored for --host=localhost
by Sergei Golubchik 05 May '21

05 May '21

Hi, Brandon! Just a couple of minor issues: On May 05, Brandon Nesterenko wrote: > revision-id: 2bceae199bb (mariadb-10.6.0-24-g2bceae199bb) > parent(s): 4ff4df3232f > author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > committer: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > timestamp: 2021-05-05 01:01:01 +0000 > message: > > MDEV-14974: --port ignored for --host=localhost > > diff --git a/client/client_priv.h b/client/client_priv.h > index 64818d2ab8d..606b629a4d5 100644 > --- a/client/client_priv.h > +++ b/client/client_priv.h > @@ -136,3 +136,37 @@ enum options_client > Name of the sys schema database. > */ > #define SYS_SCHEMA_DB_NAME "sys" > + > + > +/** > + Utility function to implicitly change the connection protocol to a > + consistent value given the command line arguments. Additionally, > + warns the user that the protocol has been changed. > + > + Arguments: if you do `git show 2bceae199bb` you'll see git highlighting invisible spaces at line ends. Could you, please, remove them? (everywhere in your commit, not only in the comment above) > + @param [in] host Name of the host to connect to > + @param [in, out] opt_protocol Location of the protocol option > + variable to update > + @param [in] new_protocol New protocol to force > diff --git a/client/mysqlcheck.c b/client/mysqlcheck.c > index fb3103a318d..4f8891817e3 100644 > --- a/client/mysqlcheck.c > +++ b/client/mysqlcheck.c > @@ -285,10 +287,14 @@ static void usage(void) > > static my_bool > get_one_option(const struct my_option *opt, > - const char *argument, > - const char *filename __attribute__((unused))) > + const char *argument, > + const char *filename) indentation went wrong here. (only here. in other files where you removed the __attribute__ it was fine) > { > int orig_what_to_do= what_to_do; > + > + /* Track when protocol is set via CLI to not force overrides */ > + static my_bool ignore_protocol_override = FALSE; > + > DBUG_ENTER("get_one_option"); > > switch(opt->id) { > diff --git a/man/mysql.1 b/man/mysql.1 > index 03f23df3660..27a7e4d4d70 100644 > --- a/man/mysql.1 > +++ b/man/mysql.1 > @@ -1199,7 +1199,8 @@ Do not write line numbers for errors\&. Useful when you want to compare result f > \fB\-S \fR\fB\fIpath\fR\fR > .sp > For connections to > -localhost, the Unix socket file to use, or, on Windows, the name of the named pipe to use\&. > +localhost, the Unix socket file to use, or, on Windows, the name of the named pipe to use\&. > +Forces --protocol=socket when specified without other connection properties\&. here and everywhere, for -P and -S: I'd clarify that "... when specified on the command line ..." > .RE > .sp > .RS 4 > diff --git a/mysql-test/main/cli_options_force_protocol.result b/mysql-test/main/cli_options_force_protocol.result > new file mode 100644 > index 00000000000..c69a2b4f578 > --- /dev/null > +++ b/mysql-test/main/cli_options_force_protocol.result > @@ -0,0 +1,25 @@ > +# > +# MDEV-14974: --port ignored for --host=localhost > +# > +# > +# The following group of tests should produce no warnings > +# > +# exec MYSQL --host=localhost -e "status" 2>&1 | grep "Connection:\|WARNING:" > +Connection: Localhost via UNIX socket > +# exec MYSQL --port=MASTER_MYPORT --protocol=tcp -e "status" 2>&1 | grep "Connection:\|WARNING:" > +Connection: localhost via TCP/IP > +# exec MYSQL --host=localhost --port=MASTER_MYPORT --protocol=socket -e "status" 2>&1 | grep "Connection:\|WARNING:" > +Connection: Localhost via UNIX socket > +# exec MYSQL --host=127.0.0.1 --port=MASTER_MYPORT -e "status" 2>&1 | grep "Connection:\|WARNING:" > +Connection: 127.0.0.1 via TCP/IP > +# exec MYSQL --host=localhost --socket=MASTER_MYSOCK --port=MASTER_MYPORT -e "status" 2>&1 | grep "Connection:\|WARNING:" > +Connection: Localhost via UNIX socket > +# > +# The remaining tests should produce warnings > +# I now have some reservations about it, see below: > +# exec MYSQL --host=localhost --port=MASTER_MYPORT -e "status" 2>&1 | grep "Connection:\|WARNING:" > +WARNING: Forcing protocol to TCP due to option specification. Please explicitly state intended protocol. > +Connection: localhost via TCP/IP old behavior was "localhost via UNIX socket", you've changed it to TCP/IP and issued a warning. Good so far. > +# exec MYSQL --host=localhost --socket=MASTER_MYSOCK -e "status" 2>&1 | grep "Connection:\|WARNING:" > +WARNING: Forcing protocol to SOCKET due to option specification. Please explicitly state intended protocol. > +Connection: Localhost via UNIX socket here the behavior isn't changed, but you still issue a warning. Is it justified? may be it'd be better only to issue a warning when the behavior changes? Regards, Sergei

1 0

Re: [Maria-developers] 2a5663ae524: MDEV-14974: --port ignored for --host=localhost
by Sergei Golubchik 04 May '21

04 May '21

Hi, Brandon! On May 04, Brandon Nesterenko wrote: > revision-id: 2a5663ae524 (mariadb-10.6.0-18-g2a5663ae524) > parent(s): 9fe681c9e4d > author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > committer: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > timestamp: 2021-05-03 19:47:07 +0000 > message: > > MDEV-14974: --port ignored for --host=localhost > > Problem: > ======= > MariaDB's command line utilities (e.g., mysql, > mysqldump, etc) silently ignore connection > property options (e.g., --port and --socket) > when protocol is not explicitly set via the > command-line for localhost connections. > > Fix: > === > If connection properties are specified without a > protocol, override the protocol to be consistent. > For example, if --port is specified, automatically > set protocol=tcp. > > Caveats: > ======= > * When multiple connection properties are > specified, nothing is overridden > * If protocol is is set via the command-line, > its value is used > > diff --git a/client/client_priv.h b/client/client_priv.h > index 64818d2ab8d..94e1fc8bb08 100644 > --- a/client/client_priv.h > +++ b/client/client_priv.h > @@ -136,3 +136,57 @@ enum options_client > Name of the sys schema database. > */ > #define SYS_SCHEMA_DB_NAME "sys" > + > + > +/** > + Utility function to implicitly change the connection protocol to a > + consistent value given the command line arguments. Additionally, > + warns the user that the protocol has been changed. > + > + Arguments: > + @param [in] warn_to The file to write the warning to > + @param [in] host Name of the host to connect to > + @param [in, out] opt_protocol Location of the protocol option > + variable to update > + @param [in] new_protocol New protocol to force > +*/ > +static inline void warn_protocol_override(FILE *warn_to, it's always stderr, isn't it? Why did you want it as an argument? > + char *host, > + uint *opt_protocol, > + uint new_protocol) > +{ > + if ((host == NULL > + || strncmp(host, LOCAL_HOST, sizeof(LOCAL_HOST)-1) == 0)) > + { > + const char *TCP_NAME = "TCP"; > + const char *SOCKET_NAME = "SOCKET"; > + char *protocol_name; > + > + if(new_protocol == MYSQL_PROTOCOL_TCP) > + protocol_name = (char *) TCP_NAME; better not to hard-code protocol names here, you can do protocol_name= sql_protocol_typelib.type_names[new_protocol-1]; > + else if(new_protocol == MYSQL_PROTOCOL_SOCKET) > + protocol_name = (char *) SOCKET_NAME; > + else > + { > + /* > + This should never be entered, but just in case we are called incorrectly > + */ > + > + fprintf(warn_to, "%s %d %s %d\n", > + "WARNING: Protocol ID ", > + new_protocol, > + " cannot override connection type. " > + "Using the configuration value of ", > + *opt_protocol); > + return; for things that cannot happen code-wise (some code guarantees that it can never happen), better add an assert (DBUG_ASSERT, in fact) and there's no need to do an warning or anything. An assert is like a self-maintaining documentation. Its main purpose is to document that something cannot happen. But unlike your comment it also checks that it doesn't become outdated. so, this could become DBUG_ASSERT(new_protocol == MYSQL_PROTOCOL_SOCKET || new_protocol == MYSQL_PROTOCOL_TCP); protocol_name= sql_protocol_typelib.type_names[new_protocol-1]; > + } > + > + fprintf(warn_to, "%s %s %s\n", > + "WARNING: Forcing protocol to ", > + protocol_name, > + " due to option specification. " > + "Please explicitly state intended protocol."); > + > + *opt_protocol = new_protocol; > + } > +} > diff --git a/client/mysql.cc b/client/mysql.cc > index 433fbd281b9..5ca9b4393ec 100644 > --- a/client/mysql.cc > +++ b/client/mysql.cc > @@ -206,6 +206,8 @@ static uint opt_protocol=0; > static const char *opt_protocol_type= ""; > static CHARSET_INFO *charset_info= &my_charset_latin1; > > +static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; > + > #include "sslopt-vars.h" > > const char *default_dbug_option="d:t:o,/tmp/mariadb.trace"; > @@ -1162,6 +1164,9 @@ int main(int argc,char *argv[]) > close(stdout_fileno_copy); /* Clean up dup(). */ > } > > + /* We need to know if protocol-related options originate from CLI args */ > + my_defaults_mark_files = TRUE; > + > load_defaults_or_exit("my", load_default_groups, &argc, &argv); > defaults_argv=argv; > if ((status.exit_status= get_options(argc, (char **) argv))) > @@ -1171,6 +1176,14 @@ int main(int argc,char *argv[]) > exit(status.exit_status); > } > > + /* Command line options override configured protocol */ > + if (protocol_to_force > MYSQL_PROTOCOL_DEFAULT > + && protocol_to_force != opt_protocol) > + { > + warn_protocol_override(stderr, current_host, &opt_protocol, protocol_to_force); > + } > + > + > if (status.batch && !status.line_buff && > !(status.line_buff= batch_readline_init(MAX_BATCH_BUFFER_SIZE, stdin))) > { > @@ -1715,8 +1728,11 @@ static void usage(int version) > > > my_bool > -get_one_option(const struct my_option *opt, const char *argument, const char *) > +get_one_option(const struct my_option *opt, const char *argument, const char *filename) > { > + /* Track when protocol is set via CLI to not force port TCP protocol override */ > + static my_bool ignore_protocol_override = FALSE; > + > switch(opt->id) { > case OPT_CHARSETS_DIR: > strmake_buf(mysql_charsets_dir, argument); > @@ -1781,6 +1797,14 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > opt->name)) <= 0) > exit(1); > #endif > + > + /* Specification of protocol via CLI trumps implicit overrides */ > + if (filename[0] == '\0') > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > break; > case OPT_SERVER_ARG: > #ifdef EMBEDDED_LIBRARY > @@ -1872,6 +1896,13 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > #ifdef __WIN__ > opt_protocol = MYSQL_PROTOCOL_PIPE; > opt_protocol_type= "pipe"; > + > + /* Prioritize pipe if explicit via command line */ > + if (filename[0] == '\0') > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > #endif > break; > #include <sslopt-case.h> > @@ -1883,6 +1914,40 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > status.exit_status= 0; > mysql_end(-1); > break; > + case 'P': > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_SOCKET) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* If port is set via CLI, try to force protocol to TCP */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_TCP > protocol_to_force) that's a strange condition. I believe it's practically identical to protocol_to_force == MYSQL_PROTOCOL_DEFAULT but a lot more confusing. Why not to write it as protocol_to_force == MYSQL_PROTOCOL_DEFAULT ? > + { > + protocol_to_force = MYSQL_PROTOCOL_TCP; > + } > + break; > + case 'S': > +#ifndef __WIN__ > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_TCP) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* Prioritize socket if set via command line */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_SOCKET > protocol_to_force) same here > + { > + protocol_to_force = MYSQL_PROTOCOL_SOCKET; > + } > +#endif > + break; > case 'I': > case '?': > usage(0); > diff --git a/client/mysqladmin.cc b/client/mysqladmin.cc > index e40e82f8038..08ec14815bd 100644 > --- a/client/mysqladmin.cc > +++ b/client/mysqladmin.cc > @@ -54,6 +54,8 @@ static bool sql_log_bin_off= false; > static uint opt_protocol=0; > static myf error_flags; /* flags to pass to my_printf_error, like ME_BELL */ > > +static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; > + > /* > When using extended-status relatively, ex_val_max_len is the estimated > maximum length for any relative value printed by extended-status. The > @@ -241,8 +243,12 @@ static const char *load_default_groups[]= > 0 }; > > my_bool > -get_one_option(const struct my_option *opt, const char *argument, const char *) > +get_one_option(const struct my_option *opt, const char *argument, const char *filename) > { > + > + /* Track when protocol is set via CLI to not force overrides */ > + static my_bool ignore_protocol_override = FALSE; > + > switch(opt->id) { > case 'c': > opt_count_iterations= 1; > @@ -274,6 +280,13 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > case 'W': > #ifdef __WIN__ > opt_protocol = MYSQL_PROTOCOL_PIPE; > + > + /* Prioritize pipe if explicit via command line */ > + if (filename[0] == '\0') > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > #endif > break; > case '#': > @@ -309,6 +322,48 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > sf_leaking_memory= 1; /* no memory leak reports here */ > exit(1); > } > + > + /* Specification of protocol via CLI trumps implicit overrides */ > + if (filename[0] == '\0') > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + break; > + case 'P': > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_SOCKET) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* If port is set via CLI, try to force protocol to TCP */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_TCP > protocol_to_force) > + { > + protocol_to_force = MYSQL_PROTOCOL_TCP; > + } > + break; > + case 'S': > +#ifndef __WIN__ > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_TCP) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* Prioritize socket if set via command line */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_SOCKET > protocol_to_force) > + { > + protocol_to_force = MYSQL_PROTOCOL_SOCKET; > + } > +#endif > break; > } > return 0; > @@ -323,6 +378,10 @@ int main(int argc,char *argv[]) > > MY_INIT(argv[0]); > sf_leaking_memory=1; /* don't report memory leaks on early exits */ > + > + /* We need to know if protocol-related options originate from CLI args */ > + my_defaults_mark_files = TRUE; > + > load_defaults_or_exit("my", load_default_groups, &argc, &argv); > save_argv = argv; /* Save for free_defaults */ > > @@ -331,6 +390,13 @@ int main(int argc,char *argv[]) > temp_argv= mask_password(argc, &argv); > temp_argc= argc; > > + /* Command line options override configured protocol */ > + if (protocol_to_force > MYSQL_PROTOCOL_DEFAULT > + && protocol_to_force != opt_protocol) > + { > + warn_protocol_override(stderr, host, &opt_protocol, protocol_to_force); > + } > + > if (debug_info_flag) > my_end_arg= MY_CHECK_ERROR | MY_GIVE_INFO; > if (debug_check_flag) > diff --git a/client/mysqlbinlog.cc b/client/mysqlbinlog.cc > index fd31ab6694e..1ff6048e5ad 100644 > --- a/client/mysqlbinlog.cc > +++ b/client/mysqlbinlog.cc > @@ -98,6 +98,8 @@ static const char *output_prefix= ""; > static char **defaults_argv= 0; > static MEM_ROOT glob_root; > > +static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; > + > #ifndef DBUG_OFF > static const char *default_dbug_option = "d:t:o,/tmp/mariadb-binlog.trace"; > const char *current_dbug_option= default_dbug_option; > @@ -1959,9 +1961,13 @@ static my_time_t convert_str_to_timestamp(const char* str) > > > extern "C" my_bool > -get_one_option(const struct my_option *opt, const char *argument, const char *) > +get_one_option(const struct my_option *opt, const char *argument, const char *filename) > { > bool tty_password=0; > + > + /* Track when protocol is set via CLI to not force overrides */ > + static my_bool ignore_protocol_override = FALSE; > + > switch (opt->id) { > #ifndef DBUG_OFF > case '#': > @@ -2011,6 +2017,14 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > sf_leaking_memory= 1; /* no memory leak reports here */ > die(); > } > + > + /* Specification of protocol via CLI trumps implicit overrides */ > + if (filename[0] == '\0') > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > break; > #ifdef WHEN_FLASHBACK_REVIEW_READY > case opt_flashback_review: > @@ -2092,6 +2106,40 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > case OPT_PRINT_ROW_EVENT_POSITIONS: > print_row_event_positions_used= 1; > break; > + case 'P': > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_SOCKET) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* If port is set via CLI, try to force protocol to TCP */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_TCP > protocol_to_force) > + { > + protocol_to_force = MYSQL_PROTOCOL_TCP; > + } > + break; > + case 'S': > +#ifndef __WIN__ > + /* If port and socket are set, fall back to default behavior */ > + if (protocol_to_force == MYSQL_PROTOCOL_TCP) > + { > + ignore_protocol_override = TRUE; > + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; > + } > + > + /* Prioritize socket if set via command line */ > + if (filename[0] == '\0' && > + !ignore_protocol_override && > + MYSQL_PROTOCOL_SOCKET > protocol_to_force) > + { > + protocol_to_force = MYSQL_PROTOCOL_SOCKET; > + } > +#endif > + break; > case 'v': > if (argument == disabled_my_option) > verbose= 0; > @@ -3049,6 +3097,9 @@ int main(int argc, char** argv) > my_init_time(); // for time functions > tzset(); // set tzname > > + /* We need to know if protocol-related options originate from CLI args */ > + my_defaults_mark_files = TRUE; > + > load_defaults_or_exit("my", load_groups, &argc, &argv); > defaults_argv= argv; > > @@ -3062,6 +3113,13 @@ int main(int argc, char** argv) > > parse_args(&argc, (char***)&argv); > > + /* Command line options override configured protocol */ > + if (protocol_to_force > MYSQL_PROTOCOL_DEFAULT > + && protocol_to_force != opt_protocol) > + { > + warn_protocol_override(stderr, host, &opt_protocol, protocol_to_force); > + } > + > if (!argc || opt_version) > { > if (!opt_version) > diff --git a/client/mysqlcheck.c b/client/mysqlcheck.c > index fb3103a318d..54f8d2eb4f3 100644 > --- a/client/mysqlcheck.c > +++ b/client/mysqlcheck.c > @@ -56,6 +56,8 @@ static char *opt_skip_database; > DYNAMIC_ARRAY tables4repair, tables4rebuild, alter_table_cmds; > DYNAMIC_ARRAY views4repair; > static uint opt_protocol=0; > + > +static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; > > enum operations { DO_CHECK=1, DO_REPAIR, DO_ANALYZE, DO_OPTIMIZE, DO_FIX_NAMES }; > const char *operation_name[]= > @@ -289,6 +291,10 @@ get_one_option(const struct my_option *opt, > const char *filename __attribute__((unused))) not __attribute__((unused)) anymore here and in other files too, I won't repeat this comment for every file > { > int orig_what_to_do= what_to_do; > + > + /* Track when protocol is set via CLI to not force overrides */ > + static my_bool ignore_protocol_override = FALSE; > + > DBUG_ENTER("get_one_option"); > > switch(opt->id) { > diff --git a/mysql-test/main/cli_options_force_protocol.result b/mysql-test/main/cli_options_force_protocol.result > --- /dev/null > +++ b/mysql-test/main/cli_options_force_protocol.result > @@ -0,0 +1,27 @@ > +# > +# MDEV-14974: --port ignored for --host=localhost > +# > +# > +# The following tests until the first cat_file should produce no warnings > +# > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost -e "status" | grep "Connection:" oh-oh. You cannot hard-code your local path into the result file. Not the port number either. That's why in my previous review I wrote --echo # --host=localhost --port=MASTER_MYPORT and not --echo # $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" (actually I had --port=$MASTER_MYPORT in my email, but that was wrong, should be no $-sign in --echo :) > +Connection: Localhost via UNIX socket > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --port=16000 --protocol=tcp -e "status" | grep "Connection:" > +Connection: localhost via TCP/IP > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --port=16000 --protocol=socket -e "status" | grep "Connection:" > +Connection: Localhost via UNIX socket > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=127.0.0.1 --port=16000 -e "status" | grep "Connection:" > +Connection: 127.0.0.1 via TCP/IP > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --socket=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/tmp/mysqld.1.sock --port=16000 -e "status" | grep "Connection:" > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.cli_options_force_protocol > +# > +# The remaining tests should produce warnings > +# > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --port=16000 -e "status" | grep "Connection:" > +Connection: localhost via TCP/IP > +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --socket=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/tmp/mysqld.1.sock -e "status" | grep "Connection:" > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.cli_options_force_protocol > +WARNING: Forcing protocol to TCP due to option specification. Please explicitly state intended protocol. > +WARNING: Forcing protocol to SOCKET due to option specification. Please explicitly state intended protocol. > diff --git a/mysql-test/main/cli_options_force_protocol.test b/mysql-test/main/cli_options_force_protocol.test > new file mode 100644 > index 00000000000..f91d7833a0d > --- /dev/null > +++ b/mysql-test/main/cli_options_force_protocol.test > @@ -0,0 +1,41 @@ > +--echo # > +--echo # MDEV-14974: --port ignored for --host=localhost > +--echo # > + > +--source include/not_embedded.inc > +--source include/not_windows.inc > + > +--echo # > +--echo # The following tests until the first cat_file should produce no warnings > +--echo # > + > +--echo # $MYSQL --host=localhost -e "status" | grep "Connection:" > +--exec $MYSQL --host=localhost -e "status" | grep "Connection:" > + > +--echo # $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:" > +--exec $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:" > + > +--echo # $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:" > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:" > + > +--echo # $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--exec $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:" > + > +--echo # $MYSQL --host=localhost --socket=$MASTER_MYSOCK --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--exec $MYSQL --host=localhost --socket=$MASTER_MYSOCK --port=$MASTER_MYPORT -e "status" | grep "Connection:" > + > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > + > +--echo # > +--echo # The remaining tests should produce warnings > +--echo # > + > +--echo # $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" > + > +--echo # $MYSQL --host=localhost --socket=$MASTER_MYSOCK -e "status" | grep "Connection:" > +--exec $MYSQL --host=localhost --socket=$MASTER_MYSOCK -e "status" | grep "Connection:" > + > +--cat_file $MYSQLTEST_VARDIR/log/current_test I still don't understand why you include current test name into the result file. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 242cc3fc41f: MDEV-14974: --port ignored for --host=localhost
by Sergei Golubchik 03 May '21

03 May '21

Hi, Brandon! On May 03, Brandon Nesterenko wrote: > revision-id: 242cc3fc41f (mariadb-10.6.0-17-g242cc3fc41f) > parent(s): fd8c68c7fe6 > author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > committer: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > timestamp: 2021-04-30 23:17:37 +0000 > message: > > MDEV-14974: --port ignored for --host=localhost > > Problem: > ======= > MariaDB's command line utilities (e.g., mysql, mysqldump, etc) silently ignore the --port option if no host is given or it is localhost. please, try to avoid very long lines in commit comments, they're difficult to read in git console tools. > > Fix: > === > During configuration processing, force protocol to TCP if port was specified via the command line. However, if protocol is additionally specified via the command line, it will be prioritized. > > diff --git a/client/client_priv.h b/client/client_priv.h > index 64818d2ab8d..6cb1f02ba33 100644 > --- a/client/client_priv.h > +++ b/client/client_priv.h > @@ -136,3 +136,85 @@ enum options_client > Name of the sys schema database. > */ > #define SYS_SCHEMA_DB_NAME "sys" > + > + > + > +/** > + Changes flags to prepare for (and revert to the previous state) > + from handle_options. > + > + When creating the state, the original state is saved and can be later > + resumed by calling this function and setting do_revert_values to TRUE. > + > + Flags: > + my_defaults_mark_files: propagates the source of a set option > + > + @param [in] do_revert_values behavior should revert when TRUE, prepare > + when FALSE > + */ > +static inline void set_flags_for_option_handling(my_bool do_revert_values) > +{ > + static my_bool prev_mark_files; > + > + if (do_revert_values) > + { > + my_defaults_mark_files = prev_mark_files; > + } > + else > + { > + prev_mark_files = my_defaults_mark_files; > + my_defaults_mark_files = TRUE; > + } > +} this isn't very helpful. my_defaults_mark_files is part of the my_getopt API. It's supposed to be used directly by whatever tool uses my_getopt. Here you've turned one line my_defaults_mark_files= TRUE; into 27 lines, so when I (or somebody reading the code) will see set_flags_for_option_handling(...); he'd have to go to the function definition, read the comment and the function, all 27 lines, understand what they do, and then jump back. I'd rather prefer to read one line with a very clear semantics and no conditionals instead. > + > +/** > + Helper function to prepare state for handle_options (current state is saved) > + */ > +static inline void prepare_option_handling_flags() > +{ > + set_flags_for_option_handling(FALSE); > +} > + > +/** > + Helper function to revert state after handle_options to that from before > + prepare_option_handling_flags was called > + */ > +static inline void revert_option_handling_flags() > +{ > + set_flags_for_option_handling(TRUE); > +} and it keeps going on. if you remove the first function, you won't need these two functions either. Saving old value, restoring it. my_defaults_mark_files affects only my_load_defaults(). It makes no sense to restore it. > +/** > + > + Utility function to force the connection protocol to TCP when > + just the port is specified via the command line. Additionally, > + warns the user that the protocol has been changed (MDEV-14974). > + > + Notes: > + 1) This only takes effect when connecting to localhost > + 2) Windows uses TCP by default > + > + Arguments: > + @param [in] warn_to The file to write the warning to > + @param [in] host Name of the host to connect to > + @param [in, out] protocol_loc Location of the protocol option > + variable to update > +*/ > +static inline void warn_tcp_protocol_override(FILE *warn_to, > + char *host, > + uint *protocol_loc) > +{ > +#ifndef _WIN32 > + if ((host == NULL > + || strncmp(host, LOCAL_HOST, sizeof(LOCAL_HOST)-1) == 0)) > + { > + fprintf(warn_to, "WARNING: " > + "Forcing protocol to TCP due to port specification. " > + "Please explicitly state TCP protocol or remove " > + "port if unintended.\n"); > + *protocol_loc = MYSQL_PROTOCOL_TCP; > + } > +#endif > +} > diff --git a/client/mysql.cc b/client/mysql.cc > index 433fbd281b9..980daa14365 100644 > --- a/client/mysql.cc > +++ b/client/mysql.cc > @@ -206,6 +206,10 @@ static uint opt_protocol=0; > static const char *opt_protocol_type= ""; > static CHARSET_INFO *charset_info= &my_charset_latin1; > > +#ifndef _WIN32 > +static my_bool port_forcing_tcp_proto = FALSE; > +#endif > + > #include "sslopt-vars.h" > > const char *default_dbug_option="d:t:o,/tmp/mariadb.trace"; > @@ -1162,6 +1166,8 @@ int main(int argc,char *argv[]) > close(stdout_fileno_copy); /* Clean up dup(). */ > } > > + prepare_option_handling_flags(); Just do my_defaults_mark_files= TRUE; here > load_defaults_or_exit("my", load_default_groups, &argc, &argv); > defaults_argv=argv; > if ((status.exit_status= get_options(argc, (char **) argv))) > @@ -1171,6 +1177,14 @@ int main(int argc,char *argv[]) > exit(status.exit_status); > } > > + revert_option_handling_flags(); and don't revert it. > + if (port_forcing_tcp_proto) > + { > + warn_tcp_protocol_override(stderr, current_host, &opt_protocol); I'm not sure it needs a warning. But I don't have strong arguments against the warning either, so let's keep a warning, if you like it that way. Why everything is not on _WIN32 ? It has named pipes and tcp. Wouldn't it have the same issue with --port? > + } > + > + > if (status.batch && !status.line_buff && > !(status.line_buff= batch_readline_init(MAX_BATCH_BUFFER_SIZE, stdin))) > { > @@ -1715,8 +1729,11 @@ static void usage(int version) > > > my_bool > -get_one_option(const struct my_option *opt, const char *argument, const char *) > +get_one_option(const struct my_option *opt, const char *argument, const char *filename) > { > + /* Track when protocol is set via CLI to not force port TCP protocol override */ > + static my_bool ignore_port_override = FALSE; > + > switch(opt->id) { > case OPT_CHARSETS_DIR: > strmake_buf(mysql_charsets_dir, argument); > @@ -1780,6 +1797,19 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > else if ((opt_protocol= find_type_with_warning(argument, &sql_protocol_typelib, > opt->name)) <= 0) > exit(1); > +#ifndef _WIN32 > + /* MDEV-14974 > + * > + * Where specifying port implicitly will set the protocol to use TCP, if the > + * protocol is explicitly set after the port, prioritize protocol. > + */ please, see how multi-comments are usually formatted, and follow the same style. Generally there's no need to specify the MDEV, and there's no vertical line of asterisks. > + if(filename[0] == '\0') > + { > + /* Protocol is specified via command line */ > + ignore_port_override = TRUE; > + port_forcing_tcp_proto = FALSE; > + } > +#endif > #endif > break; > case OPT_SERVER_ARG: > @@ -1883,6 +1913,20 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > status.exit_status= 0; > mysql_end(-1); > break; > + case 'P': > +#ifndef _WIN32 > + /* MDEV-14974: > + * > + * If port is set via CLI, change protocol to TCP unless protocol has already > + * been specified via CLI or TCP is already used > + */ > + if (filename[0] == '\0' && !ignore_port_override > + && opt_protocol != MYSQL_PROTOCOL_TCP) > + { > + port_forcing_tcp_proto = TRUE; > + } let's also do the same treatment for --socket. If it's specified on the command line - force the socket protocol. What could be the least surprising behavior? I'd think the most intuitive would be, like, WYSIWIG thing - * if port is specified on the command line (but no socket or protocol) - it forces protocol=TCP * if socket is specified on the command line (but no port of protocol) - it forces protocol=SOCKET * if protocol is specified explicitly anywhere on the command line - port and socket lose their magic behavior * if there's no protocol, but both socket and port are specified? - I don't know, perhaps, let's keep the old behavior, no magic? hmm. How does --host affect that? > +#endif > + break; > case 'I': > case '?': > usage(0); > @@ -1916,8 +1960,10 @@ static int get_options(int argc, char **argv) > opt_max_allowed_packet= *mysql_params->p_max_allowed_packet; > opt_net_buffer_length= *mysql_params->p_net_buffer_length; > > + my_defaults_mark_files = TRUE; > if ((ho_error=handle_options(&argc, &argv, my_long_options, get_one_option))) > return(ho_error); > + my_defaults_mark_files = FALSE; my_defaults_mark_files has no effect on handle_options() > *mysql_params->p_max_allowed_packet= opt_max_allowed_packet; > *mysql_params->p_net_buffer_length= opt_net_buffer_length; > > diff --git a/mysql-test/main/port_force_tcp.result b/mysql-test/main/port_force_tcp.result > new file mode 100644 > index 00000000000..ae976bda6cc > --- /dev/null > +++ b/mysql-test/main/port_force_tcp.result > @@ -0,0 +1,14 @@ > +# > +# MDEV-14974: --port ignored for --host=localhost > +# > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.port_force_tcp > +Connection: localhost via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.port_force_tcp > +Connection: 127.0.0.1 via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +Connection: localhost via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +WARNING: Forcing protocol to TCP due to port specification. Please explicitly state TCP protocol or remove port if unintended. > diff --git a/mysql-test/main/port_force_tcp.test b/mysql-test/main/port_force_tcp.test > new file mode 100644 > index 00000000000..607670d16b0 > --- /dev/null > +++ b/mysql-test/main/port_force_tcp.test > @@ -0,0 +1,20 @@ > +--echo # > +--echo # MDEV-14974: --port ignored for --host=localhost > +--echo # > + > +--source include/not_embedded.inc > + > +--exec $MYSQL --host=localhost -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test this will likely fail on windows, you need to source not_windows.inc too. why do you cat current_test after every --exec? better add --echo before every exec, like --echo # --host=localhost --port=$MASTER_MYPORT --exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 242cc3fc41f: MDEV-14974: --port ignored for --host=localhost
by Sergei Golubchik 03 May '21

03 May '21

Hi, Brandon! On May 03, Brandon Nesterenko wrote: > revision-id: 242cc3fc41f (mariadb-10.6.0-17-g242cc3fc41f) > parent(s): fd8c68c7fe6 > author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > committer: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > timestamp: 2021-04-30 23:17:37 +0000 > message: > > MDEV-14974: --port ignored for --host=localhost > > Problem: > ======= > MariaDB's command line utilities (e.g., mysql, mysqldump, etc) silently ignore the --port option if no host is given or it is localhost. please, try to avoid very long lines in commit comments, they're difficult to read in git console tools. > > Fix: > === > During configuration processing, force protocol to TCP if port was specified via the command line. However, if protocol is additionally specified via the command line, it will be prioritized. > > diff --git a/client/client_priv.h b/client/client_priv.h > index 64818d2ab8d..6cb1f02ba33 100644 > --- a/client/client_priv.h > +++ b/client/client_priv.h > @@ -136,3 +136,85 @@ enum options_client > Name of the sys schema database. > */ > #define SYS_SCHEMA_DB_NAME "sys" > + > + > + > +/** > + Changes flags to prepare for (and revert to the previous state) > + from handle_options. > + > + When creating the state, the original state is saved and can be later > + resumed by calling this function and setting do_revert_values to TRUE. > + > + Flags: > + my_defaults_mark_files: propagates the source of a set option > + > + @param [in] do_revert_values behavior should revert when TRUE, prepare > + when FALSE > + */ > +static inline void set_flags_for_option_handling(my_bool do_revert_values) > +{ > + static my_bool prev_mark_files; > + > + if (do_revert_values) > + { > + my_defaults_mark_files = prev_mark_files; > + } > + else > + { > + prev_mark_files = my_defaults_mark_files; > + my_defaults_mark_files = TRUE; > + } > +} this isn't very helpful. my_defaults_mark_files is part of the my_getopt API. It's supposed to be used directly by whatever tool uses my_getopt. Here you've turned one line my_defaults_mark_files= TRUE; into 27 lines, so when I (or somebody reading the code) will see set_flags_for_option_handling(...); he'd have to go to the function definition, read the comment and the function, all 27 lines, understand what they do, and then jump back. I'd rather prefer to read one line with a very clear semantics and no conditionals instead. > + > +/** > + Helper function to prepare state for handle_options (current state is saved) > + */ > +static inline void prepare_option_handling_flags() > +{ > + set_flags_for_option_handling(FALSE); > +} > + > +/** > + Helper function to revert state after handle_options to that from before > + prepare_option_handling_flags was called > + */ > +static inline void revert_option_handling_flags() > +{ > + set_flags_for_option_handling(TRUE); > +} and it keeps going on. if you remove the first function, you won't need these two functions either. Saving old value, restoring it. my_defaults_mark_files affects only my_load_defaults(). It makes no sense to restore it. > +/** > + > + Utility function to force the connection protocol to TCP when > + just the port is specified via the command line. Additionally, > + warns the user that the protocol has been changed (MDEV-14974). > + > + Notes: > + 1) This only takes effect when connecting to localhost > + 2) Windows uses TCP by default > + > + Arguments: > + @param [in] warn_to The file to write the warning to > + @param [in] host Name of the host to connect to > + @param [in, out] protocol_loc Location of the protocol option > + variable to update > +*/ > +static inline void warn_tcp_protocol_override(FILE *warn_to, > + char *host, > + uint *protocol_loc) > +{ > +#ifndef _WIN32 > + if ((host == NULL > + || strncmp(host, LOCAL_HOST, sizeof(LOCAL_HOST)-1) == 0)) > + { > + fprintf(warn_to, "WARNING: " > + "Forcing protocol to TCP due to port specification. " > + "Please explicitly state TCP protocol or remove " > + "port if unintended.\n"); > + *protocol_loc = MYSQL_PROTOCOL_TCP; > + } > +#endif > +} > diff --git a/client/mysql.cc b/client/mysql.cc > index 433fbd281b9..980daa14365 100644 > --- a/client/mysql.cc > +++ b/client/mysql.cc > @@ -206,6 +206,10 @@ static uint opt_protocol=0; > static const char *opt_protocol_type= ""; > static CHARSET_INFO *charset_info= &my_charset_latin1; > > +#ifndef _WIN32 > +static my_bool port_forcing_tcp_proto = FALSE; > +#endif > + > #include "sslopt-vars.h" > > const char *default_dbug_option="d:t:o,/tmp/mariadb.trace"; > @@ -1162,6 +1166,8 @@ int main(int argc,char *argv[]) > close(stdout_fileno_copy); /* Clean up dup(). */ > } > > + prepare_option_handling_flags(); Just do my_defaults_mark_files= TRUE; here > load_defaults_or_exit("my", load_default_groups, &argc, &argv); > defaults_argv=argv; > if ((status.exit_status= get_options(argc, (char **) argv))) > @@ -1171,6 +1177,14 @@ int main(int argc,char *argv[]) > exit(status.exit_status); > } > > + revert_option_handling_flags(); and don't revert it. > + if (port_forcing_tcp_proto) > + { > + warn_tcp_protocol_override(stderr, current_host, &opt_protocol); I'm not sure it needs a warning. But I don't have strong arguments against the warning either, so let's keep a warning, if you like it that way. Why everything is not on _WIN32 ? It has named pipes and tcp. Wouldn't it have the same issue with --port? > + } > + > + > if (status.batch && !status.line_buff && > !(status.line_buff= batch_readline_init(MAX_BATCH_BUFFER_SIZE, stdin))) > { > @@ -1715,8 +1729,11 @@ static void usage(int version) > > > my_bool > -get_one_option(const struct my_option *opt, const char *argument, const char *) > +get_one_option(const struct my_option *opt, const char *argument, const char *filename) > { > + /* Track when protocol is set via CLI to not force port TCP protocol override */ > + static my_bool ignore_port_override = FALSE; > + > switch(opt->id) { > case OPT_CHARSETS_DIR: > strmake_buf(mysql_charsets_dir, argument); > @@ -1780,6 +1797,19 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > else if ((opt_protocol= find_type_with_warning(argument, &sql_protocol_typelib, > opt->name)) <= 0) > exit(1); > +#ifndef _WIN32 > + /* MDEV-14974 > + * > + * Where specifying port implicitly will set the protocol to use TCP, if the > + * protocol is explicitly set after the port, prioritize protocol. > + */ please, see how multi-comments are usually formatted, and follow the same style. Generally there's no need to specify the MDEV, and there's no vertical line of asterisks. > + if(filename[0] == '\0') > + { > + /* Protocol is specified via command line */ > + ignore_port_override = TRUE; > + port_forcing_tcp_proto = FALSE; > + } > +#endif > #endif > break; > case OPT_SERVER_ARG: > @@ -1883,6 +1913,20 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) > status.exit_status= 0; > mysql_end(-1); > break; > + case 'P': > +#ifndef _WIN32 > + /* MDEV-14974: > + * > + * If port is set via CLI, change protocol to TCP unless protocol has already > + * been specified via CLI or TCP is already used > + */ > + if (filename[0] == '\0' && !ignore_port_override > + && opt_protocol != MYSQL_PROTOCOL_TCP) > + { > + port_forcing_tcp_proto = TRUE; > + } let's also do the same treatment for --socket. If it's specified on the command line - force the socket protocol. What could be the least surprising behavior? I'd think the most intuitive would be, like, WYSIWIG thing - * if port is specified on the command line (but no socket or protocol) - it forces protocol=TCP * if socket is specified on the command line (but no port of protocol) - it forces protocol=SOCKET * if protocol is specified explicitly anywhere on the command line - port and socket lose their magic behavior * if there's no protocol, but both socket and port are specified? - I don't know, perhaps, let's keep the old behavior, no magic? hmm. How does --host affect that? > +#endif > + break; > case 'I': > case '?': > usage(0); > @@ -1916,8 +1960,10 @@ static int get_options(int argc, char **argv) > opt_max_allowed_packet= *mysql_params->p_max_allowed_packet; > opt_net_buffer_length= *mysql_params->p_net_buffer_length; > > + my_defaults_mark_files = TRUE; > if ((ho_error=handle_options(&argc, &argv, my_long_options, get_one_option))) > return(ho_error); > + my_defaults_mark_files = FALSE; my_defaults_mark_files has no effect on handle_options() > *mysql_params->p_max_allowed_packet= opt_max_allowed_packet; > *mysql_params->p_net_buffer_length= opt_net_buffer_length; > > diff --git a/mysql-test/main/port_force_tcp.result b/mysql-test/main/port_force_tcp.result > new file mode 100644 > index 00000000000..ae976bda6cc > --- /dev/null > +++ b/mysql-test/main/port_force_tcp.result > @@ -0,0 +1,14 @@ > +# > +# MDEV-14974: --port ignored for --host=localhost > +# > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.port_force_tcp > +Connection: localhost via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +Connection: Localhost via UNIX socket > +CURRENT_TEST: main.port_force_tcp > +Connection: 127.0.0.1 via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +Connection: localhost via TCP/IP > +CURRENT_TEST: main.port_force_tcp > +WARNING: Forcing protocol to TCP due to port specification. Please explicitly state TCP protocol or remove port if unintended. > diff --git a/mysql-test/main/port_force_tcp.test b/mysql-test/main/port_force_tcp.test > new file mode 100644 > index 00000000000..607670d16b0 > --- /dev/null > +++ b/mysql-test/main/port_force_tcp.test > @@ -0,0 +1,20 @@ > +--echo # > +--echo # MDEV-14974: --port ignored for --host=localhost > +--echo # > + > +--source include/not_embedded.inc > + > +--exec $MYSQL --host=localhost -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test > + > +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" > +--cat_file $MYSQLTEST_VARDIR/log/current_test this will likely fail on windows, you need to source not_windows.inc too. why do you cat current_test after every --exec? better add --echo before every exec, like --echo # --host=localhost --port=$MASTER_MYPORT --exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0