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
