On 2/19/25 16:16, Fiona Ebner wrote:
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 ;)?
That was an oversight, but as mentioned in the previous response, I'd
hope to be able to make the `storage_config` to a
`storage_check_enabled` in a v3 if there's nothing breaking about this
when replacing the existing checks :).
Signed-off-by: Daniel Kral <d.k...@proxmox.com>
With the previous patch fixed:
Reviewed-by: Fiona Ebner <f.eb...@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!
Agreed, a better name would be good here, so it doesn't add confusion! I
think I'd go for the first suggestion in a v3, but I'll think about
it... The second suggestion is great, but I'd like to keep most helpers
in one line if possible - but this shouldn't be more important than
clarity of course.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
