[pve-devel] [PATCH] Fix/cleanup disk hotplug

[Derumier Alexandre]

- pass $conf to create_disks sub
- add some checks on results of "drive_del","drive_add","device_del","device_add"
- replace "sleep 2" by iterative sub vm_waitfordevicecleanup

Signed-off-by: Derumier Alexandre <aderumier at odiso.com>
---
 PVE/API2/Qemu.pm  |    4 +-
 PVE/QemuServer.pm |   76 ++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 54 insertions(+), 26 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 92493a8..c6e8ea0 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -458,7 +458,7 @@ __PACKAGE__->register_method({
 		} 
 		next if !defined($conf->{$opt});
 		if (PVE::QemuServer::valid_drivename($opt)) {
-		    PVE::QemuServer::vm_devicedel($vmid,$opt);
+		    PVE::QemuServer::vm_devicedel($vmid, $conf, $opt);
 		    my $drive = PVE::QemuServer::parse_drive($opt, $conf->{$opt});
 		    if (PVE::QemuServer::drive_is_cdrom($drive)) {
 			$cdchange->{$opt} = undef;
@@ -484,7 +484,7 @@ __PACKAGE__->register_method({
 		$unset->{$opt} = 1;
 	    }
 
-	    PVE::QemuServer::create_disks($storecfg, $vmid, $param);
+	    PVE::QemuServer::create_disks($storecfg, $vmid, $param, $conf);
 
 	    PVE::QemuServer::change_config_nolock($vmid, $param, $unset, 1);
 
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d72e07b..a97db4a 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1265,7 +1265,7 @@ sub touch_config {
 }
 
 sub create_disks {
-    my ($storecfg, $vmid, $settings) = @_;
+    my ($storecfg, $vmid, $settings, $conf) = @_;
 
     my $vollist = [];
 
@@ -1302,7 +1302,7 @@ sub create_disks {
 		    die "image '$path' does not exists\n";
 		}
 	    }
-	    PVE::QemuServer::vm_deviceadd($storecfg, $vmid, $ds, $disk);
+	    PVE::QemuServer::vm_deviceadd($storecfg, $conf, $vmid, $ds, $disk) if defined($conf);
 	});
     };
 
@@ -2272,47 +2272,75 @@ sub vm_devices_list {
     return $devices;
 }
 
+sub vm_waitfordevicecleanup {
+   my($deviceid,$vmid,$action,$time) = @_;
+
+   return undef if $time > 5;
+
+   my $devices_list = vm_devices_list($vmid);
+   if($action eq 'add') {
+      return 1 if defined($devices_list->{$deviceid});
+   }
+   elsif ($action eq 'del') {
+      return 1 if !defined($devices_list->{$deviceid});
+   }
+   sleep 1; #give a litlle time to os to add the device
+   $time++;
+   vm_waitfordevicecleanup($deviceid,$vmid,$action,$time);
+
+}
+
 sub vm_deviceadd {
-    my ($storecfg, $vmid, $deviceid, $device) = @_;
+    my ($storecfg, $conf, $vmid, $deviceid, $device) = @_;
 
-    my $cfspath = cfs_config_path($vmid);
-    my $conf = PVE::Cluster::cfs_read_file($cfspath) || {};
 
-    return if !check_running($vmid) || !$conf->{hotplug}; 
+    return if !check_running ($vmid) || $conf->{hotplug} != 1 ; # do nothing if vm is running or hotplug option not set to 1
 
     if($deviceid =~ m/^(virtio)(\d+)$/) {
 
-        my $drive = print_drive_full($storecfg, $vmid, $device);
-        vm_monitor_command($vmid, "drive_add auto $drive", 1);
-        my $devicefull = print_drivedevice_full($storecfg, $vmid, $device);
-        vm_monitor_command($vmid, "device_add $devicefull", 1);
+        my $drive = print_drive_full ($storecfg, $vmid, $device);
+        my $ret = vm_monitor_command ($vmid, "drive_add auto $drive", 1);
+        # If the command succeeds qemu prints: "OK"
+        if ($ret !~ m/OK/s) {
+           die "adding drive failed: $ret";
+        }
+       
+        my $devicefull = print_drivedevice_full ($storecfg, $vmid, $device);
+        $ret = vm_monitor_command ($vmid, "device_add $devicefull", 1);
+        $ret =~ s/^\s+//;
+         # Otherwise, if the command succeeds, no output is sent. So any non-empty string shows an error 
+        die 'error on hotplug device : $ret' if $ret ne "";
     }
 
-    #verification
-    sleep 2; #give a litlle time to os to add the device
-    my $devices_list = vm_devices_list($vmid);
-    die "error on hotplug device" if !defined($devices_list->{$deviceid});
-
+    #need to verify the device is correctly remove as device_add is async and empty result return is not reliable
+    die "error on hotplug device $deviceid" if (!defined (vm_waitfordevicecleanup($deviceid, $vmid, 'add', 0)));
 }
 
 sub vm_devicedel {
-    my ($vmid,$deviceid) = @_;
+    my ($vmid,$conf,$deviceid) = @_;
 
-    my $cfspath = cfs_config_path($vmid);
-    my $conf = PVE::Cluster::cfs_read_file($cfspath) || {};
 
-    return if !check_running ($vmid) || !$conf->{hotplug};
+    return if !check_running ($vmid) || $conf->{hotplug} != 1 ;
 
     if($deviceid =~ m/^(virtio)(\d+)$/){
 
-        vm_monitor_command($vmid, "drive_del drive-$deviceid", 1);
-        vm_monitor_command($vmid, "device_del $deviceid", 1);
+        my $ret = vm_monitor_command ($vmid, "drive_del drive-$deviceid",1);
+        $ret =~ s/^\s+//;
+        if ($ret =~ m/Device \'.*?\' not found/s) {
+            # NB: device not found errors mean the drive was auto-deleted and we ignore the error 
+        }
+        elsif ($ret ne "") {
+            die "deleting drive $deviceid failed : $ret";
+        }
+
+        $ret = vm_monitor_command ($vmid, "device_del $deviceid",1);
+        $ret =~ s/^\s+//;
+        die 'detaching device $deviceid failed : $ret' if $ret ne "";
 
     }
+    #need to verify the device is correctly remove as device_del is async and empty return is not reliable
+    die "error on hot-unplugging device $deviceid" if (!defined (vm_waitfordevicecleanup($deviceid, $vmid, 'del', 0)));
 
-    sleep 2;
-    my $devices_list = vm_devices_list($vmid);
-    die "error on hot-unplugging device " if defined($devices_list->{$deviceid});
 }
 
 sub vm_start {
-- 
1.7.2.5