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

[Kevin Wolf]

Am 28.06.2019 um 13:24 hat Denis Plotnikov geschrieben:
> 
> 
> On 28.06.2019 13:23, Kevin Wolf wrote:
> > Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> >> With the patch, qcow2 is able to process image compression type
> >> defined in the image header and choose the corresponding method
> >> for clusters compressing.
> >>
> >> Also, it rework the cluster compression code for adding more
> >> compression types.
> >>
> >> Signed-off-by: Denis Plotnikov <address@hidden>
> >> ---
> >>   block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
> >>   1 file changed, 92 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index c4b5b93408..90f15cc3c9 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState 
> >> *bs, uint64_t start_offset,
> >>               break;
> >>   
> >>           case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
> >> +            /* Compression type always goes with the compression type bit 
> >> set */
> >> +            if (!(s->incompatible_features & 
> >> QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> >> +                error_setg(errp,
> >> +                           "compression_type_ext: "
> >> +                           "expect compression type bit set");
> >> +                return -EINVAL;
> >> +            }
> >> +
> >> +            ret = bdrv_pread(bs->file, offset, &s->compression_type, 
> >> ext.len);
> >> +            s->compression_type = be32_to_cpu(s->compression_type);
> >> +
> >> +            if (ret < 0) {
> >> +                error_setg_errno(errp, -ret,
> >> +                                 "ERROR: Could not read compression 
> >> type");
> >> +                return ret;
> >> +            }
> >> +
> >>               /*
> >> -             * Setting compression type to 
> >> BDRVQcow2State->compression_type
> >> -             * from the image header is going to be here
> >> +             * The default compression type is not allowed when the 
> >> extension
> >> +             * is present. ZLIB is used as the default compression type.
> >> +             * When compression type extension header is present then
> >> +             * compression_type should have a value different from the 
> >> default.
> >>                */
> >> -             break;
> >> +            if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
> >> +                error_setg(errp,
> >> +                           "compression_type_ext:"
> >> +                           "invalid compression type %d",
> >> +                           QCOW2_COMPRESSION_TYPE_ZLIB);
> >> +            }
> > 
> > This is a restriction that the spec doesn't make, so strictly speaking
> > this implementation wouldn't be compliant to the spec.
> The idea is that ZLIB shouldn't appear in the compression type 
> extension. This allows image backward compatibility with an older qemu 
> if zlib is used.
> 
> There is no reason to set ZLIB in the extension because an older qemu 
> knows how to tread ZLIB compressed clusters.
> 
> The restriction aims to guarantee that.
> 
> I tried to describe this case in the specification:
> ...
> When the compression type bit is not set, and the compression type 
> header extension is absent, ZLIB compression is used for compressed 
> clusters.
> 
> Qemu versions older than 4.1 can use images created with compression 
> type ZLIB without any additional preparations and cannot use images 
> created with compression types != ZLIB.
> ...
> 
> Does it makes sense?

This text says that using zlib in the extension is not necessary because
it's the default. But it doesn't say that using zlib in the extension is
illegal.

I agree that there is no good reason to create a compression type
extension if you have zlib. But is there a good reason to forbid it? It
only requires us to add artificial restrictions to code that would work
fine without them.

Either way, if we want to reject such extensions, the spec needs to say
that it's illegal. And if the spec allows such images, we must accept
them.

> > We can discuss whether the code or the spec should be changed. At the
> > moment, I don't see a good reason to make the restriction
> > 
> >> +#ifdef DEBUG_EXT
> >> +            printf("Qcow2: image compression type %s\n", 
> >> s->compression_type);
> >> +#endif
> >> +            break;
> >>   
> >>           case QCOW2_EXT_MAGIC_DATA_FILE:
> >>           {
> > 
> > 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.

Kevin