[pve-devel] [PATCH qemu-server v2] Fix #2171: VM statefile was not activated

[Thomas Lamprecht]

On 10/10/19 3:58 PM, Alwin Antreich wrote:
> Machine states that were created on snapshots with memory could not be
> restored on rollback. The state volume was not activated so KVM couldn't
> load the state.
> 
> This patch removes the path generation on rollback. It uses the vmstate
> and de-/activates the state volume in vm_start. This in turn disallows
> the use of path based statefiles when used with the '--stateuri' option
> on 'qm start'. Only 'tcp', 'unix' and our storage based URIs can be
> used now.
> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>  PVE/QemuConfig.pm | 3 +--
>  PVE/QemuServer.pm | 8 +++++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index edbf1a7..e9796a3 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -359,8 +359,7 @@ sub __snapshot_rollback_vm_start {
>      my ($class, $vmid, $vmstate, $data) = @_;
>  
>      my $storecfg = PVE::Storage::config();
> -    my $statefile = PVE::Storage::path($storecfg, $vmstate);
> -    PVE::QemuServer::vm_start($storecfg, $vmid, $statefile, undef, undef, undef, $data->{forcemachine});
> +    PVE::QemuServer::vm_start($storecfg, $vmid, $vmstate, undef, undef, undef, $data->{forcemachine});
>  }
>  
>  sub __snapshot_rollback_get_unused {
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index ac9dfde..d4feae9 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5340,6 +5340,7 @@ sub vm_start {
>  	die "you can't start a vm if it's a template\n" if PVE::QemuConfig->is_template($conf);
>  
>  	my $is_suspended = PVE::QemuConfig->has_lock($conf, 'suspended');
> +	$conf->{vmstate} = $statefile if ($statefile && $statefile ne 'tcp' && $statefile ne 'unix');

why? I mean you then get it out of this hash in the same submethod, i.e,
same scope, below again? 

And even if you'd need it and you just decided to not explain why
in the commit message it would be still better to get it ...

>  
>  	PVE::QemuConfig->check_lock($conf)
>  	    if !($skiplock || $is_suspended);
> @@ -5465,8 +5466,6 @@ sub vm_start {
>  		push @$cmd, '-incoming', $migrate_uri;
>  		push @$cmd, '-S';
>  
> -	    } else {
> -		push @$cmd, '-loadstate', $statefile;

... here, as we really have exact the condition you checked
above: $statefile truthy, but neither 'tcp' or 'unix'...

But as said, I'd rather not have it in the $conf (which can get written out
again) but maybe rather:

$statefile //= $conf->{vmstate};

and then just use $statefile... (I mean rename it to $vmstate, if you want)

>  	    }
>  	} elsif ($paused) {
>  	    push @$cmd, '-S';
> @@ -5616,12 +5615,15 @@ sub vm_start {
>  		    property => "guest-stats-polling-interval",
>  		    value => 2) if (!defined($conf->{balloon}) || $conf->{balloon});
>  
> -	if ($is_suspended && (my $vmstate = $conf->{vmstate})) {
> +	my $vmstate = $conf->{vmstate};
> +	if ($is_suspended && $vmstate) {
>  	    print "Resumed VM, removing state\n";
>  	    delete $conf->@{qw(lock vmstate runningmachine)};
>  	    PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
>  	    PVE::Storage::vdisk_free($storecfg, $vmstate);
>  	    PVE::QemuConfig->write_config($vmid, $conf);
> +	} elsif ($vmstate) {
> +	    PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
>  	}

to be more clear that we always want to deactivate and for nicer code
in general I'd do:

        if ($vmstate) {
            # always deactive vmstate volume again!
            PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
            if ($is_suspended) {
                print "Resumed VM, removing state\n";
                delete $conf->@{qw(lock vmstate runningmachine)};
                PVE::Storage::vdisk_free($storecfg, $vmstate);
                PVE::QemuConfig->write_config($vmid, $conf);
            }
        }



As then you have a clear linear flow in the if branches.
(note: $vmstate is $statefile, or whatever we call it then)