Re: [pve-devel] [PATCH storage 14/26] plugins: update image/volume listing to support new types

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

On July 29, 2025 1:15 pm, Wolfgang Bumiller wrote:
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
>  src/PVE/Storage/Common.pm            | 31 ++++++++++++++++++++++++++
>  src/PVE/Storage/ISCSIDirectPlugin.pm |  5 ++++-
>  src/PVE/Storage/ISCSIPlugin.pm       |  7 ++++--
>  src/PVE/Storage/LVMPlugin.pm         | 28 +++++++++++++----------
>  src/PVE/Storage/LvmThinPlugin.pm     | 33 ++++++++++++++++++----------
>  src/PVE/Storage/PBSPlugin.pm         |  2 +-
>  src/PVE/Storage/RBDPlugin.pm         |  9 ++++++--
>  src/PVE/Storage/ZFSPoolPlugin.pm     | 16 +++++++++++++-
>  8 files changed, 102 insertions(+), 29 deletions(-)
> 
> diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm
> index b2ebd50..bab1260 100644
> --- a/src/PVE/Storage/Common.pm
> +++ b/src/PVE/Storage/Common.pm
> @@ -282,4 +282,35 @@ sub qemu_img_resize {
>      run_command($cmd, timeout => $timeout);
>  }
>  
> +=head3 should_list_images($expected_content_type, $volume_type)
> +
> +Returns whether a volume of type C<$volume_type> should be listed in C<list_images> if the expected
> +content type is C<$expcted_content_type>.
> +
> +This effectively checks if C<$expected_content_type> is a "supertype" of C<$volume_type>.
> +
> +=cut
> +
> +sub should_list_images : prototype($$) ($expected_content_type, $volume_type) {
> +    # If we have no expected type, everything should be listed.
> +    return 1 if !defined($expected_content_type);
> +
> +    if (!$volume_type) {
> +        # For 'images' and 'rootdir', an unknown volume type should also be listed.
> +        return 1
> +            if $expected_content_type eq 'images' || $expected_content_type eq 'rootdir';
> +        # Otherwise unknown volume types are unexpected.
> +        return 0;
> +    }
> +
> +    # Images should also include vm-vol.
> +    return 1 if $expected_content_type eq 'images' && $volume_type eq 'vm-vol';
> +
> +    # Rootdir should also include ct-vol.
> +    return 1 if $expected_content_type eq 'rootdir' && $volume_type eq 'ct-vol';

could we drop this if we made pve-container and qemu-server list twice,
once with the legacy and once with the new type? AFAICT the only user of
this is `PVE::Storage::vdisk_list` and list_volumes?

it seems like a potential footgun to have the legacy types refer to just
the legacy types sometimes, but both legacy and new types in other
places..

> +
> +    # Otherwise they must be equal.
> +    return $expected_content_type eq $volume_type;
> +}
> +
>  1;
> diff --git a/src/PVE/Storage/ISCSIDirectPlugin.pm b/src/PVE/Storage/ISCSIDirectPlugin.pm
> index f5b466e..98a3391 100644
> --- a/src/PVE/Storage/ISCSIDirectPlugin.pm
> +++ b/src/PVE/Storage/ISCSIDirectPlugin.pm
> @@ -152,10 +152,12 @@ sub free_image {
>  }
>  
>  sub list_images {
> -    my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
> +    my ($class, $storeid, $scfg, $vmid, $vollist, $cache, $content_type) = @_;
>  
>      my $res = [];
>  
> +    return $res if $content_type && $content_type ne 'images' && $content_type ne 'vm-vol';
> +
>      # we have no owner for iscsi devices
>  
>      my $dat = iscsi_ls($scfg);
> @@ -172,6 +174,7 @@ sub list_images {
>  
>          my $info = $dat->{$volname};
>          $info->{volid} = $volid;
> +        $info->{vtype} = 'vm-vol';
>  
>          push @$res, $info;
>      }
> diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
> index 4875a1f..77a9173 100644
> --- a/src/PVE/Storage/ISCSIPlugin.pm
> +++ b/src/PVE/Storage/ISCSIPlugin.pm
> @@ -421,17 +421,19 @@ sub list_volumes {
>      my $res = $class->list_images($storeid, $scfg, $vmid);
>  
>      for my $item (@$res) {
> -        $item->{content} = 'images'; # we only have images
> +        $item->{content} = 'vm-vol'; # we only have VM images
>      }
>  
>      return $res;
>  }
>  
>  sub list_images {
> -    my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
> +    my ($class, $storeid, $scfg, $vmid, $vollist, $cache, $content_type) = @_;
>  
>      my $res = [];
>  
> +    return $res if $content_type && $content_type ne 'images' && $content_type ne 'vm-vol';
> +
>      $cache->{iscsi_devices} = iscsi_device_list() if !$cache->{iscsi_devices};
>  
>      # we have no owner for iscsi devices
> @@ -454,6 +456,7 @@ sub list_images {
>  
>              my $info = $dat->{$volname};
>              $info->{volid} = $volid;
> +            $info->{vtype} = 'vm-vol';
>  
>              push @$res, $info;
>          }
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index 9b88c6a..55c4578 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -750,7 +750,7 @@ my $check_tags = sub {
>  };
>  
>  sub list_images {
> -    my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
> +    my ($class, $storeid, $scfg, $vmid, $vollist, $cache, $content_type) = @_;
>  
>      my $vgname = $scfg->{vgname};
>  
> @@ -762,8 +762,11 @@ sub list_images {
>  
>          foreach my $volname (keys %$dat) {
>  
> -            next if $volname !~ m/^vm-(\d+)-/;
> -            my $owner = $1;
> +            next if $volname !~ m/^(?:vol-(?<vtype>vm|ct)-|vm)-(?<owner>\d+)-/xn;
> +            my $owner = $+{owner};
> +            my $vtype = $+{vtype} ? $+{vtype} . '-vol' : undef;
> +
> +            next if !PVE::Storage::Common::should_list_images($content_type, $vtype);
>  
>              my $info = $dat->{$volname};
>  
> @@ -787,14 +790,17 @@ sub list_images {
>                  ? $class->volume_size_info($scfg, $storeid, $volname)
>                  : $info->{lv_size};
>  
> -            push @$res,
> -                {
> -                    volid => $volid,
> -                    format => $format,
> -                    size => $size,
> -                    vmid => $owner,
> -                    ctime => $info->{ctime},
> -                };
> +            my $entry = {
> +                volid => $volid,
> +                format => $format,
> +                size => $size,
> +                vmid => $owner,
> +                ctime => $info->{ctime},
> +            };
> +
> +            $entry->{vtype} = $vtype if defined($vtype);
> +
> +            push @$res, $entry;
>          }
>      }
>  
> diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
> index 751bd7b..f6615d9 100644
> --- a/src/PVE/Storage/LvmThinPlugin.pm
> +++ b/src/PVE/Storage/LvmThinPlugin.pm
> @@ -182,7 +182,7 @@ sub free_image {
>  }
>  
>  sub list_images {
> -    my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
> +    my ($class, $storeid, $scfg, $vmid, $vollist, $cache, $content_type) = @_;
>  
>      my $vgname = $scfg->{vgname};
>  
> @@ -193,9 +193,17 @@ sub list_images {
>      if (my $dat = $cache->{lvs}->{$vgname}) {
>  
>          foreach my $volname (keys %$dat) {
> +            my ($owner, $vtype);
> +            if ($volname =~ m/^(?:base-)?vol-(ct|vm)-(\d+)-/) {
> +                $vtype = "$1-vol";
> +                $owner = $2;
> +            } elsif ($volname =~ m/^(?:vm|base)-(\d+)-/) {
> +                $owner = $1;
> +            } else {
> +                next;
> +            }
>  
> -            next if $volname !~ m/^(vm|base)-(\d+)-/;
> -            my $owner = $2;
> +            next if !PVE::Storage::Common::should_list_images($content_type, $vtype);
>  
>              my $info = $dat->{$volname};
>  
> @@ -212,14 +220,17 @@ sub list_images {
>                  next if defined($vmid) && ($owner ne $vmid);
>              }
>  
> -            push @$res,
> -                {
> -                    volid => $volid,
> -                    format => 'raw',
> -                    size => $info->{lv_size},
> -                    vmid => $owner,
> -                    ctime => $info->{ctime},
> -                };
> +            my $entry = {
> +                volid => $volid,
> +                format => 'raw',
> +                size => $info->{lv_size},
> +                vmid => $owner,
> +                ctime => $info->{ctime},
> +            };
> +
> +            $entry->{vtype} = $vtype if defined($vtype);
> +
> +            push @$res, $entry;
>          }
>      }
>  
> diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
> index 00170f5..bab026b 100644
> --- a/src/PVE/Storage/PBSPlugin.pm
> +++ b/src/PVE/Storage/PBSPlugin.pm
> @@ -674,7 +674,7 @@ sub free_image {
>  }
>  
>  sub list_images {
> -    my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
> +    my ($class, $storeid, $scfg, $vmid, $vollist, $cache, $content_type) = @_;
>  
>      my $res = [];
>  
> diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
> index 4400aeb..2ef1280 100644
> --- a/src/PVE/Storage/RBDPlugin.pm
> +++ b/src/PVE/Storage/RBDPlugin.pm
> @@ -773,7 +773,7 @@ sub free_image {
>  }
>  
>  sub list_images {
> -    my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
> +    my ($class, $storeid, $scfg, $vmid, $vollist, $cache, $content_type) = @_;
>  
>      my $dat = rbd_ls($scfg, $storeid);
>      return [] if !$dat; # nothing found
> @@ -783,12 +783,17 @@ sub list_images {
>          my $info = $dat->{$image};
>          my ($volname, $parent, $owner) = $info->@{ 'name', 'parent', 'vmid' };
>  
> -        if ($parent && $parent =~ m/^(base-\d+-\S+)\@__base__$/) {
> +        my $vtype;
> +
> +        if ($parent && $parent =~ m/^(base(?:-vol-(?<vtype>vm|ct))?-\d+-\S+)\@__base__$/xn) {
> +            $vtype = $+{vtype} ? $+{vtype} . '-vol' : undef;
>              $info->{volid} = "$storeid:$1/$volname";
>          } else {
>              $info->{volid} = "$storeid:$volname";
>          }
>  
> +        next if !PVE::Storage::Common::should_list_images($content_type, $vtype);
> +
>          if ($vollist) {
>              my $found = grep { $_ eq $info->{volid} } @$vollist;
>              next if !$found;
> diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
> index 8e917b4..d65af69 100644
> --- a/src/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/src/PVE/Storage/ZFSPoolPlugin.pm
> @@ -122,6 +122,16 @@ sub zfs_parse_zvol_list {
>      return $list;
>  }
>  
> +my sub image_vtype_from_name : prototype($) {
> +    my ($name) = @_;
> +
> +    return 'ct-vol' if $name =~ /^(base-)?subvol(-ct)?-/;
> +    return 'ct-vol' if $name =~ /^basevol-/;
> +    return 'vm-vol' if $name =~ /^(base-)?vol(-vm)?-/;
> +    return 'vm-vol' if $name =~ /^base-/;
> +    return 'images';
> +}
> +
>  sub parse_volname {
>      my ($class, $volname) = @_;
>  
> @@ -305,7 +315,7 @@ sub free_image {
>  }
>  
>  sub list_images {
> -    my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
> +    my ($class, $storeid, $scfg, $vmid, $vollist, $cache, $content_type) = @_;
>  
>      my $zfs_list = $class->zfs_list_zvol($scfg);
>  
> @@ -330,6 +340,10 @@ sub list_images {
>              next if defined($vmid) && ($owner ne $vmid);
>          }
>  
> +        my $vtype = volume_type_from_name($volname);
> +        next if !PVE::Storage::Common::should_list_images($content_type, $vtype);
> +        $info->{vtype} = $vtype if $vtype ne 'images' && $vtype ne 'rootdir';
> +
>          push @$res, $info;
>      }
>      return $res;
> -- 
> 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