Re: [PATCH v1 1/8] qcow2: introduce compression type feature
On 27.02.2020 16:48, Eric Blake wrote:
On 2/27/20 1:29 AM, Denis Plotnikov wrote:
The patch adds some preparation parts for incompatible compression type
feature to Qcow2 that indicates which allow to use different compression
to qcow2, allowing the use of different
methods for image clusters (de)compressing.
It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.
The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression
than ZLIB.
The default compression is ZLIB. Images created with ZLIB compression
type
are backward compatible with older qemu versions.
Signed-off-by: Denis Plotnikov
---
block/qcow2.c | 105 ++
block/qcow2.h | 31 ---
include/block/block_int.h | 1 +
qapi/block-core.json | 22 +++-
4 files changed, 150 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..2ccb2cabd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1242,6 +1242,50 @@ static int
qcow2_update_options(BlockDriverState *bs, QDict *options,
return ret;
}
+static int validate_compression_type(BDRVQcow2State *s, Error **errp)
+{
+ /*
+ * Sanity check
+ * according to qcow2 spec, the compression type is 1-byte field
+ * but in BDRVQcow2State the compression_type is enum sizeof(int)
+ * so, the max compression_type value is 255.
+ */
+ if (s->compression_type > 0xff) {
+ error_setg(errp, "qcow2: compression type value is too big");
+ return -EINVAL;
+ }
Hmm - I think it may be worth a tweak to qcow2.txt to call out:
104: compression_type
105 - 111: padding, must be 0
or else call out:
104-111: compression type
and just blindly use all 8 bytes for the value even though really only
1 or two values will ever be defined. Of course, that moves the byte
in question from 104 to 111, thanks to our big endian encoding, but as
this series is the first one installing a non-zero value in those 8
bytes, and as we just finished documenting that the header length must
be a multiple of 8, there is no real impact - we can make such tweaks
up until the 5.0 release.
+
+ switch (s->compression_type) {
+ case QCOW2_COMPRESSION_TYPE_ZLIB:
+ break;
+
+ default:
+ error_setg(errp, "qcow2: unknown compression type: %u",
+ s->compression_type);
+ return -ENOTSUP;
+ }
Having two checks feels redundant, compared to just letting the
default catch all unrecognized values in that field.
Looks like it is.
+
+ /*
+ * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
+ * the incompatible feature flag must be set
+ */
+ if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+ if (s->incompatible_features &
QCOW2_INCOMPAT_COMPRESSION_TYPE) {
+ error_setg(errp, "qcow2: Compression type incompatible
feature "
+ "bit must not be set");
+ return -EINVAL;
+ }
+ } else {
+ if (!(s->incompatible_features &
QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+ error_setg(errp, "qcow2: Compression type incompatible
feature "
+ "bit must be set");
+ return -EINVAL;
+ }
+ }
Matches what we documented in the spec.
+
+ return 0;
+}
+
/* Called with s->lock held. */
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict
*options,
int flags, Error **errp)
@@ -1357,6 +1401,26 @@ static int coroutine_fn
qcow2_do_open(BlockDriverState *bs, QDict *options,
s->compatible_features = header.compatible_features;
s->autoclear_features = header.autoclear_features;
+ /*
+ * Handle compression type
+ * Older qcow2 images don't contain the compression type header.
+ * Distinguish them by the header length and use
+ * the only valid (default) compression type in that case
+ */
+ if (header.header_length > offsetof(QCowHeader,
compression_type)) {
+ /*
+ * don't deal with endians since compression_type is 1 byte
long
+ */
+ s->compression_type = header.compression_type;
Changes if you go with my suggestion of just making the
compression_type field occupy 8 bytes in the qcow2 header. (And if
you want to keep it 1 byte, I still think the spec should call out
explicit padding bytes).
+ } else {
+ s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+ }
+
+ ret = validate_compression_type(s, errp);
+ if (ret) {
+ goto fail;
+ }
+
if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
void *feature_table = NULL;
Re: [PATCH v1 1/8] qcow2: introduce compression type feature
On 2/27/20 8:30 AM, Vladimir Sementsov-Ogievskiy wrote: But what is the benefit? We have already documented padding in the spec, and discussed it so much time... What is the problem with padding? And why to add 8 bytes for every new feature which needs only one byte? Okay, so requiring an 8-byte field is not necessary. But still, at least mentioning padding bytes (that may be assigned meanings later) is consistent with the rest of the document (for example, we have padding bits documented for the compatible/incompatible/autoclear feature bits), and reminds implementers to keep size rounded to a multiple of 8. Yes, we can add something about it.. But I'm not sure we need, and I can't imaging correct short wording. We have section about padding: " === Header padding === @header_length must be a multiple of 8, which means that if the end of the last additional field is not aligned, some padding is needed. This padding must be zeroed, so that if some existing (or future) additional field will fall into the padding, it will be interpreted accordingly to point [3.] of the previous paragraph, i.e. in the same manner as when this field is not present. " So, if we want to add something about 104-111, it should be added to this section, not to previous "=== Additional fields (version 3 and higher) ===". And, if we want, to add something, we should consider both cases when compression type field exists and when not... What to write? "105 - 111: These bytes are padding, if header length > 104. May be turned into new additional fields in future." Sounds a bit strange... Keeping in mind that different versions of qemu may consider the same bytes as additional field or as padding, and it is correct. Looking at the header extension, it can probably be as simple as: 105 - m: Zero padding to round up the header size to the next multiple of 8. I guess I'll propose a patch to make my idea concrete. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v1 1/8] qcow2: introduce compression type feature
27.02.2020 17:13, Eric Blake wrote: On 2/27/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: Hmm - I think it may be worth a tweak to qcow2.txt to call out: 104: compression_type 105 - 111: padding, must be 0 or else call out: 104-111: compression type and just blindly use all 8 bytes for the value even though really only 1 or two values will ever be defined. Of course, that moves the byte in question from 104 to 111, thanks to our big endian encoding, but as this series is the first one installing a non-zero value in those 8 bytes, and as we just finished documenting that the header length must be a multiple of 8, there is no real impact - we can make such tweaks up until the 5.0 release. But what is the benefit? We have already documented padding in the spec, and discussed it so much time... What is the problem with padding? And why to add 8 bytes for every new feature which needs only one byte? Okay, so requiring an 8-byte field is not necessary. But still, at least mentioning padding bytes (that may be assigned meanings later) is consistent with the rest of the document (for example, we have padding bits documented for the compatible/incompatible/autoclear feature bits), and reminds implementers to keep size rounded to a multiple of 8. Yes, we can add something about it.. But I'm not sure we need, and I can't imaging correct short wording. We have section about padding: " === Header padding === @header_length must be a multiple of 8, which means that if the end of the last additional field is not aligned, some padding is needed. This padding must be zeroed, so that if some existing (or future) additional field will fall into the padding, it will be interpreted accordingly to point [3.] of the previous paragraph, i.e. in the same manner as when this field is not present. " So, if we want to add something about 104-111, it should be added to this section, not to previous "=== Additional fields (version 3 and higher) ===". And, if we want, to add something, we should consider both cases when compression type field exists and when not... What to write? "105 - 111: These bytes are padding, if header length > 104. May be turned into new additional fields in future." Sounds a bit strange... Keeping in mind that different versions of qemu may consider the same bytes as additional field or as padding, and it is correct. -- Best regards, Vladimir
Re: [PATCH v1 1/8] qcow2: introduce compression type feature
On 2/27/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: Hmm - I think it may be worth a tweak to qcow2.txt to call out: 104: compression_type 105 - 111: padding, must be 0 or else call out: 104-111: compression type and just blindly use all 8 bytes for the value even though really only 1 or two values will ever be defined. Of course, that moves the byte in question from 104 to 111, thanks to our big endian encoding, but as this series is the first one installing a non-zero value in those 8 bytes, and as we just finished documenting that the header length must be a multiple of 8, there is no real impact - we can make such tweaks up until the 5.0 release. But what is the benefit? We have already documented padding in the spec, and discussed it so much time... What is the problem with padding? And why to add 8 bytes for every new feature which needs only one byte? Okay, so requiring an 8-byte field is not necessary. But still, at least mentioning padding bytes (that may be assigned meanings later) is consistent with the rest of the document (for example, we have padding bits documented for the compatible/incompatible/autoclear feature bits), and reminds implementers to keep size rounded to a multiple of 8. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v1 1/8] qcow2: introduce compression type feature
27.02.2020 16:48, Eric Blake wrote:
On 2/27/20 1:29 AM, Denis Plotnikov wrote:
The patch adds some preparation parts for incompatible compression type
feature to Qcow2 that indicates which allow to use different compression
to qcow2, allowing the use of different
methods for image clusters (de)compressing.
It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.
The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.
Signed-off-by: Denis Plotnikov
---
block/qcow2.c | 105 ++
block/qcow2.h | 31 ---
include/block/block_int.h | 1 +
qapi/block-core.json | 22 +++-
4 files changed, 150 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..2ccb2cabd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs,
QDict *options,
return ret;
}
+static int validate_compression_type(BDRVQcow2State *s, Error **errp)
+{
+ /*
+ * Sanity check
+ * according to qcow2 spec, the compression type is 1-byte field
+ * but in BDRVQcow2State the compression_type is enum sizeof(int)
+ * so, the max compression_type value is 255.
+ */
+ if (s->compression_type > 0xff) {
+ error_setg(errp, "qcow2: compression type value is too big");
+ return -EINVAL;
+ }
Hmm - I think it may be worth a tweak to qcow2.txt to call out:
104: compression_type
105 - 111: padding, must be 0
or else call out:
104-111: compression type
and just blindly use all 8 bytes for the value even though really only 1 or two
values will ever be defined. Of course, that moves the byte in question from
104 to 111, thanks to our big endian encoding, but as this series is the first
one installing a non-zero value in those 8 bytes, and as we just finished
documenting that the header length must be a multiple of 8, there is no real
impact - we can make such tweaks up until the 5.0 release.
But what is the benefit? We have already documented padding in the spec, and
discussed it so much time... What is the problem with padding? And why to add 8
bytes for every new feature which needs only one byte?
+
+ switch (s->compression_type) {
+ case QCOW2_COMPRESSION_TYPE_ZLIB:
+ break;
+
+ default:
+ error_setg(errp, "qcow2: unknown compression type: %u",
+ s->compression_type);
+ return -ENOTSUP;
+ }
Having two checks feels redundant, compared to just letting the default catch
all unrecognized values in that field.
+
+ /*
+ * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
+ * the incompatible feature flag must be set
+ */
+ if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+ if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
+ error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must not be set");
+ return -EINVAL;
+ }
+ } else {
+ if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+ error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must be set");
+ return -EINVAL;
+ }
+ }
Matches what we documented in the spec.
+
+ return 0;
+}
+
/* Called with s->lock held. */
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
@@ -1357,6 +1401,26 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
s->compatible_features = header.compatible_features;
s->autoclear_features = header.autoclear_features;
+ /*
+ * Handle compression type
+ * Older qcow2 images don't contain the compression type header.
+ * Distinguish them by the header length and use
+ * the only valid (default) compression type in that case
+ */
+ if (header.header_length > offsetof(QCowHeader, compression_type)) {
+ /*
+ * don't deal with endians since compression_type is 1 byte long
+ */
+ s->compression_type = header.compression_type;
Changes if you go with my suggestion of just making the compression_type field
occupy 8 bytes in the qcow2 header. (And if you want to keep it 1 byte, I
still think the spec should call out explicit padding bytes).
+ } else {
+ s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+ }
+
+ ret =
Re: [PATCH v1 1/8] qcow2: introduce compression type feature
On 2/27/20 1:29 AM, Denis Plotnikov wrote:
The patch adds some preparation parts for incompatible compression type
feature to Qcow2 that indicates which allow to use different compression
to qcow2, allowing the use of different
methods for image clusters (de)compressing.
It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.
The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.
Signed-off-by: Denis Plotnikov
---
block/qcow2.c | 105 ++
block/qcow2.h | 31 ---
include/block/block_int.h | 1 +
qapi/block-core.json | 22 +++-
4 files changed, 150 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..2ccb2cabd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs,
QDict *options,
return ret;
}
+static int validate_compression_type(BDRVQcow2State *s, Error **errp)
+{
+/*
+ * Sanity check
+ * according to qcow2 spec, the compression type is 1-byte field
+ * but in BDRVQcow2State the compression_type is enum sizeof(int)
+ * so, the max compression_type value is 255.
+ */
+if (s->compression_type > 0xff) {
+error_setg(errp, "qcow2: compression type value is too big");
+return -EINVAL;
+}
Hmm - I think it may be worth a tweak to qcow2.txt to call out:
104: compression_type
105 - 111: padding, must be 0
or else call out:
104-111: compression type
and just blindly use all 8 bytes for the value even though really only 1
or two values will ever be defined. Of course, that moves the byte in
question from 104 to 111, thanks to our big endian encoding, but as this
series is the first one installing a non-zero value in those 8 bytes,
and as we just finished documenting that the header length must be a
multiple of 8, there is no real impact - we can make such tweaks up
until the 5.0 release.
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+break;
+
+default:
+error_setg(errp, "qcow2: unknown compression type: %u",
+ s->compression_type);
+return -ENOTSUP;
+}
Having two checks feels redundant, compared to just letting the default
catch all unrecognized values in that field.
+
+/*
+ * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
+ * the incompatible feature flag must be set
+ */
+if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
+error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must not be set");
+return -EINVAL;
+}
+} else {
+if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must be set");
+return -EINVAL;
+}
+}
Matches what we documented in the spec.
+
+return 0;
+}
+
/* Called with s->lock held. */
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
@@ -1357,6 +1401,26 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
s->compatible_features = header.compatible_features;
s->autoclear_features = header.autoclear_features;
+/*
+ * Handle compression type
+ * Older qcow2 images don't contain the compression type header.
+ * Distinguish them by the header length and use
+ * the only valid (default) compression type in that case
+ */
+if (header.header_length > offsetof(QCowHeader, compression_type)) {
+/*
+ * don't deal with endians since compression_type is 1 byte long
+ */
+s->compression_type = header.compression_type;
Changes if you go with my suggestion of just making the compression_type
field occupy 8 bytes in the qcow2 header. (And if you want to keep it 1
byte, I still think the spec should call out explicit padding bytes).
+} else {
+s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+}
+
+ret = validate_compression_type(s, errp);
+if (ret) {
+goto fail;
+}
+
if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
void *feature_table = NULL;
qcow2_read_extensions(bs, header.header_length, ext_end,
@@ -2720,6
Re: [PATCH v1 1/8] qcow2: introduce compression type feature
On 2/27/20 2:21 AM, Vladimir Sementsov-Ogievskiy wrote: 27.02.2020 10:29, Denis Plotnikov wrote: The patch adds some preparation parts for incompatible compression type feature to Qcow2 that indicates which allow to use different compression methods for image clusters (de)compressing. It is implied that the compression type is set on the image creation and can be changed only later by image conversion, thus compression type defines the only compression algorithm used for the image, and thus, for all image clusters. The goal of the feature is to add support of other compression methods to qcow2. For example, ZSTD which is more effective on compression than ZLIB. The default compression is ZLIB. Images created with ZLIB compression type are backward compatible with older qemu versions. Signed-off-by: Denis Plotnikov --- block/qcow2.c | 105 ++ block/qcow2.h | 31 --- include/block/block_int.h | 1 + qapi/block-core.json | 22 +++- 4 files changed, 150 insertions(+), 9 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 3c754f616b..2ccb2cabd1 100644 --- a/block/qcow2.c +++ b/block/qcow2.c Please, add to .git/config: [diff] orderFile = /path/to/qemu/scripts/git.orderfile This will force git format-patch to sort files in more comfortable order (header changes first, etc). As I learned yesterday, git 2.23 and 2.24 have a bug where git format-patch fails to honor diff.orderfile (fixed in 2.25): https://bugzilla.redhat.com/show_bug.cgi?id=1807681 (and that explains why some of my recent patches have not been ordered the way I wanted, as Fedora 31 currently has a git from the broken window in time) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v1 1/8] qcow2: introduce compression type feature
27.02.2020 10:29, Denis Plotnikov wrote:
The patch adds some preparation parts for incompatible compression type
feature to Qcow2 that indicates which allow to use different compression
methods for image clusters (de)compressing.
It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.
The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.
Signed-off-by: Denis Plotnikov
---
block/qcow2.c | 105 ++
block/qcow2.h | 31 ---
include/block/block_int.h | 1 +
qapi/block-core.json | 22 +++-
4 files changed, 150 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..2ccb2cabd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
Please, add to .git/config:
[diff]
orderFile = /path/to/qemu/scripts/git.orderfile
This will force git format-patch to sort files in more comfortable order
(header changes first, etc).
@@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs,
QDict *options,
return ret;
}
+static int validate_compression_type(BDRVQcow2State *s, Error **errp)
+{
+/*
+ * Sanity check
+ * according to qcow2 spec, the compression type is 1-byte field
+ * but in BDRVQcow2State the compression_type is enum sizeof(int)
+ * so, the max compression_type value is 255.
+ */
+if (s->compression_type > 0xff) {
+error_setg(errp, "qcow2: compression type value is too big");
+return -EINVAL;
+}
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+break;
+
+default:
+error_setg(errp, "qcow2: unknown compression type: %u",
+ s->compression_type);
+return -ENOTSUP;
+}
honestly, I think that just
if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {
error out
}
is enough, and don't see why to check > 0xff in separate..
But it works as is.
+
+/*
+ * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
+ * the incompatible feature flag must be set
+ */
+if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
+error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must not be set");
+return -EINVAL;
+}
+} else {
This is unreachable now.. But it's OK as preparation for further patches I
think.
+if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must be set");
+return -EINVAL;
+}
+}
+
+return 0;
+}
+
/* Called with s->lock held. */
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
@@ -1357,6 +1401,26 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
s->compatible_features = header.compatible_features;
s->autoclear_features = header.autoclear_features;
+/*
+ * Handle compression type
+ * Older qcow2 images don't contain the compression type header.
+ * Distinguish them by the header length and use
+ * the only valid (default) compression type in that case
+ */
+if (header.header_length > offsetof(QCowHeader, compression_type)) {
+/*
+ * don't deal with endians since compression_type is 1 byte long
+ */
this comment would be more appropriate above, where be-to-cpu transformation is
done,
as actually previous "s->autoclear_features = header.autoclear_features"
doesn't
deal with endians too.
+s->compression_type = header.compression_type;
+} else {
+s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+}
+
+ret = validate_compression_type(s, errp);
+if (ret) {
+goto fail;
+}
+
if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
void *feature_table = NULL;
qcow2_read_extensions(bs, header.header_length, ext_end,
@@ -2720,6 +2784,12 @@ int qcow2_update_header(BlockDriverState *bs)
total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
+ret = validate_compression_type(s, NULL);
+
+if (ret) {
+goto fail;
+}
+
*header = (QCowHeader) {
/* Version 2 fields */
.magic
[PATCH v1 1/8] qcow2: introduce compression type feature
The patch adds some preparation parts for incompatible compression type
feature to Qcow2 that indicates which allow to use different compression
methods for image clusters (de)compressing.
It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.
The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.
Signed-off-by: Denis Plotnikov
---
block/qcow2.c | 105 ++
block/qcow2.h | 31 ---
include/block/block_int.h | 1 +
qapi/block-core.json | 22 +++-
4 files changed, 150 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..2ccb2cabd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs,
QDict *options,
return ret;
}
+static int validate_compression_type(BDRVQcow2State *s, Error **errp)
+{
+/*
+ * Sanity check
+ * according to qcow2 spec, the compression type is 1-byte field
+ * but in BDRVQcow2State the compression_type is enum sizeof(int)
+ * so, the max compression_type value is 255.
+ */
+if (s->compression_type > 0xff) {
+error_setg(errp, "qcow2: compression type value is too big");
+return -EINVAL;
+}
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+break;
+
+default:
+error_setg(errp, "qcow2: unknown compression type: %u",
+ s->compression_type);
+return -ENOTSUP;
+}
+
+/*
+ * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
+ * the incompatible feature flag must be set
+ */
+if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
+error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must not be set");
+return -EINVAL;
+}
+} else {
+if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must be set");
+return -EINVAL;
+}
+}
+
+return 0;
+}
+
/* Called with s->lock held. */
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
@@ -1357,6 +1401,26 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
s->compatible_features = header.compatible_features;
s->autoclear_features = header.autoclear_features;
+/*
+ * Handle compression type
+ * Older qcow2 images don't contain the compression type header.
+ * Distinguish them by the header length and use
+ * the only valid (default) compression type in that case
+ */
+if (header.header_length > offsetof(QCowHeader, compression_type)) {
+/*
+ * don't deal with endians since compression_type is 1 byte long
+ */
+s->compression_type = header.compression_type;
+} else {
+s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+}
+
+ret = validate_compression_type(s, errp);
+if (ret) {
+goto fail;
+}
+
if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
void *feature_table = NULL;
qcow2_read_extensions(bs, header.header_length, ext_end,
@@ -2720,6 +2784,12 @@ int qcow2_update_header(BlockDriverState *bs)
total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
+ret = validate_compression_type(s, NULL);
+
+if (ret) {
+goto fail;
+}
+
*header = (QCowHeader) {
/* Version 2 fields */
.magic = cpu_to_be32(QCOW_MAGIC),
@@ -2742,6 +2812,7 @@ int qcow2_update_header(BlockDriverState *bs)
.autoclear_features = cpu_to_be64(s->autoclear_features),
.refcount_order = cpu_to_be32(s->refcount_order),
.header_length = cpu_to_be32(header_length),
+.compression_type = (uint8_t) s->compression_type,
};
/* For older versions, write a shorter header */
@@ -2839,6 +2910,11 @@ int qcow2_update_header(BlockDriverState *bs)
.bit = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
.name = "lazy refcounts",
},
+{
+.type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
+.bit =
