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

[Thomas Lamprecht]

On 10/11/19 12:02 PM, Alwin Antreich wrote:
> On Fri, Oct 11, 2019 at 07:10:53AM +0200, Thomas Lamprecht wrote:
>> 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? 
> No, the config_to_command takes care of activation and path generation
> of the vmstate. The same way as the resume of a hibernated VM.
> 

then write/explain such things in the commit message...

>>
>> 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)
> My first version was in this intention. After talking with Fabain G., I
> made the v2, to re-use the same method as the resume of an hibernated
> VM. I have no bias here, either way is fine for me.

but you can still do it here even if you put it in the config, here is the
correct place to do:

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

I.e., I never said you should go back and handle it in your v1.. I approve
with Fabians suggestion. The above postif just seemed out-of-place, especially
if we have a $statefile handling already here..