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.

For QMP, the schema is updated to explicitly express the supported
variants of the commands with protocol-discriminated unions.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 hmp-commands.hx    |  24 +++++-----
 monitor/hmp-cmds.c |  51 +++++++++++++++------
 monitor/qmp-cmds.c |  36 ++++++---------
 qapi/ui.json       | 112 +++++++++++++++++++++++++++++++++++----------
 4 files changed, 154 insertions(+), 69 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index cf723c69ac..9fbb207b35 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1514,33 +1514,35 @@ ERST
 
     {
         .name       = "set_password",
-        .args_type  = "protocol:s,password:s,connected:s?",
-        .params     = "protocol password action-if-connected",
+        .args_type  = "protocol:s,password:s,display:-dV,connected:s?",
+        .params     = "protocol password [-d display] [action-if-connected]",
         .help       = "set spice/vnc password",
         .cmd        = hmp_set_password,
     },
 
 SRST
-``set_password [ vnc | spice ] password [ action-if-connected ]``
-  Change spice/vnc password.  *action-if-connected* specifies what
-  should happen in case a connection is established: *fail* makes the
-  password change fail.  *disconnect* changes the password and
+``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected ]``
+  Change spice/vnc password.  *display* can be used with 'vnc' to specify
+  which display to set the password on.  *action-if-connected* specifies
+  what should happen in case a connection is established: *fail* makes
+  the password change fail.  *disconnect* changes the password and
   disconnects the client.  *keep* changes the password and keeps the
   connection up.  *keep* is the default.
 ERST
 
     {
         .name       = "expire_password",
-        .args_type  = "protocol:s,time:s",
-        .params     = "protocol time",
+        .args_type  = "protocol:s,time:s,display:-dV",
+        .params     = "protocol time [-d display]",
         .help       = "set spice/vnc password expire-time",
         .cmd        = hmp_expire_password,
     },
 
 SRST
-``expire_password [ vnc | spice ]`` *expire-time*
-  Specify when a password for spice/vnc becomes
-  invalid. *expire-time* accepts:
+``expire_password [ vnc | spice ] expire-time [ -d display ]``
+  Specify when a password for spice/vnc becomes invalid.
+  *display* behaves the same as in ``set_password``.
+  *expire-time* accepts:
 
   ``now``
     Invalidate password instantly.
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b8abe69609..daa4a8e106 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,26 +1451,39 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
 {
     const char *protocol  = qdict_get_str(qdict, "protocol");
     const char *password  = qdict_get_str(qdict, "password");
+    const char *display = qdict_get_try_str(qdict, "display");
     const char *connected = qdict_get_try_str(qdict, "connected");
     Error *err = NULL;
-    DisplayProtocol proto;
-    SetPasswordAction conn;
 
-    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-                            DISPLAY_PROTOCOL_VNC, &err);
+    SetPasswordOptions opts = {
+        .password = g_strdup(password),
+        .u.vnc.display = NULL,
+    };
+
+    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+                                    DISPLAY_PROTOCOL_VNC, &err);
     if (err) {
         goto out;
     }
 
-    conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
-                           SET_PASSWORD_ACTION_KEEP, &err);
-    if (err) {
-        goto out;
+    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+        opts.u.vnc.has_display = !!display;
+        opts.u.vnc.display = g_strdup(display);
+    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
+        opts.u.spice.has_connected = !!connected;
+        opts.u.spice.connected =
+            qapi_enum_parse(&SetPasswordAction_lookup, connected,
+                            SET_PASSWORD_ACTION_KEEP, &err);
+        if (err) {
+            goto out;
+        }
     }
 
-    qmp_set_password(proto, password, !!connected, conn, &err);
+    qmp_set_password(&opts, &err);
 
 out:
+    g_free(opts.password);
+    g_free(opts.u.vnc.display);
     hmp_handle_error(mon, err);
 }
 
@@ -1478,18 +1491,30 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
 {
     const char *protocol  = qdict_get_str(qdict, "protocol");
     const char *whenstr = qdict_get_str(qdict, "time");
+    const char *display = qdict_get_try_str(qdict, "display");
     Error *err = NULL;
-    DisplayProtocol proto;
 
-    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-                            DISPLAY_PROTOCOL_VNC, &err);
+    ExpirePasswordOptions opts = {
+        .time = g_strdup(whenstr),
+        .u.vnc.display = NULL,
+    };
+
+    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+                                    DISPLAY_PROTOCOL_VNC, &err);
     if (err) {
         goto out;
     }
 
-    qmp_expire_password(proto, whenstr, &err);
+    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+        opts.u.vnc.has_display = !!display;
+        opts.u.vnc.display = g_strdup(display);
+    }
+
+    qmp_expire_password(&opts, &err);
 
 out:
+    g_free(opts.time);
+    g_free(opts.u.vnc.display);
     hmp_handle_error(mon, err);
 }
 
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 0654d7289a..5637bd70b6 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -163,35 +163,27 @@ void qmp_system_wakeup(Error **errp)
     qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
 }
 
-void qmp_set_password(DisplayProtocol protocol, const char *password,
-                      bool has_connected, SetPasswordAction connected,
-                      Error **errp)
+void qmp_set_password(SetPasswordOptions *opts, Error **errp)
 {
-    int disconnect_if_connected = 0;
-    int fail_if_connected = 0;
-    int rc;
+    int rc = 0;
 
-    if (has_connected) {
-        fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL;
-        disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT;
-    }
-
-    if (protocol == DISPLAY_PROTOCOL_SPICE) {
+    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
         if (!qemu_using_spice(errp)) {
             return;
         }
-        rc = qemu_spice.set_passwd(password, fail_if_connected,
-                                   disconnect_if_connected);
+        rc = qemu_spice.set_passwd(opts->password,
+                opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL,
+                opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT);
     } else {
-        assert(protocol == DISPLAY_PROTOCOL_VNC);
-        if (fail_if_connected || disconnect_if_connected) {
+        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
+        if (opts->u.vnc.connected != SET_PASSWORD_ACTION_KEEP) {
             /* vnc supports "connected=keep" only */
             error_setg(errp, QERR_INVALID_PARAMETER, "connected");
             return;
         }
         /* Note that setting an empty password will not disable login through
          * this interface. */
-        rc = vnc_display_password(NULL, password);
+        rc = vnc_display_password(opts->u.vnc.display, opts->password);
     }
 
     if (rc != 0) {
@@ -199,11 +191,11 @@ void qmp_set_password(DisplayProtocol protocol, const char *password,
     }
 }
 
-void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
-                         Error **errp)
+void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
 {
     time_t when;
     int rc;
+    const char *whenstr = opts->time;
 
     if (strcmp(whenstr, "now") == 0) {
         when = 0;
@@ -215,14 +207,14 @@ void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
         when = strtoull(whenstr, NULL, 10);
     }
 
-    if (protocol == DISPLAY_PROTOCOL_SPICE) {
+    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
         if (!qemu_using_spice(errp)) {
             return;
         }
         rc = qemu_spice.set_pw_expire(when);
     } else {
-        assert(protocol == DISPLAY_PROTOCOL_VNC);
-        rc = vnc_display_pw_expire(NULL, when);
+        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
+        rc = vnc_display_pw_expire(opts->u.vnc.display, when);
     }
 
     if (rc != 0) {
diff --git a/qapi/ui.json b/qapi/ui.json
index 15cc19dcc5..99ac29ad9c 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -39,20 +39,61 @@
   'data': [ 'fail', 'disconnect', 'keep' ] }
 
 ##
-# @set_password:
+# @SetPasswordOptions:
 #
-# Sets the password of a remote display session.
+# General options for set_password.
 #
 # @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
+# Since: 6.2
+#
+##
+{ 'union': 'SetPasswordOptions',
+  'base': { 'protocol': 'DisplayProtocol',
+            'password': 'str' },
+  'discriminator': 'protocol',
+  'data': { 'vnc': 'SetPasswordOptionsVnc',
+            'spice': 'SetPasswordOptionsSpice' } }
+
+##
+# @SetPasswordOptionsSpice:
+#
+# Options for set_password specific to the SPICE 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.
+#
+# Since: 6.2
+#
+##
+{ 'struct': 'SetPasswordOptionsVnc',
+  'data': { '*display': 'str',
+            '*connected': 'SetPasswordAction' }}
+
+##
+# @set_password:
+#
+# Set the password of a remote display server.
 #
 # Returns: - Nothing on success
 #          - If Spice is not enabled, DeviceNotFound
@@ -66,18 +107,16 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'set_password',
-  'data': { 'protocol': 'DisplayProtocol',
-            'password': 'str',
-            '*connected': 'SetPasswordAction' } }
+{ 'command': 'set_password', 'boxed': true, 'data': 'SetPasswordOptions' }
 
 ##
-# @expire_password:
+# @ExpirePasswordOptions:
 #
-# Expire the password of a remote display server.
-#
-# @protocol: the name of the remote display protocol 'vnc' or 'spice'
+# General options for expire_password.
 #
+# @protocol: - 'vnc' to modify the VNC server expiration
+#            - 'spice' to modify the Spice server expiration
+
 # @time: when to expire the password.
 #
 #        - 'now' to expire the password immediately
@@ -85,16 +124,45 @@
 #        - '+INT' where INT is the number of seconds from now (integer)
 #        - 'INT' where INT is the absolute time in seconds
 #
-# Returns: - Nothing on success
-#          - If @protocol is 'spice' and Spice is not active, DeviceNotFound
-#
-# Since: 0.14
-#
 # Notes: Time is relative to the server and currently there is no way to
 #        coordinate server time with client time.  It is not recommended to
 #        use the absolute time version of the @time parameter unless you're
 #        sure you are on the same machine as the QEMU instance.
 #
+# Since: 6.2
+#
+##
+{ 'union': 'ExpirePasswordOptions',
+  'base': { 'protocol': 'DisplayProtocol',
+            'time': 'str' },
+  'discriminator': 'protocol',
+  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
+
+##
+# @ExpirePasswordOptionsVnc:
+#
+# Options for expire_password specific to the VNC procotol.
+#
+# @display: The id of the display where the expiration should be changed.
+#           Defaults to the first.
+#
+# Since: 6.2
+#
+##
+
+{ 'struct': 'ExpirePasswordOptionsVnc',
+  'data': { '*display': 'str' } }
+
+##
+# @expire_password:
+#
+# Expire the password of a remote display server.
+#
+# Returns: - Nothing on success
+#          - If @protocol is 'spice' and Spice is not active, DeviceNotFound
+#
+# Since: 0.14
+#
 # Example:
 #
 # -> { "execute": "expire_password", "arguments": { "protocol": "vnc",
@@ -102,9 +170,7 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'expire_password',
-  'data': { 'protocol': 'DisplayProtocol',
-            'time': 'str' } }
+{ 'command': 'expire_password', 'boxed': true, 'data': 'ExpirePasswordOptions' }
 
 ##
 # @screendump:
-- 
2.30.2

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 à 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.
>
> For QMP, the schema is updated to explicitly express the supported
> variants of the commands with protocol-discriminated unions.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  hmp-commands.hx    |  24 +++++-----
>  monitor/hmp-cmds.c |  51 +++++++++++++++------
>  monitor/qmp-cmds.c |  36 ++++++---------
>  qapi/ui.json       | 112 +++++++++++++++++++++++++++++++++++----------
>  4 files changed, 154 insertions(+), 69 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index cf723c69ac..9fbb207b35 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1514,33 +1514,35 @@ ERST
>  
>      {
>          .name       = "set_password",
> -        .args_type  = "protocol:s,password:s,connected:s?",
> -        .params     = "protocol password action-if-connected",
> +        .args_type  = "protocol:s,password:s,display:-dV,connected:s?",
> +        .params     = "protocol password [-d display] [action-if-connected]",
>          .help       = "set spice/vnc password",
>          .cmd        = hmp_set_password,
>      },
>  
>  SRST
> -``set_password [ vnc | spice ] password [ action-if-connected ]``
> -  Change spice/vnc password.  *action-if-connected* specifies what
> -  should happen in case a connection is established: *fail* makes the
> -  password change fail.  *disconnect* changes the password and
> +``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected ]``
> +  Change spice/vnc password.  *display* can be used with 'vnc' to specify
> +  which display to set the password on.  *action-if-connected* specifies
> +  what should happen in case a connection is established: *fail* makes
> +  the password change fail.  *disconnect* changes the password and
>    disconnects the client.  *keep* changes the password and keeps the
>    connection up.  *keep* is the default.
>  ERST
>  
>      {
>          .name       = "expire_password",
> -        .args_type  = "protocol:s,time:s",
> -        .params     = "protocol time",
> +        .args_type  = "protocol:s,time:s,display:-dV",
> +        .params     = "protocol time [-d display]",
>          .help       = "set spice/vnc password expire-time",
>          .cmd        = hmp_expire_password,
>      },
>  
>  SRST
> -``expire_password [ vnc | spice ]`` *expire-time*
> -  Specify when a password for spice/vnc becomes
> -  invalid. *expire-time* accepts:
> +``expire_password [ vnc | spice ] expire-time [ -d display ]``
> +  Specify when a password for spice/vnc becomes invalid.
> +  *display* behaves the same as in ``set_password``.
> +  *expire-time* accepts:
>  
>    ``now``
>      Invalidate password instantly.
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index b8abe69609..daa4a8e106 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1451,26 +1451,39 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
>  {
>      const char *protocol  = qdict_get_str(qdict, "protocol");
>      const char *password  = qdict_get_str(qdict, "password");
> +    const char *display = qdict_get_try_str(qdict, "display");
>      const char *connected = qdict_get_try_str(qdict, "connected");
>      Error *err = NULL;
> -    DisplayProtocol proto;
> -    SetPasswordAction conn;
>  
> -    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> -                            DISPLAY_PROTOCOL_VNC, &err);
> +    SetPasswordOptions opts = {
> +        .password = g_strdup(password),
> +        .u.vnc.display = NULL,
> +    };
> +
> +    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> +                                    DISPLAY_PROTOCOL_VNC, &err);
>      if (err) {
>          goto out;
>      }
>  
> -    conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
> -                           SET_PASSWORD_ACTION_KEEP, &err);
> -    if (err) {
> -        goto out;
> +    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
> +        opts.u.vnc.has_display = !!display;
> +        opts.u.vnc.display = g_strdup(display);
> +    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
> +        opts.u.spice.has_connected = !!connected;
> +        opts.u.spice.connected =
> +            qapi_enum_parse(&SetPasswordAction_lookup, connected,
> +                            SET_PASSWORD_ACTION_KEEP, &err);
> +        if (err) {
> +            goto out;
> +        }
>      }
>  
> -    qmp_set_password(proto, password, !!connected, conn, &err);
> +    qmp_set_password(&opts, &err);
>  
>  out:
> +    g_free(opts.password);
> +    g_free(opts.u.vnc.display);

Uh-oh...

For DISPLAY_PROTOCOL_SPICE, we execute

           .u.vnc.display = NULL,
           opts.u.spice.has_connected = !!connected;
           opts.u.spice.connected =
               qapi_enum_parse(&SetPasswordAction_lookup, connected,
                               SET_PASSWORD_ACTION_KEEP, &err);
           opts.u.vnc.has_display = !!display;
       g_free(opts.u.vnc.display);

The assignments to opts.u.spice.has_connected and opts.u.spice_connected
clobber opts.u.vnc.display.

The simplest fix is to pass @display directly.  Precedence:
hmp_drive_mirror().

Do the same for @password, of course.

>      hmp_handle_error(mon, err);
>  }
>  
> @@ -1478,18 +1491,30 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
>  {
>      const char *protocol  = qdict_get_str(qdict, "protocol");
>      const char *whenstr = qdict_get_str(qdict, "time");
> +    const char *display = qdict_get_try_str(qdict, "display");
>      Error *err = NULL;
> -    DisplayProtocol proto;
>  
> -    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> -                            DISPLAY_PROTOCOL_VNC, &err);
> +    ExpirePasswordOptions opts = {
> +        .time = g_strdup(whenstr),
> +        .u.vnc.display = NULL,
> +    };
> +
> +    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> +                                    DISPLAY_PROTOCOL_VNC, &err);
>      if (err) {
>          goto out;
>      }
>  
> -    qmp_expire_password(proto, whenstr, &err);
> +    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
> +        opts.u.vnc.has_display = !!display;
> +        opts.u.vnc.display = g_strdup(display);
> +    }

Use of -d with spice are silently ignored.  Do we care?  Same for
hmp_set_password() above.

> +
> +    qmp_expire_password(&opts, &err);
>  
>  out:
> +    g_free(opts.time);
> +    g_free(opts.u.vnc.display);

No uh-oh here, since opts.u.vnc is actually the only member of opts.u.
Still, let's pass @time and @display directly for consistency and
robustness.

>      hmp_handle_error(mon, err);
>  }
>  

[...]

On 10/21/21 7:05 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 à 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.
>>
>> For QMP, the schema is updated to explicitly express the supported
>> variants of the commands with protocol-discriminated unions.
>>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>   hmp-commands.hx    |  24 +++++-----
>>   monitor/hmp-cmds.c |  51 +++++++++++++++------
>>   monitor/qmp-cmds.c |  36 ++++++---------
>>   qapi/ui.json       | 112 +++++++++++++++++++++++++++++++++++----------
>>   4 files changed, 154 insertions(+), 69 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index cf723c69ac..9fbb207b35 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1514,33 +1514,35 @@ ERST
>>   
>>       {
>>           .name       = "set_password",
>> -        .args_type  = "protocol:s,password:s,connected:s?",
>> -        .params     = "protocol password action-if-connected",
>> +        .args_type  = "protocol:s,password:s,display:-dV,connected:s?",
>> +        .params     = "protocol password [-d display] [action-if-connected]",
>>           .help       = "set spice/vnc password",
>>           .cmd        = hmp_set_password,
>>       },
>>   
>>   SRST
>> -``set_password [ vnc | spice ] password [ action-if-connected ]``
>> -  Change spice/vnc password.  *action-if-connected* specifies what
>> -  should happen in case a connection is established: *fail* makes the
>> -  password change fail.  *disconnect* changes the password and
>> +``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected ]``
>> +  Change spice/vnc password.  *display* can be used with 'vnc' to specify
>> +  which display to set the password on.  *action-if-connected* specifies
>> +  what should happen in case a connection is established: *fail* makes
>> +  the password change fail.  *disconnect* changes the password and
>>     disconnects the client.  *keep* changes the password and keeps the
>>     connection up.  *keep* is the default.
>>   ERST
>>   
>>       {
>>           .name       = "expire_password",
>> -        .args_type  = "protocol:s,time:s",
>> -        .params     = "protocol time",
>> +        .args_type  = "protocol:s,time:s,display:-dV",
>> +        .params     = "protocol time [-d display]",
>>           .help       = "set spice/vnc password expire-time",
>>           .cmd        = hmp_expire_password,
>>       },
>>   
>>   SRST
>> -``expire_password [ vnc | spice ]`` *expire-time*
>> -  Specify when a password for spice/vnc becomes
>> -  invalid. *expire-time* accepts:
>> +``expire_password [ vnc | spice ] expire-time [ -d display ]``
>> +  Specify when a password for spice/vnc becomes invalid.
>> +  *display* behaves the same as in ``set_password``.
>> +  *expire-time* accepts:
>>   
>>     ``now``
>>       Invalidate password instantly.
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index b8abe69609..daa4a8e106 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -1451,26 +1451,39 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
>>   {
>>       const char *protocol  = qdict_get_str(qdict, "protocol");
>>       const char *password  = qdict_get_str(qdict, "password");
>> +    const char *display = qdict_get_try_str(qdict, "display");
>>       const char *connected = qdict_get_try_str(qdict, "connected");
>>       Error *err = NULL;
>> -    DisplayProtocol proto;
>> -    SetPasswordAction conn;
>>   
>> -    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
>> -                            DISPLAY_PROTOCOL_VNC, &err);
>> +    SetPasswordOptions opts = {
>> +        .password = g_strdup(password),
>> +        .u.vnc.display = NULL,
>> +    };
>> +
>> +    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
>> +                                    DISPLAY_PROTOCOL_VNC, &err);
>>       if (err) {
>>           goto out;
>>       }
>>   
>> -    conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
>> -                           SET_PASSWORD_ACTION_KEEP, &err);
>> -    if (err) {
>> -        goto out;
>> +    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
>> +        opts.u.vnc.has_display = !!display;
>> +        opts.u.vnc.display = g_strdup(display);
>> +    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
>> +        opts.u.spice.has_connected = !!connected;
>> +        opts.u.spice.connected =
>> +            qapi_enum_parse(&SetPasswordAction_lookup, connected,
>> +                            SET_PASSWORD_ACTION_KEEP, &err);
>> +        if (err) {
>> +            goto out;
>> +        }
>>       }
>>   
>> -    qmp_set_password(proto, password, !!connected, conn, &err);
>> +    qmp_set_password(&opts, &err);
>>   
>>   out:
>> +    g_free(opts.password);
>> +    g_free(opts.u.vnc.display);
> 
> Uh-oh...
> 
> For DISPLAY_PROTOCOL_SPICE, we execute
> 
>             .u.vnc.display = NULL,
>             opts.u.spice.has_connected = !!connected;
>             opts.u.spice.connected =
>                 qapi_enum_parse(&SetPasswordAction_lookup, connected,
>                                 SET_PASSWORD_ACTION_KEEP, &err);
>             opts.u.vnc.has_display = !!display;
>         g_free(opts.u.vnc.display);
> 
> The assignments to opts.u.spice.has_connected and opts.u.spice_connected
> clobber opts.u.vnc.display.
> 
> The simplest fix is to pass @display directly.  Precedence:
> hmp_drive_mirror().

With "directly", I assume you mean without g_strdup, so:

   opts.u.vnc.display = (char *)display;

right? Does it matter that we drop the 'const'?

> 
> Do the same for @password, of course.
> 
>>       hmp_handle_error(mon, err);
>>   }
>>   
>> @@ -1478,18 +1491,30 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
>>   {
>>       const char *protocol  = qdict_get_str(qdict, "protocol");
>>       const char *whenstr = qdict_get_str(qdict, "time");
>> +    const char *display = qdict_get_try_str(qdict, "display");
>>       Error *err = NULL;
>> -    DisplayProtocol proto;
>>   
>> -    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
>> -                            DISPLAY_PROTOCOL_VNC, &err);
>> +    ExpirePasswordOptions opts = {
>> +        .time = g_strdup(whenstr),
>> +        .u.vnc.display = NULL,
>> +    };
>> +
>> +    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
>> +                                    DISPLAY_PROTOCOL_VNC, &err);
>>       if (err) {
>>           goto out;
>>       }
>>   
>> -    qmp_expire_password(proto, whenstr, &err);
>> +    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
>> +        opts.u.vnc.has_display = !!display;
>> +        opts.u.vnc.display = g_strdup(display);
>> +    }
> 
> Use of -d with spice are silently ignored.  Do we care?  Same for
> hmp_set_password() above.

Depends on you, I don't. I think it's not worth catching even more
in HMP, since it's clear there's only one SPICE display anyway, and
it's all documented.

> 
>> +
>> +    qmp_expire_password(&opts, &err);
>>   
>>   out:
>> +    g_free(opts.time);
>> +    g_free(opts.u.vnc.display);
> 
> No uh-oh here, since opts.u.vnc is actually the only member of opts.u.
> Still, let's pass @time and @display directly for consistency and
> robustness.
> 
>>       hmp_handle_error(mon, err);
>>   }
>>   
> 
> [...]
> 
> 

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

> On 10/21/21 7:05 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 à 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.
>>>
>>> For QMP, the schema is updated to explicitly express the supported
>>> variants of the commands with protocol-discriminated unions.
>>>
>>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>>> ---

[...]

>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>> index b8abe69609..daa4a8e106 100644
>>> --- a/monitor/hmp-cmds.c
>>> +++ b/monitor/hmp-cmds.c
>>> @@ -1451,26 +1451,39 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
>>>   {
>>>       const char *protocol  = qdict_get_str(qdict, "protocol");
>>>       const char *password  = qdict_get_str(qdict, "password");
>>> +    const char *display = qdict_get_try_str(qdict, "display");
>>>       const char *connected = qdict_get_try_str(qdict, "connected");
>>>       Error *err = NULL;
>>> -    DisplayProtocol proto;
>>> -    SetPasswordAction conn;
>>>   
>>> -    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
>>> -                            DISPLAY_PROTOCOL_VNC, &err);
>>> +    SetPasswordOptions opts = {
>>> +        .password = g_strdup(password),
>>> +        .u.vnc.display = NULL,
>>> +    };
>>> +
>>> +    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
>>> +                                    DISPLAY_PROTOCOL_VNC, &err);
>>>       if (err) {
>>>           goto out;
>>>       }
>>>   
>>> -    conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
>>> -                           SET_PASSWORD_ACTION_KEEP, &err);
>>> -    if (err) {
>>> -        goto out;
>>> +    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
>>> +        opts.u.vnc.has_display = !!display;
>>> +        opts.u.vnc.display = g_strdup(display);
>>> +    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
>>> +        opts.u.spice.has_connected = !!connected;
>>> +        opts.u.spice.connected =
>>> +            qapi_enum_parse(&SetPasswordAction_lookup, connected,
>>> +                            SET_PASSWORD_ACTION_KEEP, &err);
>>> +        if (err) {
>>> +            goto out;
>>> +        }
>>>       }
>>>   
>>> -    qmp_set_password(proto, password, !!connected, conn, &err);
>>> +    qmp_set_password(&opts, &err);
>>>   
>>>   out:
>>> +    g_free(opts.password);
>>> +    g_free(opts.u.vnc.display);
>> 
>> Uh-oh...
>> 
>> For DISPLAY_PROTOCOL_SPICE, we execute
>> 
>>             .u.vnc.display = NULL,
>>             opts.u.spice.has_connected = !!connected;
>>             opts.u.spice.connected =
>>                 qapi_enum_parse(&SetPasswordAction_lookup, connected,
>>                                 SET_PASSWORD_ACTION_KEEP, &err);
>>             opts.u.vnc.has_display = !!display;
>>         g_free(opts.u.vnc.display);
>> 
>> The assignments to opts.u.spice.has_connected and opts.u.spice_connected
>> clobber opts.u.vnc.display.
>> 
>> The simplest fix is to pass @display directly.  Precedence:
>> hmp_drive_mirror().
>
> With "directly", I assume you mean without g_strdup, so:
>
>    opts.u.vnc.display = (char *)display;
>
> right?

Right.

>        Does it matter that we drop the 'const'?

It's ugly, but harmless.

qdict_get_try_str() returns const char * to discourage you from messing
with the string while it's in the QDict.

qmp_set_password() does not actually mess with its argument.

>> Do the same for @password, of course.
>> 
>>>       hmp_handle_error(mon, err);
>>>   }
>>>   
>>> @@ -1478,18 +1491,30 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
>>>   {
>>>       const char *protocol  = qdict_get_str(qdict, "protocol");
>>>       const char *whenstr = qdict_get_str(qdict, "time");
>>> +    const char *display = qdict_get_try_str(qdict, "display");
>>>       Error *err = NULL;
>>> -    DisplayProtocol proto;
>>>   
>>> -    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
>>> -                            DISPLAY_PROTOCOL_VNC, &err);
>>> +    ExpirePasswordOptions opts = {
>>> +        .time = g_strdup(whenstr),
>>> +        .u.vnc.display = NULL,
>>> +    };
>>> +
>>> +    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
>>> +                                    DISPLAY_PROTOCOL_VNC, &err);
>>>       if (err) {
>>>           goto out;
>>>       }
>>>   
>>> -    qmp_expire_password(proto, whenstr, &err);
>>> +    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
>>> +        opts.u.vnc.has_display = !!display;
>>> +        opts.u.vnc.display = g_strdup(display);
>>> +    }
>> 
>> Use of -d with spice are silently ignored.  Do we care?  Same for
>> hmp_set_password() above.
>
> Depends on you, I don't. I think it's not worth catching even more
> in HMP, since it's clear there's only one SPICE display anyway, and
> it's all documented.

Up to the HMP maintainer, and we'll take silence as tacit agreement with
you :)

>>> +
>>> +    qmp_expire_password(&opts, &err);
>>>   
>>>   out:
>>> +    g_free(opts.time);
>>> +    g_free(opts.u.vnc.display);
>> 
>> No uh-oh here, since opts.u.vnc is actually the only member of opts.u.
>> Still, let's pass @time and @display directly for consistency and
>> robustness.
>> 
>>>       hmp_handle_error(mon, err);
>>>   }
>>>   
>> 
>> [...]