[PATCH v3 0/3] qapi/ui: change vnc addresses

[PATCH v3 0/3] qapi/ui: change vnc addresses

[Vladimir Sementsov-Ogievskiy]

Hi all!

v3: - instead of creating new qmp command add an argument to existing
      display-reload
    - also don't touch websockets for now. I'm not sure we need it.
      Additional argument for changing websockets may be added later on
      demand

Recently our customer requested a possibility to change VNC listen port
dynamically.

Happily in Rhel7-based Qemu we already have this possibility: through
deprecated "change" qmp command.

But since 6.0 "change" qmp command was removed, with recommendation to
use change-vnc-password or blockdev-change-medium instead. Of course,
neither of them allow change VNC listen port.

So, let's reimplement the possibility.

Note: now, reconnecting may trigger existing deadlock, as I described
in my message "Re: [PULL 09/11] ui/vnc: clipboard support":
 <973ddebe-14a9-4ba7-c389-7a97d6017237@virtuozzo.com>

Simple hack helps, but I'm not sure it's safe itself:

    diff --git a/ui/vnc.c b/ui/vnc.c
    index 69bbf3b6f6..8c6b378e2e 100644
    --- a/ui/vnc.c
    +++ b/ui/vnc.c
    @@ -1354,12 +1354,12 @@ void vnc_disconnect_finish(VncState *vs)
             /* last client gone */
             vnc_update_server_surface(vs->vd);
         }
    +    vnc_unlock_output(vs);
    +
         if (vs->cbpeer.update.notify) {
             qemu_clipboard_peer_unregister(&vs->cbpeer);
         }
     
    -    vnc_unlock_output(vs);
    -
         qemu_mutex_destroy(&vs->output_mutex);
         if (vs->bh != NULL) {
             qemu_bh_delete(vs->bh);

Vladimir Sementsov-Ogievskiy (3):
  ui/vnc: refactor arrays of addresses to SocketAddressList
  qapi/ui: display-reload: add possibility to change listen address
  avocado/vnc: add test_change_listen

 docs/about/removed-features.rst |   3 +-
 qapi/ui.json                    |   6 +-
 include/ui/console.h            |   2 +-
 monitor/qmp-cmds.c              |   4 +-
 ui/vnc.c                        | 166 ++++++++++++++++----------------
 tests/avocado/vnc.py            |  10 ++
 6 files changed, 100 insertions(+), 91 deletions(-)

-- 
2.31.1

[PATCH v3 1/3] ui/vnc: refactor arrays of addresses to SocketAddressList

[Vladimir Sementsov-Ogievskiy]

Let's use SocketAddressList instead of dynamic arrays.
Benefits:
 - Automatic cleanup: don't need specific freeing function and drop
   some gotos.
 - Less indirection: no triple asterix anymore!
 - Prepare for the following commit, which will reuse new interface of
   vnc_display_listen().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/vnc.c | 129 ++++++++++++++++++++++---------------------------------
 1 file changed, 51 insertions(+), 78 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 3ccd33dedc..fa0fb736d3 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3812,30 +3812,19 @@ static int vnc_display_get_address(const char *addrstr,
     return ret;
 }
 
-static void vnc_free_addresses(SocketAddress ***retsaddr,
-                               size_t *retnsaddr)
-{
-    size_t i;
-
-    for (i = 0; i < *retnsaddr; i++) {
-        qapi_free_SocketAddress((*retsaddr)[i]);
-    }
-    g_free(*retsaddr);
-
-    *retsaddr = NULL;
-    *retnsaddr = 0;
-}
-
 static int vnc_display_get_addresses(QemuOpts *opts,
                                      bool reverse,
-                                     SocketAddress ***retsaddr,
-                                     size_t *retnsaddr,
-                                     SocketAddress ***retwsaddr,
-                                     size_t *retnwsaddr,
+                                     SocketAddressList **saddr_list_ret,
+                                     SocketAddressList **wsaddr_list_ret,
                                      Error **errp)
 {
     SocketAddress *saddr = NULL;
     SocketAddress *wsaddr = NULL;
+    g_autoptr(SocketAddressList) saddr_list = NULL;
+    SocketAddressList **saddr_tail = &saddr_list;
+    SocketAddress *single_saddr = NULL;
+    g_autoptr(SocketAddressList) wsaddr_list = NULL;
+    SocketAddressList **wsaddr_tail = &wsaddr_list;
     QemuOptsIter addriter;
     const char *addr;
     int to = qemu_opt_get_number(opts, "to", 0);
@@ -3844,23 +3833,16 @@ static int vnc_display_get_addresses(QemuOpts *opts,
     bool ipv4 = qemu_opt_get_bool(opts, "ipv4", false);
     bool ipv6 = qemu_opt_get_bool(opts, "ipv6", false);
     int displaynum = -1;
-    int ret = -1;
-
-    *retsaddr = NULL;
-    *retnsaddr = 0;
-    *retwsaddr = NULL;
-    *retnwsaddr = 0;
 
     addr = qemu_opt_get(opts, "vnc");
     if (addr == NULL || g_str_equal(addr, "none")) {
-        ret = 0;
-        goto cleanup;
+        return 0;
     }
     if (qemu_opt_get(opts, "websocket") &&
         !qcrypto_hash_supports(QCRYPTO_HASH_ALG_SHA1)) {
         error_setg(errp,
                    "SHA1 hash support is required for websockets");
-        goto cleanup;
+        return -1;
     }
 
     qemu_opt_iter_init(&addriter, opts, "vnc");
@@ -3871,7 +3853,7 @@ static int vnc_display_get_addresses(QemuOpts *opts,
                                      ipv4, ipv6,
                                      &saddr, errp);
         if (rv < 0) {
-            goto cleanup;
+            return -1;
         }
         /* Historical compat - first listen address can be used
          * to set the default websocket port
@@ -3879,13 +3861,16 @@ static int vnc_display_get_addresses(QemuOpts *opts,
         if (displaynum == -1) {
             displaynum = rv;
         }
-        *retsaddr = g_renew(SocketAddress *, *retsaddr, *retnsaddr + 1);
-        (*retsaddr)[(*retnsaddr)++] = saddr;
+        QAPI_LIST_APPEND(saddr_tail, saddr);
     }
 
-    /* If we had multiple primary displays, we don't do defaults
-     * for websocket, and require explicit config instead. */
-    if (*retnsaddr > 1) {
+    if (saddr_list && !saddr_list->next) {
+        single_saddr = saddr_list->value;
+    } else {
+        /*
+         * If we had multiple primary displays, we don't do defaults
+         * for websocket, and require explicit config instead.
+         */
         displaynum = -1;
     }
 
@@ -3895,57 +3880,50 @@ static int vnc_display_get_addresses(QemuOpts *opts,
                                     has_ipv4, has_ipv6,
                                     ipv4, ipv6,
                                     &wsaddr, errp) < 0) {
-            goto cleanup;
+            return -1;
         }
 
         /* Historical compat - if only a single listen address was
          * provided, then this is used to set the default listen
          * address for websocket too
          */
-        if (*retnsaddr == 1 &&
-            (*retsaddr)[0]->type == SOCKET_ADDRESS_TYPE_INET &&
+        if (single_saddr &&
+            single_saddr->type == SOCKET_ADDRESS_TYPE_INET &&
             wsaddr->type == SOCKET_ADDRESS_TYPE_INET &&
             g_str_equal(wsaddr->u.inet.host, "") &&
-            !g_str_equal((*retsaddr)[0]->u.inet.host, "")) {
+            !g_str_equal(single_saddr->u.inet.host, "")) {
             g_free(wsaddr->u.inet.host);
-            wsaddr->u.inet.host = g_strdup((*retsaddr)[0]->u.inet.host);
+            wsaddr->u.inet.host = g_strdup(single_saddr->u.inet.host);
         }
 
-        *retwsaddr = g_renew(SocketAddress *, *retwsaddr, *retnwsaddr + 1);
-        (*retwsaddr)[(*retnwsaddr)++] = wsaddr;
+        QAPI_LIST_APPEND(wsaddr_tail, wsaddr);
     }
 
-    ret = 0;
- cleanup:
-    if (ret < 0) {
-        vnc_free_addresses(retsaddr, retnsaddr);
-        vnc_free_addresses(retwsaddr, retnwsaddr);
-    }
-    return ret;
+    *saddr_list_ret = g_steal_pointer(&saddr_list);
+    *wsaddr_list_ret = g_steal_pointer(&wsaddr_list);
+    return 0;
 }
 
 static int vnc_display_connect(VncDisplay *vd,
-                               SocketAddress **saddr,
-                               size_t nsaddr,
-                               SocketAddress **wsaddr,
-                               size_t nwsaddr,
+                               SocketAddressList *saddr_list,
+                               SocketAddressList *wsaddr_list,
                                Error **errp)
 {
     /* connect to viewer */
     QIOChannelSocket *sioc = NULL;
-    if (nwsaddr != 0) {
+    if (wsaddr_list) {
         error_setg(errp, "Cannot use websockets in reverse mode");
         return -1;
     }
-    if (nsaddr != 1) {
+    if (!saddr_list || saddr_list->next) {
         error_setg(errp, "Expected a single address in reverse mode");
         return -1;
     }
     /* TODO SOCKET_ADDRESS_TYPE_FD when fd has AF_UNIX */
-    vd->is_unix = saddr[0]->type == SOCKET_ADDRESS_TYPE_UNIX;
+    vd->is_unix = saddr_list->value->type == SOCKET_ADDRESS_TYPE_UNIX;
     sioc = qio_channel_socket_new();
     qio_channel_set_name(QIO_CHANNEL(sioc), "vnc-reverse");
-    if (qio_channel_socket_connect_sync(sioc, saddr[0], errp) < 0) {
+    if (qio_channel_socket_connect_sync(sioc, saddr_list->value, errp) < 0) {
         object_unref(OBJECT(sioc));
         return -1;
     }
@@ -3956,20 +3934,18 @@ static int vnc_display_connect(VncDisplay *vd,
 
 
 static int vnc_display_listen(VncDisplay *vd,
-                              SocketAddress **saddr,
-                              size_t nsaddr,
-                              SocketAddress **wsaddr,
-                              size_t nwsaddr,
+                              SocketAddressList *saddr_list,
+                              SocketAddressList *wsaddr_list,
                               Error **errp)
 {
-    size_t i;
+    SocketAddressList *el;
 
-    if (nsaddr) {
+    if (saddr_list) {
         vd->listener = qio_net_listener_new();
         qio_net_listener_set_name(vd->listener, "vnc-listen");
-        for (i = 0; i < nsaddr; i++) {
+        for (el = saddr_list; el; el = el->next) {
             if (qio_net_listener_open_sync(vd->listener,
-                                           saddr[i], 1,
+                                           el->value, 1,
                                            errp) < 0)  {
                 return -1;
             }
@@ -3979,12 +3955,12 @@ static int vnc_display_listen(VncDisplay *vd,
                                          vnc_listen_io, vd, NULL);
     }
 
-    if (nwsaddr) {
+    if (wsaddr_list) {
         vd->wslistener = qio_net_listener_new();
         qio_net_listener_set_name(vd->wslistener, "vnc-ws-listen");
-        for (i = 0; i < nwsaddr; i++) {
+        for (el = wsaddr_list; el; el = el->next) {
             if (qio_net_listener_open_sync(vd->wslistener,
-                                           wsaddr[i], 1,
+                                           el->value, 1,
                                            errp) < 0)  {
                 return -1;
             }
@@ -4002,8 +3978,8 @@ void vnc_display_open(const char *id, Error **errp)
 {
     VncDisplay *vd = vnc_display_find(id);
     QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
-    SocketAddress **saddr = NULL, **wsaddr = NULL;
-    size_t nsaddr, nwsaddr;
+    g_autoptr(SocketAddressList) saddr_list = NULL;
+    g_autoptr(SocketAddressList) wsaddr_list = NULL;
     const char *share, *device_id;
     QemuConsole *con;
     bool password = false;
@@ -4028,8 +4004,8 @@ void vnc_display_open(const char *id, Error **errp)
     }
 
     reverse = qemu_opt_get_bool(opts, "reverse", false);
-    if (vnc_display_get_addresses(opts, reverse, &saddr, &nsaddr,
-                                  &wsaddr, &nwsaddr, errp) < 0) {
+    if (vnc_display_get_addresses(opts, reverse, &saddr_list, &wsaddr_list,
+                                  errp) < 0) {
         goto fail;
     }
 
@@ -4211,16 +4187,16 @@ void vnc_display_open(const char *id, Error **errp)
     }
     qkbd_state_set_delay(vd->kbd, key_delay_ms);
 
-    if (saddr == NULL) {
-        goto cleanup;
+    if (saddr_list == NULL) {
+        return;
     }
 
     if (reverse) {
-        if (vnc_display_connect(vd, saddr, nsaddr, wsaddr, nwsaddr, errp) < 0) {
+        if (vnc_display_connect(vd, saddr_list, wsaddr_list, errp) < 0) {
             goto fail;
         }
     } else {
-        if (vnc_display_listen(vd, saddr, nsaddr, wsaddr, nwsaddr, errp) < 0) {
+        if (vnc_display_listen(vd, saddr_list, wsaddr_list, errp) < 0) {
             goto fail;
         }
     }
@@ -4229,14 +4205,11 @@ void vnc_display_open(const char *id, Error **errp)
         vnc_display_print_local_addr(vd);
     }
 
- cleanup:
-    vnc_free_addresses(&saddr, &nsaddr);
-    vnc_free_addresses(&wsaddr, &nwsaddr);
+    /* Success */
     return;
 
 fail:
     vnc_display_close(vd);
-    goto cleanup;
 }
 
 void vnc_display_add_client(const char *id, int csock, bool skipauth)
-- 
2.31.1

Re: [PATCH v3 1/3] ui/vnc: refactor arrays of addresses to SocketAddressList

[Daniel P. Berrangé]

On Tue, Jan 18, 2022 at 05:09:07PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Let's use SocketAddressList instead of dynamic arrays.
> Benefits:
>  - Automatic cleanup: don't need specific freeing function and drop
>    some gotos.
>  - Less indirection: no triple asterix anymore!
>  - Prepare for the following commit, which will reuse new interface of
>    vnc_display_listen().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/vnc.c | 129 ++++++++++++++++++++++---------------------------------
>  1 file changed, 51 insertions(+), 78 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v3 2/3] qapi/ui: display-reload: add possibility to change listen address

[Vladimir Sementsov-Ogievskiy]

Add possibility to change addresses where VNC server listens for new
connections. Prior to 6.0 this functionality was available through
'change' qmp command which was deleted.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/about/removed-features.rst |  3 ++-
 qapi/ui.json                    |  6 +++++-
 include/ui/console.h            |  2 +-
 monitor/qmp-cmds.c              |  4 +---
 ui/vnc.c                        | 37 ++++++++++++++++++++++++++-------
 5 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 4c4da20d0f..b92626a74e 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -355,7 +355,8 @@ documentation of ``query-hotpluggable-cpus`` for additional details.
 ``change`` (removed in 6.0)
 '''''''''''''''''''''''''''
 
-Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
+Use ``blockdev-change-medium`` or ``change-vnc-password`` or
+``display-reload`` instead.
 
 ``query-events`` (removed in 6.0)
 '''''''''''''''''''''''''''''''''
diff --git a/qapi/ui.json b/qapi/ui.json
index 9354f4c467..4c4448f378 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1293,12 +1293,16 @@
 # Specify the VNC reload options.
 #
 # @tls-certs: reload tls certs or not.
+# @addresses: If specified, change set of addresses
+#             to listen for connections. Addresses configured
+#             for websockets are not touched. (since 7.0)
 #
 # Since: 6.0
 #
 ##
 { 'struct': 'DisplayReloadOptionsVNC',
-  'data': { '*tls-certs': 'bool' } }
+  'data': { '*tls-certs': 'bool',
+            '*addresses': ['SocketAddress'] } }
 
 ##
 # @DisplayReloadOptions:
diff --git a/include/ui/console.h b/include/ui/console.h
index f590819880..b052027915 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -512,7 +512,7 @@ int vnc_display_password(const char *id, const char *password);
 int vnc_display_pw_expire(const char *id, time_t expires);
 void vnc_parse(const char *str);
 int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
-bool vnc_display_reload_certs(const char *id,  Error **errp);
+bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp);
 
 /* input.c */
 int index_from_key(const char *key, size_t key_length);
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 14e3beeaaf..ad45baa12b 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -356,9 +356,7 @@ void qmp_display_reload(DisplayReloadOptions *arg, Error **errp)
     switch (arg->type) {
     case DISPLAY_RELOAD_TYPE_VNC:
 #ifdef CONFIG_VNC
-        if (arg->u.vnc.has_tls_certs && arg->u.vnc.tls_certs) {
-            vnc_display_reload_certs(NULL, errp);
-        }
+        vnc_display_reload(&arg->u.vnc, errp);
 #else
         error_setg(errp, "vnc is invalid, missing 'CONFIG_VNC'");
 #endif
diff --git a/ui/vnc.c b/ui/vnc.c
index fa0fb736d3..a86bd6335e 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -587,16 +587,10 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
     return prev;
 }
 
-bool vnc_display_reload_certs(const char *id, Error **errp)
+static bool vnc_display_reload_certs(VncDisplay *vd, Error **errp)
 {
-    VncDisplay *vd = vnc_display_find(id);
     QCryptoTLSCredsClass *creds = NULL;
 
-    if (!vd) {
-        error_setg(errp, "Can not find vnc display");
-        return false;
-    }
-
     if (!vd->tlscreds) {
         error_setg(errp, "vnc tls is not enabled");
         return false;
@@ -3973,6 +3967,35 @@ static int vnc_display_listen(VncDisplay *vd,
     return 0;
 }
 
+bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp)
+{
+    VncDisplay *vd = vnc_display_find(NULL);
+
+    if (!vd) {
+        error_setg(errp, "Can not find vnc display");
+        return false;
+    }
+
+    if (arg->has_tls_certs && arg->tls_certs) {
+        if (!vnc_display_reload_certs(vd, errp)) {
+            return false;
+        }
+    }
+
+    if (arg->has_addresses) {
+        if (vd->listener) {
+            qio_net_listener_disconnect(vd->listener);
+            object_unref(OBJECT(vd->listener));
+            vd->listener = NULL;
+        }
+
+        if (vnc_display_listen(vd, arg->addresses, NULL, errp) < 0) {
+            return false;
+        }
+    }
+
+    return true;
+}
 
 void vnc_display_open(const char *id, Error **errp)
 {
-- 
2.31.1

Re: [PATCH v3 2/3] qapi/ui: display-reload: add possibility to change listen address

[Daniel P. Berrangé]

On Tue, Jan 18, 2022 at 05:09:08PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Add possibility to change addresses where VNC server listens for new
> connections. Prior to 6.0 this functionality was available through
> 'change' qmp command which was deleted.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/about/removed-features.rst |  3 ++-
>  qapi/ui.json                    |  6 +++++-
>  include/ui/console.h            |  2 +-
>  monitor/qmp-cmds.c              |  4 +---
>  ui/vnc.c                        | 37 ++++++++++++++++++++++++++-------
>  5 files changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index 4c4da20d0f..b92626a74e 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -355,7 +355,8 @@ documentation of ``query-hotpluggable-cpus`` for additional details.
>  ``change`` (removed in 6.0)
>  '''''''''''''''''''''''''''
>  
> -Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
> +Use ``blockdev-change-medium`` or ``change-vnc-password`` or
> +``display-reload`` instead.
>  
>  ``query-events`` (removed in 6.0)
>  '''''''''''''''''''''''''''''''''
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 9354f4c467..4c4448f378 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1293,12 +1293,16 @@
>  # Specify the VNC reload options.
>  #
>  # @tls-certs: reload tls certs or not.
> +# @addresses: If specified, change set of addresses
> +#             to listen for connections. Addresses configured
> +#             for websockets are not touched. (since 7.0)
>  #
>  # Since: 6.0
>  #
>  ##
>  { 'struct': 'DisplayReloadOptionsVNC',
> -  'data': { '*tls-certs': 'bool' } }
> +  'data': { '*tls-certs': 'bool',
> +            '*addresses': ['SocketAddress'] } }

So I'm having second thoughts on this, because I think we in fact need to
distinguish between reloads of state related to existing configuration
vs applying changes to the actual configuration.

For example,  when I think about the 'tls-certs' option here, it
lets us reload the existing TLS credentials from disk. ie it lets
the user re-write the TLS file content on disk and then tell QEMU
to reload the files.

An equally valuable option would be to tell QEMU to simply use a
completely different set of TLS files on disk, rather than re-writing
in place.  We don't have this feature now, but I think we should
anticipate it in the design.

So I'm going to suggest that instead of 'display-reload', we should
have a general purpose 'display-update' command for modifying existing
config and , leave 'display-reload' purely for re-loading state, not
changing config.

Essentially 'display-reload' is about re-starting QEMU displays
with the same config, while 'display-update' is about restarting
with a new config.

This shouldn't be too much work to achieve in your patch. Just
clone everything related to the 'display-reload' QMP command
boilerplate, replacing 'reload' with 'update' throughout and
discarding the 'tls-certs' bits leaving only your 'addresses'
bit.

We can use this 'display-update' command for making pasword
and auth config changes possible too later.

>  
>  ##
>  # @DisplayReloadOptions:
> diff --git a/include/ui/console.h b/include/ui/console.h
> index f590819880..b052027915 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -512,7 +512,7 @@ int vnc_display_password(const char *id, const char *password);
>  int vnc_display_pw_expire(const char *id, time_t expires);
>  void vnc_parse(const char *str);
>  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
> -bool vnc_display_reload_certs(const char *id,  Error **errp);
> +bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp);
>  
>  /* input.c */
>  int index_from_key(const char *key, size_t key_length);
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 14e3beeaaf..ad45baa12b 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -356,9 +356,7 @@ void qmp_display_reload(DisplayReloadOptions *arg, Error **errp)
>      switch (arg->type) {
>      case DISPLAY_RELOAD_TYPE_VNC:
>  #ifdef CONFIG_VNC
> -        if (arg->u.vnc.has_tls_certs && arg->u.vnc.tls_certs) {
> -            vnc_display_reload_certs(NULL, errp);
> -        }
> +        vnc_display_reload(&arg->u.vnc, errp);
>  #else
>          error_setg(errp, "vnc is invalid, missing 'CONFIG_VNC'");
>  #endif
> diff --git a/ui/vnc.c b/ui/vnc.c
> index fa0fb736d3..a86bd6335e 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -587,16 +587,10 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
>      return prev;
>  }
>  
> -bool vnc_display_reload_certs(const char *id, Error **errp)
> +static bool vnc_display_reload_certs(VncDisplay *vd, Error **errp)
>  {
> -    VncDisplay *vd = vnc_display_find(id);
>      QCryptoTLSCredsClass *creds = NULL;
>  
> -    if (!vd) {
> -        error_setg(errp, "Can not find vnc display");
> -        return false;
> -    }
> -
>      if (!vd->tlscreds) {
>          error_setg(errp, "vnc tls is not enabled");
>          return false;
> @@ -3973,6 +3967,35 @@ static int vnc_display_listen(VncDisplay *vd,
>      return 0;
>  }
>  
> +bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp)
> +{
> +    VncDisplay *vd = vnc_display_find(NULL);
> +
> +    if (!vd) {
> +        error_setg(errp, "Can not find vnc display");
> +        return false;
> +    }
> +
> +    if (arg->has_tls_certs && arg->tls_certs) {
> +        if (!vnc_display_reload_certs(vd, errp)) {
> +            return false;
> +        }
> +    }
> +
> +    if (arg->has_addresses) {
> +        if (vd->listener) {
> +            qio_net_listener_disconnect(vd->listener);
> +            object_unref(OBJECT(vd->listener));
> +            vd->listener = NULL;
> +        }
> +
> +        if (vnc_display_listen(vd, arg->addresses, NULL, errp) < 0) {
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
>  
>  void vnc_display_open(const char *id, Error **errp)
>  {
> -- 
> 2.31.1
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 2/3] qapi/ui: display-reload: add possibility to change listen address

[Vladimir Sementsov-Ogievskiy]

09.02.2022 21:33, Daniel P. Berrangé wrote:
> On Tue, Jan 18, 2022 at 05:09:08PM +0100, Vladimir Sementsov-Ogievskiy wrote:
>> Add possibility to change addresses where VNC server listens for new
>> connections. Prior to 6.0 this functionality was available through
>> 'change' qmp command which was deleted.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/about/removed-features.rst |  3 ++-
>>   qapi/ui.json                    |  6 +++++-
>>   include/ui/console.h            |  2 +-
>>   monitor/qmp-cmds.c              |  4 +---
>>   ui/vnc.c                        | 37 ++++++++++++++++++++++++++-------
>>   5 files changed, 39 insertions(+), 13 deletions(-)
>>
>> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
>> index 4c4da20d0f..b92626a74e 100644
>> --- a/docs/about/removed-features.rst
>> +++ b/docs/about/removed-features.rst
>> @@ -355,7 +355,8 @@ documentation of ``query-hotpluggable-cpus`` for additional details.
>>   ``change`` (removed in 6.0)
>>   '''''''''''''''''''''''''''
>>   
>> -Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
>> +Use ``blockdev-change-medium`` or ``change-vnc-password`` or
>> +``display-reload`` instead.
>>   
>>   ``query-events`` (removed in 6.0)
>>   '''''''''''''''''''''''''''''''''
>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index 9354f4c467..4c4448f378 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -1293,12 +1293,16 @@
>>   # Specify the VNC reload options.
>>   #
>>   # @tls-certs: reload tls certs or not.
>> +# @addresses: If specified, change set of addresses
>> +#             to listen for connections. Addresses configured
>> +#             for websockets are not touched. (since 7.0)
>>   #
>>   # Since: 6.0
>>   #
>>   ##
>>   { 'struct': 'DisplayReloadOptionsVNC',
>> -  'data': { '*tls-certs': 'bool' } }
>> +  'data': { '*tls-certs': 'bool',
>> +            '*addresses': ['SocketAddress'] } }
> 
> So I'm having second thoughts on this, because I think we in fact need to
> distinguish between reloads of state related to existing configuration
> vs applying changes to the actual configuration.
> 
> For example,  when I think about the 'tls-certs' option here, it
> lets us reload the existing TLS credentials from disk. ie it lets
> the user re-write the TLS file content on disk and then tell QEMU
> to reload the files.
> 
> An equally valuable option would be to tell QEMU to simply use a
> completely different set of TLS files on disk, rather than re-writing
> in place.  We don't have this feature now, but I think we should
> anticipate it in the design.
> 
> So I'm going to suggest that instead of 'display-reload', we should
> have a general purpose 'display-update' command for modifying existing
> config and , leave 'display-reload' purely for re-loading state, not
> changing config.
> 
> Essentially 'display-reload' is about re-starting QEMU displays
> with the same config, while 'display-update' is about restarting
> with a new config.
> 
> This shouldn't be too much work to achieve in your patch. Just
> clone everything related to the 'display-reload' QMP command
> boilerplate, replacing 'reload' with 'update' throughout and
> discarding the 'tls-certs' bits leaving only your 'addresses'
> bit.

Yes, that's simple to do, I'll resend soon.

> 
> We can use this 'display-update' command for making pasword
> and auth config changes possible too later.
> 
>>   
>>   ##
>>   # @DisplayReloadOptions:
>> diff --git a/include/ui/console.h b/include/ui/console.h
>> index f590819880..b052027915 100644
>> --- a/include/ui/console.h
>> +++ b/include/ui/console.h
>> @@ -512,7 +512,7 @@ int vnc_display_password(const char *id, const char *password);
>>   int vnc_display_pw_expire(const char *id, time_t expires);
>>   void vnc_parse(const char *str);
>>   int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
>> -bool vnc_display_reload_certs(const char *id,  Error **errp);
>> +bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp);
>>   
>>   /* input.c */
>>   int index_from_key(const char *key, size_t key_length);
>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
>> index 14e3beeaaf..ad45baa12b 100644
>> --- a/monitor/qmp-cmds.c
>> +++ b/monitor/qmp-cmds.c
>> @@ -356,9 +356,7 @@ void qmp_display_reload(DisplayReloadOptions *arg, Error **errp)
>>       switch (arg->type) {
>>       case DISPLAY_RELOAD_TYPE_VNC:
>>   #ifdef CONFIG_VNC
>> -        if (arg->u.vnc.has_tls_certs && arg->u.vnc.tls_certs) {
>> -            vnc_display_reload_certs(NULL, errp);
>> -        }
>> +        vnc_display_reload(&arg->u.vnc, errp);
>>   #else
>>           error_setg(errp, "vnc is invalid, missing 'CONFIG_VNC'");
>>   #endif
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index fa0fb736d3..a86bd6335e 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -587,16 +587,10 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
>>       return prev;
>>   }
>>   
>> -bool vnc_display_reload_certs(const char *id, Error **errp)
>> +static bool vnc_display_reload_certs(VncDisplay *vd, Error **errp)
>>   {
>> -    VncDisplay *vd = vnc_display_find(id);
>>       QCryptoTLSCredsClass *creds = NULL;
>>   
>> -    if (!vd) {
>> -        error_setg(errp, "Can not find vnc display");
>> -        return false;
>> -    }
>> -
>>       if (!vd->tlscreds) {
>>           error_setg(errp, "vnc tls is not enabled");
>>           return false;
>> @@ -3973,6 +3967,35 @@ static int vnc_display_listen(VncDisplay *vd,
>>       return 0;
>>   }
>>   
>> +bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp)
>> +{
>> +    VncDisplay *vd = vnc_display_find(NULL);
>> +
>> +    if (!vd) {
>> +        error_setg(errp, "Can not find vnc display");
>> +        return false;
>> +    }
>> +
>> +    if (arg->has_tls_certs && arg->tls_certs) {
>> +        if (!vnc_display_reload_certs(vd, errp)) {
>> +            return false;
>> +        }
>> +    }
>> +
>> +    if (arg->has_addresses) {
>> +        if (vd->listener) {
>> +            qio_net_listener_disconnect(vd->listener);
>> +            object_unref(OBJECT(vd->listener));
>> +            vd->listener = NULL;
>> +        }
>> +
>> +        if (vnc_display_listen(vd, arg->addresses, NULL, errp) < 0) {
>> +            return false;
>> +        }
>> +    }
>> +
>> +    return true;
>> +}
>>   
>>   void vnc_display_open(const char *id, Error **errp)
>>   {
>> -- 
>> 2.31.1
>>
> 
> Regards,
> Daniel
> 


-- 
Best regards,
Vladimir

[PATCH v3 3/3] avocado/vnc: add test_change_listen

[Vladimir Sementsov-Ogievskiy]

Add simple test-case for new addresses argument of display-reload qmp
command.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/avocado/vnc.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/avocado/vnc.py b/tests/avocado/vnc.py
index 096432988f..936285a50b 100644
--- a/tests/avocado/vnc.py
+++ b/tests/avocado/vnc.py
@@ -51,3 +51,13 @@ def test_change_password(self):
         set_password_response = self.vm.qmp('change-vnc-password',
                                             password='new_password')
         self.assertEqual(set_password_response['return'], {})
+
+    def test_change_listen(self):
+        self.vm.add_args('-nodefaults', '-S', '-vnc', ':0')
+        self.vm.launch()
+        self.assertEqual(self.vm.qmp('query-vnc')['return']['service'], '5900')
+        res = self.vm.qmp('display-reload', type='vnc',
+                          addresses=[{'type': 'inet', 'host': '0.0.0.0',
+                                      'port': '5901'}])
+        self.assertEqual(res['return'], {})
+        self.assertEqual(self.vm.qmp('query-vnc')['return']['service'], '5901')
-- 
2.31.1

Re: [PATCH v3 3/3] avocado/vnc: add test_change_listen

[Daniel P. Berrangé]

On Tue, Jan 18, 2022 at 05:09:09PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Add simple test-case for new addresses argument of display-reload qmp
> command.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/avocado/vnc.py | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tests/avocado/vnc.py b/tests/avocado/vnc.py
> index 096432988f..936285a50b 100644
> --- a/tests/avocado/vnc.py
> +++ b/tests/avocado/vnc.py
> @@ -51,3 +51,13 @@ def test_change_password(self):
>          set_password_response = self.vm.qmp('change-vnc-password',
>                                              password='new_password')
>          self.assertEqual(set_password_response['return'], {})
> +
> +    def test_change_listen(self):
> +        self.vm.add_args('-nodefaults', '-S', '-vnc', ':0')
> +        self.vm.launch()
> +        self.assertEqual(self.vm.qmp('query-vnc')['return']['service'], '5900')

Add a check that socket connect(5900)  works here and connect(5901)
and connect(5902) fails...


> +        res = self.vm.qmp('display-reload', type='vnc',
> +                          addresses=[{'type': 'inet', 'host': '0.0.0.0',
> +                                      'port': '5901'}])

I'd suggest we provide multiple addresses to test multi address
changes on distinct IPs too

                           addresses=[{'type': 'inet', 'host': '0.0.0.0',
                                       'port': '5901'},
                                      {'type': 'inet', 'host': '127.0.0.1',
                                       'port': '5902'}]

> +        self.assertEqual(res['return'], {})
> +        self.assertEqual(self.vm.qmp('query-vnc')['return']['service'], '5901')

and connect(5900) fails here, and connect(5901) + connet(5902) works .


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3] qapi/ui: change vnc addresses

[Vladimir Sementsov-Ogievskiy]

Ping)

18.01.2022 19:09, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> v3: - instead of creating new qmp command add an argument to existing
>        display-reload
>      - also don't touch websockets for now. I'm not sure we need it.
>        Additional argument for changing websockets may be added later on
>        demand
> 
> Recently our customer requested a possibility to change VNC listen port
> dynamically.
> 
> Happily in Rhel7-based Qemu we already have this possibility: through
> deprecated "change" qmp command.
> 
> But since 6.0 "change" qmp command was removed, with recommendation to
> use change-vnc-password or blockdev-change-medium instead. Of course,
> neither of them allow change VNC listen port.
> 
> So, let's reimplement the possibility.
> 
> Note: now, reconnecting may trigger existing deadlock, as I described
> in my message "Re: [PULL 09/11] ui/vnc: clipboard support":
>   <973ddebe-14a9-4ba7-c389-7a97d6017237@virtuozzo.com>
> 
> Simple hack helps, but I'm not sure it's safe itself:
> 
>      diff --git a/ui/vnc.c b/ui/vnc.c
>      index 69bbf3b6f6..8c6b378e2e 100644
>      --- a/ui/vnc.c
>      +++ b/ui/vnc.c
>      @@ -1354,12 +1354,12 @@ void vnc_disconnect_finish(VncState *vs)
>               /* last client gone */
>               vnc_update_server_surface(vs->vd);
>           }
>      +    vnc_unlock_output(vs);
>      +
>           if (vs->cbpeer.update.notify) {
>               qemu_clipboard_peer_unregister(&vs->cbpeer);
>           }
>       
>      -    vnc_unlock_output(vs);
>      -
>           qemu_mutex_destroy(&vs->output_mutex);
>           if (vs->bh != NULL) {
>               qemu_bh_delete(vs->bh);
> 
> Vladimir Sementsov-Ogievskiy (3):
>    ui/vnc: refactor arrays of addresses to SocketAddressList
>    qapi/ui: display-reload: add possibility to change listen address
>    avocado/vnc: add test_change_listen
> 
>   docs/about/removed-features.rst |   3 +-
>   qapi/ui.json                    |   6 +-
>   include/ui/console.h            |   2 +-
>   monitor/qmp-cmds.c              |   4 +-
>   ui/vnc.c                        | 166 ++++++++++++++++----------------
>   tests/avocado/vnc.py            |  10 ++
>   6 files changed, 100 insertions(+), 91 deletions(-)
> 


-- 
Best regards,
Vladimir