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.nestere...@mariadb.com>
> committer: Brandon Nesterenko <brandon.nestere...@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
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
