Guest reboot issues since QEMU 6.0 and Linux 5.11
Hi, since about half a year ago, we're getting user reports about guest reboot issues with KVM/QEMU[0]. The most common scenario is a Windows Server VM (2012R2/2016/2019, UEFI/OVMF and SeaBIOS) getting stuck during the screen with the Windows logo and the spinning circles after a reboot was triggered from within the guest. Quitting the kvm process and booting with a fresh instance works. The issue seems to become more likely, the longer the kvm instance runs. We did not get such reports while we were providing Linux 5.4 and QEMU 5.2.0, but we do with Linux 5.11/5.13/5.15 and QEMU 6.x. I'm just wondering if anybody has seen this issue before or might have a hunch what it's about? Any tips on what to look out for when debugging are also greatly appreciated! We do have debug access to a user's test VM and the VM state was saved before a problematic reboot, but I can't modify the host system there. AFAICT QEMU just executes guest code as usual, but I'm really not sure what to look out for. That VM has CPU type host, and a colleague did have a similar enough CPU to load the VM state, but for him, the reboot went through normally. On the user's system, it triggers consistently after loading the VM state and rebooting. So unfortunately, we didn't manage to reproduce the issue locally yet. With two other images provided by users, we ran into a boot loop, where QEMU resets the CPUs and does a few KVM_RUNs before the exit reason is KVM_EXIT_SHUTDOWN (which to my understanding indicates a triple fault) and then it repeats. It's not clear if the issues are related. There are also a few reports about non-Windows VMs, mostly Ubuntu 20.04 with UEFI/OVMF, but again, it's not clear if the issues are related. [0]: https://forum.proxmox.com/threads/100744/ (the forum thread is a bit chaotic unfortunately). Best Regards, Fabi
[PATCH v3] block/gluster: correctly set max_pdiscard
On 64-bit platforms, assigning SIZE_MAX to the int64_t max_pdiscard
results in a negative value, and the following assertion would trigger
down the line (it's not the same max_pdiscard, but computed from the
other one):
qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
`max_pdiscard >= bs->bl.request_alignment' failed.
On 32-bit platforms, it's fine to keep using SIZE_MAX.
The assertion in qemu_gluster_co_pdiscard() is checking that the value
of 'bytes' can safely be passed to glfs_discard_async(), which takes a
size_t for the argument in question, so it is kept as is. And since
max_pdiscard is still <= SIZE_MAX, relying on max_pdiscard is still
fine.
Fixes: 0c8022876f ("block: use int64_t instead of int in driver discard
handlers")
Cc: qemu-sta...@nongnu.org
Signed-off-by: Fabian Ebner
---
v2 -> v3:
* Keep assertion in qemu_gluster_co_pdiscard() as is.
* Improve commit message.
v1 -> v2:
* Use an expression that works for both 64-bit and 32-bit platforms.
block/gluster.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/gluster.c b/block/gluster.c
index 398976bc66..b60213ab80 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -891,7 +891,7 @@ out:
static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
{
bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
-bs->bl.max_pdiscard = SIZE_MAX;
+bs->bl.max_pdiscard = MIN(SIZE_MAX, INT64_MAX);
}
static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
--
2.30.2
Re: [PATCH v2] block/gluster: correctly set max_pdiscard
Am 12.05.22 um 18:05 schrieb Stefano Garzarella:
> On Thu, May 12, 2022 at 05:44:13PM +0200, Stefano Garzarella wrote:
>> On Thu, May 12, 2022 at 12:30:48PM +0200, Fabian Ebner wrote:
>>> On 64-bit platforms, SIZE_MAX is too large for max_pdiscard, which is
>>
>> The main problem is that SIZE_MAX for an int64_t is a negative value.
>>
Yes, I should've stated that directly.
>>> int64_t, and the following assertion would be triggered:
>>> qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
>>> `max_pdiscard >= bs->bl.request_alignment' failed.
>>>
>>> Fixes: 0c8022876f ("block: use int64_t instead of int in driver
>>> discard handlers")
>>> Cc: qemu-sta...@nongnu.org
>>> Signed-off-by: Fabian Ebner
>>> ---
>>> block/gluster.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/gluster.c b/block/gluster.c
>>> index 398976bc66..f711bf0bd6 100644
>>> --- a/block/gluster.c
>>> +++ b/block/gluster.c
>>> @@ -891,7 +891,7 @@ out:
>>> static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error
>>> **errp)
>>> {
>>> bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
>>> - bs->bl.max_pdiscard = SIZE_MAX;
>>> + bs->bl.max_pdiscard = MIN(SIZE_MAX, INT64_MAX);
>>
>> What would be the problem if we use INT64_MAX?
>
> Okay, I just saw Eric's answer to v1 and I think this is right.
>
Sorry for not mentioning the changes from v1.
> Please explain it in the commit message and also the initial problem
> that is SIZE_MAX on a 64-bit platform is a negative number for int64_t,
> so the assert fails.
>
I'll try and improve the commit message for v3.
> Thanks,
> Stefano
>
>> (I guess the intention of the original patch was to set the maximum
>> value in drivers that do not have a specific maximum).
>>
>> Or we can set to 0, since in block/io.c we have this code:
>>
>> max_pdiscard = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_pdiscard,
>> INT64_MAX),
>> align);
>> assert(max_pdiscard >= bs->bl.request_alignment);
>>
>> Where `max_pdiscard` is set to INT64_MAX (and aligned) if
>> bs->bl.max_pdiscard is 0.
>>
>>> }
>>>
>>> static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
>>> @@ -1304,7 +1304,7 @@ static coroutine_fn int
>>> qemu_gluster_co_pdiscard(BlockDriverState *bs,
>>> GlusterAIOCB acb;
>>> BDRVGlusterState *s = bs->opaque;
>>>
>>> - assert(bytes <= SIZE_MAX); /* rely on max_pdiscard */
>>> + assert(bytes <= MIN(SIZE_MAX, INT64_MAX)); /* rely on
>>> max_pdiscard */
>>
>> Can we use bs->bl.max_pdiscard directly here?
>>
Now I'm thinking that the assert is actually for checking that the value
can be passed to glfs_discard_async(), which takes a size_t for the
argument in question. So maybe it's best to keep assert(bytes <=
SIZE_MAX) as is?
>> Thanks,
>> Stefano
>
>
>
[PATCH v2] block/gluster: correctly set max_pdiscard
On 64-bit platforms, SIZE_MAX is too large for max_pdiscard, which is
int64_t, and the following assertion would be triggered:
qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
`max_pdiscard >= bs->bl.request_alignment' failed.
Fixes: 0c8022876f ("block: use int64_t instead of int in driver discard
handlers")
Cc: qemu-sta...@nongnu.org
Signed-off-by: Fabian Ebner
---
block/gluster.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 398976bc66..f711bf0bd6 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -891,7 +891,7 @@ out:
static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
{
bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
-bs->bl.max_pdiscard = SIZE_MAX;
+bs->bl.max_pdiscard = MIN(SIZE_MAX, INT64_MAX);
}
static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
@@ -1304,7 +1304,7 @@ static coroutine_fn int
qemu_gluster_co_pdiscard(BlockDriverState *bs,
GlusterAIOCB acb;
BDRVGlusterState *s = bs->opaque;
-assert(bytes <= SIZE_MAX); /* rely on max_pdiscard */
+assert(bytes <= MIN(SIZE_MAX, INT64_MAX)); /* rely on max_pdiscard */
acb.size = 0;
acb.ret = 0;
--
2.30.2
Re: [PATCH] block/gluster: correctly set max_pdiscard which is int64_t
Am 06.05.22 um 17:39 schrieb Eric Blake:
> On Thu, May 05, 2022 at 10:31:24AM +0200, Fabian Ebner wrote:
>> Previously, max_pdiscard would be zero in the following assertion:
>> qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
>> `max_pdiscard >= bs->bl.request_alignment' failed.
>>
>> Fixes: 0c8022876f ("block: use int64_t instead of int in driver discard
>> handlers")
>> Cc: qemu-sta...@nongnu.org
>> Signed-off-by: Fabian Ebner
>> ---
>> block/gluster.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/gluster.c b/block/gluster.c
>> index 398976bc66..592e71b22a 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
>> @@ -891,7 +891,7 @@ out:
>> static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
>> {
>> bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
>> -bs->bl.max_pdiscard = SIZE_MAX;
>> +bs->bl.max_pdiscard = INT64_MAX;
>
> SIZE_MAX is unsigned, but can differ between 32- and 64-bit platforms.
> Blindly setting max_pdiscard to a signed 64-bit value seems wrong if
> glfs_discard_async() takes a size_t and you are on a 32-bit platform.
>
Sorry, I did not consider this.
> Is the real issue that SIZE_MAX on a 64-bit platform is too large,
Yes, there it's too big for max_pdiscard which is int64_t.
> where we want min(SIZE_MAX,INT_MAX) as our real cap?
>
Why not min(SIZE_MAX,INT64_MAX)? Since the constraint is to fit in both
size_t and int64_t. That would also preserve the current value on 32-bit
platforms.
>> }
>>
>> static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
>> @@ -1304,7 +1304,7 @@ static coroutine_fn int
>> qemu_gluster_co_pdiscard(BlockDriverState *bs,
>> GlusterAIOCB acb;
>> BDRVGlusterState *s = bs->opaque;
>>
>> -assert(bytes <= SIZE_MAX); /* rely on max_pdiscard */
>> +assert(bytes <= INT64_MAX); /* rely on max_pdiscard */
>>
>> acb.size = 0;
>> acb.ret = 0;
>> --
>> 2.30.2
>>
>>
>>
>
[PATCH] block/gluster: correctly set max_pdiscard which is int64_t
Previously, max_pdiscard would be zero in the following assertion:
qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
`max_pdiscard >= bs->bl.request_alignment' failed.
Fixes: 0c8022876f ("block: use int64_t instead of int in driver discard
handlers")
Cc: qemu-sta...@nongnu.org
Signed-off-by: Fabian Ebner
---
block/gluster.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 398976bc66..592e71b22a 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -891,7 +891,7 @@ out:
static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
{
bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
-bs->bl.max_pdiscard = SIZE_MAX;
+bs->bl.max_pdiscard = INT64_MAX;
}
static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
@@ -1304,7 +1304,7 @@ static coroutine_fn int
qemu_gluster_co_pdiscard(BlockDriverState *bs,
GlusterAIOCB acb;
BDRVGlusterState *s = bs->opaque;
-assert(bytes <= SIZE_MAX); /* rely on max_pdiscard */
+assert(bytes <= INT64_MAX); /* rely on max_pdiscard */
acb.size = 0;
acb.ret = 0;
--
2.30.2
Re: [PATCH] acpi: Bodge acpi_index migration
Am 05.04.22 um 21:06 schrieb Dr. David Alan Gilbert (git):
> From: "Dr. David Alan Gilbert"
>
> The 'acpi_index' field is a statically configured field, which for
> some reason is migrated; this never makes much sense because it's
> command line static.
>
> However, on piix4 it's conditional, and the condition/test function
> ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> is a macro and not another struct). This means the field is randomly
> loaded/saved based on a random pointer. In 6.x this random pointer
> randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> and trying to load a field that the source didn't send. The migration
> stream gets out of line and hits the section footer.
>
> The bodge is on piix4 never to load the field:
> a) Most 6.x builds never send it, so most of the time the migration
> will work.
> b) We can backport this fix to 6.x to remove the boobytrap.
> c) It should never have made a difference anyway since the acpi-index
> is command line configured and should be correct on the destination
> anyway
> d) ich9 is still sending/receiving this (unconditionally all the time)
> but due to (c) should never notice. We could follow up to make it
> skip.
>
> It worries me just when (a) actually happens.
>
> Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
>
> Signed-off-by: Dr. David Alan Gilbert
> ---
> hw/acpi/acpi-pci-hotplug-stub.c | 4
> hw/acpi/pcihp.c | 6 --
> hw/acpi/piix4.c | 11 ++-
> include/hw/acpi/pcihp.h | 2 --
> 4 files changed, 10 insertions(+), 13 deletions(-)
>
Fixes the issue for me, thanks!
Tested-by: Fabian Ebner
Re: [PATCH v9 3/3] qapi/monitor: allow VNC display id in set/expire_password
Am 25.02.22 um 12:34 schrieb Markus Armbruster:
> Fabian Ebner writes:
>
>> From: Stefan Reiter
>>
>> It is possible to specify more than one VNC server on the command line,
>> either with an explicit ID or the auto-generated ones à la "default",
>> "vnc2", "vnc3", ...
>>
>> It is not possible to change the password on one of these extra VNC
>> displays though. Fix this by adding a "display" parameter to the
>> "set_password" and "expire_password" QMP and HMP commands.
>>
>> For HMP, the display is specified using the "-d" value flag.
>>
>> For QMP, the schema is updated to explicitly express the supported
>> variants of the commands with protocol-discriminated unions.
>>
>> Signed-off-by: Stefan Reiter
>> [FE: update "Since: " from 6.2 to 7.0
>> make @connected a common member of @SetPasswordOptions]
>> Signed-off-by: Fabian Ebner
>
> [...]
>
>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index e112409211..4a13f883a3 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -38,20 +38,47 @@
>>'data': [ 'keep', 'fail', 'disconnect' ] }
>>
>> ##
>> -# @set_password:
>> +# @SetPasswordOptions:
>> #
>> -# Sets the password of a remote display session.
>> +# Options for set_password.
>> #
>> # @protocol: - 'vnc' to modify the VNC server password
>> #- 'spice' to modify the Spice server password
>> #
>> # @password: the new password
>> #
>> -# @connected: how to handle existing clients when changing the
>> -# password. If nothing is specified, defaults to 'keep'
>> -# 'fail' to fail the command if clients are connected
>> -# 'disconnect' to disconnect existing clients
>> -# 'keep' to maintain existing clients
>> +# @connected: How to handle existing clients when changing the
>> +# password. If nothing is specified, defaults to 'keep'.
>> +# For VNC, only 'keep' is currently implemented.
>> +#
>> +# Since: 7.0
>> +#
>> +##
>> +{ 'union': 'SetPasswordOptions',
>> + 'base': { 'protocol': 'DisplayProtocol',
>> +'password': 'str',
>> +'*connected': 'SetPasswordAction' },
>> + 'discriminator': 'protocol',
>> + 'data': { 'vnc': 'SetPasswordOptionsVnc' } }
>> +
>> +##
>> +# @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.
>
> Is this default equivalent to any value? "The first" suggests it's not.
>
The value will be NULL and QTAILQ_FIRST(&vnc_displays) is picked, which
means the display defaults to the first display. But yeah, the value
doesn't actually default to the id of the first display, it just behaves
as if it did.
[PATCH v9 2/3] qapi/monitor: refactor set/expire_password with enums
From: Stefan Reiter
'protocol' and 'connected' are better suited as enums than as strings,
make use of that. No functional change intended.
Suggested-by: Markus Armbruster
Reviewed-by: Markus Armbruster
Signed-off-by: Stefan Reiter
[FE: update "Since: " from 6.2 to 7.0
put 'keep' first in enum to ease use as a default]
Signed-off-by: Fabian Ebner
---
monitor/hmp-cmds.c | 29 +++--
monitor/qmp-cmds.c | 37 -
qapi/ui.json | 36 ++--
3 files changed, 73 insertions(+), 29 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 8c384dc1b2..ff78741b75 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1398,8 +1398,24 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
const char *password = qdict_get_str(qdict, "password");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
+DisplayProtocol proto;
+SetPasswordAction conn;
-qmp_set_password(protocol, password, !!connected, connected, &err);
+proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, &err);
+if (err) {
+goto out;
+}
+
+conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
+ SET_PASSWORD_ACTION_KEEP, &err);
+if (err) {
+goto out;
+}
+
+qmp_set_password(proto, password, !!connected, conn, &err);
+
+out:
hmp_handle_error(mon, err);
}
@@ -1408,8 +1424,17 @@ void hmp_expire_password(Monitor *mon, const QDict
*qdict)
const char *protocol = qdict_get_str(qdict, "protocol");
const char *whenstr = qdict_get_str(qdict, "time");
Error *err = NULL;
+DisplayProtocol proto;
+
+proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, &err);
+if (err) {
+goto out;
+}
-qmp_expire_password(protocol, whenstr, &err);
+qmp_expire_password(proto, whenstr, &err);
+
+out:
hmp_handle_error(mon, err);
}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index db4d186448..b6e8b57fcc 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -168,33 +168,27 @@ void qmp_system_wakeup(Error **errp)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
}
-void qmp_set_password(const char *protocol, const char *password,
- bool has_connected, const char *connected, Error **errp)
+void qmp_set_password(DisplayProtocol protocol, const char *password,
+ bool has_connected, SetPasswordAction connected,
+ Error **errp)
{
int disconnect_if_connected = 0;
int fail_if_connected = 0;
int rc;
if (has_connected) {
-if (strcmp(connected, "fail") == 0) {
-fail_if_connected = 1;
-} else if (strcmp(connected, "disconnect") == 0) {
-disconnect_if_connected = 1;
-} else if (strcmp(connected, "keep") == 0) {
-/* nothing */
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER, "connected");
-return;
-}
+fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL;
+disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT;
}
-if (strcmp(protocol, "spice") == 0) {
+if (protocol == DISPLAY_PROTOCOL_SPICE) {
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_passwd(password, fail_if_connected,
disconnect_if_connected);
-} else if (strcmp(protocol, "vnc") == 0) {
+} else {
+assert(protocol == DISPLAY_PROTOCOL_VNC);
if (fail_if_connected || disconnect_if_connected) {
/* vnc supports "connected=keep" only */
error_setg(errp, QERR_INVALID_PARAMETER, "connected");
@@ -203,10 +197,6 @@ void qmp_set_password(const char *protocol, const char
*password,
/* Note that setting an empty password will not disable login through
* this interface. */
rc = vnc_display_password(NULL, password);
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
- "'vnc' or 'spice'");
-return;
}
if (rc != 0) {
@@ -214,7 +204,7 @@ void qmp_set_password(const char *protocol, const char
*password,
}
}
-void qmp_expire_password(const char *protocol, const char *whenstr,
+void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
Error **errp)
{
time_t when;
@@ -230,17 +220,14 @@ void qmp_expire_password(const char *protocol, const cha
[PATCH v9 0/3] VNC-related HMP/QMP fixes
Original cover letter by Stefan R.: Since the removal of the generic 'qmp_change' command, one can no longer replace the 'default' VNC display listen address at runtime (AFAIK). For our users who need to set up a secondary VNC access port, this means configuring a second VNC display (in addition to our standard one for web-access), but it turns out one cannot set a password on this second display at the moment, as the 'set_password' call only operates on the 'default' display. Additionally, using secret objects, the password is only read once at startup. This could be considered a bug too, but is not touched in this series and left for a later date. v8 -> v9: * use s instead of V to indicate when a flag takes a string parameter * make @connected a common member of @SetPasswordOptions v7 -> v8: * drop last patch deprecating SetPasswordAction values besides 'keep' for VNC (unfortunately, I don't have enough time to try implementing 'disconnect' and 'fail' for VNC in the near future) * drop if conditionals for DisplayProtocol enum to make compilation with --disable-spice and/or --disable-vnc work * order 'keep' first in enum, to fix how patch #3 uses it as an implicit default * also set connected and has_connected for the VNC options in hmp_set_password * fix typo in patch #1 * add missing '#' for description in patch #3 v6 -> v7: * remove g_strdup and g_free, use strings directly * squash in last patch v5 -> v6: * consider feedback from Markus' review, mainly: * fix crash bug in patch 1 (sorry, artifact of patch-splitting) * rely on '!has_param => param == NULL' to shorten code * add note to 'docs/about/deprecated.rst' and touch up comments a bit * go back to g_free instead of qapi_free_* since the latter apparently tries to free the passed in pointer which lives on the stack... * fix bug in HMP parsing (see patch 1) v4 -> v5: * add comment to patch 1 in "monitor-internal.h" * use qapi_free_SetPasswordOptions and friends, don't leak strdups * split QAPI change into 3 seperate patches v3 -> v4: * drop previously patch 1, this was fixed here instead: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02529.html * patch 1: add Eric's R-b * patch 2: remove if-assignment, use 'deprecated' feature in schema v2 -> v3: * refactor QMP schema for set/expire_password as suggested by Eric Blake and Markus Armbruster v1 -> v2: * add Marc-André's R-b on patch 1 * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann * I didn't see a way to do this yet, so I added a "flags with values" arg type Stefan Reiter (3): monitor/hmp: add support for flag argument with value qapi/monitor: refactor set/expire_password with enums qapi/monitor: allow VNC display id in set/expire_password hmp-commands.hx| 24 monitor/hmp-cmds.c | 47 ++- monitor/hmp.c | 19 +- monitor/monitor-internal.h | 3 +- monitor/qmp-cmds.c | 49 +-- qapi/ui.json | 120 +++-- 6 files changed, 194 insertions(+), 68 deletions(-) -- 2.30.2
[PATCH v9 1/3] monitor/hmp: add support for flag argument with value
From: Stefan Reiter
Adds support for the "-xs" parameter type, where "-x" denotes a flag
name and the "s" suffix indicates that this flag is supposed to take
an arbitrary string parameter.
These parameters are always optional, the entry in the qdict will be
omitted if the flag is not given.
Reviewed-by: Dr. David Alan Gilbert
Reviewed-by: Eric Blake
Signed-off-by: Stefan Reiter
[FE: fixed typo pointed out by Eric Blake
use s instead of V to indicate string parameter]
Signed-off-by: Fabian Ebner
---
v8 -> v9:
* Use s rather than V to indicate that the flag takes a value.
monitor/hmp.c | 19 ++-
monitor/monitor-internal.h | 3 ++-
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/monitor/hmp.c b/monitor/hmp.c
index b20737e63c..569066036d 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -981,6 +981,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
{
const char *tmp = p;
int skip_key = 0;
+int ret;
/* option */
c = *typestr++;
@@ -1003,11 +1004,27 @@ static QDict *monitor_parse_arguments(Monitor *mon,
}
if (skip_key) {
p = tmp;
+} else if (*typestr == 's') {
+/* has option with string value */
+typestr++;
+tmp = p++;
+while (qemu_isspace(*p)) {
+p++;
+}
+ret = get_str(buf, sizeof(buf), &p);
+if (ret < 0) {
+monitor_printf(mon, "%s: value expected for -%c\n",
+ cmd->name, *tmp);
+goto fail;
+}
+qdict_put_str(qdict, key, buf);
} else {
-/* has option */
+/* has boolean option */
p++;
qdict_put_bool(qdict, key, true);
}
+} else if (*typestr == 's') {
+typestr++;
}
}
break;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 3da3f86c6a..caa2e90ef2 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -63,7 +63,8 @@
* '.' other form of optional type (for 'i' and 'l')
* 'b' boolean
* user mode accepts "on" or "off"
- * '-' optional parameter (eg. '-f')
+ * '-' optional parameter (eg. '-f'); if followed by a 's', it
+ * specifies an optional string param (e.g. '-fs' allows '-f foo')
*
*/
--
2.30.2
[PATCH v9 3/3] qapi/monitor: allow VNC display id in set/expire_password
From: Stefan Reiter
It is possible to specify more than one VNC server on the command line,
either with an explicit ID or the auto-generated ones à la "default",
"vnc2", "vnc3", ...
It is not possible to change the password on one of these extra VNC
displays though. Fix this by adding a "display" parameter to the
"set_password" and "expire_password" QMP and HMP commands.
For HMP, the display is specified using the "-d" value flag.
For QMP, the schema is updated to explicitly express the supported
variants of the commands with protocol-discriminated unions.
Signed-off-by: Stefan Reiter
[FE: update "Since: " from 6.2 to 7.0
make @connected a common member of @SetPasswordOptions]
Signed-off-by: Fabian Ebner
---
v8 -> v9:
* Make @connected a common member of @SetPasswordOptions.
* Use s rather than V to indicate that the flag takes a string value.
hmp-commands.hx| 24 ++--
monitor/hmp-cmds.c | 40 +--
monitor/qmp-cmds.c | 34 +++-
qapi/ui.json | 96 +++---
4 files changed, 129 insertions(+), 65 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 70a9136ac2..8476277aa9 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 ff78741b75..634968498b 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1396,24 +1396,33 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *password = qdict_get_str(qdict, "password");
+const char *display = qdict_get_try_str(qdict, "display");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
-DisplayProtocol proto;
-SetPasswordAction conn;
-proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-DISPLAY_PROTOCOL_VNC, &err);
+SetPasswordOptions opts = {
+.password = (char *)password,
+.has_connected = !!connected,
+};
+
+opts.connected = qapi_enum_parse(&SetPasswordAction_lookup, connected,
+ SET_PASSWORD_ACTION_KEEP, &err);
if (err) {
goto out;
}
-conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
- SET_PASSWORD_ACTION_KEEP, &err);
+opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, &err);
if (err) {
goto out;
}
-qmp_set_password(proto, password, !!connected, conn, &err);
+if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+opts.u.vnc.has_display = !!display;
+opts.u.vn
Re: [PATCH v8 1/3] monitor/hmp: add support for flag argument with value
Am 09.02.22 um 15:12 schrieb Markus Armbruster: > Fabian Ebner writes: 8< >> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h >> index 3da3f86c6a..a4cb307c8a 100644 >> --- a/monitor/monitor-internal.h >> +++ b/monitor/monitor-internal.h >> @@ -63,7 +63,8 @@ >> * '.' other form of optional type (for 'i' and 'l') >> * 'b' boolean >> * user mode accepts "on" or "off" >> - * '-' optional parameter (eg. '-f') >> + * '-' optional parameter (eg. '-f'); if followed by a 'V', it >> + * specifies an optional string param (e.g. '-fV' allows '-f >> foo') >> * >> */ > > For what it's worth, getopt() uses ':' after the option character for > "takes an argument". > Doing that leads to e.g. .args_type = "protocol:s,password:s,display:-d:,connected:s?", so there's two different kinds of colons now. It's not a problem functionality-wise AFAICT, but it might not be ideal. Should I still go for it? Also, wouldn't future non-string flag parameters need their own letter too? What about re-using 's' here instead? > Happy to make that tweak in my tree. But see my review of PATCH 3 > first. > >
Re: [PATCH v8 3/3] qapi/monitor: allow VNC display id in set/expire_password
Am 09.02.22 um 15:07 schrieb Markus Armbruster:
> Fabian Ebner writes:
>
>> From: Stefan Reiter
>>
>> It is possible to specify more than one VNC server on the command line,
>> either with an explicit ID or the auto-generated ones à la "default",
>> "vnc2", "vnc3", ...
>>
>> It is not possible to change the password on one of these extra VNC
>> displays though. Fix this by adding a "display" parameter to the
>> "set_password" and "expire_password" QMP and HMP commands.
>>
>> For HMP, the display is specified using the "-d" value flag.
>>
>> For QMP, the schema is updated to explicitly express the supported
>> variants of the commands with protocol-discriminated unions.
>>
>> Suggested-by: Markus Armbruster
>
> Did I suggest this feature? I don't remember... Most likely, I merely
> suggested using a union. Mind if I drop this tag?
>
Yes, Stefan might've put the tag because of the suggested approach. I'll
drop it.
>> Signed-off-by: Stefan Reiter
>> [FE: update "Since: " from 6.2 to 7.0
>> set {has_}connected for VNC in hmp_set_password]
>> Signed-off-by: Fabian Ebner
>> ---
>>
>> v7 -> v8:
>> * add missing # in the description for @ExpirePasswordOptions
>> * other changes are already mentioned above
>>
>> hmp-commands.hx| 24 +-
>> monitor/hmp-cmds.c | 39
>> monitor/qmp-cmds.c | 34 ++
>> qapi/ui.json | 110 -
>> 4 files changed, 145 insertions(+), 62 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 70a9136ac2..cc2f4bdeba 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1514,33 +1514,35 @@ ERST
>>
>> {
>> .name = "set_password",
>> -.args_type = "protocol:s,password:s,connected:s?",
>> -.params = "protocol password action-if-connected",
>> +.args_type = "protocol:s,password:s,display:-dV,connected:s?",
>> +.params = "protocol password [-d display]
>> [action-if-connected]",
>> .help = "set spice/vnc password",
>> .cmd= hmp_set_password,
>> },
>>
>> SRST
>> -``set_password [ vnc | spice ] password [ action-if-connected ]``
>> - Change spice/vnc password. *action-if-connected* specifies what
>> - should happen in case a connection is established: *fail* makes the
>> - password change fail. *disconnect* changes the password and
>> +``set_password [ vnc | spice ] password [ -d display ] [
>> action-if-connected ]``
>
> This is the first flag with an argument in HMP. The alternative is
> another optional argument.
>
> PRO optional argument: no need for PATCH 1.
>
> PRO flag with argument: can specify the display without
> action-if-connected.
>
> Dave, this is your call to make.
>
I'll go ahead with v9 once the decision is made.
8<
>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index e112409211..089f05c702 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -38,20 +38,61 @@
>>'data': [ 'keep', 'fail', 'disconnect' ] }
>>
>> ##
>> -# @set_password:
>> +# @SetPasswordOptions:
>> #
>> -# Sets the password of a remote display session.
>> +# General options for set_password.
>
> Actually, all the options there are. Let's drop "General".
>
Ok.
>> #
>> # @protocol: - 'vnc' to modify the VNC server password
>> #- 'spice' to modify the Spice server password
>> #
>> # @password: the new password
>> #
>> -# @connected: how to handle existing clients when changing the
>> -# password. If nothing is specified, defaults to 'keep'
>> -# 'fail' to fail the command if clients are connected
>> -# 'disconnect' to disconnect existing clients
>> -# 'keep' to maintain existing clients
>> +# Since: 7.0
>> +#
>> +##
>> +{ 'union': 'SetPasswordOptions',
>> + 'base': { 'protocol': 'DisplayProtocol',
>> +'password': 'str' },
>> + 'discriminator': 'protocol',
>> + 'data': { 'vnc': 'SetPass
Re: [PATCH 0/4] Make qemu-img dd more flexible
Am 11.02.22 um 17:42 schrieb Hanna Reitz:
> On 11.02.22 17:31, Eric Blake wrote:
>> On Thu, Feb 10, 2022 at 02:31:19PM +0100, Fabian Ebner wrote:
>>> Adds support for reading from stdin and writing to stdout (when raw
>>> format is used), as well as overriding the size of the output and
>>> input image/stream.
>>>
>>> Additionally, the options -n for skipping output image creation and -l
>>> for loading a snapshot are made available like for convert.
>> Without looking at the series itself, I want to refer back to earlier
>> times that someone proposed improving 'qemu-img dd':
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg00636.html
>> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
>>
>> As well as the observation that when we originally allowed 'qemu-img
>> dd' to be added, the end goal was that if 'qemu-img dd' can't operate
>> as a thin wrapper around 'qemu-img convert', then 'qemu-img convert'
>> needs to be made more powerful first. Every time we diverge on what
>> the two uses can do, rather than keeping dd as a thin wrapper, we add
>> to our maintenance burden.
I'm wondering why it's not actually implemented as a thin wrapper then?
The fact that it isn't is (part of) the reason why dd was chosen, as
mentioned in the first patch:
"While dd and convert have overlapping use cases, `dd` is a
simple read/write loop while convert is much more
sophisticated and has ways to dealing with holes and blocks
of zeroes.
Since these typically can't be detected in pipes via
SEEK_DATA/HOLE or skipped while writing, dd seems to be the
better choice for implementing stdin/stdout streams."
Adding the same feature to convert seems much more involved.
>>
>> Sadly, there is a lot of technical debt in this area ('qemu-img dd
>> skip= count=' is STILL broken, more than 4 years after I first
>> proposed a potential patch), where no one has spent the necessary time
>> to improve the situation.
>
> Note that by now (in contrast to 2018), we have FUSE disk exports, and I
> even have a script that uses them to let you run dd on any image:
>
> https://gitlab.com/hreitz/qemu-scripts/-/blob/main/qemu-dd.py
>
> Which is nice, because it gives you feature parity with dd, because it
> simply runs dd.
Thank you for the suggestion. It's definitely worth considering,
although it does add a bit of complexity and we currently don't have
FUSE support enabled in our builds.
>
> (The main problem with the script is that it lives in that personal repo
> of mine and so nobody but me knows about it. Suggestions to improve
> that situation are more than welcome.)
>
> Now, the qemu storage daemon does not support loading qcow2 snapshots
> (as far as I’m aware), which is proposed in patch 4 of this series. But
> I think that just means that it would be nice if the QSD could support
> that.
I suppose adding a snapshot-load QMP command would be a natural way to
add it?
>
> Hanna
>
>
[PATCH 3/4] qemu-img: dd: add -n option (skip target volume creation)
From: Alexandre Derumier
Same rationale as in
b2e10493c7 ("add qemu-img convert -n option (skip target volume creation)")
Originally-by: Alexandre Derumier
Signed-off-by: Thomas Lamprecht
[FE: avoid wrong colon in getopt's optstring
add documentation + commit message]
Signed-off-by: Fabian Ebner
---
docs/tools/qemu-img.rst | 6 +-
qemu-img-cmds.hx| 4 ++--
qemu-img.c | 23 ++-
3 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 43328fe108..9b022d9363 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -210,6 +210,10 @@ Parameters to dd subcommand:
.. program:: qemu-img-dd
+.. option:: -n
+
+ Skip the creation of the target volume
+
.. option:: bs=BLOCK_SIZE
Defines the block size
@@ -496,7 +500,7 @@ Command description:
it doesn't need to be specified separately in this case.
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE] [osize=OUTPUT_SIZE] [if=INPUT]
[of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n]
[bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
dd copies from *INPUT* file (default: STDIN) to *OUTPUT* file (default:
STDOUT) converting it from *FMT* format to *OUTPUT_FMT* format.
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 50993e6c47..97e750623f 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -58,9 +58,9 @@ SRST
ERST
DEF("dd", img_dd,
-"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size]
[count=blocks] [skip=blocks] [isize=input_size] [osize=output_size] [if=input]
[of=output]")
+"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [-n] [bs=block_size]
[count=blocks] [skip=blocks] [isize=input_size] [osize=output_size] [if=input]
[of=output]")
SRST
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE] [osize=OUTPUT_SIZE] [if=INPUT]
[of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n]
[bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
ERST
DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index 630928773d..89bf6fd087 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4944,7 +4944,7 @@ static int img_dd(int argc, char **argv)
const char *fmt = NULL;
int64_t size = 0, readsize = 0;
int64_t block_count = 0, out_pos, in_pos;
-bool force_share = false;
+bool force_share = false, skip_create = false;
struct DdInfo dd = {
.flags = 0,
.count = 0,
@@ -4982,7 +4982,7 @@ static int img_dd(int argc, char **argv)
{ 0, 0, 0, 0 }
};
-while ((c = getopt_long(argc, argv, ":hf:O:U", long_options, NULL))) {
+while ((c = getopt_long(argc, argv, ":hf:O:Un", long_options, NULL))) {
if (c == EOF) {
break;
}
@@ -5002,6 +5002,9 @@ static int img_dd(int argc, char **argv)
case 'h':
help();
break;
+case 'n':
+skip_create = true;
+break;
case 'U':
force_share = true;
break;
@@ -5144,13 +5147,15 @@ static int img_dd(int argc, char **argv)
size - in.bsz * in.offset, &error_abort);
}
-ret = bdrv_create(drv, out.filename, opts, &local_err);
-if (ret < 0) {
-error_reportf_err(local_err,
- "%s: error while creating output image: ",
- out.filename);
-ret = -1;
-goto out;
+if (!skip_create) {
+ret = bdrv_create(drv, out.filename, opts, &local_err);
+if (ret < 0) {
+error_reportf_err(local_err,
+ "%s: error while creating output image: ",
+ out.filename);
+ret = -1;
+goto out;
+}
}
/* TODO, we can't honour --image-opts for the target,
--
2.30.2
[PATCH 2/4] qemu-img: dd: add isize parameter
From: Wolfgang Bumiller
for writing small images from stdin to bigger ones.
In order to distinguish between an actually unexpected and
an expected end of input.
Signed-off-by: Wolfgang Bumiller
Signed-off-by: Thomas Lamprecht
[FE: override size earlier
use flag to detect parameter
add documenation]
Signed-off-by: Fabian Ebner
---
docs/tools/qemu-img.rst | 10 --
qemu-img-cmds.hx| 4 ++--
qemu-img.c | 24 +++-
3 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 775eaf3097..43328fe108 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -230,6 +230,10 @@ Parameters to dd subcommand:
Sets the number of input blocks to skip
+.. option:: isize=INPUT_SIZE
+
+ Treat the input image or stream as if it had this size
+
.. option:: osize=OUTPUT_SIZE
Sets the output image's size
@@ -492,7 +496,7 @@ Command description:
it doesn't need to be specified separately in this case.
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE] [osize=OUTPUT_SIZE] [if=INPUT]
[of=OUTPUT]
dd copies from *INPUT* file (default: STDIN) to *OUTPUT* file (default:
STDOUT) converting it from *FMT* format to *OUTPUT_FMT* format.
@@ -504,7 +508,9 @@ Command description:
The size syntax is similar to :manpage:`dd(1)`'s size syntax.
The output image will be created with size *OUTPUT_SIZE* and at most this
many
- bytes will be copied.
+ bytes will be copied. When *INPUT_SIZE* is positive, it overrides the input
+ image's size for the copy operation. When *INPUT_SIZE* is zero and reading
+ from STDIN, do not treat premature end of the input stream as an error.
.. option:: info [--object OBJECTDEF] [--image-opts] [-f FMT] [--output=OFMT]
[--backing-chain] [-U] FILENAME
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index e4935365c9..50993e6c47 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -58,9 +58,9 @@ SRST
ERST
DEF("dd", img_dd,
-"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size]
[count=blocks] [skip=blocks] [osize=output_size] [if=input] [of=output]")
+"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size]
[count=blocks] [skip=blocks] [isize=input_size] [osize=output_size] [if=input]
[of=output]")
SRST
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE] [osize=OUTPUT_SIZE] [if=INPUT]
[of=OUTPUT]
ERST
DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index ea488fd190..630928773d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4820,11 +4820,13 @@ static int img_bitmap(int argc, char **argv)
#define C_OF 010
#define C_SKIP020
#define C_OSIZE 040
+#define C_ISIZE 0100
struct DdInfo {
unsigned int flags;
int64_t count;
int64_t osize;
+int64_t isize;
};
struct DdIo {
@@ -4913,6 +4915,19 @@ static int img_dd_osize(const char *arg,
return 0;
}
+static int img_dd_isize(const char *arg,
+struct DdIo *in, struct DdIo *out,
+struct DdInfo *dd)
+{
+dd->isize = cvtnum("isize", arg);
+
+if (dd->isize < 0) {
+return 1;
+}
+
+return 0;
+}
+
static int img_dd(int argc, char **argv)
{
int ret = 0;
@@ -4934,6 +4949,7 @@ static int img_dd(int argc, char **argv)
.flags = 0,
.count = 0,
.osize = 0,
+.isize = 0,
};
struct DdIo in = {
.bsz = 512, /* Block size is by default 512 bytes */
@@ -4955,6 +4971,7 @@ static int img_dd(int argc, char **argv)
{ "of", img_dd_of, C_OF },
{ "skip", img_dd_skip, C_SKIP },
{ "osize", img_dd_osize, C_OSIZE },
+{ "isize", img_dd_isize, C_ISIZE },
{ NULL, NULL, 0 }
};
const struct option long_options[] = {
@@ -5061,7 +5078,9 @@ static int img_dd(int argc, char **argv)
}
}
-if (dd.flags & C_IF) {
+if (dd.flags & C_ISIZE && dd.isize > 0) {
+size = dd.isize;
+} else if (dd.flags & C_IF) {
size = blk_getlength(blk1);
if (size < 0) {
error_report("Failed to get size for '%s'", in.filename);
@@ -5174,6 +5193,9 @@ static int img_dd(int argc, char **argv)
} else {
in_ret = read(STDIN_FILENO, in.buf, in_bsz);
if (in_ret == 0)
[PATCH 1/4] qemu-img: dd: add osize and read from/to stdin/stdout
From: Wolfgang Bumiller
Neither convert nor dd were previously able to write to or
read from a pipe. Particularly serializing an image file
into a raw stream or vice versa can be useful, but using
`qemu-img convert -f qcow2 -O raw foo.qcow2 /dev/stdout` in
a pipe will fail trying to seek.
While dd and convert have overlapping use cases, `dd` is a
simple read/write loop while convert is much more
sophisticated and has ways to dealing with holes and blocks
of zeroes.
Since these typically can't be detected in pipes via
SEEK_DATA/HOLE or skipped while writing, dd seems to be the
better choice for implementing stdin/stdout streams.
This patch causes "if" and "of" to default to stdin and
stdout respectively, allowing only the "raw" format to be
used in these cases.
Since the input can now be a pipe we have no way of
detecting the size of the output image to create. Since we
also want to support images with a size not matching the
dd command's "bs" parameter (which, together with "count"
could be used to calculate the desired size, and is already
used to limit it), the "osize" option is added to explicitly
override the output file's size.
Signed-off-by: Wolfgang Bumiller
Signed-off-by: Thomas Lamprecht
[FE: add documentation
avoid error when osize is larger than input image's size
fail if both count and osize are specified
fail if skip is specified when reading from stdin]
Signed-off-by: Fabian Ebner
---
docs/tools/qemu-img.rst | 17 +++-
qemu-img-cmds.hx| 4 +-
qemu-img.c | 201 ++--
3 files changed, 146 insertions(+), 76 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 8885ea11cf..775eaf3097 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -220,16 +220,20 @@ Parameters to dd subcommand:
.. option:: if=INPUT
- Sets the input file
+ Sets the input file (defaults to STDIN)
.. option:: of=OUTPUT
- Sets the output file
+ Sets the output file (defaults to STDOUT)
.. option:: skip=BLOCKS
Sets the number of input blocks to skip
+.. option:: osize=OUTPUT_SIZE
+
+ Sets the output image's size
+
Parameters to snapshot subcommand:
.. program:: qemu-img-snapshot
@@ -488,10 +492,10 @@ Command description:
it doesn't need to be specified separately in this case.
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] if=INPUT of=OUTPUT
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
- dd copies from *INPUT* file to *OUTPUT* file converting it from
- *FMT* format to *OUTPUT_FMT* format.
+ dd copies from *INPUT* file (default: STDIN) to *OUTPUT* file (default:
+ STDOUT) converting it from *FMT* format to *OUTPUT_FMT* format.
The data is by default read and written using blocks of 512 bytes but can be
modified by specifying *BLOCK_SIZE*. If count=\ *BLOCKS* is specified
@@ -499,6 +503,9 @@ Command description:
The size syntax is similar to :manpage:`dd(1)`'s size syntax.
+ The output image will be created with size *OUTPUT_SIZE* and at most this
many
+ bytes will be copied.
+
.. option:: info [--object OBJECTDEF] [--image-opts] [-f FMT] [--output=OFMT]
[--backing-chain] [-U] FILENAME
Give information about the disk image *FILENAME*. Use it in
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1b1dab5b17..e4935365c9 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -58,9 +58,9 @@ SRST
ERST
DEF("dd", img_dd,
-"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size]
[count=blocks] [skip=blocks] if=input of=output")
+"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size]
[count=blocks] [skip=blocks] [osize=output_size] [if=input] [of=output]")
SRST
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] if=INPUT of=OUTPUT
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
ERST
DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index 6fe2466032..ea488fd190 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4819,10 +4819,12 @@ static int img_bitmap(int argc, char **argv)
#define C_IF 04
#define C_OF 010
#define C_SKIP020
+#define C_OSIZE 040
struct DdInfo {
unsigned int flags;
int64_t count;
+int64_t osize;
};
struct DdIo {
@@ -4898,6 +4900,19 @@ static int img_dd_skip(const char *arg,
return 0;
}
+static int img_dd_osize(const char *arg,
+struct DdIo *in, struct DdIo *out,
+struct DdInfo *dd)
+{
+dd->osize = cvtnum("osize", arg);
+
+if (
[PATCH 4/4] qemu-img: dd: add -l option for loading a snapshot
Signed-off-by: Fabian Ebner
---
docs/tools/qemu-img.rst | 7 ---
qemu-img-cmds.hx| 4 ++--
qemu-img.c | 33 +++--
3 files changed, 37 insertions(+), 7 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 9b022d9363..b2333d7b04 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -500,10 +500,11 @@ Command description:
it doesn't need to be specified separately in this case.
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n]
[bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n] [-l
SNAPSHOT_PARAM] [bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
- dd copies from *INPUT* file (default: STDIN) to *OUTPUT* file (default:
- STDOUT) converting it from *FMT* format to *OUTPUT_FMT* format.
+ dd copies from *INPUT* file (default: STDIN) or snapshot *SNAPSHOT_PARAM* to
+ *OUTPUT* file (default: STDOUT) converting it from *FMT* format to
+ *OUTPUT_FMT* format.
The data is by default read and written using blocks of 512 bytes but can be
modified by specifying *BLOCK_SIZE*. If count=\ *BLOCKS* is specified
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 97e750623f..2f527306b0 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -58,9 +58,9 @@ SRST
ERST
DEF("dd", img_dd,
-"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [-n] [bs=block_size]
[count=blocks] [skip=blocks] [isize=input_size] [osize=output_size] [if=input]
[of=output]")
+"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [-n] [-l snapshot_param]
[bs=block_size] [count=blocks] [skip=blocks] [isize=input_size]
[osize=output_size] [if=input] [of=output]")
SRST
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n]
[bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n] [-l
SNAPSHOT_PARAM] [bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
ERST
DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index 89bf6fd087..28b6430800 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4936,6 +4936,7 @@ static int img_dd(int argc, char **argv)
BlockDriver *drv = NULL, *proto_drv = NULL;
BlockBackend *blk1 = NULL, *blk2 = NULL;
QemuOpts *opts = NULL;
+QemuOpts *sn_opts = NULL;
QemuOptsList *create_opts = NULL;
Error *local_err = NULL;
bool image_opts = false;
@@ -4945,6 +4946,7 @@ static int img_dd(int argc, char **argv)
int64_t size = 0, readsize = 0;
int64_t block_count = 0, out_pos, in_pos;
bool force_share = false, skip_create = false;
+const char *snapshot_name = NULL;
struct DdInfo dd = {
.flags = 0,
.count = 0,
@@ -4982,7 +4984,7 @@ static int img_dd(int argc, char **argv)
{ 0, 0, 0, 0 }
};
-while ((c = getopt_long(argc, argv, ":hf:O:Un", long_options, NULL))) {
+while ((c = getopt_long(argc, argv, ":hf:O:l:Un", long_options, NULL))) {
if (c == EOF) {
break;
}
@@ -5005,6 +5007,19 @@ static int img_dd(int argc, char **argv)
case 'n':
skip_create = true;
break;
+case 'l':
+if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
+sn_opts = qemu_opts_parse_noisily(&internal_snapshot_opts,
+ optarg, false);
+if (!sn_opts) {
+error_report("Failed in parsing snapshot param '%s'",
+ optarg);
+goto out;
+}
+} else {
+snapshot_name = optarg;
+}
+break;
case 'U':
force_share = true;
break;
@@ -5074,11 +5089,24 @@ static int img_dd(int argc, char **argv)
if (dd.flags & C_IF) {
blk1 = img_open(image_opts, in.filename, fmt, 0, false, false,
force_share);
-
if (!blk1) {
ret = -1;
goto out;
}
+if (sn_opts) {
+bdrv_snapshot_load_tmp(blk_bs(blk1),
+ qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
+ qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
+ &local_err);
+} else if (snapshot_name != NULL) {
+bdrv_snapshot_load_tmp_by_id_or_name(blk_bs(blk1), snapshot_name,
+ &local_err);
+}
+if (local_err) {
+error_repor
[PATCH 0/4] Make qemu-img dd more flexible
Adds support for reading from stdin and writing to stdout (when raw format is used), as well as overriding the size of the output and input image/stream. Additionally, the options -n for skipping output image creation and -l for loading a snapshot are made available like for convert. Alexandre Derumier (1): qemu-img: dd: add -n option (skip target volume creation) Fabian Ebner (1): qemu-img: dd: add -l option for loading a snapshot Wolfgang Bumiller (2): qemu-img: dd: add osize and read from/to stdin/stdout qemu-img: dd: add isize parameter docs/tools/qemu-img.rst | 28 - qemu-img-cmds.hx| 4 +- qemu-img.c | 261 +--- 3 files changed, 215 insertions(+), 78 deletions(-) -- 2.30.2
[PATCH v8 3/3] qapi/monitor: allow VNC display id in set/expire_password
From: Stefan Reiter
It is possible to specify more than one VNC server on the command line,
either with an explicit ID or the auto-generated ones à la "default",
"vnc2", "vnc3", ...
It is not possible to change the password on one of these extra VNC
displays though. Fix this by adding a "display" parameter to the
"set_password" and "expire_password" QMP and HMP commands.
For HMP, the display is specified using the "-d" value flag.
For QMP, the schema is updated to explicitly express the supported
variants of the commands with protocol-discriminated unions.
Suggested-by: Markus Armbruster
Signed-off-by: Stefan Reiter
[FE: update "Since: " from 6.2 to 7.0
set {has_}connected for VNC in hmp_set_password]
Signed-off-by: Fabian Ebner
---
v7 -> v8:
* add missing # in the description for @ExpirePasswordOptions
* other changes are already mentioned above
hmp-commands.hx| 24 +-
monitor/hmp-cmds.c | 39
monitor/qmp-cmds.c | 34 ++
qapi/ui.json | 110 -
4 files changed, 145 insertions(+), 62 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 70a9136ac2..cc2f4bdeba 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1514,33 +1514,35 @@ ERST
{
.name = "set_password",
-.args_type = "protocol:s,password:s,connected:s?",
-.params = "protocol password action-if-connected",
+.args_type = "protocol:s,password:s,display:-dV,connected:s?",
+.params = "protocol password [-d display] [action-if-connected]",
.help = "set spice/vnc password",
.cmd= hmp_set_password,
},
SRST
-``set_password [ vnc | spice ] password [ action-if-connected ]``
- Change spice/vnc password. *action-if-connected* specifies what
- should happen in case a connection is established: *fail* makes the
- password change fail. *disconnect* changes the password and
+``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected
]``
+ Change spice/vnc password. *display* can be used with 'vnc' to specify
+ which display to set the password on. *action-if-connected* specifies
+ what should happen in case a connection is established: *fail* makes
+ the password change fail. *disconnect* changes the password and
disconnects the client. *keep* changes the password and keeps the
connection up. *keep* is the default.
ERST
{
.name = "expire_password",
-.args_type = "protocol:s,time:s",
-.params = "protocol time",
+.args_type = "protocol:s,time:s,display:-dV",
+.params = "protocol time [-d display]",
.help = "set spice/vnc password expire-time",
.cmd= hmp_expire_password,
},
SRST
-``expire_password [ vnc | spice ]`` *expire-time*
- Specify when a password for spice/vnc becomes
- invalid. *expire-time* accepts:
+``expire_password [ vnc | spice ] expire-time [ -d display ]``
+ Specify when a password for spice/vnc becomes invalid.
+ *display* behaves the same as in ``set_password``.
+ *expire-time* accepts:
``now``
Invalidate password instantly.
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ff78741b75..be0d919b64 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1396,13 +1396,17 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *password = qdict_get_str(qdict, "password");
+const char *display = qdict_get_try_str(qdict, "display");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
-DisplayProtocol proto;
SetPasswordAction conn;
-proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-DISPLAY_PROTOCOL_VNC, &err);
+SetPasswordOptions opts = {
+.password = (char *)password,
+};
+
+opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, &err);
if (err) {
goto out;
}
@@ -1413,7 +1417,17 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
goto out;
}
-qmp_set_password(proto, password, !!connected, conn, &err);
+if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+opts.u.vnc.has_display = !!display;
+opts.u.vnc.display = (char *)display;
+opts.u.vnc.has_connected = !!connected;
+opts.u.vnc.connected = conn;
+} else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
+opts.u.spice.has_connected = !!connected;
+opts.u.spice.connected = conn;
+
[PATCH v8 2/3] qapi/monitor: refactor set/expire_password with enums
From: Stefan Reiter
'protocol' and 'connected' are better suited as enums than as strings,
make use of that. No functional change intended.
Suggested-by: Markus Armbruster
Signed-off-by: Stefan Reiter
[FE: update "Since: " from 6.2 to 7.0
put 'keep' first in enum to ease use as a default]
Signed-off-by: Fabian Ebner
---
v7 -> v8:
* drop if conditionals for DisplayProtocol enum, so compilation with
--disable-{spice,vnc} works
monitor/hmp-cmds.c | 29 +++--
monitor/qmp-cmds.c | 37 -
qapi/ui.json | 36 ++--
3 files changed, 73 insertions(+), 29 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 8c384dc1b2..ff78741b75 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1398,8 +1398,24 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
const char *password = qdict_get_str(qdict, "password");
const char *connected = qdict_get_try_str(qdict, "connected");
Error *err = NULL;
+DisplayProtocol proto;
+SetPasswordAction conn;
-qmp_set_password(protocol, password, !!connected, connected, &err);
+proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, &err);
+if (err) {
+goto out;
+}
+
+conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
+ SET_PASSWORD_ACTION_KEEP, &err);
+if (err) {
+goto out;
+}
+
+qmp_set_password(proto, password, !!connected, conn, &err);
+
+out:
hmp_handle_error(mon, err);
}
@@ -1408,8 +1424,17 @@ void hmp_expire_password(Monitor *mon, const QDict
*qdict)
const char *protocol = qdict_get_str(qdict, "protocol");
const char *whenstr = qdict_get_str(qdict, "time");
Error *err = NULL;
+DisplayProtocol proto;
+
+proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, &err);
+if (err) {
+goto out;
+}
-qmp_expire_password(protocol, whenstr, &err);
+qmp_expire_password(proto, whenstr, &err);
+
+out:
hmp_handle_error(mon, err);
}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index db4d186448..b6e8b57fcc 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -168,33 +168,27 @@ void qmp_system_wakeup(Error **errp)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
}
-void qmp_set_password(const char *protocol, const char *password,
- bool has_connected, const char *connected, Error **errp)
+void qmp_set_password(DisplayProtocol protocol, const char *password,
+ bool has_connected, SetPasswordAction connected,
+ Error **errp)
{
int disconnect_if_connected = 0;
int fail_if_connected = 0;
int rc;
if (has_connected) {
-if (strcmp(connected, "fail") == 0) {
-fail_if_connected = 1;
-} else if (strcmp(connected, "disconnect") == 0) {
-disconnect_if_connected = 1;
-} else if (strcmp(connected, "keep") == 0) {
-/* nothing */
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER, "connected");
-return;
-}
+fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL;
+disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT;
}
-if (strcmp(protocol, "spice") == 0) {
+if (protocol == DISPLAY_PROTOCOL_SPICE) {
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_passwd(password, fail_if_connected,
disconnect_if_connected);
-} else if (strcmp(protocol, "vnc") == 0) {
+} else {
+assert(protocol == DISPLAY_PROTOCOL_VNC);
if (fail_if_connected || disconnect_if_connected) {
/* vnc supports "connected=keep" only */
error_setg(errp, QERR_INVALID_PARAMETER, "connected");
@@ -203,10 +197,6 @@ void qmp_set_password(const char *protocol, const char
*password,
/* Note that setting an empty password will not disable login through
* this interface. */
rc = vnc_display_password(NULL, password);
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
- "'vnc' or 'spice'");
-return;
}
if (rc != 0) {
@@ -214,7 +204,7 @@ void qmp_set_password(const char *protocol, const char
*password,
}
}
-void qmp_expire_password(const char *protocol, const char *whenstr,
+void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
Error **errp)
{
t
[PATCH v8 1/3] monitor/hmp: add support for flag argument with value
From: Stefan Reiter
Adds support for the "-xV" parameter type, where "-x" denotes a flag
name and the "V" suffix indicates that this flag is supposed to take
an arbitrary string parameter.
These parameters are always optional, the entry in the qdict will be
omitted if the flag is not given.
Reviewed-by: Dr. David Alan Gilbert
Reviewed-by: Eric Blake
Signed-off-by: Stefan Reiter
[FE: fixed typo pointed out by Eric Blake]
Signed-off-by: Fabian Ebner
---
monitor/hmp.c | 19 ++-
monitor/monitor-internal.h | 3 ++-
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/monitor/hmp.c b/monitor/hmp.c
index b20737e63c..fd4f5866c7 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -981,6 +981,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
{
const char *tmp = p;
int skip_key = 0;
+int ret;
/* option */
c = *typestr++;
@@ -1003,11 +1004,27 @@ static QDict *monitor_parse_arguments(Monitor *mon,
}
if (skip_key) {
p = tmp;
+} else if (*typestr == 'V') {
+/* has option with string value */
+typestr++;
+tmp = p++;
+while (qemu_isspace(*p)) {
+p++;
+}
+ret = get_str(buf, sizeof(buf), &p);
+if (ret < 0) {
+monitor_printf(mon, "%s: value expected for -%c\n",
+ cmd->name, *tmp);
+goto fail;
+}
+qdict_put_str(qdict, key, buf);
} else {
-/* has option */
+/* has boolean option */
p++;
qdict_put_bool(qdict, key, true);
}
+} else if (*typestr == 'V') {
+typestr++;
}
}
break;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 3da3f86c6a..a4cb307c8a 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -63,7 +63,8 @@
* '.' other form of optional type (for 'i' and 'l')
* 'b' boolean
* user mode accepts "on" or "off"
- * '-' optional parameter (eg. '-f')
+ * '-' optional parameter (eg. '-f'); if followed by a 'V', it
+ * specifies an optional string param (e.g. '-fV' allows '-f foo')
*
*/
--
2.30.2
[PATCH v8 0/3] VNC-related HMP/QMP fixes
Original cover letter by Stefan R.: Since the removal of the generic 'qmp_change' command, one can no longer replace the 'default' VNC display listen address at runtime (AFAIK). For our users who need to set up a secondary VNC access port, this means configuring a second VNC display (in addition to our standard one for web-access), but it turns out one cannot set a password on this second display at the moment, as the 'set_password' call only operates on the 'default' display. Additionally, using secret objects, the password is only read once at startup. This could be considered a bug too, but is not touched in this series and left for a later date. v7 -> v8: * drop last patch deprecating SetPasswordAction values besides 'keep' for VNC (unfortunately, I don't have enough time to try implementing 'disconnect' and 'fail' for VNC in the near future) * drop if conditionals for DisplayProtocol enum to make compilation with --disable-spice and/or --disable-vnc work * order 'keep' first in enum, to fix how patch #3 uses it as an implicit default * also set connected and has_connected for the VNC options in hmp_set_password * fix typo in patch #1 * add missing '#' for description in patch #3 v6 -> v7: * remove g_strdup and g_free, use strings directly * squash in last patch v5 -> v6: * consider feedback from Markus' review, mainly: * fix crash bug in patch 1 (sorry, artifact of patch-splitting) * rely on '!has_param => param == NULL' to shorten code * add note to 'docs/about/deprecated.rst' and touch up comments a bit * go back to g_free instead of qapi_free_* since the latter apparently tries to free the passed in pointer which lives on the stack... * fix bug in HMP parsing (see patch 1) v4 -> v5: * add comment to patch 1 in "monitor-internal.h" * use qapi_free_SetPasswordOptions and friends, don't leak strdups * split QAPI change into 3 seperate patches v3 -> v4: * drop previously patch 1, this was fixed here instead: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02529.html * patch 1: add Eric's R-b * patch 2: remove if-assignment, use 'deprecated' feature in schema v2 -> v3: * refactor QMP schema for set/expire_password as suggested by Eric Blake and Markus Armbruster v1 -> v2: * add Marc-André's R-b on patch 1 * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann * I didn't see a way to do this yet, so I added a "flags with values" arg type Stefan Reiter (3): monitor/hmp: add support for flag argument with value qapi/monitor: refactor set/expire_password with enums qapi/monitor: allow VNC display id in set/expire_password hmp-commands.hx| 24 --- monitor/hmp-cmds.c | 52 +- monitor/hmp.c | 19 +- monitor/monitor-internal.h | 3 +- monitor/qmp-cmds.c | 49 -- qapi/ui.json | 134 - 6 files changed, 213 insertions(+), 68 deletions(-) -- 2.30.2
Re: [PULL 18/20] block/nbd: drop connection_co
Am 02.02.22 um 15:21 schrieb Hanna Reitz:
> On 02.02.22 14:53, Eric Blake wrote:
>> On Wed, Feb 02, 2022 at 12:49:36PM +0100, Fabian Ebner wrote:
>>> Am 27.09.21 um 23:55 schrieb Eric Blake:
>>>> From: Vladimir Sementsov-Ogievskiy
>>>>
>>>> OK, that's a big rewrite of the logic.
>>>>
>>>> Pre-patch we have an always running coroutine - connection_co. It does
>>>> reply receiving and reconnecting. And it leads to a lot of difficult
>>>> and unobvious code around drained sections and context switch. We also
>>>> abuse bs->in_flight counter which is increased for connection_co and
>>>> temporary decreased in points where we want to allow drained section to
>>>> begin. One of these place is in another file: in nbd_read_eof() in
>>>> nbd/client.c.
>>>>
>>>> We also cancel reconnect and requests waiting for reconnect on drained
>>>> begin which is not correct. And this patch fixes that.
>>>>
>>>> Let's finally drop this always running coroutine and go another way:
>>>> do both reconnect and receiving in request coroutines.
>>>>
>>> Hi,
>>>
>>> while updating our stack to 6.2, one of our live-migration tests stopped
>>> working (backtrace is below) and bisecting led me to this patch.
>>>
>>> The VM has a single qcow2 disk (converting to raw doesn't make a
>>> difference) and the issue only appears when using iothread (for both
>>> virtio-scsi-pci and virtio-block-pci).
>>>
>>> Reverting 1af7737871fb3b66036f5e520acb0a98fc2605f7 (which lives on top)
>>> and 4ddb5d2fde6f22b2cf65f314107e890a7ca14fcf (the commit corresponding
>>> to this patch) in v6.2.0 makes the migration work again.
>>>
>>> Backtrace:
>>>
>>> Thread 1 (Thread 0x7f9d93458fc0 (LWP 56711) "kvm"):
>>> #0 __GI_raise (sig=sig@entry=6) at
>>> ../sysdeps/unix/sysv/linux/raise.c:50
>>> #1 0x7f9d9d6bc537 in __GI_abort () at abort.c:79
>>> #2 0x7f9d9d6bc40f in __assert_fail_base (fmt=0x7f9d9d825128
>>> "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5579153763f8
>>> "qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)",
>>> file=0x5579153764f9 "../io/channel.c", line=483, function=>> out>) at assert.c:92
>> Given that this assertion is about which aio context is set, I wonder
>> if the conversation at
>> https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg00096.html is
>> relevant; if so, Vladimir may already be working on the patch.
>
> It should be exactly that patch:
>
> https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06222.html
>
> (From the discussion it appears that for v1 I need to ensure the
> reconnection timer is deleted immediately once reconnecting succeeds,
> and then that should be good to move out of the RFC state.)
Thanks for the quick responses and happy to hear you're already working
on it! With the RFC, the issue is gone for me.
>
> Basically, I expect qemu to crash every time that you try to use an NBD
> block device in an I/O thread (unless you don’t do any I/O), for example
> this is the simplest reproducer I know of:
>
> $ qemu-nbd --fork -k /tmp/nbd.sock -f raw null-co://
>
> $ qemu-system-x86_64 \
> -object iothread,id=iothr0 \
> -device virtio-scsi,id=vscsi,iothread=iothr0 \
> -blockdev '{
> "driver": "nbd",
> "node-name": "nbd",
> "server": {
> "type": "unix",
> "path": "/tmp/nbd.sock"
> } }' \
> -device scsi-hd,bus=vscsi.0,drive=nbd
> qemu-system-x86_64: ../qemu-6.2.0/io/channel.c:483:
> qio_channel_restart_read: Assertion `qemu_get_current_aio_context() ==
> qemu_coroutine_get_aio_context(co)' failed.
> qemu-nbd: Disconnect client, due to: Unable to read from socket:
> Connection reset by peer
> [1] 108747 abort (core dumped) qemu-system-x86_64 -object
> iothread,id=iothr0 -device -blockdev -device
>
>
Interestingly, the reproducer didn't crash the very first time I tried
it. I did get the same error after ^C-ing though, and on subsequent
tries it mostly crashed immediately, but very occasionally it didn't.
Re: [PULL 18/20] block/nbd: drop connection_co
Am 27.09.21 um 23:55 schrieb Eric Blake: > From: Vladimir Sementsov-Ogievskiy > > OK, that's a big rewrite of the logic. > > Pre-patch we have an always running coroutine - connection_co. It does > reply receiving and reconnecting. And it leads to a lot of difficult > and unobvious code around drained sections and context switch. We also > abuse bs->in_flight counter which is increased for connection_co and > temporary decreased in points where we want to allow drained section to > begin. One of these place is in another file: in nbd_read_eof() in > nbd/client.c. > > We also cancel reconnect and requests waiting for reconnect on drained > begin which is not correct. And this patch fixes that. > > Let's finally drop this always running coroutine and go another way: > do both reconnect and receiving in request coroutines. > Hi, while updating our stack to 6.2, one of our live-migration tests stopped working (backtrace is below) and bisecting led me to this patch. The VM has a single qcow2 disk (converting to raw doesn't make a difference) and the issue only appears when using iothread (for both virtio-scsi-pci and virtio-block-pci). Reverting 1af7737871fb3b66036f5e520acb0a98fc2605f7 (which lives on top) and 4ddb5d2fde6f22b2cf65f314107e890a7ca14fcf (the commit corresponding to this patch) in v6.2.0 makes the migration work again. Backtrace: Thread 1 (Thread 0x7f9d93458fc0 (LWP 56711) "kvm"): #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x7f9d9d6bc537 in __GI_abort () at abort.c:79 #2 0x7f9d9d6bc40f in __assert_fail_base (fmt=0x7f9d9d825128 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5579153763f8 "qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)", file=0x5579153764f9 "../io/channel.c", line=483, function=) at assert.c:92 #3 0x7f9d9d6cb662 in __GI___assert_fail (assertion=assertion@entry=0x5579153763f8 "qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)", file=file@entry=0x5579153764f9 "../io/channel.c", line=line@entry=483, function=function@entry=0x557915376570 <__PRETTY_FUNCTION__.2> "qio_channel_restart_read") at assert.c:101 #4 0x5579150c351c in qio_channel_restart_read (opaque=) at ../io/channel.c:483 #5 qio_channel_restart_read (opaque=) at ../io/channel.c:477 #6 0x55791520182a in aio_dispatch_handler (ctx=ctx@entry=0x557916908c60, node=0x7f9d8400f800) at ../util/aio-posix.c:329 #7 0x557915201f62 in aio_dispatch_handlers (ctx=0x557916908c60) at ../util/aio-posix.c:372 #8 aio_dispatch (ctx=0x557916908c60) at ../util/aio-posix.c:382 #9 0x5579151ea74e in aio_ctx_dispatch (source=, callback=, user_data=) at ../util/async.c:311 #10 0x7f9d9e647e6b in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #11 0x557915203030 in glib_pollfds_poll () at ../util/main-loop.c:232 #12 os_host_main_loop_wait (timeout=992816) at ../util/main-loop.c:255 #13 main_loop_wait (nonblocking=nonblocking@entry=0) at ../util/main-loop.c:531 #14 0x5579150539c1 in qemu_main_loop () at ../softmmu/runstate.c:726 #15 0x557914ce8ebe in main (argc=, argv=, envp=) at ../softmmu/main.c:50
Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Am 25.01.22 um 16:06 schrieb Daniel P. Berrangé: > On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote: >> Stefan Reiter writes: >> >>> Since the removal of the generic 'qmp_change' command, one can no longer >>> replace >>> the 'default' VNC display listen address at runtime (AFAIK). For our users >>> who >>> need to set up a secondary VNC access port, this means configuring a second >>> VNC >>> display (in addition to our standard one for web-access), but it turns out >>> one >>> cannot set a password on this second display at the moment, as the >>> 'set_password' call only operates on the 'default' display. >>> >>> Additionally, using secret objects, the password is only read once at >>> startup. >>> This could be considered a bug too, but is not touched in this series and >>> left >>> for a later date. >> >> Related: Vladimir recently posted a patch to add a new command for >> changing VNC server listening addresses. Daniel asked him to work it >> into display-reload instead[1]. Vladimir complied[2]. >> >> Daniel, what do you think about this one? Should it also use >> display-reload? > > I'd ultimately intend to deprecate & remove the direct setting of > passwords on the CLI, and exclusively rely on the 'secret' object > for passing in passwords. With this in mind, I'd not be enthusiastic > about adding new commands for changing passwords in QMP directly, > rather I think we should have a way to change the 'secret' object > in use. > How should I proceed with this series then? Does adding the new argument for the display ID count as "adding new commands"? If what I should do is switching to only using secret objects, would the plan be something like the following? 1. Add an option to display-reload for switching the display's password-secret while adding SPICE as a valid display type. 2. Also include the set password action (i.e. disconnect/fail/keep) and expiration time as part of that option. 3. Extend display-reload to also take an optional display ID for VNC. 4. Deprecate expire_password and set_password. > Regards, > Daniel
Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Am 28.10.21 um 21:37 schrieb Markus Armbruster:
Markus Armbruster writes:
8<
diff --git a/qapi/ui.json b/qapi/ui.json
index 5292617b44..39ca0b5ead 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -69,8 +69,10 @@
'base': { 'protocol': 'DisplayProtocol',
'password': 'str' },
'discriminator': 'protocol',
- 'data': { 'vnc': 'SetPasswordOptionsVnc',
-'spice': 'SetPasswordOptionsSpice' } }
+ 'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
+ 'if': 'CONFIG_VNC' },
+'spice': { 'type': 'SetPasswordOptionsSpice',
+ 'if': 'CONFIG_SPICE' } } }
##
# @SetPasswordOptionsSpice:
@@ -155,7 +157,8 @@
'base': { 'protocol': 'DisplayProtocol',
'time': 'str' },
'discriminator': 'protocol',
- 'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
+ 'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
+ 'if': 'CONFIG_VNC' } } }
So I decided to give the #ifdef approach a go, but if I configure with
--disable-spice --disable-vnc, even with the above conditionals, it is
still be possible to issue a set_password qmp command, which will then
lead to an abort() in the generated code (and the patched
qmp_set_password below would do the same if it could be reached):
Thread 1 (Thread 0x7f4a86701ec0 (LWP 40487) "qemu-system-x86"):
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x7f4a90d72537 in __GI_abort () at abort.c:79
#2 0x5583ca03cef3 in visit_type_SetPasswordOptions_members
(v=v@entry=0x5583cc60, obj=obj@entry=0x7ffe5bfc3ee0,
errp=errp@entry=0x0) at qapi/qapi-visit-ui.c:71
#3 0x5583ca5df72f in qmp_marshal_set_password (args=out>, ret=, errp=0x7f4a85d96ea0) at
qapi/qapi-commands-ui.c:49
#4 0x5583ca5e89e9 in do_qmp_dispatch_bh (opaque=0x7f4a85d96eb0) at
../qapi/qmp-dispatch.c:129
#5 0x5583ca605494 in aio_bh_call (bh=0x7f4a78009270) at
../util/async.c:141
Is that expected? Should I add a conditional for {set,expire}_password
in the schema too, and add an
#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
around the whole {hmp,qmp}_{set,expire}_password functions/declarations
in C?
Or maybe that's a good indication that it's really not worth it ;)?
P.S. Thank you for the QAPI-related explanation in the other mail!
8<
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 4825d0cbea..69a9c2977a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -167,18 +167,26 @@ void qmp_set_password(SetPasswordOptions *opts, Error
**errp)
{
int rc = 0;
-if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
+switch (opts->protocol) {
+#ifdef CONFIG_SPICE
+case DISPLAY_PROTOCOL_SPICE:
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_passwd(opts->password,
opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL,
opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT);
-} else {
-assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
+break;
+#endif
+#ifdef CONFIG_VNC
+case DISPLAY_PROTOCOL_VNC:
/* Note that setting an empty password will not disable login through
* this interface. */
rc = vnc_display_password(opts->u.vnc.display, opts->password);
+break;
+#endif
+default:
+abort();
}
if (rc != 0) {
@@ -202,14 +210,22 @@ void qmp_expire_password(ExpirePasswordOptions *opts,
Error **errp)
when = strtoull(whenstr, NULL, 10);
}
-if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
+switch (opts->protocol) {
+#ifdef CONFIG_SPICE
+case DISPLAY_PROTOCOL_SPICE:
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_pw_expire(when);
-} else {
-assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
+break;
+#endif
+#ifdef CONFIG_VNC
+case DISPLAY_PROTOCOL_VNC:
rc = vnc_display_pw_expire(opts->u.vnc.display, when);
+break;
+#endif
+default:
+abort();
}
if (rc != 0) {
Re: [PATCH v7 2/4] qapi/monitor: refactor set/expire_password with enums
Am 21.10.21 um 12:01 schrieb Stefan Reiter:
'protocol' and 'connected' are better suited as enums than as strings,
make use of that. No functional change intended.
Suggested-by: Markus Armbruster
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, &err);
+proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, &err);
+if (err) {
+goto out;
+}
+
+conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
+ SET_PASSWORD_ACTION_KEEP, &err);
+if (err) {
+goto out;
+}
+
+qmp_set_password(proto, password, !!connected, conn, &err);
+
+out:
hmp_handle_error(mon, err);
}
@@ -1463,8 +1479,17 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
const char *protocol = qdict_get_str(qdict, "protocol");
const char *whenstr = qdict_get_str(qdict, "time");
Error *err = NULL;
+DisplayProtocol proto;
-qmp_expire_password(protocol, whenstr, &err);
+proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+DISPLAY_PROTOCOL_VNC, &err);
+if (err) {
+goto out;
+}
+
+qmp_expire_password(proto, whenstr, &err);
+
+out:
hmp_handle_error(mon, err);
}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5c0d5e116b..0654d7289a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -163,33 +163,27 @@ void qmp_system_wakeup(Error **errp)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
}
-void qmp_set_password(const char *protocol, const char *password,
- bool has_connected, const char *connected, Error **errp)
+void qmp_set_password(DisplayProtocol protocol, const char *password,
+ bool has_connected, SetPasswordAction connected,
+ Error **errp)
{
int disconnect_if_connected = 0;
int fail_if_connected = 0;
int rc;
if (has_connected) {
-if (strcmp(connected, "fail") == 0) {
-fail_if_connected = 1;
-} else if (strcmp(connected, "disconnect") == 0) {
-disconnect_if_connected = 1;
-} else if (strcmp(connected, "keep") == 0) {
-/* nothing */
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER, "connected");
-return;
-}
+fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL;
+disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT;
}
-if (strcmp(protocol, "spice") == 0) {
+if (protocol == DISPLAY_PROTOCOL_SPICE) {
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_passwd(password, fail_if_connected,
disconnect_if_connected);
-} else if (strcmp(protocol, "vnc") == 0) {
+} else {
+assert(protocol == DISPLAY_PROTOCOL_VNC);
if (fail_if_connected || disconnect_if_connected) {
/* vnc supports "connected=keep" only */
error_setg(errp, QERR_INVALID_PARAMETER, "connected");
@@ -198,10 +192,6 @@ void qmp_set_password(const char *protocol, const char
*password,
/* Note that setting an empty password will not disable login through
* this interface. */
rc = vnc_display_password(NULL, password);
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
- "'vnc' or 'spice'");
-return;
}
if (rc != 0) {
@@ -209,7 +199,7 @@ void qmp_set_password(const char *protocol, const char
*password,
}
}
-void qmp_expire_password(const char *protocol, const char *whenstr,
+void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
Error **errp)
{
time_t when;
@@ -225,17 +215,14 @@ void qmp_expire_password(const char *protocol, const char
*whenstr,
when = strtoull(whenstr, NULL, 10);
}
-if (strcmp(protocol, "spice") == 0) {
+if (protocol == DISPLAY_PROTOCOL_SPICE) {
if (!qemu_using_spice(errp)) {
return;
}
rc = qemu_spice.set_pw_expire(when);
-
Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Am 28.10.21 um 21:37 schrieb Markus Armbruster:
Markus Armbruster writes:
Stefan Reiter writes:
Since the removal of the generic 'qmp_change' command, one can no longer replace
the 'default' VNC display listen address at runtime (AFAIK). For our users who
need to set up a secondary VNC access port, this means configuring a second VNC
display (in addition to our standard one for web-access), but it turns out one
cannot set a password on this second display at the moment, as the
'set_password' call only operates on the 'default' display.
Additionally, using secret objects, the password is only read once at startup.
This could be considered a bug too, but is not touched in this series and left
for a later date.
Queued, thanks!
Unqueued, because it fails to compile with --disable-vnc and with
--disable-spice. I failed to catch this in review, sorry.
Making it work takes a tiresome amount of #ifdeffery (sketch appended).
Missing: removal of stubs that are no longer used,
e.g. vnc_display_password() in ui/vnc-stubs.c. Feels like more trouble
than it's worth.
To maximize our chances to get this into 6.2, please respin without the
'if' conditionals. Yes, this makes introspection less useful, but it's
no worse than before the patch.
Unfortunately, Stefan is no longer working with Proxmox, so I would pick
up these patches instead.
Since the 6.2 ship has long sailed, I suppose the way forward is using
the #ifdefs then?
From my understanding what should be done is:
* In the first patch, fix the typo spotted by Eric Blake and add the R-b
tags from this round.
* Replace "since 6.2" with "since 7.0" everywhere.
* Incorporate the #ifdef handling from below. I had to add another one
for the when/whenstr handling in qmp_expire_password to avoid an error
with -Werror=unused-but-set-variable.
* Add #ifdefs for the unused stubs too? If yes, how to best find them?
diff --git a/qapi/ui.json b/qapi/ui.json
index 5292617b44..39ca0b5ead 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -69,8 +69,10 @@
'base': { 'protocol': 'DisplayProtocol',
'password': 'str' },
'discriminator': 'protocol',
- 'data': { 'vnc': 'SetPasswordOptionsVnc',
-'spice': 'SetPasswordOptionsSpice' } }
+ 'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
+ 'if': 'CONFIG_VNC' },
+'spice': { 'type': 'SetPasswordOptionsSpice',
+ 'if': 'CONFIG_SPICE' } } }
##
# @SetPasswordOptionsSpice:
@@ -155,7 +157,8 @@
'base': { 'protocol': 'DisplayProtocol',
'time': 'str' },
'discriminator': 'protocol',
- 'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
+ 'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
+ 'if': 'CONFIG_VNC' } } }
##
# @ExpirePasswordOptionsVnc:
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index f0f0c82d59..f714b2d316 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,24 +1451,40 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *password = qdict_get_str(qdict, "password");
+#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
const char *display = qdict_get_try_str(qdict, "display");
+#endif
+#ifdef CONFIG_SPICE
const char *connected = qdict_get_try_str(qdict, "connected");
+#endif
Error *err = NULL;
+int proto;
SetPasswordOptions opts = {
.password = (char *)password,
};
-opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-DISPLAY_PROTOCOL_VNC, &err);
+proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err);
if (err) {
goto out;
}
-if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+switch (proto) {
+#ifdef CONFIG_VNC
+case -1:
+proto = DISPLAY_PROTOCOL_VNC;
+/* fall through */
+case DISPLAY_PROTOCOL_VNC:
opts.u.vnc.has_display = !!display;
opts.u.vnc.display = (char *)display;
-} else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
+break;
+#else
+case -1:
+error_setg(&err, "FIXME");
+goto out;
+#endif
+#ifdef CONFIG_SPICE
+case DISPLAY_PROTOCOL_SPICE:
opts.u.spice.has_connected = !!connected;
opts.u.spice.connected =
qapi_enum_parse(&SetPasswordAction_lookup, connected,
@@ -1476,8 +1492,13 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
if (err) {
goto out;
}
+break;
+#endif
+default:
+;
}
+opts.protocol = proto;
qmp_set_password(&opts, &err);
out:
@@ -1488,22 +1509,34 @@ void hmp_expire_password(Monitor *mon, const QDict
*qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
const char *whenstr = qdict_get_str(qdict, "time");
+#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
[PATCH] scripts/qemu-guest-agent/fsfreeze-hook: improve script description
With the current wording, users might think that the -F option is not required as long as the script is placed in the default path[0]. Be clear that the option is always required. [0]: https://forum.proxmox.com/threads/82680/post-437608 Signed-off-by: Fabian Ebner --- I also tried to improve the surrounding text as it sounded a bit off to me, but English is not my native language, so hoping that I didn't actually make it worse. scripts/qemu-guest-agent/fsfreeze-hook | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/scripts/qemu-guest-agent/fsfreeze-hook b/scripts/qemu-guest-agent/fsfreeze-hook index 13aafd4845..df9af05e3b 100755 --- a/scripts/qemu-guest-agent/fsfreeze-hook +++ b/scripts/qemu-guest-agent/fsfreeze-hook @@ -1,11 +1,12 @@ #!/bin/sh # This script is executed when a guest agent receives fsfreeze-freeze and -# fsfreeze-thaw command, if it is specified in --fsfreeze-hook (-F) -# option of qemu-ga or placed in default path (/etc/qemu/fsfreeze-hook). -# When the agent receives fsfreeze-freeze request, this script is issued with -# "freeze" argument before the filesystem is frozen. And for fsfreeze-thaw -# request, it is issued with "thaw" argument after filesystem is thawed. +# fsfreeze-thaw commands, provided that the --fsfreeze-hook (-F) option of +# qemu-ga is specified and the script is placed in /etc/qemu/fsfreeze-hook +# (or the custom path specified together with -F). When the agent receives +# fsfreeze-freeze requests, this script is called with "freeze" as its argument +# before the filesystem is frozen. And for fsfreeze-thaw requests, it is called +# with "thaw" as its argument after the filesystem is thawed. LOGFILE=/var/log/qga-fsfreeze-hook.log FSFREEZE_D=$(dirname -- "$0")/fsfreeze-hook.d -- 2.30.2
[PATCH v2] block/io_uring: resubmit when result is -EAGAIN
Linux SCSI can throw spurious -EAGAIN in some corner cases in its
completion path, which will end up being the result in the completed
io_uring request.
Resubmitting such requests should allow block jobs to complete, even
if such spurious errors are encountered.
Co-authored-by: Stefan Hajnoczi
Reviewed-by: Stefano Garzarella
Signed-off-by: Fabian Ebner
---
Changes from v1:
* Focus on what's relevant for the patch itself in the commit
message.
* Add Stefan's comment.
* Add Stefano's R-b tag (I hope that's fine, since there was no
change code-wise).
block/io_uring.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/block/io_uring.c b/block/io_uring.c
index 00a3ee9fb8..dfa475cc87 100644
--- a/block/io_uring.c
+++ b/block/io_uring.c
@@ -165,7 +165,21 @@ static void luring_process_completions(LuringState *s)
total_bytes = ret + luringcb->total_read;
if (ret < 0) {
-if (ret == -EINTR) {
+/*
+ * Only writev/readv/fsync requests on regular files or host block
+ * devices are submitted. Therefore -EAGAIN is not expected but
it's
+ * known to happen sometimes with Linux SCSI. Submit again and hope
+ * the request completes successfully.
+ *
+ * For more information, see:
+ *
https://lore.kernel.org/io-uring/20210727165811.284510-3-ax...@kernel.dk/T/#u
+ *
+ * If the code is changed to submit other types of requests in the
+ * future, then this workaround may need to be extended to deal
with
+ * genuine -EAGAIN results that should not be resubmitted
+ * immediately.
+ */
+if (ret == -EINTR || ret == -EAGAIN) {
luring_resubmit(s, luringcb);
continue;
}
--
2.30.2
[PATCH] block/io_uring: resubmit when result is -EAGAIN
Quoting from [0]:
Some setups, like SCSI, can throw spurious -EAGAIN off the softirq
completion path. Normally we expect this to happen inline as part
of submission, but apparently SCSI has a weird corner case where it
can happen as part of normal completions.
Host kernels without patch [0] can panic when this happens [1], and
resubmitting makes the panic more likely. On the other hand, for
kernels with patch [0], resubmitting ensures that a block job is not
aborted just because of such spurious errors.
[0]:
https://lore.kernel.org/io-uring/20210727165811.284510-3-ax...@kernel.dk/T/#u
[1]:
#9 [b732000c8b70] asm_exc_page_fault at a4800ade
#10 [b732000c8bf8] io_prep_async_work at a3d89c16
#11 [b732000c8c50] io_rw_reissue at a3d8b2e1
#12 [b732000c8c78] io_complete_rw at a3d8baa8
#13 [b732000c8c98] blkdev_bio_end_io at a3d62a80
#14 [b732000c8cc8] bio_endio at a3f4e800
#15 [b732000c8ce8] dec_pending at a432f854
#16 [b732000c8d30] clone_endio at a433170c
#17 [b732000c8d70] bio_endio at a3f4e800
#18 [b732000c8d90] blk_update_request at a3f53a37
#19 [b732000c8dd0] scsi_end_request at a4233a5c
#20 [b732000c8e08] scsi_io_completion at a423432c
#21 [b732000c8e58] scsi_finish_command at a422c527
#22 [b732000c8e88] scsi_softirq_done at a42341e4
Signed-off-by: Fabian Ebner
---
I'm new to this code and io_uring, so I don't know what all the
implications are, but retrying upon EAGAIN does not sound like
a bad thing to my inexperienced ears.
Some more context, leading up to this patch:
We had some users reporting issues after we switched to using io_uring
by default. Namely, kernel panics [2] for some, and failing block jobs
[3] (with a custom backup mechanism we implemented on top of QEMU's
block layer) for others.
I had luck and managed to reprouce the issue, and it was a failed
block job about half of the time and a kernel panic the other half.
When using a host kernel with [0], it's a failed block job all the
time, and this patch attempts to fix that, by resubmitting instead
of bubbling up the error.
[2]:
https://forum.proxmox.com/threads/kernel-panic-whole-server-crashes-about-every-day.91803/post-404382
[3]:
https://forum.proxmox.com/threads/backup-job-failed-with-err-11-on-2-of-6-vms.92568/
block/io_uring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/io_uring.c b/block/io_uring.c
index 00a3ee9fb8..77d162cb24 100644
--- a/block/io_uring.c
+++ b/block/io_uring.c
@@ -165,7 +165,7 @@ static void luring_process_completions(LuringState *s)
total_bytes = ret + luringcb->total_read;
if (ret < 0) {
-if (ret == -EINTR) {
+if (ret == -EINTR || ret == -EAGAIN) {
luring_resubmit(s, luringcb);
continue;
}
--
2.30.2
