[FFmpeg-devel] [PATCH/RFC] Prefer getaddrinfo over gethostbyname
Martin Storsjö
martin
Mon Jan 4 11:26:41 CET 2010
On Mon, 4 Jan 2010, Luca Abeni wrote:
> As requested, here are some comments on patch 0008:
> diff --git a/libavformat/udp.c b/libavformat/udp.c
> index a89de00..4f124c2 100644
> --- a/libavformat/udp.c
> +++ b/libavformat/udp.c
> @@ -240,13 +240,27 @@ static int udp_port(struct sockaddr_storage *addr, int
> addr_len)
>
> static int udp_set_url(struct sockaddr_in *addr, const char *hostname, int
> port)
> {
> - /* set the destination address */
> - if (resolve_host(&addr->sin_addr, hostname) < 0)
> + struct addrinfo hints, *ai, *cur;
> + char portstr[10];
> [...]
> Here, it seems to me that you are duplicating the code used by the version of
> udp_port() used if CONFIG_IPV6 is defined (the code you are adding looks very
> much like the udp_ipv6_resolve_host() function.
Yes, it does mostly the same, but forces getaddrinfo to return a IPv4
address, since that codepath assumes that the sockaddr will be a
sockaddr_in.
> Since noone seems to like the current approach (having two implementations of
> udp_set_url() and similar functions - one is used when getaddrinfo() is
> available, the other one when it is not), switching to a different approach is
> a good idea. But this patch risks to create a lot of code duplication... Maybe
> just removing the "#if CONFIG_IPV6" and using one single udp_set_url()
> implementation is a better idea.
>
> I am surely missing something obvious, but why not just always defining
> CONFIG_IPV6 (removing the code used when it is not defined)? (Provinding, of
> course, a fallback getaddrinfo() implementation if the system does not provide
> it)
That's probably the best way ahead, but does require a bit more
compatibility wrappers. The ipv6 codepath uses getnameinfo, which is
usually used in association with getaddrinfo. I've got a replacement
wrapper for this one, too, when it's needed.
It also uses some macros for multicast/ipv6, and I haven't done anything
multicast related, so I'm a bit unfamiliar with that stuff.
> + for (cur = ai; cur; cur = cur->ai_next) {
> + if (cur->ai_family == AF_INET) {
> + *addr = *(struct sockaddr_in *)cur->ai_addr;
> + freeaddrinfo(ai);
> + return sizeof(struct sockaddr_in);
> + }
> + }
>
> This for() loop always confused me...
getaddrinfo returns a linked list of addrinfo structures, and even if we
set the hint that we're only interested in an AF_INET address, we make
sure and look for such one, if a bad implementation would happen to
return an AF_INET6 address (and an AF_INET later in the list) anyway. I
don't know if there are such getaddrinfo implementations - I'm just
programming defensively.
But given this, I guess a better approach would be to simply drop this
patch (it actually feels a bit redundant, since it does the same as the
resolve_host fix in patch 2). Later (after the initial getaddrinfo support
is commited?) we could work on adding more compatibility wrappers for
making the "ipv6 codepath" in udp.c the only one.
// Martin
More information about the ffmpeg-devel
mailing list