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