[pve-devel] [PATCH] update_vm rework

[Dietmar Maurer]

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