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.eb...@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
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
