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