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

[Denis Plotnikov]

On 03.07.2019 18:13, Eric Blake wrote:
> On 7/3/19 10:01 AM, Denis Plotnikov wrote:
> 
>>>> +     * 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 have an artificial restriction one way or the other.
> 
> Method 1 - artificial restriction that incompatible bit must NOT be set
> when compression type is zlib
> 
> Method 2 - artificial restriction that older qcow2 programs can't open a
> zlib image with incompatible bit set, even though removing the bit makes
> the image more portable.
> 
> It's desirable that qemu should NOT set the incompatible bit when it
> does not need to (for the sake of portability to more apps), but
> MANDATING that it must not set the bit for zlib is a stronger spec
> restriction.
> 
> I tend to lean for the spec being looser unless there is a strong reason
> for why it must be strict; just because qemu won't be setting the
> incompatible bit does not necessarily mean that all other
> implementations will try to be that careful; they may have valid reasons
> for setting the bit even when using zlib, but only if the spec permits
> them to do so.
So, how I should implement that? Method 1 or Method 2?
How we can decide? Ask what other maintainers think about that?
> 
> 
>>>> @@ -2434,6 +2493,13 @@ int qcow2_update_header(BlockDriverState *bs)
>>>>        total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
>>>>        refcount_table_clusters = s->refcount_table_size >> 
>>>> (s->cluster_bits - 3);
>>>>    
>>>> +    ret = check_compression_type(s, NULL);
>>> Why are you ignoring the error here?
>> qcow2_update_header() doesn't use the error and just returns an error
>> code or 0
> 
> Are we potentially losing a valuable error message (in which case
> qcow2_update_header should maybe be first patched to take an errp
> parameter), or is it always going to succeed (in which case &error_abort
> would document our intention that we know it can't fail), or is it
> really a case where it may fail, but we don't care about losing the
> message?  Passing NULL is not wrong (as you say, we aren't plumbed to
> pass the message back up to the caller), but does raise enough
> suspicions as to be worth auditing the code while in the area.

Could we do it after adding this series since it already implemented 
without the error?
> 
> 
>>>> +        104 - 107:  compression_type
>>>> +                    Defines the compression method used for compressed 
>>>> clusters.
>>>> +                    A single compression type is applied to all 
>>>> compressed image
>>>> +                    clusters.
>>>> +                    The compression type is set on image creation only.
>>>> +                    The default compression type is zlib.
>>> Where is the documentation that a value of 0 corresponds to zlib?
>> Should I do it right here or it's better to add a separate chapter in
>> the  docs/interop/qcow2.txt ?
> 
> Right here.
ok
> 
> 
>>>> +++ 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 ?
> 
> Because it shows that we are running a new enough qemu that knows about
> the compression field (even when it is absent).
ok, if everybody agree with that I'll implement whatever is better
> 

-- 
Best,
Denis