Re: [Qemu-devel] [PATCH] sockets: Normalize test for addrinfo flag AI_V4MAPPED

[Daniel P. Berrange]

On Thu, Nov 02, 2017 at 10:47:10PM +0000, Antonio Huete Jiménez wrote:
> 
> "Daniel P. Berrange" <address@hidden> escribió:
> 
> > On Thu, Nov 02, 2017 at 04:56:29PM +0000, Antonio Huete Jiménez wrote:
> > > From 2b4d9d8cb617445af8f3b062f917dfea42dbdc27 Mon Sep 17 00:00:00 2001
> > > From: Antonio Huete Jimenez <address@hidden>
> > > Date: Thu, 2 Nov 2017 17:46:24 +0100
> > > Subject: [PATCH] sockets: Normalize test for addrinfo flag AI_V4MAPPED
> > 
> > Can you explain why you're making this change....
> 
> Hi Daniel,
> 
> Thanks for reviewing the patch. I am trying to fix 'make check' for
> DragonFly BSD and this is the first issue I encountered.

Ok, it would be helpful if the commit msg could mention which test fails
without this fix, ideally with the actual error message (if one is given,
as opposed to an abort()).


> > > Signed-off-by: Antonio Huete Jimenez <address@hidden>
> > > ---
> > >  util/qemu-sockets.c | 54
> > > +++++++++++++++++++++++++++++++++--------------------
> > >  1 file changed, 34 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > > index b47fb45885..ce35b6a998 100644
> > > --- a/util/qemu-sockets.c
> > > +++ b/util/qemu-sockets.c
> > > @@ -44,7 +44,6 @@
> > >  # define AI_NUMERICSERV 0
> > >  #endif
> > > 
> > > -
> > >  static int inet_getport(struct addrinfo *e)
> > >  {
> > >      struct sockaddr_in *i4;
> > > @@ -149,6 +148,31 @@ int
> > > inet_ai_family_from_address(InetSocketAddress *addr,
> > >      return PF_UNSPEC;
> > >  }
> > > 
> > > +static int
> > > +check_ai_v4mapped(const char *host, const char *port, struct addrinfo 
> > > *ai,
> > > +               struct addrinfo *res)
> > > +{
> > > +    static int useV4Mapped = -1;
> > > +    int rc;
> > > +
> > > +    /* At least FreeBSD and OS-X 10.6 declare AI_V4MAPPED but
> > > +     * then don't implement it in their getaddrinfo().
> > > +     * Unconditionally deselect AI_V4MAPPED option upon
> > > +     * getaddrinfo() failure, the next call to it will have to
> > > +     * do the error handling.
> > > +     */
> > > +    if (atomic_read(&useV4Mapped) == -1) {
> > > +        rc = getaddrinfo(host, port, ai, &res);
> > > +        if (rc == 0 && (ai->ai_flags & AI_V4MAPPED)) {
> > > +            atomic_set(&useV4Mapped, 1);
> > > +        } else {
> > > +            atomic_set(&useV4Mapped, 0);

This is treating every single failure of getaddrinfo as a sign to turn
off AI_V4MAPPED, which is really bad. I'm not a huge fan of doing the
extra getaddrinfo() call, even though it is protected behind the atomic
var. I'd prefer to just keep current style of checking the result after
the main call to getaddrinfo() - just extend that to the dgram case too.
A single 'static int useV4Mapped = 1' could still be used in the global
namespace though.

> > > +        }
> > > +    }
> > > +
> > > +    return useV4Mapped;
> > > +}
> > > +
> > >  static int create_fast_reuse_socket(struct addrinfo *e)
> > >  {
> > >      int slisten = qemu_socket(e->ai_family, e->ai_socktype,
> > > e->ai_protocol);
> > > @@ -378,14 +402,10 @@ static struct addrinfo
> > > *inet_parse_connect_saddr(InetSocketAddress *saddr,
> > >      struct addrinfo ai, *res;
> > >      int rc;
> > >      Error *err = NULL;
> > > -    static int useV4Mapped = 1;
> > > 
> > >      memset(&ai, 0, sizeof(ai));
> > > 
> > > -    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> > > -    if (atomic_read(&useV4Mapped)) {
> > > -        ai.ai_flags |= AI_V4MAPPED;
> > > -    }
> > > +    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG | AI_V4MAPPED;
> > >      ai.ai_family = inet_ai_family_from_address(saddr, &err);
> > >      ai.ai_socktype = SOCK_STREAM;
> > > 
> > > @@ -399,21 +419,11 @@ static struct addrinfo
> > > *inet_parse_connect_saddr(InetSocketAddress *saddr,
> > >          return NULL;
> > >      }
> > > 
> > > -    /* lookup */
> > > -    rc = getaddrinfo(saddr->host, saddr->port, &ai, &res);
> > > -
> > > -    /* At least FreeBSD and OS-X 10.6 declare AI_V4MAPPED but
> > > -     * then don't implement it in their getaddrinfo(). Detect
> > > -     * this and retry without the flag since that's preferrable
> > > -     * to a fatal error
> > > -     */
> > > -    if (rc == EAI_BADFLAGS &&
> > > -        (ai.ai_flags & AI_V4MAPPED)) {
> > > -        atomic_set(&useV4Mapped, 0);
> > > +    if ((check_ai_v4mapped(saddr->host, saddr->port, &ai, res)) == 0) {
> > >          ai.ai_flags &= ~AI_V4MAPPED;
> > > -        rc = getaddrinfo(saddr->host, saddr->port, &ai, &res);
> > >      }
> > > -    if (rc != 0) {
> > > +
> > > +    if ((rc = getaddrinfo(saddr->host, saddr->port, &ai, &res)) != 0) {
> > >          error_setg(errp, "address resolution failed for %s:%s: %s",
> > >                     saddr->host, saddr->port, gai_strerror(rc));
> > >          return NULL;
> > > @@ -469,7 +479,7 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
> > > 
> > >      /* lookup peer addr */
> > >      memset(&ai,0, sizeof(ai));
> > > -    ai.ai_flags = AI_CANONNAME | AI_V4MAPPED | AI_ADDRCONFIG;
> > > +    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG | AI_V4MAPPED;
> > 
> > Gratuitous re-ordering of code with no functional change.
> 
> Agree, can change it.
> 
> > 
> > >      ai.ai_family = inet_ai_family_from_address(sraddr, &err);
> > >      ai.ai_socktype = SOCK_DGRAM;
> > > 
> > > @@ -488,6 +498,10 @@ static int inet_dgram_saddr(InetSocketAddress 
> > > *sraddr,
> > >          goto err;
> > >      }
> > > 
> > > +    if ((check_ai_v4mapped(addr, port, &ai, &peer)) == 0) {
> > > +        ai.ai_flags &= ~AI_V4MAPPED;
> > > +    }
> > > +
> > >      if ((rc = getaddrinfo(addr, port, &ai, &peer)) != 0) {
> > >          error_setg(errp, "address resolution failed for %s:%s: %s", addr,
> > > port,
> > >                     gai_strerror(rc));

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|