[pve-devel] applied: [PATCH v2 storage] Use a common interface for find_free_diskname

[Thomas Lamprecht]

On 12/11/19 10:25 AM, Fabian Ebner wrote:
> We can use 'list_images' to get the desired volume IDs in
> 'find_free_diskname' for most plugins. For the two LVM plugins, 'list_images'
> potentially skips untagged volumes, so we keep the custom version. For the
> RBD plugin, 'list_images' is much more costly than the custom version, so we
> keep the custom version.
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
> 
> Changes from v1:
>     * Keep the custom versions in LVMPlugin and RBDPlugin
>     * Do not change the interface for get_next_vm_diskname
> 
> Thanks to Fabian for the suggestions!
> 
>  PVE/Storage/GlusterfsPlugin.pm | 15 ++-------------
>  PVE/Storage/LVMPlugin.pm       | 10 +++++++---
>  PVE/Storage/LvmThinPlugin.pm   |  8 ++------
>  PVE/Storage/Plugin.pm          | 19 ++++++++++---------
>  PVE/Storage/RBDPlugin.pm       | 10 +++++-----
>  PVE/Storage/ZFSPlugin.pm       |  2 +-
>  PVE/Storage/ZFSPoolPlugin.pm   | 16 +++-------------
>  7 files changed, 30 insertions(+), 50 deletions(-)
> 

applied, thanks! Did a small refactoring as follow-up (see below)

> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index d0f1df6..73f80af 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -572,18 +572,19 @@ sub get_next_vm_diskname {
>      die "unable to allocate an image name for VM $vmid in storage '$storeid'\n"
>  }
>  
> -my $find_free_diskname = sub {
> -    my ($imgdir, $vmid, $fmt, $scfg) = @_;
> +sub find_free_diskname {
> +    my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
> +
> +    my $disks = $class->list_images($storeid, $scfg, $vmid);
>  
>      my $disk_list = [];
>  
> -    if (defined(my $dh = IO::Dir->new($imgdir))) {
> -	@$disk_list = $dh->read();
> -	$dh->close();
> +    foreach my $disk (@{$disks}) {
> +	push @{$disk_list}, $disk->{volid};
>      }

for above I followed up with:
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 73f80af..353632c 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -577,11 +577,7 @@ sub find_free_diskname {

     my $disks = $class->list_images($storeid, $scfg, $vmid);

-    my $disk_list = [];
-
-    foreach my $disk (@{$disks}) {
-       push @{$disk_list}, $disk->{volid};
-    }
+    my $disk_list = [ map { $_->{volid} } @$disks ];

     return get_next_vm_diskname($disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix);
 }