He Zhenxing wrote:
> Mats Kindahl wrote:
>> Hi Jason!
>>
>> Here are my comments on the code.
>>
>> Best wishes,
>> Mats Kindahl
>>
>> He Zhenxing wrote:
[snip]
>>> @@ -3482,6 +3486,62 @@ static int safe_reconnect(THD* thd, MYSQ
>>> }
>>>
>>>
>>> +MYSQL *rpl_connect_master(MYSQL *mysql)
>>> +{
>>> + THD *thd= current_thd;
>>> + Master_info *mi= my_pthread_getspecific_ptr(Master_info*,
> RPL_MASTER_INFO);
>>> + if (!mi)
>>> + {
>>> + sql_print_error("'rpl_connect_master' must be called in slave I/O thread
> context.");
>>> + return NULL;
>>> + }
>> You can replace all this code
>>
>>> +
>>> + bool allocated= false;
>>> +
>>> + if (!mysql)
>>> + {
>>> + if(!(mysql= mysql_init(NULL)))
>>> + return NULL;
>>> + allocated= true;
>>> + }
>> with just:
>>
>> if (!(mysql= mysql_init(mysql))
>> return NULL;
>>
>> the C API keeps track of whether memory was allocated or not internally,
>> so there is no need to do that. Only difference is that now
>> rpl_connect_master() does a mysql_init() as well, but that is quite
>> natural and considering the greatly simplified code, that is a minor
>> difference.
>>
>
> What if the mysql pointer passed in are allocated by mysql_init? then it
> will be freed by this function, I think this will confuse the user.
>
> mysql = mysql_init(NULL);
> rpl_connect_master(mysql);
> mysql_close(mysql);
OK. But please document this by adding the code above as example within
@code ... @endcode blocks.
>>> +
>>> + /*
>>> + XXX: copied from connect_to_master, this function should not
>>> + change the slave status, so we cannot use connect_to_master
>>> + directly
>>> + */
>>> + mysql_options(mysql, MYSQL_OPT_CONNECT_TIMEOUT, (char *)
> &slave_net_timeout);
>>> + mysql_options(mysql, MYSQL_OPT_READ_TIMEOUT, (char *)
> &slave_net_timeout);
>> Maybe factor out the common parts into a separate function?
>>
>
> yes, but I'll leave it later
OK.
Just my few cents,
Mats Kindahl
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
