[pve-devel] [PATCH] update_vm rework

[Alexandre DERUMIER]

>> - my $updatefn = sub { 

>>>I guess we want to keep the locking code? 

To be really honest,I dont't understand really good how the locking work,
so please correct my code ;)


> >{file},$vmid); 
>> + PVE::QemuServer::change_config_nolock($vmid, {}, { $opt => 1 }, 

>>>>Why do we write the config file here - that should not be necessary, because we do that a few lines later? 

because just after

die "error hotplug $opt - put disk in unused";



>> - $res->{$key} = $volid; 
>> + PVE::QemuServer::change_config_nolock($vmid, { $key => $volid }, {}, 1); 

>>>Why do we write the config file here - that should not be necessary? 

To be sure the config is write, if something goes wrong after (die by exemple).
I try to write the conf atomic, each time a device is changed.


----- Mail original ----- 

De: "Dietmar Maurer" <dietmar at proxmox.com> 
À: "Derumier Alexandre" <aderumier at odiso.com>, pve-devel at pve.proxmox.com 
Envoyé: Vendredi 27 Janvier 2012 06:30:04 
Objet: RE: [pve-devel] [PATCH] update_vm rework 

Thanks, just applied the patch - but I still have some question (inline): 

> - my $updatefn = sub { 

I guess we want to keep the locking code? 

> - 
> - my $conf = PVE::QemuServer::load_config($vmid); 
> - 
> - die "checksum missmatch (file change by other user?)\n" 
> - if $digest && $digest ne $conf->{digest}; 
> - 
> - PVE::QemuServer::check_lock($conf) if !$skiplock; 
> - 
> - PVE::Cluster::log_msg('info', $user, "update VM $vmid: " . join (' ', 
> @paramarr)); 

... 
> - foreach my $opt (keys %$cdchange) { 
> - my $qdn = PVE::QemuServer::qemu_drive_name($opt, 'cdrom'); 
> - my $path = $cdchange->{$opt}; 
> - PVE::QemuServer::vm_monitor_command($vmid, "eject $qdn", 
> 0); 
> - PVE::QemuServer::vm_monitor_command($vmid, "change $qdn 
> \"$path\"", 0) if $path; 
> - } 
> - }; 
> + #add 
> + foreach my $opt (keys %$param) { 
> 
> - PVE::QemuServer::lock_config($vmid, $updatefn); 

Why did you remove the locking code? 

> + #drives 
> + if (PVE::QemuServer::valid_drivename($opt)) { 
> + my $drive = PVE::QemuServer::parse_drive($opt, $param- 
> >{$opt}); 
> + raise_param_exc({ $opt => "unable to parse drive options" }) if 
> +!$drive; 

... 
> + my $settings = { $opt => $param->{$opt} }; 
> + PVE::QemuServer::create_disks($storecfg, $vmid, $settings, 
> $conf); 
> + $param->{$opt} = $settings->{$opt}; 
> + #hotplug disks 
> + if(!PVE::QemuServer::vm_deviceplug($storecfg, $conf, $vmid, $opt, 
> $drive)) { 
> + PVE::QemuServer::add_unused_volume($conf,$drive- 
> >{file},$vmid); 
> + PVE::QemuServer::change_config_nolock($vmid, {}, { $opt => 1 }, 

Why do we write the config file here - that should not be necessary, because we do that a few lines later? 

> 1); 
> + die "error hotplug $opt - put disk in unused"; 
> + } 
> + } 
> + } 
> + #nics 
> + if ($opt =~ m/^net(\d+)$/) { 
> + my $net = PVE::QemuServer::parse_net($param->{$opt}); 
> + $param->{$opt} = PVE::QemuServer::print_net($net); 
> + } 
> + 
> + PVE::QemuServer::change_config_nolock($vmid, { $opt => $param- 
> >{$opt} }, {}, 1); 
> + } 
> 
> return undef; 
> }}); 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm index f714cc8..41d9e3a 
> 100644 
> --- a/PVE/QemuServer.pm 
> +++ b/PVE/QemuServer.pm 
> @@ -1019,7 +1019,7 @@ sub add_random_macs { } 
> 
> sub add_unused_volume { 
> - my ($config, $res, $volid) = @_; 
> + my ($config, $volid, $vmid) = @_; 
> 
> my $key; 
> for (my $ind = $MAX_UNUSED_DISKS - 1; $ind >= 0; $ind--) { @@ -1033,7 
> +1033,8 @@ sub add_unused_volume { 
> 
> die "To many unused volume - please delete them first.\n" if !$key; 
> 
> - $res->{$key} = $volid; 
> + PVE::QemuServer::change_config_nolock($vmid, { $key => $volid }, {}, 1); 

Why do we write the config file here - that should not be necessary? 

- Dietmar 




-- 

-- 




	
	Alexandre Derumier 
Ingénieur système 
e-mail : aderumier at odiso.com 
Tél : +33 (0)3 20 68 88 90 
Fax : +33 (0)3 20 68 90 81 
45 Bvd du Général Leclerc 
59100 ROUBAIX - FRANCE