From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42580) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyzfC-0002Mu-ES for qemu-devel@nongnu.org; Wed, 18 Nov 2015 05:08:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zyzf8-0005He-Iy for qemu-devel@nongnu.org; Wed, 18 Nov 2015 05:08:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43767) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zyzf8-0005HA-Bk for qemu-devel@nongnu.org; Wed, 18 Nov 2015 05:08:26 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 66F42C0BB281 for ; Wed, 18 Nov 2015 10:08:25 +0000 (UTC) Date: Wed, 18 Nov 2015 10:08:21 +0000 From: "Daniel P. Berrange" Message-ID: <20151118100821.GB27591@redhat.com> References: <1447779624-21625-1-git-send-email-berrange@redhat.com> <1447779624-21625-3-git-send-email-berrange@redhat.com> <564BA88C.1010403@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <564BA88C.1010403@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 2/5] sockets: remove use of QemuOpts from socket_listen Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Paolo Bonzini , qemu-devel@nongnu.org, Gerd Hoffmann 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 > > --- > > 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 > > + * 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 :|