Hi Mats
Thank you! Is this patch approved after these modifications?
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
