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 <changzih...@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 <changzih...@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é <berra...@redhat.com>
>> Subject: Re: [PATCH] vnc: add qmp to support change authz
>> Message-ID: <20210107161830.ge1029...@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).
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.
Regards,
Zihao
