Re: [pve-devel] [PATCH] don't try to hotplug disk if a disk already exist.

2014-08-29 Thread Dietmar Maurer 
 -} else { # hotplug new disks
 -
 +} elsif (!$old_volid) { # hotplug new disks
   die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg,
 $conf, $vmid, $opt, $drive);
  }

This does not display any errors if $old_volid is set?
I think we should raise an error to indicate that something went wrong?

___
pve-devel mailing list
pve-devel@pve.proxmox.com
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH] don't try to hotplug disk if a disk already exist.

2014-08-29 Thread Dietmar Maurer 
what about this:

} else { # hotplug new disks
+   die some useful error mesage if $old_volid;
die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg, 
$conf, $vmid, $opt, $drive);
}
}

 -Original Message-
 From: Alexandre DERUMIER [mailto:aderum...@odiso.com]
 Sent: Freitag, 29. August 2014 09:25
 To: Dietmar Maurer
 Cc: pve-devel@pve.proxmox.com
 Subject: Re: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
 exist.
 
 This does not display any errors if $old_volid is set?
 I think we should raise an error to indicate that something went wrong?
 
 
 
 
 Maybe
 
 elsif (!$old_volid) { # hotplug new disks
die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg,
 $conf, $vmid, $opt, $drive);
 
 }elseif ($old_voldid  $old_voldid ne $new_volid {
   raise an error ?
 }
 
 
 ?
 
 - Mail original -
 
 De: Dietmar Maurer diet...@proxmox.com
 À: Alexandre Derumier aderum...@odiso.com, pve-
 de...@pve.proxmox.com
 Envoyé: Vendredi 29 Août 2014 08:29:00
 Objet: RE: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
 exist.
 
  - } else { # hotplug new disks
  -
  + } elsif (!$old_volid) { # hotplug new disks
  die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg,
  $conf, $vmid, $opt, $drive);
  }
 
 This does not display any errors if $old_volid is set?
 I think we should raise an error to indicate that something went wrong?

___
pve-devel mailing list
pve-devel@pve.proxmox.com
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH] don't try to hotplug disk if a disk already exist.

2014-08-29 Thread Alexandre DERUMIER 
what about this:

} else { # hotplug new disks
+ die some useful error mesage if $old_volid;
die error hotplug $opt if 
 !PVE::QemuServer::vm_deviceplug($storecfg, $conf, $vmid, $opt, $drive);
}
}

The problem is that we are in $vmconfig_update_disk(),

so it'll die if we try to update any parameters (disk throttle,discard,backup).


- Mail original - 

De: Dietmar Maurer diet...@proxmox.com 
À: Alexandre DERUMIER aderum...@odiso.com 
Cc: pve-devel@pve.proxmox.com 
Envoyé: Vendredi 29 Août 2014 10:11:18 
Objet: RE: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
exist. 

what about this: 

} else { # hotplug new disks 
+ die some useful error mesage if $old_volid; 
die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg, $conf, 
$vmid, $opt, $drive); 
} 
} 

 -Original Message- 
 From: Alexandre DERUMIER [mailto:aderum...@odiso.com] 
 Sent: Freitag, 29. August 2014 09:25 
 To: Dietmar Maurer 
 Cc: pve-devel@pve.proxmox.com 
 Subject: Re: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
 exist. 
 
 This does not display any errors if $old_volid is set? 
 I think we should raise an error to indicate that something went wrong? 
 
 
 
 
 Maybe 
 
 elsif (!$old_volid) { # hotplug new disks 
 die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg, 
 $conf, $vmid, $opt, $drive); 
 
 }elseif ($old_voldid  $old_voldid ne $new_volid { 
 raise an error ? 
 } 
 
 
 ? 
 
 - Mail original - 
 
 De: Dietmar Maurer diet...@proxmox.com 
 À: Alexandre Derumier aderum...@odiso.com, pve- 
 de...@pve.proxmox.com 
 Envoyé: Vendredi 29 Août 2014 08:29:00 
 Objet: RE: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
 exist. 
 
  - } else { # hotplug new disks 
  - 
  + } elsif (!$old_volid) { # hotplug new disks 
  die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg, 
  $conf, $vmid, $opt, $drive); 
  } 
 
 This does not display any errors if $old_volid is set? 
 I think we should raise an error to indicate that something went wrong? 
___
pve-devel mailing list
pve-devel@pve.proxmox.com
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH] don't try to hotplug disk if a disk already exist.

2014-08-29 Thread Alexandre DERUMIER 
We need also to check if the vm is running and hotplug is enabled

(because without hotplug, we are allow to replace a disk with another, 
 the code manage currently the swap, putting the first disk as unused)

something like:
elsif (PVE::QemuServer::check_running($vmid)  $conf-{hotplug}) { # 
hotplug new disks
if($old_volid){
die you need to remove current disk before hotplug it if 
$old_volid ne $volid;
}else{
die error hotplug $opt if 
!PVE::QemuServer::vm_deviceplug($storecfg, $conf, $vmid, $opt, $drive);
}
}



also, I found another bug, we update the conf with new_volid  before trying 
hotplug.
If hotplug fail, we had the wrong disk updated in config

I'll check the whole vmconfig_update_disk sub, to see to improve that



- Mail original - 

De: Dietmar Maurer diet...@proxmox.com 
À: Alexandre DERUMIER aderum...@odiso.com 
Cc: pve-devel@pve.proxmox.com 
Envoyé: Vendredi 29 Août 2014 11:26:00 
Objet: RE: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
exist. 

 what about this: 
  
  } else { # hotplug new disks 
 + die some useful error mesage if $old_volid; 
  die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg, 
 $conf, $vmid, $opt, $drive); 
  } 
 } 
 
 The problem is that we are in $vmconfig_update_disk(), 

die some useful error mesage if $old_volid  $old_volid ne $new_volid; 

 
 so it'll die if we try to update any parameters (disk 
 throttle,discard,backup). 
 
 
 - Mail original - 
 
 De: Dietmar Maurer diet...@proxmox.com 
 À: Alexandre DERUMIER aderum...@odiso.com 
 Cc: pve-devel@pve.proxmox.com 
 Envoyé: Vendredi 29 Août 2014 10:11:18 
 Objet: RE: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
 exist. 
 
 what about this: 
 
 } else { # hotplug new disks 
 + die some useful error mesage if $old_volid; 
 die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg, $conf, 
 $vmid, $opt, $drive); } } 
 
  -Original Message- 
  From: Alexandre DERUMIER [mailto:aderum...@odiso.com] 
  Sent: Freitag, 29. August 2014 09:25 
  To: Dietmar Maurer 
  Cc: pve-devel@pve.proxmox.com 
  Subject: Re: [pve-devel] [PATCH] don't try to hotplug disk if a disk 
  already 
 exist. 
  
  This does not display any errors if $old_volid is set? 
  I think we should raise an error to indicate that something went wrong? 
  
  
  
  
  Maybe 
  
  elsif (!$old_volid) { # hotplug new disks die error hotplug $opt if 
  !PVE::QemuServer::vm_deviceplug($storecfg, 
  $conf, $vmid, $opt, $drive); 
  
  }elseif ($old_voldid  $old_voldid ne $new_volid { raise an error ? 
  } 
  
  
  ? 
  
  - Mail original - 
  
  De: Dietmar Maurer diet...@proxmox.com 
  À: Alexandre Derumier aderum...@odiso.com, pve- 
  de...@pve.proxmox.com 
  Envoyé: Vendredi 29 Août 2014 08:29:00 
  Objet: RE: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
  exist. 
  
   - } else { # hotplug new disks 
   - 
   + } elsif (!$old_volid) { # hotplug new disks 
   die error hotplug $opt if 
   !PVE::QemuServer::vm_deviceplug($storecfg, 
   $conf, $vmid, $opt, $drive); 
   } 
  
  This does not display any errors if $old_volid is set? 
  I think we should raise an error to indicate that something went wrong? 
___
pve-devel mailing list
pve-devel@pve.proxmox.com
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH] don't try to hotplug disk if a disk already exist.

2014-08-29 Thread Alexandre DERUMIER 
 if($old_volid){
die you need to remove current disk before hotplug it if 
 $old_volid ne $volid;

also, I think $old_voldid is put as unused before in update_disk

 if (!PVE::QemuServer::drive_is_cdrom($old_drive) 
($drive-{file} ne $old_drive-{file})) {  # delete old disks

$vmconfig_delete_option($rpcenv, $authuser, $conf, $storecfg, 
$vmid, $opt, $force);
$conf = PVE::QemuServer::load_config($vmid); # update/reload
}

as far I remember, it's working with hot-unplug too.

I'll do tests.



- Mail original - 

De: Alexandre DERUMIER aderum...@odiso.com 
À: Dietmar Maurer diet...@proxmox.com 
Cc: pve-devel@pve.proxmox.com 
Envoyé: Vendredi 29 Août 2014 12:22:49 
Objet: Re: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
exist. 

We need also to check if the vm is running and hotplug is enabled 

(because without hotplug, we are allow to replace a disk with another, 
the code manage currently the swap, putting the first disk as unused) 

something like: 
elsif (PVE::QemuServer::check_running($vmid)  $conf-{hotplug}) { # hotplug 
new disks 
if($old_volid){ 
die you need to remove current disk before hotplug it if $old_volid ne 
$volid; 
}else{ 
die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg, $conf, 
$vmid, $opt, $drive); 
} 
} 



also, I found another bug, we update the conf with new_volid before trying 
hotplug. 
If hotplug fail, we had the wrong disk updated in config 

I'll check the whole vmconfig_update_disk sub, to see to improve that 



- Mail original - 

De: Dietmar Maurer diet...@proxmox.com 
À: Alexandre DERUMIER aderum...@odiso.com 
Cc: pve-devel@pve.proxmox.com 
Envoyé: Vendredi 29 Août 2014 11:26:00 
Objet: RE: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
exist. 

 what about this: 
  
  } else { # hotplug new disks 
 + die some useful error mesage if $old_volid; 
  die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg, 
 $conf, $vmid, $opt, $drive); 
  } 
 } 
 
 The problem is that we are in $vmconfig_update_disk(), 

die some useful error mesage if $old_volid  $old_volid ne $new_volid; 

 
 so it'll die if we try to update any parameters (disk 
 throttle,discard,backup). 
 
 
 - Mail original - 
 
 De: Dietmar Maurer diet...@proxmox.com 
 À: Alexandre DERUMIER aderum...@odiso.com 
 Cc: pve-devel@pve.proxmox.com 
 Envoyé: Vendredi 29 Août 2014 10:11:18 
 Objet: RE: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
 exist. 
 
 what about this: 
 
 } else { # hotplug new disks 
 + die some useful error mesage if $old_volid; 
 die error hotplug $opt if !PVE::QemuServer::vm_deviceplug($storecfg, $conf, 
 $vmid, $opt, $drive); } } 
 
  -Original Message- 
  From: Alexandre DERUMIER [mailto:aderum...@odiso.com] 
  Sent: Freitag, 29. August 2014 09:25 
  To: Dietmar Maurer 
  Cc: pve-devel@pve.proxmox.com 
  Subject: Re: [pve-devel] [PATCH] don't try to hotplug disk if a disk 
  already 
 exist. 
  
  This does not display any errors if $old_volid is set? 
  I think we should raise an error to indicate that something went wrong? 
  
  
  
  
  Maybe 
  
  elsif (!$old_volid) { # hotplug new disks die error hotplug $opt if 
  !PVE::QemuServer::vm_deviceplug($storecfg, 
  $conf, $vmid, $opt, $drive); 
  
  }elseif ($old_voldid  $old_voldid ne $new_volid { raise an error ? 
  } 
  
  
  ? 
  
  - Mail original - 
  
  De: Dietmar Maurer diet...@proxmox.com 
  À: Alexandre Derumier aderum...@odiso.com, pve- 
  de...@pve.proxmox.com 
  Envoyé: Vendredi 29 Août 2014 08:29:00 
  Objet: RE: [pve-devel] [PATCH] don't try to hotplug disk if a disk already 
  exist. 
  
   - } else { # hotplug new disks 
   - 
   + } elsif (!$old_volid) { # hotplug new disks 
   die error hotplug $opt if 
   !PVE::QemuServer::vm_deviceplug($storecfg, 
   $conf, $vmid, $opt, $drive); 
   } 
  
  This does not display any errors if $old_volid is set? 
  I think we should raise an error to indicate that something went wrong? 
___ 
pve-devel mailing list 
pve-devel@pve.proxmox.com 
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 
___
pve-devel mailing list
pve-devel@pve.proxmox.com
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel