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

[Eric Blake <eblake@redhat.com>]

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.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org