Re: [pve-devel] [PATCH qemu-server v7 4/5] api: create: add 'import-working-storage' parameter

[Aaron Lauterer <a.lauterer@proxmox.com>]

On  2024-11-18  16:29, Dominik Csapak wrote:
> this is to override the target extraction storage for the option disk
> extraction for 'import-from'. This way if the storage does not
> supports the content type 'images', one can give an alternative  one.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v6:
> * rename 'import-extraction-storage' to 'import-working-storage'
> * rework permission checks (single branch)
> 
> i opted to not make the target the first default, since i did not want
> to introduce such a change this late in the patch/review cycle and
> AFAICS quite a few code changes would have been necessary for that
> (we can still change that later too)
> 
>   PVE/API2/Qemu.pm | 48 +++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index c947f09c..701558a7 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -132,7 +132,7 @@ my $check_drive_param = sub {
>   };
>   
>   my $check_storage_access = sub {
> -   my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
> +   my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage, $extraction_storage) = @_;
>   
>      $foreach_volume_with_alloc->($settings, sub {
>   	my ($ds, $drive) = @_;
> @@ -174,9 +174,18 @@ my $check_storage_access = sub {
>   		    if $vtype ne 'images' && $vtype ne 'import';
>   
>   		if (PVE::QemuServer::Helpers::needs_extraction($vtype, $fmt)) {
> -		    raise_param_exc({ $ds => "$src_image is not on an storage with 'images' content type."})
> -			if !$scfg->{content}->{images};
> -		    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
> +		    my $extraction_scfg = defined($extraction_storage) ?
> +			PVE::Storage::storage_config($storecfg, $extraction_storage) :
> +			$scfg;
> +		    my $extraction_param = defined($extraction_storage) ? 'import-working-storage' : $ds;
> +
> +		    if (!$extraction_scfg->{content}->{images} || !$extraction_scfg->{path}) {

I think the if condition here is grouped wrong.

As it is, once if one of the items (content->images or path) is falsy, 
we trigger it, but it should not be the case, if the other is actually true.

Consider the following diff, basically grouping the content->images || 
path condition, and only if the result is falsy, we want to raise the error.

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8db443d..545fe31 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -179,7 +179,7 @@ my $check_storage_access = sub {
                         $scfg;
                     my $extraction_param = defined($extraction_storage) 
? 'import-working-storage' : $ds;

-                   if (!$extraction_scfg->{content}->{images} || 
!$extraction_scfg->{path}) {
+                   if (!($extraction_scfg->{content}->{images} || 
$extraction_scfg->{path})) {
                         raise_param_exc({
                             $extraction_param => "storage selected for 
extraction does not support"
                                 ." 'images' content type or is not file 
based.",



> +			raise_param_exc({
> +			    $extraction_param => "storage selected for extraction does not support"
> +				." 'images' content type or is not file based.",
> +			});
> +		    }
> +		    $rpcenv->check($authuser, "/storage/" . ($extraction_storage // $storeid), ['Datastore.AllocateSpace']);
>   		}
>   	    }
>   
> @@ -349,7 +358,7 @@ my sub prohibit_tpm_version_change {


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel