[pve-devel] [RFC storage] content-dirs: check that all content dirs are pairwise inequal

[pve-devel] [RFC storage] content-dirs: check that all content dirs are pairwise inequal

[Friedrich Weber]

This prevents strange interactions in case the same content directory
is used for multiple content types.

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---
 I guess technically this is a breaking change, as users may have an
 iso+vztmpl storage that symlinks 'templates/iso' to 'templates/cache',
 which would now throw an error.

 PVE/Storage/Plugin.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index c323085..3c9a179 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -3,6 +3,7 @@ package PVE::Storage::Plugin;
 use strict;
 use warnings;
 
+use Cwd qw(abs_path);
 use Encode qw(decode);
 use Fcntl ':mode';
 use File::chdir;
@@ -1362,6 +1363,16 @@ sub activate_storage {
 	"directory '$path' does not exist or is unreachable\n";
     }
 
+    # 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;
+	}
+    }
 
     return if defined($scfg->{mkdir}) && !$scfg->{mkdir};
 
-- 
2.30.2

[pve-devel] applied: [RFC storage] content-dirs: check that all content dirs are pairwise inequal

[Thomas Lamprecht]

Am 21/03/2023 um 18:03 schrieb Friedrich Weber:
> This prevents strange interactions in case the same content directory
> is used for multiple content types.
> 
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
>  I guess technically this is a breaking change, as users may have an
>  iso+vztmpl storage that symlinks 'templates/iso' to 'templates/cache',
>  which would now throw an error.

Well, then lets apply this for upcoming 8, maybe you can add a note to
our breaking changes list so that users are aware.

Also, checking this upfront and erroring in pve7to8 checker script should
not be that hard and def. help some to avoid problems during/after the
upgrade.

> 
>  PVE/Storage/Plugin.pm | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
>

applied, thanks!

Re: [pve-devel] applied: [RFC storage] content-dirs: check that all content dirs are pairwise inequal

[Friedrich Weber]

On 06/06/2023 17:28, Thomas Lamprecht wrote:
> Well, then lets apply this for upcoming 8, maybe you can add a note to
> our breaking changes list so that users are aware.

Done!

> Also, checking this upfront and erroring in pve7to8 checker script should
> not be that hard and def. help some to avoid problems during/after the
> upgrade.

Yes, sounds sensible. I'll prepare a patch.

Re: [pve-devel] [RFC storage] content-dirs: check that all content dirs are pairwise inequal

[Fiona Ebner]

Am 21.03.23 um 18:03 schrieb Friedrich Weber:
> This prevents strange interactions in case the same content directory
> is used for multiple content types.
> 

Should we also check in the create/update API calls for syntactic
duplicates and fail the call? E.g. I can successfully issue:
pvesh set /storage/foo --content-dirs backup=bar,iso=bar
and only get the error later during activation.

Re: [pve-devel] [RFC storage] content-dirs: check that all content dirs are pairwise inequal

[Friedrich Weber]

On 07/06/2023 12:01, Fiona Ebner wrote:
> Should we also check in the create/update API calls for syntactic
> duplicates and fail the call? E.g. I can successfully issue:
> pvesh set /storage/foo --content-dirs backup=bar,iso=bar
> and only get the error later during activation.

Not allowing users to configure `content-dirs` with syntactic duplicates
sounds good to me (it wouldn't catch situations involving symlinks, but
those require quite some manual hacking from the user's side anyway).

As this would be mostly a convenience feature, I'd send a patch after
next week if that's okay.

Re: [pve-devel] [RFC storage] content-dirs: check that all content dirs are pairwise inequal

[Thomas Lamprecht]

Am 07/06/2023 um 12:18 schrieb Friedrich Weber:
> On 07/06/2023 12:01, Fiona Ebner wrote:
>> Should we also check in the create/update API calls for syntactic
>> duplicates and fail the call? E.g. I can successfully issue:
>> pvesh set /storage/foo --content-dirs backup=bar,iso=bar
>> and only get the error later during activation.

Definitively a good idea (tbh. I thought it acted already that way there too) 

> Not allowing users to configure `content-dirs` with syntactic duplicates
> sounds good to me (it wouldn't catch situations involving symlinks, but
> those require quite some manual hacking from the user's side anyway).
> 
> As this would be mostly a convenience feature, I'd send a patch after
> next week if that's okay.
> 

Yeah that's fine.