Great job. The patch is approved, although, I have a few comments:
1 - Please, update the bug report to explain why the "show slave status"
cannot be enabled when the slave is not
running or there is no slave configuration. This is important to
understand your patch.
2 - Maybe this patch will require documentation changes, won't it?
3 - In line comments:
Luis Soares wrote:
> #At file:///stuff/workspace/mysql-server/bugfix/5.0-bugteam-28796/
>
> 2732 Luis Soares 2008-12-04
> Bug #28796 CHANGE MASTER TO MASTER_HOST="" leads to invalid master.info
>
> Description:
> When setting master_host to empty string there was no explicit error thrown
> regarding this. Furthermore, show slave status would not output data despite having valid
> information to report.
>
> Solution:
> Allow setting master_host to empty string and output error when connecting to
> master. This allows show slave status to still output information.
>
> Details:
> The empty mi->host (!*mi->host[0]) has semantics associated with it in
> many places (including when connecting to master). It is the default value (when no
> options are set for instance and no change master has been issued). As such I have added
> the flag that is just turned on when explicit master_host is assigned. Furthermore should
> empty string be assigned, it just turns on the flag if it was assigned using: i) change
> master to master_host or ii) read from the master.info file.
>
> When connecting to master it checks if there is an empty string for master
> hostname. This was done because removing the existing test for empty string would lead to
> a different error (dumped in the error log) when compared to invalid hostname.
>
> Basically, with the patch you can assign empty string to master_host and still
> have show slave status to work. When issuing start slave; you get a error with invalid
> hostname instead of the previous error "The server is not configured as slave..."
> modified:
> sql/slave.cc
> sql/slave.h
> sql/sql_repl.cc
>
> === modified file 'sql/slave.cc'
> --- a/sql/slave.cc 2008-03-28 20:01:05 +0000
> +++ b/sql/slave.cc 2008-12-04 12:50:16 +0000
> @@ -2044,7 +2044,19 @@ void init_master_info_with_options(MASTE
> mi->master_log_pos = BIN_LOG_HEADER_SIZE; // skip magic number
>
> if (master_host)
> + {
> strmake(mi->host, master_host, sizeof(mi->host) - 1);
> +
> + /* There was a non NULL master host in options. */
> + mi->is_master_host_set= 1;
> + }
> + else
> + /*
> + Assuming that when the master host, from options, is
> + NULL, and mi is being filled only from options values,
> + show slave status should not output info.
> + */
> + mi->is_master_host_set= 0;
> if (master_user)
> strmake(mi->user, master_user, sizeof(mi->user) - 1);
> if (master_password)
> @@ -2275,6 +2287,12 @@ file '%s')", fname);
> mi->port= (uint) port;
> mi->connect_retry= (uint) connect_retry;
> mi->ssl= (my_bool) ssl;
> +
> + /*
> + when initializing from file the master host is
> + always set (even if empty).
> + */
> + mi->is_master_host_set= 1;
> }
> DBUG_PRINT("master_info",("log_file_name: %s position: %ld",
> mi->master_log_name,
> @@ -2453,9 +2471,9 @@ bool show_master_info(THD* thd, MASTER_I
> Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF))
> DBUG_RETURN(TRUE);
>
> - if (mi->host[0])
> + DBUG_PRINT("info",(mi->host[0] ? ("master host is set: '%s'", mi->host) :
> ("master host is not set!")));
> + if (mi->is_master_host_set)
> {
> - DBUG_PRINT("info",("host is set: '%s'", mi->host));
> String *packet= &thd->packet;
> protocol->prepare_for_resend();
>
>
> === modified file 'sql/slave.h'
> --- a/sql/slave.h 2008-02-22 15:07:07 +0000
> +++ b/sql/slave.h 2008-12-04 12:50:16 +0000
> @@ -427,6 +427,7 @@ typedef struct st_master_info
> int events_till_abort;
> #endif
> bool inited;
> + bool is_master_host_set;
> volatile bool abort_slave;
> volatile uint slave_running;
> volatile ulong slave_run_id;
> @@ -442,7 +443,7 @@ typedef struct st_master_info
> long clock_diff_with_master;
>
> st_master_info()
> - :ssl(0), fd(-1), io_thd(0), inited(0),
> + :ssl(0), fd(-1), io_thd(0), inited(0), is_master_host_set(0),
> abort_slave(0),slave_running(0), slave_run_id(0)
> {
> host[0] = 0; user[0] = 0; password[0] = 0;
>
> === modified file 'sql/sql_repl.cc'
> --- a/sql/sql_repl.cc 2008-01-31 06:19:29 +0000
> +++ b/sql/sql_repl.cc 2008-12-04 12:50:16 +0000
> @@ -763,6 +763,7 @@ err:
> int start_slave(THD* thd , MASTER_INFO* mi, bool net_report)
> {
> int slave_errno= 0;
> + int empty_master_host_detected= 0;
> int thread_mask;
> DBUG_ENTER("start_slave");
>
> @@ -860,6 +861,11 @@ int start_slave(THD* thd , MASTER_INFO*
> master_info_file,relay_log_info_file,
> thread_mask);
> }
> + else if(server_id_supplied && mi->is_master_host_set &&
> !*mi->host)
> + {
> + slave_errno = ER_CONNECT_TO_MASTER;
> + empty_master_host_detected= 1;
> + }
> else
> slave_errno = ER_BAD_SLAVE;
> }
> @@ -874,6 +880,8 @@ int start_slave(THD* thd , MASTER_INFO*
>
> if (slave_errno)
> {
> + if (empty_master_host_detected)
> + my_printf_error(slave_errno, ER(slave_errno), MYF(0), "Empty MASTER HOST!");
> if (net_report)
> my_message(slave_errno, ER(slave_errno), MYF(0));
> DBUG_RETURN(1);
> @@ -1116,7 +1124,18 @@ bool change_master(THD* thd, MASTER_INFO
> DBUG_PRINT("info", ("master_log_pos: %lu", (ulong) mi->master_log_pos));
>
> if (lex_mi->host)
> + {
> strmake(mi->host, lex_mi->host, sizeof(mi->host)-1);
> + /*
> + Although this flag is also set in init_master_info
> + called previously, this must be set here also. This
> + is because it may have been changed using the change
> + master to master_host command.
> +
>
This is a little bit fuzzy.
There are three possible entries to modify the slave's parameter:
. options
. file
. change
This is the last possibility, isn't it?
> + mi->host != lex_mi->host
>
This is not clear. Either remove this or explain what you meant say.
> + */
> + mi->is_master_host_set= 1;
> + }
> if (lex_mi->user)
> strmake(mi->user, lex_mi->user, sizeof(mi->user)-1);
> if (lex_mi->password)
>
>
>
Cheers.
