[FFmpeg-devel] [PATCH/RFC] Prefer getaddrinfo over gethostbyname

Luca Abeni lucabe72
Mon Jan 4 11:42:49 CET 2010


Hi Martin,

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
>> 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.

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 
address)?


>> 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.

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 :).


				Luca



More information about the ffmpeg-devel mailing list