[PATCH v8 0/3] VNC-related HMP/QMP fixes

[PATCH v8 0/3] VNC-related HMP/QMP fixes

[Fabian Ebner]

Original cover letter by Stefan R.:

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.

v7 -> v8:
* drop last patch deprecating SetPasswordAction values besides 'keep'
  for VNC (unfortunately, I don't have enough time to try implementing
  'disconnect' and 'fail' for VNC in the near future)
* drop if conditionals for DisplayProtocol enum to make compilation
  with --disable-spice and/or --disable-vnc work
* order 'keep' first in enum, to fix how patch #3 uses it as an
  implicit default
* also set connected and has_connected for the VNC options in
  hmp_set_password
* fix typo in patch #1
* add missing '#' for description in patch #3

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 (3):
  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

 hmp-commands.hx            |  24 ++++---
 monitor/hmp-cmds.c         |  52 +++++++++++++-
 monitor/hmp.c              |  19 +++++-
 monitor/monitor-internal.h |   3 +-
 monitor/qmp-cmds.c         |  49 ++++----------
 qapi/ui.json               | 134 ++++++++++++++++++++++++++++++++-----
 6 files changed, 213 insertions(+), 68 deletions(-)

-- 
2.30.2

[PATCH v8 1/3] monitor/hmp: add support for flag argument with value

[Fabian Ebner]

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

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.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
[FE: fixed typo pointed out by Eric Blake]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 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 b20737e63c..fd4f5866c7 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -981,6 +981,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
             {
                 const char *tmp = p;
                 int skip_key = 0;
+                int ret;
                 /* option */
 
                 c = *typestr++;
@@ -1003,11 +1004,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 3da3f86c6a..a4cb307c8a 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 a 'V', it
+ *              specifies an optional string param (e.g. '-fV' allows '-f foo')
  *
  */
 
-- 
2.30.2

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

[Markus Armbruster]

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

> From: Stefan Reiter <s.reiter@proxmox.com>
>
> 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.
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> [FE: fixed typo pointed out by Eric Blake]
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  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 b20737e63c..fd4f5866c7 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -981,6 +981,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>              {
>                  const char *tmp = p;
>                  int skip_key = 0;
> +                int ret;
>                  /* option */
>  
>                  c = *typestr++;
> @@ -1003,11 +1004,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 3da3f86c6a..a4cb307c8a 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 a 'V', it
> + *              specifies an optional string param (e.g. '-fV' allows '-f foo')
>   *
>   */

For what it's worth, getopt() uses ':' after the option character for
"takes an argument".

Happy to make that tweak in my tree.  But see my review of PATCH 3
first.

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

[Fabian Ebner]

Am 09.02.22 um 15:12 schrieb Markus Armbruster:
> Fabian Ebner <f.ebner@proxmox.com> writes:

----8<----

>> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
>> index 3da3f86c6a..a4cb307c8a 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 a 'V', it
>> + *              specifies an optional string param (e.g. '-fV' allows '-f foo')
>>   *
>>   */
> 
> For what it's worth, getopt() uses ':' after the option character for
> "takes an argument".
> 

Doing that leads to e.g.
    .args_type  = "protocol:s,password:s,display:-d:,connected:s?",
so there's two different kinds of colons now. It's not a problem
functionality-wise AFAICT, but it might not be ideal. Should I still go
for it?

Also, wouldn't future non-string flag parameters need their own letter
too? What about re-using 's' here instead?

> Happy to make that tweak in my tree.  But see my review of PATCH 3
> first.
> 
> 

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

[Markus Armbruster]

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

> Am 09.02.22 um 15:12 schrieb Markus Armbruster:
>> Fabian Ebner <f.ebner@proxmox.com> writes:
>
> ----8<----
>
>>> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
>>> index 3da3f86c6a..a4cb307c8a 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 a 'V', it
>>> + *              specifies an optional string param (e.g. '-fV' allows '-f foo')
>>>   *
>>>   */
>> 
>> For what it's worth, getopt() uses ':' after the option character for
>> "takes an argument".
>> 
>
> Doing that leads to e.g.
>     .args_type  = "protocol:s,password:s,display:-d:,connected:s?",
> so there's two different kinds of colons now.

Point.

>                                               It's not a problem
> functionality-wise AFAICT, but it might not be ideal. Should I still go
> for it?
>
> Also, wouldn't future non-string flag parameters need their own letter
> too? What about re-using 's' here instead?

Another good point.

Dave, what do you think?

>> Happy to make that tweak in my tree.  But see my review of PATCH 3
>> first.

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

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Fabian Ebner <f.ebner@proxmox.com> writes:
> 
> > Am 09.02.22 um 15:12 schrieb Markus Armbruster:
> >> Fabian Ebner <f.ebner@proxmox.com> writes:
> >
> > ----8<----
> >
> >>> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> >>> index 3da3f86c6a..a4cb307c8a 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 a 'V', it
> >>> + *              specifies an optional string param (e.g. '-fV' allows '-f foo')
> >>>   *
> >>>   */
> >> 
> >> For what it's worth, getopt() uses ':' after the option character for
> >> "takes an argument".
> >> 
> >
> > Doing that leads to e.g.
> >     .args_type  = "protocol:s,password:s,display:-d:,connected:s?",
> > so there's two different kinds of colons now.
> 
> Point.

Yeh, : is probably a bad idea.

> >                                               It's not a problem
> > functionality-wise AFAICT, but it might not be ideal. Should I still go
> > for it?
> >
> > Also, wouldn't future non-string flag parameters need their own letter
> > too? What about re-using 's' here instead?
> 
> Another good point.
> 
> Dave, what do you think?

Yeh, I'd be happy reusing 's' here.

Dave

> >> Happy to make that tweak in my tree.  But see my review of PATCH 3
> >> first.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v8 2/3] qapi/monitor: refactor set/expire_password with enums

[Fabian Ebner]

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

'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>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
[FE: update "Since: " from 6.2 to 7.0
     put 'keep' first in enum to ease use as a default]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

v7 -> v8:
* drop if conditionals for DisplayProtocol enum, so compilation with
  --disable-{spice,vnc} works

 monitor/hmp-cmds.c | 29 +++++++++++++++++++++++++++--
 monitor/qmp-cmds.c | 37 ++++++++++++-------------------------
 qapi/ui.json       | 36 ++++++++++++++++++++++++++++++++++--
 3 files changed, 73 insertions(+), 29 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 8c384dc1b2..ff78741b75 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1398,8 +1398,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);
 }
 
@@ -1408,8 +1424,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;
+
+    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+                            DISPLAY_PROTOCOL_VNC, &err);
+    if (err) {
+        goto out;
+    }
 
-    qmp_expire_password(protocol, whenstr, &err);
+    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 db4d186448..b6e8b57fcc 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -168,33 +168,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");
@@ -203,10 +197,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) {
@@ -214,7 +204,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;
@@ -230,17 +220,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 9354f4c467..e112409211 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -9,6 +9,34 @@
 { 'include': 'common.json' }
 { 'include': 'sockets.json' }
 
+##
+# @DisplayProtocol:
+#
+# Display protocols which support changing password options.
+#
+# Since: 7.0
+#
+##
+{ 'enum': 'DisplayProtocol',
+  'data': [ 'vnc', 'spice' ] }
+
+##
+# @SetPasswordAction:
+#
+# An action to take on changing a password on a connection with active clients.
+#
+# @keep: maintain existing clients
+#
+# @fail: fail the command if clients are connected
+#
+# @disconnect: disconnect existing clients
+#
+# Since: 7.0
+#
+##
+{ 'enum': 'SetPasswordAction',
+  'data': [ 'keep', 'fail', 'disconnect' ] }
+
 ##
 # @set_password:
 #
@@ -38,7 +66,9 @@
 #
 ##
 { 'command': 'set_password',
-  'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
+  'data': { 'protocol': 'DisplayProtocol',
+            'password': 'str',
+            '*connected': 'SetPasswordAction' } }
 
 ##
 # @expire_password:
@@ -71,7 +101,9 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
+{ 'command': 'expire_password',
+  'data': { 'protocol': 'DisplayProtocol',
+            'time': 'str' } }
 
 ##
 # @screendump:
-- 
2.30.2

Re: [PATCH v8 2/3] qapi/monitor: refactor set/expire_password with enums

[Markus Armbruster]

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

> From: Stefan Reiter <s.reiter@proxmox.com>
>
> '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>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> [FE: update "Since: " from 6.2 to 7.0
>      put 'keep' first in enum to ease use as a default]
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>
> v7 -> v8:
> * drop if conditionals for DisplayProtocol enum, so compilation with
>   --disable-{spice,vnc} works
>
>  monitor/hmp-cmds.c | 29 +++++++++++++++++++++++++++--
>  monitor/qmp-cmds.c | 37 ++++++++++++-------------------------
>  qapi/ui.json       | 36 ++++++++++++++++++++++++++++++++++--
>  3 files changed, 73 insertions(+), 29 deletions(-)
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 8c384dc1b2..ff78741b75 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1398,8 +1398,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);
>  }
>  
> @@ -1408,8 +1424,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;
> +
> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> +                            DISPLAY_PROTOCOL_VNC, &err);
> +    if (err) {
> +        goto out;
> +    }
>  
> -    qmp_expire_password(protocol, whenstr, &err);
> +    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 db4d186448..b6e8b57fcc 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -168,33 +168,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");
> @@ -203,10 +197,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) {
> @@ -214,7 +204,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;
> @@ -230,17 +220,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 9354f4c467..e112409211 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -9,6 +9,34 @@
>  { 'include': 'common.json' }
>  { 'include': 'sockets.json' }
>  
> +##
> +# @DisplayProtocol:
> +#
> +# Display protocols which support changing password options.

This is correct now: the enum is only used in that role.  If we ever use
DisplayReloadType for other purposes, the comment will become wrong.
Let's not worry about that now.

> +#
> +# Since: 7.0
> +#
> +##
> +{ 'enum': 'DisplayProtocol',
> +  'data': [ 'vnc', 'spice' ] }
> +
> +##
> +# @SetPasswordAction:
> +#
> +# An action to take on changing a password on a connection with active clients.
> +#
> +# @keep: maintain existing clients
> +#
> +# @fail: fail the command if clients are connected
> +#
> +# @disconnect: disconnect existing clients
> +#
> +# Since: 7.0
> +#
> +##
> +{ 'enum': 'SetPasswordAction',
> +  'data': [ 'keep', 'fail', 'disconnect' ] }
> +
>  ##
>  # @set_password:
>  #
> @@ -38,7 +66,9 @@
>  #
>  ##
>  { 'command': 'set_password',
> -  'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
> +  'data': { 'protocol': 'DisplayProtocol',
> +            'password': 'str',
> +            '*connected': 'SetPasswordAction' } }
>  
>  ##
>  # @expire_password:
> @@ -71,7 +101,9 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
> +{ 'command': 'expire_password',
> +  'data': { 'protocol': 'DisplayProtocol',
> +            'time': 'str' } }
>  
>  ##
>  # @screendump:

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

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

[Fabian Ebner]

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

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>
[FE: update "Since: " from 6.2 to 7.0
     set {has_}connected for VNC in hmp_set_password]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

v7 -> v8:
* add missing # in the description for @ExpirePasswordOptions
* other changes are already mentioned above

 hmp-commands.hx    |  24 +++++-----
 monitor/hmp-cmds.c |  39 ++++++++++++----
 monitor/qmp-cmds.c |  34 ++++++--------
 qapi/ui.json       | 110 ++++++++++++++++++++++++++++++++++++---------
 4 files changed, 145 insertions(+), 62 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 70a9136ac2..cc2f4bdeba 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 ff78741b75..be0d919b64 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1396,13 +1396,17 @@ 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;
     }
@@ -1413,7 +1417,17 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
         goto out;
     }
 
-    qmp_set_password(proto, password, !!connected, conn, &err);
+    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+        opts.u.vnc.has_display = !!display;
+        opts.u.vnc.display = (char *)display;
+        opts.u.vnc.has_connected = !!connected;
+        opts.u.vnc.connected = conn;
+    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
+        opts.u.spice.has_connected = !!connected;
+        opts.u.spice.connected = conn;
+    }
+
+    qmp_set_password(&opts, &err);
 
 out:
     hmp_handle_error(mon, err);
@@ -1423,16 +1437,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 b6e8b57fcc..37db941fd3 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -168,35 +168,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;
 
-    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) {
@@ -204,11 +196,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;
@@ -220,14 +212,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 e112409211..089f05c702 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -38,20 +38,61 @@
   'data': [ 'keep', 'fail', 'disconnect' ] }
 
 ##
-# @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: 7.0
+#
+##
+{ '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: 7.0
+#
+##
+{ '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: 7.0
+#
+##
+{ '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
@@ -65,17 +106,15 @@
 # <- { "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.
+# General options for expire_password.
 #
-# @protocol: the name of the remote display protocol 'vnc' or 'spice'
+# @protocol: - 'vnc' to modify the VNC server expiration
+#            - 'spice' to modify the Spice server expiration
 #
 # @time: when to expire the password.
 #
@@ -84,16 +123,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: 7.0
+#
+##
+{ '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: 7.0
+#
+##
+
+{ '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",
@@ -101,9 +169,7 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'expire_password',
-  'data': { 'protocol': 'DisplayProtocol',
-            'time': 'str' } }
+{ 'command': 'expire_password', 'boxed': true, 'data': 'ExpirePasswordOptions' }
 
 ##
 # @screendump:
-- 
2.30.2

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

[Markus Armbruster]

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

> From: Stefan Reiter <s.reiter@proxmox.com>
>
> 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>

Did I suggest this feature?  I don't remember...  Most likely, I merely
suggested using a union.  Mind if I drop this tag?

> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> [FE: update "Since: " from 6.2 to 7.0
>      set {has_}connected for VNC in hmp_set_password]
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>
> v7 -> v8:
> * add missing # in the description for @ExpirePasswordOptions
> * other changes are already mentioned above
>
>  hmp-commands.hx    |  24 +++++-----
>  monitor/hmp-cmds.c |  39 ++++++++++++----
>  monitor/qmp-cmds.c |  34 ++++++--------
>  qapi/ui.json       | 110 ++++++++++++++++++++++++++++++++++++---------
>  4 files changed, 145 insertions(+), 62 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 70a9136ac2..cc2f4bdeba 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 ]``

This is the first flag with an argument in HMP.  The alternative is
another optional argument.

PRO optional argument: no need for PATCH 1.

PRO flag with argument: can specify the display without
action-if-connected.

Dave, this is your call to make.

> +  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 ff78741b75..be0d919b64 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1396,13 +1396,17 @@ 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;
>      }
> @@ -1413,7 +1417,17 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
>          goto out;
>      }
>  
> -    qmp_set_password(proto, password, !!connected, conn, &err);
> +    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
> +        opts.u.vnc.has_display = !!display;
> +        opts.u.vnc.display = (char *)display;
> +        opts.u.vnc.has_connected = !!connected;
> +        opts.u.vnc.connected = conn;
> +    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
> +        opts.u.spice.has_connected = !!connected;
> +        opts.u.spice.connected = conn;
> +    }
> +
> +    qmp_set_password(&opts, &err);
>  
>  out:
>      hmp_handle_error(mon, err);
> @@ -1423,16 +1437,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 b6e8b57fcc..37db941fd3 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -168,35 +168,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;
>  
> -    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) {
> @@ -204,11 +196,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;
> @@ -220,14 +212,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 e112409211..089f05c702 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -38,20 +38,61 @@
>    'data': [ 'keep', 'fail', 'disconnect' ] }
>  
>  ##
> -# @set_password:
> +# @SetPasswordOptions:
>  #
> -# Sets the password of a remote display session.
> +# General options for set_password.

Actually, all the options there are.  Let's drop "General".

>  #
>  # @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: 7.0
> +#
> +##
> +{ '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: 7.0
> +#
> +##
> +{ '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.

Neglects to document the default, unlike SetPasswordOptionsSpice above.

> +#
> +# Since: 7.0
> +#
> +##
> +{ 'struct': 'SetPasswordOptionsVnc',
> +  'data': { '*display': 'str',
> +            '*connected': 'SetPasswordAction' }}

@connected could be made a common member.  Untested incremental patch
appended for your consideration.

> +
> +##
> +# @set_password:
> +#
> +# Set the password of a remote display server.
>  #
>  # Returns: - Nothing on success
>  #          - If Spice is not enabled, DeviceNotFound
> @@ -65,17 +106,15 @@
>  # <- { "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.
> +# General options for expire_password.
>  #
> -# @protocol: the name of the remote display protocol 'vnc' or 'spice'
> +# @protocol: - 'vnc' to modify the VNC server expiration
> +#            - 'spice' to modify the Spice server expiration
>  #
>  # @time: when to expire the password.
>  #
> @@ -84,16 +123,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: 7.0
> +#
> +##
> +{ '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: 7.0
> +#
> +##
> +
> +{ '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",
> @@ -101,9 +169,7 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'expire_password',
> -  'data': { 'protocol': 'DisplayProtocol',
> -            'time': 'str' } }
> +{ 'command': 'expire_password', 'boxed': true, 'data': 'ExpirePasswordOptions' }
>  
>  ##
>  # @screendump:

diff --git a/qapi/ui.json b/qapi/ui.json
index 089f05c702..bcc69896ed 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -47,29 +47,18 @@
 #
 # @password: the new password
 #
+# @connected: How to handle existing clients when changing the
+#             password.
+#
 # Since: 7.0
 #
 ##
 { 'union': 'SetPasswordOptions',
   'base': { 'protocol': 'DisplayProtocol',
-            'password': 'str' },
+            'password': 'str',
+            '*connected': 'SetPasswordAction' },
   '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: 7.0
-#
-##
-{ 'struct': 'SetPasswordOptionsSpice',
-  'data': { '*connected': 'SetPasswordAction' } }
+  'data': { 'vnc': 'SetPasswordOptionsVnc' } }
 
 ##
 # @SetPasswordOptionsVnc:
@@ -79,15 +68,11 @@
 # @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: 7.0
 #
 ##
 { 'struct': 'SetPasswordOptionsVnc',
-  'data': { '*display': 'str',
-            '*connected': 'SetPasswordAction' }}
+  'data': { '*display': 'str' } }
 
 ##
 # @set_password:
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index be0d919b64..54762a9396 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1401,8 +1401,16 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
     SetPasswordAction conn;
 
+    conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
+                           SET_PASSWORD_ACTION_KEEP, &err);
+    if (err) {
+        goto out;
+    }
+
     SetPasswordOptions opts = {
         .password = (char *)password,
+        .has_connected = !!connected,
+        .connected = conn,
     };
 
     opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
@@ -1411,20 +1419,9 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
         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;
-        opts.u.vnc.has_connected = !!connected;
-        opts.u.vnc.connected = conn;
-    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
-        opts.u.spice.has_connected = !!connected;
-        opts.u.spice.connected = conn;
     }
 
     qmp_set_password(&opts, &err);
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 37db941fd3..df97582dd4 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -177,11 +177,11 @@ void qmp_set_password(SetPasswordOptions *opts, Error **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);
+                opts->connected == SET_PASSWORD_ACTION_FAIL,
+                opts->connected == SET_PASSWORD_ACTION_DISCONNECT);
     } else {
         assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
-        if (opts->u.vnc.connected != SET_PASSWORD_ACTION_KEEP) {
+        if (opts->connected != SET_PASSWORD_ACTION_KEEP) {
             /* vnc supports "connected=keep" only */
             error_setg(errp, QERR_INVALID_PARAMETER, "connected");
             return;

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

[Fabian Ebner]

Am 09.02.22 um 15:07 schrieb Markus Armbruster:
> Fabian Ebner <f.ebner@proxmox.com> writes:
> 
>> From: Stefan Reiter <s.reiter@proxmox.com>
>>
>> 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>
> 
> Did I suggest this feature?  I don't remember...  Most likely, I merely
> suggested using a union.  Mind if I drop this tag?
> 

Yes, Stefan might've put the tag because of the suggested approach. I'll
drop it.

>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> [FE: update "Since: " from 6.2 to 7.0
>>      set {has_}connected for VNC in hmp_set_password]
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> v7 -> v8:
>> * add missing # in the description for @ExpirePasswordOptions
>> * other changes are already mentioned above
>>
>>  hmp-commands.hx    |  24 +++++-----
>>  monitor/hmp-cmds.c |  39 ++++++++++++----
>>  monitor/qmp-cmds.c |  34 ++++++--------
>>  qapi/ui.json       | 110 ++++++++++++++++++++++++++++++++++++---------
>>  4 files changed, 145 insertions(+), 62 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 70a9136ac2..cc2f4bdeba 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 ]``
> 
> This is the first flag with an argument in HMP.  The alternative is
> another optional argument.
> 
> PRO optional argument: no need for PATCH 1.
> 
> PRO flag with argument: can specify the display without
> action-if-connected.
> 
> Dave, this is your call to make.
> 

I'll go ahead with v9 once the decision is made.

----8<----

>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index e112409211..089f05c702 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -38,20 +38,61 @@
>>    'data': [ 'keep', 'fail', 'disconnect' ] }
>>  
>>  ##
>> -# @set_password:
>> +# @SetPasswordOptions:
>>  #
>> -# Sets the password of a remote display session.
>> +# General options for set_password.
> 
> Actually, all the options there are.  Let's drop "General".
> 

Ok.

>>  #
>>  # @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: 7.0
>> +#
>> +##
>> +{ '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: 7.0
>> +#
>> +##
>> +{ '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.
> 
> Neglects to document the default, unlike SetPasswordOptionsSpice above.
> 

Will add it in v9.

>> +#
>> +# Since: 7.0
>> +#
>> +##
>> +{ 'struct': 'SetPasswordOptionsVnc',
>> +  'data': { '*display': 'str',
>> +            '*connected': 'SetPasswordAction' }}
> 
> @connected could be made a common member.  Untested incremental patch
> appended for your consideration.
> 

Yes, sounds good.

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

[Markus Armbruster]

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

> Am 09.02.22 um 15:07 schrieb Markus Armbruster:
>> Fabian Ebner <f.ebner@proxmox.com> writes:
>> 
>>> From: Stefan Reiter <s.reiter@proxmox.com>
>>>
>>> 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.

[...]

>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>>> index 70a9136ac2..cc2f4bdeba 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 ]``
>> 
>> This is the first flag with an argument in HMP.  The alternative is
>> another optional argument.
>> 
>> PRO optional argument: no need for PATCH 1.
>> 
>> PRO flag with argument: can specify the display without
>> action-if-connected.
>> 
>> Dave, this is your call to make.
>> 
>
> I'll go ahead with v9 once the decision is made.

Dave?

[...]

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

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Fabian Ebner <f.ebner@proxmox.com> writes:
> 
> > Am 09.02.22 um 15:07 schrieb Markus Armbruster:
> >> Fabian Ebner <f.ebner@proxmox.com> writes:
> >> 
> >>> From: Stefan Reiter <s.reiter@proxmox.com>
> >>>
> >>> 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.
> 
> [...]
> 
> >>> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >>> index 70a9136ac2..cc2f4bdeba 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 ]``
> >> 
> >> This is the first flag with an argument in HMP.  The alternative is
> >> another optional argument.
> >> 
> >> PRO optional argument: no need for PATCH 1.
> >> 
> >> PRO flag with argument: can specify the display without
> >> action-if-connected.
> >> 
> >> Dave, this is your call to make.
> >> 
> >
> > I'll go ahead with v9 once the decision is made.
> 
> Dave?

I think the flag with argument is clearer; HMP has a problem of
having a lot of optional arguments that get very order dependent which
is messy; so I'd go with the flag with argument.

Dave

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

Re: [PATCH v8 0/3] VNC-related HMP/QMP fixes

[Dr. David Alan Gilbert]

* Fabian Ebner (f.ebner@proxmox.com) wrote:
> Original cover letter by Stefan R.:
> 
> 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

> v7 -> v8:
> * drop last patch deprecating SetPasswordAction values besides 'keep'
>   for VNC (unfortunately, I don't have enough time to try implementing
>   'disconnect' and 'fail' for VNC in the near future)
> * drop if conditionals for DisplayProtocol enum to make compilation
>   with --disable-spice and/or --disable-vnc work
> * order 'keep' first in enum, to fix how patch #3 uses it as an
>   implicit default
> * also set connected and has_connected for the VNC options in
>   hmp_set_password
> * fix typo in patch #1
> * add missing '#' for description in patch #3
> 
> 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 (3):
>   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
> 
>  hmp-commands.hx            |  24 ++++---
>  monitor/hmp-cmds.c         |  52 +++++++++++++-
>  monitor/hmp.c              |  19 +++++-
>  monitor/monitor-internal.h |   3 +-
>  monitor/qmp-cmds.c         |  49 ++++----------
>  qapi/ui.json               | 134 ++++++++++++++++++++++++++++++++-----
>  6 files changed, 213 insertions(+), 68 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK