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

[Dietmar Maurer]

Please can you fix the following issues.

Btw, you changes contains trailing white spaces at some lines.

> 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) = @_;

coding style - please use whit space between parameters (and after 'my')

  my ($deviceid, $vmid, $action, $time) = @_;

> 
>      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++;

Can you remove that recursive function call?

> +   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

I would not use such inline comment - the code is quite obvious.

> 
>      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);

I already fixed the coding style, and you revert that back? Please change.

> +        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);

no space after function name please.

> +        $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) = @_;

coding stype (white space).

> 
> -    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
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel