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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list