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
