[pve-devel] [PATCH storage 1/3] add new content type 'scripts'

[Thomas Lamprecht]

On 1/21/19 9:44 AM, Dominik Csapak wrote:
> will be used to contain executable files which can be executed as
> hookscripts

looks mostly good which gets reinforced by the fact that most of my complains are
whitespace/coding style only ;)

> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/Storage.pm        | 60 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  PVE/Storage/Plugin.pm |  3 +++
>  2 files changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 89a6c71..12e5c79 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -772,6 +772,60 @@ sub vdisk_free {
>      $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker);
>  }
>  
> +# lists all files in the scripts directory which are exectuable
> +sub script_list {
> +    my ($cfg, $storeid) = @_;
> +
> +    my $ids = $cfg->{ids};
> +
> +    storage_check_enabled($cfg, $storeid) if ($storeid);
> +
> +    my $res = {};
> +
> +    # query the storage
> +
> +    foreach my $sid (keys %$ids) {
> +	next if $storeid && $storeid ne $sid;
> +
> +	my $scfg = $ids->{$sid};
> +	my $type = $scfg->{type};

you do not use type, the single time you could you also have "$scfg->{type}"

Also, this method use really much whitespaces, to the point that it decreases
readability, IMO. Maybe try to keep logical stuff grouped together. E.g.,
all next ifs really near the top, no newlines.

> +
> +	next if !storage_check_enabled($cfg, $sid, undef, 1);
> +

s/\n//

> +	next if !$scfg->{content}->{scripts};
> +
> +	activate_storage($cfg, $sid);
> +
> +	if ($scfg->{path}) {
> +	    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> +	    my $path = $plugin->get_subdir($scfg, 'scripts');
> +
> +	    foreach my $fn (<$path/*>) {
> +
> +		my $info;

why my $info here? You set it only once so move it there and merge definition
and declaration.

> +
> +		next if ! -x $fn || -d $fn;

above directly below the loop header please, and maybe swap the checks as the
chance that a directory is traversable (for us here), and thus executable, is
higher than the other way around, so excluding directory firsts lets the check
abort earlier more often (surely totally noticeable, VMs will run twice as fast)

> +
> +		my $base = basename($fn);
> +
> +		$info = {
> +		    volid => "$sid:scripts/$base",
> +		    format => 'script',
> +		    size => -s $fn,
> +		};
> +
> +		push @{$res->{$sid}}, $info;
> +	    }
> +
> +	}
> +
> +	@{$res->{$sid}} = sort {lc($a->{volid}) cmp lc ($b->{volid}) } @{$res->{$sid}} if $res->{$sid};

please transform the postfix if to a "normal" one. Also, is the lowercase
really needed? Does it not break sort stability?

> +    }
> +
> +    return $res;
> +}
> +
>  #list iso or openvz template ($tt = <iso|vztmpl|backup>)
>  sub template_list {
>      my ($cfg, $storeid, $tt) = @_;
> @@ -887,7 +941,7 @@ sub vdisk_list {
>  sub volume_list {
>      my ($cfg, $storeid, $vmid, $content) = @_;
>  
> -    my @ctypes = qw(images vztmpl iso backup);
> +    my @ctypes = qw(images vztmpl iso backup scripts);
>  
>      my $cts = $content ? [ $content ] : [ @ctypes ];
>  
> @@ -909,6 +963,8 @@ sub volume_list {
>  		    @{$data->{$storeid}} = grep { $_->{volid} =~ m/\S+-$vmid-\S+/ } @{$data->{$storeid}};
>  		}
>  	    }
> +	} elsif ($ct eq 'scripts') {
> +	    $data = script_list($cfg, $storeid);
>  	}
>  
>  	next if !$data || !$data->{$storeid};
> @@ -1518,7 +1574,7 @@ sub complete_storage_enabled {
>  sub complete_content_type {
>      my ($cmdname, $pname, $cvalue) = @_;
>  
> -    return [qw(rootdir images vztmpl iso backup)];
> +    return [qw(rootdir images vztmpl iso backup scripts)];
>  }
>  
>  sub complete_volume {
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index e0c2a4e..4990255 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -427,6 +427,8 @@ sub parse_volname {
>  	    return ('backup', $fn, $2);
>  	}
>  	return ('backup', $fn);
> +    } elsif ($volname =~ m!^scripts/([^/]+)$!) {
> +	return ('scripts', $1);
>      }
>  
>      die "unable to parse directory volume name '$volname'\n";
> @@ -438,6 +440,7 @@ my $vtype_subdirs = {
>      iso => 'template/iso',
>      vztmpl => 'template/cache',
>      backup => 'dump',
> +    scripts => 'scripts',
>  };
>  
>  sub get_subdir {
>