Stefan Reiter <s.rei...@proxmox.com> writes:
> On 10/12/21 11:24 AM, Markus Armbruster wrote:
>> Stefan Reiter <s.rei...@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 <ebl...@redhat.com>
>>> Suggested-by: Markus Armbruster <arm...@redhat.com>
>>> Signed-off-by: Stefan Reiter <s.rei...@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?
[...]
