On 09/04/2014 09:11 AM, Peter Krempa wrote:
>> modify command. Technically, there is one case where getting
>> block job info can modify domain XML - we do snooping to see if
>> a 2-phase job has transitioned into the second phase, for an
>> optimization in the case of old qemu that lacked an event for
>> the transition. But I think that optimization is safe; and if
>> it proves to be a problem, we could use the difference between
>> QEMU_CAPS_BLOCKJOB_{ASYNC,SYNC} to determine whether we even
>> need snooping, and request a modifying job in that case.
>>
>> +
>> + if (info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT &&
>> + disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)
>> + info->type = disk->mirrorJob;
>> +
>> + /* Snoop block copy operations, so future cancel operations can
>> + * avoid checking if pivot is safe. Save the change to XML, but
>> + * we can ignore failure because it is only an optimization. */
>> + if (ret == 1 && disk->mirror &&
>> + info->cur == info->end && !disk->mirrorState) {
>> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> +
>> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
>
> Atomicity is guaranteed by holding the domain lock here. We should
> document that the mirrorState field can change even when the _MODIFY job
> is held though.
>
> Otherwise I don't see a problem currently with this as long as it is
> stated somewhere. Possibly even in a comment here.
>
> ACK
Here's what I squashed in.
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index e7bd504..fb13169 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -15289,7 +15289,9 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom,
const char *path,
/* Snoop block copy operations, so future cancel operations can
* avoid checking if pivot is safe. Save the change to XML, but
- * we can ignore failure because it is only an optimization. */
+ * we can ignore failure because it is only an optimization. We
+ * hold the vm lock, so modifying the in-memory representation
+ * even though we are a query rather than a modify job, is safe. */
if (ret == 1 && disk->mirror &&
info->cur == info->end && !disk->mirrorState) {
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
--
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