Re: [libvirt] [PATCH 2/5] qemu: snapshot: Fix job handling when creating snapshots
On 09/03/2014 07:53 AM, Peter Krempa wrote:
Creating snapshots modifies the domain state. Currently we
wouldn't
enter the job for certain operations although they would modify the
state. Refactor job handling so that everything is covered by an async
job.
---
src/qemu/qemu_driver.c | 176 ++++++++++++++++++++++---------------------------
1 file changed, 78 insertions(+), 98 deletions(-)
@@ -13466,10 +13430,19 @@ qemuDomainSnapshotCreateXML(virDomainPtr
domain,
goto cleanup;
}
+ /* We are going to modify the domain below. Internal snapshots would use
+ * a regular job, so we need to set the job mask to disallow query as
+ * 'savevm' blocks the monitor. External snapshot will then modify the
+ * job mas appropriately. */
s/mas/mask/
+ endjob:
+ if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
+ if (qemuDomainSnapshotWriteMetadata(vm, snap,
+ cfg->snapshotDir) < 0) {
+ /* if writing of metadata fails, error out rather than trying
+ * to silently carry on without completing the snapshot */
While reindenting this, you could also fix the accidental double space
that I stuck in there.
+
+ if (!qemuDomainObjEndAsyncJob(driver, vm)) {
+ /* Only possible if a transient vm quit while our locks were down,
+ * in which case we don't want to save snapshot metadata. */
This comment is now out of date, because you placed it below the point
where you already saved the metadata.
But overall, looks like the right thing to do - you widened the overall
job to cover everything, rather than grabbing the job inside helper
functions. ACK with the nits cleaned up.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 539 bytes)