Re: bk commit into 5.0 tree (tnurnberg:1.2476) BUG#15327

Tatjana,

> ChangeSet@1.2476, 2007-08-08 17:34:55+02:00, tnurnberg@sin.intern.azundris.com +16 -0
> Bug #15327: configure: --with-tcp-port option being partially ignored
>
> initialize default tcp port for client and server, like so:
> - if user configured --with-tcp-port, use that value as default
> - other assume "use a good default": search mysqld/tcp in
> /etc/services; if that doesn't exist, use factory default (3306)
> - environment variable MYSQL_TCP_PORT overrides this default
> - command-line option overrides all of the above

OK, I think this is good.

<cut>

> configure.in@1.452, 2007-08-08 17:34:47+02:00, tnurnberg@sin.intern.azundris.com +28 -1
> Bug #15327: configure: --with-tcp-port option being partially ignored
>
> If MYSQL_TCP_PORT defaulted in configure (factory default 3306
> at the time of this writing), set MYSQL_TCP_PORT to factory
> default, then clear factory default after. That way, we lose no
> information, and we can distinguish between "defaulted" and the
> pathological case "builder specifically configured a port that
> coincides with factory default." This can in theory happen if
> builder configures and builds several servers from a script
> (--with-tcp-port=3306, --with-tcp-port=3316, --with-tcp-port=3326).
> Not all that probably, but much preferable to having more "magic"
> happen in the server when we can solve this without any guesswork.

I find this comment (and the matching one in configure.in itself) confusing, either due to grammar or too many words or something.

Something like this might be sufficient:

"Distinguish between --with-tcp-port=3306 and no --with-tcp-port option. mysqld and clients ignore /etc/services if --with-tcp-port was specified at compile time."

Do you need help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

> include/mysql_version.h.in@1.20, 2007-08-08 17:34:49+02:00, tnurnberg@sin.intern.azundris.com +1 -0
> Bug #15327: configure: --with-tcp-port option being partially ignored
>
> make factory default for TCP port available to C source

This seems like an odd place to put this. Since it's really used as a flag (if MYSQL_TCP_PORT_DEFAULT == 0), I'd prefer leave this out, and define a flag in config.h. See my comments for configure.in below.

> diff -Nrup a/client/client_priv.h b/client/client_priv.h
> --- a/client/client_priv.h 2007-03-23 19:24:01 +01:00
> +++ b/client/client_priv.h 2007-08-08 17:34:47 +02:00
> @@ -53,3 +53,14 @@ enum options_client
> OPT_TZ_UTC, OPT_AUTO_CLOSE, OPT_SSL_VERIFY_SERVER_CERT,
> OPT_DEBUG_INFO, OPT_ERROR_LOG_FILE
> };
> +
> +/*
> + set up default client port from factory or ./configure'd default,
> + $MY_TCP_PORT etc. which can then be overridden with command-line
> + option. mysql_client_port_init() just sets mysql_port(); most

Remove parens from mysql_port.

> + clients will want to instead call mysql_client_port_setup() before
> + handling options as this also corrects the default value to the
> + above so that "print variables" will show a sensible value.

Say my_print_variables().

> +*/
> +int mysql_client_port_init(void);
> +int mysql_client_port_setup(struct my_option *opt, const char *name);
> diff -Nrup a/client/mysql.cc b/client/mysql.cc
> --- a/client/mysql.cc 2007-04-23 16:21:59 +02:00
> +++ b/client/mysql.cc 2007-08-08 17:34:47 +02:00
> @@ -28,7 +28,9 @@
> *
> **/
>
> +extern "C" {
> #include "client_priv.h"
> +}

Put the extern "C" into client_priv.h (#ifdef __cplusplus) instead.

Do you need more help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

> #include <m_ctype.h>
> #include <stdarg.h>
> #include <my_dir.h>

<cut>

> diff -Nrup a/configure.in b/configure.in
> --- a/configure.in 2007-07-16 12:42:05 +02:00
> +++ b/configure.in 2007-08-08 17:34:47 +02:00
> @@ -710,7 +710,34 @@ AC_ARG_WITH(tcp-port,
> [ --with-tcp-port=port-number
> Which port to use for MySQL services (default 3306)],
> [ MYSQL_TCP_PORT=$withval ],
> - [ MYSQL_TCP_PORT=$MYSQL_TCP_PORT_DEFAULT ]
> + [ MYSQL_TCP_PORT=$MYSQL_TCP_PORT_DEFAULT
I'd move this assignment below the comment - it gets lost up here at the top, IMO.

> + # if we actually defaulted (as opposed to the pathological case of
> + # --with-tcp-port=<MYSQL_TCP_PORT_DEFAULT> which might in theory
> + # happen if whole batch of servers was built from a script), set
> + # the default to zero to indicate that; we don't lose information
> + # that way, because 0 obviously indicates that we can get the
> + # default value from MYSQL_TCP_PORT. this seems really evil, but
> + # testing for MYSQL_TCP_PORT==MYSQL_TCP_PORT_DEFAULT would make a
> + # a port of MYSQL_TCP_PORT_DEFAULT magic even if the builder did not
> + # intend it to mean "use the default, in fact, look up a good default
> + # from /etc/services if you can", but really, really meant 3306 when
> + # they passed in 3306. When they pass in a specific value, let them
> + # have it; don't second guess user and think we know better, this will

This paragraph is confusing, IMO. I'd probably just remove it.

> + # just make people cross. this makes the the logic work like this
> + # (which is complicated enough):
> + #
> + # - if a port was set during build, use that as a default.

Add " (READ_PORT_FROM_ETC_SERVICES is not defined)" before the comma.

> + #
> + # - otherwise, try to look up a port in /etc/services; if that fails,
> + # use MYSQL_TCP_PORT_DEFAULT (at the time of this writing 3306)
> + #
> + # - allow the MYSQL_TCP_PORT environment variable to override that.
> + #
> + # - allow command-line parameters to override all of the above.
> + #
> + # the top-most MYSQL_TCP_PORT_DEFAULT is read from win/configure.js,
> + # so don't mess with that.

Can we help you?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

This comment should go by the MYSQL_TCP_PORT_DEFAULT= definition, not here.

> + MYSQL_TCP_PORT_DEFAULT=0 ]
OK, here I'd use:

AC_DEFINE([READ_PORT_FROM_ETC_SERVICES], [1], [Port in /etc/services should override MYSQL_TCP_PORT]);

Then MYSQL_TCP_PORT_DEFAULT isn't exposed anywhere - I think this is easier to understand, cleaner to implement.

> )
> AC_SUBST(MYSQL_TCP_PORT)
> # We might want to document the assigned port in the manual.

<cut>

> diff -Nrup a/include/mysql_version.h.in b/include/mysql_version.h.in
> --- a/include/mysql_version.h.in 2004-05-25 01:32:12 +02:00
> +++ b/include/mysql_version.h.in 2007-08-08 17:34:49 +02:00
> @@ -15,6 +15,7 @@
> #define FRM_VER @DOT_FRM_VERSION@
> #define MYSQL_VERSION_ID @MYSQL_VERSION_ID@
> #define MYSQL_PORT @MYSQL_TCP_PORT@
> +#define MYSQL_PORT_DEFAULT @MYSQL_TCP_PORT_DEFAULT@
> #define MYSQL_UNIX_ADDR "@MYSQL_UNIX_ADDR@"
> #define MYSQL_CONFIG_NAME "my"
> #define MYSQL_COMPILATION_COMMENT "@COMPILATION_COMMENT@"

This isn't needed, if READ_PORT_FROM_ETC_SERVICES is defined in config.h.

Can't find what you're looking for?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

> diff -Nrup a/libmysql/libmysql.c b/libmysql/libmysql.c
> --- a/libmysql/libmysql.c 2007-07-06 08:34:06 +02:00
> +++ b/libmysql/libmysql.c 2007-08-08 17:34:49 +02:00
> @@ -19,6 +19,7 @@
>
> #include <my_global.h>
> #include <my_sys.h>
> +#include <my_getopt.h>
> #include <my_time.h>
> #include <mysys_err.h>
> #include <m_string.h>
> @@ -97,6 +98,96 @@ static my_bool org_my_init_done= 0;
>
>
> /*
> + Initialize the MySQL client port address
> +
> + SYNOPSIS
> + mysql_client_port_init()
> +
> + NOTES
> + Called by mysql_server_init() (which may be wrapped in mysql_init()).
> + If none of the above is called before handling command-line options,
> + mysql_client_port_init() may be called directly before handling the
> + command-line, so "print variables" will render correct values.

I'd prefer to use the function name my_print_variables(), for easy cross-referencing.

> + May be called repeatedly.
> +
> + RETURN
> + 0 ok
> +*/
> +
> +int mysql_client_port_init(void)
> +{
> + if (!mysql_port)
> + {
> + mysql_port = MYSQL_PORT;
> +#ifndef MSDOS
> + {
> + struct servent *serv_ptr;
> + char *env;
> +
> + /*
> + if builder specifically requested a default port, use that
> + (even if it coincides with our factory default).
> + only if they didn't do we check /etc/services (and, failing
> + on that, fall back to the factory default of 3306).
> + either default can be overridden by the environment variable
> + MYSQL_TCP_PORT, which in turn can be overridden with command
> + line options.
> + */

I'd split this comment up, and put each piece before the code that implements it. Like:

/* Start with the compile-time default port value */ mysql_port= MYSQL_PORT;

/*
  Override mysql_port with /etc/services value, if MYSQL_PORT was not   specified explicitly to configure --with-tcp-port */
...

/* Override mysql_port with MYSQL_TCP_PORT environment variable */ ...

> +
> +#if MYSQL_PORT_DEFAULT == 0

Don't know where to look next?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

This would become:

#ifdef READ_PORT_FROM_ETC_SERVICES

Which is self-documenting.

> + if ((serv_ptr = getservbyname("mysql", "tcp")))
> + mysql_port = (uint) ntohs((ushort) serv_ptr->s_port);
> +#endif
> + if ((env = getenv("MYSQL_TCP_PORT")))
> + mysql_port =(uint) atoi(env);

Gotta love that whitespace.

> + }
> +#endif
> + }
> + if (!mysql_unix_port)
> + {
> + char *env;
> +#ifdef __WIN__
> + mysql_unix_port = (char*) MYSQL_NAMEDPIPE;
> +#else
> + mysql_unix_port = (char*) MYSQL_UNIX_ADDR;
> +#endif
> + if ((env = getenv("MYSQL_UNIX_PORT")))
> + mysql_unix_port = env;
> + }
> + return 0;
> +}
> +
> +
> +/*
> + Initialize the MySQL client port address
> +
> + SYNOPSIS
> + mysql_client_port_setup()
> + opt struct containing client's options
> + name name of option that contains port ("port")
> +
> + NOTES
> + Call before handling the command-line, so "print variables" will
> + render correct values.
> +
> + RETURN
> + -1 Error
> + >=0 default port (usually > 0); can be overridden on command-line
> +*/
> +
> +int mysql_client_port_setup(struct my_option *opt, const char *name)
> +{
> + char *dummy;
> +
> + mysql_client_port_init();
> + if (my_getopt_find(name, strlen(name),
> + (const struct my_option **) &opt, &dummy)>0)
> + return opt->def_value= mysql_port;
> + return -1;
> +}
> +
> +
> +/*
> Initialize the MySQL client library
>
> SYNOPSIS
> @@ -126,31 +217,7 @@ int STDCALL mysql_server_init(int argc _
> if (my_init()) /* Will init threads */
> return 1;
> init_client_errs();
> - if (!mysql_port)
> - {
> - mysql_port = MYSQL_PORT;
> -#ifndef MSDOS
> - {
> - struct servent *serv_ptr;
> - char *env;
> - if ((serv_ptr = getservbyname("mysql", "tcp")))
> - mysql_port = (uint) ntohs((ushort) serv_ptr->s_port);
> - if ((env = getenv("MYSQL_TCP_PORT")))
> - mysql_port =(uint) atoi(env);
> - }
> -#endif
> - }
> - if (!mysql_unix_port)
> - {
> - char *env;
> -#ifdef __WIN__
> - mysql_unix_port = (char*) MYSQL_NAMEDPIPE;
> -#else
> - mysql_unix_port = (char*) MYSQL_UNIX_ADDR;
> -#endif
> - if ((env = getenv("MYSQL_UNIX_PORT")))
> - mysql_unix_port = env;
> - }
> + mysql_client_port_init();
> mysql_debug(NullS);
> #if defined(SIGPIPE) && !defined(__WIN__) && !defined(__NETWARE__)
> (void) signal(SIGPIPE, SIG_IGN);
> diff -Nrup a/mysys/my_getopt.c b/mysys/my_getopt.c
> --- a/mysys/my_getopt.c 2007-02-22 15:59:54 +01:00
> +++ b/mysys/my_getopt.c 2007-08-08 17:34:49 +02:00
> @@ -23,9 +23,6 @@
> static void default_reporter(enum loglevel level, const char *format, ...);
> my_error_reporter my_getopt_error_reporter= &default_reporter;
>
> -static int findopt(char *optpat, uint length,
> - const struct my_option **opt_res,
> - char **ffname);
> my_bool getopt_compare_strings(const char *s,
> const char *t,
> uint length);
> @@ -196,7 +193,7 @@ int handle_options(int *argc, char ***ar
> */
> LINT_INIT(prev_found);
> optp= longopts;
> - if (!(opt_found= findopt(opt_str, length, &optp, &prev_found)))
> + if (!(opt_found= my_getopt_find(opt_str, length, &optp, &prev_found)))
> {
> /*
> Didn't find any matching option. Let's see if someone called
> @@ -219,9 +216,10 @@ int handle_options(int *argc, char ***ar
> opt_str+= (special_opt_prefix_lengths[i] + 1);
> if (i == OPT_LOOSE)
> option_is_loose= 1;
> - if ((opt_found= findopt(opt_str, length -
> - (special_opt_prefix_lengths[i] + 1),
> - &optp, &prev_found)))
> + if ((opt_found= my_getopt_find(opt_str, length -
> + (special_opt_prefix_lengths[i]
> + + 1),
> + &optp, &prev_found)))
> {
> if (opt_found > 1)
> {
> @@ -610,7 +608,7 @@ static int setval(const struct my_option
> Find option
>
> SYNOPSIS
> - findopt()
> + my_getopt_find()
> optpat Prefix of option to find (with - or _)
> length Length of optpat
> opt_res Options
> @@ -628,9 +626,9 @@ static int setval(const struct my_option
> ffname points to first matching option
> */
>
> -static int findopt(char *optpat, uint length,
> - const struct my_option **opt_res,
> - char **ffname)
> +int my_getopt_find(const char *optpat, uint length,
> + const struct my_option **opt_res,
> + char **ffname)
> {
> uint count;
> struct my_option *opt= (struct my_option *) *opt_res;
> diff -Nrup a/sql/mysqld.cc b/sql/mysqld.cc
> --- a/sql/mysqld.cc 2007-07-05 12:34:10 +02:00
> +++ b/sql/mysqld.cc 2007-08-08 17:34:49 +02:00
> @@ -1300,8 +1300,21 @@ static void set_ports()
> { // Get port if not from commandline
> struct servent *serv_ptr;
> mysqld_port= MYSQL_PORT;
> +
> + /*
> + if builder specifically requested a default port, use that
> + (even if it coincides with our factory default).
> + only if they didn't do we check /etc/services (and, failing
> + on that, fall back to the factory default of 3306).
> + either default can be overridden by the environment variable
> + MYSQL_TCP_PORT, which in turn can be overridden with command
> + line options.
> + */
> +

This comment is unnecessary, when READ_PORT_FROM_ETC_SERVICES is used.

> +#if MYSQL_PORT_DEFAULT == 0
> if ((serv_ptr= getservbyname("mysql", "tcp")))
> mysqld_port= ntohs((u_short) serv_ptr->s_port); /* purecov: inspected */
> +#endif
> if ((env = getenv("MYSQL_TCP_PORT")))
> mysqld_port= (uint) atoi(env); /* purecov: inspected */
> }

Confused? Frustrated?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

Regards,

Timothy

-- 
-- Timothy Smith       Team Lead, Maintenance; Dolores, Colorado, USA
-- MySQL, www.mysql.com      The best DATABASE COMPANY in the GALAXY!

-- 
MySQL Code Commits Mailing List
For list archives: 
http://lists.mysql.com/commits
To unsubscribe:    
http://lists.mysql.com/commits?unsub=lists@pantek.com