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

[Markus Armbruster]

Zihao Chang <changzihao1@huawei.com> writes:

> 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:
[...]
>>>> 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).

'boxed': true can give you a reasonable C function even then.
docs/devel/qapi-code-gen.txt:

    When member 'boxed' is absent, [the function generated for the
    command] takes the command arguments as arguments one by one, in
    QAPI schema order.  Else it takes them wrapped in the C struct
    generated for the complex argument type.  It takes an additional
    Error ** argument in either case.

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

Representing a set of named things as a "list of enum of thing names" is
workable.

It's a fairly rigid design, though.  When you start with "struct of bool
members", you can add members of other types, and the whole still looks
regular.  You can even evolve an existing bool into an alternate of bool
and something else.

What kind of evolution do you want to prepare for?  Two foolish answers
to avoid: "any and all, regardless of cost" (you should not brush off
cost like that, ever), and "none, because I can predict the future
confidently" (no, you can't).