[pve-devel] [PATCH qemu-server 4/7] Remove variable from lock

[Thomas Lamprecht]

"remove variable from lock" is as generic and obscure as it gets,
could be anything ^^ Please use a real commit subject.. concise and
intended for human digestions (e.g., no full paths, if not an essential
part for human understanding)

On 10/25/19 11:24 AM, Dominic Jäger wrote:
> The variable was used in one place only. Nesting the code into the
> lock_config_full makes clear what code is locked.

the purpose was the same as stated in my reply to your patch 2/7,
slightly better readability than just a seemingly arbitrary passed "1".
Albeit the name and even the value feels a bit off...

> 
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
>  PVE/CLI/qm.pm | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index c93f78d..a378d3d 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -625,10 +625,9 @@ __PACKAGE__->register_method ({
>  	$param->{memory} = $parsed->{qm}->{memory} if defined($parsed->{qm}->{memory});
>  	$param->{cores} = $parsed->{qm}->{cores} if defined($parsed->{qm}->{cores});
>  
> -	my $importfn = sub {
> -
> +	# why is wait_for_lock exactly 1?

why this comment? :P

The rationale, AFAICT, was the assumption that for import to work the VMID has to be
unused, and thus this either goes through fast or not at all (soon), thus a short
timeout - if someone else has a lock on this the VMID has relative high chance to
fail anyway..

In itself valid assumptions and good intentions, but not much of use as this is
local node only, and so other nodes can still create one in-between, so a 
QemuConfig->create_and_lock_config(...)
at the beginning would be more useful..

> +	PVE::QemuConfig->lock_config_full($vmid, 1, sub {
>  	    PVE::Cluster::check_vmid_unused($vmid);
> -
>  	    my $conf = $param;
>  
>  	    eval {
> @@ -649,16 +648,12 @@ __PACKAGE__->register_method ({
>  		$conf->{bootdisk} = $firstdisk if $firstdisk;
>  		PVE::QemuConfig->write_config($vmid, $conf);
>  	    };
> -
>  	    my $err = $@;
>  	    if ($err) {
>  		eval { PVE::QemuServer::destroy_vm($storecfg, $vmid, undef, 1); };
>  		die "import failed - $err";
>  	    }
> -	};
> -
> -	my $wait_for_lock = 1;
> -	PVE::QemuConfig->lock_config_full($vmid, $wait_for_lock, $importfn);
> +	});
>  
>  	return undef;
>  
>