Subject: Re: [PATCH] Allow the use of IPv6 nameservers

Re: [PATCH] Allow the use of IPv6 nameservers

From: Jakub Hrozek <jhrozek_at_redhat.com>
Date: Fri, 19 Feb 2010 18:20:23 +0100

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/18/2010 04:59 PM, Yang Tse wrote:
> 2010/2/12, Jakub Hrozek wrote:
>
>> A new patch is attached that also fixes ares_dup() and adds the manpage.
>>
>> Apart from the high-level summary in the patch itself, here's the changes:
>>
>> * struct "server state" no longer holds the address in "struct
>> in_addr" but rather "struct addrinfo *"
>> * when initializing from resolv.conf, getaddrinfo() is used in place
>> of inet_addr()
>> * a new API call ares_set_nameserves which can set the nameservers used
>> * in order not to break "struct ares_options", ares_save_options and
>> ares_init_options still works with v4 nameservers only. ares_dup() works
>> for both v4 and v6 servers.
>>
>> More info is, of course, in the patch :-)
>
> Ok, some comments on an initial review...
>

Thanks for the review!

> Makefile.inc is missing ares_set_nameservers.html and ares_set_nameservers.pdf.
>
> ares_set_nameservers.3 first line has ".TH
> ARES_SET_SOCKET_CALLBACK..." it should be changed to ".TH
> ARES_SET_NAMESERVERS...".
>

Fixed.

> And now some real meat which affects the whole patch...
>
> I believe that changing server_state struct 'addr' member data type to
> an 'ares_addr struct' instead than a pointer to 'struct addrinfo'
> serves the same purposes while keeping things a bit simpler later on,
> avoiding the need for free_addr member in server_state struct. Unless
> I've overlooked something.
>

Yeah, the reason I originally went with "struct addrinfo" was "it would
be nicer to use with connect() & friends" but using "struct adres_addr"
actually made to code shorter and way simpler.

> I also believe that moving private definition of the ares_addr struct
> from ares_private.h to the external interface in file ares.h making it
> publicly available would also help to make the interface to
> ares_set_nameservers() more palatable.
>
> CARES_EXTERN int ares_set_nameservers(ares_channel channel,
> struct ares_addr *server_addr,
> int num_servers);
>

Done

> Another important thing...
>
> Not all systems have getaddrinfo(), so if you want to use it you have
> to protect its usage with preprocessor symbol HAVE_GETADDRINFO,
> definition which fortunately is already available in config.h for
> systems that have it.
>
> Another twist with getaddrinfo(), for portability reasons, don't store
> addrinfo structures returned by getaddrinfo() directly, nor pointers
> to these. Copy the contents you need from the returned addrinfo
> structs and call freeaddrinfo() ASAP.
>

Both fixed, thanks for the heads-up.

A new version of the patch is attached.

        Jakub
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkt+yFcACgkQHsardTLnvCURIwCgthyGvOZXd3RRth2MaffYXfoH
5M8AoKulfXPFh+prq56wvVhHF4VVUrP+
=wpBh
-----END PGP SIGNATURE-----

Received on 2010-02-19