[pve-devel] [PATCH qemu-server 7/7] Use parameter lock for importovf

[Thomas Lamprecht]

On 10/25/19 11:24 AM, Dominic Jäger wrote:
> Previously creating a VM on another node between locking the config with
> lock_config_full and writing to it for the first time with write_config
> was possible.
> 

> Using create_and_lock_config eliminates this possibility. This means
> that now a parameter lock in the config is used instead of flock only.

parameter lock is slightly confusing, while I get were it comes from it's
not really used as terminology for this ^^ Most of the time a "locked config"
is a configuration with the "lock" property set.

as subject something like:
"import OVF: created locked config early to avoid VMID conflict"
could be nice.

> 
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
>  PVE/CLI/qm.pm | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 97f0f88..3bf8e81 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -621,14 +621,14 @@ __PACKAGE__->register_method ({
>  	    return;
>  	}
>  
> -	$param->{name} = $parsed->{qm}->{name} if defined($parsed->{qm}->{name});
> -	$param->{memory} = $parsed->{qm}->{memory} if defined($parsed->{qm}->{memory});
> -	$param->{cores} = $parsed->{qm}->{cores} if defined($parsed->{qm}->{cores});
> +	eval { PVE::QemuConfig->create_and_lock_config($vmid, 0); };

FYI, last statement in blocks do not need to have a semicolon after them,
so `eval { foo() };` is OK and can make one-liner looks slightly nicer.
But, probably just my personal preference, thus just FYI ;)

> +	die "Import failed: $@" if $@;

maybe:
"Reserving empty config for OVF import failed: $@";

>  
> -	# why is wait_for_lock exactly 1?
> -	PVE::QemuConfig->lock_config_full($vmid, 1, sub {
> -	    PVE::Cluster::check_vmid_unused($vmid);
> -	    my $conf = $param;
> +	my $conf = PVE::QemuConfig->load_config($vmid);
> +	if (PVE::QemuConfig->has_lock($conf, "create")) {

rather do a "return early" pattern here:

die "internal error, expected 'create' lock" if !PVE::QemuConfig->has_lock($conf, "create");

then the rest of the code below can move on intend-level out (easier to
read)

> +	    $conf->{name} = $parsed->{qm}->{name} if defined($parsed->{qm}->{name});
> +	    $conf->{memory} = $parsed->{qm}->{memory} if defined($parsed->{qm}->{memory});
> +	    $conf->{cores} = $parsed->{qm}->{cores} if defined($parsed->{qm}->{cores});
>  
>  	    eval {
>  		# order matters, as do_import() will load_config() internally
> @@ -639,7 +639,7 @@ __PACKAGE__->register_method ({
>  		foreach my $disk (@{ $parsed->{disks} }) {
>  		    my ($file, $drive) = ($disk->{backing_file}, $disk->{disk_address});
>  		    PVE::QemuServer::ImportDisk::do_import($file, $vmid, $storeid,
> -			0, { drive_name => $drive, format => $format });
> +			1, { drive_name => $drive, format => $format });
>  		}
>  
>  		# reload after disks entries have been created
> @@ -653,7 +653,10 @@ __PACKAGE__->register_method ({
>  		PVE::QemuServer::destroy_vm($storecfg, $vmid, undef, 1);
>  		die "import failed - $err";
>  	    }
> -	});
> +	    PVE::QemuConfig->remove_lock ($vmid, "create");
> +	} else {
> +	    die "Import failed as 'create' lock has gone lost!";> +	}
>  
>  	return undef;
>  
>