[pve-devel] [PATCH pve-storage v2 3/5] tree-wide: make use of content type assertion helper

[Fiona Ebner]

Am 11.02.25 um 17:07 schrieb Daniel Kral:
> Make any code path with an existent content type assertion use the newly
> introduced content type assertion helper.
> 
> As those code paths must perform actions on the storage anyway, the
> `storage_check_enabled` in the helper subroutine adds an additional
> precondition check without breaking the existing APIs with a new error.
> 

So here you do talk about storage_check_enabled(). Did you maybe send an
incorrect version of the previous patch ;)?

> Signed-off-by: Daniel Kral <d.kral at proxmox.com>

With the previous patch fixed:

Reviewed-by: Fiona Ebner <f.ebner at proxmox.com>

However, see below:

> ---
> changes since v1:
> - new!
> 
>  src/PVE/API2/Storage/Status.pm | 6 ++----
>  src/PVE/Storage.pm             | 3 ++-
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
> index c854b53..e5652f4 100644
> --- a/src/PVE/API2/Storage/Status.pm
> +++ b/src/PVE/API2/Storage/Status.pm
> @@ -478,8 +478,7 @@ __PACKAGE__->register_method ({
>  	    raise_param_exc({ content => "upload content type '$content' not allowed" });
>  	}
>  
> -	die "storage '$storage' does not support '$content' content\n"
> -	    if !$scfg->{content}->{$content};
> +	PVE::Storage::assert_content_type_supported($cfg, $storage, $content, $node);

Above here is already a storage_check_enabled() check that would become
superfluous and could be removed. While it doesn't hurt to keep, I'm
wondering if we can better encode the semantics for the new helper in
its name and get rid of the duplicate check after all. Also to make it
easier for future usages to remember that the enabled check is already
done too. Maybe calling the helper assert_content_type_available() or to
be rather explicit assert_storage_ready_for_content_type() would make it
clear that it means that both, the storage is enabled on the node and
the content type is configured for the storage? Other suggestions are
welcome!

>  
>  	my $dest = "$path/$filename";
>  	my $dirname = dirname($dest);
> @@ -660,8 +659,7 @@ __PACKAGE__->register_method({
>  
>  	my ($content, $url) = $param->@{'content', 'url'};
>  
> -	die "storage '$storage' is not configured for content-type '$content'\n"
> -	    if !$scfg->{content}->{$content};
> +	PVE::Storage::assert_content_type_supported($cfg, $storage, $content, $node);

Similar here.

>  
>  	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
>  
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index ca69cd6..0134893 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -1816,7 +1816,8 @@ sub prune_backups {
>      my ($cfg, $storeid, $keep, $vmid, $type, $dryrun, $logfunc) = @_;
>  
>      my $scfg = storage_config($cfg, $storeid);
> -    die "storage '$storeid' does not support backups\n" if !$scfg->{content}->{backup};
> +
> +    PVE::Storage::assert_content_type_supported($cfg, $storeid, "backup");
>  
>      if (!defined($keep)) {
>  	die "no prune-backups options configured for storage '$storeid'\n"