[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