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.eb...@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;
> }
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
