Hi Luís,
Great work, the patch is perfect now! Approved.
/Sven
Luis Soares wrote:
> #At file:///stuff/workspace/mysql-server/bugfix/28796/5.0-bugteam/
>
> 2746 Luis Soares 2008-12-20
> BUG#28796: CHANGE MASTER TO MASTER_HOST="" leads to invalid master.info
>
> This patch addresses the bug reported by checking wether
> host argument is an empty string or not. If empty, an error is
> reported to the client, otherwise continue normally.
>
> This commit is based on the originally proposed patch and adds
> a test case as requested during review as well as refines comments,
> and makes test case result file less verbose (compared to previous patch).
> added:
> mysql-test/r/rpl_empty_master_host.result
> mysql-test/t/rpl_empty_master_host.test
> modified:
> sql/sql_repl.cc
>
> === added file 'mysql-test/r/rpl_empty_master_host.result'
> --- a/mysql-test/r/rpl_empty_master_host.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/r/rpl_empty_master_host.result 2008-12-20 15:11:31 +0000
> @@ -0,0 +1,16 @@
> +stop slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +reset master;
> +reset slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +start slave;
> +STOP SLAVE;
> +Master_Host = '127.0.0.1' (expected '127.0.0.1')
> +CHANGE MASTER TO MASTER_HOST="";
> +ERROR HY000: Incorrect arguments to MASTER_HOST
> +Master_Host = '127.0.0.1' (expected '127.0.0.1')
> +CHANGE MASTER TO MASTER_HOST="foo";
> +Master_Host = 'foo' (expected 'foo')
> +CHANGE MASTER TO MASTER_HOST="127.0.0.1";
> +Master_Host = '127.0.0.1' (expected '127.0.0.1')
> +START SLAVE;
>
> === added file 'mysql-test/t/rpl_empty_master_host.test'
> --- a/mysql-test/t/rpl_empty_master_host.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/t/rpl_empty_master_host.test 2008-12-20 15:11:31 +0000
> @@ -0,0 +1,51 @@
> +#
> +# BUG
> +# ---
> +# BUG#28796: CHANGE MASTER TO MASTER_HOST="" leads to invalid master.info
> +#
> +# Description
> +# -----------
> +#
> +# This test aims at:
> +# i) verifying that an error is thrown when setting MASTER_HOST=''
> +# ii) no error is thrown when setting non empty MASTER_HOST
> +# iii) replication works after setting a correct host name/ip
> +#
> +# Implementation is performed by feeding different values (according
> +# to i), ii) and iii) ) to CHANGE MASTER TO MASTER_HOST= x and checking
> +# along the way if error/no error is thrown and/or if replication starts
> +# working when expected.
> +
> +--source include/master-slave.inc
> +
> +connection slave;
> +STOP SLAVE;
> +--source include/wait_for_slave_to_stop.inc
> +
> +let $master_host= query_get_value(SHOW SLAVE STATUS, Master_Host, 1);
> +--echo Master_Host = '$master_host' (expected '127.0.0.1')
> +
> +# attempt to change to an empty master host should
> +# result in error ER_WRONG_ARGUMENTS: "Incorrect arguments to ..."
> +error ER_WRONG_ARGUMENTS;
> +CHANGE MASTER TO MASTER_HOST="";
> +
> +# show slave status still holds previous information
> +let $master_host= query_get_value(SHOW SLAVE STATUS, Master_Host, 1);
> +--echo Master_Host = '$master_host' (expected '127.0.0.1')
> +
> +# changing master to other than empty master host succeeds
> +CHANGE MASTER TO MASTER_HOST="foo";
> +
> +# show slave status should hold "foo" as master host
> +let $master_host= query_get_value(SHOW SLAVE STATUS, Master_Host, 1);
> +--echo Master_Host = '$master_host' (expected 'foo')
> +
> +# changing back to localhost
> +CHANGE MASTER TO MASTER_HOST="127.0.0.1";
> +let $master_host= query_get_value(SHOW SLAVE STATUS, Master_Host, 1);
> +--echo Master_Host = '$master_host' (expected '127.0.0.1')
> +
> +# start slave must succeed.
> +START SLAVE;
> +--source include/wait_for_slave_to_start.inc
>
> === 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-20 15:11:31 +0000
> @@ -1080,6 +1080,21 @@ bool change_master(THD* thd, MASTER_INFO
>
> thd->proc_info = "Changing master";
> LEX_MASTER_INFO* lex_mi= &thd->lex->mi;
> +
> + /*
> + We need to check if there is an empty master_host. Otherwise
> + change master succeeds, a master.info file is created containing
> + empty master_host string and when issuing: start slave; an error
> + is thrown stating that the server is not configured as slave.
> + (See BUG#28796).
> + */
> + if(lex_mi->host && strcmp(lex_mi->host, "") == 0)
> + {
> + my_error(ER_WRONG_ARGUMENTS, MYF(0), "MASTER_HOST");
> + unlock_slave_threads(mi);
> + DBUG_RETURN(TRUE);
> + }
> +
> // TODO: see if needs re-write
> if (init_master_info(mi, master_info_file, relay_log_info_file, 0,
> thread_mask))
>
>
--
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com
