Re: [pve-devel] [PATCH storage 13/26] plugin, btrfs: update list_images and list_volumes

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

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 <w.bumiller@proxmox.com>
> ---
>  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