Re: [Qemu-devel] [PATCH 04/10] vnc: switch to QemuOpts, allow multiple servers

[Markus Armbruster <armbru@redhat.com>]

Gerd Hoffmann <kraxel@redhat.com> writes:

> This patch switches vnc over to QemuOpts, and it (more or less
> as side effect) allows multiple vnc server instances.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

I'm afraid this broke monitor command change vnc.

Reproducer

    Terminal 1:

        $ qemu -nodefaults -S -display vnc=:0 -monitor stdio
        QEMU 2.2.50 monitor - type 'help' for more information

    Terminal 2:

        $ vncviewer :0 

    Terminal 1:

        (qemu) change vnc :1

    Terminal 3:

        $ vncviewer :1

Before this patch, vncviewer works both times.  The second one kills the
first one.

After this patch, the first one still works, but the second one fails.
netstat shows nobody's listening on the port.

Speculation on possible cause inline.

Furthermore, the conversion to QemuOpts makes VNC visible in
-writeconfig, but if you -readconfig it back, it doesn't quite work.

With -display vnc=:0, -writeconfig produces

    [vnc]
      vnc = ":0"

If I append that to the config file I -readconfig, I a working VNC
display on :0 (good), but I also get an SDL display (not good).

> ---
>  include/ui/console.h |   4 +-
>  qmp.c                |  15 ++-
>  ui/vnc.c             | 270 ++++++++++++++++++++++++++++++++-------------------
>  vl.c                 |  42 +++-----
>  4 files changed, 199 insertions(+), 132 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 5ff2e27..887ed91 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -328,12 +328,14 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
>  
>  /* vnc.c */
>  void vnc_display_init(const char *id);
> -void vnc_display_open(const char *id, const char *display, Error **errp);
> +void vnc_display_open(const char *id, Error **errp);
>  void vnc_display_add_client(const char *id, int csock, bool skipauth);
>  char *vnc_display_local_addr(const char *id);
>  #ifdef CONFIG_VNC
>  int vnc_display_password(const char *id, const char *password);
>  int vnc_display_pw_expire(const char *id, time_t expires);
> +QemuOpts *vnc_parse_func(const char *str);
> +int vnc_init_func(QemuOpts *opts, void *opaque);
>  #else
>  static inline int vnc_display_password(const char *id, const char *password)
>  {
> diff --git a/qmp.c b/qmp.c
> index 0b4f131..963305c 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -368,7 +368,20 @@ void qmp_change_vnc_password(const char *password, Error **errp)
>  
>  static void qmp_change_vnc_listen(const char *target, Error **errp)
>  {
> -    vnc_display_open(NULL, target, errp);
> +    QemuOptsList *olist = qemu_find_opts("vnc");
> +    QemuOpts *opts;
> +
> +    if (strstr(target, "id=")) {
> +        error_setg(errp, "id not supported");
> +        return;
> +    }

Aside: this is unclean.  Could we somehow test qemu_opts_id() instead?

> +

This deletes the QemuOpts with id=default:

> +    opts = qemu_opts_find(olist, "default");
> +    if (opts) {
> +        qemu_opts_del(opts);
> +    }

But this creates one without id:

> +    opts = vnc_parse_func(target);

The first argument orders vnc_display_open() to look for QemuOpts with
id=default, which doesn't exist anymore:

> +    vnc_display_open("default", errp);

>  }
>  
>  static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
[...]