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

[Daniel P. Berrange]

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.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  ui/vnc.c | 161 
> > ++++++++++++++++++++++++++++++++++++---------------------------
> >  1 file changed, 91 insertions(+), 70 deletions(-)
> > 
> > @@ -3539,44 +3535,83 @@ void vnc_display_open(const char *id, Error **errp)
> >          return;
> >      }
> >  
> > -    sopts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
> > -    wsopts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
> > -
> >      h = strrchr(vnc, ':');
> >      if (h) {
> > -        char *host;
> >          size_t hlen = h - vnc;
> >  
> > -        if (vnc[0] == '[' && vnc[hlen - 1] == ']') {
> > -            host = g_strndup(vnc + 1, hlen - 2);
> > +        const char *websocket = qemu_opt_get(opts, "websocket");
> > +        int to = qemu_opt_get_number(opts, "to", 0);
> 
> We really ought to improve the qapi notion of InetSocketAddress to allow
> { 'alternate':'StrOrInt', 'data': { 'i':'int', 's':'str' } } rather than
> only 'str' for port.  But that's a project for another day. I guess
> things get a bit weird if "to" is present but doesn't parse as a number,
> but I don't think your patch is making things any worse.
> 
> > +
> > +        if (strncmp(vnc, "unix:", 5) == 0) {
> > +            saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
> > +            saddr->q_unix = g_new0(UnixSocketAddress, 1);
> 
> More clashes with my pending qapi patches; rebase fun for one of us :)
> 
> 
> > +            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.

I'll be so happy to finally kill QemuOpts from the sockets code and
use SocketAddress everywhere....

> 
> 
> > @@ -3770,37 +3795,32 @@ void vnc_display_open(const char *id, Error **errp)
> >          int csock;
> >          vs->lsock = -1;
> >          vs->lwebsock = -1;
> > -        if (strncmp(vnc, "unix:", 5) == 0) {
> > -            csock = unix_connect(vnc+5, errp);
> > -        } else {
> > -            csock = inet_connect(vnc, errp);
> > +        if (vs->ws_enabled) {
> > +            error_setg(errp, "Cannot use websockets in reverse mode");
> > +            goto fail;
> >          }
> > +        csock = socket_connect(saddr, errp,
> > +                               NULL, NULL);
> 
> Looks like the line-wrapping isn't needed here.
> 
> Overall, looks like a sane conversion.
> 
> Reviewed-by: Eric Blake <address@hidden>


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