Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing

[Denis Plotnikov]

On 28.06.2019 22:34, Eric Blake wrote:
> On 6/28/19 9:54 AM, Kevin Wolf wrote:
> 
>>>>>>>> We would save most of this code if we added a new field to the header
>>>>>>>> instead of adding a header extension. Not saying that we should
>>>>>>>> definitely do this, but let's discuss it at least.
>>>>>>>
>>>>>>> If we add the new field to the header will the older qemu be able to use
>>>>>>> it. Or we will add the header only if needed, i.e. if compression_type
>>>>>>> != zlib
>>>>>>
>>>>>> Increasing the header size is backwards compatible. Older qemu versions
>>>>>> should handle such images correctly. They would store the unknown part
>>>>>> of the header in s->unknown_header_fields and keep it unmodified when
>>>>>> updating the image header.
>>>>>>
>>>>>> We would still add the incompatible feature flag for non-zlib, of
>>>>>> course.
>>>>> so, we basically need to do the same: store compression type and forbid
>>>>> to use because of flag if not zlib.
>>>>>
>>>>> Sounds like it doesn't differ that much from the extension header 
>>>>> approach.
>>>>
>>>> It provides more or less the same functionality, but would probably make
>>>> this patch half the size because all of the code related to reading and
>>>> checking the header extension would go away. It also saves a few bytes
>>>> in the header cluster (4 bytes vs. 16 bytes).
>>> ok, will re-do it that way.
>>>
>>> Do you agree in general with how zlib compression type is treated?
>>
>> As I said, I think both ways are justifiable as long as we stay
>> consistent between qemu and spec.
>>
>> I'd prefer to allow zlib in the extension, you'd prefer to forbid it.
>> So I'd like to hear opinions from some more people on which way they
>> prefer.
> 
> My preferences - use a 4 byte header field, and require the incompatible
> feature bit if the field is non-zero. The standard should allow someone
> to explicitly request zlib compression (done by leaving the incompatible
> bit clear, then specifying a header length of 108 instead of 104, but
> leaving the compression field at 104-107 at 0), to implicitly request
> zlib compression (done by leaving the incompatible bit clear, and
> specifying a header length of 104); or to explicitly request some other
> compression (done by setting the incompatible bit, specifying a header
> length of 108, and putting a non-zero value in the compression field
> 104-107).
> 
> Under these rules, if you implicitly or explicitly request zlib, your
> image can be opened without problems by both older and newer qemu.  If
> you explicitly request zstd, your image will fail to open by older qemu
> (good, because they would misinterpret compressed clusters), and work
> with newer qemu.  And since providing a 108-byte header works just fine
> with older qemu as long as the header contains 0, I recommend that we
> just always make newer qemu provide that field (even if it is explicitly
> set to zlib), as that is less complicated than only providing the larger
> header for non-zlib files (we still have to parse 104-byte headers, but
> that doesn't mean we have to create brand-new files that way).
> 
> There's one more corner case. I recommend that the standard require that
> it be an error to set the incompatible feature bit but use a header size
> of 104 - if you have an imabe like that, the image would be treated as
> using zlib (implicitly due to the header size), so older images _could_
> use it other than the fact that they don't recognize the incompatible
> feature bit.  On the other hand, requiring such an image to be rejected
> is a bit of a stretch - no qemu (whether one that understands the
> feature bit or one that does not) would misinterpret the image contents
> as being zlib compressed, if it had not been for the bit being set.  So
> in this corner case, I'm fine if you end up documenting whatever is
> easiest to code.
> 

Ok, I'll re-do the series introducing compression type in the header.
Thanks!

Denis

-- 
Best,
Denis