[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] vnc: add qmp to support reload vnc tls certificates
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v2 2/2] vnc: add qmp to support reload vnc tls certificates |
Date: |
Mon, 18 Jan 2021 14:27:21 +0000 |
User-agent: |
Mutt/1.14.6 (2020-07-11) |
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 <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.
> >>
> 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é <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
> >
>
> 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 :|
[PATCH v2 1/2] crypto: add reload for QCryptoTLSCredsClass, Zihao Chang, 2021/01/07