activate storage: ensure content directories are created before checking them

checking the content dirs for clashes via abs_path must be done after
the logic for creating them ran, as abs_path is working on actual
filesystem level, so it will return undf if the directory does not
exist, in which case we then set a hash entry for "undef", and the
next for loop round then resolved again to "undef", resulting in a
false-positive of the check.

Avoid the dangerous "return if" stanzas and reverse them to an actual
if block, which is much safer to adapt. Then move the check for
duplicate content-dir usage after that.

best viewed with white space change ignored: git show -w

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 727b7a6b58f79c5c5fab9d93062858af93e12c82..4ab1282bb7f0449df29c9fc07b88648d6c80a69e 100644 (file)
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1372,30 +1372,39 @@ sub activate_storage {
     warn "${storeid}: 'mkdir' option is deprecated. Use 'create-base-path' or 'create-subdirs' instead.\n"
        if defined($scfg->{mkdir});
 
-    # check that content dirs are pairwise inequal
-    my $resolved_subdirs = {};
     if (defined($scfg->{content})) {
-       foreach my $vtype (keys $scfg->{content}->%*) {
-           my $abs_subdir = abs_path($class->get_subdir($scfg, $vtype));
-           die "storage '$storeid' uses directory $abs_subdir for multiple content types\n"
-               if defined($resolved_subdirs->{$abs_subdir});
-           $resolved_subdirs->{$abs_subdir} = 1;
+       # (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;
+               }
+           }
        }
-    }
 
-    return if defined($scfg->{'create-subdirs'}) && !$scfg->{'create-subdirs'};
+       # 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);
+           if (!defined($abs_subdir)) {
+               warn "could not get absolute path for '$subdir' - $!\n";
+               next;
+           }
 
-    # FIXME The mkdir option is deprecated. Remove with PVE 9?
-    return if defined($scfg->{mkdir}) && !$scfg->{mkdir};
+           die "storage '$storeid' uses directory $abs_subdir for multiple content types\n"
+               if defined($abs_subdir) && defined($resolved_subdirs->{$abs_subdir});
 
-    if (defined($scfg->{content})) {
-       foreach my $vtype (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;
-           }
+           $resolved_subdirs->{$abs_subdir} = 1;
        }
     }
 }