From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id D48AB1FF183 for ; Wed, 30 Jul 2025 10:36:07 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 51AE1365CC; Wed, 30 Jul 2025 10:37:33 +0200 (CEST) Date: Wed, 30 Jul 2025 10:36:55 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20250729111557.136012-1-w.bumiller@proxmox.com> <20250729111557.136012-14-w.bumiller@proxmox.com> In-Reply-To: <20250729111557.136012-14-w.bumiller@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1753862076.pz6d0yemxs.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1753864607740 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.046 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 storage 13/26] plugin, btrfs: update list_images and list_volumes 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: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On July 29, 2025 1:15 pm, Wolfgang Bumiller wrote: > `list_images()` now takes a vtype. > > If it is not set, we act like we did previously by listing *all* > images. This now includes the vm-vol and ct-vol types ones. > > If a new vtype is set (vm-vol or ct-vol), then we list only those. > > For "images" we list both 'vm-vol', for "rootdir" we list both > "ct-vol" and the "rootdir" type. > > NOTE: Previously the "rootdir" `content-dirs` option did not take > effect, which is why the `list_images()` implementation for `rootdir` > uses the `images` subdir. > This means that `list_images()` in particular lists all/the same > untyped images regardless of whether `images` or `rootdir` is used as > a type. For `list_volumes()`, the previous strategy of using the > existing VMs to decide the volume-type will be used. > > Signed-off-by: Wolfgang Bumiller > --- > src/PVE/Storage/BTRFSPlugin.pm | 44 ++++++++++- > src/PVE/Storage/Plugin.pm | 139 +++++++++++++++++++++++++++++---- > 2 files changed, 163 insertions(+), 20 deletions(-) > > diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm > index d1c0cf9..585489c 100644 > --- a/src/PVE/Storage/BTRFSPlugin.pm > +++ b/src/PVE/Storage/BTRFSPlugin.pm > @@ -645,13 +645,40 @@ sub volume_has_feature { > } > > sub list_images { > - my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_; > - my $imagedir = $class->get_subdir($scfg, 'images'); > + my ($class, $storeid, $scfg, $vmid, $vollist, $cache, $content_type) = @_; > + > + my @image_dirs; > + if (defined($content_type)) { > + if ($content_type eq 'images') { > + @image_dirs = ( > + [undef, $class->get_subdir($scfg, 'images')], > + ['vm-vol', $class->get_subdir($scfg, 'vm-vol')], > + ); > + } elsif ($content_type eq 'rootdir') { > + # In the legacy case, the 'rootdir' `content-dir` option did not take > + # effect, so use the 'images' dir for it, as that is what it used to return! > + @image_dirs = ( > + [undef, $class->get_subdir($scfg, 'images')], > + ['ct-vol', $class->get_subdir($scfg, 'ct-vol')], > + ); > + } else { > + @image_dirs = [$content_type, $class->get_subdir($scfg, $content_type)]; > + } > + } else { > + @image_dirs = ( > + [undef, $class->get_subdir($scfg, 'images')], > + ['vm-vol', $class->get_subdir($scfg, 'vm-vol')], > + ['ct-vol', $class->get_subdir($scfg, 'ct-vol')], > + ); > + } > > my $res = []; > > # Copied from Plugin.pm, with file_size_info calls adapted: > - foreach my $fn (<$imagedir/[0-9][0-9]*/*>) { > + my $current_type; > + my $code = sub { > + my ($fn) = @_; > + > # different to in Plugin.pm the regex below also excludes '@' as valid file name > next if $fn !~ m@^(/.+/(\d+)/([^/\@.]+(?:\.(qcow2|vmdk|subvol))?))$@; > $fn = $1; # untaint > @@ -701,9 +728,20 @@ sub list_images { > parent => $parent, > }; > > + # Only add vtype if it is not 'images'... > + $info->{vtype} = $current_type if defined($current_type); > + > $info->{ctime} = $ctime if $ctime; > > push @$res, $info; > + }; > + > + my %dedup; > + for my $dir_entry (@image_dirs) { > + ($current_type, my $dir) = @$dir_entry; > + next if $dedup{$dir}; > + $dedup{$dir} = 1; > + PVE::Storage::Plugin::foreach_guest_file($dir, $code); > } > > return $res; > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm > index 047b2fc..660045d 100644 > --- a/src/PVE/Storage/Plugin.pm > +++ b/src/PVE/Storage/Plugin.pm > @@ -10,8 +10,9 @@ use File::chdir; > use File::Path; > use File::Basename; > use File::stat qw(); > +use IO::Dir; > > -use PVE::Tools qw(run_command); > +use PVE::Tools qw(dir_glob_foreach run_command); > use PVE::JSONSchema qw(get_standard_option register_standard_option); > use PVE::Cluster qw(cfs_register_file); > > @@ -1567,36 +1568,91 @@ sub volume_has_feature { > > return undef; > } > +# > +# Given an volume directory, this iterates over vmid directories and recurses > +# once to the files inside. > +# > +# In other words, this is `glob($dir/[0-9][0-9]*/*)`. > +my $MAX_VMID; > + > +sub foreach_guest_file : prototype($$) { should this go into PVE::Storage::Common? > + my ($dir, $code) = @_; > + > + $MAX_VMID = get_standard_option("pve-vmid")->{maximum} if !defined($MAX_VMID); > + > + dir_glob_foreach( > + $dir, > + qr/\d+/, > + sub { > + my ($vmid) = @_; > + $vmid = int($vmid); > + return if $vmid < 100 || $vmid > $MAX_VMID; this now means list_images lists less than before - somebody might have used a very high ID as a placeholder not realizing its outside the "allowed" range.. not sure it's worth it to have this restriction here.. > + my $dir = "$dir/$vmid"; > + my $dh = IO::Dir->new($dir) or return; > + while (defined(my $entry = $dh->read)) { > + next if $entry eq '.' || $entry eq '..'; > + $code->("$dir/$entry"); > + } > + close $dh; > + }, > + ); > +} > > sub list_images { > - my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_; > - > - my $imagedir = $class->get_subdir($scfg, 'images'); > + my ($class, $storeid, $scfg, $vmid, $vollist, $cache, $content_type) = @_; > + > + my @image_dirs; > + if (defined($content_type)) { > + if ($content_type eq 'images') { > + @image_dirs = ( > + [undef, $class->get_subdir($scfg, 'images')], > + ['vm-vol', $class->get_subdir($scfg, 'vm-vol')], > + ); > + } elsif ($content_type eq 'rootdir') { > + # In the legacy case, the 'rootdir' `content-dir` option did not take > + # effect, so use the 'images' dir for it, as that is what it used to return! > + @image_dirs = ( > + [undef, $class->get_subdir($scfg, 'images')], > + ['ct-vol', $class->get_subdir($scfg, 'ct-vol')], > + ); > + } else { > + @image_dirs = [$content_type, $class->get_subdir($scfg, $content_type)]; > + } > + } else { > + @image_dirs = ( > + [undef, $class->get_subdir($scfg, 'images')], > + ['vm-vol', $class->get_subdir($scfg, 'vm-vol')], > + ['ct-vol', $class->get_subdir($scfg, 'ct-vol')], > + ); > + } > > my $format_info = $class->get_formats($scfg, $storeid); > my $fmts = join('|', sort keys $format_info->{valid}->%*); > > my $res = []; > > - foreach my $fn (<$imagedir/[0-9][0-9]*/*>) { > + my $current_type; > + my $code = sub { > + my ($fn) = @_; > > - next if $fn !~ m!^(/.+/(\d+)/([^/]+\.($fmts)))$!; > + return if $fn !~ m!^(/.+/(\d+)/([^/]+\.($fmts)))$!; > $fn = $1; # untaint > > my $owner = $2; > my $name = $3; > my $format = $4; > > - next if !$vollist && defined($vmid) && ($owner ne $vmid); > + return if !$vollist && defined($vmid) && ($owner ne $vmid); > > - my ($size, undef, $used, $parent, $ctime) = eval { file_size_info($fn, undef, $format); }; > + my ($size, undef, $used, $parent, $ctime) = > + eval { file_size_info($fn, undef, $format); }; > if (my $err = $@) { > die $err if $err !~ m/Image is not in \S+ format$/; > warn "image '$fn' is not in expected format '$format', querying as raw\n"; > ($size, undef, $used, $parent, $ctime) = file_size_info($fn, undef, 'raw'); > $format = 'invalid'; > } > - next if !defined($size); > + return if !defined($size); > > my $volid; > if ($parent && $parent =~ m!^../(\d+)/([^/]+\.($fmts))$!) { > @@ -1608,7 +1664,7 @@ sub list_images { > > if ($vollist) { > my $found = grep { $_ eq $volid } @$vollist; > - next if !$found; > + return if !$found; > } > > my $info = { > @@ -1620,9 +1676,19 @@ sub list_images { > parent => $parent, > }; > > + # Only add vtype if it is not 'images'... > + $info->{vtype} = $current_type if defined($current_type); > $info->{ctime} = $ctime if $ctime; > > push @$res, $info; > + }; > + > + my %dedup; > + for my $dir_entry (@image_dirs) { > + ($current_type, my $dir) = @$dir_entry; > + next if $dedup{$dir}; > + $dedup{$dir} = 1; > + foreach_guest_file($dir, $code); > } > > return $res; > @@ -1709,12 +1775,29 @@ sub list_volumes { > > my $res = []; > my $vmlist = PVE::Cluster::get_vmlist(); > + > + my $guest_dirs = 0; > + my $DIRS_IMAGES = 1 << 0; > + my $DIRS_VMS = 1 << 1; > + my $DIRS_CTS = 1 << 2; > + my $DIRS_ALL = $DIRS_IMAGES | $DIRS_VMS | $DIRS_CTS; > foreach my $type (@$content_types) { > my $data; > + if ($type eq 'images') { > + $guest_dirs |= $DIRS_IMAGES | $DIRS_VMS; > + next; > + } elsif ($type eq 'rootdir') { > + $guest_dirs |= $DIRS_IMAGES | $DIRS_CTS; > + next; > + } elsif ($type eq 'vm-vol') { > + $guest_dirs |= $DIRS_VMS; > + next; > + } elsif ($type eq 'ct-vol') { > + $guest_dirs |= $DIRS_CTS; > + next; > + } > > - if ($type eq 'images' || $type eq 'rootdir') { > - $data = $class->list_images($storeid, $scfg, $vmid); > - } elsif ($scfg->{path}) { > + if ($scfg->{path}) { > my $path = $class->get_subdir($scfg, $type); > > if ($type eq 'iso' && !defined($vmid)) { > @@ -1733,7 +1816,32 @@ sub list_volumes { > next if !$data; > > foreach my $item (@$data) { > - if ($type eq 'images' || $type eq 'rootdir') { > + $item->{content} = $type; > + push @$res, $item; > + } > + } > + > + if ($guest_dirs) { > + my $data; > + > + if ($guest_dirs == $DIRS_ALL) { > + $data = $class->list_images($storeid, $scfg, $vmid, undef, undef, undef); > + } elsif ($guest_dirs == ($DIRS_IMAGES | $DIRS_VMS)) { > + $data = $class->list_images($storeid, $scfg, $vmid, undef, undef, 'images'); > + } elsif ($guest_dirs == ($DIRS_IMAGES | $DIRS_CTS)) { > + $data = $class->list_images($storeid, $scfg, $vmid, undef, undef, 'rootdir'); > + } elsif ($guest_dirs == $DIRS_VMS) { > + $data = $class->list_images($storeid, $scfg, $vmid, undef, undef, 'vm-vol'); > + } elsif ($guest_dirs == $DIRS_CTS) { > + $data = $class->list_images($storeid, $scfg, $vmid, undef, undef, 'ct-vol'); > + } else { > + die "unexpected request to list only untyped images\n"; > + } > + > + for my $item (@$data) { > + if (defined(my $vtype = $item->{vtype})) { > + $item->{content} = $vtype; > + } else { > my $vminfo = $vmlist->{ids}->{ $item->{vmid} }; > my $vmtype; > if (defined($vminfo)) { > @@ -1744,9 +1852,6 @@ sub list_volumes { > } else { > $item->{content} = 'images'; > } > - next if $type ne $item->{content}; > - } else { > - $item->{content} = $type; > } > > push @$res, $item; > -- > 2.47.2 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel