Eric Blake <ebl...@redhat.com> writes:
> On Wed, Sep 01, 2021 at 05:17:48PM +0200, Stefan Reiter wrote:
>> It is possible to specify more than one VNC server on the command line,
>> either with an explicit ID or the auto-generated ones à la "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.
>>
>> Signed-off-by: Stefan Reiter <s.rei...@proxmox.com>
>> ---
>
> QMP review:
>
>> +++ b/qapi/ui.json
>> @@ -25,6 +25,9 @@
>> # 'disconnect' to disconnect existing clients
>> # 'keep' to maintain existing clients
>> #
>> +# @display: In case of VNC, the id of the display where the password
>> +# should be changed. Defaults to the first.
>> +#
>> # Returns: - Nothing on success
>> # - If Spice is not enabled, DeviceNotFound
>> #
>> @@ -38,7 +41,8 @@
>> #
>> ##
>> { 'command': 'set_password',
>> - 'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
>> + 'data': {'protocol': 'str', 'password': 'str', '*connected': 'str',
>
> Pre-existing, but given the documentation that protocol is either
> 'vnc' or 'spice', this feels like set_password should take a
> discriminated union type with 'protocol' as an enum type,...
>
>> + '*display': 'str'} }
>
> ...so that you only add the optional 'display' member to 'vnc'. This
> would keep the status quo of rejecting it as invalid when protocol is
> 'spice', and make it easier to introspect that no other protocols are
> supported.
>
> Markus may have better advice on whether cleaning this up is worth it.
Changing @protocol from str to enum is straightforward, and backward
compatible. qmp_set_password() becomes simpler (we lose a failure
mode). If we ever add another protocol, introspection will show it. It
also reflects CONFIG_VNC and CONFIG_SPICE, which is perhaps less useful
than it was before modularization, but still nice. Yes, please.
Same for @connected.
We may have more 'str' parameters that should be enum elsewhere. I'm
not demanding you hunt them down :)
Adding the new parameter only to the protocol that actually supports it
is more complicated. Untested:
{ 'command': 'set_password', 'boxed': true,
'data': 'SetPasswordOptions' }
{ 'union': 'SetPasswordOptions',
'base': { 'protocol: 'PasswordProtocol',
'connected': 'FailDisconnectKeep' },
'discriminator': protocol',
'data': {
'vnc': 'SetPasswordOptionsVnc' } }
{ 'enum': 'PasswordProtocol'
'data': [ { 'name': 'vnc', 'if': 'CONFIG_VNC' },
{ 'name': 'spice', 'if': 'CONFIG_SPICE } ] }
{ 'enum': 'FailDisconnectKeep',
'data': [ 'fail', 'disconnect', 'keep' ] }
{ 'struct': 'SetPasswordOptionsVnc',
'data': { '*display': 'str } }
Advangages are similar: qmp_set_password() doesn't have to reject
@display for protocols other than 'vnc', and introspection is more
accurate. Please give it a try.
>>
>> ##
>> # @expire_password:
>> @@ -54,6 +58,9 @@
>> # - '+INT' where INT is the number of seconds from now (integer)
>> # - 'INT' where INT is the absolute time in seconds
>> #
>> +# @display: In case of VNC, the id of the display where the password
>> +# should be set to expire. Defaults to the first.
>> +#
>> # Returns: - Nothing on success
>> # - If @protocol is 'spice' and Spice is not active, DeviceNotFound
>> #
>> @@ -71,7 +78,8 @@
>> # <- { "return": {} }
>> #
>> ##
>> -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
>> +{ 'command': 'expire_password',
>> + 'data': {'protocol': 'str', 'time': 'str', '*display': 'str'} }
>
> This would benefit from the same treatment, if we decide to use a QAPI
> enum type and discriminated union.
Either both or neither.
