Thanks for the feedback! I totally agree with your reasoning, I was not yet aware of the plugin system. Will implement it in the pve-storage Dir plugin.
> On January 18, 2019 at 9:14 AM Thomas Lamprecht <[email protected]> > wrote: > > > 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
