[pve-devel] [PATCH storage v2 1/3] refactor disk/storage checks for Disk API

[Thomas Lamprecht]

On 9/25/18 10:38 AM, Dominik Csapak wrote:
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> changes from v1:
> * new in v2
> * only refactors the checks according to thomas feedback

nice in general, two nits inline (sorry ^^)

> 
>  PVE/API2/Disks/Directory.pm |  9 ++-------
>  PVE/API2/Disks/LVM.pm       |  2 +-
>  PVE/API2/Disks/LVMThin.pm   |  9 ++-------
>  PVE/API2/Disks/ZFS.pm       |  8 ++------
>  PVE/Diskmanage.pm           |  8 ++++++++
>  PVE/Storage.pm              | 12 ++++++++++++
>  6 files changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm
> index 9d27762..f076ee7 100644
> --- a/PVE/API2/Disks/Directory.pm
> +++ b/PVE/API2/Disks/Directory.pm
> @@ -201,13 +201,8 @@ __PACKAGE__->register_method ({
>  	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";
> -	}
> +	PVE::Diskmanage::check_unused($dev);
> +	PVE::Storage::check_available($name);
>  
>  	my $worker = sub {
>  	    my $path = "/mnt/pve/$name";
> diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm
> index 19fba07..009ee0d 100644
> --- a/PVE/API2/Disks/LVM.pm
> +++ b/PVE/API2/Disks/LVM.pm
> @@ -149,7 +149,7 @@ __PACKAGE__->register_method ({
>  	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);
> +	PVE::Diskmanage::check_unused($dev);
>  
>  	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..62d3b61 100644
> --- a/PVE/API2/Disks/LVMThin.pm
> +++ b/PVE/API2/Disks/LVMThin.pm
> @@ -103,13 +103,8 @@ __PACKAGE__->register_method ({
>  	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";
> -	}
> +	PVE::Diskmanage::check_unused($dev);
> +	PVE::Storage::check_available($name);
>  
>  	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..19ccedf 100644
> --- a/PVE/API2/Disks/ZFS.pm
> +++ b/PVE/API2/Disks/ZFS.pm
> @@ -341,14 +341,10 @@ __PACKAGE__->register_method ({
>  
>  	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);
> +	    PVE::Diskmanage::check_unused($dev);
>  	}
>  
> -	my $cfg = PVE::Storage::config();
> -
> -	if (my $scfg = PVE::Storage::storage_config($cfg, $name, 1)) {
> -	    die "storage ID '$name' already defined\n";
> -	}
> +	PVE::Storage::check_available($name);
>  
>  	my $numdisks = scalar(@$devs);
>  	my $mindisks = {
> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
> index 1938fa8..5a5fc87 100644
> --- a/PVE/Diskmanage.pm
> +++ b/PVE/Diskmanage.pm
> @@ -602,4 +602,12 @@ sub locked_disk_action {
>      return $res;
>  }
>  
> +sub check_unused {
> +    my ($dev) = @_;
> +
> +    die "device $dev is already in use\n" if disk_is_used($dev);
> +

I now the name was indirect suggest by me, but maybe
assert_disk_unused($dev);

would be nicer? because this is a real assert and looking at it now
I'd find it nicer if the method includes disk (or even better dev ?) in
its name.

> +    return undef;
> +}
> +
>  1;
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index f9732fe..81f9b67 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1647,4 +1647,16 @@ sub get_bandwidth_limit {
>      return $override;
>  }
>  
> +# checks if the storage id is available and dies if not
> +sub check_available {


assert_sid_available
(or to mirror the other new methods name: assert_sid_unused ?)

> +    my ($id) = @_;

we normally use $sid as variable name for storage ids

> +
> +    my $cfg = config();
> +    if (my $scfg = storage_config($cfg, $id, 1)) {
> +	die "storage ID '$id' already defined\n";
> +    }
> +
> +    return undef;
> +}
> +
>  1;
>