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

[Thomas Lamprecht]

On 10/11/19 1:45 PM, Alwin Antreich wrote:
> On Fri, Oct 11, 2019 at 12:17:28PM +0200, Thomas Lamprecht wrote:
>> 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

this is also API breakage, or? Why not a simple path check fallback in cfg2cmd?

>>>>> 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(-)
>>>>>

>>>>

>>>>>  	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; 
> Do you mean by "correct place", the else clause with the "--loadstate"?
> It can't go there because the config_to_command has to happen before, as
> it assigns the @$cmd. The other options are then pushed in addition to the
> @$cmd, if the statefile is equal to tcp or unix.
> 

but we could move that below?
Do a @extra_cmd (or the like) and have then one unified statefile handling?
I mean this method is already very big and the more things inside are spread
out the harder it's to maintain it..

Also this makes your "It uses the vmstate and de-/activates the state volume
in vm_start" sentence from the commit message false, as it's activated in
config_to_command not vm_start ...