[pve-devel] [PATCH v2 qemu-server 3/3] map vmid correctly for cloudinit on restore

[Thomas Lamprecht]

On 3/29/19 4:32 PM, Mira Limbeck wrote:
> on restore the cloudinit disk is now changed to the correct vmid. in
> addition targetstorage is taken into account as well.

not yet quite sure with this one...

> 
> Signed-off-by: Mira Limbeck <m.limbeck at proxmox.com>
> Tested-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> v2:
>  - accept sata and scsi as well on restore (regex match)
> 
>  PVE/QemuServer.pm | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index e60aa28..8e700f5 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6265,6 +6265,14 @@ sub restore_vma_archive {
>  		$storage_limits{$storeid} = $bwlimit;
>  
>  		$virtdev_hash->{$virtdev} = $devinfo->{$devname};
> +	    } elsif ($line =~ m/^((?:ide|sata|scsi)\d):\s*(\S*\/vm-\d+-cloudinit(.(qcow2|raw))?)\S+\s*$/) {

regex is a bit strange..
first you need to escape the dot before the (qcow2|raw) format,
else you can place anything there.

Further, the trailing  \S+\s* is bit strange, IMO does it normally only captures the
',media=cdrom' string? if so I'd write that out, as currently the capturing is a bit
strange if one tests it out.

why the parenthesis around the (.(qcow2|raw)) ? to sync the matching group number
of the $format with  the other if branch? If so it does not really help, as it's
clearly another regex and one need to look anyway, extra unused parenthesis may just
add confusion.

> +		my ($virtdev, $volid, $format) = ($1, $2, $4);
> +		my $d = {};
> +		$d->{volid} = $volid;
> +		$d->{format} = $format;

can be undefined, which will get you then a "undef in string comparison" warning from
perl when iterating over the %$virtdev_hash below (you actually have it in context so
see below)

> +		$d->{size} = 4 * 1024 * 1024;
> +		$d->{storeid} = $opts->{storage} // PVE::Storage::parse_volume_id($volid);
> +		$virtdev_hash->{$virtdev} = $d;
>  	    }
>  	}
>  
> @@ -6321,6 +6329,7 @@ sub restore_vma_archive {
>  	my $map = {};
>  	foreach my $virtdev (sort keys %$virtdev_hash) {
>  	    my $d = $virtdev_hash->{$virtdev};
> +	    my $is_cloudinit = $d->{volid} ? $d->{volid} =~ m/vm-\d+-cloudinit/ : 0;
>  	    my $alloc_size = int(($d->{size} + 1024 - 1)/1024);
>  	    my $storeid = $d->{storeid};
>  	    my $scfg = PVE::Storage::storage_config($cfg, $storeid);
> @@ -6335,8 +6344,13 @@ sub restore_vma_archive {
>  	    my $supported = grep { $_ eq $d->{format} } @$validFormats;

...                                  ^^^^^^^^^^^^^^^^

expects to be defined, the other code path also uses "raw" as fallback, do we want this
here too? please re-check this one, thanks!

>  	    $d->{format} = $defFormat if !$supported;
>  
> +	    my $name = undef;
> +	    if ($is_cloudinit) {
> +		$name = "vm-$vmid-cloudinit";
> +		$name .= ".qcow2" if $scfg->{path};
> +	    }
>  	    my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid,
> -						  $d->{format}, undef, $alloc_size);
> +						  $d->{format}, $name, $alloc_size);
>  	    print STDERR "new volume ID is '$volid'\n";
>  	    $d->{volid} = $volid;
>  	    my $path = PVE::Storage::path($cfg, $volid);
> @@ -6348,9 +6362,12 @@ sub restore_vma_archive {
>  		$write_zeros = 0;
>  	    }
>  
> -	    print $fifofh "${map_opts}format=$d->{format}:${write_zeros}:$d->{devname}=$path\n";
> +	    if (!$is_cloudinit) { # $d->{devname} undefined if it is cloudinit
> +		print $fifofh "${map_opts}format=$d->{format}:${write_zeros}:$d->{devname}=$path\n";
> +
> +		print "map '$d->{devname}' to '$path' (write zeros = ${write_zeros})\n";
> +	    }
>  
> -	    print "map '$d->{devname}' to '$path' (write zeros = ${write_zeros})\n";
>  	    $map->{$virtdev} = $volid;
>  	}
>  
>