[PATCH v4] VNC-related HMP/QMP fixes

[PATCH v4] VNC-related HMP/QMP fixes

[Stefan Reiter]

Since the removal of the generic 'qmp_change' command, one can no longer replace
the 'default' VNC display listen address at runtime (AFAIK). For our users who
need to set up a secondary VNC access port, this means configuring a second VNC
display (in addition to our standard one for web-access), but it turns out one
cannot set a password on this second display at the moment, as the
'set_password' call only operates on the 'default' display.

Additionally, using secret objects, the password is only read once at startup.
This could be considered a bug too, but is not touched in this series and left
for a later date.


v3 -> v4:
* drop previously patch 1, this was fixed here instead:
  https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02529.html
* patch 1: add Eric's R-b
* patch 2: remove if-assignment, use 'deprecated' feature in schema

v2 -> v3:
* refactor QMP schema for set/expire_password as suggested by Eric Blake and
  Markus Armbruster

v1 -> v2:
* add Marc-André's R-b on patch 1
* use '-d' flag as suggested by Eric Blake and Gerd Hoffmann
  * I didn't see a way to do this yet, so I added a "flags with values" arg type


Stefan Reiter (2):
  monitor/hmp: add support for flag argument with value
  monitor: refactor set/expire_password and allow VNC display id

 hmp-commands.hx    |  24 ++++---
 monitor/hmp-cmds.c |  57 ++++++++++++++-
 monitor/hmp.c      |  17 ++++-
 monitor/qmp-cmds.c |  62 ++++++----------
 qapi/ui.json       | 173 +++++++++++++++++++++++++++++++++++++--------
 5 files changed, 251 insertions(+), 82 deletions(-)

-- 
2.30.2

[PATCH v4 1/2] monitor/hmp: add support for flag argument with value

[Stefan Reiter]

Adds support for the "-xS" parameter type, where "-x" denotes a flag
name and the "S" suffix indicates that this flag is supposed to take an
arbitrary string parameter.

These parameters are always optional, the entry in the qdict will be
omitted if the flag is not given.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 monitor/hmp.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index d50c3124e1..a32dce7a35 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -980,6 +980,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
             {
                 const char *tmp = p;
                 int skip_key = 0;
+                int ret;
                 /* option */
 
                 c = *typestr++;
@@ -1002,8 +1003,22 @@ static QDict *monitor_parse_arguments(Monitor *mon,
                     }
                     if (skip_key) {
                         p = tmp;
+                    } else if (*typestr == 'S') {
+                        /* has option with string value */
+                        typestr++;
+                        tmp = p++;
+                        while (qemu_isspace(*p)) {
+                            p++;
+                        }
+                        ret = get_str(buf, sizeof(buf), &p);
+                        if (ret < 0) {
+                            monitor_printf(mon, "%s: value expected for -%c\n",
+                                           cmd->name, *tmp);
+                            goto fail;
+                        }
+                        qdict_put_str(qdict, key, buf);
                     } else {
-                        /* has option */
+                        /* has boolean option */
                         p++;
                         qdict_put_bool(qdict, key, true);
                     }
-- 
2.30.2

[PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Stefan Reiter]

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: Eric Blake <eblake@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 hmp-commands.hx    |  24 ++++---
 monitor/hmp-cmds.c |  57 ++++++++++++++-
 monitor/qmp-cmds.c |  62 ++++++----------
 qapi/ui.json       | 173 +++++++++++++++++++++++++++++++++++++--------
 4 files changed, 235 insertions(+), 81 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index cf723c69ac..d78e4cfc47 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:-dS,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:-dS",
+        .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 b5e71d9e6f..48b3fe4519 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,10 +1451,41 @@ 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;
 
-    qmp_set_password(protocol, password, !!connected, connected, &err);
+    SetPasswordOptions opts = {
+        .password = g_strdup(password),
+        .u.vnc.display = NULL,
+    };
+
+    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+                            DISPLAY_PROTOCOL_VNC, &err);
+    if (err) {
+        hmp_handle_error(mon, err);
+        return;
+    }
+    opts.protocol = proto;
+
+    if (proto == DISPLAY_PROTOCOL_VNC) {
+        opts.u.vnc.has_display = !!display;
+        opts.u.vnc.display = g_strdup(display);
+    } else if (proto == 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) {
+            hmp_handle_error(mon, err);
+            return;
+        }
+    }
+
+    qmp_set_password(&opts, &err);
+    g_free(opts.password);
+    g_free(opts.u.vnc.display);
     hmp_handle_error(mon, err);
 }
 
@@ -1462,9 +1493,31 @@ 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;
 
-    qmp_expire_password(protocol, whenstr, &err);
+    ExpirePasswordOptions opts = {
+        .time = g_strdup(whenstr),
+        .u.vnc.display = NULL,
+    };
+
+    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+                            DISPLAY_PROTOCOL_VNC, &err);
+    if (err) {
+        hmp_handle_error(mon, err);
+        return;
+    }
+    opts.protocol = proto;
+
+    if (proto == DISPLAY_PROTOCOL_VNC) {
+        opts.u.vnc.has_display = !!display;
+        opts.u.vnc.display = g_strdup(display);
+    }
+
+    qmp_expire_password(&opts, &err);
+    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 5c0d5e116b..cb229c01f8 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -163,45 +163,30 @@ void qmp_system_wakeup(Error **errp)
     qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
 }
 
-void qmp_set_password(const char *protocol, const char *password,
-                      bool has_connected, const char *connected, Error **errp)
+void qmp_set_password(SetPasswordOptions *opts, Error **errp)
 {
-    int disconnect_if_connected = 0;
-    int fail_if_connected = 0;
-    int rc;
+    bool disconnect_if_connected = false;
+    bool fail_if_connected = false;
+    int rc = 0;
 
-    if (has_connected) {
-        if (strcmp(connected, "fail") == 0) {
-            fail_if_connected = 1;
-        } else if (strcmp(connected, "disconnect") == 0) {
-            disconnect_if_connected = 1;
-        } else if (strcmp(connected, "keep") == 0) {
-            /* nothing */
-        } else {
-            error_setg(errp, QERR_INVALID_PARAMETER, "connected");
-            return;
-        }
-    }
-
-    if (strcmp(protocol, "spice") == 0) {
+    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
         if (!qemu_using_spice(errp)) {
             return;
         }
-        rc = qemu_spice.set_passwd(password, fail_if_connected,
-                                   disconnect_if_connected);
-    } else if (strcmp(protocol, "vnc") == 0) {
-        if (fail_if_connected || disconnect_if_connected) {
-            /* vnc supports "connected=keep" only */
-            error_setg(errp, QERR_INVALID_PARAMETER, "connected");
-            return;
+        if (opts->u.spice.has_connected) {
+            fail_if_connected =
+                opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL;
+            disconnect_if_connected =
+                opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT;
         }
+        rc = qemu_spice.set_passwd(opts->password, fail_if_connected,
+                                   disconnect_if_connected);
+    } else if (opts->protocol == DISPLAY_PROTOCOL_VNC) {
         /* Note that setting an empty password will not disable login through
          * this interface. */
-        rc = vnc_display_password(NULL, password);
-    } else {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
-                   "'vnc' or 'spice'");
-        return;
+        rc = vnc_display_password(
+                opts->u.vnc.has_display ? opts->u.vnc.display : NULL,
+                opts->password);
     }
 
     if (rc != 0) {
@@ -209,11 +194,11 @@ void qmp_set_password(const char *protocol, const char *password,
     }
 }
 
-void qmp_expire_password(const char *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;
@@ -225,17 +210,14 @@ void qmp_expire_password(const char *protocol, const char *whenstr,
         when = strtoull(whenstr, NULL, 10);
     }
 
-    if (strcmp(protocol, "spice") == 0) {
+    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
         if (!qemu_using_spice(errp)) {
             return;
         }
         rc = qemu_spice.set_pw_expire(when);
-    } else if (strcmp(protocol, "vnc") == 0) {
-        rc = vnc_display_pw_expire(NULL, when);
-    } else {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
-                   "'vnc' or 'spice'");
-        return;
+    } else if (opts->protocol == DISPLAY_PROTOCOL_VNC) {
+        rc = vnc_display_pw_expire(
+                opts->u.vnc.has_display ? opts->u.vnc.display : NULL, when);
     }
 
     if (rc != 0) {
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'] } } }
 
 ##
 # @expire_password:
 #
 # Expire the password of a remote display server.
 #
-# @protocol: the name of the remote display protocol 'vnc' or 'spice'
-#
-# @time: when to expire the password.
-#
-#        - 'now' to expire the password immediately
-#        - 'never' to cancel password expiration
-#        - '+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.
-#
 # Example:
 #
 # -> { "execute": "expire_password", "arguments": { "protocol": "vnc",
@@ -71,7 +145,50 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
+{ 'command': 'expire_password', 'boxed': true, 'data': 'ExpirePasswordOptions' }
+
+##
+# @ExpirePasswordOptions:
+#
+# Data required to set password expiration on a display server protocol.
+#
+# @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
+#        - 'never' to cancel password expiration
+#        - '+INT' where INT is the number of seconds from now (integer)
+#        - 'INT' where INT is the absolute time in seconds
+#
+# 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' } }
 
 ##
 # @screendump:
-- 
2.30.2

Re: [PATCH v4 1/2] monitor/hmp: add support for flag argument with value

[Dr. David Alan Gilbert]

* Stefan Reiter (s.reiter@proxmox.com) wrote:
> Adds support for the "-xS" parameter type, where "-x" denotes a flag
> name and the "S" suffix indicates that this flag is supposed to take an
> arbitrary string parameter.
> 
> These parameters are always optional, the entry in the qdict will be
> omitted if the flag is not given.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>

It would be great if you could send a patch to update the big comment in
monitor/monitor-internal.h that describes all the parameters.
But that can come another time.

Dave

> ---
>  monitor/hmp.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index d50c3124e1..a32dce7a35 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -980,6 +980,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>              {
>                  const char *tmp = p;
>                  int skip_key = 0;
> +                int ret;
>                  /* option */
>  
>                  c = *typestr++;
> @@ -1002,8 +1003,22 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>                      }
>                      if (skip_key) {
>                          p = tmp;
> +                    } else if (*typestr == 'S') {
> +                        /* has option with string value */
> +                        typestr++;
> +                        tmp = p++;
> +                        while (qemu_isspace(*p)) {
> +                            p++;
> +                        }
> +                        ret = get_str(buf, sizeof(buf), &p);
> +                        if (ret < 0) {
> +                            monitor_printf(mon, "%s: value expected for -%c\n",
> +                                           cmd->name, *tmp);
> +                            goto fail;
> +                        }
> +                        qdict_put_str(qdict, key, buf);
>                      } else {
> -                        /* has option */
> +                        /* has boolean option */
>                          p++;
>                          qdict_put_bool(qdict, key, true);
>                      }
> -- 
> 2.30.2
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Dr. David Alan Gilbert]

* Stefan Reiter (s.reiter@proxmox.com) 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.
> 
> For QMP, the schema is updated to explicitly express the supported
> variants of the commands with protocol-discriminated unions.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  hmp-commands.hx    |  24 ++++---
>  monitor/hmp-cmds.c |  57 ++++++++++++++-
>  monitor/qmp-cmds.c |  62 ++++++----------
>  qapi/ui.json       | 173 +++++++++++++++++++++++++++++++++++++--------
>  4 files changed, 235 insertions(+), 81 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index cf723c69ac..d78e4cfc47 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:-dS,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:-dS",
> +        .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 b5e71d9e6f..48b3fe4519 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1451,10 +1451,41 @@ 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;
>  
> -    qmp_set_password(protocol, password, !!connected, connected, &err);
> +    SetPasswordOptions opts = {
> +        .password = g_strdup(password),

You're leaking that strdup on the error returns; you probably want to
add an error:  exit and goto it to do all the cleanup.

> +        .u.vnc.display = NULL,
> +    };
> +
> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> +                            DISPLAY_PROTOCOL_VNC, &err);
> +    if (err) {
> +        hmp_handle_error(mon, err);
> +        return;
> +    }
> +    opts.protocol = proto;
> +
> +    if (proto == DISPLAY_PROTOCOL_VNC) {
> +        opts.u.vnc.has_display = !!display;
> +        opts.u.vnc.display = g_strdup(display);
> +    } else if (proto == 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) {
> +            hmp_handle_error(mon, err);
> +            return;
> +        }
> +    }
> +
> +    qmp_set_password(&opts, &err);
> +    g_free(opts.password);
> +    g_free(opts.u.vnc.display);
>      hmp_handle_error(mon, err);
>  }
>  
> @@ -1462,9 +1493,31 @@ 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;
>  
> -    qmp_expire_password(protocol, whenstr, &err);
> +    ExpirePasswordOptions opts = {
> +        .time = g_strdup(whenstr),
> +        .u.vnc.display = NULL,
> +    };

Same here; that 'whenstr' gets leaked on errors.

> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> +                            DISPLAY_PROTOCOL_VNC, &err);
> +    if (err) {
> +        hmp_handle_error(mon, err);
> +        return;
> +    }
> +    opts.protocol = proto;
> +
> +    if (proto == DISPLAY_PROTOCOL_VNC) {
> +        opts.u.vnc.has_display = !!display;
> +        opts.u.vnc.display = g_strdup(display);
> +    }
> +
> +    qmp_expire_password(&opts, &err);
> +    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 5c0d5e116b..cb229c01f8 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -163,45 +163,30 @@ void qmp_system_wakeup(Error **errp)
>      qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
>  }
>  
> -void qmp_set_password(const char *protocol, const char *password,
> -                      bool has_connected, const char *connected, Error **errp)
> +void qmp_set_password(SetPasswordOptions *opts, Error **errp)
>  {
> -    int disconnect_if_connected = 0;
> -    int fail_if_connected = 0;
> -    int rc;
> +    bool disconnect_if_connected = false;
> +    bool fail_if_connected = false;
> +    int rc = 0;
>  
> -    if (has_connected) {
> -        if (strcmp(connected, "fail") == 0) {
> -            fail_if_connected = 1;
> -        } else if (strcmp(connected, "disconnect") == 0) {
> -            disconnect_if_connected = 1;
> -        } else if (strcmp(connected, "keep") == 0) {
> -            /* nothing */
> -        } else {
> -            error_setg(errp, QERR_INVALID_PARAMETER, "connected");
> -            return;
> -        }
> -    }
> -
> -    if (strcmp(protocol, "spice") == 0) {
> +    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
>          if (!qemu_using_spice(errp)) {
>              return;
>          }
> -        rc = qemu_spice.set_passwd(password, fail_if_connected,
> -                                   disconnect_if_connected);
> -    } else if (strcmp(protocol, "vnc") == 0) {
> -        if (fail_if_connected || disconnect_if_connected) {
> -            /* vnc supports "connected=keep" only */
> -            error_setg(errp, QERR_INVALID_PARAMETER, "connected");
> -            return;
> +        if (opts->u.spice.has_connected) {
> +            fail_if_connected =
> +                opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL;
> +            disconnect_if_connected =
> +                opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT;
>          }
> +        rc = qemu_spice.set_passwd(opts->password, fail_if_connected,
> +                                   disconnect_if_connected);
> +    } else if (opts->protocol == DISPLAY_PROTOCOL_VNC) {
>          /* Note that setting an empty password will not disable login through
>           * this interface. */
> -        rc = vnc_display_password(NULL, password);
> -    } else {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
> -                   "'vnc' or 'spice'");
> -        return;
> +        rc = vnc_display_password(
> +                opts->u.vnc.has_display ? opts->u.vnc.display : NULL,
> +                opts->password);
>      }
>  
>      if (rc != 0) {
> @@ -209,11 +194,11 @@ void qmp_set_password(const char *protocol, const char *password,
>      }
>  }
>  
> -void qmp_expire_password(const char *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;
> @@ -225,17 +210,14 @@ void qmp_expire_password(const char *protocol, const char *whenstr,
>          when = strtoull(whenstr, NULL, 10);
>      }
>  
> -    if (strcmp(protocol, "spice") == 0) {
> +    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
>          if (!qemu_using_spice(errp)) {
>              return;
>          }
>          rc = qemu_spice.set_pw_expire(when);
> -    } else if (strcmp(protocol, "vnc") == 0) {
> -        rc = vnc_display_pw_expire(NULL, when);
> -    } else {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
> -                   "'vnc' or 'spice'");
> -        return;
> +    } else if (opts->protocol == DISPLAY_PROTOCOL_VNC) {
> +        rc = vnc_display_pw_expire(
> +                opts->u.vnc.has_display ? opts->u.vnc.display : NULL, when);
>      }
>  
>      if (rc != 0) {
> 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'] } } }
>  
>  ##
>  # @expire_password:
>  #
>  # Expire the password of a remote display server.
>  #
> -# @protocol: the name of the remote display protocol 'vnc' or 'spice'
> -#
> -# @time: when to expire the password.
> -#
> -#        - 'now' to expire the password immediately
> -#        - 'never' to cancel password expiration
> -#        - '+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.
> -#
>  # Example:
>  #
>  # -> { "execute": "expire_password", "arguments": { "protocol": "vnc",
> @@ -71,7 +145,50 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
> +{ 'command': 'expire_password', 'boxed': true, 'data': 'ExpirePasswordOptions' }
> +
> +##
> +# @ExpirePasswordOptions:
> +#
> +# Data required to set password expiration on a display server protocol.
> +#
> +# @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
> +#        - 'never' to cancel password expiration
> +#        - '+INT' where INT is the number of seconds from now (integer)
> +#        - 'INT' where INT is the absolute time in seconds
> +#
> +# 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' } }
>  
>  ##
>  # @screendump:
> -- 
> 2.30.2

Hmm, I'd have to get Markus to check that qmp changes.

Dave

> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Eric Blake]

On Mon, Oct 11, 2021 at 04:03:01PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Reiter (s.reiter@proxmox.com) 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", ...
> > 

> > +++ b/monitor/hmp-cmds.c
> > @@ -1451,10 +1451,41 @@ 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;
> >  
> > -    qmp_set_password(protocol, password, !!connected, connected, &err);
> > +    SetPasswordOptions opts = {
> > +        .password = g_strdup(password),
> 
> You're leaking that strdup on the error returns; you probably want to
> add an error:  exit and goto it to do all the cleanup.

Or maybe there's a way to use g_autofree to let the compiler clean it
up automatically.

> 
> > +        .u.vnc.display = NULL,
> > +    };
> > +
> > +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> > +                            DISPLAY_PROTOCOL_VNC, &err);
> > +    if (err) {
> > +        hmp_handle_error(mon, err);
> > +        return;
> > +    }
> > +    opts.protocol = proto;
> > +
> > +    if (proto == DISPLAY_PROTOCOL_VNC) {
> > +        opts.u.vnc.has_display = !!display;
> > +        opts.u.vnc.display = g_strdup(display);
> > +    } else if (proto == 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) {
> > +            hmp_handle_error(mon, err);
> > +            return;
> > +        }
> > +    }
> > +
> > +    qmp_set_password(&opts, &err);
> > +    g_free(opts.password);
> > +    g_free(opts.u.vnc.display);

Hmm. Why are we hand-cleaning only portions of the QAPI type instead
of using the generated qapi_free_SetPasswordOptions() do to things?

> >      hmp_handle_error(mon, err);
> >  }
> >  
> > @@ -1462,9 +1493,31 @@ 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;
> >  
> > -    qmp_expire_password(protocol, whenstr, &err);
> > +    ExpirePasswordOptions opts = {
> > +        .time = g_strdup(whenstr),
> > +        .u.vnc.display = NULL,
> > +    };
> 
> Same here; that 'whenstr' gets leaked on errors.
> 
> > +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> > +                            DISPLAY_PROTOCOL_VNC, &err);
> > +    if (err) {
> > +        hmp_handle_error(mon, err);
> > +        return;
> > +    }
> > +    opts.protocol = proto;
> > +
> > +    if (proto == DISPLAY_PROTOCOL_VNC) {
> > +        opts.u.vnc.has_display = !!display;
> > +        opts.u.vnc.display = g_strdup(display);
> > +    }
> > +
> > +    qmp_expire_password(&opts, &err);
> > +    g_free(opts.time);
> > +    g_free(opts.u.vnc.display);
> >      hmp_handle_error(mon, err);

Same here - using the generated qapi_free_ function rather than doing
things by hand, and/or smart use of g_autofree, may make this easier
to maintain.


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

Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Markus Armbruster]

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: Eric Blake <eblake@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Reiter <s.reiter@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?

Ignorant question: is it possible to have more than one 'spice' display?

Item 5. is not a compatibility break: before your patch,
qmp_set_password() rejects the values you drop.

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'd split this patch into three parts: item 1., 2.+3., 4.-6., because
each part can stand on its own.

>  
>  ##
>  # @expire_password:
>  #
>  # Expire the password of a remote display server.
>  #
> -# @protocol: the name of the remote display protocol 'vnc' or 'spice'
> -#
> -# @time: when to expire the password.
> -#
> -#        - 'now' to expire the password immediately
> -#        - 'never' to cancel password expiration
> -#        - '+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.
> -#
>  # Example:
>  #
>  # -> { "execute": "expire_password", "arguments": { "protocol": "vnc",
> @@ -71,7 +145,50 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
> +{ 'command': 'expire_password', 'boxed': true, 'data': 'ExpirePasswordOptions' }
> +
> +##
> +# @ExpirePasswordOptions:
> +#
> +# Data required to set password expiration on a display server protocol.
> +#
> +# @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
> +#        - 'never' to cancel password expiration
> +#        - '+INT' where INT is the number of seconds from now (integer)
> +#        - 'INT' where INT is the absolute time in seconds
> +#
> +# 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' } }
>  
>  ##
>  # @screendump:

Same as above, less item 4.-6.

Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Stefan Reiter]

On 10/12/21 11:24 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: Eric Blake <eblake@redhat.com>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Stefan Reiter <s.reiter@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.

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

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

> 
>>   
>>   ##
>>   # @expire_password:
>>   #
>>   # Expire the password of a remote display server.
>>   #
>> -# @protocol: the name of the remote display protocol 'vnc' or 'spice'
>> -#
>> -# @time: when to expire the password.
>> -#
>> -#        - 'now' to expire the password immediately
>> -#        - 'never' to cancel password expiration
>> -#        - '+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.
>> -#
>>   # Example:
>>   #
>>   # -> { "execute": "expire_password", "arguments": { "protocol": "vnc",
>> @@ -71,7 +145,50 @@
>>   # <- { "return": {} }
>>   #
>>   ##
>> -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
>> +{ 'command': 'expire_password', 'boxed': true, 'data': 'ExpirePasswordOptions' }
>> +
>> +##
>> +# @ExpirePasswordOptions:
>> +#
>> +# Data required to set password expiration on a display server protocol.
>> +#
>> +# @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
>> +#        - 'never' to cancel password expiration
>> +#        - '+INT' where INT is the number of seconds from now (integer)
>> +#        - 'INT' where INT is the absolute time in seconds
>> +#
>> +# 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' } }
>>   
>>   ##
>>   # @screendump:
> 
> Same as above, less item 4.-6.
> 
> 

Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Markus Armbruster]

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

> On 10/12/21 11:24 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 à "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 <eblake@redhat.com>
>>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Stefan Reiter <s.reiter@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?

[...]

Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Stefan Reiter]

On 10/14/21 9:14 AM, Markus Armbruster wrote:
> Stefan Reiter <s.reiter@proxmox.com> writes:
> 
>> On 10/12/21 11:24 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 à "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 <eblake@redhat.com>
>>>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>>>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>>>
>>> [...]
>>>
>>>> [...]
>>>
>>> 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.

I'll just have a go and see what falls out. I do agree that this patch is a
bit long on its own.

> 
>>                       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?

Well, before this patch 'hmp_set_password' was for all intents and purposes a
single function call to 'qmp_set_password'. Now it has a bunch of parameter
parsing and even validation (e.g. enum parsing). That's why I asked in the
v3 patch (I think?) if there was a way to call the QAPI style parsing from
there, since the parameters are all named the same and in a qdict already..

Something like:

   void hmp_set_password(Monitor *mon, const QDict *qdict)
   {
     ExpirePasswordOptions opts = qapi_magical_parse_fn(qdict);
     qmp_set_password(&opts,·&err);
     [error handling]
   }

That being said, I don't mind the current form enough to make this a bigger
discussion either, so if there isn't an easy way to do the above, let's just
leave it like it is.

Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Markus Armbruster]

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

> On 10/14/21 9:14 AM, Markus Armbruster wrote:
>> Stefan Reiter <s.reiter@proxmox.com> writes:
>> 
>>> On 10/12/21 11:24 AM, Markus Armbruster wrote:

[...]

>>>> 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.
>
> I'll just have a go and see what falls out. I do agree that this patch is a
> bit long on its own.

Thanks!

>>>                       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?
>
> Well, before this patch 'hmp_set_password' was for all intents and purposes a
> single function call to 'qmp_set_password'. Now it has a bunch of parameter
> parsing and even validation (e.g. enum parsing).

Yes, HMP requires us to do more work manually than QMP does.  Raising
HMP's level of automation to QMP's would be nice, but it would also be a
big project.

>                                                  That's why I asked in the
> v3 patch (I think?) if there was a way to call the QAPI style parsing from
> there, since the parameters are all named the same and in a qdict already..
>
> Something like:
>
>    void hmp_set_password(Monitor *mon, const QDict *qdict)
>    {
>      ExpirePasswordOptions opts = qapi_magical_parse_fn(qdict);
>      qmp_set_password(&opts, &err);
>      [error handling]
>    }

Same structure as qmp_marshal_set_password(), where the "magical parse"
part uses a visitor function generated from the QAPI schema for its
argument type.

HMP works differently.  There, we only have .args_type in
hmp-commands.hx.  Since this is much less expressive than the QAPI
schema, generic code can do much less work for us.  Which means we get
to write more code by hand.

By converting QMP from 'str' to enum, we lift parsing from the
qmp_set_password() to its callers.  qmp_marshal_set_password() does it
for free.  hmp_set_password() needs handwritten code.  It wouldn't with
a QAPI-schema-based HMP, but as I said, that's a big project.

> That being said, I don't mind the current form enough to make this a bigger
> discussion either, so if there isn't an easy way to do the above, let's just
> leave it like it is.

There is no easy way to do the above.