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

[Daniel P. Berrange]

On Wed, Nov 18, 2015 at 08:44:28AM -0700, Eric Blake wrote:
> On 11/18/2015 03:08 AM, Daniel P. Berrange wrote:
> > 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
> >>>
> 
> >>> + *  ipv4  ipv6   family
> >>> + *   -     -       PF_UNSPEC
> 
> This says I have no preference, so pick the one that works.
> 
> >>> + *   -     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,
> 
> Generally to mean "I specifically requested this" or "I specifically
> don't want that", where there is no collision.
> 
> >>
> >>> + *   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=-.
> 
> But if I'm specifically requesting that both families be used, either
> that should be an error (we can't honor two families at once) or it
> should be allowed (use the family that makes sense), not a synonym for
> ipv6-only.

Yes, you are right that this code means  ipv6=t & ipv4=t is
essentially equivalent to ipv6=t & ipv4=-, but that is a
limitation of getaddrinfo().

To address this semantic flaw, when ipv6=t, then we need
better handling of the IPV6_V6ONLY flag to take account
of ipv4= setting, and then actually verify whether ipv4
really was enabled when we asked for it.  This is a
pre-existing bug that my patch is not making worse. I
will have a think about fixing it separately.

And yes, we so badly need a unit test to validate all
this logic, which I also want to look into...

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