MDEV-21465: expose bind-address parameter via cmd options in client

[yvef]

• The Jira issue number for this PR is: MDEV-21465

Description

1. The PR is intended to provide the command line parameter to use existing functionality of address binding for outbound connections in mariadb client.
2. __
3. No side-effects.

How can this PR be tested?

TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on "Writing good test cases".

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

Basing the PR against the correct MariaDB version

• This is a new feature and the PR is based against the latest MariaDB development branch.
• This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

• I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
• For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

[CLAassistant]

All committers have signed the CLA.

[grooverdan]

Thanks for the contribution.

Can you also update man/mariadb.1 to include the option?

While the MDEV did include only the mysql/mariadb as the client, there are other clients that could be handled the same way.

In client:

• mysqladmin.cc
• mysqlbinlog.cc
• mysqlcheck.c
• mysqldump.c
• mysqlimport.c
• mysqlshow.c
• mysqlslap.c
• mysqltest.cc

And extra/mariabackup/backup_mysql.cc.

If you feel particular keen, and I'd appreciate it (though obviously optional), consolidate the connection methods across the client into a separate file like sql-common/connect.c and include in CMakeLists.txt like:

MYSQL_ADD_EXECUTABLE(mariadb-dump mysqldump.c ../sql-common/my_user.c ../sql-common/connect.c)

[yvef]

If you feel particular keen, and I'd appreciate it (though obviously optional), consolidate the connection methods across the client into a separate file like sql-common/connect.c and include in CMakeLists.txt like:

Sure, It's might be important point of refactoring and I would appreciate some hints on this. So, as far as I can see, all clients (except mariadb-backup) use the pvio plugin from connector. Once the functionality is implemented there, it is already spread across clients. But only in backup util I had to do it separately. Maybe it worth doing this the same way as it is done in other clients?

[grooverdan]

mysqlcheck.cc:dbConnect
mysql.cc:do_connect
backup_mysql.cc:xb_mysql_connect

And the other functions you've added to bind to are effectively the same function.

They have a program name (which would appreciate an update to the latest version ('mysql' -> 'mariadb', mysql_dump -> mariadb-dump etc).

So an interfaces of:

bool client_connect(MYSQL *mysql, const char *clientname, const char *host, const char *user,
                          const char *password, const char *database, const char *bind_addr, ulong flags)
{

   mysql_options(&mysql_connectio ....

...
  return  mysql_real_connect(&mysql_connection,host,user,passwd,
                            NULL,opt_mysql_port,opt_mysql_unix_port, 0))


}

Callers would do their:

 mysql_init()

any items specific to that client.
ret = client_connect(..... )

any post error handling or post connection options.

[yvef]

While hiding the common infrastructure for the all clients connection is a good idea, I didn't see any meaningful reason to only use this parameters as it is ( + bind_address). As long as the code use the same own general variables, the extensibility such method is not the pleasant copy-pasting. So, I suggest using the shared options pack to distribute the shared connection method. I had to make some number of changes, that's why I marked the pr as draft.

[grooverdan]

Very well done. This is so much neater.

Small changes:

• rebased and changed a few trivial conflicts where get_tty_password -> my_get_tty_password was renamed.
• Added a couple of missing includes in libmysqld/examples/ for an embedded build.
• squashed commits up and the commit message

Otherwise looks perfect.

I was concerned at one point there was missing my_free(cl_opts.bind_address); in mysql.cc:mysql_end, mysqlbinlog.cc:cleanup, mysqldump.cc:free_resources, and also the inconsistent my_free(cl_options.{user,password)), but it all seems correct to match the strdups that apply slightly differently in each function.

[grooverdan]

@yvef how are the defined meant to work for the Windows build - its not currently.

[vuvova]

Note that this is for 11.4, not 11.3.
And better wait for 11.3.1 release, when commits c8d903e and c275dc4 will be pushed, as your commit will not apply cleanly after them, there will be merge conflicts

[yvef]

@grooverdan your are right. that was fixed and included into the main commit.

[LinuxJedi]

Excellent cleanup here too, approved for 11.4

[LinuxJedi]

Good catch on the duplicate there too :)

[grooverdan]

Thank you @yvef

[vuvova]

0. I don't see it in 11.4 branch, how was it "merged"?
1. refactoring and a feature must be in different commits
2. sql-common is for the code common between a client and a server (thus the name). do_client_connect is client only, should be in client/
3. could've unified more code, like command-line options could've been all defined in one place, all my_free() calls too. (optional)
4. see comments in the code too
5. sslopt* changes are way too complex and redundant, just keep the files are they were, only with changes like

-      opt_ssl_crl= NULL;
+      cl_opts.opt_ssl_crl= NULL;

[vuvova]

So, it's not merged, needs many changes, way past the 11.4 preview deadline.
I'll try to fix it, but might not have enough time for that. Thus it likely won't be in 11.4 despite the confusing status.