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

["Daniel P. Berrangé" <berrange@redhat.com>]

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