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().
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?
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.
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?
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;
+ {
...
+ 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.
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.
Other patches are straightforward and there are no comments.
cheers,
Andrei
