* [PATCH v3 0/3] VNC-related HMP/QMP fixes
@ 2021-09-20 10:56 Stefan Reiter
2021-09-20 10:56 ` [PATCH v3 1/3] monitor/hmp: correctly invert password argument detection again Stefan Reiter
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Stefan Reiter @ 2021-09-20 10:56 UTC (permalink / raw)
To: Marc-André Lureau, Marc-André Lureau,
Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini,
Eric Blake, Gerd Hoffmann, Wolfgang Bumiller, Thomas Lamprecht
Cc: qemu-devel
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.
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: correctly invert password argument detection again
monitor/hmp: add support for flag argument with value
monitor: refactor set/expire_password and allow VNC display id
hmp-commands.hx | 29 ++++----
monitor/hmp-cmds.c | 62 ++++++++++++++++-
monitor/hmp.c | 17 ++++-
monitor/qmp-cmds.c | 62 ++++++-----------
qapi/ui.json | 168 +++++++++++++++++++++++++++++++++++++--------
5 files changed, 252 insertions(+), 86 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/3] monitor/hmp: correctly invert password argument detection again
2021-09-20 10:56 [PATCH v3 0/3] VNC-related HMP/QMP fixes Stefan Reiter
@ 2021-09-20 10:56 ` Stefan Reiter
2021-09-20 10:56 ` [PATCH v3 2/3] monitor/hmp: add support for flag argument with value Stefan Reiter
2021-09-20 10:56 ` [PATCH v3 3/3] monitor: refactor set/expire_password and allow VNC display id Stefan Reiter
2 siblings, 0 replies; 6+ messages in thread
From: Stefan Reiter @ 2021-09-20 10:56 UTC (permalink / raw)
To: Marc-André Lureau, Marc-André Lureau,
Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini,
Eric Blake, Gerd Hoffmann, Wolfgang Bumiller, Thomas Lamprecht
Cc: qemu-devel
Commit cfb5387a1d 'hmp: remove "change vnc TARGET" command' claims to
remove the HMP "change vnc" command, but doesn't actually do that.
Instead it rewires it to use 'qmp_change_vnc_password', and in the
process inverts the argument detection - ignoring the first issue, this
inversion is wrong, as this will now ask the user for a password if one
is already provided, and simply fail if none is given.
Fixes: cfb5387a1d ("hmp: remove "change vnc TARGET" command")
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
monitor/hmp-cmds.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index e00255f7ee..a7e197a90b 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1496,7 +1496,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
}
if (strcmp(target, "passwd") == 0 ||
strcmp(target, "password") == 0) {
- if (arg) {
+ if (!arg) {
MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
return;
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/3] monitor/hmp: add support for flag argument with value
2021-09-20 10:56 [PATCH v3 0/3] VNC-related HMP/QMP fixes Stefan Reiter
2021-09-20 10:56 ` [PATCH v3 1/3] monitor/hmp: correctly invert password argument detection again Stefan Reiter
@ 2021-09-20 10:56 ` Stefan Reiter
2021-09-20 21:27 ` Eric Blake
2021-09-20 10:56 ` [PATCH v3 3/3] monitor: refactor set/expire_password and allow VNC display id Stefan Reiter
2 siblings, 1 reply; 6+ messages in thread
From: Stefan Reiter @ 2021-09-20 10:56 UTC (permalink / raw)
To: Marc-André Lureau, Marc-André Lureau,
Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini,
Eric Blake, Gerd Hoffmann, Wolfgang Bumiller, Thomas Lamprecht
Cc: qemu-devel
Adds support for the "-xS" parameter type, where "-x" denotes a flag
name and the "S" suffix indicates that this flag is supposed to take an
arbitrary string parameter.
These parameters are always optional, the entry in the qdict will be
omitted if the flag is not given.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
monitor/hmp.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/monitor/hmp.c b/monitor/hmp.c
index d50c3124e1..a32dce7a35 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -980,6 +980,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
{
const char *tmp = p;
int skip_key = 0;
+ int ret;
/* option */
c = *typestr++;
@@ -1002,8 +1003,22 @@ static QDict *monitor_parse_arguments(Monitor *mon,
}
if (skip_key) {
p = tmp;
+ } else if (*typestr == 'S') {
+ /* has option with string value */
+ typestr++;
+ tmp = p++;
+ while (qemu_isspace(*p)) {
+ p++;
+ }
+ ret = get_str(buf, sizeof(buf), &p);
+ if (ret < 0) {
+ monitor_printf(mon, "%s: value expected for -%c\n",
+ cmd->name, *tmp);
+ goto fail;
+ }
+ qdict_put_str(qdict, key, buf);
} else {
- /* has option */
+ /* has boolean option */
p++;
qdict_put_bool(qdict, key, true);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 3/3] monitor: refactor set/expire_password and allow VNC display id
2021-09-20 10:56 [PATCH v3 0/3] VNC-related HMP/QMP fixes Stefan Reiter
2021-09-20 10:56 ` [PATCH v3 1/3] monitor/hmp: correctly invert password argument detection again Stefan Reiter
2021-09-20 10:56 ` [PATCH v3 2/3] monitor/hmp: add support for flag argument with value Stefan Reiter
@ 2021-09-20 10:56 ` Stefan Reiter
2021-09-20 21:37 ` Eric Blake
2 siblings, 1 reply; 6+ messages in thread
From: Stefan Reiter @ 2021-09-20 10:56 UTC (permalink / raw)
To: Marc-André Lureau, Marc-André Lureau,
Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini,
Eric Blake, Gerd Hoffmann, Wolfgang Bumiller, Thomas Lamprecht
Cc: qemu-devel
It is possible to specify more than one VNC server on the command line,
either with an explicit ID or the auto-generated ones à la "default",
"vnc2", "vnc3", ...
It is not possible to change the password on one of these extra VNC
displays though. Fix this by adding a "display" parameter to the
"set_password" and "expire_password" QMP and HMP commands.
For HMP, the display is specified using the "-d" value flag.
For QMP, the schema is updated to explicitly express the supported
variants of the commands with protocol-discriminated unions.
Suggested-by: Eric Blake <eblake@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
The union schema simplifies the QMP code, but makes the HMP part a bit more
involved. Since the parameters are practically the same, is there a way to just
pass the HMP generated qdict off to the qapi parser to get the correct struct
for calling the qmp_ methods?
hmp-commands.hx | 29 ++++----
monitor/hmp-cmds.c | 60 +++++++++++++++-
monitor/qmp-cmds.c | 62 ++++++-----------
qapi/ui.json | 168 +++++++++++++++++++++++++++++++++++++--------
4 files changed, 235 insertions(+), 84 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8e45bce2cd..d78e4cfc47 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1514,34 +1514,35 @@ ERST
{
.name = "set_password",
- .args_type = "protocol:s,password:s,connected:s?",
- .params = "protocol password action-if-connected",
+ .args_type = "protocol:s,password:s,display:-dS,connected:s?",
+ .params = "protocol password [-d display] [action-if-connected]",
.help = "set spice/vnc password",
.cmd = hmp_set_password,
},
SRST
-``set_password [ vnc | spice ] password [ action-if-connected ]``
- Change spice/vnc password. Use zero to make the password stay valid
- forever. *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.
+``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected ]``
+ Change spice/vnc password. *display* can be used with 'vnc' to specify
+ which display to set the password on. *action-if-connected* specifies
+ what should happen in case a connection is established: *fail* makes
+ the password change fail. *disconnect* changes the password and
+ disconnects the client. *keep* changes the password and keeps the
+ connection up. *keep* is the default.
ERST
{
.name = "expire_password",
- .args_type = "protocol:s,time:s",
- .params = "protocol time",
+ .args_type = "protocol:s,time:s,display:-dS",
+ .params = "protocol time [-d display]",
.help = "set spice/vnc password expire-time",
.cmd = hmp_expire_password,
},
SRST
-``expire_password [ vnc | spice ]`` *expire-time*
- Specify when a password for spice/vnc becomes
- invalid. *expire-time* accepts:
+``expire_password [ vnc | spice ] expire-time [ -d display ]``
+ Specify when a password for spice/vnc becomes invalid.
+ *display* behaves the same as in ``set_password``.
+ *expire-time* accepts:
``now``
Invalidate password instantly.
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a7e197a90b..1a49c1c0d9 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,10 +1451,43 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *password = qdict_get_str(qdict, "password");
+ const char *display = qdict_get_try_str(qdict, "display");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
+ DisplayProtocol proto;
- qmp_set_password(protocol, password, !!connected, connected, &err);
+ SetPasswordOptions opts = {
+ .password = g_strdup(password),
+ .u.vnc.display = NULL,
+ };
+
+ proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+ DISPLAY_PROTOCOL_VNC, &err);
+ if (err) {
+ hmp_handle_error(mon, err);
+ return;
+ }
+ opts.protocol = proto;
+
+ if (proto == DISPLAY_PROTOCOL_VNC) {
+ if ((opts.u.vnc.has_display = !!display)) {
+ opts.u.vnc.display = g_strdup(display);
+ }
+ } else if (proto == DISPLAY_PROTOCOL_SPICE) {
+ if ((opts.u.spice.has_connected = !!connected)) {
+ opts.u.spice.connected =
+ qapi_enum_parse(&SetPasswordAction_lookup, connected,
+ SET_PASSWORD_ACTION_KEEP, &err);
+ if (err) {
+ hmp_handle_error(mon, err);
+ return;
+ }
+ }
+ }
+
+ qmp_set_password(&opts, &err);
+ g_free(opts.password);
+ g_free(opts.u.vnc.display);
hmp_handle_error(mon, err);
}
@@ -1462,9 +1495,32 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *whenstr = qdict_get_str(qdict, "time");
+ const char *display = qdict_get_try_str(qdict, "display");
Error *err = NULL;
+ DisplayProtocol proto;
- qmp_expire_password(protocol, whenstr, &err);
+ ExpirePasswordOptions opts = {
+ .time = g_strdup(whenstr),
+ .u.vnc.display = NULL,
+ };
+
+ proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+ DISPLAY_PROTOCOL_VNC, &err);
+ if (err) {
+ hmp_handle_error(mon, err);
+ return;
+ }
+ opts.protocol = proto;
+
+ if (proto == DISPLAY_PROTOCOL_VNC) {
+ if ((opts.u.vnc.has_display = !!display)) {
+ opts.u.vnc.display = g_strdup(display);
+ }
+ }
+
+ qmp_expire_password(&opts, &err);
+ g_free(opts.time);
+ g_free(opts.u.vnc.display);
hmp_handle_error(mon, err);
}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5c0d5e116b..cb229c01f8 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -163,45 +163,30 @@ void qmp_system_wakeup(Error **errp)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
}
-void qmp_set_password(const char *protocol, const char *password,
- bool has_connected, const char *connected, Error **errp)
+void qmp_set_password(SetPasswordOptions *opts, Error **errp)
{
- int disconnect_if_connected = 0;
- int fail_if_connected = 0;
- int rc;
+ bool disconnect_if_connected = false;
+ bool fail_if_connected = false;
+ int rc = 0;
- if (has_connected) {
- if (strcmp(connected, "fail") == 0) {
- fail_if_connected = 1;
- } else if (strcmp(connected, "disconnect") == 0) {
- disconnect_if_connected = 1;
- } else if (strcmp(connected, "keep") == 0) {
- /* nothing */
- } else {
- error_setg(errp, QERR_INVALID_PARAMETER, "connected");
- return;
- }
- }
-
- if (strcmp(protocol, "spice") == 0) {
+ if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
if (!qemu_using_spice(errp)) {
return;
}
- rc = qemu_spice.set_passwd(password, fail_if_connected,
- disconnect_if_connected);
- } else if (strcmp(protocol, "vnc") == 0) {
- if (fail_if_connected || disconnect_if_connected) {
- /* vnc supports "connected=keep" only */
- error_setg(errp, QERR_INVALID_PARAMETER, "connected");
- return;
+ if (opts->u.spice.has_connected) {
+ fail_if_connected =
+ opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL;
+ disconnect_if_connected =
+ opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT;
}
+ rc = qemu_spice.set_passwd(opts->password, fail_if_connected,
+ disconnect_if_connected);
+ } else if (opts->protocol == DISPLAY_PROTOCOL_VNC) {
/* Note that setting an empty password will not disable login through
* this interface. */
- rc = vnc_display_password(NULL, password);
- } else {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
- "'vnc' or 'spice'");
- return;
+ rc = vnc_display_password(
+ opts->u.vnc.has_display ? opts->u.vnc.display : NULL,
+ opts->password);
}
if (rc != 0) {
@@ -209,11 +194,11 @@ void qmp_set_password(const char *protocol, const char *password,
}
}
-void qmp_expire_password(const char *protocol, const char *whenstr,
- Error **errp)
+void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
{
time_t when;
int rc;
+ const char* whenstr = opts->time;
if (strcmp(whenstr, "now") == 0) {
when = 0;
@@ -225,17 +210,14 @@ void qmp_expire_password(const char *protocol, const char *whenstr,
when = strtoull(whenstr, NULL, 10);
}
- if (strcmp(protocol, "spice") == 0) {
+ if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_pw_expire(when);
- } else if (strcmp(protocol, "vnc") == 0) {
- rc = vnc_display_pw_expire(NULL, when);
- } else {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
- "'vnc' or 'spice'");
- return;
+ } else if (opts->protocol == DISPLAY_PROTOCOL_VNC) {
+ rc = vnc_display_pw_expire(
+ opts->u.vnc.has_display ? opts->u.vnc.display : NULL, when);
}
if (rc != 0) {
diff --git a/qapi/ui.json b/qapi/ui.json
index b2cf7a6759..494c92a65e 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -9,22 +9,23 @@
{ 'include': 'common.json' }
{ 'include': 'sockets.json' }
+##
+# @DisplayProtocol:
+#
+# Display protocols which support changing password options.
+#
+# Since: 6.2
+#
+##
+{ 'enum': 'DisplayProtocol',
+ 'data': [ { 'name': 'vnc', 'if': 'CONFIG_VNC' },
+ { 'name': 'spice', 'if': 'CONFIG_SPICE' } ] }
+
##
# @set_password:
#
# Sets the password of a remote display session.
#
-# @protocol: - 'vnc' to modify the VNC server password
-# - 'spice' to modify the Spice server password
-#
-# @password: the new password
-#
-# @connected: how to handle existing clients when changing the
-# password. If nothing is specified, defaults to 'keep'
-# 'fail' to fail the command if clients are connected
-# 'disconnect' to disconnect existing clients
-# 'keep' to maintain existing clients
-#
# Returns: - Nothing on success
# - If Spice is not enabled, DeviceNotFound
#
@@ -37,33 +38,101 @@
# <- { "return": {} }
#
##
-{ 'command': 'set_password',
- 'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
+{ 'command': 'set_password', 'boxed': true, 'data': 'SetPasswordOptions' }
+
+##
+# @SetPasswordOptions:
+#
+# Data required to set a new password on a display server protocol.
+#
+# @protocol: - 'vnc' to modify the VNC server password
+# - 'spice' to modify the Spice server password
+#
+# @password: the new password
+#
+# Since: 6.2
+#
+##
+{ 'union': 'SetPasswordOptions',
+ 'base': { 'protocol': 'DisplayProtocol',
+ 'password': 'str' },
+ 'discriminator': 'protocol',
+ 'data': { 'vnc': 'SetPasswordOptionsVnc',
+ 'spice': 'SetPasswordOptionsSpice' } }
+
+##
+# @SetPasswordAction:
+#
+# An action to take on changing a password on a connection with active clients.
+#
+# @fail: fail the command if clients are connected
+#
+# @disconnect: disconnect existing clients
+#
+# @keep: maintain existing clients
+#
+# Since: 6.2
+#
+##
+{ 'enum': 'SetPasswordAction',
+ 'data': [ 'fail', 'disconnect', 'keep' ] }
+
+##
+# @SetPasswordActionVnc:
+#
+# See @SetPasswordAction. VNC only supports the keep action. 'connection'
+# should just be omitted for VNC, this is kept for backwards compatibility.
+#
+# @keep: maintain existing clients
+#
+# Since: 6.2
+#
+##
+{ 'enum': 'SetPasswordActionVnc',
+ 'data': [ 'keep' ] }
+
+##
+# @SetPasswordOptionsSpice:
+#
+# Options for set_password specific to the VNC procotol.
+#
+# @connected: How to handle existing clients when changing the
+# password. If nothing is specified, defaults to 'keep'.
+#
+# Since: 6.2
+#
+##
+{ 'struct': 'SetPasswordOptionsSpice',
+ 'data': { '*connected': 'SetPasswordAction' } }
+
+##
+# @SetPasswordOptionsVnc:
+#
+# Options for set_password specific to the VNC procotol.
+#
+# @display: The id of the display where the password should be changed.
+# Defaults to the first.
+#
+# @connected: How to handle existing clients when changing the
+# password. Will always be 'keep' for VNC, parameter is
+# deprecated and should be omitted.
+#
+# Since: 6.2
+#
+##
+{ 'struct': 'SetPasswordOptionsVnc',
+ 'data': { '*display': 'str', '*connected': 'SetPasswordActionVnc' } }
##
# @expire_password:
#
# Expire the password of a remote display server.
#
-# @protocol: the name of the remote display protocol 'vnc' or 'spice'
-#
-# @time: when to expire the password.
-#
-# - 'now' to expire the password immediately
-# - 'never' to cancel password expiration
-# - '+INT' where INT is the number of seconds from now (integer)
-# - 'INT' where INT is the absolute time in seconds
-#
# Returns: - Nothing on success
# - If @protocol is 'spice' and Spice is not active, DeviceNotFound
#
# Since: 0.14
#
-# Notes: Time is relative to the server and currently there is no way to
-# coordinate server time with client time. It is not recommended to
-# use the absolute time version of the @time parameter unless you're
-# sure you are on the same machine as the QEMU instance.
-#
# Example:
#
# -> { "execute": "expire_password", "arguments": { "protocol": "vnc",
@@ -71,7 +140,50 @@
# <- { "return": {} }
#
##
-{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
+{ 'command': 'expire_password', 'boxed': true, 'data': 'ExpirePasswordOptions' }
+
+##
+# @ExpirePasswordOptions:
+#
+# Data required to set password expiration on a display server protocol.
+#
+# @protocol: - 'vnc' to modify the VNC server expiration
+# - 'spice' to modify the Spice server expiration
+
+# @time: when to expire the password.
+#
+# - 'now' to expire the password immediately
+# - 'never' to cancel password expiration
+# - '+INT' where INT is the number of seconds from now (integer)
+# - 'INT' where INT is the absolute time in seconds
+#
+# Notes: Time is relative to the server and currently there is no way to
+# coordinate server time with client time. It is not recommended to
+# use the absolute time version of the @time parameter unless you're
+# sure you are on the same machine as the QEMU instance.
+#
+# Since: 6.2
+#
+##
+{ 'union': 'ExpirePasswordOptions',
+ 'base': { 'protocol': 'DisplayProtocol',
+ 'time': 'str' },
+ 'discriminator': 'protocol',
+ 'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
+
+##
+# @ExpirePasswordOptionsVnc:
+#
+# Options for expire_password specific to the VNC procotol.
+#
+# @display: The id of the display where the expiration should be changed.
+# Defaults to the first.
+#
+# Since: 6.2
+#
+##
+{ 'struct': 'ExpirePasswordOptionsVnc',
+ 'data': { '*display': 'str' } }
##
# @screendump:
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/3] monitor/hmp: add support for flag argument with value
2021-09-20 10:56 ` [PATCH v3 2/3] monitor/hmp: add support for flag argument with value Stefan Reiter
@ 2021-09-20 21:27 ` Eric Blake
0 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2021-09-20 21:27 UTC (permalink / raw)
To: Stefan Reiter
Cc: Wolfgang Bumiller, qemu-devel, Dr. David Alan Gilbert,
Markus Armbruster, Marc-André Lureau, Gerd Hoffmann,
Paolo Bonzini, Marc-André Lureau, Thomas Lamprecht
On Mon, Sep 20, 2021 at 12:56:40PM +0200, Stefan Reiter wrote:
> Adds support for the "-xS" parameter type, where "-x" denotes a flag
> name and the "S" suffix indicates that this flag is supposed to take an
> arbitrary string parameter.
>
> These parameters are always optional, the entry in the qdict will be
> omitted if the flag is not given.
>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> monitor/hmp.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
Looks like yet another hack on top of our existing pile of hacks, but
it is cleaner in v3 than it was in v2, and I'm not coming up with
anything better.
>
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index d50c3124e1..a32dce7a35 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -980,6 +980,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
> {
> const char *tmp = p;
> int skip_key = 0;
> + int ret;
> /* option */
>
> c = *typestr++;
> @@ -1002,8 +1003,22 @@ static QDict *monitor_parse_arguments(Monitor *mon,
> }
> if (skip_key) {
> p = tmp;
> + } else if (*typestr == 'S') {
> + /* has option with string value */
> + typestr++;
> + tmp = p++;
> + while (qemu_isspace(*p)) {
> + p++;
> + }
> + ret = get_str(buf, sizeof(buf), &p);
> + if (ret < 0) {
> + monitor_printf(mon, "%s: value expected for -%c\n",
> + cmd->name, *tmp);
> + goto fail;
> + }
> + qdict_put_str(qdict, key, buf);
Do we have any documentation that also needs a matching update? Or is
our documentation for the pseudo-grammar of .hx parsing limited to the
code?
> } else {
> - /* has option */
> + /* has boolean option */
> p++;
> qdict_put_bool(qdict, key, true);
> }
Holding my nose a bit, but only because of the mess that this already
is, not because of what you added.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 3/3] monitor: refactor set/expire_password and allow VNC display id
2021-09-20 10:56 ` [PATCH v3 3/3] monitor: refactor set/expire_password and allow VNC display id Stefan Reiter
@ 2021-09-20 21:37 ` Eric Blake
0 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2021-09-20 21:37 UTC (permalink / raw)
To: Stefan Reiter
Cc: Wolfgang Bumiller, qemu-devel, Dr. David Alan Gilbert,
Markus Armbruster, Marc-André Lureau, Gerd Hoffmann,
Paolo Bonzini, Marc-André Lureau, Thomas Lamprecht
On Mon, Sep 20, 2021 at 12:56:41PM +0200, Stefan Reiter wrote:
> It is possible to specify more than one VNC server on the command line,
> either with an explicit ID or the auto-generated ones à la "default",
> "vnc2", "vnc3", ...
>
> It is not possible to change the password on one of these extra VNC
> displays though. Fix this by adding a "display" parameter to the
> "set_password" and "expire_password" QMP and HMP commands.
>
> For HMP, the display is specified using the "-d" value flag.
>
> For QMP, the schema is updated to explicitly express the supported
> variants of the commands with protocol-discriminated unions.
>
> Suggested-by: Eric Blake <eblake@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>
> The union schema simplifies the QMP code, but makes the HMP part a bit more
> involved. Since the parameters are practically the same, is there a way to just
> pass the HMP generated qdict off to the qapi parser to get the correct struct
> for calling the qmp_ methods?
Possibly, but I don't know it off-hand.
>
> hmp-commands.hx | 29 ++++----
> monitor/hmp-cmds.c | 60 +++++++++++++++-
> monitor/qmp-cmds.c | 62 ++++++-----------
> qapi/ui.json | 168 +++++++++++++++++++++++++++++++++++++--------
> 4 files changed, 235 insertions(+), 84 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8e45bce2cd..d78e4cfc47 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1514,34 +1514,35 @@ ERST
>
> {
> .name = "set_password",
> - .args_type = "protocol:s,password:s,connected:s?",
> - .params = "protocol password action-if-connected",
> + .args_type = "protocol:s,password:s,display:-dS,connected:s?",
> + .params = "protocol password [-d display] [action-if-connected]",
Puts your new -xS of patch 2/3 to use.
> +++ b/monitor/hmp-cmds.c
> @@ -1451,10 +1451,43 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
> {
> const char *protocol = qdict_get_str(qdict, "protocol");
> const char *password = qdict_get_str(qdict, "password");
> + const char *display = qdict_get_try_str(qdict, "display");
> const char *connected = qdict_get_try_str(qdict, "connected");
> Error *err = NULL;
> + DisplayProtocol proto;
>
> - qmp_set_password(protocol, password, !!connected, connected, &err);
> + SetPasswordOptions opts = {
> + .password = g_strdup(password),
> + .u.vnc.display = NULL,
> + };
> +
> + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> + DISPLAY_PROTOCOL_VNC, &err);
> + if (err) {
> + hmp_handle_error(mon, err);
> + return;
> + }
> + opts.protocol = proto;
> +
> + if (proto == DISPLAY_PROTOCOL_VNC) {
> + if ((opts.u.vnc.has_display = !!display)) {
Assignment as a side-effect of the 'if' is a bit unusual in qemu
style.
> + opts.u.vnc.display = g_strdup(display);
> + }
In fact, g_strdup(NULL) does what you want; you can just write:
opts.u.vnc.has_display = !!display;
opts.u.vnc.display = g_strdup(display);
without an 'if'.
> + } else if (proto == DISPLAY_PROTOCOL_SPICE) {
> + if ((opts.u.spice.has_connected = !!connected)) {
And again.
> + opts.u.spice.connected =
> + qapi_enum_parse(&SetPasswordAction_lookup, connected,
> + SET_PASSWORD_ACTION_KEEP, &err);
> + if (err) {
> + hmp_handle_error(mon, err);
> + return;
> + }
> + }
> + }
> +
> + qmp_set_password(&opts, &err);
> + g_free(opts.password);
> + g_free(opts.u.vnc.display);
> hmp_handle_error(mon, err);
> }
>
> @@ -1462,9 +1495,32 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
> {
> const char *protocol = qdict_get_str(qdict, "protocol");
> const char *whenstr = qdict_get_str(qdict, "time");
> + const char *display = qdict_get_try_str(qdict, "display");
> Error *err = NULL;
> + DisplayProtocol proto;
>
> - qmp_expire_password(protocol, whenstr, &err);
> + ExpirePasswordOptions opts = {
> + .time = g_strdup(whenstr),
> + .u.vnc.display = NULL,
> + };
> +
> + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> + DISPLAY_PROTOCOL_VNC, &err);
> + if (err) {
> + hmp_handle_error(mon, err);
> + return;
> + }
> + opts.protocol = proto;
> +
> + if (proto == DISPLAY_PROTOCOL_VNC) {
> + if ((opts.u.vnc.has_display = !!display)) {
> + opts.u.vnc.display = g_strdup(display);
and again
> + }
> + }
> +
> + qmp_expire_password(&opts, &err);
> + g_free(opts.time);
> + g_free(opts.u.vnc.display);
> hmp_handle_error(mon, err);
> }
>
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 5c0d5e116b..cb229c01f8 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
...
> diff --git a/qapi/ui.json b/qapi/ui.json
> index b2cf7a6759..494c92a65e 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -9,22 +9,23 @@
> { 'include': 'common.json' }
> { 'include': 'sockets.json' }
>
> +##
> +# @DisplayProtocol:
> +#
> +# Display protocols which support changing password options.
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'enum': 'DisplayProtocol',
> + 'data': [ { 'name': 'vnc', 'if': 'CONFIG_VNC' },
> + { 'name': 'spice', 'if': 'CONFIG_SPICE' } ] }
> +
> ##
> # @set_password:
> #
> # Sets the password of a remote display session.
> #
> -# @protocol: - 'vnc' to modify the VNC server password
> -# - 'spice' to modify the Spice server password
> -#
> -# @password: the new password
> -#
> -# @connected: how to handle existing clients when changing the
> -# password. If nothing is specified, defaults to 'keep'
> -# 'fail' to fail the command if clients are connected
> -# 'disconnect' to disconnect existing clients
> -# 'keep' to maintain existing clients
> -#
> # Returns: - Nothing on success
> # - If Spice is not enabled, DeviceNotFound
> #
> @@ -37,33 +38,101 @@
> # <- { "return": {} }
> #
> ##
> -{ 'command': 'set_password',
> - 'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
> +{ 'command': 'set_password', 'boxed': true, 'data': 'SetPasswordOptions' }
> +
> +##
> +# @SetPasswordOptions:
> +#
> +# Data required to set a new password on a display server protocol.
> +#
> +# @protocol: - 'vnc' to modify the VNC server password
> +# - 'spice' to modify the Spice server password
> +#
> +# @password: the new password
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'union': 'SetPasswordOptions',
> + 'base': { 'protocol': 'DisplayProtocol',
> + 'password': 'str' },
> + 'discriminator': 'protocol',
> + 'data': { 'vnc': 'SetPasswordOptionsVnc',
> + 'spice': 'SetPasswordOptionsSpice' } }
> +
> +##
> +# @SetPasswordAction:
> +#
> +# An action to take on changing a password on a connection with active clients.
> +#
> +# @fail: fail the command if clients are connected
> +#
> +# @disconnect: disconnect existing clients
> +#
> +# @keep: maintain existing clients
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'enum': 'SetPasswordAction',
> + 'data': [ 'fail', 'disconnect', 'keep' ] }
> +
> +##
> +# @SetPasswordActionVnc:
> +#
> +# See @SetPasswordAction. VNC only supports the keep action. 'connection'
> +# should just be omitted for VNC, this is kept for backwards compatibility.
> +#
> +# @keep: maintain existing clients
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'enum': 'SetPasswordActionVnc',
> + 'data': [ 'keep' ] }
> +
> +##
> +# @SetPasswordOptionsSpice:
> +#
> +# Options for set_password specific to the VNC procotol.
> +#
> +# @connected: How to handle existing clients when changing the
> +# password. If nothing is specified, defaults to 'keep'.
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'struct': 'SetPasswordOptionsSpice',
> + 'data': { '*connected': 'SetPasswordAction' } }
> +
> +##
> +# @SetPasswordOptionsVnc:
> +#
> +# Options for set_password specific to the VNC procotol.
> +#
> +# @display: The id of the display where the password should be changed.
> +# Defaults to the first.
> +#
> +# @connected: How to handle existing clients when changing the
> +# password. Will always be 'keep' for VNC, parameter is
> +# deprecated and should be omitted.
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'struct': 'SetPasswordOptionsVnc',
> + 'data': { '*display': 'str', '*connected': 'SetPasswordActionVnc' } }
Doesn't have to be in this patch (as you are refactoring a
pre-existing problem), but we should consider a followup patch to
actually use the QAPI 'features':'deprected' ability to actually
declare the deprecation through introspection.
Overall, the QAPI changes look sane to me.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-20 22:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-20 10:56 [PATCH v3 0/3] VNC-related HMP/QMP fixes Stefan Reiter
2021-09-20 10:56 ` [PATCH v3 1/3] monitor/hmp: correctly invert password argument detection again Stefan Reiter
2021-09-20 10:56 ` [PATCH v3 2/3] monitor/hmp: add support for flag argument with value Stefan Reiter
2021-09-20 21:27 ` Eric Blake
2021-09-20 10:56 ` [PATCH v3 3/3] monitor: refactor set/expire_password and allow VNC display id Stefan Reiter
2021-09-20 21:37 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.