[pve-devel] [PATCH-SERIES qemu-server v2 0/3] qmp: improve error handling for mon_cmd()

[pve-devel] [PATCH-SERIES qemu-server v2 0/3] qmp: improve error handling for mon_cmd()

[Fiona Ebner]

Changes in v2:
* use eval+warn around new detach() call sites, as failure should not
  be critical there.

Add a new noerr argument to mon_cmd() to allow callers to opt-in to
handling errors themselves.

Without passing 'noerr' to mon_cmd(), errors are logged to the system
journal. In the blockdev module, in attach() and detach(), there are
two mon_cmd() calls that are expected to fail in some scenarios for
which the errors should not be logged.

Again in the blockdev module, the functions for deletion and replace
had the same issue. Have them re-use the detach() function.


Fiona Ebner (3):
  qmp client: add $noerr argument
  blockdev: attach/detach: silence errors for QMP commands for which
    failure may be expected
  blockdev: delete/replace: re-use detach() helper

 src/PVE/QMPClient.pm           |  8 ++++---
 src/PVE/QemuServer/Blockdev.pm | 16 ++++++--------
 src/PVE/QemuServer/Monitor.pm  | 40 +++++++++++++++++++++++++++++++---
 3 files changed, 49 insertions(+), 15 deletions(-)

-- 
2.47.2



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

[pve-devel] [PATCH qemu-server v2 1/3] qmp client: add $noerr argument

[Fiona Ebner]

Using the $noerr argument will allow callers to opt-in to handling
errors themselves.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QMPClient.pm          |  8 ++++---
 src/PVE/QemuServer/Monitor.pm | 40 ++++++++++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/src/PVE/QMPClient.pm b/src/PVE/QMPClient.pm
index 87d61144..7b19be9d 100644
--- a/src/PVE/QMPClient.pm
+++ b/src/PVE/QMPClient.pm
@@ -86,7 +86,7 @@ sub queue_cmd {
 
 # execute a single command
 sub cmd {
-    my ($self, $vmid, $cmd, $timeout) = @_;
+    my ($self, $vmid, $cmd, $timeout, $noerr) = @_;
 
     my $result;
 
@@ -155,8 +155,10 @@ sub cmd {
 
     $self->queue_execute($timeout, 2);
 
-    die "VM $vmid qmp command '$cmd->{execute}' failed - $queue_info->{error}"
-        if defined($queue_info->{error});
+    if (defined($queue_info->{error})) {
+        die "VM $vmid qmp command '$cmd->{execute}' failed - $queue_info->{error}" if !$noerr;
+        $result = { error => $queue_info->{error} };
+    }
 
     return $result;
 }
diff --git a/src/PVE/QemuServer/Monitor.pm b/src/PVE/QemuServer/Monitor.pm
index 5956e1c4..062e71be 100644
--- a/src/PVE/QemuServer/Monitor.pm
+++ b/src/PVE/QemuServer/Monitor.pm
@@ -12,14 +12,48 @@ our @EXPORT_OK = qw(
     mon_cmd
 );
 
+=head3 qmp_cmd
+
+    my $cmd = { execute => $qmp_command_name, arguments => \%params };
+    my $result = qmp_cmd($vmid, $cmd);
+
+Execute the C<$qmp_command_name> with arguments C<%params> for VM C<$vmid>. Dies if the VM is not
+running or the monitor socket cannot be reached, even if the C<noerr> argument is used. Returns the
+structured result from the QMP side converted from JSON to structured Perl data. In case the
+C<noerr> argument is used and the QMP command failed or timed out, the result is a hash reference
+with an C<error> key containing the error message.
+
+Parameters:
+
+=over
+
+=item C<$vmid>: The ID of the virtual machine.
+
+=item C<$cmd>: Hash reference containing the QMP command name for the C<execute> key and additional
+arguments for the QMP command under the C<arguments> key. The following custom arguments are not
+part of the QMP schema and supported for all commands:
+
+=over
+
+=item C<timeout>: wait at most for this amount of time. If there was no actual error, the QMP/QGA
+command will still continue to be executed even after the timeout reached.
+
+=item C<noerr>: do not die when the command gets an error or the timeout is hit. The caller needs to
+handle the error that is returned as a structured result.
+
+=back
+
+=back
+
+=cut
 sub qmp_cmd {
     my ($vmid, $cmd) = @_;
 
     my $res;
 
-    my $timeout;
+    my ($noerr, $timeout);
     if ($cmd->{arguments}) {
-        $timeout = delete $cmd->{arguments}->{timeout};
+        ($noerr, $timeout) = delete($cmd->{arguments}->@{qw(noerr timeout)});
     }
 
     eval {
@@ -28,7 +62,7 @@ sub qmp_cmd {
         if (-e $sname) { # test if VM is reasonably new and supports qmp/qga
             my $qmpclient = PVE::QMPClient->new();
 
-            $res = $qmpclient->cmd($vmid, $cmd, $timeout);
+            $res = $qmpclient->cmd($vmid, $cmd, $timeout, $noerr);
         } else {
             die "unable to open monitor socket\n";
         }
-- 
2.47.2



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

[pve-devel] [PATCH qemu-server v2 2/3] blockdev: attach/detach: silence errors for QMP commands for which failure may be expected

[Fiona Ebner]

Without passing 'noerr' to mon_cmd(), errors are logged to the system
journal. In attach() and detach(), there are two mon_cmd() calls that
are expected to fail in some scenarios for which the errors should not
be logged.

Reported-by: Friedrich Weber <f.weber@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer/Blockdev.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
index d0d7e684..04eeed3c 100644
--- a/src/PVE/QemuServer/Blockdev.pm
+++ b/src/PVE/QemuServer/Blockdev.pm
@@ -575,7 +575,7 @@ sub attach {
     eval {
         if ($throttle_group_id) {
             # Try to remove potential left-over.
-            eval { mon_cmd($vmid, 'object-del', id => $throttle_group_id); };
+            mon_cmd($vmid, 'object-del', id => $throttle_group_id, noerr => 1);
 
             my $throttle_group = generate_throttle_group($drive);
             mon_cmd($vmid, 'object-add', $throttle_group->%*);
@@ -630,8 +630,8 @@ sub detach {
     while ($node_name) {
         last if !$block_info->{$node_name}; # already gone
 
-        eval { mon_cmd($vmid, 'blockdev-del', 'node-name' => "$node_name"); };
-        if (my $err = $@) {
+        my $res = mon_cmd($vmid, 'blockdev-del', 'node-name' => "$node_name", noerr => 1);
+        if (my $err = $res->{error}) {
             last if $err =~ m/Failed to find node with node-name/; # already gone
             die "deleting blockdev '$node_name' failed : $err\n";
         }
-- 
2.47.2



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

[pve-devel] [PATCH qemu-server v2 3/3] blockdev: delete/replace: re-use detach() helper

[Fiona Ebner]

Re-using the detach() helper has the side effect of avoiding logging
errors to syslog for automatically removed child nodes. This should be
the case for all file nodes here. None are explicitly added via
blockdev-add and thus QEMU already auto-removes them.

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

Changes in v2:
* use eval+warn around new detach() call sites, as failure should not
  be critical there.

 src/PVE/QemuServer/Blockdev.pm | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
index 04eeed3c..f5798cbf 100644
--- a/src/PVE/QemuServer/Blockdev.pm
+++ b/src/PVE/QemuServer/Blockdev.pm
@@ -873,9 +873,8 @@ sub blockdev_external_snapshot {
 sub blockdev_delete {
     my ($storecfg, $vmid, $drive, $file_blockdev, $fmt_blockdev, $snap) = @_;
 
-    #add eval as reopen is auto removing the old nodename automatically only if it was created at vm start in command line argument
-    eval { mon_cmd($vmid, 'blockdev-del', 'node-name' => $fmt_blockdev->{'node-name'}) };
-    eval { mon_cmd($vmid, 'blockdev-del', 'node-name' => $file_blockdev->{'node-name'}) };
+    eval { detach($vmid, $fmt_blockdev->{'node-name'}); };
+    warn "detaching block node for $file_blockdev->{filename} failed - $@" if $@;
 
     #delete the file (don't use vdisk_free as we don't want to delete all snapshot chain)
     print "delete old $file_blockdev->{filename}\n";
@@ -982,9 +981,8 @@ sub blockdev_replace {
     }
 
     # delete old file|fmt nodes
-    # add eval as reopen is auto removing the old nodename automatically only if it was created at vm start in command line argument
-    eval { mon_cmd($vmid, 'blockdev-del', 'node-name' => $src_fmt_blockdev_name) };
-    eval { mon_cmd($vmid, 'blockdev-del', 'node-name' => $src_file_blockdev_name) };
+    eval { detach($vmid, $src_fmt_blockdev_name); };
+    warn "detaching block node for $src_snap failed - $@" if $@;
 }
 
 sub blockdev_commit {
-- 
2.47.2



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

[pve-devel] applied-series: [PATCH-SERIES qemu-server v2 0/3] qmp: improve error handling for mon_cmd()

[Fabian Grünbichler]

On Tue, 12 Aug 2025 13:56:28 +0200, Fiona Ebner wrote:
> Changes in v2:
> * use eval+warn around new detach() call sites, as failure should not
>   be critical there.
> 
> Add a new noerr argument to mon_cmd() to allow callers to opt-in to
> handling errors themselves.
> 
> [...]

Applied, thanks!

[1/3] qmp client: add $noerr argument
      commit: 9e973210eaba91fc7a3f8d9ef5b796bcf12e3e38
[2/3] blockdev: attach/detach: silence errors for QMP commands for which failure may be expected
      commit: d2f055b03a1acc69296a04d8514acc6256ce33fa
[3/3] blockdev: delete/replace: re-use detach() helper
      commit: e7cf7c0056f8fe7dc4487d3465a4ba27e9289190

Best regards,
-- 
Fabian Grünbichler <f.gruenbichler@proxmox.com>


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