Re: [PATCH] Allow the use of IPv6 nameservers

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

On 02/19/2010 08:23 PM, Yang Tse wrote:
> More nits...
>

Sorry for the late response, I've been quite busy lately..

> Not all systems have in6_addr, that's the reason for having
> ares_in6_addr in ares.h, so it would be convenient to move ares_addr
> definition further down in ares.h and use ares_in6_addr in the
> definition of the ares_addr struct. ares_set_nameservers() would also
> need to be moved further down in taht header file.
>

Hmm, fair enough. But I noticed that there are many places in c-ares
where in6_addr is used unconditionally and also there is a definition of
struct in6addr in ares_ipv6.h.

Anyhow, attached is a second patch that uses ares_in6_addr at least in
the code I touch with the first patch.

> If you expose definitions of addrV4 and addrV6 in ares.h these should
> be 'ares' or cares' prefixed. Better just keep them in the private
> part of the library and avoid the need to rename them.
>

OK, good suggestion.

> It seems that you are using getaddrinfo() to validate an address
> string, and that it makes no sense to feed it with a host name.

Not host name, I used the AI_NUMERICHOST flag. Per man getaddrinfo(3),
that flag denotes that we are passing a numeric address to getaddrinfo
and suppresses any network lookups.

> Why
> not use inet_addr() in a fisrt instance, this handles short hand IPv4
> dot notation, and if it fails then use ares_inet_pton() which should
> be capable of also handling IPv6 afterwards, getting rid of the call
> to getaddrinfo()? Or am I overlooking something here?
>

It is more or less just a matter of style. I generally dislike having if
(IPv4) {
   do_stuff();
} else if (IPv6) {
  do_stuff6();
}.

Using getaddrinfo there allowed for a protocol-agnostic code. The
attached version of the patch uses inet_addr() and ares_inet_pton(), though.

> There also seems to be some 'strange' memory management at the
> beginning of set_nameserver()
>

Correct. Sloppy refactoring. Should be fixed.

Thank you for the review! Hopefully, we're getting there :-)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkuGtGwACgkQHsardTLnvCWQKwCcCExH9FGwxmKfazTOhz5gupW0
FesAn2HfTceuCNRechLpwNvEEZo2G7Ev
=gFZq
-----END PGP SIGNATURE-----