Re: [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature

[Denis Plotnikov]

On 03.07.2019 18:46, Kevin Wolf wrote:
> Am 03.07.2019 um 17:01 hat Denis Plotnikov geschrieben:
>> On 03.07.2019 17:14, Eric Blake wrote:
>>> On 7/3/19 6:00 AM, Denis Plotnikov wrote:
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 3ace3b2209..921eb67b80 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState 
>>>> *bs, QDict *options,
>>>>        return ret;
>>>>    }
>>>>    
>>>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>>>> +{
>>>> +    bool is_set;
>>>> +    int ret = 0;
>>>> +
>>>> +    switch (s->compression_type) {
>>>> +    case QCOW2_COMPRESSION_TYPE_ZLIB:
>>>> +        break;
>>>> +
>>>> +    default:
>>>> +        if (errp) {
>>> Useless check for errp being non-NULL.  What's worse, if the caller
>>> passes in NULL because they don't care about the error, then your code
>>> behaves differently.  Just call error_setg() and return -ENOTSUP
>>> unconditionally.
>> ok
>>>
>>>> +            error_setg(errp, "qcow2: unknown compression type: %u",
>>>> +                       s->compression_type);
>>>> +            return -ENOTSUP;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
>>>> +     * feature flag must be absent, with other compression types the
>>>> +     * incompatible feature flag must be set
>>> Is there a strong reason for forbid the incompatible feature flag with
>>> ZLIB?
>> The main reason is to guarantee image opening for older qemu if
>> compression type is zlib.
>>> Sure, it makes the image impossible to open with older qemu, but
>>> it doesn't get in the way of newer qemu opening it. I'll have to see how
>>> your spec changes documented this, to see if you could instead word it
>>> as 'for ZLIB, the incompatible feature flag may be absent'.
>> I just can't imagine when and why we would want to set incompatible
>> feature flag with zlib. Suppose we have zlib with the flag set, then
>> older qemu can't open the image although it is able to work with the
>> zlib compression type. For now, I don't understand why we should make
>> such an artificial restriction.
> 
> We don't want to create such images, but we want to keep our code as
> simple as possible.
> 
> As your patch shows, forbidding the case is more work than just allowing
> it. So if external software can create such images, and it would just
> automatically work in QEMU, then why do the extra work to articifially
> forbid this?
> 
> (Actually, it's likely that on the first header update, QEMU would just
> end up dropping the useless flag, so we even "fix" such images.)
ok, removing the restriction
> 
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 7ccbfff9d0..6aa8b99993 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -78,6 +78,8 @@
>>>>    #
>>>>    # @bitmaps: A list of qcow2 bitmap details (since 4.0)
>>>>    #
>>>> +# @compression-type: the image cluster compression method (since 4.1)
>>>> +#
>>>>    # Since: 1.7
>>>>    ##
>>>>    { 'struct': 'ImageInfoSpecificQCow2',
>>>> @@ -89,7 +91,8 @@
>>>>          '*corrupt': 'bool',
>>>>          'refcount-bits': 'int',
>>>>          '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>>> -      '*bitmaps': ['Qcow2BitmapInfo']
>>>> +      '*bitmaps': ['Qcow2BitmapInfo'],
>>>> +      '*compression-type': 'Qcow2CompressionType'
>>> Why is this field optional? Can't we always populate it, even for older
>>> images?
>> Why we should if we don't care ?
> 
> I was trying too check what the condition is under which the field will
> be present in the output, but I couldn't find any code for it.
> 
> So it looks like this patch never makes use of the field and it's dead
> code?
ok
> 
> Kevin
> 

-- 
Best,
Denis