[PATCH storage] fix #6969: plugin: keep content directory when freeing its last file

[Michal Fox <me@dualfroz.com>]

After removing a volume, free_image tries to remove the parent
directory, so that no empty per-guest directories are left behind once
all images of a guest are gone. This was done unconditionally, so
deleting the last ISO, container template, snippet or import file also
removed the shared content directory like 'snippets' or 'template/iso'.

The directory gets re-created shortly after by pvestatd, but as
root:root with default permissions, losing any ownership or mode that
the admin set up, for example to allow a non-root user to upload
snippets.

Only clean up the parent directory for guest images, which are the only
volumes stored in per-guest directories.

Signed-off-by: Michal Fox <me@dualfroz.com>
---
 src/PVE/Storage/Plugin.pm    |  6 ++---
 src/test/free_image_test.pm  | 51 ++++++++++++++++++++++++++++++++++++
 src/test/run_plugin_tests.pl |  1 +
 3 files changed, 55 insertions(+), 3 deletions(-)
 create mode 100644 src/test/free_image_test.pm

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index a9e1751..d499fd9 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1166,9 +1166,9 @@ sub free_image {
     }
 
     # try to cleanup directory to not clutter storage with empty $vmid dirs if
-    # all images from a guest got deleted
-    my $dir = dirname($path);
-    rmdir($dir);
+    # all images from a guest got deleted, but keep the shared content type dirs
+    my ($vtype) = $class->parse_volname($volname);
+    rmdir(dirname($path)) if $vtype eq 'images';
 
     return undef;
 }
diff --git a/src/test/free_image_test.pm b/src/test/free_image_test.pm
new file mode 100644
index 0000000..6161866
--- /dev/null
+++ b/src/test/free_image_test.pm
@@ -0,0 +1,51 @@
+package PVE::Storage::TestFreeImage;
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use File::Path qw(make_path);
+use File::Temp;
+use PVE::Storage;
+use PVE::Tools qw(file_set_contents);
+use Test::More;
+
+my $storage_dir = File::Temp->newdir();
+my $scfg = { type => 'dir', path => "$storage_dir" };
+
+# each test is comprised of the following array keys:
+# [0] => volname of the only volume in its directory, freed by the test
+# [1] => parent directory of the volume, relative to the storage path
+# [2] => whether the now empty parent directory is expected to be kept
+my $tests = [
+    ['100/vm-100-disk-0.raw', 'images/100', 0],
+    ['iso/some.iso', 'template/iso', 1],
+    ['vztmpl/some.tar.zst', 'template/cache', 1],
+    ['snippets/hook.pl', 'snippets', 1],
+    ['import/some.ova', 'import', 1],
+];
+
+plan tests => 2 * scalar(@$tests);
+
+for my $tt (@$tests) {
+    my ($volname, $subdir, $keep_dir) = @$tt;
+
+    my $path = PVE::Storage::DirPlugin->filesystem_path($scfg, $volname);
+    make_path("$storage_dir/$subdir");
+    file_set_contents($path, '');
+
+    my $format = (PVE::Storage::DirPlugin->parse_volname($volname))[6];
+    PVE::Storage::DirPlugin->free_image('local', $scfg, $volname, 0, $format);
+
+    ok(!-e $path, "$volname - volume removed");
+    is(
+        !!-d "$storage_dir/$subdir",
+        !!$keep_dir,
+        "$volname - empty directory '$subdir' " . ($keep_dir ? 'kept' : 'removed'),
+    );
+}
+
+done_testing();
+
+1;
diff --git a/src/test/run_plugin_tests.pl b/src/test/run_plugin_tests.pl
index 8bce9d3..2e1c64f 100755
--- a/src/test/run_plugin_tests.pl
+++ b/src/test/run_plugin_tests.pl
@@ -17,6 +17,7 @@ my $res = $harness->runtests(
     "get_subdir_test.pm",
     "filesystem_path_test.pm",
     "prune_backups_test.pm",
+    "free_image_test.pm",
 );
 
 exit -1 if !$res || $res->{failed} || $res->{parse_errors};
-- 
2.43.0