[libvirt] [PATCH v3 03/18] blockjob: split out block info driver handling

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140904/84ab0247/attachment-0001.sig>