From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 70EC61FF15F for ; Mon, 18 Nov 2024 18:24:53 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9041A16142; Mon, 18 Nov 2024 18:24:56 +0100 (CET) Message-ID: <683efffa-4ef1-4295-bb13-6828026e1f6c@proxmox.com> Date: Mon, 18 Nov 2024 18:24:52 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Proxmox VE development discussion , Dominik Csapak References: <20241118152928.858590-1-d.csapak@proxmox.com> <20241118152928.858590-16-d.csapak@proxmox.com> From: Aaron Lauterer In-Reply-To: <20241118152928.858590-16-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.035 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH qemu-server v7 4/5] api: create: add 'import-working-storage' parameter X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 > --- > 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