[libvirt] [PATCH] blockcommit: document semantics of committing active layer

[Eric Blake]

On 05/19/2014 01:58 AM, Peter Krempa wrote:

[Sorry for not putting RFC in the subject line]

>> +++ b/src/qemu/qemu_driver.c
>> @@ -15377,6 +15377,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
>>      const char *top_parent = NULL;
>>      bool clean_access = false;
>>
>> +    /* XXX Add support for COMMIT_ACTIVE, COMMIT_DELETE */
>>      virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1);
>>
>>      if (!(vm = qemuDomObjFromDomain(dom)))
>> @@ -15423,6 +15424,14 @@ qemuDomainBlockCommit(virDomainPtr dom,
>>                                                       &top_parent)))
>>          goto endjob;
>>
>> +    /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */
> 
> I think we should. But I agree in postponing it to later patch.
> 
>> +    if (topSource == &disk->src && !(flags & VIR_DOMAIN_BLOCK_COPY_ACTIVE)) {
> 
> As mentioned in the previous mail, this breaks build.
> 
> s/BLOCK_COPY/BLOCK_COMMIT/

Blah - serves me right for sending a patch at the end of my day. But I'm
glad you were able to see the intent.

> 
> Also shouldn't this hunk actually go in the patch that adds the active
> block commit feature? It seems a bit out of place here.

_This_ patch is fixing the qemu driver to not hang, by acknowledging
that we _don't_ support active block commit (the virCheckFlags() at the
beginning of the function fails if the new flag is specified, and this
check fails if the flag is not specified - which means you cannot do
active commits).  The next few patches (when I develop it into a full
series) will first be to add support for the new flag (fixing the first
XXX) and then to relax things to auto-pivot when the flag is not present
(fixing the second XXX).

> 
>> +        virReportError(VIR_ERR_INVALID_ARG,
>> +                       _("commit of '%s' active layer requires active flag"),
>> +                       disk->dst);
>> +        goto endjob;
>> +    }
>> +
>>      if (!topSource->backingStore) {
>>          virReportError(VIR_ERR_INVALID_ARG,
>>                         _("top '%s' in chain for '%s' has no backing file"),
>>
> 
> I think that this patch should also export this new flag in virsh as we
> usually do with API flag additions.

Yes, that's my next task before turning this from an RFC into an actual
patch series, even before the qemu implementation is done.

> 
> Additionally a change in virsh is missing again breaks the build process:

Yep, my fault for not actually compile-testing in my rush to get it out
before the weekend :)

-- 
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: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140519/9a67dd4b/attachment-0001.sig>