Hi Andrei,
>
>>>> 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.
>
Ok
>
>
>>> 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...
>
I can ask him.
>
>>> 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 managed to setup IPv6 replication between my 2 laptops at home. It
wasn't too hard in the end. I'll send an email to Paul describing the steps.
>>> 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.
>
Ok, thanks
>
>>> Other patches are straightforward and there are no comments.
>>>
>>>
>>> cheers,
>>>
>>> Andrei
>>>
>>>
>>>
>>>
>>>
>
> cheers,
>
> Andrei
>
>
--
Frazer Clement, Software Engineer, MySQL Cluster
Sun Microsystems - www.mysql.com
Office: Reading, UK
Are you MySQL certified? www.mysql.com/certification
