This is going to need multiple sets of eyes as I don't trust that we
don't have some rogue direct `$scfg->{path}` usage ruining this for some
case ;-)
As for the code, see inline comments:
On Thu, Dec 15, 2022 at 12:21:46PM +0100, Leo Nunner wrote:
> Allowing overrides for the default directory locations seems to
> integrate rather well into the existing system. Custom locations
> are specified using the "dirs" parameter as a comma-separated list
> of "vtype:/location" values.
>
> For now, the option has been enabled for the Directory, CIFS and NFS
> backends.
>
> Signed-off-by: Leo Nunner <l.nun...@proxmox.com>
> ---
> PVE/Storage/CIFSPlugin.pm | 1 +
> PVE/Storage/DirPlugin.pm | 1 +
> PVE/Storage/NFSPlugin.pm | 1 +
> PVE/Storage/Plugin.pm | 43 +++++++++++++++++++++++++++++++++++----
> test/get_subdir_test.pm | 7 +++++++
> 5 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
> index 982040a..4284c35 100644
> --- a/PVE/Storage/CIFSPlugin.pm
> +++ b/PVE/Storage/CIFSPlugin.pm
> @@ -128,6 +128,7 @@ sub properties {
> sub options {
> return {
> path => { fixed => 1 },
> + dirs => { optional => 1 },
> server => { fixed => 1 },
> share => { fixed => 1 },
> nodes => { optional => 1 },
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index 8715a9d..3c907ca 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -54,6 +54,7 @@ sub properties {
> sub options {
> return {
> path => { fixed => 1 },
> + dirs => { optional => 1 },
> nodes => { optional => 1 },
> shared => { optional => 1 },
> disable => { optional => 1 },
> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
> index 5bd7313..b7e8318 100644
> --- a/PVE/Storage/NFSPlugin.pm
> +++ b/PVE/Storage/NFSPlugin.pm
> @@ -79,6 +79,7 @@ sub properties {
> sub options {
> return {
> path => { fixed => 1 },
> + dirs => { optional => 1 },
> server => { fixed => 1 },
> export => { fixed => 1 },
> nodes => { optional => 1 },
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 8a41df1..de9227b 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -181,6 +181,11 @@ my $defaultData = {
> default => 'metadata',
> optional => 1,
> },
> + dirs => {
> + description => "Overrides for default directories",
> + type => "string", format => "pve-volume-id-list",
> + optional => 1,
> + },
> },
> };
>
> @@ -205,6 +210,17 @@ sub valid_content_types {
> return $def->{content}->[0];
> }
>
> +sub dirs_hash_to_string {
> + my $hash = shift;
> +
> + my @cta;
Better just build the string right away and skip the cryptically named
array ;-)
(can do a `$str .= ',' if length($str);` line for the comma part)
Alternatively you can use a more functional style with `map {}`, eg.:
join(',', map { "$_:$hash->{$_}" } sort keys %$hash);
Finally: please `sort` the keys to avoid adding yet another case of
reordering happening on every single storage.cfg modification as perl
hashes are randomized ;-)
> + foreach my $dir (keys %$hash) {
> + push @cta, "$dir:$hash->{$dir}" if $hash->{$dir};
> + }
> +
> + return join(',', @cta);
> +}
> +
> sub default_format {
> my ($scfg) = @_;
>
> @@ -405,6 +421,23 @@ sub decode_value {
> # die "storage '$storeid' does not allow node restrictions\n";
> #}
>
> + return $res;
> + } elsif ($key eq 'dirs') {
> + my $valid_content = $def->{content}->[0];
> + my $res = {};
> +
> + foreach my $dir (PVE::Tools::split_list($value)) {
> + my ($c, $path) = parse_volume_id($dir);
Using `parse_volume_id` here can lead to confusing error messages.
Plus you verify each part below anyway, so IMO you can just do
split(/:/, $dir, 2)
> + if (!$valid_content->{$c}) {
> + warn "storage does not support content type '$c'\n";
> + next;
> + } elsif (!verify_path($path, 1)) {
> + warn "not a valid path: $path";
> + next;
> + }
> + $res->{$c} = $path;
> + }
> +
> return $res;
> }
>
> @@ -419,6 +452,9 @@ sub encode_value {
> } elsif ($key eq 'content') {
> my $res = content_hash_to_string($value) || 'none';
> return $res;
> + } elsif ($key eq 'dirs') {
> + my $res = dirs_hash_to_string($value);
> + return $res;
> }
>
> return $value;
> @@ -610,12 +646,11 @@ sub get_subdir {
> my $path = $scfg->{path};
>
> die "storage definition has no path\n" if !$path;
> + die "unknown vtype '$vtype'\n" if !exists($vtype_subdirs->{$vtype});
>
> - my $subdir = $vtype_subdirs->{$vtype};
> -
> - die "unknown vtype '$vtype'\n" if !defined($subdir);
> + my $subdir = $scfg->{dirs}->{$vtype} // "/".$vtype_subdirs->{$vtype};
>
> - return "$path/$subdir";
> + return $path.$subdir;
> }
>
> sub filesystem_path {
> diff --git a/test/get_subdir_test.pm b/test/get_subdir_test.pm
> index 1e58350..26c08d5 100644
> --- a/test/get_subdir_test.pm
> +++ b/test/get_subdir_test.pm
> @@ -27,6 +27,13 @@ foreach my $type (keys %$vtype_subdirs) {
> push @$tests, [ $scfg_with_path, $type, $path ];
> }
>
> +# creates additional tests for overrides
> +foreach my $type (keys %$vtype_subdirs) {
> + my $override = "/${type}_override";
> + my $scfg_with_override = { path => '/some/path', dirs => { $type =>
> $override } };
> + push @$tests, [ $scfg_with_override, $type,
> "$scfg_with_override->{path}$scfg_with_override->{dirs}->{$type}" ];
> +}
> +
> plan tests => scalar @$tests;
>
> foreach my $tt (@$tests) {
> --
> 2.30.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
