Tatjana,
> ChangeSet@stripped, 2007-08-08 17:34:55+02:00, tnurnberg@stripped +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@stripped, 2007-08-08 17:34:47+02:00, tnurnberg@stripped
> +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."
> include/mysql_version.h.in@stripped, 2007-08-08 17:34:49+02:00,
> tnurnberg@stripped +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.
> #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.
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.
> 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
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 */
> }
Regards,
Timothy
--
-- Timothy Smith Team Lead, Maintenance; Dolores, Colorado, USA
-- MySQL, www.mysql.com The best DATABASE COMPANY in the GALAXY!
