maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #12692
Re: 2a5663ae524: MDEV-14974: --port ignored for --host=localhost
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@xxxxxxxxxxx>
> committer: Brandon Nesterenko <brandon.nesterenko@xxxxxxxxxxx>
> 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@xxxxxxxxxxx
