[pve-devel] applied: [PATCH pve-client] implement config file locking

[Dietmar Maurer]

applied

> On June 15, 2018 at 12:29 PM Dietmar Maurer <dietmar at proxmox.com> wrote:
> 
> 
> Signed-off-by: Dietmar Maurer <dietmar at proxmox.com>
> ---
>  PVE/APIClient/Commands/config.pm | 57 +++++++++++++++--------------
>  PVE/APIClient/Commands/remote.pm | 78
> ++++++++++++++++++++++++----------------
>  PVE/APIClient/Config.pm          | 12 +++++++
>  3 files changed, 90 insertions(+), 57 deletions(-)
> 
> diff --git a/PVE/APIClient/Commands/config.pm
> b/PVE/APIClient/Commands/config.pm
> index d467b4c..3b208a0 100644
> --- a/PVE/APIClient/Commands/config.pm
> +++ b/PVE/APIClient/Commands/config.pm
> @@ -43,35 +44,37 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> -	# fixme: lock config file
> -
>  	my $digest = extract_param($param, 'digest');
>  	my $delete = extract_param($param, 'delete');
>  
> -	my $config = PVE::APIClient::Config->load();
> -	my $defaults = PVE::APIClient::Config->get_defaults($config);
> -	
> -	my $plugin = PVE::APIClient::Config->lookup('defaults');
> -	my $opts = $plugin->check_config('defaults', $param, 0, 1);
> -
> -	foreach my $k (%$opts) {
> -	    $defaults->{$k} = $opts->{$k};
> -	}
> -
> -	if ($delete) {
> -	    my $options = $plugin->private()->{options}->{'defaults'};
> -	    foreach my $k (PVE::APIClient::Tools::split_list($delete)) {
> -		my $d = $options->{$k} ||
> -		    die "no such option '$k'\n";
> -		die "unable to delete required option '$k'\n"
> -		    if !$d->{optional};
> -		die "unable to delete fixed option '$k'\n"
> -		    if $d->{fixed};
> -		delete $defaults->{$k};
> +	my $code = sub {
> +	    my $config = PVE::APIClient::Config->load();
> +	    my $defaults = PVE::APIClient::Config->get_defaults($config);
> +
> +	    my $plugin = PVE::APIClient::Config->lookup('defaults');
> +	    my $opts = $plugin->check_config('defaults', $param, 0, 1);
> +
> +	    foreach my $k (%$opts) {
> +		$defaults->{$k} = $opts->{$k};
>  	    }
> -	}
>  
> -	PVE::APIClient::Config->save($config);
> +	    if ($delete) {
> +		my $options = $plugin->private()->{options}->{'defaults'};
> +		foreach my $k (PVE::APIClient::Tools::split_list($delete)) {
> +		    my $d = $options->{$k} ||
> +			die "no such option '$k'\n";
> +		    die "unable to delete required option '$k'\n"
> +			if !$d->{optional};
> +		    die "unable to delete fixed option '$k'\n"
> +			if $d->{fixed};
> +		    delete $defaults->{$k};
> +		}
> +	    }
> +
> +	    PVE::APIClient::Config->save($config);
> +	};
> +
> +	PVE::APIClient::Config->lock_config(undef, $code);
>  
>  	return undef;
>      }});
> diff --git a/PVE/APIClient/Commands/remote.pm
> b/PVE/APIClient/Commands/remote.pm
> index 090672e..03960af 100644
> --- a/PVE/APIClient/Commands/remote.pm
> +++ b/PVE/APIClient/Commands/remote.pm
> @@ -3,6 +3,7 @@ package PVE::APIClient::Commands::remote;
>  use strict;
>  use warnings;
>  
> +use PVE::APIClient::Helpers;
>  use PVE::APIClient::JSONSchema qw(get_standard_option);
>  use PVE::APIClient::Tools qw(extract_param);
>  use PVE::APIClient::Config;
> @@ -51,10 +52,9 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> -	# fixme: lock config file
> -
>  	my $remote = $param->{name};
>  
> +	# Note: we try to keep lock time sort, and lock later when we have all info
>  	my $config = PVE::APIClient::Config->load();
>  
>  	die "Remote '$remote' already exists\n"
> @@ -90,11 +90,25 @@ __PACKAGE__->register_method ({
>  	$api->login();
>  
>  	$param->{fingerprint} = $last_fp if !defined($param->{fingerprint});
> +
>  	my $plugin = PVE::APIClient::Config->lookup('remote');
> -	my $opts = $plugin->check_config($remote, $param, 1, 1);
> -	$config->{ids}->{$remote} = $opts;
>  
> -	PVE::APIClient::Config->save($config);
> +	my $code = sub {
> +
> +	    $config = PVE::APIClient::Config->load(); # reload
> +
> +	    # check again (file is locked now)
> +	    die "Remote '$remote' already exists\n"
> +		if $config->{ids}->{$remote};
> +
> +	    my $opts = $plugin->check_config($remote, $param, 1, 1);
> +
> +	    $config->{ids}->{$remote} = $opts;
> +
> +	    PVE::APIClient::Config->save($config);
> +	};
> +
> +	PVE::APIClient::Config->lock_config(undef, $code);
>  
>  	return undef;
>      }});
> @@ -109,36 +123,38 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> -	# fixme: lock config file
> -
>  	my $name = extract_param($param, 'name');
>  	my $digest = extract_param($param, 'digest');
>  	my $delete = extract_param($param, 'delete');
>  
> -	my $config = PVE::APIClient::Config->load();
> -	my $remote = PVE::APIClient::Config->lookup_remote($config, $name);
> +	my $code = sub {
> +	    my $config = PVE::APIClient::Config->load();
> +	    my $remote = PVE::APIClient::Config->lookup_remote($config, $name);
>  
> -	my $plugin = PVE::APIClient::Config->lookup('remote');
> -	my $opts = $plugin->check_config($name, $param, 0, 1);
> +	    my $plugin = PVE::APIClient::Config->lookup('remote');
> +	    my $opts = $plugin->check_config($name, $param, 0, 1);
>  
> -	foreach my $k (%$opts) {
> -	    $remote->{$k} = $opts->{$k};
> -	}
> +	    foreach my $k (%$opts) {
> +		$remote->{$k} = $opts->{$k};
> +	    }
>  
> -	if ($delete) {
> -	    my $options = $plugin->private()->{options}->{'remote'};
> -	    foreach my $k (PVE::APIClient::Tools::APIClient::split_list($delete)) {
> -		my $d = $options->{$k} ||
> -		    die "no such option '$k'\n";
> -		die "unable to delete required option '$k'\n"
> -		    if !$d->{optional};
> -		die "unable to delete fixed option '$k'\n"
> -		    if $d->{fixed};
> -		delete $remote->{$k};
> +	    if ($delete) {
> +		my $options = $plugin->private()->{options}->{'remote'};
> +		foreach my $k (PVE::APIClient::Tools::APIClient::split_list($delete)) {
> +		    my $d = $options->{$k} ||
> +			die "no such option '$k'\n";
> +		    die "unable to delete required option '$k'\n"
> +			if !$d->{optional};
> +		    die "unable to delete fixed option '$k'\n"
> +			if $d->{fixed};
> +		    delete $remote->{$k};
> +		}
>  	    }
> -	}
>  
> -	PVE::APIClient::Config->save($config);
> +	    PVE::APIClient::Config->save($config);
> +	};
> +
> +	PVE::APIClient::Config->lock_config(undef, $code);
>  
>  	return undef;
>      }});
> @@ -158,11 +174,13 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> -	# fixme: lock config
> +	my $code = sub {
> +	    my $config = PVE::APIClient::Config->load();
> +	    delete $config->{ids}->{$param->{name}};
> +	    PVE::APIClient::Config->save($config);
> +	};
>  
> -	my $config = PVE::APIClient::Config->load();
> -	delete $config->{ids}->{$param->{name}};
> -	PVE::APIClient::Config->save($config);
> +	PVE::APIClient::Config->lock_config(undef, $code);
>  
>  	return undef;
>      }});
> diff --git a/PVE/APIClient/Config.pm b/PVE/APIClient/Config.pm
> index 3878425..a4aa4c6 100644
> --- a/PVE/APIClient/Config.pm
> +++ b/PVE/APIClient/Config.pm
> @@ -64,6 +64,18 @@ sub config_filename {
>      return "$dir/config";
>  }
>  
> +sub lock_config {
> +    my ($class, $timeout, $code, @param) = @_;
> +
> +    my $filename = $class->config_filename();
> +
> +    my $res = PVE::APIClient::Tools::lock_file($filename, $timeout, $code,
> @param);
> +
> +    die $@ if $@;
> +
> +    return $res;
> +}
> +
>  sub format_section_header {
>      my ($class, $type, $sectionId, $scfg, $done_hash) = @_;
>  
> -- 
> 2.11.0
> 
>