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
