Re: WL4562 (IPv6 support for replication)

[Andrei Elkin]

Frazer, hello.

>>>
>>> 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?

Sounds, ok. If the orig NULL assignment changes there is something to
free.


>> 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.

Thanks!

>> 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.

I wonder if it'd be possible to engage Brian himself to double-prove...

>> 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 !

Unless you know that, test.opt file is a possible way for specifying
the test's preferences to the server. For our case such ones could be
bind addresses. 
In a case your test is exclusively for manual invocation you can put it
in `mysql-test/suite/manual' directory.

>>
>>
>> Other patches are straightforward and there are no comments.
>>
>>
>> cheers,
>>
>> Andrei
>>
>>
>>
>>

cheers,

Andrei