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

["Max R. Carrara" <m.carrara@proxmox.com>]

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};