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 <dplotnikov@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);
+ }
+#ifdef DEBUG_EXT
+ printf("Qcow2: image compression type %s\n", s->compression_type);
+#endif
+ break;
case QCOW2_EXT_MAGIC_DATA_FILE:
{
@@ -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;
+ }
+
+ /* 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)
{
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.
@@ -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)
{
int ret = 0;
@@ -4122,16 +4181,38 @@ static ssize_t coroutine_fn
qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
const void *src, size_t src_size)
{
- return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
- qcow2_compress);
+ BDRVQcow2State *s = bs->opaque;
+ Qcow2CompressFunc fn;
+
+ switch (s->compression_type) {
+ case QCOW2_COMPRESSION_TYPE_ZLIB:
+ fn = zlib_compress;
+ break;
+
+ default:
+ return -ENOTSUP;
+ }
+
+ return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
}
static ssize_t coroutine_fn
qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
const void *src, size_t src_size)
{
- return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
- qcow2_decompress);
+ BDRVQcow2State *s = bs->opaque;
+ Qcow2CompressFunc fn;
+
+ switch (s->compression_type) {
+ case QCOW2_COMPRESSION_TYPE_ZLIB:
+ fn = zlib_decompress;
+ break;
+
+ default:
+ return -ENOTSUP;
+ }
+
+ return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
}
/* XXX: put compressed sectors first, then all the cluster aligned
--
2.17.0
28.05.2019 17:37, Denis Plotnikov wrote:
> 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 <dplotnikov@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);
> + }
> +#ifdef DEBUG_EXT
> + printf("Qcow2: image compression type %s\n", s->compression_type);
> +#endif
> + break;
>
> case QCOW2_EXT_MAGIC_DATA_FILE:
> {
> @@ -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;
> + }
> +
> + /* 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)
> {
> 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.
> @@ -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)
> {
> int ret = 0;
> @@ -4122,16 +4181,38 @@ static ssize_t coroutine_fn
> qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
> const void *src, size_t src_size)
> {
> - return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
> - qcow2_compress);
> + BDRVQcow2State *s = bs->opaque;
> + Qcow2CompressFunc fn;
> +
> + switch (s->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> + fn = zlib_compress;
> + break;
> +
> + default:
> + return -ENOTSUP;
> + }
> +
> + return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
> }
>
> static ssize_t coroutine_fn
> qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
> const void *src, size_t src_size)
> {
> - return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
> - qcow2_decompress);
> + BDRVQcow2State *s = bs->opaque;
> + Qcow2CompressFunc fn;
> +
> + switch (s->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> + fn = zlib_decompress;
> + break;
> +
> + default:
> + return -ENOTSUP;
> + }
> +
> + return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
> }
>
> /* XXX: put compressed sectors first, then all the cluster aligned
>
These things (compression) are moved to separate file by my patches, staged in Max's block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
--
Best regards,
Vladimir
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 <dplotnikov@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.
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.
> @@ -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()?
Kevin
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 <dplotnikov@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
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 <dplotnikov@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?
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
On 28.06.2019 15:06, Kevin Wolf wrote:
> 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 <dplotnikov@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?
>
> 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?
I think yes, if we create image with the extension set to zlib we
prevent an older qemu from using that image. Furthermore, to allow older
qemu using such images we need to create special conversion procedure
which has to remove the extension header.
If zlib is a "special compression type" which is always set by default
without the extension header we'll get rid of such image conversion
procedure and an older qemu could use it "as is"
Might it work as a good reason?
> 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.
Yes, it's true
The only reasons that zlib compression type even exists in the
enumeration is to avoid ambiguity for users.
For them it may be hard to understand why they can set zstd and cannot
set zlib as compression type and to really set zlib they have to set no
compression type to make the default zlib to apply.
When a user set zlib as compression type the image is created as before
the extension header were introduced.
Reasonable?
>
>>> 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.
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.
May be I'm missing something. Please correct my if so.
Denis
>
> Kevin
>
--
Best,
Denis
Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
>
>
> On 28.06.2019 15:06, Kevin Wolf wrote:
> > 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 <dplotnikov@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?
> >
> > 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?
> I think yes, if we create image with the extension set to zlib we
> prevent an older qemu from using that image. Furthermore, to allow older
> qemu using such images we need to create special conversion procedure
> which has to remove the extension header.
>
> If zlib is a "special compression type" which is always set by default
> without the extension header we'll get rid of such image conversion
> procedure and an older qemu could use it "as is"
>
> Might it work as a good reason?
>
> > 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.
> Yes, it's true
>
> The only reasons that zlib compression type even exists in the
> enumeration is to avoid ambiguity for users.
> For them it may be hard to understand why they can set zstd and cannot
> set zlib as compression type and to really set zlib they have to set no
> compression type to make the default zlib to apply.
>
> When a user set zlib as compression type the image is created as before
> the extension header were introduced.
>
> Reasonable?
> >
> >>> 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.
> 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).
Kevin
On 28.06.2019 17:24, Kevin Wolf wrote:
> Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
>>
>>
>> On 28.06.2019 15:06, Kevin Wolf wrote:
>>> 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 <dplotnikov@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?
>>>
>>> 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?
>> I think yes, if we create image with the extension set to zlib we
>> prevent an older qemu from using that image. Furthermore, to allow older
>> qemu using such images we need to create special conversion procedure
>> which has to remove the extension header.
>>
>> If zlib is a "special compression type" which is always set by default
>> without the extension header we'll get rid of such image conversion
>> procedure and an older qemu could use it "as is"
>>
>> Might it work as a good reason?
>>
>>> 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.
>> Yes, it's true
>>
>> The only reasons that zlib compression type even exists in the
>> enumeration is to avoid ambiguity for users.
>> For them it may be hard to understand why they can set zstd and cannot
>> set zlib as compression type and to really set zlib they have to set no
>> compression type to make the default zlib to apply.
>>
>> When a user set zlib as compression type the image is created as before
>> the extension header were introduced.
>>
>> Reasonable?
>>>
>>>>> 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.
>> 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?
Denis
>
> Kevin
>
--
Best,
Denis
Am 28.06.2019 um 16:40 hat Denis Plotnikov geschrieben:
>
>
> On 28.06.2019 17:24, Kevin Wolf wrote:
> > Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
> >>
> >>
> >> On 28.06.2019 15:06, Kevin Wolf wrote:
> >>> 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 <dplotnikov@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?
> >>>
> >>> 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?
> >> I think yes, if we create image with the extension set to zlib we
> >> prevent an older qemu from using that image. Furthermore, to allow older
> >> qemu using such images we need to create special conversion procedure
> >> which has to remove the extension header.
> >>
> >> If zlib is a "special compression type" which is always set by default
> >> without the extension header we'll get rid of such image conversion
> >> procedure and an older qemu could use it "as is"
> >>
> >> Might it work as a good reason?
> >>
> >>> 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.
> >> Yes, it's true
> >>
> >> The only reasons that zlib compression type even exists in the
> >> enumeration is to avoid ambiguity for users.
> >> For them it may be hard to understand why they can set zstd and cannot
> >> set zlib as compression type and to really set zlib they have to set no
> >> compression type to make the default zlib to apply.
> >>
> >> When a user set zlib as compression type the image is created as before
> >> the extension header were introduced.
> >>
> >> Reasonable?
> >>>
> >>>>> 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.
> >> 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.
Kevin
On 28.06.19 16:54, Kevin Wolf wrote:
> Am 28.06.2019 um 16:40 hat Denis Plotnikov geschrieben:
>>
>>
>> On 28.06.2019 17:24, Kevin Wolf wrote:
>>> Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
>>>>
>>>>
>>>> On 28.06.2019 15:06, Kevin Wolf wrote:
>>>>> 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 <dplotnikov@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?
>>>>>
>>>>> 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?
>>>> I think yes, if we create image with the extension set to zlib we
>>>> prevent an older qemu from using that image. Furthermore, to allow older
>>>> qemu using such images we need to create special conversion procedure
>>>> which has to remove the extension header.
>>>>
>>>> If zlib is a "special compression type" which is always set by default
>>>> without the extension header we'll get rid of such image conversion
>>>> procedure and an older qemu could use it "as is"
>>>>
>>>> Might it work as a good reason?
>>>>
>>>>> 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.
>>>> Yes, it's true
>>>>
>>>> The only reasons that zlib compression type even exists in the
>>>> enumeration is to avoid ambiguity for users.
>>>> For them it may be hard to understand why they can set zstd and cannot
>>>> set zlib as compression type and to really set zlib they have to set no
>>>> compression type to make the default zlib to apply.
>>>>
>>>> When a user set zlib as compression type the image is created as before
>>>> the extension header were introduced.
>>>>
>>>> Reasonable?
>>>>>
>>>>>>> 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.
>>>> 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.
I don’t think it’s any better to completely forbid it than to just
recommend in the spec that software should not set this field to zlib to
ensure backwards compatibility.
I see the point of forbidding it, but if I were to know nothing of qcow2
and read the spec, I guess I’d find it a bit weird to read “If this
field is not present, the compression type is zlib; if it is, it is not
zlib, but the specified value.” I’d ask myself why it isn’t simply “The
compression type is given by this field, it defaults to zlib.”
Max
On 28.06.2019 18:03, Max Reitz wrote:
> On 28.06.19 16:54, Kevin Wolf wrote:
>> Am 28.06.2019 um 16:40 hat Denis Plotnikov geschrieben:
>>>
>>>
>>> On 28.06.2019 17:24, Kevin Wolf wrote:
>>>> Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
>>>>>
>>>>>
>>>>> On 28.06.2019 15:06, Kevin Wolf wrote:
>>>>>> 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 <dplotnikov@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?
>>>>>>
>>>>>> 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?
>>>>> I think yes, if we create image with the extension set to zlib we
>>>>> prevent an older qemu from using that image. Furthermore, to allow older
>>>>> qemu using such images we need to create special conversion procedure
>>>>> which has to remove the extension header.
>>>>>
>>>>> If zlib is a "special compression type" which is always set by default
>>>>> without the extension header we'll get rid of such image conversion
>>>>> procedure and an older qemu could use it "as is"
>>>>>
>>>>> Might it work as a good reason?
>>>>>
>>>>>> 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.
>>>>> Yes, it's true
>>>>>
>>>>> The only reasons that zlib compression type even exists in the
>>>>> enumeration is to avoid ambiguity for users.
>>>>> For them it may be hard to understand why they can set zstd and cannot
>>>>> set zlib as compression type and to really set zlib they have to set no
>>>>> compression type to make the default zlib to apply.
>>>>>
>>>>> When a user set zlib as compression type the image is created as before
>>>>> the extension header were introduced.
>>>>>
>>>>> Reasonable?
>>>>>>
>>>>>>>> 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.
>>>>> 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.
>
> I don’t think it’s any better to completely forbid it than to just
> recommend in the spec that software should not set this field to zlib to
> ensure backwards compatibility.
>
> I see the point of forbidding it, but if I were to know nothing of qcow2
> and read the spec, I guess I’d find it a bit weird to read “If this
> field is not present, the compression type is zlib; if it is, it is not
> zlib, but the specified value.” I’d ask myself why it isn’t simply “The
> compression type is given by this field, it defaults to zlib.”
>
As I understood, if we go with the header extending we'll add a new
field in the header anyway which always hold some value. By, default it
will be zlib.
So, the only question now is whether we allow zlib value with the
incompatible feature flag set. My point is if compression type is zlib
the corresponding incompatible feature flag should NOT be set.
In that case an older qemu will put the compression type to
s->unknown_header_fields and run with zlib, otherwise it couldn't open
the file.
Denis
> Max
>
--
Best,
Denis
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
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
© 2016 - 2026 Red Hat, Inc.