Re: [Maria-developers] 2bceae199bb: MDEV-14974: --port ignored for --host=localhost

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 committer: Brandon Nesterenko 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