Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Markus Armbruster <armbru@redhat.com>]

Stefan Reiter <s.reiter@proxmox.com> writes:

> On 10/12/21 11:24 AM, Markus Armbruster wrote:
>> Stefan Reiter <s.reiter@proxmox.com> writes:
>> 
>>> It is possible to specify more than one VNC server on the command line,
>>> either with an explicit ID or the auto-generated ones à "default",
>>> "vnc2", "vnc3", ...
>>>
>>> It is not possible to change the password on one of these extra VNC
>>> displays though. Fix this by adding a "display" parameter to the
>>> "set_password" and "expire_password" QMP and HMP commands.
>>>
>>> For HMP, the display is specified using the "-d" value flag.
>>>
>>> For QMP, the schema is updated to explicitly express the supported
>>> variants of the commands with protocol-discriminated unions.
>>>
>>> Suggested-by: Eric Blake <eblake@redhat.com>
>>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> 
>> [...]
>> 
>>> diff --git a/qapi/ui.json b/qapi/ui.json
>>> index d7567ac866..4244c62c30 100644
>>> --- a/qapi/ui.json
>>> +++ b/qapi/ui.json
>>> @@ -9,22 +9,23 @@
>>>   { 'include': 'common.json' }
>>>   { 'include': 'sockets.json' }
>>>   
>>> +##
>>> +# @DisplayProtocol:
>>> +#
>>> +# Display protocols which support changing password options.
>>> +#
>>> +# Since: 6.2
>>> +#
>>> +##
>>> +{ 'enum': 'DisplayProtocol',
>>> +  'data': [ { 'name': 'vnc', 'if': 'CONFIG_VNC' },
>>> +            { 'name': 'spice', 'if': 'CONFIG_SPICE' } ] }
>>> +
>> 
>> 
>> 
>>>   ##
>>>   # @set_password:
>>>   #
>>>   # Sets the password of a remote display session.
>>>   #
>>> -# @protocol: - 'vnc' to modify the VNC server password
>>> -#            - 'spice' to modify the Spice server password
>>> -#
>>> -# @password: the new password
>>> -#
>>> -# @connected: how to handle existing clients when changing the
>>> -#             password.  If nothing is specified, defaults to 'keep'
>>> -#             'fail' to fail the command if clients are connected
>>> -#             'disconnect' to disconnect existing clients
>>> -#             'keep' to maintain existing clients
>>> -#
>>>   # Returns: - Nothing on success
>>>   #          - If Spice is not enabled, DeviceNotFound
>>>   #
>>> @@ -37,33 +38,106 @@
>>>   # <- { "return": {} }
>>>   #
>>>   ##
>>> -{ 'command': 'set_password',
>>> -  'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
>>> +{ 'command': 'set_password', 'boxed': true, 'data': 'SetPasswordOptions' }
>>> +
>>> +##
>>> +# @SetPasswordOptions:
>>> +#
>>> +# Data required to set a new password on a display server protocol.
>>> +#
>>> +# @protocol: - 'vnc' to modify the VNC server password
>>> +#            - 'spice' to modify the Spice server password
>>> +#
>>> +# @password: the new password
>>> +#
>>> +# Since: 6.2
>>> +#
>>> +##
>>> +{ 'union': 'SetPasswordOptions',
>>> +  'base': { 'protocol': 'DisplayProtocol',
>>> +            'password': 'str' },
>>> +  'discriminator': 'protocol',
>>> +  'data': { 'vnc': 'SetPasswordOptionsVnc',
>>> +            'spice': 'SetPasswordOptionsSpice' } }
>>> +
>>> +##
>>> +# @SetPasswordAction:
>>> +#
>>> +# An action to take on changing a password on a connection with active clients.
>>> +#
>>> +# @fail: fail the command if clients are connected
>>> +#
>>> +# @disconnect: disconnect existing clients
>>> +#
>>> +# @keep: maintain existing clients
>>> +#
>>> +# Since: 6.2
>>> +#
>>> +##
>>> +{ 'enum': 'SetPasswordAction',
>>> +  'data': [ 'fail', 'disconnect', 'keep' ] }
>>> +
>>> +##
>>> +# @SetPasswordActionVnc:
>>> +#
>>> +# See @SetPasswordAction. VNC only supports the keep action. 'connection'
>>> +# should just be omitted for VNC, this is kept for backwards compatibility.
>>> +#
>>> +# @keep: maintain existing clients
>>> +#
>>> +# Since: 6.2
>>> +#
>>> +##
>>> +{ 'enum': 'SetPasswordActionVnc',
>>> +  'data': [ 'keep' ] }
>>> +
>>> +##
>>> +# @SetPasswordOptionsSpice:
>>> +#
>>> +# Options for set_password specific to the VNC procotol.
>>> +#
>>> +# @connected: How to handle existing clients when changing the
>>> +#             password. If nothing is specified, defaults to 'keep'.
>>> +#
>>> +# Since: 6.2
>>> +#
>>> +##
>>> +{ 'struct': 'SetPasswordOptionsSpice',
>>> +  'data': { '*connected': 'SetPasswordAction' } }
>>> +
>>> +##
>>> +# @SetPasswordOptionsVnc:
>>> +#
>>> +# Options for set_password specific to the VNC procotol.
>>> +#
>>> +# @display: The id of the display where the password should be changed.
>>> +#           Defaults to the first.
>>> +#
>>> +# @connected: How to handle existing clients when changing the
>>> +#             password.
>>> +#
>>> +# Features:
>>> +# @deprecated: For VNC, @connected will always be 'keep', parameter should be
>>> +#              omitted.
>>> +#
>>> +# Since: 6.2
>>> +#
>>> +##
>>> +{ 'struct': 'SetPasswordOptionsVnc',
>>> +  'data': { '*display': 'str',
>>> +            '*connected': { 'type': 'SetPasswordActionVnc',
>>> +                            'features': ['deprecated'] } } }
>> 
>> Let me describe what you're doing in my own words, to make sure I got
>> both the what and the why:
>> 
>> 1. Change @protocol and @connected from str to enum.
>> 
>> 2. Turn the argument struct into a union tagged by @protocol, to enable
>>     3. and 4.
>> 
>> 3. Add argument @display in branch 'vnc'.
>> 
>> 4. Use a separate enum for @connected in each union branch, to enable 4.
>> 
>> 5. Trim this enum in branch 'vnc' to the values that are actually
>>     supported.  Note that just one value remains, i.e. @connected is now
>>     an optional argument that can take only the default value.
>> 
>> 6. Since such an argument is pointless, deprecate @connected in branch
>>     'vnc'.
>> 
>> Correct?
>
> Yes.

Thanks!

>> Ignorant question: is it possible to have more than one 'spice' display?
>
> Not in terms of passwords anyway. So only one SPICE password can be set at
> any time, i.e. the 'display' parameter in this function does not apply.
>
>> 
>> Item 5. is not a compatibility break: before your patch,
>> qmp_set_password() rejects the values you drop.
>
> Yes.
>
>> 
>> Item 5. adds another enum to the schema in return for more accurate
>> introspection and slightly simpler qmp_set_password().  I'm not sure
>> that's worthwhile.  I think we could use the same enum for both union
>> branches.
>
> I tried to avoid mixing manual parsing and declarative QAPI stuff as much
> as I could, so I moved as much as possible to QAPI. I personally like the
> extra documentation of having the separate enum.

It's a valid choice.  I'm just pointing out another valid choice.  The
difference between them will go away when we remove deprecated
@connected.  You choose :)

>> I'd split this patch into three parts: item 1., 2.+3., 4.-6., because
>> each part can stand on its own.
>
> The reason why I didn't do that initially is more to do with the C side.
> I think splitting it up as you describe would make for some awkward diffs
> on the qmp_set_password/hmp_set_password side.
>
> I can try of course.

It's a suggestion, not a demand.  I'm a compulsory patch splitter.

>                      Though I also want to have it said that I'm not a fan
> of how the HMP functions have to expand so much to accommodate the QAPI
> structure in general.

Care to elaborate?

[...]