Re: [PATCH v2 2/2] vnc: add qmp to support reload vnc tls certificates

[Daniel P . Berrangé]

On Fri, Jan 15, 2021 at 02:37:33PM +0100, Markus Armbruster wrote:
> Zihao Chang <changzihao1@huawei.com> writes:
> 
> > QEMU loads vnc tls certificates only when vm is started. This patch
> > provides a new qmp to reload vnc tls certificates without restart
> > vnc-server/VM.
> > {"execute": "reload-vnc-cert"}
> >
> > Signed-off-by: Zihao Chang <changzihao1@huawei.com>
> > ---
> >  include/ui/console.h |  1 +
> >  monitor/qmp-cmds.c   |  5 +++++
> >  qapi/ui.json         | 18 ++++++++++++++++++
> >  ui/vnc.c             | 24 ++++++++++++++++++++++++
> >  4 files changed, 48 insertions(+)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index 5dd21976a3..60a24a8bb5 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -441,6 +441,7 @@ int vnc_display_password(const char *id, const char 
> > *password);
> >  int vnc_display_pw_expire(const char *id, time_t expires);
> >  QemuOpts *vnc_parse(const char *str, Error **errp);
> >  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
> > +void vnc_display_reload_cert(const char *id,  Error **errp);
> 
> Make this return bool, please.
> 
> error.h's big comment:
> 
>  * = Rules =
>  *
>  * - Functions that use Error to report errors have an Error **errp
>  *   parameter.  It should be the last parameter, except for functions
>  *   taking variable arguments.
>  *
>  * - You may pass NULL to not receive the error, &error_abort to abort
>  *   on error, &error_fatal to exit(1) on error, or a pointer to a
>  *   variable containing NULL to receive the error.
>  *
>  * - Separation of concerns: the function is responsible for detecting
>  *   errors and failing cleanly; handling the error is its caller's
>  *   job.  Since the value of @errp is about handling the error, the
>  *   function should not examine it.
>  *
>  * - The function may pass @errp to functions it calls to pass on
>  *   their errors to its caller.  If it dereferences @errp to check
>  *   for errors, it must use ERRP_GUARD().
>  *
>  * - On success, the function should not touch *errp.  On failure, it
>  *   should set a new error, e.g. with error_setg(errp, ...), or
>  *   propagate an existing one, e.g. with error_propagate(errp, ...).
>  *
>  * - Whenever practical, also return a value that indicates success /
>  *   failure.  This can make the error checking more concise, and can
>  *   avoid useless error object creation and destruction.  Note that
>  *   we still have many functions returning void.  We recommend
>  *   • bool-valued functions return true on success / false on failure,
>  *   • pointer-valued functions return non-null / null pointer, and
>  *   • integer-valued functions return non-negative / negative.
> 
> >  
> >  /* 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 34f7e75b7b..84f2b74ea8 100644
> > --- a/monitor/qmp-cmds.c
> > +++ b/monitor/qmp-cmds.c
> > @@ -287,6 +287,11 @@ static void qmp_change_vnc(const char *target, bool 
> > has_arg, const char *arg,
> >          qmp_change_vnc_listen(target, errp);
> >      }
> >  }
> > +
> > +void qmp_reload_vnc_cert(Error **errp)
> > +{
> > +    vnc_display_reload_cert(NULL, errp);
> > +}
> >  #endif /* !CONFIG_VNC */
> >  
> >  void qmp_change(const char *device, const char *target,
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index d08d72b439..855b1ac007 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1179,3 +1179,21 @@
> >  ##
> >  { 'command': 'query-display-options',
> >    'returns': 'DisplayOptions' }
> > +
> > +##
> > +# @reload-vnc-cert:
> > +#
> > +# Reload certificates for vnc.
> > +#
> > +# Returns: nothing
> > +#
> > +# Since: 5.2
> 
> 6.0 now.
> 
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "reload-vnc-cert" }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'reload-vnc-cert',
> > +  'if': 'defined(CONFIG_VNC)' }
> 
> Daniel's objection to another patch that adds a rather specialized QMP
> command may apply: "I'm not a fan of adding adhoc new commands for
> specific properties."
> 
>     From: Daniel P. Berrangé <berrange@redhat.com>
>     Subject: Re: [PATCH] vnc: add qmp to support change authz
>     Message-ID: <20210107161830.GE1029501@redhat.com>

Yeah, though this scenario is a ittle more complicated. Tihs patch is
not actually changing any object properties in the VNC server config.
It is simply telling the VNC server to reload the existing object it
has configured.

My proposed  "display-update" command would not directly map to what
this "reload-vnc-cert" command does, unless we declared the certs are
always reloaded after every display-update command.

Or we could have a more generic  "display-reload" command, which has
fields indicating what aspect to reload. eg a 'tls-certs: bool' field
to indicate reload of TLS certs in the display. This could be useful
if we wanted the ability to reload authz access control lists, though
we did actually wire them up to auto-reload using inotify.


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