3:42 p.m.
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