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

[Eric Blake]

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.


>>> @@ -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.


>>> +        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.


>>> +++ 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).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature