[pve-devel] [PATCH #1941 storage] Plugin: Remove empty directories on VM destroy

[Thomas Lamprecht]

On 1/23/19 5:39 PM, Christian Ebner wrote:
> Remove directories if they are empty after destroying a VM.
> 

works OK, some comment/nit inline.

> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
>  PVE/Storage/Plugin.pm | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index e0c2a4e..00131df 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -5,6 +5,7 @@ use warnings;
>  
>  use File::chdir;
>  use File::Path;
> +use File::Basename;
>  
>  use PVE::Tools qw(run_command);
>  use PVE::JSONSchema qw(get_standard_option);
> @@ -689,6 +690,10 @@ sub free_image {
>  
>  	unlink($path) || die "unlink '$path' failed - $!\n";
>      }
> +
> +    # Clean up empty directories

comments should, in general, not describe (only) what happens but rather why.
The what should be deductible from reading the code, so maybe something like

# try to cleanup directory to not clutter storage with empty $vmid dirs if
# all images from a guest got deleted

> +    my $dir = File::Basename::dirname($path);

Just FYI: as File::Basename exports all four of it's functions[0], and you do not
explicitly deny import you could drop the "File::Basename::" here, we use it
that way most of the other times, AFAICT. If you'd write the use statement like:

use File::Basename qw();

you would prevent the default export and you'd need to actually use the module
prefix as you did here.

[0]: editor /usr/lib/x86_64-linux-gnu/perl-base/File/Basename.pm

> +    rmdir($dir);
>      
>      return undef;
>  }
>