[pve-devel] [storage] Add option check if the image can safely remove

[Dietmar Maurer]

comments inline

> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
> index e941cb6..5c75d87 100644
> --- a/PVE/API2/Storage/Content.pm
> +++ b/PVE/API2/Storage/Content.pm
> @@ -285,6 +285,11 @@ __PACKAGE__->register_method ({
>  		type => 'string',
>  		completion => \&PVE::Storage::complete_volume,
>  	    },
> +	    check => {
> +		description => "Check if the image can remove safely",
> +		type => 'boolean',
> +		optional => 1,
> +	    },
>  	},
>      },
>      returns => { type => 'null' },
> @@ -295,8 +300,10 @@ __PACKAGE__->register_method ({
>  	my $authuser = $rpcenv->get_user();
>  
>  	my $cfg = PVE::Storage::config();
> +	my $volume = PVE::Tools::extract_param($param, 'volume');
> +	my $node = PVE::Tools::extract_param($param, 'node');
>  
> -	my ($volid, $storeid) = &$real_volume_id($param->{storage}, $param->{volume});
> +	my ($volid, $storeid) = &$real_volume_id($param->{storage}, $volume);
>  
>  	my ($path, $ownervm, $vtype) = PVE::Storage::path($cfg, $volid);

Note: we already have the $ownervm !

>  	if ($vtype eq 'backup' && $ownervm) {
> @@ -306,6 +313,21 @@ __PACKAGE__->register_method ({
>  	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.Allocate']);
>  	}
>  
> +	if ($param->{check}) {
> +
> +		my $vmid = (PVE::Storage::parse_volname($cfg, $volume))[2];

Why not simply use the $ownervm here. Also, some volumes do not have an owner.

> +		my $vms = PVE::Cluster::get_vmlist();
> +
> +		my $guest_exist = defined($vms->{ids}->{$vmid});
> +		my $guest_on_localhost = $guest_exist && $vms->{ids}->{$vmid}->{node} eq $node;

Not sure if is is a good idea to use the $node parameter here (which is allowed to be 'localhost').
Maybe better to compare with the local nodename?

> +
> +		my $storage_is_shared = $cfg->{ids}->{$storeid}->{shared};
> +		$storage_is_shared = defined($storage_is_shared) ? $storage_is_shared : 0;

above 2 lines looks quite strange, at least I do not understand what it does exactly?. Simple use:

my $storage_is_shared = defined($cfg->{ids}->{$storeid}->{shared});

> +
> +		die "Guest: $vmid exist on local node. Remove image from guest Hardware.\n"
> +			if $guest_on_localhost || ($guest_exist && $storage_is_shared);
> +	}
> +
>  	PVE::Storage::vdisk_free ($cfg, $volid);
>  
>  	return undef;
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel