[libvirt PATCH 02/11] qemu_snapshot: revert: always restart QEMU process for running VM

[Pavel Hrdina]

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 at redhat.com>
> > ---
> >  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?

Pavel

> Apart from that I agree that the "feature" of not restarting qemu for
> some reversions was more of a gimmick and micro-optimization and getting
> rid of it for code clarity is worthwhile even when it results in the
> users losing/needing reconnect the remote viewer connections.
> 
> Once you ensure that the genid is always regenerated when present:
> 
> Reviewed-by: Peter Krempa <pkrempa at redhat.com>
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211122/4c28ef1c/attachment-0001.sig>