From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <pve-devel-bounces@lists.proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id D91B41FF15C for <inbox@lore.proxmox.com>; Wed, 19 Feb 2025 16:16:17 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E29802DECA; Wed, 19 Feb 2025 16:16:11 +0100 (CET) Message-ID: <a61d4129-190a-46a8-82fc-8e1ae9419335@proxmox.com> Date: Wed, 19 Feb 2025 16:16:08 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>, Daniel Kral <d.kral@proxmox.com> References: <20250211160825.254167-1-d.kral@proxmox.com> <20250211160825.254167-4-d.kral@proxmox.com> Content-Language: en-US From: Fiona Ebner <f.ebner@proxmox.com> In-Reply-To: <20250211160825.254167-4-d.kral@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.045 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [storage.pm, status.pm] Subject: Re: [pve-devel] [PATCH pve-storage v2 3/5] tree-wide: make use of content type assertion helper X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com> 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@proxmox.com> With the previous patch fixed: Reviewed-by: Fiona Ebner <f.ebner@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" _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel