On Mon, Nov 22, 2021 at 12:32:03 +0100, Pavel Hrdina wrote:
> On Tue, Nov 16, 2021 at 03:17:56PM +0100, Peter Krempa wrote:
> > On Mon, Nov 15, 2021 at 17:22:45 +0100, Pavel Hrdina wrote:
> > > Our compatibility check code isn't complete and there are cases where it
> > > fails to detect incompatible configuration and the revert fails. In
> > > addition future support for external snapshot will always require
> > > restarting the QEMU process.
> > >
> > > To unify the behavior drop the compatibility check code and always
> > > restart the QEMU process.
> > >
> > > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > > ---
> > > src/qemu/qemu_snapshot.c | 66 ++++++----------------------------------
> > > 1 file changed, 10 insertions(+), 56 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> > > index 3d6ec490ab..661f74146c 100644
> > > --- a/src/qemu/qemu_snapshot.c
> > > +++ b/src/qemu/qemu_snapshot.c
> > > @@ -1989,62 +1989,16 @@ qemuSnapshotRevert(virDomainObj *vm,
> >
> > [...]
> >
> > > /* Transitions 5, 6, 8, 9 */
> > > - /* If using VM GenID, there is no way currently to change
> > > - * the genid for the running guest, so set an error,
> > > - * mark as incompatible, and don't allow change of genid
> > > - * if the revert force flag would start the guest again. */
> > > - if (compatible && config->genidRequested) {
> > > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > - _("domain genid update requires restart"));
> > > - compatible = false;
> > > - start_flags &= ~VIR_QEMU_PROCESS_START_GEN_VMID;
> >
> > We still need this bit. If genid is requested we must ensure that the
> > new start of the VM will regenerate it to ensure that the guest can
> > detect the reversion.
>
> At the beginning of the function qemuSnapshotRevert() there is this
> line:
>
> unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
>
> so generating of VMID is enabled by default. This part of code disabled
> the VMID regeneration if FORCE flag had to be used to revert the
> snapshot. That change was introduced by commit <e5d7064be02a0105b7c>
> but it doesn't make sense to me.
>
> The commit messages states:
>
> If the the snapshot revert involves a forced revert option, then
> let's not cause startup to change the genid flag in order to signify
> that we're still running the same/previous guest and not some
> snapshot reversion.
>
> but how we would run the same guest if this code is reverting to
> different snapshot?
Oh, I didn't actually notice that this is masking out the
VIR_QEMU_PROCESS_START_GEN_VMID flag, so yeah it can be removed
completely.
