[pve-devel] applied: [PATCH storage v2 1/3] add vm_diskname helpers (get_next, is_valid)

[Thomas Lamprecht]

applied, with a followup - see comments inline

On 9/7/18 3:08 PM, Stoiko Ivanov wrote:
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>  PVE/Storage/Plugin.pm | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 7db3a95..32acf69 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -523,6 +523,49 @@ sub create_base {
>      return $newvolname;
>  }
>  
> +sub is_valid_vm_diskname {
> +    my ($disk_name, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
> +
> +    $vmid = qr/\d+/ if !defined($vmid);

made sub a "private" helper (i.e., coderef in module variable), and
replace the $fmt, $add_fmt_suffix helper with a $suffix one.

@Dominik, rather than patch-gluing this method into rbd_ls I'd like a
per-plugin method which parses the diskname, each plugin knows what it
can expect, and most can just inherit the one from base

something like:

sub parse_vm_disk {
    my ($disk) = @_;

    my $disk_re = qr/(vm|base)-([1-9]{2}\d+)-disk-(\d+)(\.\S+)/;

    my ($type, $vmid, $diskid, $suffix) = $disk =~ $disk_re;

    return ($vmid, $diskid, $type, $suffix);
}

optionally, there really could be a $vmid (to differ between a fixed and
arbitrary ones) or also a $suffix and there like in the signature, maybe
you could takje a look what seems to make sense...

> +
> +    my $suffix = (defined($fmt) && $add_fmt_suffix) ? ".$fmt" : '';
> +
> +    my $type = $scfg->{type};
> +    my $def = $defaultData->{plugindata}->{$type};
> +    my $valid_formats = $def->{format}[0];
> +
> +    my $disk_regex = qr/(vm|base)-$vmid-disk-(\d+)$suffix/;
> +    $disk_regex = qr/(vm|base|subvol|basevol)-$vmid-disk-(\d+)/
> +	if $valid_formats->{subvol};
> +
> +    if($disk_name =~ m/$disk_regex/){
> +	return wantarray ? (1, $2) : 1;

removed wantarray mechanism, nowhere used in your series, no mention for
what it should be used and just returning the disk number in a sub called
is_valid_vm_diskname was a bit weird, IMO.

> +    }

added return undef; as else the last expression gets returned by perl,
may be OK now, but can be surprising if someone changes this..

> +}
> +
> +sub get_next_vm_diskname {
> +    my ($disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix) = @_;
> +
> +    my $disk_ids = {};
> +    my ($match, $disknum);
> +    foreach my $disk (@$disk_list) {
> +	($match, $disknum) = is_valid_vm_diskname($disk,  $scfg, $vmid, $fmt, $add_fmt_suffix);
> +	$disk_ids->{$disknum} = 1 if $match;
> +    }
> +
> +    $fmt //= '';
> +    my $prefix = ($fmt eq 'subvol') ? 'subvol' : 'vm';
> +    my $suffix = $add_fmt_suffix ? ".$fmt" : '';
> +
> +    for (my $i = 1; $i < 100; $i++) {
> +	if (!$disk_ids->{$i}) {
> +	    return "$prefix-$vmid-disk-$i$suffix";
> +	}
> +    }
> +
> +    die "unable to allocate an image name for VM $vmid in storage '$storeid'\n"
> +}
> +
>  my $find_free_diskname = sub {
>      my ($imgdir, $vmid, $fmt) = @_;
>  
>