Hi Sven,
Thanks for the comments. These are indeed added value for this patch.
I have committed the new patch with your requests on it.
Additional comments inline.
Regards,
Luís
Sven Sandberg wrote:
> Hi Luís,
>
> Good work, the source patch does the work and I think the test tests the
> right things. I have some suggestions for the test case, and a comment
> on the source code comment, could you fix that?
>
> /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.
>> 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 03:16:54
>> +0000
>> @@ -0,0 +1,24 @@
>> +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;
>> +SHOW SLAVE STATUS;
>> +Slave_IO_State Master_Host Master_User Master_Port
>> Connect_Retry Master_Log_File Read_Master_Log_Pos
>> Relay_Log_File Relay_Log_Pos Relay_Master_Log_File
>> Slave_IO_Running Slave_SQL_Running Replicate_Do_DB
>> Replicate_Ignore_DB Replicate_Do_Table Replicate_Ignore_Table
>> Replicate_Wild_Do_Table Replicate_Wild_Ignore_Table
>> Last_Errno Last_Error Skip_Counter Exec_Master_Log_Pos
>> Relay_Log_Space Until_Condition Until_Log_File
>> Until_Log_Pos Master_SSL_Allowed Master_SSL_CA_File
>> Master_SSL_CA_Path Master_SSL_Cert Master_SSL_Cipher
>> Master_SSL_Key Seconds_Behind_Master
>> +X 127.0.0.1 X X X X X X X X X X
>> X X X X X X X X X X X X X X
>> X X X X X X X
>> +CHANGE MASTER TO MASTER_HOST="";
>> +ERROR HY000: Incorrect arguments to MASTER_HOST
>> +SHOW SLAVE STATUS;
>> +Slave_IO_State Master_Host Master_User Master_Port
>> Connect_Retry Master_Log_File Read_Master_Log_Pos
>> Relay_Log_File Relay_Log_Pos Relay_Master_Log_File
>> Slave_IO_Running Slave_SQL_Running Replicate_Do_DB
>> Replicate_Ignore_DB Replicate_Do_Table Replicate_Ignore_Table
>> Replicate_Wild_Do_Table Replicate_Wild_Ignore_Table
>> Last_Errno Last_Error Skip_Counter Exec_Master_Log_Pos
>> Relay_Log_Space Until_Condition Until_Log_File
>> Until_Log_Pos Master_SSL_Allowed Master_SSL_CA_File
>> Master_SSL_CA_Path Master_SSL_Cert Master_SSL_Cipher
>> Master_SSL_Key Seconds_Behind_Master
>> +X 127.0.0.1 X X X X X X X X X X
>> X X X X X X X X X X X X X X
>> X X X X X X X
>> +CHANGE MASTER TO MASTER_HOST="foo";
>> +SHOW SLAVE STATUS;
>> +Slave_IO_State Master_Host Master_User Master_Port
>> Connect_Retry Master_Log_File Read_Master_Log_Pos
>> Relay_Log_File Relay_Log_Pos Relay_Master_Log_File
>> Slave_IO_Running Slave_SQL_Running Replicate_Do_DB
>> Replicate_Ignore_DB Replicate_Do_Table Replicate_Ignore_Table
>> Replicate_Wild_Do_Table Replicate_Wild_Ignore_Table
>> Last_Errno Last_Error Skip_Counter Exec_Master_Log_Pos
>> Relay_Log_Space Until_Condition Until_Log_File
>> Until_Log_Pos Master_SSL_Allowed Master_SSL_CA_File
>> Master_SSL_CA_Path Master_SSL_Cert Master_SSL_Cipher
>> Master_SSL_Key Seconds_Behind_Master
>> +X foo X X X X X X X X X X X
>> X X X X X X X X X X X X X X
>> X X X X X X
>> +CHANGE MASTER TO MASTER_HOST="127.0.0.1";
>> +SHOW SLAVE STATUS;
>> +Slave_IO_State Master_Host Master_User Master_Port
>> Connect_Retry Master_Log_File Read_Master_Log_Pos
>> Relay_Log_File Relay_Log_Pos Relay_Master_Log_File
>> Slave_IO_Running Slave_SQL_Running Replicate_Do_DB
>> Replicate_Ignore_DB Replicate_Do_Table Replicate_Ignore_Table
>> Replicate_Wild_Do_Table Replicate_Wild_Ignore_Table
>> Last_Errno Last_Error Skip_Counter Exec_Master_Log_Pos
>> Relay_Log_Space Until_Condition Until_Log_File
>> Until_Log_Pos Master_SSL_Allowed Master_SSL_CA_File
>> Master_SSL_CA_Path Master_SSL_Cert Master_SSL_Cipher
>> Master_SSL_Key Seconds_Behind_Master
>> +X 127.0.0.1 X X X X X X X X X X
>> X X X X X X X X X X X X X X
>> X X X X X X X
>> +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 03:16:54
>> +0000
>> @@ -0,0 +1,38 @@
>> +# tests error when setting empty master host +# Details in BUG#28796
>
> Please state the purpose of the test and give some hint on how it is
> implemented.
Done.
> - I think the purpose is to verify that (1) an error is generated when
> setting MASTER_HOST = ''; (2) no error is generated when setting
> MASTER_HOST to a nonempty string; (3) replication works after setting a
> correct MASTER_HOST.
>
> - It is implemented by issuing CHANGE MASTER TO MASTER_HOST = x and
> verifying that error/no error is generated. You use SHOW SLAVE STATUS to
> verify that the master host was set correctly.
>
> See e.g., rpl_trunc_temp.test for examples.
>
>> +
>> +--source include/master-slave.inc
>> +
>> +connection slave;
>> +STOP SLAVE;
>> +--source include/wait_for_slave_to_stop.inc
>> +
>> +replace_column 1 X 3 X 4 X 5 X 6 X 7 X 8 X 9 X 10 X 11 X 12 X 13 X 14
>> X 15 X 16 X 17 X 18 X 19 X 20 X 21 X 22 X 23 X 24 X 25 X 26 X 27 X 28
>> X 29 X 30 X 31 X 32 X 33 X; +SHOW SLAVE STATUS;
>
> There is a much nicer way to do this:
>
> let $master_host= query_get_value(SHOW SLAVE STATUS, Master_Host, 1);
> --echo Master_Host = '$master_host' (expected '')
>
> Could you do that here, and in three places below?
Done. Good suggestion, thanks.
>> +
>> +# attempt to change to an empty master host should +# result in error
>> 1210: "Incorrect arguments to ..."
>> +
>> +--error 1210
>
> Please use named constants instead of numbers:
>
> error ER_WRONG_ARGUMENTS;
Done.
>> +CHANGE MASTER TO MASTER_HOST="";
>> +
>> +# show slave status still holds previous information
>> +replace_column 1 X 3 X 4 X 5 X 6 X 7 X 8 X 9 X 10 X 11 X 12 X 13 X 14
>> X 15 X 16 X 17 X 18 X 19 X 20 X 21 X 22 X 23 X 24 X 25 X 26 X 27 X 28
>> X 29 X 30 X 31 X 32 X 33 X; +SHOW SLAVE STATUS;
>> +
>> +# changing master to other than empty master host succeeds
>> +CHANGE MASTER TO MASTER_HOST="foo";
>> +
>> +# show slave status should hold "foo" as master host
>> +replace_column 1 X 3 X 4 X 5 X 6 X 7 X 8 X 9 X 10 X 11 X 12 X 13 X 14
>> X 15 X 16 X 17 X 18 X 19 X 20 X 21 X 22 X 23 X 24 X 25 X 26 X 27 X 28
>> X 29 X 30 X 31 X 32 X 33 X; +SHOW SLAVE STATUS;
>> +
>> +# changing back to localhost
>> +CHANGE MASTER TO MASTER_HOST="127.0.0.1";
>> +
>> +replace_column 1 X 3 X 4 X 5 X 6 X 7 X 8 X 9 X 10 X 11 X 12 X 13 X 14
>> X 15 X 16 X 17 X 18 X 19 X 20 X 21 X 22 X 23 X 24 X 25 X 26 X 27 X 28
>> X 29 X 30 X 31 X 32 X 33 X; +SHOW SLAVE STATUS;
>> +
>> +# 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 03:16:54 +0000
>> @@ -1080,6 +1080,15 @@ bool change_master(THD* thd, MASTER_INFO
>>
>> thd->proc_info = "Changing master";
>> LEX_MASTER_INFO* lex_mi= &thd->lex->mi;
>> +
>> + /* checks whether host argument is an empty string or not. */
>
> I don't think you need to document that we check if host is empty, it's
> easy to see if you read the next line. But could you add a short
> sentence explaining why we need to generate an error if the host
> argument is empty?
Agree and done.
>
>> + 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))
>>
>>
>
>
