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 <dplotni...@virtuozzo.com> >> --- >> 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? > > 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 > >> @@ -1492,6 +1520,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState >> *bs, QDict *options, >> QLIST_INIT(&s->cluster_allocs); >> QTAILQ_INIT(&s->discards); >> >> + /* Set default compression type */ >> + s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB; >> + >> /* read qcow2 extensions */ >> if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL, >> flags, &update_header, &local_err)) { >> @@ -1500,6 +1531,34 @@ static int coroutine_fn >> qcow2_do_open(BlockDriverState *bs, QDict *options, >> goto fail; >> } >> >> + /* >> + * The compression type is set on the extension header processing >> + * if the compression type extension header is present. >> + * When the compression type is different from ZLIB (default) there >> + * should be both the compression type bit and the compression >> + * type extension header set. When the ZLIB (default) compression >> + * type is used there should be neither the compression type bit nor >> + * the compression type extension header set. >> + */ >> + >> + if ((s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) & >> + (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB)) { >> + error_setg(errp, "Illegal compression type setting"); >> + ret = -EINVAL; >> + goto fail; >> + } > > Almost the same restriction as above, implemented a second time. > > The difference is that you get the first one only for compression type > extensions that specify zlib, and you get this one also for images that > have the compression type feature flag, but are missing the extension. > > So in the current shape of the patch, this is in fact just the wrong > error message. It should be something like "compression type header > extension missing". > > This restriction (if the flag is set, the header extension must be > present) actually makes sense to me. Switching to a new header field > would get rid of the whole case. > >> + /* Check available compression types */ >> + switch (s->compression_type) { >> + case QCOW2_COMPRESSION_TYPE_ZLIB: >> + break; >> + >> + default: >> + error_setg(errp, "Unknown compression type"); >> + ret = -EINVAL; >> + goto fail; >> + } >> + >> /* Open external data file */ >> s->data_file = bdrv_open_child(NULL, options, "data-file", bs, >> &child_file, >> true, &local_err); >> @@ -3970,7 +4029,7 @@ fail: >> } >> >> /* >> - * qcow2_compress() >> + * zlib_compress() >> * >> * @dest - destination buffer, @dest_size bytes >> * @src - source buffer, @src_size bytes >> @@ -3979,7 +4038,7 @@ fail: >> * -1 destination buffer is not enough to store compressed data >> * -2 on any other error >> */ >> -static ssize_t qcow2_compress(void *dest, size_t dest_size, >> +static ssize_t zlib_compress(void *dest, size_t dest_size, >> const void *src, size_t src_size) > > Indentation is now off in the second line. > >> { >> ssize_t ret; >> @@ -4013,7 +4072,7 @@ static ssize_t qcow2_compress(void *dest, size_t >> dest_size, >> } >> >> /* >> - * qcow2_decompress() >> + * zlib_decompress() >> * >> * Decompress some data (not more than @src_size bytes) to produce exactly >> * @dest_size bytes. > > Should the description be changed, too? (qcow2_compress() doesn't have > any - should it have some?) > >> @@ -4024,7 +4083,7 @@ static ssize_t qcow2_compress(void *dest, size_t >> dest_size, >> * Returns: 0 on success >> * -1 on fail >> */ >> -static ssize_t qcow2_decompress(void *dest, size_t dest_size, >> +static ssize_t zlib_decompress(void *dest, size_t dest_size, >> const void *src, size_t src_size) > > Indentation again. > > Maybe keep the qcow2_ prefix and make it qcow2_(de)compress_zlib()? ok Denis > > Kevin > -- Best, Denis