public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH v2 qemu-server] qmp helpers: device add/del: use HMP interface
@ 2025-02-04 15:51 Fiona Ebner
  2025-02-04 16:22 ` [pve-devel] applied: " Thomas Lamprecht
  0 siblings, 1 reply; 2+ messages in thread
From: Fiona Ebner @ 2025-02-04 15:51 UTC (permalink / raw)
  To: pve-devel

Fixes device hotplug in combination with QEMU 9.2.

QEMU commit be93fd5372 ("qdev-monitor: avoid QemuOpts in QMP device_add")
notes:

> This patch changes the behavior of QMP device_add but not HMP
> device_add. QMP clients that sent incorrectly typed device_add QMP
> commands no longer work. This is a breaking change but clients should be
> using the correct types already.

The qemu_deviceadd() helper does not have the required type
information right now, so switch to using HMP, which still behaves the
same when passing a device commandline string. QEMU commit be93fd5372
fixes passing in complex properties via JSON, but the qemu_deviceadd()
helper never uses any such, as it already only received a string (and
naively split it up).

Use HMP for 'device_del' too, simply to keep the qemu_deviceadd() and
qemu_devicedel() helpers consistent.

Switching back to QMP using the correct types in the JSON can still be
done later. Unfortunately, 'qmp-query-schema' does not provide
device-specific types, so another way is needed.

A timeout of 30 seconds is used rather then relying on the low default
like before, since device plug operations require actions by the guest
kernel and might require IO. Device plug is often an interactive
operation, so a too high timeout could lead to bad UX. Should specific
devices need a higher timeout, it can still be increased further for
them in the future.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
* also mention 'del' in commit title
* fix commit message (bug #5985 is not actually fixed by this, that
  requires bumping the timeout for netdev_add)
* use 30 seconds as a timeout (see commit message for rationale)

 PVE/QemuServer/QMPHelpers.pm | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer/QMPHelpers.pm b/PVE/QemuServer/QMPHelpers.pm
index 1bc9e5dc..9b077394 100644
--- a/PVE/QemuServer/QMPHelpers.pm
+++ b/PVE/QemuServer/QMPHelpers.pm
@@ -25,15 +25,14 @@ sub qemu_deviceadd {
     my ($vmid, $devicefull) = @_;
 
     $devicefull = "driver=".$devicefull;
-    my %options =  split(/[=,]/, $devicefull);
 
-    mon_cmd($vmid, "device_add" , %options);
+    PVE::QemuServer::Monitor::hmp_cmd($vmid, "device_add $devicefull", 30);
 }
 
 sub qemu_devicedel {
     my ($vmid, $deviceid) = @_;
 
-    my $ret = mon_cmd($vmid, "device_del", id => $deviceid);
+    PVE::QemuServer::Monitor::hmp_cmd($vmid, "device_del $deviceid", 30);
 }
 
 sub qemu_objectadd {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 2+ messages in thread

* [pve-devel] applied: [PATCH v2 qemu-server] qmp helpers: device add/del: use HMP interface
  2025-02-04 15:51 [pve-devel] [PATCH v2 qemu-server] qmp helpers: device add/del: use HMP interface Fiona Ebner
@ 2025-02-04 16:22 ` Thomas Lamprecht
  0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2025-02-04 16:22 UTC (permalink / raw)
  To: Proxmox VE development discussion, Fiona Ebner

Am 04.02.25 um 16:51 schrieb Fiona Ebner:
> Fixes device hotplug in combination with QEMU 9.2.
> 
> QEMU commit be93fd5372 ("qdev-monitor: avoid QemuOpts in QMP device_add")
> notes:
> 
>> This patch changes the behavior of QMP device_add but not HMP
>> device_add. QMP clients that sent incorrectly typed device_add QMP
>> commands no longer work. This is a breaking change but clients should be
>> using the correct types already.
> 
> The qemu_deviceadd() helper does not have the required type
> information right now, so switch to using HMP, which still behaves the
> same when passing a device commandline string. QEMU commit be93fd5372
> fixes passing in complex properties via JSON, but the qemu_deviceadd()
> helper never uses any such, as it already only received a string (and
> naively split it up).
> 
> Use HMP for 'device_del' too, simply to keep the qemu_deviceadd() and
> qemu_devicedel() helpers consistent.
> 
> Switching back to QMP using the correct types in the JSON can still be
> done later. Unfortunately, 'qmp-query-schema' does not provide
> device-specific types, so another way is needed.
> 
> A timeout of 30 seconds is used rather then relying on the low default
> like before, since device plug operations require actions by the guest
> kernel and might require IO. Device plug is often an interactive
> operation, so a too high timeout could lead to bad UX. Should specific
> devices need a higher timeout, it can still be increased further for
> them in the future.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v2:
> * also mention 'del' in commit title
> * fix commit message (bug #5985 is not actually fixed by this, that
>   requires bumping the timeout for netdev_add)
> * use 30 seconds as a timeout (see commit message for rationale)
> 
>  PVE/QemuServer/QMPHelpers.pm | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

I reduced the timeout to 25s to ensure we got some headroom to the 30s timeout
that the web UI imposes for API requests, so that anything that doesn't triggers
a task-worker gets a nicer error. We can still increase that, before the change
it was only 5s anyway, so even with "just" 25s it's quite a bit longer now.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2025-02-04 16:22 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-02-04 15:51 [pve-devel] [PATCH v2 qemu-server] qmp helpers: device add/del: use HMP interface Fiona Ebner
2025-02-04 16:22 ` [pve-devel] applied: " Thomas Lamprecht

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal