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

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

[Michal Fox]

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

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

[Max R. Carrara]

On Sat Sep 26, 2026 at 11:32 AM CEST, Michal Fox wrote:
> 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>
> ---

Tested this on my workstation by creating an empty file in
`/var/lib/vz/snippets/` and changing the directory's permissions to see
whether it was re-created at a glance.

With the patch, the content dir is indeed left as-is. So, works as
advertised.

LGTM! Thanks for your contribution, it's very appreciated!

Consider:

Reviewed-by: Max R. Carrara <m.carrara@proxmox.com>
Tested-by: Max R. Carrara <m.carrara@proxmox.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};