[pve-devel] [qemu-server 5/5] PVE/QemuMigrate.pm - use PVE::QemuServer::foreach_volid

[Fabian Grünbichler]

comment inline

On Tue, Jun 13, 2017 at 08:38:58AM +0200, Dietmar Maurer wrote:
> Signed-off-by: Dietmar Maurer <dietmar at proxmox.com>
> ---
>  PVE/QemuMigrate.pm | 42 +++++++++++++++++-------------------------
>  1 file changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 22c0d7d..2527cba 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -269,9 +269,7 @@ sub sync_disks {
>  	}
>  
>  	my $test_volid = sub {
> -	    my ($volid, $is_cdrom, $snapname) = @_;
> -
> -	    return if !$volid;
> +	    my ($volid, $is_cdrom, $snaprefs) = @_;
>  
>  	    if ($volid =~ m|^/|) {
>  		$local_volumes->{$volid} = 'config';
> @@ -281,8 +279,9 @@ sub sync_disks {
>  	    if ($is_cdrom) {
>  		if ($volid eq 'cdrom') {
>  		    my $msg = "can't migrate local cdrom drive";
> -		    $msg .= " (referenced in snapshot '$snapname')"
> -			if defined($snapname);
> +		    if (defined($snaprefs)) {
> +			my $snapnames = join(', ', sort keys %$snaprefs);
> +			$msg .= " (referenced in snapshot - $snapnames)"
>  
>  		    &$log_error("$msg\n");
>  		    return;
> @@ -301,7 +300,7 @@ sub sync_disks {
>  
>  	    $sharedvm = 0;
>  
> -	    $local_volumes->{$volid} = defined($snapname) ? 'snapshot' : 'config';
> +	    $local_volumes->{$volid} = defined($snaprefs) ? 'snapshot' : 'config';
>  
>  	    die "local cdrom image\n" if $is_cdrom;
>  
> @@ -310,7 +309,7 @@ sub sync_disks {
>  	    die "owned by other VM (owner = VM $owner)\n"
>  		if !$owner || ($owner != $self->{vmid});
>  
> -	    if (defined($snapname)) {
> +	    if (defined($snaprefs)) {
>  		# we cannot migrate shapshots on local storage
>  		# exceptions: 'zfspool' or 'qcow2' files (on directory storage)
>  
> @@ -325,26 +324,19 @@ sub sync_disks {
>  		if PVE::Storage::volume_is_base_and_used($self->{storecfg}, $volid);
>  	};
>  
> -	my $test_drive = sub {
> -	    my ($ds, $drive, $snapname) = @_;
> -
> -	    eval {
> -		&$test_volid($drive->{file}, PVE::QemuServer::drive_is_cdrom($drive), $snapname);
> -	    };
> -
> -	    &$log_error($@, $drive->{file}) if $@;
> -	};
> -
> -	foreach my $snapname (keys %{$conf->{snapshots}}) {
> +	PVE::QemuServer::foreach_volid($conf, sub {
> +	    my ($volid, $attr) = @_;
>  	    eval {
> -		&$test_volid($conf->{snapshots}->{$snapname}->{'vmstate'}, 0, undef)
> -		    if defined($conf->{snapshots}->{$snapname}->{'vmstate'});
> +		if ($attr->{referenced_in_config}) {
> +		    $test_volid->($volid, $attr->{cdrom}, undef);
> +		} else {
> +		    $test_volid->($volid, $attr->{cdrom}, $attr->{referenced_in_snapshot});
> +		}

haven't actually tested it, but this looks wrong to me. if a volid is
referenced both in the current config and in a snapshot, we now ONLY do
the config related checks (meaning we don't catch that it might not be
migratable because it is referenced in a snapshot).

the previous behaviour was (IIRC) akin to this:

$test_volid->($volid, $attr->{cdrom}, $attr->{referenced_in_snapshot})
  if $attr->{referenced_in_snapshot};
$test_volid->($volid, $attr->{cdrom}, undef)
  if $attr->{referenced_in_config};

but it is also possible that I overlooked something?

>  	    };
> -	    &$log_error($@, $conf->{snapshots}->{$snapname}->{'vmstate'}) if $@;
> -
> -	    PVE::QemuServer::foreach_drive($conf->{snapshots}->{$snapname}, $test_drive, $snapname);
> -	}
> -	PVE::QemuServer::foreach_drive($conf, $test_drive);
> +	    if (my $err = $@) {
> +		&$log_error($err, $volid);
> +	    }
> +        });
>  
>  	foreach my $vol (sort keys %$local_volumes) {
>  	    if ($local_volumes->{$vol} eq 'storage') {
> -- 
> 2.11.0
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel