[PATCH pve-storage v2 0/3] Improve mkpath Error Message in activate_storage

[PATCH pve-storage v2 0/3] Improve mkpath Error Message in activate_storage

[Max R. Carrara]

See the cover letter of v1 (linked below) for a description of this
series.

Changes since v1:
- Rebased on master (nothing changed)
- Clarified message of patch #2, as pointed out by Fiona [1] (thanks!)
- Improved the error matching done in patch #3 as suggested by Fabian [2]
  (thanks!) and updated the commit message accordingly

Older Versions
==============

v1: https://lore.proxmox.com/pve-devel/20251205154404.446338-1-m.carrara@proxmox.com/

References
==========

[1]: https://lore.proxmox.com/pve-devel/60139202-64a0-4111-9773-a571ffde0389@proxmox.com/
[2]: https://lore.proxmox.com/pve-devel/1768487461.gbidzul2yw.astroid@yuna.none/

Summary of Changes
==================

Max R. Carrara (3):
  plugin: use guard clause to reduce nesting
  plugin: remove needless inequality check when creating content subdirs
  plugin: improve error handling when creating a content subdir fails

 src/PVE/Storage/Plugin.pm | 80 ++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 26 deletions(-)

--
2.47.3

[PATCH pve-storage v2 1/3] plugin: use guard clause to reduce nesting

[Max R. Carrara]

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 54 ++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index afd31414..a7c5d083 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1899,40 +1899,42 @@ sub activate_storage {
             . "directory '$path' does not exist or is unreachable\n";
     }
 
+    if (!defined($scfg->{content})) {
+        return;
+    }
+
     # TODO: mkdir is basically deprecated since 8.0, but we don't warn here until 8.4 or 9.0, as we
     # only got the replacement in 8.0, so no real replacement window, and its really noisy.
 
-    if (defined($scfg->{content})) {
-        # (opt-out) create content dirs and check validity
-        if (
-            (!defined($scfg->{'create-subdirs'}) || $scfg->{'create-subdirs'})
-            # FIXME The mkdir option is deprecated. Remove with PVE 9?
-            && (!defined($scfg->{mkdir}) || $scfg->{mkdir})
-        ) {
-            for my $vtype (sort keys %$vtype_subdirs) {
-                # OpenVZMigrate uses backup (dump) dir
-                if (
-                    defined($scfg->{content}->{$vtype})
-                    || ($vtype eq 'backup' && defined($scfg->{content}->{'rootdir'}))
-                ) {
-                    my $subdir = $class->get_subdir($scfg, $vtype);
-                    mkpath $subdir if $subdir ne $path;
-                }
+    # (opt-out) create content dirs and check validity
+    if (
+        (!defined($scfg->{'create-subdirs'}) || $scfg->{'create-subdirs'})
+        # FIXME The mkdir option is deprecated. Remove with PVE 9?
+        && (!defined($scfg->{mkdir}) || $scfg->{mkdir})
+    ) {
+        for my $vtype (sort keys %$vtype_subdirs) {
+            # OpenVZMigrate uses backup (dump) dir
+            if (
+                defined($scfg->{content}->{$vtype})
+                || ($vtype eq 'backup' && defined($scfg->{content}->{'rootdir'}))
+            ) {
+                my $subdir = $class->get_subdir($scfg, $vtype);
+                mkpath $subdir if $subdir ne $path;
             }
         }
+    }
 
-        # check that content dirs are pairwise inequal
-        my $resolved_subdirs = {};
-        for my $vtype (sort keys $scfg->{content}->%*) {
-            my $subdir = $class->get_subdir($scfg, $vtype);
-            my $abs_subdir = abs_path($subdir);
-            next if !defined($abs_subdir);
+    # check that content dirs are pairwise inequal
+    my $resolved_subdirs = {};
+    for my $vtype (sort keys $scfg->{content}->%*) {
+        my $subdir = $class->get_subdir($scfg, $vtype);
+        my $abs_subdir = abs_path($subdir);
+        next if !defined($abs_subdir);
 
-            die "storage '$storeid' uses directory $abs_subdir for multiple content types\n"
-                if defined($abs_subdir) && defined($resolved_subdirs->{$abs_subdir});
+        die "storage '$storeid' uses directory $abs_subdir for multiple content types\n"
+            if defined($abs_subdir) && defined($resolved_subdirs->{$abs_subdir});
 
-            $resolved_subdirs->{$abs_subdir} = 1;
-        }
+        $resolved_subdirs->{$abs_subdir} = 1;
     }
 }
 
-- 
2.47.3

[PATCH pve-storage v2 2/3] plugin: remove needless inequality check when creating content subdirs

[Max R. Carrara]

`$subdir` is a subdirectory of `$path`, so the variables are never
equal. Even if they were equal for some reason, calling `mkpath()`
here wouldn't fail or be otherwise dangerous.

To elaborate, note that even if the user happened to configure an
empty path for a given content subdirectory, `get_subdir()` would
still return "$path/", which is obviously not equal to `$path`.
However, third-party plugins which have overridden `get_subdir()`
might actually return "$path", in which case the inequality check
indeed passes. Nevertheless, calling `mkpath()` remains harmless,
since it will at most create the `$path` dir for the given storage.

Therefore, remove this check.

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index a7c5d083..04d87842 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1919,7 +1919,7 @@ sub activate_storage {
                 || ($vtype eq 'backup' && defined($scfg->{content}->{'rootdir'}))
             ) {
                 my $subdir = $class->get_subdir($scfg, $vtype);
-                mkpath $subdir if $subdir ne $path;
+                mkpath $subdir;
             }
         }
     }
-- 
2.47.3

[PATCH pve-storage v2 3/3] plugin: improve error handling when creating a content subdir fails

[Max R. Carrara]

Replace the call to `mkpath()` with a helper inline subroutine that
uses `make_path()` instead. Follow the docs of `File::Path` [1] in
order to make our error handling here more finely-grained.

Note that the helper sub is mainly there to make the body of the loop
where `mkpath()` was originally called less convoluted / complex.

Add some additional context to the error message, namely that creating
the content directory for a given vtype failed. Also extract and
attach `make_path()`'s error message.

Handle "Permission denied" and "Read-only file system" errors
separately and, in addition to the context, tell the user to ensure
that the storage has read and write access when such errors occur.

Initially spotted this in the community forum [2].

[1]: https://perldoc.perl.org/File::Path#ERROR-HANDLING
[2]: https://forum.proxmox.com/threads/bug-creating-nfs-volume-storage.177354/

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---

Notes:
    NOTE: I ran into the second branch (the one for "Read-only file
    system") when testing things using my local NFS share—I figured I
    might as well handle that one too, since it's similar in nature.

 src/PVE/Storage/Plugin.pm | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 04d87842..294a59e8 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1903,6 +1903,33 @@ sub activate_storage {
         return;
     }

+    my $try_create_subdir = sub {
+        my ($vtype) = @_;
+
+        my $subdir = $class->get_subdir($scfg, $vtype);
+
+        File::Path::make_path($subdir, { error => \my $errors });
+        if (defined($errors) && scalar($errors->@*)) {
+            my $msg = "failed to create content directory '$subdir' for content '$vtype'";
+
+            for my $err_hash ($errors->@*) {
+                my ($file, $err_msg) = $err_hash->%*;
+
+                if (
+                    $err_msg =~ m/permission \s denied/ix
+                    || $err_msg =~ m/read -? only \s file \s? system/ix
+                ) {
+                    my $msg_prompt =
+                        "ensure that you have read and write access to your storage!";
+
+                    die "$msg - $err_msg - $msg_prompt\n";
+                }
+
+                die "$msg - $err_msg\n";
+            }
+        }
+    };
+
     # TODO: mkdir is basically deprecated since 8.0, but we don't warn here until 8.4 or 9.0, as we
     # only got the replacement in 8.0, so no real replacement window, and its really noisy.

@@ -1918,8 +1945,7 @@ sub activate_storage {
                 defined($scfg->{content}->{$vtype})
                 || ($vtype eq 'backup' && defined($scfg->{content}->{'rootdir'}))
             ) {
-                my $subdir = $class->get_subdir($scfg, $vtype);
-                mkpath $subdir;
+                $try_create_subdir->($vtype);
             }
         }
     }
--
2.47.3