Re: [pve-devel] [PATCH pve-storage 1/2] update `list_volumes` to allow subdirs

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On March 3, 2023 3:50 pm, Noel Ullreich wrote:
> iterate through subdirs to find all the isos/container
> templates/snippets.

might be worth it to call out that this patch is broken without the second one,
unless you have appropriate "middle dirs" to make the unmodified REs in
get_subdir_files match.

e.g.,

$storage_path/template/iso/foobar/something.iso

doesn't work (it will return $storage:iso/something.iso as volume ID)

$storage_path/template/iso/foobar/template/iso/nested.iso

is even worse, since it's also broken *with* the second patch and returns
$storage:iso/nested.iso as volume ID.. but more about this in comments for the
second patch..

> 
> Signed-off-by: Noel Ullreich <n.ullreich@proxmox.com>
> ---
>  PVE/Storage/Plugin.pm | 65 ++++++++++++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 22 deletions(-)
> 
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index ca7a0d4..bf1d564 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -9,6 +9,7 @@ use File::chdir;
>  use File::Path;
>  use File::Basename;
>  use File::stat qw();
> +use File::Find;

File::Find docs contain the following warning about warnings ;)

WARNINGS
    If you run your program with the "-w" switch, or if you use the
    "warnings" pragma, File::Find will report warnings for several weird
    situations. You can disable these warnings by putting the statement

        no warnings 'File::Find';

    in the appropriate scope. See warnings for more info about lexical
    warnings.

this is also visible (I think?) in the list_volumes test case output when
building the package.

>  
>  use PVE::Tools qw(run_command);
>  use PVE::JSONSchema qw(get_standard_option register_standard_option);
> @@ -1275,46 +1276,66 @@ sub list_volumes {
>      my $vmlist = PVE::Cluster::get_vmlist();
>      foreach my $type (@$content_types) {
>  	my $data;
> +	my @list_of_dirs;
>  
>  	if ($type eq 'images' || $type eq 'rootdir') {
>  	    $data = $class->list_images($storeid, $scfg, $vmid);
> +	    push (@list_of_dirs, $data) if $data; #fix this

fix what? or leftover?

>  	} elsif ($scfg->{path}) {
>  	    my $path = $class->get_subdir($scfg, $type);
> +	    my @subdirs;
> +	    my $wanted = sub {
> +		my ($dev,$ino,$mode,$nlink,$uid,$gid);
> +
> +		(($dev,$ino,$mode,$nlink,$uid,$gid) = lstat($_)) &&
> +		-d _
> +		&& push(@subdirs, $File::Find::name);

nit: style (&& placement!)

do we want/need to forbid symlinks here? we don't for regular files or content
dirs IIRC.. if we want to forbid them, we'd need to also add appropriate checks
in other places whenever translation from volid to path happens..

one thing I wondered here is whether it wouldn't be easier to extend
get_subdir_files with a "$recursive" boolean, and then in the beginning of that
helper change the "next" to recurse? did you try that and it was worse (it would
save us from iterating over each dir twice and having to use File::Find..)

> +	    };
> +	    File::Find::find({wanted => $wanted, untaint => 1}, $path);
>  
>  	    if ($type eq 'iso' && !defined($vmid)) {
> -		$data = $get_subdir_files->($storeid, $path, 'iso');
> +		for (@subdirs) {

nit: I prefer

for my $dir (@subdirs) {

}

although our style guide is not really clear (just says to prefer `for` over
`foreach`), so not sure whether that is consensus ;)

> +		    $data = $get_subdir_files->($storeid, $_, 'iso',);
> +		    push @list_of_dirs, $data;
> +		}
>  	    } elsif ($type eq 'vztmpl'&& !defined($vmid)) {
> -		$data = $get_subdir_files->($storeid, $path, 'vztmpl');
> +		for (@subdirs) {
> +		    $data = $get_subdir_files->($storeid, $_, 'vztmpl');
> +		    push @list_of_dirs, $data;

so list of dirs is not actually a list of dirs? :-P

> +		}
>  	    } elsif ($type eq 'backup') {
>  		$data = $get_subdir_files->($storeid, $path, 'backup', $vmid);
> +		push(@list_of_dirs, $data) if $data;
>  	    } elsif ($type eq 'snippets') {
> -		$data = $get_subdir_files->($storeid, $path, 'snippets');
> +		for (@subdirs){
> +		    $data = $get_subdir_files->($storeid, $_, 'snippets');
> +		    push(@list_of_dirs, $data) if $data;
> +		}
>  	    }
>  	}
> -
> -	next if !$data;
> -
> -	foreach my $item (@$data) {
> -	    if ($type eq 'images' || $type eq 'rootdir') {
> -		my $vminfo = $vmlist->{ids}->{$item->{vmid}};
> -		my $vmtype;
> -		if (defined($vminfo)) {
> -		    $vmtype = $vminfo->{type};
> -		}
> -		if (defined($vmtype) && $vmtype eq 'lxc') {
> -		    $item->{content} = 'rootdir';
> +	next if !@list_of_dirs; #if list of dirs is empty, there is no data either
> +
> +	for (@list_of_dirs) {

same nit as above..

> +	    foreach my $item (@$_) {
> +		if ($type eq 'images' || $type eq 'rootdir') {
> +		    my $vminfo = $vmlist->{ids}->{$item->{vmid}};
> +		    my $vmtype;
> +		    if (defined($vminfo)) {
> +			$vmtype = $vminfo->{type};
> +		    }
> +		    if (defined($vmtype) && $vmtype eq 'lxc') {
> +			$item->{content} = 'rootdir';
> +		    } else {
> +			$item->{content} = 'images';
> +		    }
> +		    next if $type ne $item->{content};
>  		} else {
> -		    $item->{content} = 'images';
> +		    $item->{content} = $type;
>  		}
> -		next if $type ne $item->{content};
> -	    } else {
> -		$item->{content} = $type;
> +		push @$res, $item;
>  	    }
> -
> -	    push @$res, $item;
>  	}
>      }
> -
>      return $res;
>  }
>  
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>