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

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

[Max R. Carrara]

Noticed a thread in the forum[forum] today where a user ran into the
following error when configuring a new NFS storage:

"create storage failed: mkdir /path/to/mountpoint/: Permission denied at
/usr/share/perl5/PVE/Storage/Plugin.pm line 1919"

Since it seemed that the user couldn't really interpret the error
correctly, improve that particular part in the code and make the error
handling / messaging a bit nicer for end users there.

Patch 01 and 02 just adapt the surrounding code in preparation; patch 03
contains the improvement as well as additional details.

References
----------

[forum]: https://forum.proxmox.com/threads/bug-creating-nfs-volume-storage.177354/

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 | 75 +++++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 26 deletions(-)

-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-storage v1 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 6f2e434..65f2551 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



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-storage v1 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.

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`.

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 65f2551..617b0f8 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



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Max R. Carrara]

Wrap the call to `mkpath` in an `eval` and add some additional context
to the error message, namely that creating the content directory for a
given vtype failed.

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

Do all this with the help of an anonymous subroutine, because the body
inside the loop would otherwise get a little convoluted / hard to
parse. Also, use the `/x` flag for the regexes to make them a tiny bit
easier to read.

Spotted in the community forum:
https://forum.proxmox.com/threads/bug-creating-nfs-volume-storage.177354/

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

Notes:
    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 | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 617b0f8..975641b 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1903,6 +1903,28 @@ sub activate_storage {
         return;
     }
 
+    my $try_create_subdir = sub {
+        my ($vtype) = @_;
+
+        my $subdir = $class->get_subdir($scfg, $vtype);
+
+        eval { mkpath $subdir; };
+        if ($@) {
+            my $msg = "failed to create content directory '$subdir' for content '$vtype'";
+            my $msg_prompt = "ensure that the storage has read and write access!";
+
+            if ($@ =~ m/permission \s denied/ix) {
+                die "$msg - permission denied - $msg_prompt\n";
+            }
+
+            if ($@ =~ m/read -? only \s file \s? system/ix) {
+                die "$msg - read-only file system - $msg_prompt\n";
+            }
+
+            die "$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 +1940,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



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel