On 04.03.20 14:35, Denis Plotnikov wrote: > The patch adds some preparation parts for incompatible compression type > feature to qcow2 allowing the 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. > > Adding of the compression type breaks a number of tests because now the > compression type is reported on image creation and there are some changes > in the qcow2 header in size and offsets. > > The tests are fixed in the following ways: > * filter out compression_type for all the tests > * fix header size, feature table size and backing file offset > affected tests: 031, 036, 061, 080 > header_size +=8: 1 byte compression type > 7 bytes padding > feature_table += 48: incompatible feture compression type > backing_file_offset += 56 (8 + 48 -> header_change + > fature_table_change) > * add "compression type" for test output matching when it isn't filtered > affected tests: 049, 060, 061, 065, 144, 182, 242, 255 > > Signed-off-by: Denis Plotnikov <dplotni...@virtuozzo.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > qapi/block-core.json | 22 ++++++- > block/qcow2.h | 18 +++++- > include/block/block_int.h | 1 + > block/qcow2.c | 101 ++++++++++++++++++++++++++++++ > tests/qemu-iotests/031.out | 14 ++--- > tests/qemu-iotests/036.out | 4 +- > tests/qemu-iotests/049.out | 102 +++++++++++++++---------------- > tests/qemu-iotests/060.out | 1 + > tests/qemu-iotests/061.out | 34 ++++++----- > tests/qemu-iotests/065 | 28 ++++++--- > tests/qemu-iotests/080 | 2 +- > tests/qemu-iotests/144.out | 4 +- > tests/qemu-iotests/182.out | 2 +- > tests/qemu-iotests/242.out | 5 ++ > tests/qemu-iotests/255.out | 8 +-- > tests/qemu-iotests/common.filter | 3 +- > 16 files changed, 253 insertions(+), 96 deletions(-)
Looks good, just mostly nit picks: > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 85e27bb61f..a67eb8bff4 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json [...] > @@ -4392,6 +4395,18 @@ > 'data': [ 'v2', 'v3' ] } > > > +## > +# @Qcow2CompressionType: As far as I can see, all other types beginning with /@qcow2/i use the same capitalization, so it should be the correct way. > +# > +# Compression type used in qcow2 image file > +# > +# @zlib: zlib compression, see <http://zlib.net/> Are the two spaces after the colon intentional? [...] > diff --git a/block/qcow2.h b/block/qcow2.h > index 0942126232..485effcb70 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -146,6 +146,12 @@ typedef struct QCowHeader { > > uint32_t refcount_order; > uint32_t header_length; > + > + /* Additional fields */ > + uint8_t compression_type; > + > + /* header must be a multiple of 8 */ I like Berto’s idea of adding a static assertion for that. > + uint8_t padding[7]; > } QEMU_PACKED QCowHeader; > > typedef struct QEMU_PACKED QCowSnapshotHeader { [...] > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 6f9fd5e20e..2c6bb9dc99 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h [...] > @@ -2720,6 +2770,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); > + Why this empty line? > + if (ret) { > + goto fail; > + } > + > *header = (QCowHeader) { > /* Version 2 fields */ > .magic = cpu_to_be32(QCOW_MAGIC), [...] > @@ -5248,6 +5340,9 @@ static int qcow2_amend_options(BlockDriverState *bs, > QemuOpts *opts, > "images"); > return -EINVAL; > } > + } else if (!strcmp(desc->name, BLOCK_OPT_COMPRESSION_TYPE)) { > + error_setg(errp, "Changing the compression type is not > supported"); > + return -ENOTSUP; Most other branches check whether it’s really a change (unless it’d be unreasonably complicated to do so), so maybe we should do the same here. Max > } else { > /* if this point is reached, this probably means a new option was > * added without having it covered here */
signature.asc
Description: OpenPGP digital signature
