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

[Martin Storsjö]

Hi Luca,

On Mon, 4 Jan 2010, Luca Abeni wrote:

> On 04/01/10 11:26, Martin Storsj? wrote:
> > On Mon, 4 Jan 2010, Luca Abeni wrote:
> > 
> > > 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)?

Probably yes.

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

I'm not sure if the functions can be upgraded one at a time or that 
requires more refactoring - I'll look into it later if you don't get there 
first.

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

I added a comment to the getaddrinfo usage in resolve_host, locally.

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

Well, the udp protocol is used internally within the rtp protocol, 
launched e.g. from the rtsp demuxer, where it probably would happen to use 
hostnames.

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

Ok, so lets drop the current patch for the udp protocol, and instead aim 
at cleaning it up and merging the codepaths later, after the first patches 
have been commited.

// Martin