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

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

lore.proxmox.com seems to show the diff wrong...

On  2024-11-18  18:24, Aaron Lauterer wrote:
> 
> 
> 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.",

Lets try it once more, hopefully the line endings will be as I want 
them, even on lore.proxmox.com


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.",


Also, I was able to run into this by uploading the OVA to a CephFS.

> 
> 
> 
>> +            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
> 
> 



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