The flag creates additional points of failure which are hard to recover from, such as when thawing of the filesystems fails after an otherwise successful snapshot. Encougage use of explicit virDomainFSFreeze/virDomainFSThaw. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libvirt-domain-snapshot.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index f856e5b9b8..15bf4d8634 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -174,6 +174,9 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * file systems in use within domain OS. However, if the guest agent * is not present, an error is thrown. Moreover, this flag requires * VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY to be passed as well. + * For better control and error recovery users should invoke virDomainFSFreeze + * manually before taking the snapshot and then virDomainFSThaw to restore the + * VM rather than using VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE. * * By default, if the snapshot involves external files, and any of the * destination files already exist as a non-empty regular file, the -- 2.29.2

If we didn't freeze any filesystems we should not even attempt thawing them. Additionally 'guest-fsfreeze-freeze' fails if the filesystems are already frozen, where thawing them may break users data integrity if they used VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE accidentally after an explicit virDomainFSFreeze and the next snapshot without that flag would be taken with already thawed filesystems. This effectively reverts 7c736bab06479ccec59df69fb79a5c06d112d8fb . Libvirt nowadays checks whether the guest agent is connected and pings it before issuing an command so it's very unlikely that we'd end up in a situation where qemuSnapshotCreateActiveExternal froze filesystems and didn't thaw them. Additionally we now discourage the use of VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE since users have better control if they freeze the FS themselves. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 115c2fc91b..eacc8c6400 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1403,7 +1403,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver, virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); bool memory = snapdef->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; bool memory_unlink = false; - int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ + bool thaw = false; bool pmsuspended = false; int compressed; g_autoptr(virCommand) compressor = NULL; @@ -1415,7 +1415,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver, * The command will fail if the guest is paused or the guest agent * is not running, or is already quiesced. */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) { - int freeze; + int frozen; if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup; @@ -1425,16 +1425,14 @@ qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver, goto cleanup; } - freeze = qemuSnapshotFSFreeze(vm, NULL, 0); + frozen = qemuSnapshotFSFreeze(vm, NULL, 0); qemuDomainObjEndAgentJob(vm); - if (freeze < 0) { - /* the helper reported the error */ - if (freeze == -2) - thaw = -1; /* the command is sent but agent failed */ + if (frozen < 0) goto cleanup; - } - thaw = 1; + + if (frozen > 0) + thaw = true; } /* We need to track what state the guest is in, since taking the @@ -1527,7 +1525,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); resume = false; - thaw = 0; + thaw = false; virObjectEventStateQueue(driver->domainEventState, event); } else if (memory && pmsuspended) { /* qemu 1.3 is unable to save a domain in pm-suspended (S3) @@ -1559,14 +1557,12 @@ qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver, ret = -1; } - if (thaw != 0 && + if (thaw && qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) >= 0 && virDomainObjIsActive(vm)) { - if (qemuSnapshotFSThaw(vm, ret == 0 && thaw > 0) < 0) { - /* helper reported the error, if it was needed */ - if (thaw > 0) - ret = -1; - } + /* report error only on an otherwise successful snapshot */ + if (qemuSnapshotFSThaw(vm, ret == 0) < 0) + ret = -1; qemuDomainObjEndAgentJob(vm); } -- 2.29.2

The -2 value is misleading because if 'qemuAgentFSFreeze' fails it doesn't necessarily mean that the command was sent to the agent. Since callers don't care about the -2 value specifically, remove it. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index eacc8c6400..93b74b035a 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -119,9 +119,6 @@ qemuSnapshotCountExternal(void *payload, } -/* Return -1 if request is not sent to agent due to misconfig, -2 if request - * is sent but failed, and number of frozen filesystems on success. If -2 is - * returned, FSThaw should be called revert the quiesced status. */ int qemuSnapshotFSFreeze(virDomainObjPtr vm, const char **mountpoints, @@ -136,7 +133,7 @@ qemuSnapshotFSFreeze(virDomainObjPtr vm, agent = qemuDomainObjEnterAgent(vm); frozen = qemuAgentFSFreeze(agent, mountpoints, nmountpoints); qemuDomainObjExitAgent(vm, agent); - return frozen < 0 ? -2 : frozen; + return frozen; } -- 2.29.2