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

[Thomas Lamprecht]

On 10/14/19 11:59 AM, Alwin Antreich wrote:
> On Mon, Oct 14, 2019 at 11:44:59AM +0200, Thomas Lamprecht wrote:
>> 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?
> That's what I am not sure about, see my earlier email. Should I check
> for file/device paths or just drop it?
> https://pve.proxmox.com/pipermail/pve-devel/2019-October/039465.html
> 

I wouldn't just drop it, even if it's just a edge case and does not affect
many users (if even some)I mean if it worked before it should keep doing so.
A simple "if (-e $vmstate)" could be enough and cheap to check.. 

>>> 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..
> Yes, I will go ahead and put this into v3.
I mean it was rather an open question, .. not 100% sure it's the best
thing to do (just FYI)

> 
>>
>> 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 ...
> True, I will rewrite the commit message in v3 to also better reflect the
> outcome of the discussion.
> 
thanks!