On 07/29/2014 05:40 AM, Peter Krempa wrote:
Fix things by saving state any time we modify live XML, while delaying XML disk modifications until after the event completes. We still need a to teach libvirtd restarts to examine all existing <mirror> elements to see if the job completed in the meantime (that is, if libvirtd misses the event, the updated state still needs to be updated in live XML), but that will be a later patch, in part because we also need to to start taking advantage of newer qemu's ability to keep the job around after completion rather than the current usage where the job disappears both on error and on success.
+ /* If we completed a block pull or commit, then update the XML + * to match. */ + if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) { + bool has_mirror = !!disk->mirror;
The control flow is becoming less obvious in this function.
Yeah, I struggled on that for a while.
+ + if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_PIVOT) { + /* XXX We want to revoke security labels and disk + * lease, as well as audit that revocation, before + * dropping the original source. But it gets tricky + * if both source and mirror share common backing + * files (we want to only revoke the non-shared + * portion of the chain); so for now, we leak the + * access to the original. */ + virStorageSourceFree(disk->src); + disk->src = disk->mirror; + disk->mirror = NULL;
If we were here, then
+ } + if (has_mirror) {
This condition is also true and we try to free the source of the mirror again, even if we did it above. On the other hand, this is reached even if the _ABORT job is completed, where this makes sense.
Basically, once a job completes, we have to clean up disk->mirror whether it was abort or pivot; but if it was pivot, we also have to adjust the source to be the former mirror contents. But I agree that a minor tweak to the flow makes it easier to read.
qemuDomainDetermineDiskChain(driver, vm, disk, true); - if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + } + + if (disk->mirror &&
Now this really is an else section to the above as both paths above clear disk->mirror.
Also correct.
ACK, the function flow isn't obvious but it's correct from my POV
After rebasing on top of 1/4, here's what I squashed in to improve the control flow, before pushing 1 and 2. I'll wait on 3 and 4 for any other opinions on whether active commit deserves to be in rc2. diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 1340c8d..48da843 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1033,8 +1033,6 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, /* If we completed a block pull or commit, then update the XML * to match. */ if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) { - bool has_mirror = !!disk->mirror; - if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) { /* XXX We want to revoke security labels and disk * lease, as well as audit that revocation, before @@ -1045,22 +1043,19 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, * access to the original. */ virStorageSourceFree(disk->src); disk->src = disk->mirror; - disk->mirror = NULL; - } - if (has_mirror) { + } else { virStorageSourceFree(disk->mirror); - disk->mirror = NULL; - save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE; - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; } + /* Recompute the cached backing chain to match our * updates. Better would be storing the chain ourselves * rather than reprobing, but we haven't quite completed * that conversion to use our XML tracking. */ + disk->mirror = NULL; + save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; qemuDomainDetermineDiskChain(driver, vm, disk, true); - } - - if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + } else if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { if (status == VIR_DOMAIN_BLOCK_JOB_READY) { disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; save = true; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org