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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3712 bytes --]

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 <berrange@redhat.com>
> ---
>  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?

> + */
> +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?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]