[PATCH v7 0/4] VNC-related HMP/QMP fixes

[PATCH v7 0/4] 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.

v6 -> v7:
* remove g_strdup and g_free, use strings directly
* squash in last patch

v5 -> v6:
* consider feedback from Markus' review, mainly:
  * fix crash bug in patch 1 (sorry, artifact of patch-splitting)
  * rely on '!has_param => param == NULL' to shorten code
  * add note to 'docs/about/deprecated.rst' and touch up comments a bit
* go back to g_free instead of qapi_free_* since the latter apparently tries to
  free the passed in pointer which lives on the stack...
* fix bug in HMP parsing (see patch 1)

v4 -> v5:
* add comment to patch 1 in "monitor-internal.h"
* use qapi_free_SetPasswordOptions and friends, don't leak strdups
* split QAPI change into 3 seperate patches

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 (4):
  monitor/hmp: add support for flag argument with value
  qapi/monitor: refactor set/expire_password with enums
  qapi/monitor: allow VNC display id in set/expire_password
  qapi/monitor: only allow 'keep' SetPasswordAction for VNC and
    deprecate

 docs/about/deprecated.rst  |   6 ++
 hmp-commands.hx            |  24 +++---
 monitor/hmp-cmds.c         |  48 +++++++++++-
 monitor/hmp.c              |  19 ++++-
 monitor/monitor-internal.h |   3 +-
 monitor/qmp-cmds.c         |  54 ++++---------
 qapi/ui.json               | 156 ++++++++++++++++++++++++++++++++-----
 7 files changed, 236 insertions(+), 74 deletions(-)

-- 
2.30.2

[PATCH v7 1/4] monitor/hmp: add support for flag argument with value

[Stefan Reiter]

Adds support for the "-xV" parameter type, where "-x" denotes a flag
name and the "V" 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.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

v6:
It wasn't possible to pass the 'connected' parameter to set_password, since the
code to handle optional parameters couldn't live with a different param (not
starting with '-') coming up instead - fix that by advancing over the 'value
flag' modifier in case `*p != '-'`.

Also change the modifier to 'V' instead of 'S' so it can be distinguished from
an actual trailing 'S' type param.

Discovered in testing. I dropped Eric's R-b due to the code change.

 monitor/hmp.c              | 19 ++++++++++++++++++-
 monitor/monitor-internal.h |  3 ++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index d50c3124e1..899e0c990f 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,11 +1003,27 @@ static QDict *monitor_parse_arguments(Monitor *mon,
                     }
                     if (skip_key) {
                         p = tmp;
+                    } else if (*typestr == 'V') {
+                        /* 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);
                     }
+                } else if (*typestr == 'V') {
+                    typestr++;
                 }
             }
             break;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 9c3a09cb01..9e708b329d 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -63,7 +63,8 @@
  * '.'          other form of optional type (for 'i' and 'l')
  * 'b'          boolean
  *              user mode accepts "on" or "off"
- * '-'          optional parameter (eg. '-f')
+ * '-'          optional parameter (eg. '-f'); if followed by an 'V', it
+ *              specifies an optional string param (e.g. '-fV' allows '-f foo')
  *
  */
 
-- 
2.30.2

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

[Dr. David Alan Gilbert]

* Stefan Reiter (s.reiter@proxmox.com) wrote:
> Adds support for the "-xV" parameter type, where "-x" denotes a flag
> name and the "V" 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.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
> 
> v6:
> It wasn't possible to pass the 'connected' parameter to set_password, since the
> code to handle optional parameters couldn't live with a different param (not
> starting with '-') coming up instead - fix that by advancing over the 'value
> flag' modifier in case `*p != '-'`.
> 
> Also change the modifier to 'V' instead of 'S' so it can be distinguished from
> an actual trailing 'S' type param.
> 
> Discovered in testing. I dropped Eric's R-b due to the code change.
> 
>  monitor/hmp.c              | 19 ++++++++++++++++++-
>  monitor/monitor-internal.h |  3 ++-
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index d50c3124e1..899e0c990f 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,11 +1003,27 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>                      }
>                      if (skip_key) {
>                          p = tmp;
> +                    } else if (*typestr == 'V') {
> +                        /* 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);
>                      }
> +                } else if (*typestr == 'V') {
> +                    typestr++;
>                  }
>              }
>              break;
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 9c3a09cb01..9e708b329d 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -63,7 +63,8 @@
>   * '.'          other form of optional type (for 'i' and 'l')
>   * 'b'          boolean
>   *              user mode accepts "on" or "off"
> - * '-'          optional parameter (eg. '-f')
> + * '-'          optional parameter (eg. '-f'); if followed by an 'V', it
> + *              specifies an optional string param (e.g. '-fV' allows '-f foo')
>   *
>   */
>  
> -- 
> 2.30.2
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Eric Blake]

On Thu, Oct 21, 2021 at 12:01:32PM +0200, Stefan Reiter wrote:
> Adds support for the "-xV" parameter type, where "-x" denotes a flag
> name and the "V" 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.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---

[so my late v6 reply doesn't get lost...]

> +++ b/monitor/monitor-internal.h
> @@ -63,7 +63,8 @@
>   * '.'          other form of optional type (for 'i' and 'l')
>   * 'b'          boolean
>   *              user mode accepts "on" or "off"
> - * '-'          optional parameter (eg. '-f')
> + * '-'          optional parameter (eg. '-f'); if followed by an 'V', it

s/an/a/

Reviewed-by: Eric Blake <eblake@redhat.com>

> + *              specifies an optional string param (e.g. '-fV' allows '-f foo')
>   *
>   */
>  
> -- 
> 2.30.2
> 
> 

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

[PATCH v7 2/4] qapi/monitor: refactor set/expire_password with enums

[Stefan Reiter]

'protocol' and 'connected' are better suited as enums than as strings,
make use of that. No functional change intended.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 monitor/hmp-cmds.c | 29 +++++++++++++++++++++++++++--
 monitor/qmp-cmds.c | 37 ++++++++++++-------------------------
 qapi/ui.json       | 37 +++++++++++++++++++++++++++++++++++--
 3 files changed, 74 insertions(+), 29 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index bcaa41350e..b8abe69609 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1453,8 +1453,24 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
     const char *password  = qdict_get_str(qdict, "password");
     const char *connected = qdict_get_try_str(qdict, "connected");
     Error *err = NULL;
+    DisplayProtocol proto;
+    SetPasswordAction conn;
 
-    qmp_set_password(protocol, password, !!connected, connected, &err);
+    proto = 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;
+    }
+
+    qmp_set_password(proto, password, !!connected, conn, &err);
+
+out:
     hmp_handle_error(mon, err);
 }
 
@@ -1463,8 +1479,17 @@ 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");
     Error *err = NULL;
+    DisplayProtocol proto;
 
-    qmp_expire_password(protocol, whenstr, &err);
+    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+                            DISPLAY_PROTOCOL_VNC, &err);
+    if (err) {
+        goto out;
+    }
+
+    qmp_expire_password(proto, whenstr, &err);
+
+out:
     hmp_handle_error(mon, err);
 }
 
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5c0d5e116b..0654d7289a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -163,33 +163,27 @@ 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(DisplayProtocol protocol, const char *password,
+                      bool has_connected, SetPasswordAction connected,
+                      Error **errp)
 {
     int disconnect_if_connected = 0;
     int fail_if_connected = 0;
     int rc;
 
     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;
-        }
+        fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL;
+        disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT;
     }
 
-    if (strcmp(protocol, "spice") == 0) {
+    if (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) {
+    } else {
+        assert(protocol == DISPLAY_PROTOCOL_VNC);
         if (fail_if_connected || disconnect_if_connected) {
             /* vnc supports "connected=keep" only */
             error_setg(errp, QERR_INVALID_PARAMETER, "connected");
@@ -198,10 +192,6 @@ void qmp_set_password(const char *protocol, const char *password,
         /* 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;
     }
 
     if (rc != 0) {
@@ -209,7 +199,7 @@ void qmp_set_password(const char *protocol, const char *password,
     }
 }
 
-void qmp_expire_password(const char *protocol, const char *whenstr,
+void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
                          Error **errp)
 {
     time_t when;
@@ -225,17 +215,14 @@ void qmp_expire_password(const char *protocol, const char *whenstr,
         when = strtoull(whenstr, NULL, 10);
     }
 
-    if (strcmp(protocol, "spice") == 0) {
+    if (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;
+        assert(protocol == DISPLAY_PROTOCOL_VNC);
+        rc = vnc_display_pw_expire(NULL, when);
     }
 
     if (rc != 0) {
diff --git a/qapi/ui.json b/qapi/ui.json
index d7567ac866..15cc19dcc5 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -9,6 +9,35 @@
 { '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' } ] }
+
+##
+# @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' ] }
+
 ##
 # @set_password:
 #
@@ -38,7 +67,9 @@
 #
 ##
 { 'command': 'set_password',
-  'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
+  'data': { 'protocol': 'DisplayProtocol',
+            'password': 'str',
+            '*connected': 'SetPasswordAction' } }
 
 ##
 # @expire_password:
@@ -71,7 +102,9 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
+{ 'command': 'expire_password',
+  'data': { 'protocol': 'DisplayProtocol',
+            'time': 'str' } }
 
 ##
 # @screendump:
-- 
2.30.2

Re: [PATCH v7 2/4] qapi/monitor: refactor set/expire_password with enums

[Fabian Ebner]

Am 21.10.21 um 12:01 schrieb Stefan Reiter:
> 'protocol' and 'connected' are better suited as enums than as strings,
> make use of that. No functional change intended.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>   monitor/hmp-cmds.c | 29 +++++++++++++++++++++++++++--
>   monitor/qmp-cmds.c | 37 ++++++++++++-------------------------
>   qapi/ui.json       | 37 +++++++++++++++++++++++++++++++++++--
>   3 files changed, 74 insertions(+), 29 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index bcaa41350e..b8abe69609 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1453,8 +1453,24 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
>       const char *password  = qdict_get_str(qdict, "password");
>       const char *connected = qdict_get_try_str(qdict, "connected");
>       Error *err = NULL;
> +    DisplayProtocol proto;
> +    SetPasswordAction conn;
>   
> -    qmp_set_password(protocol, password, !!connected, connected, &err);
> +    proto = 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;
> +    }
> +
> +    qmp_set_password(proto, password, !!connected, conn, &err);
> +
> +out:
>       hmp_handle_error(mon, err);
>   }
>   
> @@ -1463,8 +1479,17 @@ 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");
>       Error *err = NULL;
> +    DisplayProtocol proto;
>   
> -    qmp_expire_password(protocol, whenstr, &err);
> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> +                            DISPLAY_PROTOCOL_VNC, &err);
> +    if (err) {
> +        goto out;
> +    }
> +
> +    qmp_expire_password(proto, whenstr, &err);
> +
> +out:
>       hmp_handle_error(mon, err);
>   }
>   
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 5c0d5e116b..0654d7289a 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -163,33 +163,27 @@ 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(DisplayProtocol protocol, const char *password,
> +                      bool has_connected, SetPasswordAction connected,
> +                      Error **errp)
>   {
>       int disconnect_if_connected = 0;
>       int fail_if_connected = 0;
>       int rc;
>   
>       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;
> -        }
> +        fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL;
> +        disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT;
>       }
>   
> -    if (strcmp(protocol, "spice") == 0) {
> +    if (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) {
> +    } else {
> +        assert(protocol == DISPLAY_PROTOCOL_VNC);
>           if (fail_if_connected || disconnect_if_connected) {
>               /* vnc supports "connected=keep" only */
>               error_setg(errp, QERR_INVALID_PARAMETER, "connected");
> @@ -198,10 +192,6 @@ void qmp_set_password(const char *protocol, const char *password,
>           /* 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;
>       }
>   
>       if (rc != 0) {
> @@ -209,7 +199,7 @@ void qmp_set_password(const char *protocol, const char *password,
>       }
>   }
>   
> -void qmp_expire_password(const char *protocol, const char *whenstr,
> +void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
>                            Error **errp)
>   {
>       time_t when;
> @@ -225,17 +215,14 @@ void qmp_expire_password(const char *protocol, const char *whenstr,
>           when = strtoull(whenstr, NULL, 10);
>       }
>   
> -    if (strcmp(protocol, "spice") == 0) {
> +    if (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;
> +        assert(protocol == DISPLAY_PROTOCOL_VNC);
> +        rc = vnc_display_pw_expire(NULL, when);
>       }
>   
>       if (rc != 0) {
> diff --git a/qapi/ui.json b/qapi/ui.json
> index d7567ac866..15cc19dcc5 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -9,6 +9,35 @@
>   { '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' } ] }
> +
> +##
> +# @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' ] }

Since 'keep' should be the default, shouldn't it come first? I didn't 
find an explicit mention in the QAPI docs, but testing suggests that the 
first member will be picked. Is that correct?

qmp_set_password still relies on has_connected to guard its checks here, 
but the next patch removes that, which AFAICT makes the default be 
'fail' instead of keeping 'keep'. While it's only temporary breakage for 
VNC as the final patch in the series allows only 'keep' (still, should 
be avoided if possible), it does matter for SPICE.

> +
>   ##
>   # @set_password:
>   #
> @@ -38,7 +67,9 @@
>   #
>   ##
>   { 'command': 'set_password',
> -  'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
> +  'data': { 'protocol': 'DisplayProtocol',
> +            'password': 'str',
> +            '*connected': 'SetPasswordAction' } }
>   
>   ##
>   # @expire_password:
> @@ -71,7 +102,9 @@
>   # <- { "return": {} }
>   #
>   ##
> -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
> +{ 'command': 'expire_password',
> +  'data': { 'protocol': 'DisplayProtocol',
> +            'time': 'str' } }
>   
>   ##
>   # @screendump:

Re: [PATCH v7 2/4] qapi/monitor: refactor set/expire_password with enums

[Markus Armbruster]

Fabian Ebner <f.ebner@proxmox.com> writes:

> Am 21.10.21 um 12:01 schrieb Stefan Reiter:
>> 'protocol' and 'connected' are better suited as enums than as strings,
>> make use of that. No functional change intended.
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-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..15cc19dcc5 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -9,6 +9,35 @@
>>   { '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' } ] }
>> +
>> +##
>> +# @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' ] }
>
> Since 'keep' should be the default, shouldn't it come first? I didn't
> find an explicit mention in the QAPI docs, but testing suggests that
> the first member will be picked. Is that correct?

Not quite.

An optional member @connected generates a pair of C struct members
@connected and @has_connected.

If @has_connected is true, the argument is present, and @connected is
its value.

If @has_connected is false, the argument is absent.  The input visitor
zeros @connected then.  Other code should as well, for robustness, but I
wouldn't bet my own money on it.

Putting the default value first in an enum makes it zero in C.  Instead
of

    has_connected ? connected : SET_PASSWORD_ACTION_KEEP

you can then write just

   connected

when you know absent values are zero.  Easier on the eyes.

A possible improvement to the QAPI schema language: optional members may
have a default value.  When given, we don't generate has_FOO.

> qmp_set_password still relies on has_connected to guard its checks
> here, but the next patch removes that, which AFAICT makes the default
> be 'fail' instead of keeping 'keep'. While it's only temporary
> breakage for VNC as the final patch in the series allows only 'keep'
> (still, should be avoided if possible), it does matter for SPICE.

Even temporary breakage should be avoided whenever practical.

[...]

[PATCH v7 3/4] qapi/monitor: allow VNC display id in set/expire_password

[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: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 hmp-commands.hx    |  24 +++++-----
 monitor/hmp-cmds.c |  45 ++++++++++++------
 monitor/qmp-cmds.c |  36 ++++++---------
 qapi/ui.json       | 112 +++++++++++++++++++++++++++++++++++----------
 4 files changed, 148 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..f0f0c82d59 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,24 +1451,34 @@ 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 = (char *)password,
+    };
+
+    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 = (char *)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:
     hmp_handle_error(mon, err);
@@ -1478,16 +1488,25 @@ 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 = (char *)whenstr,
+    };
+
+    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 = (char *)display;
+    }
+
+    qmp_expire_password(&opts, &err);
 
 out:
     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

Re: [PATCH v7 3/4] qapi/monitor: allow VNC display id in set/expire_password

[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: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v7 3/4] qapi/monitor: allow VNC display id in set/expire_password

[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: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>

For the HMP side:


Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hmp-commands.hx    |  24 +++++-----
>  monitor/hmp-cmds.c |  45 ++++++++++++------
>  monitor/qmp-cmds.c |  36 ++++++---------
>  qapi/ui.json       | 112 +++++++++++++++++++++++++++++++++++----------
>  4 files changed, 148 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..f0f0c82d59 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1451,24 +1451,34 @@ 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 = (char *)password,
> +    };
> +
> +    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 = (char *)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:
>      hmp_handle_error(mon, err);
> @@ -1478,16 +1488,25 @@ 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 = (char *)whenstr,
> +    };
> +
> +    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 = (char *)display;
> +    }
> +
> +    qmp_expire_password(&opts, &err);
>  
>  out:
>      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
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v7 4/4] qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate

[Stefan Reiter]

VNC only supports 'keep' here, enforce this via a seperate
SetPasswordActionVnc enum and mark the option 'deprecated' (as it is
useless with only one value possible).

Also add a deprecation note to docs.

Suggested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 docs/about/deprecated.rst |  6 ++++++
 monitor/qmp-cmds.c        |  5 -----
 qapi/ui.json              | 21 ++++++++++++++++++++-
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 0bed6ecb1d..f484b058bc 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -228,6 +228,12 @@ Use the more generic commands ``block-export-add`` and ``block-export-del``
 instead.  As part of this deprecation, where ``nbd-server-add`` used a
 single ``bitmap``, the new ``block-export-add`` uses a list of ``bitmaps``.
 
+``set_password`` argument ``connected`` for VNC protocol (since 6.2)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Only the value ``keep`` is and was ever supported for VNC. The (useless)
+argument will be dropped in a future version of QEMU.
+
 System accelerators
 -------------------
 
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5637bd70b6..4825d0cbea 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -176,11 +176,6 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
                 opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT);
     } else {
         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(opts->u.vnc.display, opts->password);
diff --git a/qapi/ui.json b/qapi/ui.json
index 99ac29ad9c..5292617b44 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -38,6 +38,20 @@
 { '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' ] }
+
 ##
 # @SetPasswordOptions:
 #
@@ -83,12 +97,17 @@
 # @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': 'SetPasswordAction' }}
+            '*connected': { 'type': 'SetPasswordActionVnc',
+                            'features': ['deprecated'] } } }
 
 ##
 # @set_password:
-- 
2.30.2

Re: [PATCH v7 4/4] qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate

[Daniel P. Berrangé]

On Thu, Oct 21, 2021 at 12:01:35PM +0200, Stefan Reiter wrote:
> VNC only supports 'keep' here, enforce this via a seperate
> SetPasswordActionVnc enum and mark the option 'deprecated' (as it is
> useless with only one value possible).
> 
> Also add a deprecation note to docs.

IMHO we should just implement 'fail' and 'disconnect' in the VNC
server. Consistency across SPICE and VNC is more user friendly
than restricting the available options, and all three policies
are useful behaviours for VNC.

So I'm not in favour of adding this deprecation.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Markus Armbruster]

I'm done reviewing.  David, care to have another look at the HMP part?

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> I'm done reviewing.  David, care to have another look at the HMP part?

Yep, looking good to me - is that going via qmp, hmp, or vnc ?

Dave

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

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> I'm done reviewing.  David, care to have another look at the HMP part?
>
> Yep, looking good to me - is that going via qmp, hmp, or vnc ?

Either is fine with me.

David, Gerd, do you have anything queued up already?

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Gerd Hoffmann]

On Tue, Oct 26, 2021 at 01:32:29PM +0200, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> I'm done reviewing.  David, care to have another look at the HMP part?
> >
> > Yep, looking good to me - is that going via qmp, hmp, or vnc ?
> 
> Either is fine with me.
> 
> David, Gerd, do you have anything queued up already?

Nothing queued atm, no objections to someone else picking this up.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> I'm done reviewing.  David, care to have another look at the HMP part?
> >
> > Yep, looking good to me - is that going via qmp, hmp, or vnc ?
> 
> Either is fine with me.
> 
> David, Gerd, do you have anything queued up already?

No I've also got nothing else at th emoment.

Dave

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

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Markus Armbruster]

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

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

Queued, thanks!

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Stefan Reiter <s.reiter@proxmox.com> writes:
>
>> 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.
>
> Queued, thanks!

Unqueued, because it fails to compile with --disable-vnc and with
--disable-spice.  I failed to catch this in review, sorry.

Making it work takes a tiresome amount of #ifdeffery (sketch appended).
Missing: removal of stubs that are no longer used,
e.g. vnc_display_password() in ui/vnc-stubs.c.  Feels like more trouble
than it's worth.

To maximize our chances to get this into 6.2, please respin without the
'if' conditionals.  Yes, this makes introspection less useful, but it's
no worse than before the patch.


diff --git a/qapi/ui.json b/qapi/ui.json
index 5292617b44..39ca0b5ead 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -69,8 +69,10 @@
   'base': { 'protocol': 'DisplayProtocol',
             'password': 'str' },
   'discriminator': 'protocol',
-  'data': { 'vnc': 'SetPasswordOptionsVnc',
-            'spice': 'SetPasswordOptionsSpice' } }
+  'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
+                     'if': 'CONFIG_VNC' },
+            'spice': { 'type': 'SetPasswordOptionsSpice',
+                       'if': 'CONFIG_SPICE' } } }
 
 ##
 # @SetPasswordOptionsSpice:
@@ -155,7 +157,8 @@
   'base': { 'protocol': 'DisplayProtocol',
             'time': 'str' },
   'discriminator': 'protocol',
-  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
+  'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
+                     'if': 'CONFIG_VNC' } } }
 
 ##
 # @ExpirePasswordOptionsVnc:
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index f0f0c82d59..f714b2d316 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,24 +1451,40 @@ 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");
+#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
     const char *display = qdict_get_try_str(qdict, "display");
+#endif
+#ifdef CONFIG_SPICE
     const char *connected = qdict_get_try_str(qdict, "connected");
+#endif
     Error *err = NULL;
+    int proto;
 
     SetPasswordOptions opts = {
         .password = (char *)password,
     };
 
-    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-                                    DISPLAY_PROTOCOL_VNC, &err);
+    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err);
     if (err) {
         goto out;
     }
 
-    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+    switch (proto) {
+#ifdef CONFIG_VNC
+    case -1:
+        proto = DISPLAY_PROTOCOL_VNC;
+        /* fall through */
+    case DISPLAY_PROTOCOL_VNC:
         opts.u.vnc.has_display = !!display;
         opts.u.vnc.display = (char *)display;
-    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
+        break;
+#else
+    case -1:
+        error_setg(&err, "FIXME");
+        goto out;
+#endif
+#ifdef CONFIG_SPICE
+    case DISPLAY_PROTOCOL_SPICE:
         opts.u.spice.has_connected = !!connected;
         opts.u.spice.connected =
             qapi_enum_parse(&SetPasswordAction_lookup, connected,
@@ -1476,8 +1492,13 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
         if (err) {
             goto out;
         }
+        break;
+#endif
+    default:
+        ;
     }
 
+    opts.protocol = proto;
     qmp_set_password(&opts, &err);
 
 out:
@@ -1488,22 +1509,34 @@ 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");
+#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
     const char *display = qdict_get_try_str(qdict, "display");
+#endif
     Error *err = NULL;
+    int proto;
 
     ExpirePasswordOptions opts = {
         .time = (char *)whenstr,
     };
 
-    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-                                    DISPLAY_PROTOCOL_VNC, &err);
+    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err);
     if (err) {
         goto out;
     }
 
-    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+    switch (proto) {
+#ifdef CONFIG_VNC
+    case -1:
+        proto = DISPLAY_PROTOCOL_VNC;
+        /* fall through */
+    case DISPLAY_PROTOCOL_VNC:
         opts.u.vnc.has_display = !!display;
         opts.u.vnc.display = (char *)display;
+#else
+    case -1:
+        error_setg(&err, "FIXME");
+        goto out;
+#endif
     }
 
     qmp_expire_password(&opts, &err);
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 4825d0cbea..69a9c2977a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -167,18 +167,26 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
 {
     int rc = 0;
 
-    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
+    switch (opts->protocol) {
+#ifdef CONFIG_SPICE
+    case DISPLAY_PROTOCOL_SPICE:
         if (!qemu_using_spice(errp)) {
             return;
         }
         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(opts->protocol == DISPLAY_PROTOCOL_VNC);
+        break;
+#endif
+#ifdef CONFIG_VNC
+    case DISPLAY_PROTOCOL_VNC:
         /* Note that setting an empty password will not disable login through
          * this interface. */
         rc = vnc_display_password(opts->u.vnc.display, opts->password);
+        break;
+#endif
+    default:
+        abort();
     }
 
     if (rc != 0) {
@@ -202,14 +210,22 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
         when = strtoull(whenstr, NULL, 10);
     }
 
-    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
+    switch (opts->protocol) {
+#ifdef CONFIG_SPICE
+    case DISPLAY_PROTOCOL_SPICE:
         if (!qemu_using_spice(errp)) {
             return;
         }
         rc = qemu_spice.set_pw_expire(when);
-    } else {
-        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
+        break;
+#endif
+#ifdef CONFIG_VNC
+    case DISPLAY_PROTOCOL_VNC:
         rc = vnc_display_pw_expire(opts->u.vnc.display, when);
+        break;
+#endif
+    default:
+        abort();
     }
 
     if (rc != 0) {

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Fabian Ebner]

Am 28.10.21 um 21:37 schrieb Markus Armbruster:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Stefan Reiter <s.reiter@proxmox.com> writes:
>>
>>> 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.
>>
>> Queued, thanks!
> 
> Unqueued, because it fails to compile with --disable-vnc and with
> --disable-spice.  I failed to catch this in review, sorry.
> 
> Making it work takes a tiresome amount of #ifdeffery (sketch appended).
> Missing: removal of stubs that are no longer used,
> e.g. vnc_display_password() in ui/vnc-stubs.c.  Feels like more trouble
> than it's worth.
> 
> To maximize our chances to get this into 6.2, please respin without the
> 'if' conditionals.  Yes, this makes introspection less useful, but it's
> no worse than before the patch.
> 

Unfortunately, Stefan is no longer working with Proxmox, so I would pick 
up these patches instead.

Since the 6.2 ship has long sailed, I suppose the way forward is using 
the #ifdefs then?

 From my understanding what should be done is:

* In the first patch, fix the typo spotted by Eric Blake and add the R-b 
tags from this round.

* Replace "since 6.2" with "since 7.0" everywhere.

* Incorporate the #ifdef handling from below. I had to add another one 
for the when/whenstr handling in qmp_expire_password to avoid an error 
with -Werror=unused-but-set-variable.

* Add #ifdefs for the unused stubs too? If yes, how to best find them?

> 
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 5292617b44..39ca0b5ead 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -69,8 +69,10 @@
>     'base': { 'protocol': 'DisplayProtocol',
>               'password': 'str' },
>     'discriminator': 'protocol',
> -  'data': { 'vnc': 'SetPasswordOptionsVnc',
> -            'spice': 'SetPasswordOptionsSpice' } }
> +  'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
> +                     'if': 'CONFIG_VNC' },
> +            'spice': { 'type': 'SetPasswordOptionsSpice',
> +                       'if': 'CONFIG_SPICE' } } }
>   
>   ##
>   # @SetPasswordOptionsSpice:
> @@ -155,7 +157,8 @@
>     'base': { 'protocol': 'DisplayProtocol',
>               'time': 'str' },
>     'discriminator': 'protocol',
> -  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
> +  'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
> +                     'if': 'CONFIG_VNC' } } }
>   
>   ##
>   # @ExpirePasswordOptionsVnc:
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index f0f0c82d59..f714b2d316 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1451,24 +1451,40 @@ 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");
> +#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
>       const char *display = qdict_get_try_str(qdict, "display");
> +#endif
> +#ifdef CONFIG_SPICE
>       const char *connected = qdict_get_try_str(qdict, "connected");
> +#endif
>       Error *err = NULL;
> +    int proto;
>   
>       SetPasswordOptions opts = {
>           .password = (char *)password,
>       };
>   
> -    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> -                                    DISPLAY_PROTOCOL_VNC, &err);
> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err);
>       if (err) {
>           goto out;
>       }
>   
> -    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
> +    switch (proto) {
> +#ifdef CONFIG_VNC
> +    case -1:
> +        proto = DISPLAY_PROTOCOL_VNC;
> +        /* fall through */
> +    case DISPLAY_PROTOCOL_VNC:
>           opts.u.vnc.has_display = !!display;
>           opts.u.vnc.display = (char *)display;
> -    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
> +        break;
> +#else
> +    case -1:
> +        error_setg(&err, "FIXME");
> +        goto out;
> +#endif
> +#ifdef CONFIG_SPICE
> +    case DISPLAY_PROTOCOL_SPICE:
>           opts.u.spice.has_connected = !!connected;
>           opts.u.spice.connected =
>               qapi_enum_parse(&SetPasswordAction_lookup, connected,
> @@ -1476,8 +1492,13 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
>           if (err) {
>               goto out;
>           }
> +        break;
> +#endif
> +    default:
> +        ;
>       }
>   
> +    opts.protocol = proto;
>       qmp_set_password(&opts, &err);
>   
>   out:
> @@ -1488,22 +1509,34 @@ 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");
> +#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
>       const char *display = qdict_get_try_str(qdict, "display");
> +#endif
>       Error *err = NULL;
> +    int proto;
>   
>       ExpirePasswordOptions opts = {
>           .time = (char *)whenstr,
>       };
>   
> -    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> -                                    DISPLAY_PROTOCOL_VNC, &err);
> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err);
>       if (err) {
>           goto out;
>       }
>   
> -    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
> +    switch (proto) {
> +#ifdef CONFIG_VNC
> +    case -1:
> +        proto = DISPLAY_PROTOCOL_VNC;
> +        /* fall through */
> +    case DISPLAY_PROTOCOL_VNC:
>           opts.u.vnc.has_display = !!display;
>           opts.u.vnc.display = (char *)display;
> +#else
> +    case -1:
> +        error_setg(&err, "FIXME");
> +        goto out;
> +#endif
>       }
>   
>       qmp_expire_password(&opts, &err);
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 4825d0cbea..69a9c2977a 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -167,18 +167,26 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
>   {
>       int rc = 0;
>   
> -    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
> +    switch (opts->protocol) {
> +#ifdef CONFIG_SPICE
> +    case DISPLAY_PROTOCOL_SPICE:
>           if (!qemu_using_spice(errp)) {
>               return;
>           }
>           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(opts->protocol == DISPLAY_PROTOCOL_VNC);
> +        break;
> +#endif
> +#ifdef CONFIG_VNC
> +    case DISPLAY_PROTOCOL_VNC:
>           /* Note that setting an empty password will not disable login through
>            * this interface. */
>           rc = vnc_display_password(opts->u.vnc.display, opts->password);
> +        break;
> +#endif
> +    default:
> +        abort();
>       }
>   
>       if (rc != 0) {
> @@ -202,14 +210,22 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
>           when = strtoull(whenstr, NULL, 10);
>       }
>   
> -    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
> +    switch (opts->protocol) {
> +#ifdef CONFIG_SPICE
> +    case DISPLAY_PROTOCOL_SPICE:
>           if (!qemu_using_spice(errp)) {
>               return;
>           }
>           rc = qemu_spice.set_pw_expire(when);
> -    } else {
> -        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
> +        break;
> +#endif
> +#ifdef CONFIG_VNC
> +    case DISPLAY_PROTOCOL_VNC:
>           rc = vnc_display_pw_expire(opts->u.vnc.display, when);
> +        break;
> +#endif
> +    default:
> +        abort();
>       }
>   
>       if (rc != 0) {
> 
> 
> 

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Markus Armbruster]

Fabian Ebner <f.ebner@proxmox.com> writes:

> Am 28.10.21 um 21:37 schrieb Markus Armbruster:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>>> Stefan Reiter <s.reiter@proxmox.com> writes:
>>>
>>>> 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.
>>>
>>> Queued, thanks!
>> 
>> Unqueued, because it fails to compile with --disable-vnc and with
>> --disable-spice.  I failed to catch this in review, sorry.
>>
>> Making it work takes a tiresome amount of #ifdeffery (sketch appended).
>> Missing: removal of stubs that are no longer used,
>> e.g. vnc_display_password() in ui/vnc-stubs.c.  Feels like more trouble
>> than it's worth.
>> 
>> To maximize our chances to get this into 6.2, please respin without the
>> 'if' conditionals.  Yes, this makes introspection less useful, but it's
>> no worse than before the patch.
>
> Unfortunately, Stefan is no longer working with Proxmox, so I would
> pick up these patches instead.

Appreciated!

> Since the 6.2 ship has long sailed, I suppose the way forward is using
> the #ifdefs then?

Maybe.

We can choose to improve introspection: keep the 'if' conditionals, and
fix the C code to compile with --disable-vnc and --disable-spice.

Or we can leave it imperfect as it was: drop the 'if' conditionals.

If we had a concrete need for better introspection here, the choice
would be easy.  But as far as I know, we don't.  Is better introspection
worth the additional work anyway?  Since you're volunteering to do the
work, you get to decide :)

> From my understanding what should be done is:
>
> * In the first patch, fix the typo spotted by Eric Blake and add the
>   R-b tags from this round.
>
> * Replace "since 6.2" with "since 7.0" everywhere.
>
> * Incorporate the #ifdef handling from below. I had to add another one
>   for the when/whenstr handling in qmp_expire_password to avoid an
>  error with -Werror=unused-but-set-variable.
>
> * Add #ifdefs for the unused stubs too? If yes, how to best find them?

For every call you put under #if, check whether there are are any
unconditional calls left, and if not, whether there is a stub function
for it.  If this is too terse, just ask for more help.

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Fabian Ebner]

Am 28.10.21 um 21:37 schrieb Markus Armbruster:
> Markus Armbruster <armbru@redhat.com> writes:
> 

----8<----

> 
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 5292617b44..39ca0b5ead 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -69,8 +69,10 @@
>     'base': { 'protocol': 'DisplayProtocol',
>               'password': 'str' },
>     'discriminator': 'protocol',
> -  'data': { 'vnc': 'SetPasswordOptionsVnc',
> -            'spice': 'SetPasswordOptionsSpice' } }
> +  'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
> +                     'if': 'CONFIG_VNC' },
> +            'spice': { 'type': 'SetPasswordOptionsSpice',
> +                       'if': 'CONFIG_SPICE' } } }
>   
>   ##
>   # @SetPasswordOptionsSpice:
> @@ -155,7 +157,8 @@
>     'base': { 'protocol': 'DisplayProtocol',
>               'time': 'str' },
>     'discriminator': 'protocol',
> -  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
> +  'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
> +                     'if': 'CONFIG_VNC' } } }
>   

So I decided to give the #ifdef approach a go, but if I configure with 
--disable-spice --disable-vnc, even with the above conditionals, it is 
still be possible to issue a set_password qmp command, which will then 
lead to an abort() in the generated code (and the patched 
qmp_set_password below would do the same if it could be reached):

Thread 1 (Thread 0x7f4a86701ec0 (LWP 40487) "qemu-system-x86"):
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f4a90d72537 in __GI_abort () at abort.c:79
#2  0x00005583ca03cef3 in visit_type_SetPasswordOptions_members 
(v=v@entry=0x5583cc6cccc0, obj=obj@entry=0x7ffe5bfc3ee0, 
errp=errp@entry=0x0) at qapi/qapi-visit-ui.c:71
#3  0x00005583ca5df72f in qmp_marshal_set_password (args=<optimized 
out>, ret=<optimized out>, errp=0x7f4a85d96ea0) at 
qapi/qapi-commands-ui.c:49
#4  0x00005583ca5e89e9 in do_qmp_dispatch_bh (opaque=0x7f4a85d96eb0) at 
../qapi/qmp-dispatch.c:129
#5  0x00005583ca605494 in aio_bh_call (bh=0x7f4a78009270) at 
../util/async.c:141

Is that expected? Should I add a conditional for {set,expire}_password 
in the schema too, and add an
#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
around the whole {hmp,qmp}_{set,expire}_password functions/declarations 
in C?

Or maybe that's a good indication that it's really not worth it ;)?

P.S. Thank you for the QAPI-related explanation in the other mail!

----8<----

> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 4825d0cbea..69a9c2977a 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -167,18 +167,26 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
>   {
>       int rc = 0;
>   
> -    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
> +    switch (opts->protocol) {
> +#ifdef CONFIG_SPICE
> +    case DISPLAY_PROTOCOL_SPICE:
>           if (!qemu_using_spice(errp)) {
>               return;
>           }
>           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(opts->protocol == DISPLAY_PROTOCOL_VNC);
> +        break;
> +#endif
> +#ifdef CONFIG_VNC
> +    case DISPLAY_PROTOCOL_VNC:
>           /* Note that setting an empty password will not disable login through
>            * this interface. */
>           rc = vnc_display_password(opts->u.vnc.display, opts->password);
> +        break;
> +#endif
> +    default:
> +        abort();
>       }
>   
>       if (rc != 0) {
> @@ -202,14 +210,22 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
>           when = strtoull(whenstr, NULL, 10);
>       }
>   
> -    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
> +    switch (opts->protocol) {
> +#ifdef CONFIG_SPICE
> +    case DISPLAY_PROTOCOL_SPICE:
>           if (!qemu_using_spice(errp)) {
>               return;
>           }
>           rc = qemu_spice.set_pw_expire(when);
> -    } else {
> -        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
> +        break;
> +#endif
> +#ifdef CONFIG_VNC
> +    case DISPLAY_PROTOCOL_VNC:
>           rc = vnc_display_pw_expire(opts->u.vnc.display, when);
> +        break;
> +#endif
> +    default:
> +        abort();
>       }
>   
>       if (rc != 0) {
> 
> 
> 

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Markus Armbruster]

Fabian Ebner <f.ebner@proxmox.com> writes:

> Am 28.10.21 um 21:37 schrieb Markus Armbruster:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>
> ----8<----
>
>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index 5292617b44..39ca0b5ead 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -69,8 +69,10 @@
>>     'base': { 'protocol': 'DisplayProtocol',
>>               'password': 'str' },
>>     'discriminator': 'protocol',
>> -  'data': { 'vnc': 'SetPasswordOptionsVnc',
>> -            'spice': 'SetPasswordOptionsSpice' } }
>> +  'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
>> +                     'if': 'CONFIG_VNC' },
>> +            'spice': { 'type': 'SetPasswordOptionsSpice',
>> +                       'if': 'CONFIG_SPICE' } } }
>>     ##
>>   # @SetPasswordOptionsSpice:
>> @@ -155,7 +157,8 @@
>>     'base': { 'protocol': 'DisplayProtocol',
>>               'time': 'str' },
>>     'discriminator': 'protocol',
>> -  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
>> +  'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
>> +                     'if': 'CONFIG_VNC' } } }
>>   
>
> So I decided to give the #ifdef approach a go, but if I configure with
> --disable-spice --disable-vnc, even with the above conditionals, it is 
> still be possible to issue a set_password qmp command, which will then
> lead to an abort() in the generated code (and the patched 
> qmp_set_password below would do the same if it could be reached):
>
> Thread 1 (Thread 0x7f4a86701ec0 (LWP 40487) "qemu-system-x86"):
> #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> #1  0x00007f4a90d72537 in __GI_abort () at abort.c:79
> #2  0x00005583ca03cef3 in visit_type_SetPasswordOptions_members
>  (v=v@entry=0x5583cc6cccc0, obj=obj@entry=0x7ffe5bfc3ee0, 
> errp=errp@entry=0x0) at qapi/qapi-visit-ui.c:71
> #3  0x00005583ca5df72f in qmp_marshal_set_password (args=<optimized
>  out>, ret=<optimized out>, errp=0x7f4a85d96ea0) at 
> qapi/qapi-commands-ui.c:49
> #4  0x00005583ca5e89e9 in do_qmp_dispatch_bh (opaque=0x7f4a85d96eb0)
>  at ../qapi/qmp-dispatch.c:129
> #5  0x00005583ca605494 in aio_bh_call (bh=0x7f4a78009270) at
>  ../util/async.c:141
>
> Is that expected? Should I add a conditional for {set,expire}_password
> in the schema too, and add an
> #if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
> around the whole {hmp,qmp}_{set,expire}_password
> functions/declarations in C?

I can have a closer look.  To make it easy, tell me how I can pull your
patches, or, if that's inconvenient for you, send them to me.

> Or maybe that's a good indication that it's really not worth it ;)?

Possibly.

> P.S. Thank you for the QAPI-related explanation in the other mail!

You're welcome!

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Fabian Ebner <f.ebner@proxmox.com> writes:
>
>> Am 28.10.21 um 21:37 schrieb Markus Armbruster:
>>> Markus Armbruster <armbru@redhat.com> writes:
>>> 
>>
>> ----8<----
>>
>>> diff --git a/qapi/ui.json b/qapi/ui.json
>>> index 5292617b44..39ca0b5ead 100644
>>> --- a/qapi/ui.json
>>> +++ b/qapi/ui.json
>>> @@ -69,8 +69,10 @@
>>>     'base': { 'protocol': 'DisplayProtocol',
>>>               'password': 'str' },
>>>     'discriminator': 'protocol',
>>> -  'data': { 'vnc': 'SetPasswordOptionsVnc',
>>> -            'spice': 'SetPasswordOptionsSpice' } }
>>> +  'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
>>> +                     'if': 'CONFIG_VNC' },
>>> +            'spice': { 'type': 'SetPasswordOptionsSpice',
>>> +                       'if': 'CONFIG_SPICE' } } }
>>>     ##
>>>   # @SetPasswordOptionsSpice:
>>> @@ -155,7 +157,8 @@
>>>     'base': { 'protocol': 'DisplayProtocol',
>>>               'time': 'str' },
>>>     'discriminator': 'protocol',
>>> -  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
>>> +  'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
>>> +                     'if': 'CONFIG_VNC' } } }
>>>   
>>
>> So I decided to give the #ifdef approach a go, but if I configure with
>> --disable-spice --disable-vnc, even with the above conditionals, it is 
>> still be possible to issue a set_password qmp command, which will then
>> lead to an abort() in the generated code (and the patched 
>> qmp_set_password below would do the same if it could be reached):
>>
>> Thread 1 (Thread 0x7f4a86701ec0 (LWP 40487) "qemu-system-x86"):
>> #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
>> #1  0x00007f4a90d72537 in __GI_abort () at abort.c:79
>> #2  0x00005583ca03cef3 in visit_type_SetPasswordOptions_members
>>  (v=v@entry=0x5583cc6cccc0, obj=obj@entry=0x7ffe5bfc3ee0, 
>> errp=errp@entry=0x0) at qapi/qapi-visit-ui.c:71
>> #3  0x00005583ca5df72f in qmp_marshal_set_password (args=<optimized
>>  out>, ret=<optimized out>, errp=0x7f4a85d96ea0) at 
>> qapi/qapi-commands-ui.c:49
>> #4  0x00005583ca5e89e9 in do_qmp_dispatch_bh (opaque=0x7f4a85d96eb0)
>>  at ../qapi/qmp-dispatch.c:129
>> #5  0x00005583ca605494 in aio_bh_call (bh=0x7f4a78009270) at
>>  ../util/async.c:141
>>
>> Is that expected? Should I add a conditional for {set,expire}_password
>> in the schema too, and add an
>> #if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
>> around the whole {hmp,qmp}_{set,expire}_password
>> functions/declarations in C?
>
> I can have a closer look.  To make it easy, tell me how I can pull your
> patches, or, if that's inconvenient for you, send them to me.

I got them by e-mail, thanks!

The dealloc visitor for unions (here: SetPasswordOptions) falls apart
when the tag enum (here: DisplayProtocol) is effectively empty.

The dealloc visitor's job is to recursively free a QAPI object.  Unlike
the other visitors, the dealloc visitor needs to work on
half-constructed objects where parts are still zero.  Easy, because
freeing null pointers does nothing.

Complication: for a union, the common visitor core still needs to decide
which branch to enter.  If we never constructed the union's tag value,
it's zero, and so is the branch corresponding to the zero tag value.
The visitor core happily goes down that branch, and the dealloc visitor
happily does nothing for it.  Not exactly the cleanest solution ever,
but it works.

*Except* when the tag enum is empty.  Then we run into the abort() here:

    bool visit_type_SetPasswordOptions_members(Visitor *v, SetPasswordOptions *obj, Error **errp)
    {
        if (!visit_type_q_obj_SetPasswordOptions_base_members(v, (q_obj_SetPasswordOptions_base *)obj, errp)) {
            return false;
        }
        switch (obj->protocol) {
    #if defined(CONFIG_VNC)
        case DISPLAY_PROTOCOL_VNC:
            return visit_type_SetPasswordOptionsVnc_members(v, &obj->u.vnc, errp);
    #endif /* defined(CONFIG_VNC) */
    #if defined(CONFIG_SPICE)
        case DISPLAY_PROTOCOL_SPICE:
            return visit_type_SetPasswordOptionsSpice_members(v, &obj->u.spice, errp);
    #endif /* defined(CONFIG_SPICE) */
        default:
            abort();
        }
        return true;
    }

This is actually why the QAPI generator rejects a union with an empty
tag enum (test case tests/qapi-schema/union-empty.json): it's a
restriction to protect the dealloc visitor.

The QAPI generator doesn't reject a union with a tag enum where all
members are conditional.  Hole in the restriction.

We can either plug the hole in the restriction, or lift the restriction.

>> Or maybe that's a good indication that it's really not worth it ;)?
>
> Possibly.

Let's drop the 'if' conditionals in the interest of decoupling this
series from the QAPI infrastructure fixes needed to make them work.

But first see my question about display-reload upthread.

>> P.S. Thank you for the QAPI-related explanation in the other mail!
>
> You're welcome!

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Markus Armbruster]

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

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

Related: Vladimir recently posted a patch to add a new command for
changing VNC server listening addresses.  Daniel asked him to work it
into display-reload instead[1].  Vladimir complied[2].

Daniel, what do you think about this one?  Should it also use
display-reload?


[1] Message-ID: <YdRJ06CS4qoDJI/F@redhat.com>
https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg00292.html

[2] Message-ID: <20220118160909.2502374-1-vsementsov@virtuozzo.com>
https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg03864.html

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Daniel P. Berrangé]

On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote:
> Stefan Reiter <s.reiter@proxmox.com> writes:
> 
> > 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.
> 
> Related: Vladimir recently posted a patch to add a new command for
> changing VNC server listening addresses.  Daniel asked him to work it
> into display-reload instead[1].  Vladimir complied[2].
> 
> Daniel, what do you think about this one?  Should it also use
> display-reload?

I'd ultimately intend to deprecate & remove the direct setting of
passwords on the CLI, and exclusively rely on the 'secret' object
for passing in passwords. With this in mind, I'd not be enthusiastic
about adding new commands for changing passwords in QMP directly,
rather I think we should have a way to change the 'secret' object
in use.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Fabian Ebner]

Am 25.01.22 um 16:06 schrieb Daniel P. Berrangé:
> On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote:
>> Stefan Reiter <s.reiter@proxmox.com> writes:
>>
>>> 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.
>>
>> Related: Vladimir recently posted a patch to add a new command for
>> changing VNC server listening addresses.  Daniel asked him to work it
>> into display-reload instead[1].  Vladimir complied[2].
>>
>> Daniel, what do you think about this one?  Should it also use
>> display-reload?
> 
> I'd ultimately intend to deprecate & remove the direct setting of
> passwords on the CLI, and exclusively rely on the 'secret' object
> for passing in passwords. With this in mind, I'd not be enthusiastic
> about adding new commands for changing passwords in QMP directly,
> rather I think we should have a way to change the 'secret' object
> in use.
> 

How should I proceed with this series then? Does adding the new argument
for the display ID count as "adding new commands"?

If what I should do is switching to only using secret objects, would the
plan be something like the following?

1. Add an option to display-reload for switching the display's
password-secret while adding SPICE as a valid display type.
2. Also include the set password action (i.e. disconnect/fail/keep) and
expiration time as part of that option.
3. Extend display-reload to also take an optional display ID for VNC.
4. Deprecate expire_password and set_password.

> Regards,
> Daniel

Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Daniel P. Berrangé]

On Mon, Jan 31, 2022 at 10:45:08AM +0100, Fabian Ebner wrote:
> Am 25.01.22 um 16:06 schrieb Daniel P. Berrangé:
> > On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote:
> >> Stefan Reiter <s.reiter@proxmox.com> writes:
> >>
> >>> 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.
> >>
> >> Related: Vladimir recently posted a patch to add a new command for
> >> changing VNC server listening addresses.  Daniel asked him to work it
> >> into display-reload instead[1].  Vladimir complied[2].
> >>
> >> Daniel, what do you think about this one?  Should it also use
> >> display-reload?
> > 
> > I'd ultimately intend to deprecate & remove the direct setting of
> > passwords on the CLI, and exclusively rely on the 'secret' object
> > for passing in passwords. With this in mind, I'd not be enthusiastic
> > about adding new commands for changing passwords in QMP directly,
> > rather I think we should have a way to change the 'secret' object
> > in use.
> > 
> 
> How should I proceed with this series then? Does adding the new argument
> for the display ID count as "adding new commands"?

Ok, so I've gone back and properly read the series. Since you're simply
extending existing commands with new arguments I've no objection to
carrying on with this approach.

We should still aim to have a general purpose command for live config
changes, but that shouldn't be considered a blocker for you series
here, as your series isn't making the existing situation worse.

> If what I should do is switching to only using secret objects, would the
> plan be something like the following?
> 
> 1. Add an option to display-reload for switching the display's
> password-secret while adding SPICE as a valid display type.
> 2. Also include the set password action (i.e. disconnect/fail/keep) and
> expiration time as part of that option.
> 3. Extend display-reload to also take an optional display ID for VNC.
> 4. Deprecate expire_password and set_password.

I've realized that we shouldn't overload display-reload for dual purposes.

We have two distinct usage scenarios that are meaningul

 - Re-loading the value of the existing secret
 - Changing which secret is used

The 'display-reload' command is reasonable for the first.

We should have a 'display-update' command for the second.

Either way, I don't think this series should be blocked on this since
it is merely modifying an existing command.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|