[pve-devel] [PATCH storage 2/9] plugin: add get_formats() method and use it instead of default_format()

[Wolfgang Bumiller]

minor nit but the whole storage side LGTM

On Mon, Jul 21, 2025 at 02:10:47PM +0200, Fiona Ebner wrote:
> The LVM plugin can only use qcow2 format when the
> 'snapshot-as-volume-chain' configuration option is set. The format
> information is currently only recorded statically in the plugin data.
> This causes issues, for example, restoring a guest volume that uses
> qcow2 as a format hint on an LVM storage without the option set will
> fail, because the plugin data indicates that qcow2 is supported.
> Introduce a dedicated method, so that plugins can indicate what
> actually is supported according to the storage configuration.
> 
> The implementation for LVM is done in a separate commit.
> 
> Remove the now unused default_format() function from Plugin.pm.
> 
> Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>
> ---
>  ApiChangeLog              |  8 ++++++
>  src/PVE/Storage.pm        | 19 +++++++------
>  src/PVE/Storage/Plugin.pm | 59 ++++++++++++++++++++++++++-------------
>  3 files changed, 59 insertions(+), 27 deletions(-)
> 
> diff --git a/ApiChangeLog b/ApiChangeLog
> index 0984afb..d80bfb3 100644
> --- a/ApiChangeLog
> +++ b/ApiChangeLog
> @@ -49,6 +49,14 @@ Future changes should be documented in here.
>        NOTE: Storages must support using "current" as a special name in `rename_snapshot()` to
>        cheaply convert a snapshot into the current disk state and back.
>  
> +* Introduce `get_formats()` plugin method
> +
> +  Get information about the supported formats and default format according to the current storage
> +  configuration. The default implemenation is backwards-compatible with previous behavior and looks
> +  at the definition given in the plugin data, as well as the `format` storage configuration option,
> +  which can override the default format. Must be implemented when the supported formats or default
> +  format depend on the storage configuration.
> +
>  ##  Version 11:
>  
>  * Allow declaring storage features via plugin data
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 6ca9f88..1faf893 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -857,10 +857,11 @@ my $volname_for_storage = sub {
>  
>      my $scfg = storage_config($cfg, $storeid);
>  
> -    my (undef, $valid_formats) = PVE::Storage::Plugin::default_format($scfg);
> -    my $format_is_valid = grep { $_ eq $format } @$valid_formats;
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> +    my $formats = $plugin->get_formats($scfg, $storeid);
>      die "unsupported format '$format' for storage type $scfg->{type}\n"
> -        if !$format_is_valid;
> +        if !$formats->{valid}->{$format};
>  
>      (my $name_without_extension = $name) =~ s/\.$format$//;
>  
> @@ -1184,14 +1185,12 @@ sub vdisk_alloc {
>  
>      $vmid = parse_vmid($vmid);
>  
> -    my $defformat = PVE::Storage::Plugin::default_format($scfg);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>  
> -    $fmt = $defformat if !$fmt;
> +    $fmt = $plugin->get_formats($scfg, $storeid)->{default} if !$fmt;
>  
>      activate_storage($cfg, $storeid);
>  
> -    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> -
>      # lock shared storage
>      return $plugin->cluster_lock_storage(
>          $storeid,
> @@ -1673,8 +1672,12 @@ sub storage_default_format {
>      my ($cfg, $storeid) = @_;
>  
>      my $scfg = storage_config($cfg, $storeid);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>  
> -    return PVE::Storage::Plugin::default_format($scfg);
> +    my $formats = $plugin->get_formats($scfg, $storeid);
> +
> +    return
> +        wantarray ? ($formats->{default}, [sort keys $formats->{valid}->%*]) : $formats->{default};
>  }
>  
>  sub vgroup_is_used {
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index ef04cb1..c3c1b63 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -287,23 +287,6 @@ sub storage_has_feature {
>      return;
>  }
>  
> -sub default_format {
> -    my ($scfg) = @_;
> -
> -    my $type = $scfg->{type};
> -    my $def = $defaultData->{plugindata}->{$type};
> -
> -    my $def_format = 'raw';
> -    my $valid_formats = [$def_format];
> -
> -    if (defined($def->{format})) {
> -        $def_format = $scfg->{format} || $def->{format}->[1];
> -        $valid_formats = [sort keys %{ $def->{format}->[0] }];
> -    }
> -
> -    return wantarray ? ($def_format, $valid_formats) : $def_format;
> -}
> -
>  PVE::JSONSchema::register_format('pve-storage-path', \&verify_path);
>  
>  sub verify_path {
> @@ -640,6 +623,44 @@ sub preallocation_cmd_opt {
>  
>  # Storage implementation
>  
> +=pod
> +

↑ can be dropped

> +=head3 get_formats
> +
> +    my $formats = $plugin->get_formats($scfg);

↑ misses the `$storeid` parameter

> +    my $default_format = $formats->{default};
> +    my $is_valid = $formats->{valid}->{$format} ? 1 : 0;

↑ could just use !! instead of `? 1 : 0`

> +
> +Get information about the supported formats and default format according to the current storage
> +configuration C<$scfg>. The return value is a hash reference with C<default> mapping to the default
> +format and C<valid> mapping to a hash reference, where each supported format is present as a key
> +mapping to C<1>. For example:
> +
> +    {
> +        default => 'raw',
> +        valid => {
> +            'qcow2 => 1,
> +            'raw' => 1,
> +        },
> +    }
> +
> +=cut
> +
> +sub get_formats {
> +    my ($class, $scfg, $storeid) = @_;
> +
> +    my $type = $scfg->{type};
> +    my $plugin_data = $defaultData->{plugindata}->{$type};
> +
> +    return { default => 'raw', valid => { raw => 1 } } if !defined($plugin_data->{format});
> +
> +    return {
> +        default => $scfg->{format} || $plugin_data->{format}->[1],
> +        # copy rather than passing direct reference
> +        valid => { $plugin_data->{format}->[0]->%* },
> +    };
> +}
> +
>  # called during addition of storage (before the new storage config got written)
>  # die to abort addition if there are (grave) problems
>  # NOTE: runs in a storage config *locked* context
> @@ -1526,8 +1547,8 @@ sub list_images {
>  
>      my $imagedir = $class->get_subdir($scfg, 'images');
>  
> -    my ($defFmt, $vaidFmts) = default_format($scfg);
> -    my $fmts = join('|', @$vaidFmts);
> +    my $format_info = $class->get_formats($scfg, $storeid);
> +    my $fmts = join('|', sort keys $format_info->{valid}->%*);
>  
>      my $res = [];
>  
> -- 
> 2.47.2