Re: [Qemu-devel] [PATCH v2 2/5] sockets: remove use of QemuOpts from socket_listen

[Daniel P. Berrange]

On Tue, Nov 17, 2015 at 03:22:04PM -0700, Eric Blake wrote:
> On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
> > The socket_listen method accepts a QAPI SocketAddress object
> > which it then turns into QemuOpts before calling the
> > inet_listen_opts/unix_listen_opts helper methods. By
> > converting the latter to use QAPI SocketAddress directly,
> > the QemuOpts conversion step can be eliminated
> > 
> > This also fixes the problem where ipv4=off && ipv6=off
> > would be treated the same as ipv4=on && ipv6=on
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  util/qemu-sockets.c | 144 
> > +++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 87 insertions(+), 57 deletions(-)
> > 
> 
> > +++ b/util/qemu-sockets.c
> > @@ -114,36 +114,68 @@ NetworkAddressFamily inet_netfamily(int family)
> >      return NETWORK_ADDRESS_FAMILY_UNKNOWN;
> >  }
> >  
> > -static int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
> > +/*
> > + * Matrix we're trying to apply
> > + *
> > + *  ipv4  ipv6   family
> > + *   -     -       PF_UNSPEC
> > + *   -     f       PF_INET
> > + *   -     t       PF_INET6
> > + *   f     -       PF_INET6
> > + *   f     f       <error>
> > + *   f     t       PF_INET6
> > + *   t     -       PF_INET
> > + *   t     f       PF_INET
> 
> These I understand,
> 
> > + *   t     t       PF_INET6
> 
> but why is this one PF_INET6 instead of PF_UNSPEC?

If you use PF_UNSPEC, then the addresses we listen on will be automatically
deteremined by results of the DNS lookup. ie if DNS only returns an IPv4
address, it won't listen on IPv6.  When the user has said ipv6=on, then
they expect to get an error if it was not possible to listen on IPv6. So
we must use PF_INET6 here to ensure that error, otherwise ipv6=on & ipv4=on
would be no different from ipv6=- & ipv4=-.

> 
> > + */
> > +static int inet_ai_family_from_address(InetSocketAddress *addr,
> > +                                       Error **errp)
> > +{
> > +    if (addr->has_ipv6 && addr->has_ipv4 &&
> > +        !addr->ipv6 && !addr->ipv4) {
> > +        error_setg(errp, "Cannot disable IPv4 and IPv6 at same time");
> > +        return PF_UNSPEC;
> > +    }
> > +    if ((addr->has_ipv6 && addr->ipv6) || (addr->has_ipv4 && !addr->ipv4)) 
> > {
> > +        return PF_INET6;
> > +    }
> > +    if ((addr->has_ipv4 && addr->ipv4) || (addr->has_ipv6 && !addr->ipv6)) 
> > {
> > +        return PF_INET;
> > +    }
> > +    return PF_UNSPEC;
> 
> This logic matches the matrix as listed, even if I'm not positive that
> the matrix is correct.  If we want PF_UNSPEC when both v4 and v6 are
> explicitly requested (as in, pick whichever works), then I think it
> should be something like:
> 
> if (addr->has_ipv6 && addr->has_ipv4 &&
>     addr->ipv6 == addr->ipv4) {
>     if (!addr->ipv6) {
>         error_setg(errp, "cannot disable IPv4 and IPv6 at the hsame time");
>     }
>     return PF_UNSPEC;
> }
> if ((addr->has_ipv6 && addr->ipv6) || (addr->has_ipv4 && !addr->ipv4)) {
>     return PF_INET6;
> }
> assert((addr->has_ipv4 && addr->ipv4) || (addr->has_ipv6 && !addr->ipv6));
> return PF_INET;
> 
> > @@ -219,13 +251,15 @@ listen:
> >          freeaddrinfo(res);
> >          return -1;
> >      }
> > -    qemu_opt_set(opts, "host", uaddr, &error_abort);
> > -    qemu_opt_set_number(opts, "port", inet_getport(e) - port_offset,
> > -                        &error_abort);
> > -    qemu_opt_set_bool(opts, "ipv6", e->ai_family == PF_INET6,
> > -                      &error_abort);
> > -    qemu_opt_set_bool(opts, "ipv4", e->ai_family != PF_INET6,
> > -                      &error_abort);
> > +    if (update_addr) {
> > +        g_free(saddr->host);
> > +        saddr->host = g_strdup(uaddr);
> > +        g_free(saddr->port);
> > +        saddr->port = g_strdup_printf("%d",
> > +                                      inet_getport(e) - port_offset);
> > +        saddr->has_ipv6 = saddr->ipv6 = e->ai_family == PF_INET6;
> > +        saddr->has_ipv4 = saddr->ipv4 = e->ai_family != PF_INET6;
> 
> Should we handle PF_UNSPEC specifically, maybe by having the has_ipv6
> assignment based on e->ai_family != PF_INET?

The returne e->ai_family from getaddrinfo will never have a value
of PF_UNSPEC - that's an input only value. So we're OK to assume
we'll have PF_INET6 and PF_INET only.  Well at least until someone
invents IPv7 but I'll let my great grandchildren deal with that
problem ;-)

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|