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