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
> > >>>>> used now. > >>>>> > >>>>> Signed-off-by: Alwin Antreich <a.antre...@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.. Yes, I will go ahead and put this into v3. > > 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. _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel