maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> committer: Brandon Nesterenko <brandon.nesterenko@xxxxxxxxxxx>
> 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