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

[Eric Blake]

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.

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

signature.asc
Description: OpenPGP digital signature