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.

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.
> 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?
> 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 
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 
> 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 -
Office: Reading, UK

Are you MySQL certified?

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