Re: [Qemu-devel] [PATCH v2 04/16] ui: convert VNC startup code to use SocketAddress

[Daniel P. Berrange]

On Tue, Oct 20, 2015 at 09:36:29PM +0100, Peter Maydell wrote:
> On 20 October 2015 at 19:50, Daniel P. Berrange <address@hidden> wrote:
> > On Tue, Oct 20, 2015 at 06:01:37PM +0100, Peter Maydell wrote:
> >> On 20 October 2015 at 14:39, Daniel P. Berrange <address@hidden> wrote:
> >> > On Mon, Oct 19, 2015 at 04:20:09PM -0600, Eric Blake wrote:
> >> >> On 10/12/2015 05:14 AM, Daniel P. Berrange wrote:
> >> >> > The VNC code is currently using QemuOpts to configure the
> >> >> > sockets connections / listeners it needs. Convert it to
> >> >> > use SocketAddress to bring it in line with modern QAPI
> >> >> > based code elsewhere in QEMU.
> >>
> >> >>
> >> >> > +            if (to) {
> >> >> > +                saddr->inet->has_to = true;
> >> >> > +                saddr->inet->to = to;
> >> >> > +            }
> >> >> > +            saddr->inet->ipv4 = saddr->inet->has_ipv4 = has_ipv4;
> >> >> > +            saddr->inet->ipv6 = saddr->inet->has_ipv6 = has_ipv6;
> >> >>
> >> >> Do we want to specify has_ipvX as true even when setting inet->ipvX to
> >> >> false?
> >> >
> >> > This saddr instance is passed into 'socket_connect' which then
> >> > converts it back into a QemuOpts object using
> >> >
> >> >     bool ipv4 = addr->has_ipv4 && addr->ipv4;
> >> >
> >> > So, we don't need to set has_ipvX to true, when ipvX is false.
> >>
> >> That is not the *only* thing that inet_addr_to_opts does with
> >> the has_* flags... this kind of thing can be the difference
> >> between "force ipv4" and "use ipv4 or ipv6".
> >
> > The original code which used QemuOpts directly had this logic:
> >
> > inet_listen_opts / inet_connect_opts use this logic:
> >
> >     ai.ai_family = PF_UNSPEC;
> >     ....
> >     if (qemu_opt_get_bool(opts, "ipv4", 0))
> >         ai.ai_family = PF_INET;
> >     if (qemu_opt_get_bool(opts, "ipv6", 0))
> >         ai.ai_family = PF_INET6;
> >
> > IOW, these methods treat ipv4 being omitted, as equivalent to ipv4=off.
> >
> > Thus the existing VNC -> qemu-sockets code has this logic:
> >
> >      VNC      | qemu-sockets
> >    QemuOpts   |  ai_family
> >  ----------------------------
> >   ipv4   ipv6 |
> >  ----------------------------
> >   -      -    |  PF_UNSPEC
> >   -      off  |  PF_UNSPEC
> >   -      on   |  PF_INET6
> >   off    -    |  PF_UNSPEC
> >   off    off  |  PF_UNSPEC
> >   off    on   |  PF_INET6
> >   on     -    |  PF_INET
> >   on     off  |  PF_INET
> >   on     on   |  PF_INET6
> >
> >
> > My patch, which also does not distinguish ipv4=off from ipv4 omitted
> > has the following logic, at various stages of conversion between
> > QemuOpts & QAPI SocketAddress:
> >
> >
> >      VNC      |          VNC                 | qemu-sockets | qemu-sockets
> >    QemuOpts   |      QAPI  SocketAddress     |  QemuOpts    |  ai_family
> >  -------------------------------------------------------------------------
> >   ipv4   ipv6 |  has_ipv4 ipv4 has_ipv6 ipv6 |   ipv4 ipv6  |
> >  -------------------------------------------------------------------------
> >   -      -    |  f        f    f        f    |  -     -     | PF_UNSPEC
> >   -      off  |  f        f    f        f    |  -     -     | PF_UNSPEC
> >   -      on   |  f        f    t        t    |  off   on    | PF_INET6
> >   off    -    |  f        f    f        f    |  -     -     | PF_UNSPEC
> >   off    off  |  f        f    f        f    |  -     -     | PF_UNSPEC
> >   off    on   |  f        f    t        t    |  off   on    | PF_INET6
> >   on     -    |  t        t    f        f    |  on    off   | PF_INET
> >   on     off  |  t        t    f        f    |  on    off   | PF_INET
> >   on     on   |  t        t    t        t    |  on    on    | PF_INET6
> >
> > So, unless I've missed something, the final ai_family is set the same
> > way before & after my patch, for all combinations of VNC ipv4/ipv6
> > CLI options.
> 
> I think you're right that this patch doesn't change anything,
> but doesn't it mean that vnc still has one of the bugs we fixed in
> commit b77e7c8e99f9a: if you say 'ipv6=off' it will give you a
> PF_UNSPEC rather than an PF_INET socket? (In particular on Windows
> PF_UNSPEC will get you ipv6, which is exactly what the user asked
> not to get...)
> 
> Since that's a pre-existing bug I guess we can fix it in a
> subsequent patch.

Hmm, yes, you are correct in that - I can certainly look at creating a
further patch to fix that bug.

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