[libvirt] [PATCH] qemu: Add missing VIR_DOMAIN_BLOCK_COMMIT_DELETE flags

[Alex Jia]

On 08/27/2013 05:54 PM, Peter Krempa wrote:
> On 08/27/13 11:37, Michal Privoznik wrote:
>> On 27.08.2013 10:58, Alex Jia wrote:
>>> On 08/27/2013 04:47 PM, Peter Krempa wrote:
>>>> On 08/27/13 09:53, Alex Jia wrote:
>>>>> The flag "VIR_DOMAIN_BLOCK_COMMIT_DELETE" is missed by
>>>>> qemuDomainBlockCommit(),
>>>>> and then will hit error "unsupported flags (0x2) in function
>>>>> qemuDomainBlockCommit" if users run 'virsh blockcommit' with
>>>>> '--delete' option.
>>>>>
>>>>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1001475
>>>>>
>>>>> Signed-off-by: Alex Jia<ajia at redhat.com>
>>>>> ---
>>>>>    src/qemu/qemu_driver.c |    3 ++-
>>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>>> index ed29373..8863124 100644
>>>>> --- a/src/qemu/qemu_driver.c
>>>>> +++ b/src/qemu/qemu_driver.c
>>>>> @@ -14444,7 +14444,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const
>>>>> char *path, const char *base,
>>>>>        const char *base_canon = NULL;
>>>>>        bool clean_access = false;
>>>>>
>>>>> -    virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1);
>>>>> +    virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
>>>>> +                  VIR_DOMAIN_BLOCK_COMMIT_DELETE, -1);
>>>>>
>>>>>        if (!(vm = qemuDomObjFromDomain(dom)))
>>>>>            goto cleanup;
>>>>>
>>>> The code doesn't seem to support the BLOCK_COMMIT_DELETE flag. It was
>>> Yes, the codes haven't any implementation for BLOCK_COMMIT_DELETE flag
>>> now, maybe, only need to raise a friendly error message in here instead
>>> of "unsupported flags (0x2) xxxx".
>> I agree that this error message is not user-friendly. Bare virsh users
>> know nothing about our flags and their numerical expression. However, I
>> don't think there is a way how to produce "Unsupported flag
>> VIR_DOMAIN_BLOCK_COMMIT_DELETE" instead of "Unsupported flag 0x2" since
>> all we see in the qemuDomainBlockCommit() function is just number. I
>> mean, mapping of flag onto numeric value is not one-to-one function (aka
>> injective function). That is, a value 0x2 can express
>> VIR_DOMAIN_BLOCK_COMMIT_DELETE, VIR_DOMAIN_START_AUTODESTROY,
>> VIR_DUMP_DESTROY, etc. (git grep "1<<  1," include/).
>>
>> If we want to make it work, we have to introduce an injective function,
>> e.g. virUnsupportedFlags(), which would accept a list (not an ORed
>> value) of all flags that are not supported. Too much effort for not much
>> outcome.
> Additionally this would require updating the list of unsupported flags
> at each function when adding a new flag to keep them in sync or add a
> way to autogenerate the list automagically.
>
> It would be nice to have this automatic, but I think that Alex was
> suggesting something like
>
> if (flags&  VIR_DOMAIN_BLOCK_COMMIT_DELETE) {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "we don't support this yet");
> goto cleanup;
> }

Exactly, but I think it's only a temporary solution, the Michal's 
suggestion is more
reasonable, and your solution looks like more intelligent :)

> Peter
>
>> Michal
>>