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

[Eric Blake <eblake@redhat.com>]

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

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


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

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