From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id C56591FF09B for ; Mon, 28 Sep 2026 09:17:53 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D1D5E21A2C; Mon, 28 Sep 2026 09:15:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dualfroz.com; s=dkim; t=1790415178; h=from:subject:date:message-id:to:mime-version: content-transfer-encoding; bh=B4j27igKo8S4gJfAQXqP+EV+h6yXjnPKUTG+laxFq7A=; b=XCM1lxolq7vMnAx4N+oiKeqz6bUJqdvhKjalP4PGM2PXH4coqXCk/RuL0HC06mIvUmn7Df ver3Nck1tkPikfPjxrpfxKjgNSxke+lyTz6z0Lq9W8FVbRxQiSzmyDwGzS8+ykCsX+5E9a P5f3nzcD3CT+ZXpPLXeAx20q6bS81Nv8HsvWLpSoGNynz1yTG0rFdBFIfiDNC0sW/2Nh0p q3ACerbcPgH7bKW6QVYh6+8tTRWcsq31l3kgzQErRUk5d4QN7aJJ1F/psKrsEOMoTX1BT5 NnBQAHdM1wWYp12F+ze0i2DeZiPyo3zUKjQnEKkwrqkoz7aALpBXcNcqZ525Ew== From: Michal Fox To: pve-devel@lists.proxmox.com Subject: [PATCH storage] fix #6969: plugin: keep content directory when freeing its last file Date: Sat, 26 Sep 2026 09:32:57 +0000 Message-ID: <20260926093257.7-1-me@dualfroz.com> X-Mailer: git-send-email 2.47.3 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Last-TLS-Session-Version: TLSv1.3 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.269 Adjusted score from AWL reputation of From: address DKIM_SIGNED 0.1 Message has a DKIM or DK signature, not necessarily valid DKIM_VALID -0.1 Message has at least one valid DKIM or DK signature DKIM_VALID_AU -0.1 Message has a valid DKIM or DK signature from author's domain DKIM_VALID_EF -0.1 Message has a valid DKIM or DK signature from envelope-from domain DMARC_PASS -0.1 DMARC pass policy SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record X-MailFrom: me@dualfroz.com X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation Message-ID-Hash: BBXL24YKV2OI2SG6JWZZ55WLE43LQ5H7 X-Message-ID-Hash: BBXL24YKV2OI2SG6JWZZ55WLE43LQ5H7 X-Mailman-Approved-At: Mon, 28 Sep 2026 09:14:43 +0200 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- 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