[pve-devel] [PATCH storage] fix #1929: refactor requirements check for disks

[Thomas Lamprecht]

On 9/21/18 4:32 PM, Dominik Csapak wrote:
> and check correctly that the storage exists only when the user
> want to create a storage

strange line break here

> this is useful if you want to create the same storage on
> multiple nodes
> 

could we please split this the next time?
1. Refactor out common stuff
2. fix check for add-storage

> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/API2/Disks/Directory.pm |  9 +--------
>  PVE/API2/Disks/LVM.pm       |  3 +--
>  PVE/API2/Disks/LVMThin.pm   |  9 +--------
>  PVE/API2/Disks/ZFS.pm       | 11 +----------
>  PVE/Diskmanage.pm           | 23 +++++++++++++++++++++++
>  5 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm
> index 9d27762..0a3afdb 100644
> --- a/PVE/API2/Disks/Directory.pm
> +++ b/PVE/API2/Disks/Directory.pm
> @@ -200,14 +200,7 @@ __PACKAGE__->register_method ({
>  	my $node = $param->{node};
>  	my $type = $param->{filesystem} // 'ext4';
>  
> -	$dev = PVE::Diskmanage::verify_blockdev_path($dev);
> -	die "device $dev is already in use\n" if PVE::Diskmanage::disk_is_used($dev);
> -
> -	my $cfg = PVE::Storage::config();
> -
> -	if (my $scfg = PVE::Storage::storage_config($cfg, $name, 1)) {
> -	    die "storage ID '$name' already defined\n";
> -	}
> +	$dev = PVE::Diskmanage::assert_disk_requirements($name, $dev, $param->{add_storage});
>  
>  	my $worker = sub {
>  	    my $path = "/mnt/pve/$name";
> diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm
> index 19fba07..9c66705 100644
> --- a/PVE/API2/Disks/LVM.pm
> +++ b/PVE/API2/Disks/LVM.pm
> @@ -148,8 +148,7 @@ __PACKAGE__->register_method ({
>  	my $dev = $param->{device};
>  	my $node = $param->{node};
>  
> -	$dev = PVE::Diskmanage::verify_blockdev_path($dev);
> -	die "device $dev is already in use\n" if PVE::Diskmanage::disk_is_used($dev);
> +	$dev = PVE::Diskmanage::assert_disk_requirements($name, $dev, $param->{add_storage});
>  
>  	my $worker = sub {
>  	    PVE::Diskmanage::locked_disk_action(sub {
> diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm
> index 5b72c8c..46630e3 100644
> --- a/PVE/API2/Disks/LVMThin.pm
> +++ b/PVE/API2/Disks/LVMThin.pm
> @@ -102,14 +102,7 @@ __PACKAGE__->register_method ({
>  	my $dev = $param->{device};
>  	my $node = $param->{node};
>  
> -	$dev = PVE::Diskmanage::verify_blockdev_path($dev);
> -	die "device $dev is already in use\n" if PVE::Diskmanage::disk_is_used($dev);
> -
> -	my $cfg = PVE::Storage::config();
> -
> -	if (my $scfg = PVE::Storage::storage_config($cfg, $name, 1)) {
> -	    die "storage ID '$name' already defined\n";
> -	}
> +	$dev = PVE::Diskmanage::assert_disk_requirements($name, $dev, $param->{add_storage});
>  
>  	my $worker = sub {
>  	    PVE::Diskmanage::locked_disk_action(sub {
> diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
> index 3c36ef9..b7329f6 100644
> --- a/PVE/API2/Disks/ZFS.pm
> +++ b/PVE/API2/Disks/ZFS.pm
> @@ -339,16 +339,7 @@ __PACKAGE__->register_method ({
>  	my $ashift = $param->{ashift} // 12;
>  	my $compression = $param->{compression} // 'on';
>  
> -	foreach my $dev (@$devs) {
> -	    $dev = PVE::Diskmanage::verify_blockdev_path($dev);
> -	    die "device $dev is already in use\n" if PVE::Diskmanage::disk_is_used($dev);
> -	}
> -
> -	my $cfg = PVE::Storage::config();
> -
> -	if (my $scfg = PVE::Storage::storage_config($cfg, $name, 1)) {
> -	    die "storage ID '$name' already defined\n";
> -	}
> +	$devs = PVE::Diskmanage::assert_disk_requirements($name, $devs, $param->{add_storage});
>  
>  	my $numdisks = scalar(@$devs);
>  	my $mindisks = {
> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
> index 1938fa8..7cc0d0d 100644
> --- a/PVE/Diskmanage.pm
> +++ b/PVE/Diskmanage.pm
> @@ -602,4 +602,27 @@ sub locked_disk_action {
>      return $res;
>  }
>  
> +sub assert_disk_requirements {

strange naming? asserts normally do not transform and/or return data structures,
they check if a predicate is true as in all other case the current view of things
are false and the code should abort (die in the case of perl).


> +    my ($name, $device, $addstorage) = @_;
> +
> +    if (ref($device) eq 'ARRAY') {
> +	foreach my $dev (@$device) {
> +	    $dev = verify_blockdev_path($dev);

same here, albeit this method already exists, when reading one wonders what
this may return, verify sound like it would just return a boolean (i.e., the
language equivalent to boolean ;) )

> +	    die "device $dev is already in use\n" if disk_is_used($dev);
> +	}
> +    } elsif (!ref($device)) {
> +	$device = verify_blockdev_path($device);
> +	die "device $device is already in use\n" if disk_is_used($device);
> +    }

also not sure if it would be nicer if a single disk get checked only, and
the caller iterates over the checks...

> +
> +    if ($addstorage) {
> +	my $cfg = PVE::Storage::config();
> +	if (my $scfg = PVE::Storage::storage_config($cfg, $name, 1)) {
> +	    die "storage ID '$name' already defined\n";
> +	}
> +    }

is this a disk_requirement? It'd say no. Maybe it's better to add
a storage_exists($storeid, [optional $scfg]) to PVE::Storage?

Then you just could do a:

PVE::Storage::storage_exists($name) if $param->{add_storage};

to each API call, it's more explicit there without duplicating
code. (naming, signature are - as always - loosely thought out suggestions only)

> +
> +    return $device;
> +}
> +
>  1;
>