From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id DE5749219B for ; Tue, 28 Mar 2023 15:28:01 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C04C91D067 for ; Tue, 28 Mar 2023 15:28:01 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 28 Mar 2023 15:27:58 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C98BA470B3 for ; Tue, 28 Mar 2023 15:27:57 +0200 (CEST) Date: Tue, 28 Mar 2023 15:27:51 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230303145051.109925-1-n.ullreich@proxmox.com> <20230303145051.109925-2-n.ullreich@proxmox.com> In-Reply-To: <20230303145051.109925-2-n.ullreich@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1680006783.9liwe0gegr.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.072 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH pve-storage 1/2] update `list_volumes` to allow subdirs X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Mar 2023 13:28:01 -0000 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.. >=20 > Signed-off-by: Noel Ullreich > --- > PVE/Storage/Plugin.pm | 65 ++++++++++++++++++++++++++++--------------- > 1 file changed, 43 insertions(+), 22 deletions(-) >=20 > 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. > =20 > 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 =3D PVE::Cluster::get_vmlist(); > foreach my $type (@$content_types) { > my $data; > + my @list_of_dirs; > =20 > if ($type eq 'images' || $type eq 'rootdir') { > $data =3D $class->list_images($storeid, $scfg, $vmid); > + push (@list_of_dirs, $data) if $data; #fix this fix what? or leftover? > } elsif ($scfg->{path}) { > my $path =3D $class->get_subdir($scfg, $type); > + my @subdirs; > + my $wanted =3D sub { > + my ($dev,$ino,$mode,$nlink,$uid,$gid); > + > + (($dev,$ino,$mode,$nlink,$uid,$gid) =3D 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 cont= ent dirs IIRC.. if we want to forbid them, we'd need to also add appropriate ch= ecks 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 =3D> $wanted, untaint =3D> 1}, $path); > =20 > if ($type eq 'iso' && !defined($vmid)) { > - $data =3D $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` ove= r `foreach`), so not sure whether that is consensus ;) > + $data =3D $get_subdir_files->($storeid, $_, 'iso',); > + push @list_of_dirs, $data; > + } > } elsif ($type eq 'vztmpl'&& !defined($vmid)) { > - $data =3D $get_subdir_files->($storeid, $path, 'vztmpl'); > + for (@subdirs) { > + $data =3D $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 =3D $get_subdir_files->($storeid, $path, 'backup', $vmid); > + push(@list_of_dirs, $data) if $data; > } elsif ($type eq 'snippets') { > - $data =3D $get_subdir_files->($storeid, $path, 'snippets'); > + for (@subdirs){ > + $data =3D $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 =3D $vmlist->{ids}->{$item->{vmid}}; > - my $vmtype; > - if (defined($vminfo)) { > - $vmtype =3D $vminfo->{type}; > - } > - if (defined($vmtype) && $vmtype eq 'lxc') { > - $item->{content} =3D 'rootdir'; > + next if !@list_of_dirs; #if list of dirs is empty, there is no data eit= her > + > + for (@list_of_dirs) { same nit as above.. > + foreach my $item (@$_) { > + if ($type eq 'images' || $type eq 'rootdir') { > + my $vminfo =3D $vmlist->{ids}->{$item->{vmid}}; > + my $vmtype; > + if (defined($vminfo)) { > + $vmtype =3D $vminfo->{type}; > + } > + if (defined($vmtype) && $vmtype eq 'lxc') { > + $item->{content} =3D 'rootdir'; > + } else { > + $item->{content} =3D 'images'; > + } > + next if $type ne $item->{content}; > } else { > - $item->{content} =3D 'images'; > + $item->{content} =3D $type; > } > - next if $type ne $item->{content}; > - } else { > - $item->{content} =3D $type; > + push @$res, $item; > } > - > - push @$res, $item; > } > } > - > return $res; > } > =20 > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20