[PATCH v7 2/4] qapi/monitor: refactor set/expire_password with enums
'protocol' and 'connected' are better suited as enums than as strings,
make use of that. No functional change intended.
Suggested-by: Markus Armbruster
Reviewed-by: Markus Armbruster
Signed-off-by: Stefan Reiter
---
monitor/hmp-cmds.c | 29 +++--
monitor/qmp-cmds.c | 37 -
qapi/ui.json | 37 +++--
3 files changed, 74 insertions(+), 29 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index bcaa41350e..b8abe69609 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1453,8 +1453,24 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
const char *password = qdict_get_str(qdict, "password");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
+DisplayProtocol proto;
+SetPasswordAction conn;
-qmp_set_password(protocol, password, !!connected, connected, );
+proto = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
+if (err) {
+goto out;
+}
+
+conn = qapi_enum_parse(_lookup, connected,
+ SET_PASSWORD_ACTION_KEEP, );
+if (err) {
+goto out;
+}
+
+qmp_set_password(proto, password, !!connected, conn, );
+
+out:
hmp_handle_error(mon, err);
}
@@ -1463,8 +1479,17 @@ void hmp_expire_password(Monitor *mon, const QDict
*qdict)
const char *protocol = qdict_get_str(qdict, "protocol");
const char *whenstr = qdict_get_str(qdict, "time");
Error *err = NULL;
+DisplayProtocol proto;
-qmp_expire_password(protocol, whenstr, );
+proto = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
+if (err) {
+goto out;
+}
+
+qmp_expire_password(proto, whenstr, );
+
+out:
hmp_handle_error(mon, err);
}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5c0d5e116b..0654d7289a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -163,33 +163,27 @@ void qmp_system_wakeup(Error **errp)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
}
-void qmp_set_password(const char *protocol, const char *password,
- bool has_connected, const char *connected, Error **errp)
+void qmp_set_password(DisplayProtocol protocol, const char *password,
+ bool has_connected, SetPasswordAction connected,
+ Error **errp)
{
int disconnect_if_connected = 0;
int fail_if_connected = 0;
int rc;
if (has_connected) {
-if (strcmp(connected, "fail") == 0) {
-fail_if_connected = 1;
-} else if (strcmp(connected, "disconnect") == 0) {
-disconnect_if_connected = 1;
-} else if (strcmp(connected, "keep") == 0) {
-/* nothing */
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER, "connected");
-return;
-}
+fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL;
+disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT;
}
-if (strcmp(protocol, "spice") == 0) {
+if (protocol == DISPLAY_PROTOCOL_SPICE) {
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_passwd(password, fail_if_connected,
disconnect_if_connected);
-} else if (strcmp(protocol, "vnc") == 0) {
+} else {
+assert(protocol == DISPLAY_PROTOCOL_VNC);
if (fail_if_connected || disconnect_if_connected) {
/* vnc supports "connected=keep" only */
error_setg(errp, QERR_INVALID_PARAMETER, "connected");
@@ -198,10 +192,6 @@ void qmp_set_password(const char *protocol, const char
*password,
/* Note that setting an empty password will not disable login through
* this interface. */
rc = vnc_display_password(NULL, password);
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
- "'vnc' or 'spice'");
-return;
}
if (rc != 0) {
@@ -209,7 +199,7 @@ void qmp_set_password(const char *protocol, const char
*password,
}
}
-void qmp_expire_password(const char *protocol, const char *whenstr,
+void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
Error **errp)
{
time_t when;
@@ -225,17 +215,14 @@ void qmp_expire_password(const char *protocol, const char
*whenstr,
when = strtoull(whenstr, NULL, 10);
}
-if (strcmp(protocol, "spice") == 0) {
+if (protocol == DISPLAY_PROTOCOL_SPICE) {
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_pw_expire(when);
-} else if (strcmp(pro
[PATCH v7 4/4] qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate
VNC only supports 'keep' here, enforce this via a seperate
SetPasswordActionVnc enum and mark the option 'deprecated' (as it is
useless with only one value possible).
Also add a deprecation note to docs.
Suggested-by: Eric Blake
Reviewed-by: Markus Armbruster
Signed-off-by: Stefan Reiter
---
docs/about/deprecated.rst | 6 ++
monitor/qmp-cmds.c| 5 -
qapi/ui.json | 21 -
3 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 0bed6ecb1d..f484b058bc 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -228,6 +228,12 @@ Use the more generic commands ``block-export-add`` and
``block-export-del``
instead. As part of this deprecation, where ``nbd-server-add`` used a
single ``bitmap``, the new ``block-export-add`` uses a list of ``bitmaps``.
+``set_password`` argument ``connected`` for VNC protocol (since 6.2)
+
+
+Only the value ``keep`` is and was ever supported for VNC. The (useless)
+argument will be dropped in a future version of QEMU.
+
System accelerators
---
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5637bd70b6..4825d0cbea 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -176,11 +176,6 @@ void qmp_set_password(SetPasswordOptions *opts, Error
**errp)
opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT);
} else {
assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
-if (opts->u.vnc.connected != SET_PASSWORD_ACTION_KEEP) {
-/* vnc supports "connected=keep" only */
-error_setg(errp, QERR_INVALID_PARAMETER, "connected");
-return;
-}
/* Note that setting an empty password will not disable login through
* this interface. */
rc = vnc_display_password(opts->u.vnc.display, opts->password);
diff --git a/qapi/ui.json b/qapi/ui.json
index 99ac29ad9c..5292617b44 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -38,6 +38,20 @@
{ 'enum': 'SetPasswordAction',
'data': [ 'fail', 'disconnect', 'keep' ] }
+##
+# @SetPasswordActionVnc:
+#
+# See @SetPasswordAction. VNC only supports the keep action. 'connection'
+# should just be omitted for VNC, this is kept for backwards compatibility.
+#
+# @keep: maintain existing clients
+#
+# Since: 6.2
+#
+##
+{ 'enum': 'SetPasswordActionVnc',
+ 'data': [ 'keep' ] }
+
##
# @SetPasswordOptions:
#
@@ -83,12 +97,17 @@
# @connected: How to handle existing clients when changing the
# password.
#
+# Features:
+# @deprecated: For VNC, @connected will always be 'keep', parameter should be
+# omitted.
+#
# Since: 6.2
#
##
{ 'struct': 'SetPasswordOptionsVnc',
'data': { '*display': 'str',
-'*connected': 'SetPasswordAction' }}
+'*connected': { 'type': 'SetPasswordActionVnc',
+'features': ['deprecated'] } } }
##
# @set_password:
--
2.30.2
[PATCH v7 0/4] VNC-related HMP/QMP fixes
Since the removal of the generic 'qmp_change' command, one can no longer replace the 'default' VNC display listen address at runtime (AFAIK). For our users who need to set up a secondary VNC access port, this means configuring a second VNC display (in addition to our standard one for web-access), but it turns out one cannot set a password on this second display at the moment, as the 'set_password' call only operates on the 'default' display. Additionally, using secret objects, the password is only read once at startup. This could be considered a bug too, but is not touched in this series and left for a later date. v6 -> v7: * remove g_strdup and g_free, use strings directly * squash in last patch v5 -> v6: * consider feedback from Markus' review, mainly: * fix crash bug in patch 1 (sorry, artifact of patch-splitting) * rely on '!has_param => param == NULL' to shorten code * add note to 'docs/about/deprecated.rst' and touch up comments a bit * go back to g_free instead of qapi_free_* since the latter apparently tries to free the passed in pointer which lives on the stack... * fix bug in HMP parsing (see patch 1) v4 -> v5: * add comment to patch 1 in "monitor-internal.h" * use qapi_free_SetPasswordOptions and friends, don't leak strdups * split QAPI change into 3 seperate patches v3 -> v4: * drop previously patch 1, this was fixed here instead: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02529.html * patch 1: add Eric's R-b * patch 2: remove if-assignment, use 'deprecated' feature in schema v2 -> v3: * refactor QMP schema for set/expire_password as suggested by Eric Blake and Markus Armbruster v1 -> v2: * add Marc-André's R-b on patch 1 * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann * I didn't see a way to do this yet, so I added a "flags with values" arg type Stefan Reiter (4): monitor/hmp: add support for flag argument with value qapi/monitor: refactor set/expire_password with enums qapi/monitor: allow VNC display id in set/expire_password qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate docs/about/deprecated.rst | 6 ++ hmp-commands.hx| 24 +++--- monitor/hmp-cmds.c | 48 +++- monitor/hmp.c | 19 - monitor/monitor-internal.h | 3 +- monitor/qmp-cmds.c | 54 - qapi/ui.json | 156 - 7 files changed, 236 insertions(+), 74 deletions(-) -- 2.30.2
[PATCH v7 1/4] monitor/hmp: add support for flag argument with value
Adds support for the "-xV" parameter type, where "-x" denotes a flag
name and the "V" suffix indicates that this flag is supposed to take an
arbitrary string parameter.
These parameters are always optional, the entry in the qdict will be
omitted if the flag is not given.
Signed-off-by: Stefan Reiter
---
v6:
It wasn't possible to pass the 'connected' parameter to set_password, since the
code to handle optional parameters couldn't live with a different param (not
starting with '-') coming up instead - fix that by advancing over the 'value
flag' modifier in case `*p != '-'`.
Also change the modifier to 'V' instead of 'S' so it can be distinguished from
an actual trailing 'S' type param.
Discovered in testing. I dropped Eric's R-b due to the code change.
monitor/hmp.c | 19 ++-
monitor/monitor-internal.h | 3 ++-
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/monitor/hmp.c b/monitor/hmp.c
index d50c3124e1..899e0c990f 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -980,6 +980,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
{
const char *tmp = p;
int skip_key = 0;
+int ret;
/* option */
c = *typestr++;
@@ -1002,11 +1003,27 @@ static QDict *monitor_parse_arguments(Monitor *mon,
}
if (skip_key) {
p = tmp;
+} else if (*typestr == 'V') {
+/* has option with string value */
+typestr++;
+tmp = p++;
+while (qemu_isspace(*p)) {
+p++;
+}
+ret = get_str(buf, sizeof(buf), );
+if (ret < 0) {
+monitor_printf(mon, "%s: value expected for -%c\n",
+ cmd->name, *tmp);
+goto fail;
+}
+qdict_put_str(qdict, key, buf);
} else {
-/* has option */
+/* has boolean option */
p++;
qdict_put_bool(qdict, key, true);
}
+} else if (*typestr == 'V') {
+typestr++;
}
}
break;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 9c3a09cb01..9e708b329d 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -63,7 +63,8 @@
* '.' other form of optional type (for 'i' and 'l')
* 'b' boolean
* user mode accepts "on" or "off"
- * '-' optional parameter (eg. '-f')
+ * '-' optional parameter (eg. '-f'); if followed by an 'V', it
+ * specifies an optional string param (e.g. '-fV' allows '-f foo')
*
*/
--
2.30.2
[PATCH v7 3/4] qapi/monitor: allow VNC display id in set/expire_password
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
Signed-off-by: Stefan Reiter
---
hmp-commands.hx| 24 +-
monitor/hmp-cmds.c | 45 --
monitor/qmp-cmds.c | 36 ++-
qapi/ui.json | 112 +++--
4 files changed, 148 insertions(+), 69 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index cf723c69ac..9fbb207b35 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1514,33 +1514,35 @@ ERST
{
.name = "set_password",
-.args_type = "protocol:s,password:s,connected:s?",
-.params = "protocol password action-if-connected",
+.args_type = "protocol:s,password:s,display:-dV,connected:s?",
+.params = "protocol password [-d display] [action-if-connected]",
.help = "set spice/vnc password",
.cmd= hmp_set_password,
},
SRST
-``set_password [ vnc | spice ] password [ action-if-connected ]``
- Change spice/vnc password. *action-if-connected* specifies what
- should happen in case a connection is established: *fail* makes the
- password change fail. *disconnect* changes the password and
+``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected
]``
+ Change spice/vnc password. *display* can be used with 'vnc' to specify
+ which display to set the password on. *action-if-connected* specifies
+ what should happen in case a connection is established: *fail* makes
+ the password change fail. *disconnect* changes the password and
disconnects the client. *keep* changes the password and keeps the
connection up. *keep* is the default.
ERST
{
.name = "expire_password",
-.args_type = "protocol:s,time:s",
-.params = "protocol time",
+.args_type = "protocol:s,time:s,display:-dV",
+.params = "protocol time [-d display]",
.help = "set spice/vnc password expire-time",
.cmd= hmp_expire_password,
},
SRST
-``expire_password [ vnc | spice ]`` *expire-time*
- Specify when a password for spice/vnc becomes
- invalid. *expire-time* accepts:
+``expire_password [ vnc | spice ] expire-time [ -d display ]``
+ Specify when a password for spice/vnc becomes invalid.
+ *display* behaves the same as in ``set_password``.
+ *expire-time* accepts:
``now``
Invalidate password instantly.
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b8abe69609..f0f0c82d59 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,24 +1451,34 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *password = qdict_get_str(qdict, "password");
+const char *display = qdict_get_try_str(qdict, "display");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
-DisplayProtocol proto;
-SetPasswordAction conn;
-proto = qapi_enum_parse(_lookup, protocol,
-DISPLAY_PROTOCOL_VNC, );
+SetPasswordOptions opts = {
+.password = (char *)password,
+};
+
+opts.protocol = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
if (err) {
goto out;
}
-conn = qapi_enum_parse(_lookup, connected,
- SET_PASSWORD_ACTION_KEEP, );
-if (err) {
-goto out;
+if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+opts.u.vnc.has_display = !!display;
+opts.u.vnc.display = (char *)display;
+} else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
+opts.u.spice.has_connected = !!connected;
+opts.u.spice.connected =
+qapi_enum_parse(_lookup, connected,
+SET_PASSWORD_ACTION_KEEP, );
+if (err) {
+goto out;
+}
}
-qmp_set_password(proto, password, !!connected, conn, );
+qmp_set_password(, );
out:
hmp_handle_error(mon, err);
@@ -1478,16 +1488,25 @@ void hmp_expire_password(Monitor *mon, const QDict
*qdict)
{
const char *protocol = qdict_get_str(qdict, &quo
Re: [PATCH v6 3/5] qapi/monitor: allow VNC display id in set/expire_password
On 10/21/21 7:05 AM, Markus Armbruster wrote:
Stefan Reiter writes:
It is possible to specify more than one VNC server on the command line,
either with an explicit ID or the auto-generated ones à la "default",
"vnc2", "vnc3", ...
It is not possible to change the password on one of these extra VNC
displays though. Fix this by adding a "display" parameter to the
"set_password" and "expire_password" QMP and HMP commands.
For HMP, the display is specified using the "-d" value flag.
For QMP, the schema is updated to explicitly express the supported
variants of the commands with protocol-discriminated unions.
Suggested-by: Markus Armbruster
Signed-off-by: Stefan Reiter
---
hmp-commands.hx| 24 +-
monitor/hmp-cmds.c | 51 +++--
monitor/qmp-cmds.c | 36 ++-
qapi/ui.json | 112 +++--
4 files changed, 154 insertions(+), 69 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index cf723c69ac..9fbb207b35 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1514,33 +1514,35 @@ ERST
{
.name = "set_password",
-.args_type = "protocol:s,password:s,connected:s?",
-.params = "protocol password action-if-connected",
+.args_type = "protocol:s,password:s,display:-dV,connected:s?",
+.params = "protocol password [-d display] [action-if-connected]",
.help = "set spice/vnc password",
.cmd= hmp_set_password,
},
SRST
-``set_password [ vnc | spice ] password [ action-if-connected ]``
- Change spice/vnc password. *action-if-connected* specifies what
- should happen in case a connection is established: *fail* makes the
- password change fail. *disconnect* changes the password and
+``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected
]``
+ Change spice/vnc password. *display* can be used with 'vnc' to specify
+ which display to set the password on. *action-if-connected* specifies
+ what should happen in case a connection is established: *fail* makes
+ the password change fail. *disconnect* changes the password and
disconnects the client. *keep* changes the password and keeps the
connection up. *keep* is the default.
ERST
{
.name = "expire_password",
-.args_type = "protocol:s,time:s",
-.params = "protocol time",
+.args_type = "protocol:s,time:s,display:-dV",
+.params = "protocol time [-d display]",
.help = "set spice/vnc password expire-time",
.cmd= hmp_expire_password,
},
SRST
-``expire_password [ vnc | spice ]`` *expire-time*
- Specify when a password for spice/vnc becomes
- invalid. *expire-time* accepts:
+``expire_password [ vnc | spice ] expire-time [ -d display ]``
+ Specify when a password for spice/vnc becomes invalid.
+ *display* behaves the same as in ``set_password``.
+ *expire-time* accepts:
``now``
Invalidate password instantly.
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b8abe69609..daa4a8e106 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,26 +1451,39 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *password = qdict_get_str(qdict, "password");
+const char *display = qdict_get_try_str(qdict, "display");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
-DisplayProtocol proto;
-SetPasswordAction conn;
-proto = qapi_enum_parse(_lookup, protocol,
-DISPLAY_PROTOCOL_VNC, );
+SetPasswordOptions opts = {
+.password = g_strdup(password),
+.u.vnc.display = NULL,
+};
+
+opts.protocol = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
if (err) {
goto out;
}
-conn = qapi_enum_parse(_lookup, connected,
- SET_PASSWORD_ACTION_KEEP, );
-if (err) {
-goto out;
+if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+opts.u.vnc.has_display = !!display;
+opts.u.vnc.display = g_strdup(display);
+} else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
+opts.u.spice.has_connected = !!connected;
+opts.u.spice.connected =
+qapi_enum_parse(_lookup, connected,
+SET_PASSWORD_ACTION_KEEP, );
+if (err) {
+goto out;
+}
}
-qmp_set_password(proto, password, !!connected, conn, );
+qmp_set_password(, );
out:
+g_free(opts.password);
+g_free(
[PATCH v6 1/5] monitor/hmp: add support for flag argument with value
Adds support for the "-xV" parameter type, where "-x" denotes a flag
name and the "V" suffix indicates that this flag is supposed to take an
arbitrary string parameter.
These parameters are always optional, the entry in the qdict will be
omitted if the flag is not given.
Signed-off-by: Stefan Reiter
---
v6:
It wasn't possible to pass the 'connected' parameter to set_password, since the
code to handle optional parameters couldn't live with a different param (not
starting with '-') coming up instead - fix that by advancing over the 'value
flag' modifier in case `*p != '-'`.
Also change the modifier to 'V' instead of 'S' so it can be distinguished from
an actual trailing 'S' type param.
Discovered in testing. I dropped Eric's R-b due to the code change.
monitor/hmp.c | 19 ++-
monitor/monitor-internal.h | 3 ++-
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/monitor/hmp.c b/monitor/hmp.c
index d50c3124e1..899e0c990f 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -980,6 +980,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
{
const char *tmp = p;
int skip_key = 0;
+int ret;
/* option */
c = *typestr++;
@@ -1002,11 +1003,27 @@ static QDict *monitor_parse_arguments(Monitor *mon,
}
if (skip_key) {
p = tmp;
+} else if (*typestr == 'V') {
+/* has option with string value */
+typestr++;
+tmp = p++;
+while (qemu_isspace(*p)) {
+p++;
+}
+ret = get_str(buf, sizeof(buf), );
+if (ret < 0) {
+monitor_printf(mon, "%s: value expected for -%c\n",
+ cmd->name, *tmp);
+goto fail;
+}
+qdict_put_str(qdict, key, buf);
} else {
-/* has option */
+/* has boolean option */
p++;
qdict_put_bool(qdict, key, true);
}
+} else if (*typestr == 'V') {
+typestr++;
}
}
break;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 9c3a09cb01..9e708b329d 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -63,7 +63,8 @@
* '.' other form of optional type (for 'i' and 'l')
* 'b' boolean
* user mode accepts "on" or "off"
- * '-' optional parameter (eg. '-f')
+ * '-' optional parameter (eg. '-f'); if followed by an 'V', it
+ * specifies an optional string param (e.g. '-fV' allows '-f foo')
*
*/
--
2.30.2
[PATCH v6 0/5] VNC-related HMP/QMP fixes
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. 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 (5): monitor/hmp: add support for flag argument with value qapi/monitor: refactor set/expire_password with enums qapi/monitor: allow VNC display id in set/expire_password qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate docs: add deprecation note about 'set_password' param 'connected' docs/about/deprecated.rst | 6 ++ hmp-commands.hx| 24 +++--- monitor/hmp-cmds.c | 54 - monitor/hmp.c | 19 - monitor/monitor-internal.h | 3 +- monitor/qmp-cmds.c | 54 - qapi/ui.json | 156 - 7 files changed, 242 insertions(+), 74 deletions(-) -- 2.30.2
[PATCH v6 3/5] qapi/monitor: allow VNC display id in set/expire_password
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
Signed-off-by: Stefan Reiter
---
hmp-commands.hx| 24 +-
monitor/hmp-cmds.c | 51 +++--
monitor/qmp-cmds.c | 36 ++-
qapi/ui.json | 112 +++--
4 files changed, 154 insertions(+), 69 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index cf723c69ac..9fbb207b35 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1514,33 +1514,35 @@ ERST
{
.name = "set_password",
-.args_type = "protocol:s,password:s,connected:s?",
-.params = "protocol password action-if-connected",
+.args_type = "protocol:s,password:s,display:-dV,connected:s?",
+.params = "protocol password [-d display] [action-if-connected]",
.help = "set spice/vnc password",
.cmd= hmp_set_password,
},
SRST
-``set_password [ vnc | spice ] password [ action-if-connected ]``
- Change spice/vnc password. *action-if-connected* specifies what
- should happen in case a connection is established: *fail* makes the
- password change fail. *disconnect* changes the password and
+``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected
]``
+ Change spice/vnc password. *display* can be used with 'vnc' to specify
+ which display to set the password on. *action-if-connected* specifies
+ what should happen in case a connection is established: *fail* makes
+ the password change fail. *disconnect* changes the password and
disconnects the client. *keep* changes the password and keeps the
connection up. *keep* is the default.
ERST
{
.name = "expire_password",
-.args_type = "protocol:s,time:s",
-.params = "protocol time",
+.args_type = "protocol:s,time:s,display:-dV",
+.params = "protocol time [-d display]",
.help = "set spice/vnc password expire-time",
.cmd= hmp_expire_password,
},
SRST
-``expire_password [ vnc | spice ]`` *expire-time*
- Specify when a password for spice/vnc becomes
- invalid. *expire-time* accepts:
+``expire_password [ vnc | spice ] expire-time [ -d display ]``
+ Specify when a password for spice/vnc becomes invalid.
+ *display* behaves the same as in ``set_password``.
+ *expire-time* accepts:
``now``
Invalidate password instantly.
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b8abe69609..daa4a8e106 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,26 +1451,39 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *password = qdict_get_str(qdict, "password");
+const char *display = qdict_get_try_str(qdict, "display");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
-DisplayProtocol proto;
-SetPasswordAction conn;
-proto = qapi_enum_parse(_lookup, protocol,
-DISPLAY_PROTOCOL_VNC, );
+SetPasswordOptions opts = {
+.password = g_strdup(password),
+.u.vnc.display = NULL,
+};
+
+opts.protocol = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
if (err) {
goto out;
}
-conn = qapi_enum_parse(_lookup, connected,
- SET_PASSWORD_ACTION_KEEP, );
-if (err) {
-goto out;
+if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+opts.u.vnc.has_display = !!display;
+opts.u.vnc.display = g_strdup(display);
+} else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
+opts.u.spice.has_connected = !!connected;
+opts.u.spice.connected =
+qapi_enum_parse(_lookup, connected,
+SET_PASSWORD_ACTION_KEEP, );
+if (err) {
+goto out;
+}
}
-qmp_set_password(proto, password, !!connected, conn, );
+qmp_set_password(, );
out:
+g_free(opts.password);
+g_free(opts.u.vnc.display);
hmp_handle_error(mon, err);
}
@@ -1478,18 +1491,30 @@ void hmp_expire_password(Monitor *mon,
[PATCH v6 5/5] docs: add deprecation note about 'set_password' param 'connected'
Signed-off-by: Stefan Reiter --- Seperate patch since it read a bit unsure in the review, feel free to either drop or squash this. docs/about/deprecated.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 0bed6ecb1d..1ad08e57d2 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -228,6 +228,12 @@ Use the more generic commands ``block-export-add`` and ``block-export-del`` instead. As part of this deprecation, where ``nbd-server-add`` used a single ``bitmap``, the new ``block-export-add`` uses a list of ``bitmaps``. +``set_password`` argument ``connected`` for VNC protocol (since 6.2) + + +Only the value ``keep`` is and was ever supported for VNC. It is recommended to +just drop the argument. + System accelerators --- -- 2.30.2
[PATCH v6 4/5] qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate
VNC only supports 'keep' here, enforce this via a seperate
SetPasswordActionVnc enum and mark the option 'deprecated' (as it is
useless with only one value possible).
Suggested-by: Eric Blake
Signed-off-by: Stefan Reiter
---
monitor/qmp-cmds.c | 5 -
qapi/ui.json | 21 -
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5637bd70b6..4825d0cbea 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -176,11 +176,6 @@ void qmp_set_password(SetPasswordOptions *opts, Error
**errp)
opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT);
} else {
assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
-if (opts->u.vnc.connected != SET_PASSWORD_ACTION_KEEP) {
-/* vnc supports "connected=keep" only */
-error_setg(errp, QERR_INVALID_PARAMETER, "connected");
-return;
-}
/* Note that setting an empty password will not disable login through
* this interface. */
rc = vnc_display_password(opts->u.vnc.display, opts->password);
diff --git a/qapi/ui.json b/qapi/ui.json
index 99ac29ad9c..5292617b44 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -38,6 +38,20 @@
{ 'enum': 'SetPasswordAction',
'data': [ 'fail', 'disconnect', 'keep' ] }
+##
+# @SetPasswordActionVnc:
+#
+# See @SetPasswordAction. VNC only supports the keep action. 'connection'
+# should just be omitted for VNC, this is kept for backwards compatibility.
+#
+# @keep: maintain existing clients
+#
+# Since: 6.2
+#
+##
+{ 'enum': 'SetPasswordActionVnc',
+ 'data': [ 'keep' ] }
+
##
# @SetPasswordOptions:
#
@@ -83,12 +97,17 @@
# @connected: How to handle existing clients when changing the
# password.
#
+# Features:
+# @deprecated: For VNC, @connected will always be 'keep', parameter should be
+# omitted.
+#
# Since: 6.2
#
##
{ 'struct': 'SetPasswordOptionsVnc',
'data': { '*display': 'str',
-'*connected': 'SetPasswordAction' }}
+'*connected': { 'type': 'SetPasswordActionVnc',
+'features': ['deprecated'] } } }
##
# @set_password:
--
2.30.2
[PATCH v6 2/5] qapi/monitor: refactor set/expire_password with enums
'protocol' and 'connected' are better suited as enums than as strings,
make use of that. No functional change intended.
Suggested-by: Markus Armbruster
Signed-off-by: Stefan Reiter
---
monitor/hmp-cmds.c | 29 +++--
monitor/qmp-cmds.c | 37 -
qapi/ui.json | 37 +++--
3 files changed, 74 insertions(+), 29 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index bcaa41350e..b8abe69609 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1453,8 +1453,24 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
const char *password = qdict_get_str(qdict, "password");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
+DisplayProtocol proto;
+SetPasswordAction conn;
-qmp_set_password(protocol, password, !!connected, connected, );
+proto = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
+if (err) {
+goto out;
+}
+
+conn = qapi_enum_parse(_lookup, connected,
+ SET_PASSWORD_ACTION_KEEP, );
+if (err) {
+goto out;
+}
+
+qmp_set_password(proto, password, !!connected, conn, );
+
+out:
hmp_handle_error(mon, err);
}
@@ -1463,8 +1479,17 @@ void hmp_expire_password(Monitor *mon, const QDict
*qdict)
const char *protocol = qdict_get_str(qdict, "protocol");
const char *whenstr = qdict_get_str(qdict, "time");
Error *err = NULL;
+DisplayProtocol proto;
-qmp_expire_password(protocol, whenstr, );
+proto = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
+if (err) {
+goto out;
+}
+
+qmp_expire_password(proto, whenstr, );
+
+out:
hmp_handle_error(mon, err);
}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5c0d5e116b..0654d7289a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -163,33 +163,27 @@ void qmp_system_wakeup(Error **errp)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
}
-void qmp_set_password(const char *protocol, const char *password,
- bool has_connected, const char *connected, Error **errp)
+void qmp_set_password(DisplayProtocol protocol, const char *password,
+ bool has_connected, SetPasswordAction connected,
+ Error **errp)
{
int disconnect_if_connected = 0;
int fail_if_connected = 0;
int rc;
if (has_connected) {
-if (strcmp(connected, "fail") == 0) {
-fail_if_connected = 1;
-} else if (strcmp(connected, "disconnect") == 0) {
-disconnect_if_connected = 1;
-} else if (strcmp(connected, "keep") == 0) {
-/* nothing */
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER, "connected");
-return;
-}
+fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL;
+disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT;
}
-if (strcmp(protocol, "spice") == 0) {
+if (protocol == DISPLAY_PROTOCOL_SPICE) {
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_passwd(password, fail_if_connected,
disconnect_if_connected);
-} else if (strcmp(protocol, "vnc") == 0) {
+} else {
+assert(protocol == DISPLAY_PROTOCOL_VNC);
if (fail_if_connected || disconnect_if_connected) {
/* vnc supports "connected=keep" only */
error_setg(errp, QERR_INVALID_PARAMETER, "connected");
@@ -198,10 +192,6 @@ void qmp_set_password(const char *protocol, const char
*password,
/* Note that setting an empty password will not disable login through
* this interface. */
rc = vnc_display_password(NULL, password);
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
- "'vnc' or 'spice'");
-return;
}
if (rc != 0) {
@@ -209,7 +199,7 @@ void qmp_set_password(const char *protocol, const char
*password,
}
}
-void qmp_expire_password(const char *protocol, const char *whenstr,
+void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
Error **errp)
{
time_t when;
@@ -225,17 +215,14 @@ void qmp_expire_password(const char *protocol, const char
*whenstr,
when = strtoull(whenstr, NULL, 10);
}
-if (strcmp(protocol, "spice") == 0) {
+if (protocol == DISPLAY_PROTOCOL_SPICE) {
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_pw_expire(when);
-} else if (strcmp(protocol, "vnc") == 0) {
-
[PATCH v5 3/4] qapi/monitor: allow VNC display id in set/expire_password
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
Signed-off-by: Stefan Reiter
---
hmp-commands.hx| 24 +-
monitor/hmp-cmds.c | 51 ++---
monitor/qmp-cmds.c | 44 +-
qapi/ui.json | 112 +++--
4 files changed, 170 insertions(+), 61 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index cf723c69ac..d78e4cfc47 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1514,33 +1514,35 @@ ERST
{
.name = "set_password",
-.args_type = "protocol:s,password:s,connected:s?",
-.params = "protocol password action-if-connected",
+.args_type = "protocol:s,password:s,display:-dS,connected:s?",
+.params = "protocol password [-d display] [action-if-connected]",
.help = "set spice/vnc password",
.cmd= hmp_set_password,
},
SRST
-``set_password [ vnc | spice ] password [ action-if-connected ]``
- Change spice/vnc password. *action-if-connected* specifies what
- should happen in case a connection is established: *fail* makes the
- password change fail. *disconnect* changes the password and
+``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected
]``
+ Change spice/vnc password. *display* can be used with 'vnc' to specify
+ which display to set the password on. *action-if-connected* specifies
+ what should happen in case a connection is established: *fail* makes
+ the password change fail. *disconnect* changes the password and
disconnects the client. *keep* changes the password and keeps the
connection up. *keep* is the default.
ERST
{
.name = "expire_password",
-.args_type = "protocol:s,time:s",
-.params = "protocol time",
+.args_type = "protocol:s,time:s,display:-dS",
+.params = "protocol time [-d display]",
.help = "set spice/vnc password expire-time",
.cmd= hmp_expire_password,
},
SRST
-``expire_password [ vnc | spice ]`` *expire-time*
- Specify when a password for spice/vnc becomes
- invalid. *expire-time* accepts:
+``expire_password [ vnc | spice ] expire-time [ -d display ]``
+ Specify when a password for spice/vnc becomes invalid.
+ *display* behaves the same as in ``set_password``.
+ *expire-time* accepts:
``now``
Invalidate password instantly.
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 0ff18ebb3c..79549596e4 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,19 +1451,40 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *password = qdict_get_str(qdict, "password");
+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;
+
+SetPasswordOptions opts = {
+.password = g_strdup(password),
+.u.vnc.display = NULL,
+};
proto = qapi_enum_parse(_lookup, protocol,
DISPLAY_PROTOCOL_VNC, );
-if (!!connected) {
-conn = qapi_enum_parse(_lookup, connected,
- SET_PASSWORD_ACTION_KEEP, );
+if (err) {
+goto out;
+}
+opts.protocol = proto;
+
+if (proto == DISPLAY_PROTOCOL_VNC) {
+opts.u.vnc.has_display = !!display;
+opts.u.vnc.display = g_strdup(display);
+} else if (proto == DISPLAY_PROTOCOL_SPICE) {
+opts.u.spice.has_connected = !!connected;
+opts.u.spice.connected =
+qapi_enum_parse(_lookup, connected,
+SET_PASSWORD_ACTION_KEEP, );
+if (err) {
+goto out;
+}
}
-qmp_set_password(proto, password, !!connected, conn, );
+qmp_set_password(, );
+
+out:
+qapi_free_SetPasswordOptions();
hmp_handle_error(mon, err);
}
@@ -1471,13 +1492,31 @@ void hmp_expire_password(Monitor *mon, const QDict
*qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *whenstr = qdict_get_
[PATCH v5 1/4] monitor/hmp: add support for flag argument with value
Adds support for the "-xS" parameter type, where "-x" denotes a flag
name and the "S" suffix indicates that this flag is supposed to take an
arbitrary string parameter.
These parameters are always optional, the entry in the qdict will be
omitted if the flag is not given.
Reviewed-by: Eric Blake
Signed-off-by: Stefan Reiter
---
monitor/hmp.c | 17 -
monitor/monitor-internal.h | 3 ++-
2 files changed, 18 insertions(+), 2 deletions(-)
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), );
+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);
}
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 9c3a09cb01..c848ba5bcf 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -63,7 +63,8 @@
* '.' other form of optional type (for 'i' and 'l')
* 'b' boolean
* user mode accepts "on" or "off"
- * '-' optional parameter (eg. '-f')
+ * '-' optional parameter (eg. '-f'); if followed by an 'S', it
+ * specifies an optional string param (e.g. '-fS' allows '-f foo')
*
*/
--
2.30.2
[PATCH v5 0/4] VNC-related HMP/QMP fixes
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. v4 -> v5: * add comment to patch 1 in "monitor-internal.h" - I left Eric's R-b since the code is the same, hope that's ok * 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 qemu: Stefan Reiter (4): monitor/hmp: add support for flag argument with value qapi/monitor: refactor set/expire_password with enums qapi/monitor: allow VNC display id in set/expire_password qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate hmp-commands.hx| 24 +++--- monitor/hmp-cmds.c | 56 - monitor/hmp.c | 17 +++- monitor/monitor-internal.h | 3 +- monitor/qmp-cmds.c | 62 ++- qapi/ui.json | 156 - 6 files changed, 244 insertions(+), 74 deletions(-) -- 2.30.2
[PATCH v5 2/4] qapi/monitor: refactor set/expire_password with enums
'protocol' and 'connected' are better suited as enums than as strings,
make use of that. No functional change intended.
Suggested-by: Markus Armbruster
Signed-off-by: Stefan Reiter
---
monitor/hmp-cmds.c | 17 +++--
monitor/qmp-cmds.c | 35 ++-
qapi/ui.json | 37 +++--
3 files changed, 60 insertions(+), 29 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index bcaa41350e..0ff18ebb3c 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1453,8 +1453,17 @@ 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, );
+proto = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
+if (!!connected) {
+conn = qapi_enum_parse(_lookup, connected,
+ SET_PASSWORD_ACTION_KEEP, );
+}
+
+qmp_set_password(proto, password, !!connected, conn, );
hmp_handle_error(mon, err);
}
@@ -1463,8 +1472,12 @@ void hmp_expire_password(Monitor *mon, const QDict
*qdict)
const char *protocol = qdict_get_str(qdict, "protocol");
const char *whenstr = qdict_get_str(qdict, "time");
Error *err = NULL;
+DisplayProtocol proto;
-qmp_expire_password(protocol, whenstr, );
+proto = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
+
+qmp_expire_password(proto, whenstr, );
hmp_handle_error(mon, err);
}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5c0d5e116b..f1746a31fb 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -163,33 +163,26 @@ 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 if (protocol == DISPLAY_PROTOCOL_VNC) {
if (fail_if_connected || disconnect_if_connected) {
/* vnc supports "connected=keep" only */
error_setg(errp, QERR_INVALID_PARAMETER, "connected");
@@ -198,10 +191,6 @@ void qmp_set_password(const char *protocol, const char
*password,
/* Note that setting an empty password will not disable login through
* this interface. */
rc = vnc_display_password(NULL, password);
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
- "'vnc' or 'spice'");
-return;
}
if (rc != 0) {
@@ -209,7 +198,7 @@ void qmp_set_password(const char *protocol, const char
*password,
}
}
-void qmp_expire_password(const char *protocol, const char *whenstr,
+void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
Error **errp)
{
time_t when;
@@ -225,17 +214,13 @@ 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) {
+} else if (protocol == DISPLAY_PROTOCOL_VNC) {
rc = vnc_display_pw_expire(NULL, when);
-} else {
-error_setg(errp, Q
[PATCH v5 4/4] qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate
VNC only supports 'keep' here, enforce this via a seperate
SetPasswordActionVnc enum and mark the option 'deprecated' (as it is
useless with only one value possible).
Suggested-by: Eric Blake
Signed-off-by: Stefan Reiter
---
monitor/qmp-cmds.c | 5 -
qapi/ui.json | 21 -
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index b0630d00ab..cb229c01f8 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -182,11 +182,6 @@ void qmp_set_password(SetPasswordOptions *opts, Error
**errp)
rc = qemu_spice.set_passwd(opts->password, fail_if_connected,
disconnect_if_connected);
} else if (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(
diff --git a/qapi/ui.json b/qapi/ui.json
index 17939d0dda..70410bb633 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -38,6 +38,20 @@
{ 'enum': 'SetPasswordAction',
'data': [ 'fail', 'disconnect', 'keep' ] }
+##
+# @SetPasswordActionVnc:
+#
+# See @SetPasswordAction. VNC only supports the keep action. 'connection'
+# should just be omitted for VNC, this is kept for backwards compatibility.
+#
+# @keep: maintain existing clients
+#
+# Since: 6.2
+#
+##
+{ 'enum': 'SetPasswordActionVnc',
+ 'data': [ 'keep' ] }
+
##
# @SetPasswordOptions:
#
@@ -83,12 +97,17 @@
# @connected: How to handle existing clients when changing the
# password.
#
+# Features:
+# @deprecated: For VNC, @connected will always be 'keep', parameter should be
+# omitted.
+#
# Since: 6.2
#
##
{ 'struct': 'SetPasswordOptionsVnc',
'data': { '*display': 'str',
-'*connected': 'SetPasswordAction' }}
+'*connected': { 'type': 'SetPasswordActionVnc',
+'features': ['deprecated'] } } }
##
# @set_password:
--
2.30.2
Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id
On 10/14/21 9:14 AM, Markus Armbruster wrote:
Stefan Reiter writes:
On 10/12/21 11:24 AM, Markus Armbruster wrote:
Stefan Reiter writes:
It is possible to specify more than one VNC server on the command line,
either with an explicit ID or the auto-generated ones à "default",
"vnc2", "vnc3", ...
It is not possible to change the password on one of these extra VNC
displays though. Fix this by adding a "display" parameter to the
"set_password" and "expire_password" QMP and HMP commands.
For HMP, the display is specified using the "-d" value flag.
For QMP, the schema is updated to explicitly express the supported
variants of the commands with protocol-discriminated unions.
Suggested-by: Eric Blake
Suggested-by: Markus Armbruster
Signed-off-by: Stefan Reiter
[...]
[...]
Let me describe what you're doing in my own words, to make sure I got
both the what and the why:
1. Change @protocol and @connected from str to enum.
2. Turn the argument struct into a union tagged by @protocol, to enable
3. and 4.
3. Add argument @display in branch 'vnc'.
4. Use a separate enum for @connected in each union branch, to enable 4.
5. Trim this enum in branch 'vnc' to the values that are actually
supported. Note that just one value remains, i.e. @connected is now
an optional argument that can take only the default value.
6. Since such an argument is pointless, deprecate @connected in branch
'vnc'.
Correct?
Yes.
Thanks!
Ignorant question: is it possible to have more than one 'spice' display?
Not in terms of passwords anyway. So only one SPICE password can be set at
any time, i.e. the 'display' parameter in this function does not apply.
Item 5. is not a compatibility break: before your patch,
qmp_set_password() rejects the values you drop.
Yes.
Item 5. adds another enum to the schema in return for more accurate
introspection and slightly simpler qmp_set_password(). I'm not sure
that's worthwhile. I think we could use the same enum for both union
branches.
I tried to avoid mixing manual parsing and declarative QAPI stuff as much
as I could, so I moved as much as possible to QAPI. I personally like the
extra documentation of having the separate enum.
It's a valid choice. I'm just pointing out another valid choice. The
difference between them will go away when we remove deprecated
@connected. You choose :)
I'd split this patch into three parts: item 1., 2.+3., 4.-6., because
each part can stand on its own.
The reason why I didn't do that initially is more to do with the C side.
I think splitting it up as you describe would make for some awkward diffs
on the qmp_set_password/hmp_set_password side.
I can try of course.
It's a suggestion, not a demand. I'm a compulsory patch splitter.
I'll just have a go and see what falls out. I do agree that this patch is a
bit long on its own.
Though I also want to have it said that I'm not a fan
of how the HMP functions have to expand so much to accommodate the QAPI
structure in general.
Care to elaborate?
Well, before this patch 'hmp_set_password' was for all intents and purposes a
single function call to 'qmp_set_password'. Now it has a bunch of parameter
parsing and even validation (e.g. enum parsing). That's why I asked in the
v3 patch (I think?) if there was a way to call the QAPI style parsing from
there, since the parameters are all named the same and in a qdict already..
Something like:
void hmp_set_password(Monitor *mon, const QDict *qdict)
{
ExpirePasswordOptions opts = qapi_magical_parse_fn(qdict);
qmp_set_password(,·);
[error handling]
}
That being said, I don't mind the current form enough to make this a bigger
discussion either, so if there isn't an easy way to do the above, let's just
leave it like it is.
Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id
On 10/12/21 11:24 AM, Markus Armbruster wrote:
Stefan Reiter writes:
It is possible to specify more than one VNC server on the command line,
either with an explicit ID or the auto-generated ones à la "default",
"vnc2", "vnc3", ...
It is not possible to change the password on one of these extra VNC
displays though. Fix this by adding a "display" parameter to the
"set_password" and "expire_password" QMP and HMP commands.
For HMP, the display is specified using the "-d" value flag.
For QMP, the schema is updated to explicitly express the supported
variants of the commands with protocol-discriminated unions.
Suggested-by: Eric Blake
Suggested-by: Markus Armbruster
Signed-off-by: Stefan Reiter
[...]
diff --git a/qapi/ui.json b/qapi/ui.json
index d7567ac866..4244c62c30 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -9,22 +9,23 @@
{ 'include': 'common.json' }
{ 'include': 'sockets.json' }
+##
+# @DisplayProtocol:
+#
+# Display protocols which support changing password options.
+#
+# Since: 6.2
+#
+##
+{ 'enum': 'DisplayProtocol',
+ 'data': [ { 'name': 'vnc', 'if': 'CONFIG_VNC' },
+{ 'name': 'spice', 'if': 'CONFIG_SPICE' } ] }
+
##
# @set_password:
#
# Sets the password of a remote display session.
#
-# @protocol: - 'vnc' to modify the VNC server password
-#- 'spice' to modify the Spice server password
-#
-# @password: the new password
-#
-# @connected: how to handle existing clients when changing the
-# password. If nothing is specified, defaults to 'keep'
-# 'fail' to fail the command if clients are connected
-# 'disconnect' to disconnect existing clients
-# 'keep' to maintain existing clients
-#
# Returns: - Nothing on success
# - If Spice is not enabled, DeviceNotFound
#
@@ -37,33 +38,106 @@
# <- { "return": {} }
#
##
-{ 'command': 'set_password',
- 'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
+{ 'command': 'set_password', 'boxed': true, 'data': 'SetPasswordOptions' }
+
+##
+# @SetPasswordOptions:
+#
+# Data required to set a new password on a display server protocol.
+#
+# @protocol: - 'vnc' to modify the VNC server password
+#- 'spice' to modify the Spice server password
+#
+# @password: the new password
+#
+# Since: 6.2
+#
+##
+{ 'union': 'SetPasswordOptions',
+ 'base': { 'protocol': 'DisplayProtocol',
+'password': 'str' },
+ 'discriminator': 'protocol',
+ 'data': { 'vnc': 'SetPasswordOptionsVnc',
+'spice': 'SetPasswordOptionsSpice' } }
+
+##
+# @SetPasswordAction:
+#
+# An action to take on changing a password on a connection with active clients.
+#
+# @fail: fail the command if clients are connected
+#
+# @disconnect: disconnect existing clients
+#
+# @keep: maintain existing clients
+#
+# Since: 6.2
+#
+##
+{ 'enum': 'SetPasswordAction',
+ 'data': [ 'fail', 'disconnect', 'keep' ] }
+
+##
+# @SetPasswordActionVnc:
+#
+# See @SetPasswordAction. VNC only supports the keep action. 'connection'
+# should just be omitted for VNC, this is kept for backwards compatibility.
+#
+# @keep: maintain existing clients
+#
+# Since: 6.2
+#
+##
+{ 'enum': 'SetPasswordActionVnc',
+ 'data': [ 'keep' ] }
+
+##
+# @SetPasswordOptionsSpice:
+#
+# Options for set_password specific to the VNC procotol.
+#
+# @connected: How to handle existing clients when changing the
+# password. If nothing is specified, defaults to 'keep'.
+#
+# Since: 6.2
+#
+##
+{ 'struct': 'SetPasswordOptionsSpice',
+ 'data': { '*connected': 'SetPasswordAction' } }
+
+##
+# @SetPasswordOptionsVnc:
+#
+# Options for set_password specific to the VNC procotol.
+#
+# @display: The id of the display where the password should be changed.
+# Defaults to the first.
+#
+# @connected: How to handle existing clients when changing the
+# password.
+#
+# Features:
+# @deprecated: For VNC, @connected will always be 'keep', parameter should be
+# omitted.
+#
+# Since: 6.2
+#
+##
+{ 'struct': 'SetPasswordOptionsVnc',
+ 'data': { '*display': 'str',
+'*connected': { 'type': 'SetPasswordActionVnc',
+'features': ['deprecated'] } } }
Let me describe what you're doing in my own words, to make sure I got
both the what and the why:
1. Change @protocol and @connected from str to enum.
2. Turn the argument struct into a union tagged by @protocol, to enable
3. and 4.
3. Add argument @display in branch 'vnc'.
4. Use a separate enum for @connected in each union branch, to enable 4.
5. Trim this enum in branch 'vnc' to the values that are actually
supported. Note that just one value remains, i.e. @connected is now
an optional argument that can take only the default value.
6. Since such an argument is pointless, deprecate @connected in branch
'vnc'.
Correct?
Yes.
Ignorant question: is it
Re: [PATCH] monitor/qmp: fix race with clients disconnecting early
On 10/12/21 11:27 AM, Markus Armbruster wrote:
Stefan, any thoughts on this?
Sorry, I didn't get to work on implementing this. Idea 3 does seem very
reasonable, though I suppose the question is what all should go into the
per-session state, and also how it is passed down.
We did roll out my initial patch to our users in the meantime and got
some positive feedback (that specific error disappeared), however another
one (reported at the same time) still exists, so I was trying to diagnose
- it might also turn out to be monitor related and resolved by the more
thorough fix proposed here, but unsure.
Markus Armbruster writes:
I've thought a bit more.
A monitor can serve a series of clients.
Back when all of the monitor ran in the main thread, we completely
finished serving the current client before we started serving the next
one (I think). In other words, sessions did not overlap.
Since we moved parts of the monitor to the monitor I/O thread (merge
commit 4bdc24fa018), sessions can overlap, and this causes issues, as
you demonstrated.
Possible fixes:
1. Go back to "don't overlap" with suitable synchronization.
I'm afraid this won't cut it, because exec-oob would have wait in
line behind reconnect.
It currently waits for other reasons, but that feels fixable. Going
back to "don't overlap" would make it unfixable.
2. Make the lingering session not affect / be affected by the new
session's state
This is what your patch tries. Every access of session state needs
to be guarded like
if (conn_nr_before == qatomic_read(>connection_nr)) {
access session state
} else {
???
}
Issues:
* We have to find and guard all existing accesses. That's work.
* We have to guard all future accesses. More work, and easy to
forget, which makes this fix rather brittle.
* The fix is spread over many places.
* We may run into cases where the ??? part gets complicated.
Consider file descriptors. The command in flight will have its
monitor_get_fd() fail after disconnect. Probably okay, because it
can also fail for other reasons. But we might run into cases where
the ??? part creates new failure modes for us to handle.
3. Per-session state
Move per-session state from monitor state into a separate object.
Use reference counts to keep this object alive until both threads are
done with the session.
Monitor I/O thread executes monitor core and the out-of-band
commands; its stops using the old session on disconnect, and starts
using the new session on connect.
Main thread executes in-band commands, and these use the session that
submitted them.
Do I make sense, or should I explain my idea in more detail?
[PATCH v4 1/2] monitor/hmp: add support for flag argument with value
Adds support for the "-xS" parameter type, where "-x" denotes a flag
name and the "S" suffix indicates that this flag is supposed to take an
arbitrary string parameter.
These parameters are always optional, the entry in the qdict will be
omitted if the flag is not given.
Reviewed-by: Eric Blake
Signed-off-by: Stefan Reiter
---
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), );
+if (ret < 0) {
+monitor_printf(mon, "%s: value expected for -%c\n",
+ cmd->name, *tmp);
+goto fail;
+}
+qdict_put_str(qdict, key, buf);
} else {
-/* has option */
+/* has boolean option */
p++;
qdict_put_bool(qdict, key, true);
}
--
2.30.2
[PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id
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
Suggested-by: Markus Armbruster
Signed-off-by: Stefan Reiter
---
hmp-commands.hx| 24 ---
monitor/hmp-cmds.c | 57 ++-
monitor/qmp-cmds.c | 62 ++--
qapi/ui.json | 173 +
4 files changed, 235 insertions(+), 81 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index cf723c69ac..d78e4cfc47 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1514,33 +1514,35 @@ ERST
{
.name = "set_password",
-.args_type = "protocol:s,password:s,connected:s?",
-.params = "protocol password action-if-connected",
+.args_type = "protocol:s,password:s,display:-dS,connected:s?",
+.params = "protocol password [-d display] [action-if-connected]",
.help = "set spice/vnc password",
.cmd= hmp_set_password,
},
SRST
-``set_password [ vnc | spice ] password [ action-if-connected ]``
- Change spice/vnc password. *action-if-connected* specifies what
- should happen in case a connection is established: *fail* makes the
- password change fail. *disconnect* changes the password and
+``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected
]``
+ Change spice/vnc password. *display* can be used with 'vnc' to specify
+ which display to set the password on. *action-if-connected* specifies
+ what should happen in case a connection is established: *fail* makes
+ the password change fail. *disconnect* changes the password and
disconnects the client. *keep* changes the password and keeps the
connection up. *keep* is the default.
ERST
{
.name = "expire_password",
-.args_type = "protocol:s,time:s",
-.params = "protocol time",
+.args_type = "protocol:s,time:s,display:-dS",
+.params = "protocol time [-d display]",
.help = "set spice/vnc password expire-time",
.cmd= hmp_expire_password,
},
SRST
-``expire_password [ vnc | spice ]`` *expire-time*
- Specify when a password for spice/vnc becomes
- invalid. *expire-time* accepts:
+``expire_password [ vnc | spice ] expire-time [ -d display ]``
+ Specify when a password for spice/vnc becomes invalid.
+ *display* behaves the same as in ``set_password``.
+ *expire-time* accepts:
``now``
Invalidate password instantly.
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b5e71d9e6f..48b3fe4519 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,10 +1451,41 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *password = qdict_get_str(qdict, "password");
+const char *display = qdict_get_try_str(qdict, "display");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
+DisplayProtocol proto;
-qmp_set_password(protocol, password, !!connected, connected, );
+SetPasswordOptions opts = {
+.password = g_strdup(password),
+.u.vnc.display = NULL,
+};
+
+proto = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
+if (err) {
+hmp_handle_error(mon, err);
+return;
+}
+opts.protocol = proto;
+
+if (proto == DISPLAY_PROTOCOL_VNC) {
+opts.u.vnc.has_display = !!display;
+opts.u.vnc.display = g_strdup(display);
+} else if (proto == DISPLAY_PROTOCOL_SPICE) {
+opts.u.spice.has_connected = !!connected;
+opts.u.spice.connected =
+qapi_enum_parse(_lookup, connected,
+SET_PASSWORD_ACTION_KEEP, );
+if (err) {
+hmp_handle_error(mon, err);
+return;
+}
+}
+
+qmp_set_password(, );
+g_free(opts.password);
+g_free(opts.u.vnc.display);
hmp_handle_error(mon, err);
}
@@ -1462,9 +1493,31 @@ void hmp_expire_password(Monitor *mon, const QDict
*qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *whenstr = qdict_get_str(qdict, "time");
+const char *
[PATCH v4] VNC-related HMP/QMP fixes
Since the removal of the generic 'qmp_change' command, one can no longer replace the 'default' VNC display listen address at runtime (AFAIK). For our users who need to set up a secondary VNC access port, this means configuring a second VNC display (in addition to our standard one for web-access), but it turns out one cannot set a password on this second display at the moment, as the 'set_password' call only operates on the 'default' display. Additionally, using secret objects, the password is only read once at startup. This could be considered a bug too, but is not touched in this series and left for a later date. v3 -> v4: * drop previously patch 1, this was fixed here instead: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02529.html * patch 1: add Eric's R-b * patch 2: remove if-assignment, use 'deprecated' feature in schema v2 -> v3: * refactor QMP schema for set/expire_password as suggested by Eric Blake and Markus Armbruster v1 -> v2: * add Marc-André's R-b on patch 1 * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann * I didn't see a way to do this yet, so I added a "flags with values" arg type Stefan Reiter (2): monitor/hmp: add support for flag argument with value monitor: refactor set/expire_password and allow VNC display id hmp-commands.hx| 24 --- monitor/hmp-cmds.c | 57 ++- monitor/hmp.c | 17 - monitor/qmp-cmds.c | 62 ++-- qapi/ui.json | 173 + 5 files changed, 251 insertions(+), 82 deletions(-) -- 2.30.2
[PATCH v3 1/3] monitor/hmp: correctly invert password argument detection again
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
Signed-off-by: Stefan Reiter
---
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
[PATCH v3 3/3] monitor: refactor set/expire_password and allow VNC display id
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
Suggested-by: Markus Armbruster
Signed-off-by: Stefan Reiter
---
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, );
+SetPasswordOptions opts = {
+.password = g_strdup(password),
+.u.vnc.display = NULL,
+};
+
+proto = qapi_enum_parse(_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, );
+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(_lookup, connected,
+SET_PASSWORD_
[PATCH v3 0/3] VNC-related HMP/QMP fixes
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
[PATCH v3 2/3] monitor/hmp: add support for flag argument with value
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
---
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), );
+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
Re: [PATCH v3] qemu-sockets: fix unix socket path copy (again)
Thanks for the patch, ran into the same issue here and was
about to send my own ;)
On 9/1/21 3:16 PM, Michael Tokarev wrote:
Commit 4cfd970ec188558daa6214f26203fe553fb1e01f added an
assert which ensures the path within an address of a unix
socket returned from the kernel is at least one byte and
does not exceed sun_path buffer. Both of this constraints
are wrong:
A unix socket can be unnamed, in this case the path is
completely empty (not even \0)
And some implementations (notable linux) can add extra
trailing byte (\0) _after_ the sun_path buffer if we
passed buffer larger than it (and we do).
So remove the assertion (since it causes real-life breakage)
but at the same time fix the usage of sun_path. Namely,
we should not access sun_path[0] if kernel did not return
it at all (this is the case for unnamed sockets),
and use the returned salen when copyig actual path as an
upper constraint for the amount of bytes to copy - this
will ensure we wont exceed the information provided by
the kernel, regardless whenever there is a trailing \0
or not. This also helps with unnamed sockets.
Note the case of abstract socket, the sun_path is actually
a blob and can contain \0 characters, - it should not be
passed to g_strndup and the like, it should be accessed by
memcpy-like functions.
I know this is already applied now, but I noticed that you
don't actually follow through on that part - g_strndup is
still used in both cases for sun_path.
Haven't run into a bug with this, just noting that the
commit message is a bit confusing paired with the patch.
Might be a bit tricky though, as all callers would need to
be careful too...
Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f
Fixes: http://bugs.debian.org/993145
Signed-off-by: Michael Tokarev
CC: qemu-sta...@nongnu.org
---
util/qemu-sockets.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index f2f3676d1f..c5043999e9 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1345,25 +1345,22 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage
*sa,
SocketAddress *addr;
struct sockaddr_un *su = (struct sockaddr_un *)sa;
-assert(salen >= sizeof(su->sun_family) + 1 &&
- salen <= sizeof(struct sockaddr_un));
-
addr = g_new0(SocketAddress, 1);
addr->type = SOCKET_ADDRESS_TYPE_UNIX;
+salen -= offsetof(struct sockaddr_un, sun_path);
#ifdef CONFIG_LINUX
-if (!su->sun_path[0]) {
+if (salen > 0 && !su->sun_path[0]) {
/* Linux abstract socket */
-addr->u.q_unix.path = g_strndup(su->sun_path + 1,
-salen - sizeof(su->sun_family) - 1);
+addr->u.q_unix.path = g_strndup(su->sun_path + 1, salen - 1);
addr->u.q_unix.has_abstract = true;
addr->u.q_unix.abstract = true;
addr->u.q_unix.has_tight = true;
-addr->u.q_unix.tight = salen < sizeof(*su);
+addr->u.q_unix.tight = salen < sizeof(su->sun_path);
return addr;
}
#endif
-addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path));
+addr->u.q_unix.path = g_strndup(su->sun_path, salen);
return addr;
}
#endif /* WIN32 */
[PATCH v2 0/3] VNC-related HMP/QMP fixes
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. 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: allow VNC related QMP and HMP commands to take a display ID hmp-commands.hx| 29 +++-- monitor/hmp-cmds.c | 9 ++--- monitor/hmp.c | 17 - monitor/qmp-cmds.c | 9 + qapi/ui.json | 12 ++-- 5 files changed, 52 insertions(+), 24 deletions(-) -- 2.30.2
[PATCH v2 2/3] monitor/hmp: add support for flag argument with value
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
---
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), );
+if (ret < 0) {
+monitor_printf(mon, "%s: value expected for -%c\n",
+ cmd->name, *tmp);
+goto fail;
+}
+qdict_put_str(qdict, key, buf);
} else {
-/* has option */
+/* has boolean option */
p++;
qdict_put_bool(qdict, key, true);
}
--
2.30.2
[PATCH v2 3/3] monitor: allow VNC related QMP and HMP commands to take a display ID
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.
Signed-off-by: Stefan Reiter
---
hmp-commands.hx| 29 +++--
monitor/hmp-cmds.c | 7 +--
monitor/qmp-cmds.c | 9 +
qapi/ui.json | 12 ++--
4 files changed, 35 insertions(+), 22 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..168ca62371 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,10 +1451,12 @@ 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;
-qmp_set_password(protocol, password, !!connected, connected, );
+qmp_set_password(protocol, password, !!connected, connected, !!display,
+ display, );
hmp_handle_error(mon, err);
}
@@ -1462,9 +1464,10 @@ 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;
-qmp_expire_password(protocol, whenstr, );
+qmp_expire_password(protocol, whenstr, !!display, display, );
hmp_handle_error(mon, err);
}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5c0d5e116b..b53869d10c 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -164,7 +164,8 @@ void qmp_system_wakeup(Error **errp)
}
void qmp_set_password(const char *protocol, const char *password,
- bool has_connected, const char *connected, Error **errp)
+ bool has_connected, const char *connected,
+ bool has_display, const char *display, Error **errp)
{
int disconnect_if_connected = 0;
int fail_if_connected = 0;
@@ -197,7 +198,7 @@ void qmp_set_password(const char *protocol, const cha
[PATCH v2 1/3] monitor/hmp: correctly invert password argument detection again
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
Signed-off-by: Stefan Reiter
---
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
Re: [PATCH] monitor/qmp: fix race with clients disconnecting early
On 26/08/2021 15:50, Markus Armbruster wrote: Markus Armbruster writes: [...] Let me re-explain the bug in my own words, to make sure I understand. A QMP monitor normally runs in the monitor I/O thread. A QMP monitor can serve only one client at a time. It executes out-of-band commands right as it reads them. In-band commands are queued, and executed one after the other in the main loop. Command output is buffered. We write it out as fast as the character device can take it. If a write fails, we throw away the entire buffer contents. A client can disconnect at any time. This throws away the queue. An in-band command may be executing in the main loop. An out-of-band command may be executing in the monitor's thread. Such commands (if any) are not affected by the disconnect. Their output gets buffered, but write out fails, so it's thrown away. *Except* when another client connects quickly enough. Then we send it output meant for the previous client. This is wrong. I suspect this could even send invalid JSON. I'm not sure this is the case. In all testing I have *never* encountered the case of broken JS or any other indication that partial output was received. I think the issue is just between starting to execute the command in the BH and the new client connecting... can the CHR_EVENTs even be triggered when the main thread is busy with the BH? Special case: if in-band command qmp_capabilities is executing when the client disconnects, and another client connects before the command flips the monitor from capabilities negotiation mode to command mode, that client starts in the wrong mode. What the cases have in common: disconnect + connect in monitor I/O thread and the command executing in the main thread change the same monitor state. You observed two issues: one involving the output buffer (new client receives old client's output), and one involving monitor mode (new client has its mode flipped by the old client's qmp_capabilities command). Any monitor state accessed by commands could cause issues. Right now, I can see only one more: file descriptors. Cleaning them up on disconnect could mess with the command. Right, that would make sense, but also only an issue if the reconnect can happen in the middle of the command itself. Maybe we can acquire some kind of lock during actual in-band QMP command execution? [...]
Re: [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID
On 8/25/21 12:59 PM, Marc-André Lureau wrote:
Hi
On Wed, Aug 25, 2021 at 1:39 PM 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, this is a bit trickier, since at least 'set_password' already
has the 'connected' parameter following the mandatory 'password' one, so
we need to prefix the display ID with "id=" to allow correct parsing.
It's not something done with other commands afaik, feels a bit awkward (the
"connected = display"...).
Indeed, if there is a better way I'd love to use it.
One idea I had was making the parameter 'connected' OR 'display', since
the former only supports 'keep' for VNC anyway - but that introduces a
weird double-meaning again.
Is it really necessary to add support for HMP?
For us it would be, as we provide an easy HMP interface to our users, but
not a QMP one, so it ended up being a bit of a regression with 6.0.
With this prefix, no existing command or workflow should be affected.
While rewriting the descriptions, also remove the line "Use zero to make
the password stay valid forever." from 'set_password', I believe this was
intended for 'expire_password', but would even be wrong there.
Signed-off-by: Stefan Reiter
---
hmp-commands.hx| 28 +++-
monitor/hmp-cmds.c | 20 ++--
monitor/qmp-cmds.c | 9 +
qapi/ui.json | 12 ++--
4 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index e01ca13ca8..0b5abcfb8a 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1541,34 +1541,36 @@ 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:s?,connected:s?",
+.params = "protocol password [id=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
+``set_password [ vnc | spice ] password [ id=display ] [
action-if-connected ]``
+ Change spice/vnc password. *display* (must be prefixed with
+ 'id=') 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.
+ 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:s?",
+.params = "protocol time [id=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 [ id=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 31366e6331..30f5b2c3e3 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1546,10 +1546,20 @@ 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;
-qmp_set_password(protocol, password, !!connected, connected, );
+if (display && strncmp(display, "id=", 3)) {
+connected = display;
+display = NULL;
+} else if (display) {
+/* skip "id=" */
+disp
[PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID
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, this is a bit trickier, since at least 'set_password' already
has the 'connected' parameter following the mandatory 'password' one, so
we need to prefix the display ID with "id=" to allow correct parsing.
With this prefix, no existing command or workflow should be affected.
While rewriting the descriptions, also remove the line "Use zero to make
the password stay valid forever." from 'set_password', I believe this was
intended for 'expire_password', but would even be wrong there.
Signed-off-by: Stefan Reiter
---
hmp-commands.hx| 28 +++-
monitor/hmp-cmds.c | 20 ++--
monitor/qmp-cmds.c | 9 +
qapi/ui.json | 12 ++--
4 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index e01ca13ca8..0b5abcfb8a 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1541,34 +1541,36 @@ 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:s?,connected:s?",
+.params = "protocol password [id=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
+``set_password [ vnc | spice ] password [ id=display ] [ action-if-connected
]``
+ Change spice/vnc password. *display* (must be prefixed with
+ 'id=') 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.
+ 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:s?",
+.params = "protocol time [id=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 [ id=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 31366e6331..30f5b2c3e3 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1546,10 +1546,20 @@ 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;
-qmp_set_password(protocol, password, !!connected, connected, );
+if (display && strncmp(display, "id=", 3)) {
+connected = display;
+display = NULL;
+} else if (display) {
+/* skip "id=" */
+display = display + 3;
+}
+
+qmp_set_password(protocol, password, !!connected, connected, !!display,
+ display, );
hmp_handle_error(mon, err);
}
@@ -1557,9 +1567,15 @@ 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;
-qmp_expire_password(protocol, whenstr, );
+if (display && !strncmp(display, "id=", 3)) {
+/* skip "id=" */
+
[PATCH 0/2] VNC-related HMP/QMP fixes
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. Stefan Reiter (2): monitor/hmp: correctly invert password argument detection again monitor: allow VNC related QMP and HMP commands to take a display ID hmp-commands.hx| 28 +++- monitor/hmp-cmds.c | 22 +++--- monitor/qmp-cmds.c | 9 + qapi/ui.json | 12 ++-- 4 files changed, 49 insertions(+), 22 deletions(-) -- 2.30.2
[PATCH 1/2] monitor/hmp: correctly invert password argument detection again
Commit cfb5387a1d 'hmp: remove "change vnc TARGET" command' claims to
remove the HMP "change vnc" command, but doesn't actually do that.
Instead if 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")
Signed-off-by: Stefan Reiter
---
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 f7a211e5a4..31366e6331 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1591,7 +1591,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
[PATCH] monitor/qmp: fix race with clients disconnecting early
From: Stefan Reiter
The following sequence can produce a race condition that results in
responses meant for different clients being sent to the wrong one:
(QMP, no OOB)
1) client A connects
2) client A sends 'qmp_capabilities'
3) 'qmp_dispatch' runs in coroutine, schedules out to
'do_qmp_dispatch_bh' and yields
4) client A disconnects (i.e. aborts, crashes, etc...)
5) client B connects
6) 'do_qmp_dispatch_bh' runs 'qmp_capabilities' and wakes calling coroutine
7) capabilities are now set and 'mon->commands' is set to '_commands'
8) 'qmp_dispatch' returns to 'monitor_qmp_dispatch'
9) success message is sent to client B *without it ever having sent
'qmp_capabilities' itself*
9a) even if client B ignores it, it will now presumably send it's own
greeting, which will error because caps are already set
The fix proposed here uses an atomic, sequential connection number
stored in the MonitorQMP struct, which is incremented every time a new
client connects. Since it is not changed on CHR_EVENT_CLOSED, the
behaviour of allowing a client to disconnect only one side of the
connection is retained.
The connection_nr needs to be exposed outside of the monitor subsystem,
since qmp_dispatch lives in qapi code. It needs to be checked twice,
once for actually running the command in the BH (fixes 7/9a), and once for
sending back a response (fixes 9).
This satisfies my local reproducer - using multiple clients constantly
looping to open a connection, send the greeting, then exiting no longer
crashes other, normally behaving clients with unrelated responses.
Signed-off-by: Stefan Reiter
---
This is another patch in the apparently never-ending series of fixes to
QMP-related race conditions. Our PVE users really seem to have a knack for
triggering these ;)
Here's one of the (several) forum threads where we tried to diagnose the issue:
https://forum.proxmox.com/threads/error-with-backup-when-backing-up-qmp-command-query-backup-failed-got-wrong-command-id.88017/
It manifested itself under load, where sometimes our monitor code (which uses
the 'id' system of QMP to track commands) would receive bogus responses, showing
up as "got wrong command id" errors in our logs.
I'm not sure this approach is the best fix, it seems a lot like a band-aid to
me, but it's the best I could come up with for now - open for different ideas of
course.
Note that with this patch, we're no longer running a command that was scheduled
after a client has disconnected early. This seems like a slight behaviour change
to me... On the other hand, I didn't want to drag the connection number into
qmp_capabilities() and special case that even further.
I didn't look to deeply into 'QMP in iothread' yet, if that does what I think it
does there might be two more potential races here:
* between 'do_qmp_dispatch_bh' running 'aio_co_wake' and 'qmp_dispatch' actually
yielding (very unlikely though)
* plus a TOCTOU in 'do_qmp_dispatch_bh' with the atomic read of the
connection_nr and actually running cmd->fn()
Thanks!
include/monitor/monitor.h | 1 +
monitor/monitor-internal.h | 7 +++
monitor/monitor.c | 15 +++
monitor/qmp.c | 15 ++-
qapi/qmp-dispatch.c| 21 +
stubs/monitor-core.c | 5 +
6 files changed, 59 insertions(+), 5 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index af3887bb71..ff6db1448b 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -16,6 +16,7 @@ extern QemuOptsList qemu_mon_opts;
Monitor *monitor_cur(void);
Monitor *monitor_set_cur(Coroutine *co, Monitor *mon);
bool monitor_cur_is_qmp(void);
+int monitor_get_connection_nr(const Monitor *mon);
void monitor_init_globals(void);
void monitor_init_globals_core(void);
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 9c3a09cb01..a92be8c3f7 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -144,6 +144,13 @@ typedef struct {
QemuMutex qmp_queue_lock;
/* Input queue that holds all the parsed QMP requests */
GQueue *qmp_requests;
+
+/*
+ * A sequential number that gets incremented on every new CHR_EVENT_OPENED.
+ * Used to avoid leftover responses in BHs from being sent to the wrong
+ * client. Access with atomics.
+ */
+int connection_nr;
} MonitorQMP;
/**
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 636bcc81c5..ee5ac26c37 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -136,6 +136,21 @@ bool monitor_cur_is_qmp(void)
return cur_mon && monitor_is_qmp(cur_mon);
}
+/**
+ * If @mon is a QMP monitor, return the connection_nr, otherwise -1.
+ */
+int monitor_get_connection_nr(const Monitor *mon)
+{
+MonitorQMP *qmp_mon;
+
+if (!monitor_is_qmp(mon)) {
+return -1;
+}
+
+qmp_mon = container_of(mon, MonitorQMP, common);
+return qatomic_read(_mon->connection
Re: [PATCH] monitor/qmp: fix race on CHR_EVENT_CLOSED without OOB
On 3/26/21 3:48 PM, Markus Armbruster wrote: Wolfgang Bumiller writes: On Thu, Mar 18, 2021 at 02:35:50PM +0100, Stefan Reiter wrote: If OOB is disabled, events received in monitor_qmp_event will be handled in the main context. Thus, we must not acquire a qmp_queue_lock there, as the dispatcher coroutine holds one over a yield point, where it expects to be rescheduled from the main context. If a CHR_EVENT_CLOSED event is received just then, it can race and block the main thread by waiting on the queue lock. Run monitor_qmp_cleanup_queue_and_resume in a BH on the iohandler thread, so the main thread can always make progress during the reschedule. The delaying of the cleanup is safe, since the dispatcher always moves back to the iothread afterward, and thus the cleanup will happen before it gets to its next iteration. Signed-off-by: Stefan Reiter --- This is a tough one. It *may* be fine, but I wonder if we can approach this differently: You guys make my head hurt. That makes three of us, I think. I understand we're talking about a bug. Is it a recent regression, or an older bug? How badly does the bug affect users? It's a regression introduced with the coroutinization of QMP in 5.2. It only occurs when OOB is disabled - in our downstream we have it disabled unconditionally, as it caused some issues in the past. It affected quite a lot of our users, some when the host was under CPU load, some seemingly random. When it happened it usually hit multiple VMs at once, completely hanging them. Just for reference, our forum has the full story: https://forum.proxmox.com/threads/all-vms-locking-up-after-latest-pve-update.85397/ I'm about to vanish for my Easter break... If the bug must be fixed for 6.0, just waiting for me to come back seems unadvisable. Since it doesn't happen with OOB (so, by default), I don't think it's that urgent. BTW, I've sent a v2 as well: https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg07590.html That one we have shipped to our users for now, with mostly good success, though a few reports that something still hangs - which could be people failing to upgrade, or some other issue still unsolved... And happy easter break :)
[PATCH v2] monitor/qmp: fix race on CHR_EVENT_CLOSED without OOB
The QMP dispatcher coroutine holds the qmp_queue_lock over a yield
point, where it expects to be rescheduled from the main context. If a
CHR_EVENT_CLOSED event is received just then, it can race and block the
main thread on the mutex in monitor_qmp_cleanup_queue_and_resume.
monitor_resume does not need to be called from main context, so we can
call it immediately after popping a request from the queue, which allows
us to drop the qmp_queue_lock mutex before yielding.
Suggested-by: Wolfgang Bumiller
Signed-off-by: Stefan Reiter
---
v2:
* different approach: move everything that needs the qmp_queue_lock mutex before
the yield point, instead of moving the event handling to a different context
monitor/qmp.c | 40 ++--
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 2b0308f933..092c527b6f 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -257,24 +257,6 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
trace_monitor_qmp_in_band_dequeue(req_obj,
req_obj->mon->qmp_requests->length);
-if (qatomic_xchg(_dispatcher_co_busy, true) == true) {
-/*
- * Someone rescheduled us (probably because a new requests
- * came in), but we didn't actually yield. Do that now,
- * only to be immediately reentered and removed from the
- * list of scheduled coroutines.
- */
-qemu_coroutine_yield();
-}
-
-/*
- * Move the coroutine from iohandler_ctx to qemu_aio_context for
- * executing the command handler so that it can make progress if it
- * involves an AIO_WAIT_WHILE().
- */
-aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co);
-qemu_coroutine_yield();
-
/*
* @req_obj has a request, we hold req_obj->mon->qmp_queue_lock
*/
@@ -298,8 +280,30 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
monitor_resume(>common);
}
+/*
+ * Drop the queue mutex now, before yielding, otherwise we might
+ * deadlock if the main thread tries to lock it.
+ */
qemu_mutex_unlock(>qmp_queue_lock);
+if (qatomic_xchg(_dispatcher_co_busy, true) == true) {
+/*
+ * Someone rescheduled us (probably because a new requests
+ * came in), but we didn't actually yield. Do that now,
+ * only to be immediately reentered and removed from the
+ * list of scheduled coroutines.
+ */
+qemu_coroutine_yield();
+}
+
+/*
+ * Move the coroutine from iohandler_ctx to qemu_aio_context for
+ * executing the command handler so that it can make progress if it
+ * involves an AIO_WAIT_WHILE().
+ */
+aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co);
+qemu_coroutine_yield();
+
/* Process request */
if (req_obj->req) {
if (trace_event_get_state(TRACE_MONITOR_QMP_CMD_IN_BAND)) {
--
2.20.1
Re: [PATCH] monitor/qmp: fix race on CHR_EVENT_CLOSED without OOB
On 3/22/21 12:08 PM, Wolfgang Bumiller wrote:
On Thu, Mar 18, 2021 at 02:35:50PM +0100, Stefan Reiter wrote:
If OOB is disabled, events received in monitor_qmp_event will be handled
in the main context. Thus, we must not acquire a qmp_queue_lock there,
as the dispatcher coroutine holds one over a yield point, where it
expects to be rescheduled from the main context. If a CHR_EVENT_CLOSED
event is received just then, it can race and block the main thread by
waiting on the queue lock.
Run monitor_qmp_cleanup_queue_and_resume in a BH on the iohandler
thread, so the main thread can always make progress during the
reschedule.
The delaying of the cleanup is safe, since the dispatcher always moves
back to the iothread afterward, and thus the cleanup will happen before
it gets to its next iteration.
Signed-off-by: Stefan Reiter
---
This is a tough one. It *may* be fine, but I wonder if we can approach
this differently:
From what I can gather we have the following call stacks & contexts:
Guarded lock (lock & release):
* monitor_qmp_cleanup_queue_and_resume
by monitor_qmp_event
by file handler (from I/O loop)
^ iohandler_context (assuming that's where the file handling happens...)
(after this patch as BH though)
* handle_qmp_command
a) by the json parser (which is also re-initialized by
monitor_qmp_event btw., haven't checked if that can also
"trigger" its methods immediately)
b) by monitor_qmp_read
by file handler (from I/O loop)
^ iohandler_context
Lock-"returning":
* monitor_qmp_requests_pop_any_with_lock
by coroutine_fn monitor_qmp_dispatcher_co
^ iohandler_context
Lock-releasing:
* coroutine_fn monitor_qmp_dispatcher_co
^ qemu_aio_context
The only *weird* thing that immediately pops out here is
`monitor_qmp_requests_pop_any_with_lock()` keeping a lock while
switching contexts.
monitor_qmp_dispatcher_co? _pop_any_ doesn't switch contexts...
But yes, that is weird, as I mentioned in my original mail too.
This is done in order to allow `AIO_WAIT_WHILE` to work while making
progress on the events, but do we actually already need to be in this
context for the OOB `monitor_resume()` call or can we defer the context
switch to after having done that and released the lock?
`monitor_resume()` itself seems to simply schedule a BH which should
work regardless if I'm not mistaken. There's also a
`readline_show_prompt()` call, but that *looks* harmless?
The BH should indeed be harmless since we don't schedule on
qemu_get_current_aio_context, and the readline_show_prompt call we can
ignore here since it's guarded with "!monitor_is_qmp(mon)".
`monitor_resume()` is also called without the lock later on, so even if
it needs to be in this context at that point for whatever reason, does
it need the lock?
It doesn't access the queue, so I don't see why it'd need the lock. And
as you said, it currently works without too, actually, before commit
88daf0996c ("qmp: Resume OOB-enabled monitor before processing the
request") it always did so.
I'll cobble together a v2 with this in mind.
[PATCH] monitor/qmp: fix race on CHR_EVENT_CLOSED without OOB
If OOB is disabled, events received in monitor_qmp_event will be handled
in the main context. Thus, we must not acquire a qmp_queue_lock there,
as the dispatcher coroutine holds one over a yield point, where it
expects to be rescheduled from the main context. If a CHR_EVENT_CLOSED
event is received just then, it can race and block the main thread by
waiting on the queue lock.
Run monitor_qmp_cleanup_queue_and_resume in a BH on the iohandler
thread, so the main thread can always make progress during the
reschedule.
The delaying of the cleanup is safe, since the dispatcher always moves
back to the iothread afterward, and thus the cleanup will happen before
it gets to its next iteration.
Signed-off-by: Stefan Reiter
---
We've had some spurious reports of people reporting (usually multiple) total VM
hangs on our forum, and finally got our hands on some stack traces:
Thread 1 (Thread 0x7fa59d476340 (LWP 1306954)):
#0 __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:103
#1 0x7fa5a8335714 in __GI___pthread_mutex_lock
(mutex=mutex@entry=0x55722bf3a848) at ../nptl/pthread_mutex_lock.c:80
#2 0x55722a475e39 in qemu_mutex_lock_impl (mutex=0x55722bf3a848,
file=0x55722a5d7149 "../monitor/qmp.c", line=80) at
../util/qemu-thread-posix.c:79
#3 0x55722a3fb686 in monitor_qmp_cleanup_queue_and_resume
(mon=0x55722bf3a730) at ../monitor/qmp.c:80
#4 monitor_qmp_event (opaque=0x55722bf3a730, event=) at
../monitor/qmp.c:421
#5 0x55722a3f9505 in tcp_chr_disconnect_locked (chr=0x55722bc68fa0) at
../chardev/char-socket.c:507
#6 0x55722a3f9550 in tcp_chr_disconnect (chr=0x55722bc68fa0) at
../chardev/char-socket.c:517
#7 0x55722a3f959e in tcp_chr_hup (channel=, cond=, opaque=) at ../chardev/char-socket.c:557
#8 0x7fa5a9c2edd8 in g_main_context_dispatch () from
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#9 0x55722a47a848 in glib_pollfds_poll () at ../util/main-loop.c:221
#10 os_host_main_loop_wait (timeout=) at ../util/main-loop.c:244
#11 main_loop_wait (nonblocking=nonblocking@entry=0) at ../util/main-loop.c:520
#12 0x55722a2570a1 in qemu_main_loop () at ../softmmu/vl.c:1678
#13 0x557229fc178e in main (argc=, argv=,
envp=) at ../softmmu/main.c:50
I managed to reproduce it reliably by adding a 'sleep(1)' to the beginning of
handle_qmp_command, then running this:
./build/qemu-system-x86_64 \
-chardev 'socket,id=qmp,path=./qmp.socket,server=on,wait=off' \
-mon 'chardev=qmp,mode=control'
yes "query-version" | timeout 2s ./scripts/qmp/qmp-shell ./qmp.socket
When the timeout hits, the main loop always ends up in the trace above, with the
dispatch coroutine having just yielded to reschedule itself.
Holding a QemuMutex over a yield point seems pretty dangerous in general, but it
seems to be working for now...
monitor/qmp.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 2b0308f933..83eb440b29 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -74,8 +74,9 @@ static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP
*mon)
}
}
-static void monitor_qmp_cleanup_queue_and_resume(MonitorQMP *mon)
+static void monitor_qmp_cleanup_queue_and_resume(void *opaque)
{
+MonitorQMP *mon = opaque;
QEMU_LOCK_GUARD(>qmp_queue_lock);
/*
@@ -453,8 +454,18 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent
event)
* backend is still open. For example, when the backend is
* stdio, it's possible that stdout is still open when stdin
* is closed.
+ *
+ * monitor_qmp_cleanup_queue_and_resume must not run in main
+ * context, as it acquires a qmp_queue_lock, which is held by
+ * the dispatcher coroutine during a reschedule/yield to the
+ * main context, and could thus lead to a deadlock.
+ *
+ * This is safe to delay, since the dispatcher also moves
+ * back to the iohandler context before reaquiring any
+ * queue locks.
*/
-monitor_qmp_cleanup_queue_and_resume(mon);
+aio_bh_schedule_oneshot(iohandler_get_aio_context(),
+monitor_qmp_cleanup_queue_and_resume, mon);
json_message_parser_destroy(>parser);
json_message_parser_init(>parser, handle_qmp_command,
mon, NULL);
--
2.20.1
Re: Potential regression in 'qemu-img convert' to LVM
On 07/01/2021 21:03, Nir Soffer wrote: On Tue, Sep 15, 2020 at 2:51 PM Stefan Reiter wrote: On 9/15/20 11:08 AM, Nir Soffer wrote: On Mon, Sep 14, 2020 at 3:25 PM Stefan Reiter wrote: Hi list, following command fails since 5.1 (tested on kernel 5.4.60): # qemu-img convert -p -f raw -O raw /dev/zvol/pool/disk-1 /dev/vg/disk-1 qemu-img: error while writing at byte 2157968896: Device or resource busy (source is ZFS here, but doesn't matter in practice, it always fails the same; offset changes slightly but consistently hovers around 2^31) strace shows the following: fallocate(13, FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE, 2157968896, 4608) = -1 EBUSY (Device or resource busy) What is the size of the LV? Same as the source, 5GB in my test case. Created with: # lvcreate -ay --size 5242880k --name disk-1 vg Does it happen if you change sparse minimum size (-S)? For example: -S 64k qemu-img convert -p -f raw -O raw -S 64k /dev/zvol/pool/disk-1 /dev/vg/disk-1 Tried a few different values, always the same result: EBUSY at byte 2157968896. Other fallocate calls leading up to this work fine. This happens since commit edafc70c0c "qemu-img convert: Don't pre-zero images", before that all fallocates happened at the start. Reverting the commit and calling qemu-img exactly the same way on the same data works fine. But slowly, doing up to 100% more work for fully allocated images. Of course, I'm not saying the patch is wrong, reverting it just avoids triggering the bug. Simply retrying the syscall on EBUSY (like EINTR) does *not* work, once it fails it keeps failing with the same error. I couldn't find anything related to EBUSY on fallocate, and it only happens on LVM targets... Any idea or pointers where to look? Is this thin LV? No, regular LV. See command above. This works for us using regular LVs. Which kernel? which distro? Reproducible on: * PVE w/ kernel 5.4.60 (Ubuntu based) * Manjaro w/ kernel 5.8.6 I found that it does not happen with all images, I suppose there must be a certain number of smaller holes for it to happen. I am using a VM image with a bare-bones Alpine Linux installation, but it's not an isolated case, we've had two people report the issue on our bug tracker: https://bugzilla.proxmox.com/show_bug.cgi?id=3002 I think that this issue may be fixed by https://lists.nongnu.org/archive/html/qemu-block/2020-11/msg00358.html Nir Sorry for the late reply, but yes, I can confirm this fixes the issue. ~
[PATCH] migration: only check page size match if RAM postcopy is enabled
Postcopy may also be advised for dirty-bitmap migration only, in which
case the remote page size will not be available and we'll instead read
bogus data, blocking migration with a mismatch error if the VM uses
hugepages.
Fixes: 58110f0acb ("migration: split common postcopy out of ram postcopy")
Signed-off-by: Stefan Reiter
---
migration/ram.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index 7811cde643..6ace15261c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3521,7 +3521,7 @@ static int ram_load_precopy(QEMUFile *f)
}
}
/* For postcopy we need to check hugepage sizes match */
-if (postcopy_advised &&
+if (postcopy_advised && migrate_postcopy_ram() &&
block->page_size != qemu_host_page_size) {
uint64_t remote_page_size = qemu_get_be64(f);
if (remote_page_size != block->page_size) {
--
2.20.1
[PATCH] docs: don't install corresponding man page if guest agent is disabled
No sense outputting the qemu-ga and qemu-ga-ref man pages when the guest
agent binary itself is disabled. This mirrors behaviour from before the
meson switch.
Signed-off-by: Stefan Reiter
---
docs/meson.build | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/docs/meson.build b/docs/meson.build
index bb14eaebd3..f84306ba7e 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -46,9 +46,11 @@ if build_docs
meson.source_root() / 'docs/sphinx/qmp_lexer.py',
qapi_gen_depends ]
+ have_ga = have_tools and config_host.has_key('CONFIG_GUEST_AGENT')
+
man_pages = {
-'qemu-ga.8': (have_tools ? 'man8' : ''),
-'qemu-ga-ref.7': 'man7',
+'qemu-ga.8': (have_ga ? 'man8' : ''),
+'qemu-ga-ref.7': (have_ga ? 'man7' : ''),
'qemu-qmp-ref.7': 'man7',
'qemu-storage-daemon-qmp-ref.7': (have_tools ? 'man7' : ''),
'qemu-img.1': (have_tools ? 'man1' : ''),
--
2.20.1
Re: [PATCH] migration/block-dirty-bitmap: fix larger granularity bitmaps
On 10/21/20 5:17 PM, Vladimir Sementsov-Ogievskiy wrote:
21.10.2020 17:44, Stefan Reiter wrote:
sectors_per_chunk is a 64 bit integer, but the calculation is done in 32
bits, leading to an overflow for coarse bitmap granularities.
If that results in the value 0, it leads to a hang where no progress is
made but send_bitmap_bits is constantly called with nr_sectors being 0.
Signed-off-by: Stefan Reiter
---
migration/block-dirty-bitmap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/migration/block-dirty-bitmap.c
b/migration/block-dirty-bitmap.c
index 5bef793ac0..5398869e2b 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -562,8 +562,9 @@ static int add_bitmaps_to_list(DBMSaveState *s,
BlockDriverState *bs,
dbms->bitmap_alias = g_strdup(bitmap_alias);
dbms->bitmap = bitmap;
dbms->total_sectors = bdrv_nb_sectors(bs);
- dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
+ dbms->sectors_per_chunk = CHUNK_SIZE * 8lu *
I'd prefer 8llu for absolute safety.
bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+ assert(dbms->sectors_per_chunk != 0);
I doubt that we need this assertion. Bug fixed, and it's obviously
impossible.
And if we really want to assert that there is no overflow (assuming
future changes),
it should look like this:
assert(bdrv_dirty_bitmap_granularity(bitmap) < (1ull << 63) /
CHUNK_SIZE / 8 >> BDRV_SECTOR_BITS);
to cover not only corner case but any overflow.. And of course we should
modify original expression
to do ">> BDRV_SECTOR_BITS" earlier than all multiplies, like
dbms->sectors_per_chunk = CHUNK_SIZE * 8llu *
(bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS);
But I think that only s/8/8ull/ change is enough.
I agree, and I wouldn't mind removing the assert, but just to clarify it
was mostly meant to prevent the case where the migration gets stuck
entirely. Even if the calculation is wrong, it would at least do
_something_ instead of endlessly looping.
Maybe an
assert(nr_sectors != 0);
in send_bitmap_bits instead for that?
if (bdrv_dirty_bitmap_enabled(bitmap)) {
dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_ENABLED;
}
With 8llu and with or without assertion:
Reviewed-by: Vladimir Sementsov-Ogievskiy
[PATCH] migration/block-dirty-bitmap: fix larger granularity bitmaps
sectors_per_chunk is a 64 bit integer, but the calculation is done in 32
bits, leading to an overflow for coarse bitmap granularities.
If that results in the value 0, it leads to a hang where no progress is
made but send_bitmap_bits is constantly called with nr_sectors being 0.
Signed-off-by: Stefan Reiter
---
migration/block-dirty-bitmap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 5bef793ac0..5398869e2b 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -562,8 +562,9 @@ static int add_bitmaps_to_list(DBMSaveState *s,
BlockDriverState *bs,
dbms->bitmap_alias = g_strdup(bitmap_alias);
dbms->bitmap = bitmap;
dbms->total_sectors = bdrv_nb_sectors(bs);
-dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
+dbms->sectors_per_chunk = CHUNK_SIZE * 8lu *
bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+assert(dbms->sectors_per_chunk != 0);
if (bdrv_dirty_bitmap_enabled(bitmap)) {
dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_ENABLED;
}
--
2.20.1
Re: [SeaBIOS] Regression with latest SeaBIOS booting multi-disk root LVs?
On 21/09/2020 15:44, Paul Menzel wrote: Dear Stefan, Am 21.09.20 um 15:10 schrieb Stefan Reiter: since SeaBIOS 1.14.0 (QEMU 5.1) VMs with LVM root disks spanning more than one PV fail to boot, if only the first is set as bootable. I believe this is due to the changes in SeaBIOS only initializing drives marked as 'bootable' by QEMU. One fix is to mark all disks containing root data as bootable, but existing setups will still break on upgrade (where only the disk containing the bootloader is marked). This is not ideal. Discovered by a user in our bugtracker: https://bugzilla.proxmox.com/show_bug.cgi?id=3011 and verified by installing Ubuntu 20.04 w/ LVM and GRUB on virtio-scsi, then expanding the LV to a second disk. I found that just reverting SeaBIOS to 1.13.0 makes it work again, same guest install, even with QEMU 5.1. Is this intended behaviour? A bug in GRUB? Any fix or workaround? As SeaBIOS 1.13.0 works and SeaBIOS 1.14.0 does not, please bisect the issue, and tell us the commit introducing the regression. Bisected for virtio-blk drives: 2f4d068645 "virtio: Do not init non-bootable devices" ...as expected. Also, untested, but I would strongly assume: d6bdb85eb0 "virtio-scsi: skip initializing non-bootable devices" is the equivalent for virtio-scsi devices, and 76551856b2 "nvme: skip initializing non-bootable devices" for NVMe. Kind regards, Paul
Regression with latest SeaBIOS booting multi-disk root LVs?
Hi list, since SeaBIOS 1.14.0 (QEMU 5.1) VMs with LVM root disks spanning more than one PV fail to boot, if only the first is set as bootable. I believe this is due to the changes in SeaBIOS only initializing drives marked as 'bootable' by QEMU. One fix is to mark all disks containing root data as bootable, but existing setups will still break on upgrade (where only the disk containing the bootloader is marked). This is not ideal. Discovered by a user in our bugtracker: https://bugzilla.proxmox.com/show_bug.cgi?id=3011 and verified by installing Ubuntu 20.04 w/ LVM and GRUB on virtio-scsi, then expanding the LV to a second disk. I found that just reverting SeaBIOS to 1.13.0 makes it work again, same guest install, even with QEMU 5.1. Is this intended behaviour? A bug in GRUB? Any fix or workaround? ~ Stefan
Re: Potential regression in 'qemu-img convert' to LVM
On 9/15/20 11:08 AM, Nir Soffer wrote: On Mon, Sep 14, 2020 at 3:25 PM Stefan Reiter wrote: Hi list, following command fails since 5.1 (tested on kernel 5.4.60): # qemu-img convert -p -f raw -O raw /dev/zvol/pool/disk-1 /dev/vg/disk-1 qemu-img: error while writing at byte 2157968896: Device or resource busy (source is ZFS here, but doesn't matter in practice, it always fails the same; offset changes slightly but consistently hovers around 2^31) strace shows the following: fallocate(13, FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE, 2157968896, 4608) = -1 EBUSY (Device or resource busy) What is the size of the LV? Same as the source, 5GB in my test case. Created with: # lvcreate -ay --size 5242880k --name disk-1 vg Does it happen if you change sparse minimum size (-S)? For example: -S 64k qemu-img convert -p -f raw -O raw -S 64k /dev/zvol/pool/disk-1 /dev/vg/disk-1 Tried a few different values, always the same result: EBUSY at byte 2157968896. Other fallocate calls leading up to this work fine. This happens since commit edafc70c0c "qemu-img convert: Don't pre-zero images", before that all fallocates happened at the start. Reverting the commit and calling qemu-img exactly the same way on the same data works fine. But slowly, doing up to 100% more work for fully allocated images. Of course, I'm not saying the patch is wrong, reverting it just avoids triggering the bug. Simply retrying the syscall on EBUSY (like EINTR) does *not* work, once it fails it keeps failing with the same error. I couldn't find anything related to EBUSY on fallocate, and it only happens on LVM targets... Any idea or pointers where to look? Is this thin LV? No, regular LV. See command above. This works for us using regular LVs. Which kernel? which distro? Reproducible on: * PVE w/ kernel 5.4.60 (Ubuntu based) * Manjaro w/ kernel 5.8.6 I found that it does not happen with all images, I suppose there must be a certain number of smaller holes for it to happen. I am using a VM image with a bare-bones Alpine Linux installation, but it's not an isolated case, we've had two people report the issue on our bug tracker: https://bugzilla.proxmox.com/show_bug.cgi?id=3002 Thanks, Stefan Nir
Potential regression in 'qemu-img convert' to LVM
Hi list, following command fails since 5.1 (tested on kernel 5.4.60): # qemu-img convert -p -f raw -O raw /dev/zvol/pool/disk-1 /dev/vg/disk-1 qemu-img: error while writing at byte 2157968896: Device or resource busy (source is ZFS here, but doesn't matter in practice, it always fails the same; offset changes slightly but consistently hovers around 2^31) strace shows the following: fallocate(13, FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE, 2157968896, 4608) = -1 EBUSY (Device or resource busy) Other fallocate calls leading up to this work fine. This happens since commit edafc70c0c "qemu-img convert: Don't pre-zero images", before that all fallocates happened at the start. Reverting the commit and calling qemu-img exactly the same way on the same data works fine. Simply retrying the syscall on EBUSY (like EINTR) does *not* work, once it fails it keeps failing with the same error. I couldn't find anything related to EBUSY on fallocate, and it only happens on LVM targets... Any idea or pointers where to look? ~ Stefan
Re: [PATCH 0/3] Add support for sequential backups
Friendly ping :) On 8/26/20 2:13 PM, Stefan Reiter wrote: Backups can already be done for multiple drives in a transaction. However, these jobs will start all at once, potentially hogging a lot of disk IO all at once. This problem is made worse, since IO throttling is only available on a per-job basis. Add a flag to QMP to support sequential transactions for backups. This way, every job will be executed one after the other, while still providing the benefit of transactions (i.e. once one fails, all remaining ones will be cancelled). We've internally (in Proxmox VE) been doing sequential backups for a long time with great success, albeit in a different fashion. This series is the result of aligning our internal changes closer to upstream, and might be useful for other people as well. Stefan Reiter (3): job: add sequential transaction support blockdev: add sequential mode to *-backup transactions backup: initialize bcs bitmap on job create, not start block/backup.c| 4 ++-- blockdev.c| 25 ++--- include/qemu/job.h| 12 job.c | 24 qapi/transaction.json | 6 +- 5 files changed, 65 insertions(+), 6 deletions(-)
[PATCH 2/3] blockdev: add sequential mode to *-backup transactions
Only supported with completion-mode 'grouped', since it relies on a
JobTxn to exist. This means that for now it is only available for
{drive,blockdev}-backup transactions.
Since only one job will be running at a time, bandwidth-limits can be
applied effectively. It can also prevent overloading a host's IO
capabilities in general.
Signed-off-by: Stefan Reiter
---
blockdev.c| 25 ++---
qapi/transaction.json | 6 +-
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 3848a9c8ab..3691e5e791 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1826,7 +1826,10 @@ static void drive_backup_commit(BlkActionState *common)
aio_context_acquire(aio_context);
assert(state->job);
-job_start(>job->job);
+
+if (!common->txn_props->sequential) {
+job_start(>job->job);
+}
aio_context_release(aio_context);
}
@@ -1927,7 +1930,9 @@ static void blockdev_backup_commit(BlkActionState *common)
aio_context_acquire(aio_context);
assert(state->job);
-job_start(>job->job);
+if (!common->txn_props->sequential) {
+job_start(>job->job);
+}
aio_context_release(aio_context);
}
@@ -2303,6 +2308,11 @@ static TransactionProperties *get_transaction_properties(
props->completion_mode = ACTION_COMPLETION_MODE_INDIVIDUAL;
}
+if (!props->has_sequential) {
+props->has_sequential = true;
+props->sequential = false;
+}
+
return props;
}
@@ -2328,7 +2338,11 @@ void qmp_transaction(TransactionActionList *dev_list,
*/
props = get_transaction_properties(props);
if (props->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
-block_job_txn = job_txn_new();
+block_job_txn = props->sequential ? job_txn_new_seq() : job_txn_new();
+} else if (props->sequential) {
+error_setg(errp, "Sequential transaction mode is not supported with "
+ "completion-mode = individual");
+return;
}
/* drain all i/o before any operations */
@@ -2367,6 +2381,11 @@ void qmp_transaction(TransactionActionList *dev_list,
}
}
+/* jobs in sequential txns don't start themselves on commit */
+if (block_job_txn && props->sequential) {
+job_txn_start_seq(block_job_txn);
+}
+
/* success */
goto exit;
diff --git a/qapi/transaction.json b/qapi/transaction.json
index 15ddebdbc3..4808383088 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -84,11 +84,15 @@
# Actions will complete or fail as a group.
# See @ActionCompletionMode for details.
#
+# @sequential: Run the jobs in the transaction one after the other, instead
+# of all at once. Not supported for completion-mode 'individual'.
+#
# Since: 2.5
##
{ 'struct': 'TransactionProperties',
'data': {
- '*completion-mode': 'ActionCompletionMode'
+ '*completion-mode': 'ActionCompletionMode',
+ '*sequential': 'bool'
}
}
--
2.20.1
[PATCH 1/3] job: add sequential transaction support
Jobs in a sequential transaction should never be started with job_start
manually. job_txn_start_seq and the sequentially called job_start will
take care of it, 'assert'ing in case a job is already running or has
finished.
Signed-off-by: Stefan Reiter
---
include/qemu/job.h | 12
job.c | 24
2 files changed, 36 insertions(+)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 32aabb1c60..f7a6a0926a 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -280,6 +280,18 @@ typedef enum JobCreateFlags {
*/
JobTxn *job_txn_new(void);
+/**
+ * Create a new transaction and set it to sequential mode, i.e. run all jobs
+ * one after the other instead of at the same time.
+ */
+JobTxn *job_txn_new_seq(void);
+
+/**
+ * Helper method to start the first job in a sequential transaction to kick it
+ * off. Other jobs will be run after this one completes.
+ */
+void job_txn_start_seq(JobTxn *txn);
+
/**
* Release a reference that was previously acquired with job_txn_add_job or
* job_txn_new. If it's the last reference to the object, it will be freed.
diff --git a/job.c b/job.c
index 8fecf38960..4df7c1d2ca 100644
--- a/job.c
+++ b/job.c
@@ -72,6 +72,8 @@ struct JobTxn {
/* Reference count */
int refcnt;
+
+bool sequential;
};
/* Right now, this mutex is only needed to synchronize accesses to job->busy
@@ -102,6 +104,25 @@ JobTxn *job_txn_new(void)
return txn;
}
+JobTxn *job_txn_new_seq(void)
+{
+JobTxn *txn = job_txn_new();
+txn->sequential = true;
+return txn;
+}
+
+void job_txn_start_seq(JobTxn *txn)
+{
+assert(txn->sequential);
+assert(!txn->aborting);
+
+Job *first = QLIST_FIRST(>jobs);
+assert(first);
+assert(first->status == JOB_STATUS_CREATED);
+
+job_start(first);
+}
+
static void job_txn_ref(JobTxn *txn)
{
txn->refcnt++;
@@ -840,6 +861,9 @@ static void job_completed_txn_success(Job *job)
*/
QLIST_FOREACH(other_job, >jobs, txn_list) {
if (!job_is_completed(other_job)) {
+if (txn->sequential) {
+job_start(other_job);
+}
return;
}
assert(other_job->ret == 0);
--
2.20.1
[PATCH 0/3] Add support for sequential backups
Backups can already be done for multiple drives in a transaction. However, these jobs will start all at once, potentially hogging a lot of disk IO all at once. This problem is made worse, since IO throttling is only available on a per-job basis. Add a flag to QMP to support sequential transactions for backups. This way, every job will be executed one after the other, while still providing the benefit of transactions (i.e. once one fails, all remaining ones will be cancelled). We've internally (in Proxmox VE) been doing sequential backups for a long time with great success, albeit in a different fashion. This series is the result of aligning our internal changes closer to upstream, and might be useful for other people as well. Stefan Reiter (3): job: add sequential transaction support blockdev: add sequential mode to *-backup transactions backup: initialize bcs bitmap on job create, not start block/backup.c| 4 ++-- blockdev.c| 25 ++--- include/qemu/job.h| 12 job.c | 24 qapi/transaction.json | 6 +- 5 files changed, 65 insertions(+), 6 deletions(-) -- 2.20.1
[PATCH 3/3] backup: initialize bcs bitmap on job create, not start
After backup_init_bcs_bitmap the copy-before-write behaviour is active.
This way, multiple backup jobs created at once but running in a
sequential transaction will still represent the same point in time.
Signed-off-by: Stefan Reiter
---
I'd imagine this was done on job start for a purpose, so this is potentially
wrong. In testing it works fine.
Sent along for feedback, since it would be necessary to really make use of the
sequential backup feature (without it, the individual backup jobs would not have
a consistent view of the guest).
block/backup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 4f13bb20a5..14660eef45 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -237,8 +237,6 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
int ret = 0;
-backup_init_bcs_bitmap(s);
-
if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
int64_t offset = 0;
int64_t count;
@@ -471,6 +469,8 @@ BlockJob *backup_job_create(const char *job_id,
BlockDriverState *bs,
block_job_add_bdrv(>common, "target", target, 0, BLK_PERM_ALL,
_abort);
+backup_init_bcs_bitmap(job);
+
return >common;
error:
--
2.20.1
Re: [PATCH for-5.1 v2 2/2] iotests: add test for unaligned granularity bitmap backup
On 8/10/20 5:11 PM, Max Reitz wrote:
(Note: When submitting a patch series with multiple patches, our
guidelines require a cover letter:
https://wiki.qemu.org/Contribute/SubmitAPatch#Include_a_meaningful_cover_letter
But not too important now.)
Sorry, remembered for next time. Thanks for applying the patches!
On 10.08.20 11:55, Stefan Reiter wrote:
Start a VM with a 4097 byte image attached, add a 4096 byte granularity
dirty bitmap, mark it dirty, and then do a backup.
This used to run into an assert and fail, check that it works as
expected and also check the created image to ensure that misaligned
backups in general work correctly.
Signed-off-by: Stefan Reiter
---
I saw Andrey's big series covering iotest 303 so I went for 304.
Works for me.
Never submitted
one before so I hope that's okay, if not feel free to renumber it.
Yep, if there’s a clash I tend to just renumber it when applying it.
tests/qemu-iotests/304 | 68 ++
tests/qemu-iotests/304.out | 2 ++
tests/qemu-iotests/group | 1 +
3 files changed, 71 insertions(+)
create mode 100755 tests/qemu-iotests/304
create mode 100644 tests/qemu-iotests/304.out
diff --git a/tests/qemu-iotests/304 b/tests/qemu-iotests/304
new file mode 100755
index 00..9a3b0224fa
--- /dev/null
+++ b/tests/qemu-iotests/304
@@ -0,0 +1,68 @@
+#!/usr/bin/env python3
+#
+# Tests dirty-bitmap backup with unaligned bitmap granularity
+#
+# Copyright (c) 2020 Proxmox Server Solutions
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+# owner=s.rei...@proxmox.com
+
+import iotests
+from iotests import qemu_img_create, qemu_img_log, file_path
+
+iotests.script_initialize(supported_fmts=['qcow2'],
+ supported_protocols=['file'])
+
+test_img = file_path('test.qcow2')
+target_img = file_path('target.qcow2')
+
+# unaligned by one byte
+image_len = 4097
+bitmap_granularity = 4096
+
+qemu_img_create('-f', iotests.imgfmt, test_img, str(image_len))
+
+# create VM and add dirty bitmap
+vm = iotests.VM().add_drive(test_img)
+vm.launch()
+
+vm.qmp('block-dirty-bitmap-add', **{
+'node': 'drive0',
+'name': 'bitmap0',
+'granularity': bitmap_granularity
+})
+
+# mark entire bitmap as dirty
+vm.hmp_qemu_io('drive0', 'write -P0x16 0 4096');
+vm.hmp_qemu_io('drive0', 'write -P0x17 4097 1');
s/4097/4096/?
(4097 works, too, because of something somewhere aligning up the 4097 to
512 byte sectors, I suppose, but I don’t think it’s the address you want
here)
You're right, it seems counting is hard. I'll take you up on the offer
from your other mail, you can fix this please :)
+
+# do backup and wait for completion
+vm.qmp('drive-backup', **{
+'device': 'drive0',
+'sync': 'full',
+'target': target_img,
+'bitmap': 'bitmap0',
+'bitmap-mode': 'on-success'
The bitmap is unnecessary, isn’t it? I.e., if I drop the
block-dirty-bitmap-add call and the bitmap* parameters here, I still get
an assertion failure without patch 1.
Not that it really matters, it’s just that this makes it look like less
of an issue than it actually is. (Which is why I’d drop the bitmap
stuff in case there’s no actual reason for it.)
Oh my, I just realized that I misunderstood the root cause then. I mean,
the fix is fine, I see it now, but you're right, no dirty bitmap needed
- you can remove that as well if you want.
+})
+
+event = vm.event_wait(name='BLOCK_JOB_COMPLETED',
+ match={'data': {'device': 'drive0'}},
+ timeout=5.0)
(By the way, “vm.run_job('drive0', auto_dismiss=True)” would have worked
as well. But since the backup job just needs waiting for a single
event, I suppose it doesn’t matter. Just a hint in case you start
writing more iotests in the future.)
Max
[PATCH for-5.1 v2 2/2] iotests: add test for unaligned granularity bitmap backup
Start a VM with a 4097 byte image attached, add a 4096 byte granularity
dirty bitmap, mark it dirty, and then do a backup.
This used to run into an assert and fail, check that it works as
expected and also check the created image to ensure that misaligned
backups in general work correctly.
Signed-off-by: Stefan Reiter
---
I saw Andrey's big series covering iotest 303 so I went for 304. Never submitted
one before so I hope that's okay, if not feel free to renumber it.
tests/qemu-iotests/304 | 68 ++
tests/qemu-iotests/304.out | 2 ++
tests/qemu-iotests/group | 1 +
3 files changed, 71 insertions(+)
create mode 100755 tests/qemu-iotests/304
create mode 100644 tests/qemu-iotests/304.out
diff --git a/tests/qemu-iotests/304 b/tests/qemu-iotests/304
new file mode 100755
index 00..9a3b0224fa
--- /dev/null
+++ b/tests/qemu-iotests/304
@@ -0,0 +1,68 @@
+#!/usr/bin/env python3
+#
+# Tests dirty-bitmap backup with unaligned bitmap granularity
+#
+# Copyright (c) 2020 Proxmox Server Solutions
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+# owner=s.rei...@proxmox.com
+
+import iotests
+from iotests import qemu_img_create, qemu_img_log, file_path
+
+iotests.script_initialize(supported_fmts=['qcow2'],
+ supported_protocols=['file'])
+
+test_img = file_path('test.qcow2')
+target_img = file_path('target.qcow2')
+
+# unaligned by one byte
+image_len = 4097
+bitmap_granularity = 4096
+
+qemu_img_create('-f', iotests.imgfmt, test_img, str(image_len))
+
+# create VM and add dirty bitmap
+vm = iotests.VM().add_drive(test_img)
+vm.launch()
+
+vm.qmp('block-dirty-bitmap-add', **{
+'node': 'drive0',
+'name': 'bitmap0',
+'granularity': bitmap_granularity
+})
+
+# mark entire bitmap as dirty
+vm.hmp_qemu_io('drive0', 'write -P0x16 0 4096');
+vm.hmp_qemu_io('drive0', 'write -P0x17 4097 1');
+
+# do backup and wait for completion
+vm.qmp('drive-backup', **{
+'device': 'drive0',
+'sync': 'full',
+'target': target_img,
+'bitmap': 'bitmap0',
+'bitmap-mode': 'on-success'
+})
+
+event = vm.event_wait(name='BLOCK_JOB_COMPLETED',
+ match={'data': {'device': 'drive0'}},
+ timeout=5.0)
+
+# shutdown to sync images
+vm.shutdown()
+
+# backup succeeded, check if image is correct
+qemu_img_log('compare', test_img, target_img)
diff --git a/tests/qemu-iotests/304.out b/tests/qemu-iotests/304.out
new file mode 100644
index 00..381cc056f7
--- /dev/null
+++ b/tests/qemu-iotests/304.out
@@ -0,0 +1,2 @@
+Images are identical.
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 025ed5238d..7f76066640 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -309,3 +309,4 @@
299 auto quick
301 backing quick
302 quick
+304 rw quick
--
2.20.1
[PATCH for-5.1 v2 1/2] block/block-copy: always align copied region to cluster size
Since commit 42ac214406e0 (block/block-copy: refactor task creation) block_copy_task_create calculates the area to be copied via bdrv_dirty_bitmap_next_dirty_area, but that can return an unaligned byte count if the image's last cluster end is not aligned to the bitmap's granularity. Always ALIGN_UP the resulting bytes value to satisfy block_copy_do_copy, which requires the 'bytes' parameter to be aligned to cluster size. Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Stefan Reiter --- I've now marked it for-5.1 since it is just a fix, but it's probably okay if done later as well. v2: * add assert on offset alignment * remove 'backing image' wording from commit * collect R-b block/block-copy.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/block/block-copy.c b/block/block-copy.c index f7428a7c08..a30b9097ef 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -142,6 +142,9 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s, return NULL; } +assert(QEMU_IS_ALIGNED(offset, s->cluster_size)); +bytes = QEMU_ALIGN_UP(bytes, s->cluster_size); + /* region is dirty, so no existent tasks possible in it */ assert(!find_conflicting_task(s, offset, bytes)); -- 2.20.1
[PATCH] block/block-copy: always align copied region to cluster size
Since commit 42ac214406e0 (block/block-copy: refactor task creation) block_copy_task_create calculates the area to be copied via bdrv_dirty_bitmap_next_dirty_area, but that can return an unaligned byte count if the backing image's last cluster end is not aligned to the bitmap's granularity. Always ALIGN_UP the resulting bytes value to satisfy block_copy_do_copy, which requires the 'bytes' parameter to be aligned to cluster size. Signed-off-by: Stefan Reiter --- This causes backups with unaligned image sizes to fail on the last block in my testing (e.g. a backup job with 4k cluster size fails on a drive with 4097 bytes). Alternatively one could remove the assert(QEMU_IS_ALIGNED(bytes, s->cluster_size)); from block_copy_do_copy, but I'd wager that's there for a reason? block/block-copy.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block/block-copy.c b/block/block-copy.c index f7428a7c08..023cb03200 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -142,6 +142,8 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s, return NULL; } +bytes = QEMU_ALIGN_UP(bytes, s->cluster_size); + /* region is dirty, so no existent tasks possible in it */ assert(!find_conflicting_task(s, offset, bytes)); -- 2.20.1
Re: [RFC PATCH 2/3] block: Allow bdrv_run_co() from different AioContext
On 5/26/20 6:42 PM, Kevin Wolf wrote:
Am 25.05.2020 um 18:41 hat Kevin Wolf geschrieben:
Am 25.05.2020 um 16:18 hat Stefan Reiter geschrieben:
On 5/12/20 4:43 PM, Kevin Wolf wrote:
Coroutine functions that are entered through bdrv_run_co() are already
safe to call from synchronous code in a different AioContext because
bdrv_coroutine_enter() will schedule them in the context of the node.
However, the coroutine fastpath still requires that we're already in the
right AioContext when called in coroutine context.
In order to make the behaviour more consistent and to make life a bit
easier for callers, let's check the AioContext and automatically move
the current coroutine around if we're not in the right context yet.
Signed-off-by: Kevin Wolf
---
block/io.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/block/io.c b/block/io.c
index c1badaadc9..7808e8bdc0 100644
--- a/block/io.c
+++ b/block/io.c
@@ -895,8 +895,21 @@ static int bdrv_run_co(BlockDriverState *bs,
CoroutineEntry *entry,
void *opaque, int *ret)
{
if (qemu_in_coroutine()) {
-/* Fast-path if already in coroutine context */
+Coroutine *self = qemu_coroutine_self();
+AioContext *bs_ctx = bdrv_get_aio_context(bs);
+AioContext *co_ctx = qemu_coroutine_get_aio_context(self);
+
+if (bs_ctx != co_ctx) {
+/* Move to the iothread of the node */
+aio_co_schedule(bs_ctx, self);
+qemu_coroutine_yield();
I'm pretty sure this can lead to a race: When the thread we're re-scheduling
to is faster to schedule us than we can reach qemu_coroutine_yield, then
we'll get an abort ("Co-routine re-entered recursively"), since co->caller
is still set.
I've seen this happen in our code when I try to do the scheduling fandangle
there.
Ah, crap. I guess letting a coroutine re-schedule itself is only safe
within the same thread then.
Is there a safer way to have a coroutine reschedule itself? Some lock
missing?
There is no problem that can't be solved by adding another level of
indirection... We would have to schedule a BH in the original thread
that will only schedule the coroutine in its new thread after it has
yielded.
Maybe we should actually introduce a helper function that moves the
current coroutine to a different AioContext this way.
Like this:
https://repo.or.cz/qemu/kevin.git/commitdiff/ed0244ba4ac699f7e8eaf7512ff25645cf43bda2
Commit looks good to me, using aio_co_reschedule_self fixes all aborts
I've been seeing.
The series for which I need this isn't quite ready yet, so I haven't
sent it as a patch yet, but if it proves useful in other contexts, we
can always commit it without the rest.
I did a quick search for places where a similar pattern is used and
found 'hw/9pfs/coth.h', where this behavior is already described (though
the bh seems to be scheduled using the threadpool API, which I'm not
really familiar with). All other places where qemu_coroutine_yield() is
preceded by a aio_co_schedule() only do so in the same AioContext, which
should be safe.
Kevin
Re: [RFC PATCH 2/3] block: Allow bdrv_run_co() from different AioContext
On 5/12/20 4:43 PM, Kevin Wolf wrote:
Coroutine functions that are entered through bdrv_run_co() are already
safe to call from synchronous code in a different AioContext because
bdrv_coroutine_enter() will schedule them in the context of the node.
However, the coroutine fastpath still requires that we're already in the
right AioContext when called in coroutine context.
In order to make the behaviour more consistent and to make life a bit
easier for callers, let's check the AioContext and automatically move
the current coroutine around if we're not in the right context yet.
Signed-off-by: Kevin Wolf
---
block/io.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/block/io.c b/block/io.c
index c1badaadc9..7808e8bdc0 100644
--- a/block/io.c
+++ b/block/io.c
@@ -895,8 +895,21 @@ static int bdrv_run_co(BlockDriverState *bs,
CoroutineEntry *entry,
void *opaque, int *ret)
{
if (qemu_in_coroutine()) {
-/* Fast-path if already in coroutine context */
+Coroutine *self = qemu_coroutine_self();
+AioContext *bs_ctx = bdrv_get_aio_context(bs);
+AioContext *co_ctx = qemu_coroutine_get_aio_context(self);
+
+if (bs_ctx != co_ctx) {
+/* Move to the iothread of the node */
+aio_co_schedule(bs_ctx, self);
+qemu_coroutine_yield();
I'm pretty sure this can lead to a race: When the thread we're
re-scheduling to is faster to schedule us than we can reach
qemu_coroutine_yield, then we'll get an abort ("Co-routine re-entered
recursively"), since co->caller is still set.
I've seen this happen in our code when I try to do the scheduling
fandangle there.
Is there a safer way to have a coroutine reschedule itself? Some lock
missing?
+}
entry(opaque);
+if (bs_ctx != co_ctx) {
+/* Move back to the original AioContext */
+aio_co_schedule(bs_ctx, self);
+qemu_coroutine_yield();
+}
} else {
Coroutine *co = qemu_coroutine_create(entry, opaque);
*ret = NOT_DONE;
Re: [RFC PATCH 3/3] block: Assert we're running in the right thread
On 5/12/20 4:43 PM, Kevin Wolf wrote:
tracked_request_begin() is called for most I/O operations, so it's a
good place to assert that we're indeed running in the home thread of the
node's AioContext.
Is this patch supposed to be always correct or only together with nr. 2?
I changed our code to call bdrv_flush_all from the main AIO context and
it certainly works just fine (even without this series, so I suppose
that would be the 'correct' way to fix it you mention on the cover),
though of course it trips this assert without patch 2.
Signed-off-by: Kevin Wolf
---
block/io.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/block/io.c b/block/io.c
index 7808e8bdc0..924bf5ba46 100644
--- a/block/io.c
+++ b/block/io.c
@@ -695,14 +695,17 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
uint64_t bytes,
enum BdrvTrackedRequestType type)
{
+Coroutine *self = qemu_coroutine_self();
+
assert(bytes <= INT64_MAX && offset <= INT64_MAX - bytes);
+assert(bs->aio_context == qemu_coroutine_get_aio_context(self));
*req = (BdrvTrackedRequest){
.bs = bs,
.offset = offset,
.bytes = bytes,
.type = type,
-.co = qemu_coroutine_self(),
+.co = self,
.serialising= false,
.overlap_offset = offset,
.overlap_bytes = bytes,
Re: [RFC] bdrv_flush: only use fast path when in owned AioContext
On 5/12/20 1:32 PM, Kevin Wolf wrote: Am 12.05.2020 um 12:57 hat Kevin Wolf geschrieben: Am 11.05.2020 um 18:50 hat Stefan Reiter geschrieben: Just because we're in a coroutine doesn't imply ownership of the context of the flushed drive. In such a case use the slow path which explicitly enters bdrv_flush_co_entry in the correct AioContext. Signed-off-by: Stefan Reiter --- We've experienced some lockups in this codepath when taking snapshots of VMs with drives that have IO-Threads enabled (we have an async 'savevm' implementation running from a coroutine). Currently no reproducer for upstream versions I could find, but in testing this patch fixes all issues we're seeing and I think the logic checks out. The fast path pattern is repeated a few times in this file, so if this change makes sense, it's probably worth evaluating the other occurences as well. What do you mean by "owning" the context? If it's about taking the AioContext lock, isn't the problem more with calling bdrv_flush() from code that doesn't take the locks? Though I think we have some code that doesn't only rely on holding the AioContext locks, but that actually depends on running in the right thread, so the change looks right anyway. "Owning" as in it only works (doesn't hang) when bdrv_flush_co_entry runs on the same AioContext that the BlockDriverState it's flushing belongs to. We hold the locks for all AioContexts we want to flush in our code (in this case called from do_vm_stop/bdrv_flush_all so we're even in a drained section). Well, the idea is right, but the change itself isn't, of course. If we're already in coroutine context, we must not busy wait with BDRV_POLL_WHILE(). I'll see if I can put something together after lunch. Kevin Thanks for taking a look!
[RFC] bdrv_flush: only use fast path when in owned AioContext
Just because we're in a coroutine doesn't imply ownership of the context
of the flushed drive. In such a case use the slow path which explicitly
enters bdrv_flush_co_entry in the correct AioContext.
Signed-off-by: Stefan Reiter
---
We've experienced some lockups in this codepath when taking snapshots of VMs
with drives that have IO-Threads enabled (we have an async 'savevm'
implementation running from a coroutine).
Currently no reproducer for upstream versions I could find, but in testing this
patch fixes all issues we're seeing and I think the logic checks out.
The fast path pattern is repeated a few times in this file, so if this change
makes sense, it's probably worth evaluating the other occurences as well.
block/io.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/io.c b/block/io.c
index aba67f66b9..ee7310fa13 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2895,8 +2895,9 @@ int bdrv_flush(BlockDriverState *bs)
.ret = NOT_DONE,
};
-if (qemu_in_coroutine()) {
-/* Fast-path if already in coroutine context */
+if (qemu_in_coroutine() &&
+bdrv_get_aio_context(bs) == qemu_get_current_aio_context()) {
+/* Fast-path if already in coroutine and we own the drive's context */
bdrv_flush_co_entry(_co);
} else {
co = qemu_coroutine_create(bdrv_flush_co_entry, _co);
--
2.20.1
qemu_coroutine_yield switches thread?
Hi list, quick question: Can a resume from a qemu_coroutine_yield happen in a different thread? Well, it can, since I'm seeing it happen, but is that okay or a bug? I.e. in a backup-job the following can sporadically trip: unsigned long tid = pthread_self(); qemu_get_current_aio_context(); // returns main context qemu_coroutine_yield(); qemu_get_current_aio_context(); // still returns main context, but: assert(tid == pthread_self()); // this fails It seems to be called from a vCPU thread when it happens. VM uses no iothreads. ~
[PATCH for-5.0 v5 3/3] backup: don't acquire aio_context in backup_clean
All code-paths leading to backup_clean (via job_clean) have the job's
context already acquired. The job's context is guaranteed to be the same
as the one used by backup_top via backup_job_create.
Since the previous logic effectively acquired the lock twice, this
broke cleanup of backups for disks using IO threads, since the BDRV_POLL_WHILE
in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock
once, thus deadlocking with the IO thread.
This is a partial revert of 0abf2581717a19.
Signed-off-by: Stefan Reiter
Reviewed-by: Max Reitz
---
block/backup.c | 4
1 file changed, 4 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 7430ca5883..a7a7dcaf4c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -126,11 +126,7 @@ static void backup_abort(Job *job)
static void backup_clean(Job *job)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-AioContext *aio_context = bdrv_get_aio_context(s->backup_top);
-
-aio_context_acquire(aio_context);
bdrv_backup_top_drop(s->backup_top);
-aio_context_release(aio_context);
}
void backup_do_checkpoint(BlockJob *job, Error **errp)
--
2.26.0
[PATCH for-5.0 v5 0/3] Fix some AIO context locking in jobs
Contains three seperate but related patches cleaning up and fixing some issues regarding aio_context_acquire/aio_context_release for jobs. Mostly affects blockjobs running for devices that have IO threads enabled AFAICT. Changes from v4: * Do job_ref/job_unref in job_txn_apply and job_exit since we need the job to survive the callback to access the potentially changed lock afterwards * Reduce patch 2/3 to an assert, the context should already be acquired since it's a bdrv handler * Collect R-by for 3/3 I've marked it 'for-5.0' this time, I think it would make sense to be picked up together with Kevin's "block: Fix blk->in_flight during blk_wait_while_drained()" series. With that series and these three patches applied I can no longer reproduce any of the reported related crashes/hangs. Changes from v3: * commit_job appears to be unset in certain cases when replication_close is called, only access when necessary to avoid SIGSEGV Missed this when shuffling around patches, sorry for noise with still-broken v3. Changes from v2: * reordered patch 1 to the end to not introduce temporary breakages * added more fixes to job txn patch (should now pass the tests) Changes from v1: * fixed commit message for patch 1 * added patches 2 and 3 qemu: Stefan Reiter (3): job: take each job's lock individually in job_txn_apply replication: assert we own context before job_cancel_sync backup: don't acquire aio_context in backup_clean block/backup.c| 4 block/replication.c | 5 - blockdev.c| 9 job-qmp.c | 9 job.c | 50 ++- tests/test-blockjob.c | 2 ++ 6 files changed, 64 insertions(+), 15 deletions(-) -- 2.26.0
[PATCH for-5.0 v5 1/3] job: take each job's lock individually in job_txn_apply
All callers of job_txn_apply hold a single job's lock, but different
jobs within a transaction can have different contexts, thus we need to
lock each one individually before applying the callback function.
Similar to job_completed_txn_abort this also requires releasing the
caller's context before and reacquiring it after to avoid recursive
locks which might break AIO_WAIT_WHILE in the callback. This is safe, since
existing code would already have to take this into account, lest
job_completed_txn_abort might have broken.
This also brings to light a different issue: When a callback function in
job_txn_apply moves it's job to a different AIO context, callers will
try to release the wrong lock (now that we re-acquire the lock
correctly, previously it would just continue with the old lock, leaving
the job unlocked for the rest of the return path). Fix this by not caching
the job's context.
This is only necessary for qmp_block_job_finalize, qmp_job_finalize and
job_exit, since everyone else calls through job_exit.
One test needed adapting, since it calls job_finalize directly, so it
manually needs to acquire the correct context.
Signed-off-by: Stefan Reiter
---
blockdev.c| 9
job-qmp.c | 9
job.c | 50 ++-
tests/test-blockjob.c | 2 ++
4 files changed, 60 insertions(+), 10 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index fa8630cb41..5faddaa705 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3612,7 +3612,16 @@ void qmp_block_job_finalize(const char *id, Error **errp)
}
trace_qmp_block_job_finalize(job);
+job_ref(>job);
job_finalize(>job, errp);
+
+/*
+ * Job's context might have changed via job_finalize (and job_txn_apply
+ * automatically acquires the new one), so make sure we release the correct
+ * one.
+ */
+aio_context = blk_get_aio_context(job->blk);
+job_unref(>job);
aio_context_release(aio_context);
}
diff --git a/job-qmp.c b/job-qmp.c
index fecc939ebd..f9a58832e1 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -114,7 +114,16 @@ void qmp_job_finalize(const char *id, Error **errp)
}
trace_qmp_job_finalize(job);
+job_ref(job);
job_finalize(job, errp);
+
+/*
+ * Job's context might have changed via job_finalize (and job_txn_apply
+ * automatically acquires the new one), so make sure we release the correct
+ * one.
+ */
+aio_context = job->aio_context;
+job_unref(job);
aio_context_release(aio_context);
}
diff --git a/job.c b/job.c
index 134a07b92e..53be57a3a0 100644
--- a/job.c
+++ b/job.c
@@ -136,17 +136,38 @@ static void job_txn_del_job(Job *job)
}
}
-static int job_txn_apply(JobTxn *txn, int fn(Job *))
+static int job_txn_apply(Job *job, int fn(Job *))
{
-Job *job, *next;
+AioContext *inner_ctx;
+Job *other_job, *next;
+JobTxn *txn = job->txn;
int rc = 0;
-QLIST_FOREACH_SAFE(job, >jobs, txn_list, next) {
-rc = fn(job);
+/*
+ * Similar to job_completed_txn_abort, we take each job's lock before
+ * applying fn, but since we assume that outer_ctx is held by the caller,
+ * we need to release it here to avoid holding the lock twice - which would
+ * break AIO_WAIT_WHILE from within fn.
+ */
+job_ref(job);
+aio_context_release(job->aio_context);
+
+QLIST_FOREACH_SAFE(other_job, >jobs, txn_list, next) {
+inner_ctx = other_job->aio_context;
+aio_context_acquire(inner_ctx);
+rc = fn(other_job);
+aio_context_release(inner_ctx);
if (rc) {
break;
}
}
+
+/*
+ * Note that job->aio_context might have been changed by calling fn, so we
+ * can't use a local variable to cache it.
+ */
+aio_context_acquire(job->aio_context);
+job_unref(job);
return rc;
}
@@ -774,11 +795,11 @@ static void job_do_finalize(Job *job)
assert(job && job->txn);
/* prepare the transaction to complete */
-rc = job_txn_apply(job->txn, job_prepare);
+rc = job_txn_apply(job, job_prepare);
if (rc) {
job_completed_txn_abort(job);
} else {
-job_txn_apply(job->txn, job_finalize_single);
+job_txn_apply(job, job_finalize_single);
}
}
@@ -824,10 +845,10 @@ static void job_completed_txn_success(Job *job)
assert(other_job->ret == 0);
}
-job_txn_apply(txn, job_transition_to_pending);
+job_txn_apply(job, job_transition_to_pending);
/* If no jobs need manual finalization, automatically do so */
-if (job_txn_apply(txn, job_needs_finalize) == 0) {
+if (job_txn_apply(job, job_needs_finalize) == 0) {
job_do_finalize(job);
}
}
@@ -849,9 +870,10 @@ static void job_completed(Job *job)
static void job_exit(void *opaque)
{
Job *job = (Job *)opaque;
-AioContext *ctx = job->aio_context;
+Aio
[PATCH for-5.0 v5 2/3] replication: assert we own context before job_cancel_sync
job_cancel_sync requires the job's lock to be held, all other callers
already do this (replication_stop, drive_backup_abort,
blockdev_backup_abort, job_cancel_sync_all, cancel_common).
In this case we're in a BlockDriver handler, so we already have a lock,
just assert that it is the same as the one used for the commit_job.
Signed-off-by: Stefan Reiter
---
block/replication.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/block/replication.c b/block/replication.c
index 413d95407d..da013c2041 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -144,12 +144,15 @@ fail:
static void replication_close(BlockDriverState *bs)
{
BDRVReplicationState *s = bs->opaque;
+Job *commit_job;
if (s->stage == BLOCK_REPLICATION_RUNNING) {
replication_stop(s->rs, false, NULL);
}
if (s->stage == BLOCK_REPLICATION_FAILOVER) {
-job_cancel_sync(>commit_job->job);
+commit_job = >commit_job->job;
+assert(commit_job->aio_context == qemu_get_current_aio_context());
+job_cancel_sync(commit_job);
}
if (s->mode == REPLICATION_MODE_SECONDARY) {
--
2.26.0
Re: [PATCH v4 2/3] replication: acquire aio context before calling job_cancel_sync
On 02/04/2020 14:41, Max Reitz wrote:
On 01.04.20 10:15, Stefan Reiter wrote:
job_cancel_sync requires the job's lock to be held, all other callers
already do this (replication_stop, drive_backup_abort,
blockdev_backup_abort, job_cancel_sync_all, cancel_common).
I think all other callers come directly from QMP, though, so they have
no locks yet. This OTOH is called from a block driver function, so I
would assume the BDS context is locked already (or rather, this is
executed in the BDS context).
I also think that the commit job runs in the same context. So I would
assume that this would be a nested lock, which should be unnecessary and
might cause problems. Maybe we should just assert that the job’s
context is the current context?
(Or would that still be problematic because now job_txn_apply() wants to
release some context, and that isn’t possible without this patch? I
would hope it’s possible, because I think we shouldn’t have to acquire
the current context, and should be free to release it...? I have no
idea. Maybe we are actually free to reacquire the current context here.)
You're right, this seems to be unnecessary. Adding an
assert(commit_job->aio_context == qemu_get_current_aio_context());
to make sure seems like a good idea though. bdrv_close appears to always
have the lock on its driver's context held.
Signed-off-by: Stefan Reiter
---
block/replication.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/block/replication.c b/block/replication.c
index 413d95407d..17ddc31569 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -144,12 +144,18 @@ fail:
static void replication_close(BlockDriverState *bs)
{
BDRVReplicationState *s = bs->opaque;
+Job *commit_job;
+AioContext *commit_ctx;
if (s->stage == BLOCK_REPLICATION_RUNNING) {
replication_stop(s->rs, false, NULL);
}
if (s->stage == BLOCK_REPLICATION_FAILOVER) {
-job_cancel_sync(>commit_job->job);
+commit_job = >commit_job->job;
+commit_ctx = commit_job->aio_context;
+aio_context_acquire(commit_ctx);
+job_cancel_sync(commit_job);
+aio_context_release(commit_ctx);
Anyway, there’s also the problem that I would guess the
job_cancel_sync() might move the job from its current context back into
the main context. Then we’d release the wrong context here.
> Max
}
if (s->mode == REPLICATION_MODE_SECONDARY) {
Re: [PATCH v4 1/3] job: take each job's lock individually in job_txn_apply
On 02/04/2020 14:33, Max Reitz wrote:
On 01.04.20 10:15, Stefan Reiter wrote:
All callers of job_txn_apply hold a single job's lock, but different
jobs within a transaction can have different contexts, thus we need to
lock each one individually before applying the callback function.
Similar to job_completed_txn_abort this also requires releasing the
caller's context before and reacquiring it after to avoid recursive
locks which might break AIO_WAIT_WHILE in the callback.
This also brings to light a different issue: When a callback function in
job_txn_apply moves it's job to a different AIO context, job_exit will
try to release the wrong lock (now that we re-acquire the lock
correctly, previously it would just continue with the old lock, leaving
the job unlocked for the rest of the codepath back to job_exit). Fix
this by not caching the job's context in job_exit and add a comment
about why this is done.
One test needed adapting, since it calls job_finalize directly, so it
manually needs to acquire the correct context.
Signed-off-by: Stefan Reiter
---
job.c | 48 ++-
tests/test-blockjob.c | 2 ++
2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/job.c b/job.c
index 134a07b92e..5fbaaabf78 100644
--- a/job.c
+++ b/job.c
@@ -136,17 +136,36 @@ static void job_txn_del_job(Job *job)
}
}
-static int job_txn_apply(JobTxn *txn, int fn(Job *))
+static int job_txn_apply(Job *job, int fn(Job *))
{
-Job *job, *next;
+AioContext *inner_ctx;
+Job *other_job, *next;
+JobTxn *txn = job->txn;
int rc = 0;
-QLIST_FOREACH_SAFE(job, >jobs, txn_list, next) {
-rc = fn(job);
+/*
+ * Similar to job_completed_txn_abort, we take each job's lock before
+ * applying fn, but since we assume that outer_ctx is held by the caller,
+ * we need to release it here to avoid holding the lock twice - which would
+ * break AIO_WAIT_WHILE from within fn.
+ */
+aio_context_release(job->aio_context);
Hm, is it safe to drop the lock here and then reacquire it later? I.e.,
is the job in a consistent state in between? I don’t know. Looks like
it. Maybe someone else knows better.
I would have said so too, but the iotest segfaults Kevin discovered (I
can reproduce them after a dozen or so cycles) seem to be triggered by
some kind of race, which I can only imagine being caused by it not being
safe to drop the lock.
It can be resolved by doing a job_ref/unref in job_txn_apply, but that
might be only fixing the symptoms and ignoring the problem.
(I find the job code rather confusing.)
That seems to be common around here ;)
+
+QLIST_FOREACH_SAFE(other_job, >jobs, txn_list, next) {
+inner_ctx = other_job->aio_context;
+aio_context_acquire(inner_ctx);
Ignoring the fact that fn() can change a job's lock, one idea I had was
to do a
if (inner_ctx != job->aio_context) {
aio_context_acquire(inner_ctx);
}
instead of releasing the lock prior.
However, that gets complicated when the job's context does change in fn.
+rc = fn(other_job);
+aio_context_release(inner_ctx);
if (rc) {
break;
}
}
+
+/*
+ * Note that job->aio_context might have been changed by calling fn, so we
+ * can't use a local variable to cache it.
+ */
+aio_context_acquire(job->aio_context);
But all callers of job_txn_apply() (but job_exit(), which you fix in
this patch) cache it. Won’t they release the wrong context then? Or is
a context change only possible when job_txn_apply() is called from
job_exit()?
You're right that it can probably change for other callers as well
(though at least it doesn't seem to currently, since no other test cases
fail?). Looking at the analysis Vladimir did [0], there's a few places
where that would need fixing.
The issue is that all of these places would also need the job_ref/unref
part AFAICT, which would make this a bit unwieldy...
[0] https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg07867.html
Max
[PATCH v4 1/3] job: take each job's lock individually in job_txn_apply
All callers of job_txn_apply hold a single job's lock, but different
jobs within a transaction can have different contexts, thus we need to
lock each one individually before applying the callback function.
Similar to job_completed_txn_abort this also requires releasing the
caller's context before and reacquiring it after to avoid recursive
locks which might break AIO_WAIT_WHILE in the callback.
This also brings to light a different issue: When a callback function in
job_txn_apply moves it's job to a different AIO context, job_exit will
try to release the wrong lock (now that we re-acquire the lock
correctly, previously it would just continue with the old lock, leaving
the job unlocked for the rest of the codepath back to job_exit). Fix
this by not caching the job's context in job_exit and add a comment
about why this is done.
One test needed adapting, since it calls job_finalize directly, so it
manually needs to acquire the correct context.
Signed-off-by: Stefan Reiter
---
job.c | 48 ++-
tests/test-blockjob.c | 2 ++
2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/job.c b/job.c
index 134a07b92e..5fbaaabf78 100644
--- a/job.c
+++ b/job.c
@@ -136,17 +136,36 @@ static void job_txn_del_job(Job *job)
}
}
-static int job_txn_apply(JobTxn *txn, int fn(Job *))
+static int job_txn_apply(Job *job, int fn(Job *))
{
-Job *job, *next;
+AioContext *inner_ctx;
+Job *other_job, *next;
+JobTxn *txn = job->txn;
int rc = 0;
-QLIST_FOREACH_SAFE(job, >jobs, txn_list, next) {
-rc = fn(job);
+/*
+ * Similar to job_completed_txn_abort, we take each job's lock before
+ * applying fn, but since we assume that outer_ctx is held by the caller,
+ * we need to release it here to avoid holding the lock twice - which would
+ * break AIO_WAIT_WHILE from within fn.
+ */
+aio_context_release(job->aio_context);
+
+QLIST_FOREACH_SAFE(other_job, >jobs, txn_list, next) {
+inner_ctx = other_job->aio_context;
+aio_context_acquire(inner_ctx);
+rc = fn(other_job);
+aio_context_release(inner_ctx);
if (rc) {
break;
}
}
+
+/*
+ * Note that job->aio_context might have been changed by calling fn, so we
+ * can't use a local variable to cache it.
+ */
+aio_context_acquire(job->aio_context);
return rc;
}
@@ -774,11 +793,11 @@ static void job_do_finalize(Job *job)
assert(job && job->txn);
/* prepare the transaction to complete */
-rc = job_txn_apply(job->txn, job_prepare);
+rc = job_txn_apply(job, job_prepare);
if (rc) {
job_completed_txn_abort(job);
} else {
-job_txn_apply(job->txn, job_finalize_single);
+job_txn_apply(job, job_finalize_single);
}
}
@@ -824,10 +843,10 @@ static void job_completed_txn_success(Job *job)
assert(other_job->ret == 0);
}
-job_txn_apply(txn, job_transition_to_pending);
+job_txn_apply(job, job_transition_to_pending);
/* If no jobs need manual finalization, automatically do so */
-if (job_txn_apply(txn, job_needs_finalize) == 0) {
+if (job_txn_apply(job, job_needs_finalize) == 0) {
job_do_finalize(job);
}
}
@@ -849,9 +868,10 @@ static void job_completed(Job *job)
static void job_exit(void *opaque)
{
Job *job = (Job *)opaque;
-AioContext *ctx = job->aio_context;
+AioContext *ctx;
-aio_context_acquire(ctx);
+job_ref(job);
+aio_context_acquire(job->aio_context);
/* This is a lie, we're not quiescent, but still doing the completion
* callbacks. However, completion callbacks tend to involve operations that
@@ -862,6 +882,14 @@ static void job_exit(void *opaque)
job_completed(job);
+/*
+ * Note that calling job_completed can move the job to a different
+ * aio_context, so we cannot cache from above. job_txn_apply takes care of
+ * acquiring the new lock, and we ref/unref to avoid job_completed freeing
+ * the job underneath us.
+ */
+ctx = job->aio_context;
+job_unref(job);
aio_context_release(ctx);
}
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 4eeb184caf..7519847912 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -367,7 +367,9 @@ static void test_cancel_concluded(void)
aio_poll(qemu_get_aio_context(), true);
assert(job->status == JOB_STATUS_PENDING);
+aio_context_acquire(job->aio_context);
job_finalize(job, _abort);
+aio_context_release(job->aio_context);
assert(job->status == JOB_STATUS_CONCLUDED);
cancel_common(s);
--
2.26.0
[PATCH v4 0/3] Fix some AIO context locking in jobs
Contains three seperate but related patches cleaning up and fixing some issues regarding aio_context_acquire/aio_context_release for jobs. Mostly affects blockjobs running for devices that have IO threads enabled AFAICT. This is based on the discussions here: https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg07929.html Changes from v3: * commit_job appears to be unset in certain cases when replication_close is called, only access when necessary to avoid SIGSEGV Missed this when shuffling around patches, sorry for noise with still-broken v3. Changes from v2: * reordered patch 1 to the end to not introduce temporary breakages * added more fixes to job txn patch (should now pass the tests) Changes from v1: * fixed commit message for patch 1 * added patches 2 and 3 qemu: Stefan Reiter (3): job: take each job's lock individually in job_txn_apply replication: acquire aio context before calling job_cancel_sync backup: don't acquire aio_context in backup_clean block/backup.c| 4 block/replication.c | 8 +++- job.c | 48 ++- tests/test-blockjob.c | 2 ++ 4 files changed, 47 insertions(+), 15 deletions(-) -- 2.26.0
[PATCH v4 3/3] backup: don't acquire aio_context in backup_clean
All code-paths leading to backup_clean (via job_clean) have the job's
context already acquired. The job's context is guaranteed to be the same
as the one used by backup_top via backup_job_create.
Since the previous logic effectively acquired the lock twice, this
broke cleanup of backups for disks using IO threads, since the BDRV_POLL_WHILE
in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock
once, thus deadlocking with the IO thread.
This is a partial revert of 0abf2581717a19.
Signed-off-by: Stefan Reiter
---
With the two previous patches applied, the commit message should now hold true.
block/backup.c | 4
1 file changed, 4 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 7430ca5883..a7a7dcaf4c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -126,11 +126,7 @@ static void backup_abort(Job *job)
static void backup_clean(Job *job)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-AioContext *aio_context = bdrv_get_aio_context(s->backup_top);
-
-aio_context_acquire(aio_context);
bdrv_backup_top_drop(s->backup_top);
-aio_context_release(aio_context);
}
void backup_do_checkpoint(BlockJob *job, Error **errp)
--
2.26.0
[PATCH v4 2/3] replication: acquire aio context before calling job_cancel_sync
job_cancel_sync requires the job's lock to be held, all other callers
already do this (replication_stop, drive_backup_abort,
blockdev_backup_abort, job_cancel_sync_all, cancel_common).
Signed-off-by: Stefan Reiter
---
block/replication.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/block/replication.c b/block/replication.c
index 413d95407d..17ddc31569 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -144,12 +144,18 @@ fail:
static void replication_close(BlockDriverState *bs)
{
BDRVReplicationState *s = bs->opaque;
+Job *commit_job;
+AioContext *commit_ctx;
if (s->stage == BLOCK_REPLICATION_RUNNING) {
replication_stop(s->rs, false, NULL);
}
if (s->stage == BLOCK_REPLICATION_FAILOVER) {
-job_cancel_sync(>commit_job->job);
+commit_job = >commit_job->job;
+commit_ctx = commit_job->aio_context;
+aio_context_acquire(commit_ctx);
+job_cancel_sync(commit_job);
+aio_context_release(commit_ctx);
}
if (s->mode == REPLICATION_MODE_SECONDARY) {
--
2.26.0
[PATCH v3 1/3] job: take each job's lock individually in job_txn_apply
All callers of job_txn_apply hold a single job's lock, but different
jobs within a transaction can have different contexts, thus we need to
lock each one individually before applying the callback function.
Similar to job_completed_txn_abort this also requires releasing the
caller's context before and reacquiring it after to avoid recursive
locks which might break AIO_WAIT_WHILE in the callback.
This also brings to light a different issue: When a callback function in
job_txn_apply moves it's job to a different AIO context, job_exit will
try to release the wrong lock (now that we re-acquire the lock
correctly, previously it would just continue with the old lock, leaving
the job unlocked for the rest of the codepath back to job_exit). Fix
this by not caching the job's context in job_exit and add a comment
about why this is done.
One test needed adapting, since it calls job_finalize directly, so it
manually needs to acquire the correct context.
Signed-off-by: Stefan Reiter
---
job.c | 48 ++-
tests/test-blockjob.c | 2 ++
2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/job.c b/job.c
index 134a07b92e..5fbaaabf78 100644
--- a/job.c
+++ b/job.c
@@ -136,17 +136,36 @@ static void job_txn_del_job(Job *job)
}
}
-static int job_txn_apply(JobTxn *txn, int fn(Job *))
+static int job_txn_apply(Job *job, int fn(Job *))
{
-Job *job, *next;
+AioContext *inner_ctx;
+Job *other_job, *next;
+JobTxn *txn = job->txn;
int rc = 0;
-QLIST_FOREACH_SAFE(job, >jobs, txn_list, next) {
-rc = fn(job);
+/*
+ * Similar to job_completed_txn_abort, we take each job's lock before
+ * applying fn, but since we assume that outer_ctx is held by the caller,
+ * we need to release it here to avoid holding the lock twice - which would
+ * break AIO_WAIT_WHILE from within fn.
+ */
+aio_context_release(job->aio_context);
+
+QLIST_FOREACH_SAFE(other_job, >jobs, txn_list, next) {
+inner_ctx = other_job->aio_context;
+aio_context_acquire(inner_ctx);
+rc = fn(other_job);
+aio_context_release(inner_ctx);
if (rc) {
break;
}
}
+
+/*
+ * Note that job->aio_context might have been changed by calling fn, so we
+ * can't use a local variable to cache it.
+ */
+aio_context_acquire(job->aio_context);
return rc;
}
@@ -774,11 +793,11 @@ static void job_do_finalize(Job *job)
assert(job && job->txn);
/* prepare the transaction to complete */
-rc = job_txn_apply(job->txn, job_prepare);
+rc = job_txn_apply(job, job_prepare);
if (rc) {
job_completed_txn_abort(job);
} else {
-job_txn_apply(job->txn, job_finalize_single);
+job_txn_apply(job, job_finalize_single);
}
}
@@ -824,10 +843,10 @@ static void job_completed_txn_success(Job *job)
assert(other_job->ret == 0);
}
-job_txn_apply(txn, job_transition_to_pending);
+job_txn_apply(job, job_transition_to_pending);
/* If no jobs need manual finalization, automatically do so */
-if (job_txn_apply(txn, job_needs_finalize) == 0) {
+if (job_txn_apply(job, job_needs_finalize) == 0) {
job_do_finalize(job);
}
}
@@ -849,9 +868,10 @@ static void job_completed(Job *job)
static void job_exit(void *opaque)
{
Job *job = (Job *)opaque;
-AioContext *ctx = job->aio_context;
+AioContext *ctx;
-aio_context_acquire(ctx);
+job_ref(job);
+aio_context_acquire(job->aio_context);
/* This is a lie, we're not quiescent, but still doing the completion
* callbacks. However, completion callbacks tend to involve operations that
@@ -862,6 +882,14 @@ static void job_exit(void *opaque)
job_completed(job);
+/*
+ * Note that calling job_completed can move the job to a different
+ * aio_context, so we cannot cache from above. job_txn_apply takes care of
+ * acquiring the new lock, and we ref/unref to avoid job_completed freeing
+ * the job underneath us.
+ */
+ctx = job->aio_context;
+job_unref(job);
aio_context_release(ctx);
}
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 4eeb184caf..7519847912 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -367,7 +367,9 @@ static void test_cancel_concluded(void)
aio_poll(qemu_get_aio_context(), true);
assert(job->status == JOB_STATUS_PENDING);
+aio_context_acquire(job->aio_context);
job_finalize(job, _abort);
+aio_context_release(job->aio_context);
assert(job->status == JOB_STATUS_CONCLUDED);
cancel_common(s);
--
2.26.0
[PATCH v3 0/3] Fix some AIO context locking in jobs
Contains three seperate but related patches cleaning up and fixing some issues regarding aio_context_acquire/aio_context_release for jobs. Mostly affects blockjobs running for devices that have IO threads enabled AFAICT. This is based on the discussions here: https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg07929.html Changes from v2: * reordered patch 1 to the end to not introduce temporary breakages * added more fixes to job txn patch (should now pass the tests) Changes from v1: * fixed commit message for patch 1 * added patches 2 and 3 qemu: Stefan Reiter (3): job: take each job's lock individually in job_txn_apply replication: acquire aio context before calling job_cancel_sync backup: don't acquire aio_context in backup_clean block/backup.c| 4 block/replication.c | 6 +- job.c | 48 ++- tests/test-blockjob.c | 2 ++ 4 files changed, 45 insertions(+), 15 deletions(-) -- 2.26.0
[PATCH v3 2/3] replication: acquire aio context before calling job_cancel_sync
job_cancel_sync requires the job's lock to be held, all other callers
already do this (replication_stop, drive_backup_abort,
blockdev_backup_abort, job_cancel_sync_all, cancel_common).
Signed-off-by: Stefan Reiter
---
block/replication.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/block/replication.c b/block/replication.c
index 413d95407d..6c809cda4e 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -144,12 +144,16 @@ fail:
static void replication_close(BlockDriverState *bs)
{
BDRVReplicationState *s = bs->opaque;
+Job *commit_job = >commit_job->job;
+AioContext *commit_ctx = commit_job->aio_context;
if (s->stage == BLOCK_REPLICATION_RUNNING) {
replication_stop(s->rs, false, NULL);
}
if (s->stage == BLOCK_REPLICATION_FAILOVER) {
-job_cancel_sync(>commit_job->job);
+aio_context_acquire(commit_ctx);
+job_cancel_sync(commit_job);
+aio_context_release(commit_ctx);
}
if (s->mode == REPLICATION_MODE_SECONDARY) {
--
2.26.0
[PATCH v3 3/3] backup: don't acquire aio_context in backup_clean
All code-paths leading to backup_clean (via job_clean) have the job's
context already acquired. The job's context is guaranteed to be the same
as the one used by backup_top via backup_job_create.
Since the previous logic effectively acquired the lock twice, this
broke cleanup of backups for disks using IO threads, since the BDRV_POLL_WHILE
in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock
once, thus deadlocking with the IO thread.
This is a partial revert of 0abf2581717a19.
Signed-off-by: Stefan Reiter
---
With the two previous patches applied, the commit message should now hold true.
block/backup.c | 4
1 file changed, 4 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 7430ca5883..a7a7dcaf4c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -126,11 +126,7 @@ static void backup_abort(Job *job)
static void backup_clean(Job *job)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-AioContext *aio_context = bdrv_get_aio_context(s->backup_top);
-
-aio_context_acquire(aio_context);
bdrv_backup_top_drop(s->backup_top);
-aio_context_release(aio_context);
}
void backup_do_checkpoint(BlockJob *job, Error **errp)
--
2.26.0
[PATCH v2 3/3] replication: acquire aio context before calling job_cancel_sync
job_cancel_sync requires the job's lock to be held, all other callers
already do this (replication_stop, drive_backup_abort,
blockdev_backup_abort, job_cancel_sync_all, cancel_common).
Signed-off-by: Stefan Reiter
---
block/replication.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/block/replication.c b/block/replication.c
index 413d95407d..6c809cda4e 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -144,12 +144,16 @@ fail:
static void replication_close(BlockDriverState *bs)
{
BDRVReplicationState *s = bs->opaque;
+Job *commit_job = >commit_job->job;
+AioContext *commit_ctx = commit_job->aio_context;
if (s->stage == BLOCK_REPLICATION_RUNNING) {
replication_stop(s->rs, false, NULL);
}
if (s->stage == BLOCK_REPLICATION_FAILOVER) {
-job_cancel_sync(>commit_job->job);
+aio_context_acquire(commit_ctx);
+job_cancel_sync(commit_job);
+aio_context_release(commit_ctx);
}
if (s->mode == REPLICATION_MODE_SECONDARY) {
--
2.26.0
[PATCH v2 2/3] job: take each job's lock individually in job_txn_apply
All callers of job_txn_apply hold a single job's lock, but different
jobs within a transaction can have different contexts, thus we need to
lock each one individually before applying the callback function.
Similar to job_completed_txn_abort this also requires releasing the
caller's context before and reacquiring it after to avoid recursive
locks which might break AIO_WAIT_WHILE in the callback.
Signed-off-by: Stefan Reiter
---
job.c | 32
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/job.c b/job.c
index 134a07b92e..e0966162fa 100644
--- a/job.c
+++ b/job.c
@@ -136,17 +136,33 @@ static void job_txn_del_job(Job *job)
}
}
-static int job_txn_apply(JobTxn *txn, int fn(Job *))
+static int job_txn_apply(Job *job, int fn(Job *))
{
-Job *job, *next;
+AioContext *outer_ctx = job->aio_context;
+AioContext *inner_ctx;
+Job *other_job, *next;
+JobTxn *txn = job->txn;
int rc = 0;
-QLIST_FOREACH_SAFE(job, >jobs, txn_list, next) {
-rc = fn(job);
+/*
+ * Similar to job_completed_txn_abort, we take each job's lock before
+ * applying fn, but since we assume that outer_ctx is held by the caller,
+ * we need to release it here to avoid holding the lock twice - which would
+ * break AIO_WAIT_WHILE from within fn.
+ */
+aio_context_release(outer_ctx);
+
+QLIST_FOREACH_SAFE(other_job, >jobs, txn_list, next) {
+inner_ctx = other_job->aio_context;
+aio_context_acquire(inner_ctx);
+rc = fn(other_job);
+aio_context_release(inner_ctx);
if (rc) {
break;
}
}
+
+aio_context_acquire(outer_ctx);
return rc;
}
@@ -774,11 +790,11 @@ static void job_do_finalize(Job *job)
assert(job && job->txn);
/* prepare the transaction to complete */
-rc = job_txn_apply(job->txn, job_prepare);
+rc = job_txn_apply(job, job_prepare);
if (rc) {
job_completed_txn_abort(job);
} else {
-job_txn_apply(job->txn, job_finalize_single);
+job_txn_apply(job, job_finalize_single);
}
}
@@ -824,10 +840,10 @@ static void job_completed_txn_success(Job *job)
assert(other_job->ret == 0);
}
-job_txn_apply(txn, job_transition_to_pending);
+job_txn_apply(job, job_transition_to_pending);
/* If no jobs need manual finalization, automatically do so */
-if (job_txn_apply(txn, job_needs_finalize) == 0) {
+if (job_txn_apply(job, job_needs_finalize) == 0) {
job_do_finalize(job);
}
}
--
2.26.0
[PATCH v2 1/3] backup: don't acquire aio_context in backup_clean
All code-paths leading to backup_clean (via job_clean) have the job's
context already acquired. The job's context is guaranteed to be the same
as the one used by backup_top via backup_job_create.
Since the previous logic effectively acquired the lock twice, this
broke cleanup of backups for disks using IO threads, since the BDRV_POLL_WHILE
in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock
once, thus deadlocking with the IO thread.
This is a partial revert of 0abf2581717a19.
Signed-off-by: Stefan Reiter
---
block/backup.c | 4
1 file changed, 4 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 7430ca5883..a7a7dcaf4c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -126,11 +126,7 @@ static void backup_abort(Job *job)
static void backup_clean(Job *job)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-AioContext *aio_context = bdrv_get_aio_context(s->backup_top);
-
-aio_context_acquire(aio_context);
bdrv_backup_top_drop(s->backup_top);
-aio_context_release(aio_context);
}
void backup_do_checkpoint(BlockJob *job, Error **errp)
--
2.26.0
[PATCH v2 0/3] Fix some AIO context locking in jobs
Contains three seperate but related patches cleaning up and fixing some issues regarding aio_context_acquire/aio_context_release for jobs. Mostly affects blockjobs running for devices that have IO threads enabled AFAICT. This is based on the discussions here: https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg07929.html I *think* the second patch also fixes the hangs on backup abort that I and Dietmar noticed in v1, but I'm not sure, they we're somewhat intermittent before too. Changes from v1: * fixed commit message for patch 1 * added patches 2 and 3 Stefan Reiter (3): backup: don't acquire aio_context in backup_clean job: take each job's lock individually in job_txn_apply replication: acquire aio context before calling job_cancel_sync block/backup.c | 4 block/replication.c | 6 +- job.c | 32 3 files changed, 29 insertions(+), 13 deletions(-) -- 2.26.0
Re: [PATCH] backup: don't acquire aio_context in backup_clean
On 26/03/2020 06:54, Vladimir Sementsov-Ogievskiy wrote:
25.03.2020 18:50, Stefan Reiter wrote:
backup_clean is only ever called as a handler via job_exit, which
Hmm.. I'm afraid it's not quite correct.
job_clean
job_finalize_single
job_completed_txn_abort (lock aio context)
job_do_finalize
Hmm. job_do_finalize calls job_completed_txn_abort, which cares to lock
aio context..
And on the same time, it directaly calls job_txn_apply(job->txn,
job_finalize_single)
without locking. Is it a bug?
I think, as you say, the idea is that job_do_finalize is always called
with the lock acquired. That's why job_completed_txn_abort takes care to
release the lock (at least of the "outer_ctx" as it calls it) before
reacquiring it.
And, even if job_do_finalize called always with locked context, where is
guarantee that all
context of all jobs in txn are locked?
I also don't see anything that guarantees that... I guess it could be
adapted to handle locks like job_completed_txn_abort does?
Haven't looked into transactions too much, but does it even make sense
to have jobs in different contexts in one transaction?
Still, let's look through its callers.
job_finalize
qmp_block_job_finalize (lock aio context)
qmp_job_finalize (lock aio context)
test_cancel_concluded (doesn't lock, but it's a test)
job_completed_txn_success
job_completed
job_exit (lock aio context)
job_cancel
blockdev_mark_auto_del (lock aio context)
job_user_cancel
qmp_block_job_cancel (locks context)
qmp_job_cancel (locks context)
job_cancel_err
job_cancel_sync (return
job_finish_sync(job, _cancel_err, NULL);, job_finish_sync just calls
callback)
replication_close (it's
.bdrv_close.. Hmm, I don't see context locking, where is it ?)
Hm, don't see it either. This might indeed be a way to get to job_clean
without a lock held.
I don't have any testing set up for replication atm, but if you believe
this would be correct I can send a patch for that as well (just acquire
the lock in replication_close before job_cancel_async?).
replication_stop (locks context)
drive_backup_abort (locks context)
blockdev_backup_abort (locks context)
job_cancel_sync_all (locks context)
cancel_common (locks context)
test_* (I don't care)
To clarify, aside from the commit message the patch itself does not
appear to be wrong? All paths (aside from replication_close mentioned
above) guarantee the job lock to be held.
already acquires the job's context. The job's context is guaranteed to
be the same as the one used by backup_top via backup_job_create.
Since the previous logic effectively acquired the lock twice, this
broke cleanup of backups for disks using IO threads, since the
BDRV_POLL_WHILE
in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release
the lock
once, thus deadlocking with the IO thread.
Signed-off-by: Stefan Reiter
Just note, that this thing were recently touched by 0abf2581717a19 , so
add Sergio (its author) to CC.
---
This is a fix for the issue discussed in this part of the thread:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg07639.html
...not the original problem (core dump) posted by Dietmar.
I've still seen it occasionally hang during a backup abort. I'm trying
to figure
out why that happens, stack trace indicates a similar problem with the
main
thread hanging at bdrv_do_drained_begin, though I have no clue why as
of yet.
block/backup.c | 4
1 file changed, 4 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 7430ca5883..a7a7dcaf4c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -126,11 +126,7 @@ static void backup_abort(Job *job)
static void backup_clean(Job *job)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
- AioContext *aio_context = bdrv_get_aio_context(s->backup_top);
-
- aio_context_acquire(aio_context);
bdrv_backup_top_drop(s->backup_top);
- aio_context_release(aio_context);
}
void backup_do_checkpoint(BlockJob *job, Error **errp)
[PATCH] backup: don't acquire aio_context in backup_clean
backup_clean is only ever called as a handler via job_exit, which
already acquires the job's context. The job's context is guaranteed to
be the same as the one used by backup_top via backup_job_create.
Since the previous logic effectively acquired the lock twice, this
broke cleanup of backups for disks using IO threads, since the BDRV_POLL_WHILE
in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock
once, thus deadlocking with the IO thread.
Signed-off-by: Stefan Reiter
---
This is a fix for the issue discussed in this part of the thread:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg07639.html
...not the original problem (core dump) posted by Dietmar.
I've still seen it occasionally hang during a backup abort. I'm trying to figure
out why that happens, stack trace indicates a similar problem with the main
thread hanging at bdrv_do_drained_begin, though I have no clue why as of yet.
block/backup.c | 4
1 file changed, 4 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 7430ca5883..a7a7dcaf4c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -126,11 +126,7 @@ static void backup_abort(Job *job)
static void backup_clean(Job *job)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-AioContext *aio_context = bdrv_get_aio_context(s->backup_top);
-
-aio_context_acquire(aio_context);
bdrv_backup_top_drop(s->backup_top);
-aio_context_release(aio_context);
}
void backup_do_checkpoint(BlockJob *job, Error **errp)
--
2.25.2
Re: backup transaction with io-thread core dumps
On 24/03/2020 17:49, Dietmar Maurer wrote:
A more serious issue is that I also get a hang inside the poll loop
when the VM is under load. For example, running "stress -d 5" inside
the VM (Debian Buster).
Then running a simply drive-backup like:
{ "execute": "drive-backup", "arguments": { "device": "drive-scsi0", "sync": "full",
"target": "backup-scsi0.raw" } }
At the end of the backup, VM hangs, gdb says:
(gdb) bt
#0 0x75cb3916 in __GI_ppoll (fds=0x7fff63d35c40, nfds=2,
timeout=,
timeout@entry=0x0, sigmask=sigmask@entry=0x0) at
../sysdeps/unix/sysv/linux/ppoll.c:39
#1 0x55c5f2c9 in ppoll (__ss=0x0, __timeout=0x0, __nfds=,
__fds=)
at /usr/include/x86_64-linux-gnu/bits/poll2.h:77
#2 0x55c5f2c9 in qemu_poll_ns (fds=, nfds=, timeout=timeout@entry=-1)
at util/qemu-timer.c:335
#3 0x55c61ab1 in fdmon_poll_wait (ctx=0x7fffe8905e80,
ready_list=0x7fffdc68, timeout=-1)
at util/fdmon-poll.c:79
#4 0x55c61097 in aio_poll (ctx=0x7fffe8905e80,
blocking=blocking@entry=true) at util/aio-posix.c:589
#5 0x55bc2243 in bdrv_do_drained_begin
(poll=, ignore_bds_parents=false, parent=0x0,
recursive=false, bs=0x7fff671c11c0) at block/io.c:429
#6 0x55bc2243 in bdrv_do_drained_begin
(bs=0x7fff671c11c0, recursive=, parent=0x0,
ignore_bds_parents=, poll=)
at block/io.c:395
#7 0x55bdd132 in bdrv_backup_top_drop (bs=0x7fff671c11c0) at
block/backup-top.c:273
#8 0x55bd883c in backup_clean (job=0x7fffe5609a00) at
block/backup.c:114
#9 0x55b6d46d in job_clean (job=0x7fffe5609a00) at job.c:657
#10 0x55b6d46d in job_finalize_single (job=0x7fffe5609a00) at job.c:673
#11 0x55b6d46d in job_finalize_single (job=0x7fffe5609a00) at job.c:661
#12 0x55b6dedc in job_txn_apply (txn=, fn=0x55b6d420
) at job.c:145
#13 0x55b6dedc in job_do_finalize (job=0x7fffe5609a00) at job.c:782
#14 0x55b6e131 in job_completed_txn_success (job=0x7fffe5609a00) at
job.c:832
#15 0x55b6e131 in job_completed (job=0x7fffe5609a00) at job.c:845
#16 0x55b6e131 in job_completed (job=0x7fffe5609a00) at job.c:836
#17 0x55b6e190 in job_exit (opaque=0x7fffe5609a00) at job.c:864
#18 0x55c5d855 in aio_bh_call (bh=0x7fffe721a3c0) at util/async.c:164
#19 0x55c5d855 in aio_bh_poll (ctx=ctx@entry=0x7fffe8905e80) at
util/async.c:164
#20 0x55c60ede in aio_dispatch (ctx=0x7fffe8905e80) at
util/aio-posix.c:380
#21 0x55c5d73e in aio_ctx_dispatch (source=, callback=, user_data=)
at util/async.c:298
#22 0x77c7ef2e in g_main_context_dispatch () at
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#23 0x55c60148 in glib_pollfds_poll () at util/main-loop.c:219
#24 0x55c60148 in os_host_main_loop_wait (timeout=) at
util/main-loop.c:242
#25 0x55c60148 in main_loop_wait (nonblocking=nonblocking@entry=0) at
util/main-loop.c:518
#26 0x558fc579 in qemu_main_loop () at
/home/dietmar/pve5-devel/mirror_qemu/softmmu/vl.c:1665
#27 0x55800c2e in main (argc=, argv=,
envp=)
at /home/dietmar/pve5-devel/mirror_qemu/softmmu/main.c:49
The VM uses a similar hardware setup with io-threads as the previous
example.
Is somebody able to reproduce that? And ideas whats wrong here?
Can't speak for the original issue, but this one I can reproduce exactly
as you say: main thread hangs at the end of a backup job of a drive that
uses IO threads, when said drive is under load. Single disk, raw-file
backed.
I've bisected the issue to commit 00e30f05de1d ("block/backup: use
backup-top instead of write notifiers") - CC'ing Vladimir, since he
wrote that one. Before that patch, backup works fine.
For completeness, here's the trace I get on my machine (latest master)
when everything's stuck, Thread 1 is the main thread, 3 the IO thread
for the drive:
Thread 1 (Thread 0x75d20680 (LWP 958009)):
#00x76cf2916 in __GI_ppoll (fds=0x74a0c2c0, nfds=0x2,
timeout=, sigmask=0x0) at
../sysdeps/unix/sysv/linux/ppoll.c:39
#10x55dba2dd in qemu_poll_ns (fds=0x74a0c2c0, nfds=0x2,
timeout=0x) at util/qemu-timer.c:335
#20x55dbdb1d in fdmon_poll_wait (ctx=0x749e3380,
ready_list=0x7fffdab0, timeout=0x) at
util/fdmon-poll.c:79
#30x55dbd48d in aio_poll (ctx=0x749e3380, blocking=0x1)
at util/aio-posix.c:589
#40x55cf5a83 in bdrv_do_drained_begin (bs=0x7fffd1577180,
recursive=0x0, parent=0x0, ignore_bds_parents=0x0, poll=0x1) at
block/io.c:429
#50x55cf5aef in bdrv_drained_begin (bs=0x7fffd1577180) at
block/io.c:435
#60x55d1b135 in bdrv_backup_top_drop (bs=0x7fffd1577180) at
block/backup-top.c:273
#70x55d14734 in backup_clean (job=0x749fba00) at
block/backup.c:132
#80x55c81be5 in job_clean (job=0x749fba00) at job.c:656
#90x55c81c60 in
