Re: [PATCH v2 3/3] monitor: allow VNC related QMP and HMP commands to take a display ID

[Markus Armbruster]

Eric Blake <eblake@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.reiter@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.