He Zhenxing wrote:
> Hi Mats
>
> Thank you! Is this patch approved after these modifications?
Yes.
/Matz
>
> Mats Kindahl wrote:
>> 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
>> --
>> MySQL Code Commits Mailing List
>> For list archives: http://lists.mysql.com/commits
>> To unsubscribe: http://lists.mysql.com/commits?unsub=1
>
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
