From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Make it more obvious how to add new fields to the version 3 header and

how to interpret them.

The specification is adjusted so that for new defined optional fields:

1. Software may support some of these optional fields and ignore the

others, which means that features may be backported to downstream

Qemu independently.

2. If we want to add incompatible field (or a field, for which some of

its values would be incompatible), it must be accompanied by

incompatible feature bit.

Also the concept of "default is zero" is clarified, as it's strange to

say that the value of the field is assumed to be zero for the software

version which don't know about the field at all and don't know how to

treat it be it zero or not.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

Message-Id: <20200131142219.3264-2-vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

[mreitz: s/some its/some of its/]

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

docs/interop/qcow2.txt | 45 +++++++++++++++++++++++++++++++++++++++---

1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt

index XXXXXXX..XXXXXXX 100644

--- a/docs/interop/qcow2.txt

+++ b/docs/interop/qcow2.txt

@@ -XXX,XX +XXX,XX @@ The first cluster of a qcow2 image contains the file header:

Offset into the image file at which the snapshot table

starts. Must be aligned to a cluster boundary.

-If the version is 3 or higher, the header has the following additional fields.

-For version 2, the values are assumed to be zero, unless specified otherwise

-in the description of a field.

+For version 2, the header is exactly 72 bytes in length, and finishes here.

+For version 3 or higher, the header length is at least 104 bytes, including

+the next fields through header_length.

72 - 79: incompatible_features

Bitmask of incompatible features. An implementation must

@@ -XXX,XX +XXX,XX @@ in the description of a field.

100 - 103: header_length

Length of the header structure in bytes. For version 2

images, the length is always assumed to be 72 bytes.

+ For version 3 it's at least 104 bytes and must be a multiple

+ of 8.

+

+

+=== Additional fields (version 3 and higher) ===

+

+In general, these fields are optional and may be safely ignored by the software,

+as well as filled by zeros (which is equal to field absence), if software needs

+to set field B, but does not care about field A which precedes B. More

+formally, additional fields have the following compatibility rules:

+

+1. If the value of the additional field must not be ignored for correct

+handling of the file, it will be accompanied by a corresponding incompatible

+feature bit.

+

+2. If there are no unrecognized incompatible feature bits set, an unknown

+additional field may be safely ignored other than preserving its value when

+rewriting the image header.

+

+3. An explicit value of 0 will have the same behavior as when the field is not

+present*, if not altered by a specific incompatible bit.

+

+*. A field is considered not present when header_length is less than or equal

+to the field's offset. Also, all additional fields are not present for

+version 2.

+

+ < ... No additional fields in the header currently ... >

+

+

+=== 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.

+

+

+=== Header extensions ===

Directly after the image header, optional sections called header extensions can

be stored. Each extension has a structure like the following:

--

2.24.1

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

The patch adds a new additional field to the qcow2 header: compression_type,

which specifies compression type. If field is absent or zero, default

compression type is set: ZLIB, which corresponds to current behavior.

New compression type (ZSTD) is to be added in further commit.

Suggested-by: Denis Plotnikov <dplotnikov@virtuozzo.com>

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20200131142219.3264-3-vsementsov@virtuozzo.com>

[mreitz: s/Bits 3-63: Reserved/Bits 4-63: Reserved/]

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

docs/interop/qcow2.txt | 21 +++++++++++++++++++--

1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt

index XXXXXXX..XXXXXXX 100644

--- a/docs/interop/qcow2.txt

+++ b/docs/interop/qcow2.txt

@@ -XXX,XX +XXX,XX @@ the next fields through header_length.

An External Data File Name header extension may

be present if this bit is set.

- Bits 3-63: Reserved (set to 0)

+ Bit 3: Compression type bit. If this bit is set,

+ a non-default compression is used for compressed

+ clusters. The compression_type field must be

+ present and not zero.

+

+ Bits 4-63: Reserved (set to 0)

80 - 87: compatible_features

Bitmask of compatible features. An implementation can

@@ -XXX,XX +XXX,XX @@ present*, if not altered by a specific incompatible bit.

to the field's offset. Also, all additional fields are not present for

version 2.

- < ... No additional fields in the header currently ... >

+ 104: compression_type

+

+ Defines the compression method used for compressed clusters.

+ All compressed clusters in an image use the same compression

+ type.

+

+ If the incompatible bit "Compression type" is set: the field

+ must be present and non-zero (which means non-zlib

+ compression type). Otherwise, this field must not be present

+ or must be zero (which means zlib).

+

+ Available compression type values:

+ 0: zlib <https://www.zlib.net/>

=== Header padding ===

--

2.24.1

From: Thomas Huth <thuth@redhat.com>

Commit d9df28e7b07 ("iotests: check whitelisted formats") added the

modern @iotests.skip_if_unsupported() to the functions in this test,

so we don't need the old explicit test here anymore.

Signed-off-by: Thomas Huth <thuth@redhat.com>

Message-Id: <20200129141751.32652-1-thuth@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

Tested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

tests/qemu-iotests/139 | 3 ---

1 file changed, 3 deletions(-)

diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/139

+++ b/tests/qemu-iotests/139

@@ -XXX,XX +XXX,XX @@ class TestBlockdevDel(iotests.QMPTestCase):

@iotests.skip_if_unsupported(['quorum'])

def testQuorum(self):

- if not iotests.supports_quorum():

- return

-

self.addQuorum('quorum0', 'node0', 'node1')

# We cannot remove the children of a Quorum device

self.delBlockDriverState('node0', expect_error = True)

--

2.24.1

8dff69b94 added an aio parameter to the drive parameter but forgot to

add a comma before, thus breaking the test. Fix it again.

Fixes: 8dff69b9415b4287e900358744b732195e1ab2e2

Signed-off-by: Max Reitz <mreitz@redhat.com>

Message-Id: <20200206130812.612960-1-mreitz@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

Tested-by: Eric Blake <eblake@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

tests/qemu-iotests/147 | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/147

+++ b/tests/qemu-iotests/147

@@ -XXX,XX +XXX,XX @@ class BuiltinNBD(NBDBlockdevAddBase):

self.server.add_drive_raw('if=none,id=nbd-export,' +

'file=%s,' % test_img +

'format=%s,' % imgfmt +

- 'cache=%s' % cachemode +

+ 'cache=%s,' % cachemode +

'aio=%s' % aiomode)

self.server.launch()

--

2.24.1

From: Peter Krempa <pkrempa@redhat.com>

When a management application manages node names there's no reason to

recurse into backing images in the output of query-named-block-nodes.

Add a parameter to the command which will return just the top level

structs.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>

Message-Id: <4470f8c779abc404dcf65e375db195cd91a80651.1579509782.git.pkrempa@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

[mreitz: Fixed coding style]

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

block.c | 5 +++--

block/qapi.c | 11 +++++++++--

blockdev.c | 8 ++++++--

include/block/block.h | 2 +-

include/block/qapi.h | 4 +++-

monitor/hmp-cmds.c | 2 +-

qapi/block-core.json | 7 ++++++-

7 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_find_node(const char *node_name)

}

/* Put this QMP function here so it can access the static graph_bdrv_states. */

-BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp)

+BlockDeviceInfoList *bdrv_named_nodes_list(bool flat,

+ Error **errp)

{

BlockDeviceInfoList *list, *entry;

BlockDriverState *bs;

list = NULL;

QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {

- BlockDeviceInfo *info = bdrv_block_device_info(NULL, bs, errp);

+ BlockDeviceInfo *info = bdrv_block_device_info(NULL, bs, flat, errp);

if (!info) {

qapi_free_BlockDeviceInfoList(list);

return NULL;

diff --git a/block/qapi.c b/block/qapi.c

index XXXXXXX..XXXXXXX 100644

--- a/block/qapi.c

+++ b/block/qapi.c

@@ -XXX,XX +XXX,XX @@

#include "qemu/cutils.h"

BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,

- BlockDriverState *bs, Error **errp)

+ BlockDriverState *bs,

+ bool flat,

+ Error **errp)

{

ImageInfo **p_image_info;

BlockDriverState *bs0;

@@ -XXX,XX +XXX,XX @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,

return NULL;

}

+ /* stop gathering data for flat output */

+ if (flat) {

+ break;

+ }

+

if (bs0->drv && bs0->backing) {

info->backing_file_depth++;

bs0 = bs0->backing->bs;

@@ -XXX,XX +XXX,XX @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,

if (bs && bs->drv) {

info->has_inserted = true;

- info->inserted = bdrv_block_device_info(blk, bs, errp);

+ info->inserted = bdrv_block_device_info(blk, bs, false, errp);

if (info->inserted == NULL) {

goto err;

}

diff --git a/blockdev.c b/blockdev.c

index XXXXXXX..XXXXXXX 100644

--- a/blockdev.c

+++ b/blockdev.c

@@ -XXX,XX +XXX,XX @@ void qmp_drive_backup(DriveBackup *backup, Error **errp)

blockdev_do_action(&action, errp);

}

-BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)

+BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat,

+ bool flat,

+ Error **errp)

{

- return bdrv_named_nodes_list(errp);

+ bool return_flat = has_flat && flat;

+

+ return bdrv_named_nodes_list(return_flat, errp);

}

XDbgBlockGraph *qmp_x_debug_query_block_graph(Error **errp)

diff --git a/include/block/block.h b/include/block/block.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/block.h

+++ b/include/block/block.h

@@ -XXX,XX +XXX,XX @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked);

void bdrv_eject(BlockDriverState *bs, bool eject_flag);

const char *bdrv_get_format_name(BlockDriverState *bs);

BlockDriverState *bdrv_find_node(const char *node_name);

-BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp);

+BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp);

XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp);

BlockDriverState *bdrv_lookup_bs(const char *device,

const char *node_name,

diff --git a/include/block/qapi.h b/include/block/qapi.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/qapi.h

+++ b/include/block/qapi.h

@@ -XXX,XX +XXX,XX @@

#include "block/snapshot.h"

BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,

- BlockDriverState *bs, Error **errp);

+ BlockDriverState *bs,

+ bool flat,

+ Error **errp);

int bdrv_query_snapshot_info_list(BlockDriverState *bs,

SnapshotInfoList **p_list,

Error **errp);

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c

index XXXXXXX..XXXXXXX 100644

--- a/monitor/hmp-cmds.c

+++ b/monitor/hmp-cmds.c

@@ -XXX,XX +XXX,XX @@ void hmp_info_block(Monitor *mon, const QDict *qdict)

}

/* Print node information */

- blockdev_list = qmp_query_named_block_nodes(NULL);

+ blockdev_list = qmp_query_named_block_nodes(false, false, NULL);

for (blockdev = blockdev_list; blockdev; blockdev = blockdev->next) {

assert(blockdev->value->has_node_name);

if (device && strcmp(device, blockdev->value->node_name)) {

diff --git a/qapi/block-core.json b/qapi/block-core.json

index XXXXXXX..XXXXXXX 100644

--- a/qapi/block-core.json

+++ b/qapi/block-core.json

@@ -XXX,XX +XXX,XX @@

#

# Get the named block driver list

#

+# @flat: Omit the nested data about backing image ("backing-image" key) if true.

+# Default is false (Since 5.0)

+#

# Returns: the list of BlockDeviceInfo

#

# Since: 2.0

@@ -XXX,XX +XXX,XX @@

# } } ] }

#

##

-{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }

+{ 'command': 'query-named-block-nodes',

+ 'returns': [ 'BlockDeviceInfo' ],

+ 'data': { '*flat': 'bool' } }

##

# @XDbgBlockGraphNodeType:

--

2.24.1

From: David Edmondson <david.edmondson@oracle.com>

In many cases the target of a convert operation is a newly provisioned

target that the user knows is blank (reads as zero). In this situation

there is no requirement for qemu-img to wastefully zero out the entire

device.

Add a new option, --target-is-zero, allowing the user to indicate that

an existing target device will return zeros for all reads.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>

Message-Id: <20200205110248.2009589-2-david.edmondson@oracle.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

docs/interop/qemu-img.rst | 9 ++++++++-

qemu-img-cmds.hx | 4 ++--

qemu-img.c | 26 +++++++++++++++++++++++---

3 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst

index XXXXXXX..XXXXXXX 100644

--- a/docs/interop/qemu-img.rst

+++ b/docs/interop/qemu-img.rst

@@ -XXX,XX +XXX,XX @@ Parameters to convert subcommand:

will still be printed. Areas that cannot be read from the source will be

treated as containing only zeroes.

+.. option:: --target-is-zero

+

+ Assume that reading the destination image will always return

+ zeros. This parameter is mutually exclusive with a destination image

+ that has a backing file. It is required to also use the ``-n``

+ parameter to skip image creation.

+

Parameters to dd subcommand:

.. program:: qemu-img-dd

@@ -XXX,XX +XXX,XX @@ Command description:

4

Error on reading data

-.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME

+.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME

Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*

to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx

index XXXXXXX..XXXXXXX 100644

--- a/qemu-img-cmds.hx

+++ b/qemu-img-cmds.hx

@@ -XXX,XX +XXX,XX @@ SRST

ERST

DEF("convert", img_convert,

- "convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")

+ "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")

SRST

-.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME

+.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME

ERST

DEF("create", img_create,

diff --git a/qemu-img.c b/qemu-img.c

index XXXXXXX..XXXXXXX 100644

--- a/qemu-img.c

+++ b/qemu-img.c

@@ -XXX,XX +XXX,XX @@ enum {

OPTION_PREALLOCATION = 265,

OPTION_SHRINK = 266,

OPTION_SALVAGE = 267,

+ OPTION_TARGET_IS_ZERO = 268,

};

typedef enum OutputFormat {

@@ -XXX,XX +XXX,XX @@ static int convert_do_copy(ImgConvertState *s)

int64_t sector_num = 0;

/* Check whether we have zero initialisation or can get it efficiently */

- if (s->target_is_new && s->min_sparse && !s->target_has_backing) {

+ if (!s->has_zero_init && s->target_is_new && s->min_sparse &&

+ !s->target_has_backing) {

s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));

- } else {

- s->has_zero_init = false;

}

if (!s->has_zero_init && !s->target_has_backing &&

@@ -XXX,XX +XXX,XX @@ static int img_convert(int argc, char **argv)

{"force-share", no_argument, 0, 'U'},

{"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},

{"salvage", no_argument, 0, OPTION_SALVAGE},

+ {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},

{0, 0, 0, 0}

};

c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",

@@ -XXX,XX +XXX,XX @@ static int img_convert(int argc, char **argv)

case OPTION_TARGET_IMAGE_OPTS:

tgt_image_opts = true;

break;

+ case OPTION_TARGET_IS_ZERO:

+ /*

+ * The user asserting that the target is blank has the

+ * same effect as the target driver supporting zero

+ * initialisation.

+ */

+ s.has_zero_init = true;

+ break;

}

}

@@ -XXX,XX +XXX,XX @@ static int img_convert(int argc, char **argv)

warn_report("This will become an error in future QEMU versions.");

}

+ if (s.has_zero_init && !skip_create) {

+ error_report("--target-is-zero requires use of -n flag");

+ goto fail_getopt;

+ }

+

s.src_num = argc - optind - 1;

out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;

@@ -XXX,XX +XXX,XX @@ static int img_convert(int argc, char **argv)

}

s.target_has_backing = (bool) out_baseimg;

+ if (s.has_zero_init && s.target_has_backing) {

+ error_report("Cannot use --target-is-zero when the destination "

+ "image has a backing file");

+ goto out;

+ }

+

if (s.src_num > 1 && out_baseimg) {

error_report("Having a backing file for the target makes no sense when "

"concatenating multiple input images");

--

2.24.1

From: Daniel P. Berrangé <berrange@redhat.com>

When initializing the LUKS header the size with default encryption

parameters will currently be 2068480 bytes. This is rounded up to

a multiple of the cluster size, 2081792, with 64k sectors. If the

end of the header is not the same as the end of the cluster we fill

the extra space with zeros. This was forgetting that not even the

space allocated for the header will be fully initialized, as we

only write key material for the first key slot. The space left

for the other 7 slots is never written to.

An optimization to the ref count checking code:

commit a5fff8d4b4d928311a5005efa12d0991fe3b66f9 (refs/bisect/bad)

Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Date: Wed Feb 27 16:14:30 2019 +0300

qcow2-refcount: avoid eating RAM

made the assumption that every cluster which was allocated would

have at least some data written to it. This was violated by way

the LUKS header is only partially written, with much space simply

reserved for future use.

Depending on the cluster size this problem was masked by the

logic which wrote zeros between the end of the LUKS header and

the end of the cluster.

$ qemu-img create --object secret,id=cluster_encrypt0,data=123456 \

-f qcow2 -o cluster_size=2k,encrypt.iter-time=1,\

encrypt.format=luks,encrypt.key-secret=cluster_encrypt0 \

cluster_size_check.qcow2 100M

Formatting 'cluster_size_check.qcow2', fmt=qcow2 size=104857600

encrypt.format=luks encrypt.key-secret=cluster_encrypt0

encrypt.iter-time=1 cluster_size=2048 lazy_refcounts=off refcount_bits=16

$ qemu-img check --object secret,id=cluster_encrypt0,data=redhat \

'json:{"driver": "qcow2", "encrypt.format": "luks", \

"encrypt.key-secret": "cluster_encrypt0", \

"file.driver": "file", "file.filename": "cluster_size_check.qcow2"}'

ERROR: counting reference for region exceeding the end of the file by one cluster or more: offset 0x2000 size 0x1f9000

Leaked cluster 4 refcount=1 reference=0

...snip...

Leaked cluster 130 refcount=1 reference=0

1 errors were found on the image.

Data may be corrupted, or further writes to the image may corrupt it.

127 leaked clusters were found on the image.

This means waste of disk space, but no harm to data.

Image end offset: 268288

The problem only exists when the disk image is entirely empty. Writing

data to the disk image payload will solve the problem by causing the

end of the file to be extended further.

The change fixes it by ensuring that the entire allocated LUKS header

region is fully initialized with zeros. The qemu-img check will still

fail for any pre-existing disk images created prior to this change,

unless at least 1 byte of the payload is written to.

Fully writing zeros to the entire LUKS header is a good idea regardless

as it ensures that space has been allocated on the host filesystem (or

whatever block storage backend is used).

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Message-Id: <20200207135520.2669430-1-berrange@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

block/qcow2.c | 11 +++--

tests/qemu-iotests/284 | 97 ++++++++++++++++++++++++++++++++++++++

tests/qemu-iotests/284.out | 62 ++++++++++++++++++++++++

tests/qemu-iotests/group | 1 +

4 files changed, 167 insertions(+), 4 deletions(-)

create mode 100755 tests/qemu-iotests/284

create mode 100644 tests/qemu-iotests/284.out

diff --git a/block/qcow2.c b/block/qcow2.c

index XXXXXXX..XXXXXXX 100644

--- a/block/qcow2.c

+++ b/block/qcow2.c

@@ -XXX,XX +XXX,XX @@ static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t headerlen,

s->crypto_header.length = headerlen;

s->crypto_header.offset = ret;

- /* Zero fill remaining space in cluster so it has predictable

- * content in case of future spec changes */

+ /*

+ * Zero fill all space in cluster so it has predictable

+ * content, as we may not initialize some regions of the

+ * header (eg only 1 out of 8 key slots will be initialized)

+ */

clusterlen = size_to_clusters(s, headerlen) * s->cluster_size;

assert(qcow2_pre_write_overlap_check(bs, 0, ret, clusterlen, false) == 0);

ret = bdrv_pwrite_zeroes(bs->file,

- ret + headerlen,

- clusterlen - headerlen, 0);

+ ret,

+ clusterlen, 0);

if (ret < 0) {

error_setg_errno(errp, -ret, "Could not zero fill encryption header");

return -1;

diff --git a/tests/qemu-iotests/284 b/tests/qemu-iotests/284

new file mode 100755

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/284

@@ -XXX,XX +XXX,XX @@

+#!/usr/bin/env bash

+#

+# Test ref count checks on encrypted images

+#

+# Copyright (C) 2019 Red Hat, Inc.

+#

+# This program is free software; you can redistribute it and/or modify

+# it under the terms of the GNU General Public License as published by

+# the Free Software Foundation; either version 2 of the License, or

+# (at your option) any later version.

+#

+# This program is distributed in the hope that it will be useful,

+# but WITHOUT ANY WARRANTY; without even the implied warranty of

+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

+# GNU General Public License for more details.

+#

+# You should have received a copy of the GNU General Public License

+# along with this program. If not, see <http://www.gnu.org/licenses/>.

+#

+

+# creator

+owner=berrange@redhat.com

+

+seq=`basename $0`

+echo "QA output created by $seq"

+

+status=1 # failure is the default!

+

+_cleanup()

+{

+ _cleanup_test_img

+}

+trap "_cleanup; exit \$status" 0 1 2 3 15

+

+# get standard environment, filters and checks

+. ./common.rc

+. ./common.filter

+

+_supported_fmt qcow2

+_supported_proto generic

+_supported_os Linux

+

+

+size=1M

+

+SECRET="secret,id=sec0,data=astrochicken"

+

+IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0"

+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT

+

+_run_test()

+{

+ IMGOPTSSYNTAX=true

+ OLD_TEST_IMG="$TEST_IMG"

+ TEST_IMG="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0"

+ QEMU_IMG_EXTRA_ARGS="--image-opts --object $SECRET"

+

+ echo

+ echo "== cluster size $csize"

+ echo "== checking image refcounts =="

+ _check_test_img

+

+ echo

+ echo "== writing some data =="

+ $QEMU_IO -c "write -P 0x9 0 1" $QEMU_IMG_EXTRA_ARGS $TEST_IMG | _filter_qemu_io | _filter_testdir

+ echo

+ echo "== rechecking image refcounts =="

+ _check_test_img

+

+ echo

+ echo "== writing some more data =="

+ $QEMU_IO -c "write -P 0x9 $csize 1" $QEMU_IMG_EXTRA_ARGS $TEST_IMG | _filter_qemu_io | _filter_testdir

+ echo

+ echo "== rechecking image refcounts =="

+ _check_test_img

+

+ TEST_IMG="$OLD_TEST_IMG"

+ QEMU_IMG_EXTRA_ARGS=

+ IMGOPTSSYNTAX=

+}

+

+

+echo

+echo "testing LUKS qcow2 encryption"

+echo

+

+for csize in 512 2048 32768

+do

+ _make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,cluster_size=$csize" $size

+ _run_test

+ _cleanup_test_img

+done

+

+# success, all done

+echo "*** done"

+rm -f $seq.full

+status=0

diff --git a/tests/qemu-iotests/284.out b/tests/qemu-iotests/284.out

new file mode 100644

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/284.out

@@ -XXX,XX +XXX,XX @@

+QA output created by 284

+

+testing LUKS qcow2 encryption

+

+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10

+

+== cluster size 512

+== checking image refcounts ==

+No errors were found on the image.

+

+== writing some data ==

+wrote 1/1 bytes at offset 0

+1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+== rechecking image refcounts ==

+No errors were found on the image.

+

+== writing some more data ==

+wrote 1/1 bytes at offset 512

+1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+== rechecking image refcounts ==

+No errors were found on the image.

+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10

+

+== cluster size 2048

+== checking image refcounts ==

+No errors were found on the image.

+

+== writing some data ==

+wrote 1/1 bytes at offset 0

+1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+== rechecking image refcounts ==

+No errors were found on the image.

+

+== writing some more data ==

+wrote 1/1 bytes at offset 2048

+1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+== rechecking image refcounts ==

+No errors were found on the image.

+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10

+

+== cluster size 32768

+== checking image refcounts ==

+No errors were found on the image.

+

+== writing some data ==

+wrote 1/1 bytes at offset 0

+1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+== rechecking image refcounts ==

+No errors were found on the image.

+

+== writing some more data ==

+wrote 1/1 bytes at offset 32768

+1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+== rechecking image refcounts ==

+No errors were found on the image.

+*** done

diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/group

+++ b/tests/qemu-iotests/group

@@ -XXX,XX +XXX,XX @@

280 rw migration quick

281 rw quick

283 auto quick

+284 rw

--

2.24.1

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

backup-top "supports" write-unchanged, by skipping CBW operation in

backup_top_co_pwritev. But it forgets to do the same in

backup_top_co_pwrite_zeroes, as well as declare support for

BDRV_REQ_WRITE_UNCHANGED.

Fix this, and, while being here, declare also support for flags

supported by source child.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20200207161231.32707-1-vsementsov@virtuozzo.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

block/backup-top.c | 31 ++++++++++++++++++++-----------

1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/block/backup-top.c b/block/backup-top.c

index XXXXXXX..XXXXXXX 100644

--- a/block/backup-top.c

+++ b/block/backup-top.c

@@ -XXX,XX +XXX,XX @@ static coroutine_fn int backup_top_co_preadv(

}

static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset,

- uint64_t bytes)

+ uint64_t bytes, BdrvRequestFlags flags)

{

BDRVBackupTopState *s = bs->opaque;

- uint64_t end = QEMU_ALIGN_UP(offset + bytes, s->bcs->cluster_size);

- uint64_t off = QEMU_ALIGN_DOWN(offset, s->bcs->cluster_size);

+ uint64_t off, end;

+

+ if (flags & BDRV_REQ_WRITE_UNCHANGED) {

+ return 0;

+ }

+

+ off = QEMU_ALIGN_DOWN(offset, s->bcs->cluster_size);

+ end = QEMU_ALIGN_UP(offset + bytes, s->bcs->cluster_size);

return block_copy(s->bcs, off, end - off, NULL);

}

@@ -XXX,XX +XXX,XX @@ static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset,

static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs,

int64_t offset, int bytes)

{

- int ret = backup_top_cbw(bs, offset, bytes);

+ int ret = backup_top_cbw(bs, offset, bytes, 0);

if (ret < 0) {

return ret;

}

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs,

static int coroutine_fn backup_top_co_pwrite_zeroes(BlockDriverState *bs,

int64_t offset, int bytes, BdrvRequestFlags flags)

{

- int ret = backup_top_cbw(bs, offset, bytes);

+ int ret = backup_top_cbw(bs, offset, bytes, flags);

if (ret < 0) {

return ret;

}

@@ -XXX,XX +XXX,XX @@ static coroutine_fn int backup_top_co_pwritev(BlockDriverState *bs,

uint64_t bytes,

QEMUIOVector *qiov, int flags)

{

- if (!(flags & BDRV_REQ_WRITE_UNCHANGED)) {

- int ret = backup_top_cbw(bs, offset, bytes);

- if (ret < 0) {

- return ret;

- }

+ int ret = backup_top_cbw(bs, offset, bytes, flags);

+ if (ret < 0) {

+ return ret;

}

return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);

@@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,

return NULL;

}

- top->total_sectors = source->total_sectors;

state = top->opaque;

+ top->total_sectors = source->total_sectors;

+ top->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |

+ (BDRV_REQ_FUA & source->supported_write_flags);

+ top->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |

+ ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &

+ source->supported_zero_flags);

bdrv_ref(target);

state->target = bdrv_attach_child(top, target, "target", &child_file, errp);

--

2.24.1

First, driver=qcow2 will not work so well with non-qcow2 formats (and

this test claims to support qcow, qed, and vmdk).

Second, vmdk will always report the backing file format to be vmdk.

Filter that out so the output looks like for all other formats.

Third, the flat vmdk subformats do not support backing files, so they

will not work with this test.

Signed-off-by: Max Reitz <mreitz@redhat.com>

Message-Id: <20191219144243.1763246-1-mreitz@redhat.com>

Tested-by: Thomas Huth <thuth@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

tests/qemu-iotests/279 | 7 +++++--

1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/279 b/tests/qemu-iotests/279

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/279

+++ b/tests/qemu-iotests/279

@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15

_supported_fmt qcow qcow2 vmdk qed

_supported_proto file

_supported_os Linux

+_unsupported_imgopts "subformat=monolithicFlat" \

+ "subformat=twoGbMaxExtentFlat" \

TEST_IMG="$TEST_IMG.base" _make_test_img 64M

TEST_IMG="$TEST_IMG.mid" _make_test_img -b "$TEST_IMG.base"

@@ -XXX,XX +XXX,XX @@ _make_test_img -b "$TEST_IMG.mid"

echo

echo '== qemu-img info --backing-chain =='

-_img_info --backing-chain | _filter_img_info

+_img_info --backing-chain | _filter_img_info | grep -v 'backing file format'

echo

echo '== qemu-img info --backing-chain --image-opts =='

-TEST_IMG="driver=qcow2,file.driver=file,file.filename=$TEST_IMG" _img_info --backing-chain --image-opts | _filter_img_info

+TEST_IMG="driver=$IMGFMT,file.driver=file,file.filename=$TEST_IMG" _img_info --backing-chain --image-opts \

+ | _filter_img_info | grep -v 'backing file format'

# success, all done

echo "*** done"

--

2.24.1

When nbd_close() is called from a coroutine, the connection_co never

gets to run, and thus nbd_teardown_connection() hangs.

This is because aio_co_enter() only puts the connection_co into the main

coroutine's wake-up queue, so this main coroutine needs to yield and

wait for connection_co to terminate.

Suggested-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

Message-Id: <20200122164532.178040-2-mreitz@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

block/nbd.c | 14 +++++++++++++-

1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/nbd.c b/block/nbd.c

index XXXXXXX..XXXXXXX 100644

--- a/block/nbd.c

+++ b/block/nbd.c

@@ -XXX,XX +XXX,XX @@ typedef struct BDRVNBDState {

CoMutex send_mutex;

CoQueue free_sema;

Coroutine *connection_co;

+ Coroutine *teardown_co;

QemuCoSleepState *connection_co_sleep_ns_state;

bool drained;

bool wait_drained_end;

@@ -XXX,XX +XXX,XX @@ static void nbd_teardown_connection(BlockDriverState *bs)

qemu_co_sleep_wake(s->connection_co_sleep_ns_state);

}

}

- BDRV_POLL_WHILE(bs, s->connection_co);

+ if (qemu_in_coroutine()) {

+ s->teardown_co = qemu_coroutine_self();

+ /* connection_co resumes us when it terminates */

+ qemu_coroutine_yield();

+ s->teardown_co = NULL;

+ } else {

+ BDRV_POLL_WHILE(bs, s->connection_co);

+ }

+ assert(!s->connection_co);

}

static bool nbd_client_connecting(BDRVNBDState *s)

@@ -XXX,XX +XXX,XX @@ static coroutine_fn void nbd_connection_entry(void *opaque)

s->ioc = NULL;

}

+ if (s->teardown_co) {

+ aio_co_wake(s->teardown_co);

+ }

aio_wait_kick();

}

--

2.24.1