MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Frazer Clement Date:January 14 2009 11:13am
Subject:Re: WL4562 (IPv6 support for replication)
View as plain text  
Hi Andrei,
Thanks for looking through the patches.
Some responses inline.
Frazer

Andrei Elkin wrote:
> Frazer, hello.
>
> I think your work is a good for acceptance.
>
>   
>> Hi Andrei,
>>  Thanks for looking at the patches.
>>  I'm afraid I've got another small one to add to the growing pile.
>> Valgrind kindly told me about missing calls to freeaddrinfo() in
>> client.c which I've now fixed.
>>
>> http://lists.mysql.com/commits/63092
>>     
>
>
> Per the 4th patch, I looked at the 6.0 CLI_MYSQL_REAL_CONNECT() to find out a suspect
> of
> the same crime in 
>
>    gai_errno= getaddrinfo(host, port_buf, &hints, &res_lst);
>
>     if (gai_errno != 0) 
>     {
>       ...  //  no freeaddrinfo()
>       goto error;
>     }
>
> The rest of error handling ifs calls  freeaddrinfo().
>   
Yes, I was assuming that when getaddrinfo returns an error, it does not 
update the passed res_lst ptr.
Reading the manual pages for getaddrinfo, there's nothing that 
specifically mentions this behaviour, so I suppose it's safer to 
initialise res_lst to NULL and then pass it to freeaddrinfo() on this 
error path.
What do you think?
> I wonder if that's a bug in 6.0 too. According to your fixes, it is a
> bug. Maybe you could report and fix it as well while you are working
> within this context?
>   
I'll have a look.
> In the telco's code there seems to be yet another possibility for
> valgrind; CLI_MYSQL_REAL_CONNECT() has
>
>       if (mysql->options.bind_name) {
>         /* TODOs for client bind:
>            - check error codes
>            - don't use socket for localhost if this option is given
>         */
>         struct addrinfo* clientBindAddrInfo= NULL;
>         struct addrinfo* currAddr= NULL;
>         int gai_errno= 0;
>         int bindResult= -1;
>
>         DBUG_PRINT("info",("Attempting client bind to address : %s",
>                            mysql->options.bind_name));
>         /* Lookup address info for name */
>         gai_errno= getaddrinfo(mysql->options.bind_name, 0,
>                                &hints, &clientBindAddrInfo);
>         if (gai_errno)
>         {
>           DBUG_PRINT("info",("getaddrinfo error %d", gai_errno));
>           set_mysql_extended_error(mysql, CR_UNKNOWN_HOST, unknown_sqlstate,
>                                    ER(CR_UNKNOWN_HOST), mysql->options.bind_name,
> errno);
>           goto error;
>         }
>
> and the latest patch does not fix lack of
> freeaddrinfo(clientBindAddrInfo) at goto error.
>   
Yes, same case as above.
> For the 1st patch, I have a few comments.
> First, I can help to ask you how did you `identify' the 6.0 patches? 
> Are we ensured that entire IPv6 work of the 6.0 is imported to the telco?
>   
Not particularly scientifically - Brian mentioned some of the files he'd 
modified, I looked at the annotated source and found the changesets.
Then I kind of manually merged the changesets with mysql-5.1-telco-6.4, 
also looking at the mysql-6.0 source.
Hopefully I have got the required patches from mysql-6.0, but I am not 
*sure* of this.
> It all looks fine, I have only one question: why  there is an alias for res_lst at
> all
>
> +    /* We only look at the first item (something to think about changing in the
> future) */
> +    t_res= res_lst; 
>   
Agree that it's not necessary. If the code did look at > 1 result then 
it would probably use t_res to iterate over them, and keep res_lst 
around for freeing.
While the code does not do that, it is probably clearer not to use t_res.
> +    {
> ...
> +      sock= my_socket_create(t_res->ai_family
>
> whereas res_lst can work.
>
> Anyway, i would not like to improve the code in the telco patch
> only. Maybe to fix the new bug and clean up on the way?
>   
Ok.
>
> 2. About testing. I have seen you mentioned testing with IPv6 address
> other than ::1. I assume you did them anyway.
>   
Well I've tested with the various local interface IPv6 addresses, and 
verified that binding to them (as opposed to ::1, or to localhost) etc 
works.
I still have not tested a real IPv6 connection via a router to a 
different machine.
One idea to get this environment is to use one of the 'free IPv6 tunnel' 
services - to connect 2 machines via an IPv6 tunnel over my IPv4 ISP 
connection.
> I think it makes sense to create *.test possibly with *.opt files with
> instructions to start both the servers and replication. The tests can
> be run if the user/tester has set up IPv6 network.
>   
Ok, will look into what *.opt files are - a chance to improve my MTR 
knowledge !
>
>
> Other patches are straightforward and there are no comments.
>
>
> cheers,
>
> Andrei
>
>
>
>
>
>   

-- 
Frazer Clement, Software Engineer, MySQL Cluster 
Sun Microsystems - www.mysql.com
Office: Reading, UK

Are you MySQL certified?  www.mysql.com/certification

Thread
Re: WL4562 (IPv6 support for replication)Andrei Elkin13 Jan
  • Re: WL4562 (IPv6 support for replication)Paul DuBois13 Jan
    • Re: WL4562 (IPv6 support for replication)Frazer Clement14 Jan
    • Re: WL4562 (IPv6 support for replication)Andrei Elkin14 Jan
  • Re: WL4562 (IPv6 support for replication)Frazer Clement14 Jan
    • Re: WL4562 (IPv6 support for replication)Andrei Elkin14 Jan
      • Re: WL4562 (IPv6 support for replication)Frazer Clement15 Jan