On 1/17/19 5:10 PM, Christian Ebner wrote: > No, I had no special reason to put it there, I was just looking for the > seemingly easiest place to clean up the directory. > > I will implement it for the Dir plugin if this is more suitable!
pve-storage, and its plugin system, is here to abstract the specifics and detailed semantics away, as much as possible and as much it makes sense. So, if a specific storage type (in this case directory based storage) needs some special handling it should be handled by the respective plugin itself, so all plugin users can profit from this and less tight-coupling is produced. This is not always possible, there are cases where the thing one wants to do is inherently coupled and splitting it apart may make things even more complicated or add much overhead, but that is not, or at least, should not be often the case, just to get you some rationale why this seemed a bit off to me. > > Thx >> On January 17, 2019 at 4:56 PM Thomas Lamprecht <[email protected]> >> wrote: >> >> >> On 1/17/19 12:13 PM, Christian Ebner wrote: >>> Fix #1941 >> >> It's a bit of an untold convention in PVE to put above in the beginning of >> the commit messages subject line, makes it easier for some of us to copy the >> line for the package changelog. >> >>> >>> Removes the empty vmid subdirectories when a VM gets destroyed and all the >>> contained image files are deleted. >> >> Hmm, any reason that you do not do this in pve-storage? >> Without thinking to much it seems to me that this is not the job of >> qemu-server, >> it shouldn't care about such details, it only wants to free an image? >> Or did you have specific reasons to put it here? >> >> Else, maybe you could integrate something like this in the Dir plugin, which >> in this case means the base Plugin class DirPlug bases on, as it inherits the >> free_image method. >> >>> >>> Signed-off-by: Christian Ebner <[email protected]> >>> --- >>> PVE/QemuServer.pm | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm >>> index 1ccdccf..da4be85 100644 >>> --- a/PVE/QemuServer.pm >>> +++ b/PVE/QemuServer.pm >>> @@ -2502,6 +2502,8 @@ sub destroy_vm { >>> return if drive_is_cdrom($drive, 1); >>> >>> my $volid = $drive->{file}; >>> + my $storeid = PVE::Storage::parse_volume_id($volid); >>> + my $storetype = $storecfg->{ids}->{$storeid}->{type}; >>> >>> return if !$volid || $volid =~ m|^/|; >>> >>> @@ -2513,6 +2515,12 @@ sub destroy_vm { >>> }; >>> warn "Could not remove disk '$volid', check manually: $@" if $@; >>> >>> + # cleanup empty directroies on dir based storage >>> + if ($storetype eq 'dir') { >>> + my $dir = dirname($path); >>> + rmdir($dir); >>> + } >>> + >>> }); >>> >>> if ($keep_empty_config) { >>> >> _______________________________________________ pve-devel mailing list [email protected] https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
