[pve-devel] [PATCH qemu-server] cleanup pci devices in more situations

[pve-devel] [PATCH qemu-server] cleanup pci devices in more situations

[Dominik Csapak]

if the preparing of pci devices or the start of the vm fails, we need
to cleanup the pci devices (reservations *and* mdevs), or else
it might happen that there are leftovers which must be manually removed.

to include also mdevs now, refactor the cleanup code from 'vm_stop_cleanup'
into it's own function, and call that instead of only 'remove_pci_reservation'

also print the errors of the cleanup steps with 'warn', otherwise we
might discard important errors

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/QemuServer.pm | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 7d9cf22..72f0f58 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5602,7 +5602,7 @@ sub vm_start_nolock {
 	}
     };
     if (my $err = $@) {
-	eval { PVE::QemuServer::PCI::remove_pci_reservation($pci_id_list) };
+	eval { cleanup_pci_devices($vmid, $conf) };
 	warn $@ if $@;
 	die $err;
     }
@@ -5698,7 +5698,9 @@ sub vm_start_nolock {
     if (my $err = $@) {
 	# deactivate volumes if start fails
 	eval { PVE::Storage::deactivate_volumes($storecfg, $vollist); };
-	eval { PVE::QemuServer::PCI::remove_pci_reservation($pci_id_list) };
+	warn $@ if $@;
+	eval { cleanup_pci_devices($vmid, $conf) };
+	warn $@ if $@;
 
 	die "start failed: $err";
     }
@@ -5863,6 +5865,25 @@ sub get_vm_volumes {
     return $vollist;
 }
 
+sub cleanup_pci_devices {
+    my ($vmid, $conf) = @_;
+
+    my $ids = [];
+    foreach my $key (keys %$conf) {
+	next if $key !~ m/^hostpci(\d+)$/;
+	my $hostpciindex = $1;
+	my $d = parse_hostpci($conf->{$key});
+	my $uuid = PVE::SysFSTools::generate_mdev_uuid($vmid, $hostpciindex);
+
+	foreach my $pci (@{$d->{pciid}}) {
+	    my $pciid = $pci->{id};
+	    push @$ids, $pci->{id};
+	    PVE::SysFSTools::pci_cleanup_mdev_device($pciid, $uuid);
+	}
+    }
+    PVE::QemuServer::PCI::remove_pci_reservation($ids);
+}
+
 sub vm_stop_cleanup {
     my ($storecfg, $vmid, $conf, $keepActive, $apply_pending_changes) = @_;
 
@@ -5894,20 +5915,7 @@ sub vm_stop_cleanup {
 	    unlink '/dev/shm/pve-shm-' . ($ivshmem->{name} // $vmid);
 	}
 
-	my $ids = [];
-	foreach my $key (keys %$conf) {
-	    next if $key !~ m/^hostpci(\d+)$/;
-	    my $hostpciindex = $1;
-	    my $d = parse_hostpci($conf->{$key});
-	    my $uuid = PVE::SysFSTools::generate_mdev_uuid($vmid, $hostpciindex);
-
-	    foreach my $pci (@{$d->{pciid}}) {
-		my $pciid = $pci->{id};
-		push @$ids, $pci->{id};
-		PVE::SysFSTools::pci_cleanup_mdev_device($pciid, $uuid);
-	    }
-	}
-	PVE::QemuServer::PCI::remove_pci_reservation($ids);
+	cleanup_pci_devices($vmid, $conf);
 
 	vmconfig_apply_pending($vmid, $conf, $storecfg) if $apply_pending_changes;
     };
-- 
2.30.2

[pve-devel] applied: [PATCH qemu-server] cleanup pci devices in more situations

[Thomas Lamprecht]

Am 27/07/2022 um 15:28 schrieb Dominik Csapak:
> if the preparing of pci devices or the start of the vm fails, we need
> to cleanup the pci devices (reservations *and* mdevs), or else
> it might happen that there are leftovers which must be manually removed.
> 
> to include also mdevs now, refactor the cleanup code from 'vm_stop_cleanup'
> into it's own function, and call that instead of only 'remove_pci_reservation'
> 
> also print the errors of the cleanup steps with 'warn', otherwise we
> might discard important errors
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/QemuServer.pm | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
>

applied, thanks!

fyi, there where some context changes but `git am -3` managed to auto-merge just
fine, so hopefully I did not overlook some problematic (semantic) changes for
this.