On Mon, Jan 18, 2021 at 03:27:33PM +0800, Zihao Chang wrote: > > > On 2021/1/15 21:47, Daniel P. Berrangé wrote: > > On Fri, Jan 15, 2021 at 02:37:33PM +0100, Markus Armbruster wrote: > >> Zihao Chang <[email protected]> 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 <[email protected]> > >>> --- > >>> 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. > >> > I will fix this in the next version. > Thank your for your reviews. > > >> 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é <[email protected]> > >> Subject: Re: [PATCH] vnc: add qmp to support change authz > >> Message-ID: <[email protected]> > > > > 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 > > > > If we add field for each reloadable attribute(tls-certs, authz,...), > the number of parameters for qmp_display_reload() may increase sharply > (bool has_tls_certs, bool tls_certs, ... twice the number of attributes).
There's a fairly limited conceptual set of VNC features which are going to require a reload operation, so I don't think it'll grow too unreasonably large. > How about using a list[] to determine which attributes need to be > reloaded["tls_certs", "authz*"...], and define an enum to ensure the > validity of list elements. The enum is simple, but if we require data to be provided fr the reload operation, instead of a simple boolean, then it gets a bit more limiting. 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 :|
