[FFmpeg-devel] [PATCH/RFC] Prefer getaddrinfo over gethostbyname
Mon Jan 4 11:42:49 CET 2010
On 04/01/10 11:26, Martin Storsj? wrote:
> 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
>> static int udp_set_url(struct sockaddr_in *addr, const char *hostname, int
>> - /* set the destination address */
>> - if (resolve_host(&addr->sin_addr, hostname)< 0)
>> + struct addrinfo hints, *ai, *cur;
>> + char portstr;
>> 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
So, can udp_ipv6_resolve_host() be reused here (maybe renaming it,
slightly modifying it if needed, or adding a parameter to force an IPv4
>> 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
> 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.
Maybe, the "non CONFIG_IPV6" functions can be disabled/removed one by
one, instead of removing the whole "non CONFIG_IPV6" at one time?
I mean, you can start by removing the functions that do not use
getnameinfo and multicast macros... Just an idea.
>> 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
Well, this can be what confused me ;-)
Anyway, I suspect that at least a comment in the code is needed (or
maybe we can just remove the for() loop, and simply check if the
returned address is what we asked for - I do not know, I leave this
decision to other people).
But let's keep in mind that 90% of the times (or maybe even 99%) when we
use UDP we use numeric addresses, so there are no symbolic names to be
resolved, and it should be immediately clear if an address is IPv4 or IPv6.
> 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.
AFAIR, this was the original plan. But I do not want to stop or slow
down things, so if people feel that a different plan is better, let's go
for a new plan. My only suggestion is that the disabling/removal of the
"non CONFIG_IPV6" codepath can probably be done incrementally (I hope :).
More information about the ffmpeg-devel