229 relies on the mirror running into an I/O error when the target is

smaller than the source. After changing mirror to catch this condition

while starting the job, this test case won't get a job that is paused

for an I/O error any more. Use blkdebug instead to inject an error.

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

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

Message-Id: <20200511135825.219437-3-kwolf@redhat.com>

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

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

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

---

tests/qemu-iotests/229 | 15 +++++++++++----

tests/qemu-iotests/229.out | 6 +++---

2 files changed, 14 insertions(+), 7 deletions(-)

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/229

+++ b/tests/qemu-iotests/229

@@ -XXX,XX +XXX,XX @@ _cleanup()

_cleanup_test_img

_rm_test_img "$TEST_IMG"

_rm_test_img "$DEST_IMG"

+ rm -f "$TEST_DIR/blkdebug.conf"

}

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

@@ -XXX,XX +XXX,XX @@ _supported_os Linux

DEST_IMG="$TEST_DIR/d.$IMGFMT"

TEST_IMG="$TEST_DIR/b.$IMGFMT"

+BLKDEBUG_CONF="$TEST_DIR/blkdebug.conf"

_make_test_img 2M

-

-# destination for mirror will be too small, causing error

-TEST_IMG=$DEST_IMG _make_test_img 1M

+TEST_IMG=$DEST_IMG _make_test_img 2M

$QEMU_IO -c 'write 0 2M' "$TEST_IMG" | _filter_qemu_io

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

echo '=== Starting drive-mirror, causing error & stop ==='

echo

+cat > "$BLKDEBUG_CONF" <<EOF

+[inject-error]

+event = "write_aio"

+errno = "5"

+once = "on"

+EOF

+

_send_qemu_cmd $QEMU_HANDLE \

"{'execute': 'drive-mirror',

'arguments': {'device': 'testdisk',

'format': '$IMGFMT',

- 'target': '$DEST_IMG',

+ 'target': 'blkdebug:$BLKDEBUG_CONF:$DEST_IMG',

'sync': 'full',

'mode': 'existing',

'on-source-error': 'stop',

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

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/229.out

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

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

QA output created by 229

Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=2097152

-Formatting 'TEST_DIR/d.IMGFMT', fmt=IMGFMT size=1048576

+Formatting 'TEST_DIR/d.IMGFMT', fmt=IMGFMT size=2097152

wrote 2097152/2097152 bytes at offset 0

2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

{'execute': 'qmp_capabilities'}

@@ -XXX,XX +XXX,XX @@ wrote 2097152/2097152 bytes at offset 0

=== Starting drive-mirror, causing error & stop ===

-{'execute': 'drive-mirror', 'arguments': {'device': 'testdisk', 'format': 'IMGFMT', 'target': 'TEST_DIR/d.IMGFMT', 'sync': 'full', 'mode': 'existing', 'on-source-error': 'stop', 'on-target-error': 'stop' }}

+{'execute': 'drive-mirror', 'arguments': {'device': 'testdisk', 'format': 'IMGFMT', 'target': 'blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/d.IMGFMT', 'sync': 'full', 'mode': 'existing', 'on-source-error': 'stop', 'on-target-error': 'stop' }}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "testdisk"}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "testdisk"}}

{"return": {}}

@@ -XXX,XX +XXX,XX @@ wrote 2097152/2097152 bytes at offset 0

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "testdisk"}}

{"return": {}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "testdisk"}}

-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "testdisk", "len": 2097152, "offset": 1048576, "speed": 0, "type": "mirror"}}

+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "testdisk", "len": 2097152, "offset": 2097152, "speed": 0, "type": "mirror"}}

*** done

--

2.25.4

If the target is shorter than the source, mirror would copy data until

it reaches the end of the target and then fail with an I/O error when

trying to write past the end.

If the target is longer than the source, the mirror job would complete

successfully, but the target wouldn't actually be an accurate copy of

the source image (it would contain some additional garbage at the end).

Fix this by checking that both images have the same size when the job

starts.

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

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

Message-Id: <20200511135825.219437-4-kwolf@redhat.com>

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

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

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

---

block/mirror.c | 21 ++++++++++++---------

1 file changed, 12 insertions(+), 9 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/mirror.c

+++ b/block/mirror.c

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn mirror_run(Job *job, Error **errp)

BlockDriverState *target_bs = blk_bs(s->target);

bool need_drain = true;

int64_t length;

+ int64_t target_length;

BlockDriverInfo bdi;

char backing_filename[2]; /* we only need 2 characters because we are only

checking for a NULL string */

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn mirror_run(Job *job, Error **errp)

goto immediate_exit;

}

+ target_length = blk_getlength(s->target);

+ if (target_length < 0) {

+ ret = target_length;

+ goto immediate_exit;

+ }

+

/* Active commit must resize the base image if its size differs from the

* active layer. */

if (s->base == blk_bs(s->target)) {

- int64_t base_length;

-

- base_length = blk_getlength(s->target);

- if (base_length < 0) {

- ret = base_length;

- goto immediate_exit;

- }

-

- if (s->bdev_length > base_length) {

+ if (s->bdev_length > target_length) {

ret = blk_truncate(s->target, s->bdev_length, false,

PREALLOC_MODE_OFF, 0, NULL);

if (ret < 0) {

goto immediate_exit;

}

}

+ } else if (s->bdev_length != target_length) {

+ error_setg(errp, "Source and target image have different sizes");

+ ret = -EINVAL;

+ goto immediate_exit;

}

if (s->bdev_length == 0) {

--

2.25.4

This tests that the mirror job catches situations where the target node

has a different size than the source node. It must also forbid resize

operations when the job is already running.

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

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

Message-Id: <20200511135825.219437-5-kwolf@redhat.com>

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

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

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

---

tests/qemu-iotests/041 | 45 ++++++++++++++++++++++++++++++++++++++

tests/qemu-iotests/041.out | 4 ++--

2 files changed, 47 insertions(+), 2 deletions(-)

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/041

+++ b/tests/qemu-iotests/041

@@ -XXX,XX +XXX,XX @@ class TestSingleBlockdev(TestSingleDrive):

target=self.qmp_target)

self.assert_qmp(result, 'error/class', 'GenericError')

+ def do_test_resize(self, device, node):

+ def pre_finalize():

+ if device:

+ result = self.vm.qmp('block_resize', device=device, size=65536)

+ self.assert_qmp(result, 'error/class', 'GenericError')

+

+ result = self.vm.qmp('block_resize', node_name=node, size=65536)

+ self.assert_qmp(result, 'error/class', 'GenericError')

+

+ result = self.vm.qmp(self.qmp_cmd, job_id='job0', device='drive0',

+ sync='full', target=self.qmp_target,

+ auto_finalize=False, auto_dismiss=False)

+ self.assert_qmp(result, 'return', {})

+

+ result = self.vm.run_job('job0', auto_finalize=False,

+ pre_finalize=pre_finalize)

+ self.assertEqual(result, None)

+

+ def test_source_resize(self):

+ self.do_test_resize('drive0', 'top')

+

+ def test_target_resize(self):

+ self.do_test_resize(None, self.qmp_target)

+

+ def do_test_target_size(self, size):

+ result = self.vm.qmp('block_resize', node_name=self.qmp_target,

+ size=size)

+ self.assert_qmp(result, 'return', {})

+

+ result = self.vm.qmp(self.qmp_cmd, job_id='job0',

+ device='drive0', sync='full', auto_dismiss=False,

+ target=self.qmp_target)

+ self.assert_qmp(result, 'return', {})

+

+ result = self.vm.run_job('job0')

+ self.assertEqual(result, 'Source and target image have different sizes')

+

+ def test_small_target(self):

+ self.do_test_target_size(self.image_len // 2)

+

+ def test_large_target(self):

+ self.do_test_target_size(self.image_len * 2)

+

test_large_cluster = None

test_image_not_found = None

test_small_buffer2 = None

@@ -XXX,XX +XXX,XX @@ class TestSingleDriveZeroLength(TestSingleDrive):

class TestSingleBlockdevZeroLength(TestSingleBlockdev):

image_len = 0

+ test_small_target = None

+ test_large_target = None

class TestSingleDriveUnalignedLength(TestSingleDrive):

image_len = 1025 * 1024

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

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/041.out

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

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

-..............................................................................................

+........................................................................................................

----------------------------------------------------------------------

-Ran 94 tests

+Ran 104 tests

OK

--

2.25.4

From: Lukas Straub <lukasstraub2@web.de>

If qemu in colo secondary mode is stopped, it crashes because

s->backup_job is canceled twice: First with job_cancel_sync_all()

in qemu_cleanup() and then in replication_stop().

Fix this by assigning NULL to s->backup_job when the job completes

so replication_stop() and replication_do_checkpoint() won't touch

the job.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>

Message-Id: <20200511090801.7ed5d8f3@luklap>

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

---

block/replication.c | 2 ++

1 file changed, 2 insertions(+)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/replication.c

+++ b/block/replication.c

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

BDRVReplicationState *s = bs->opaque;

BlockDriverState *top_bs;

+ s->backup_job = NULL;

+

top_bs = bdrv_lookup_bs(s->top_id, s->top_id, NULL);

if (!top_bs) {

return;

--

2.25.4

From: Max Reitz <mreitz@redhat.com>

Right now, all users of bdrv_make_empty() call the BlockDriver method

directly. That is not only bad style, it is also wrong, unless the

caller has a BdrvChild with a WRITE or WRITE_UNCHANGED permission.

(WRITE_UNCHANGED suffices, because callers generally use this function

to clear a node with a backing file after a commit operation.)

Introduce bdrv_make_empty() that verifies that it does.

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

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

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

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

---

include/block/block.h | 1 +

block.c | 23 +++++++++++++++++++++++

2 files changed, 24 insertions(+)

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 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,

void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);

void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);

int bdrv_commit(BlockDriverState *bs);

+int bdrv_make_empty(BdrvChild *c, Error **errp);

int bdrv_change_backing_file(BlockDriverState *bs,

const char *backing_file, const char *backing_fmt);

void bdrv_register(BlockDriver *bdrv);

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)

parent_bs->drv->bdrv_del_child(parent_bs, child, errp);

}

+

+int bdrv_make_empty(BdrvChild *c, Error **errp)

+{

+ BlockDriver *drv = c->bs->drv;

+ int ret;

+

+ assert(c->perm & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED));

+

+ if (!drv->bdrv_make_empty) {

+ error_setg(errp, "%s does not support emptying nodes",

+ drv->format_name);

+ return -ENOTSUP;

+ }

+

+ ret = drv->bdrv_make_empty(c->bs);

+ if (ret < 0) {

+ error_setg_errno(errp, -ret, "Failed to empty %s",

+ c->bs->filename);

+ return ret;

+ }

+

+ return 0;

+}

--

2.25.4

Two callers of BlockDriver.bdrv_make_empty() remain that should not call

this method directly. Both do not have access to a BdrvChild, but they

can use a BlockBackend, so we add this function that lets them use it.

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

Message-Id: <20200429141126.85159-4-mreitz@redhat.com>

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

From: Max Reitz <mreitz@redhat.com>

bdrv_commit() already has a BlockBackend pointing to the BDS that we

want to empty, it just has the wrong permissions.

qemu-img commit has no BlockBackend pointing to the old backing file

yet, but introducing one is simple.

After this commit, bdrv_make_empty() is the only remaining caller of

BlockDriver.bdrv_make_empty().

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

Message-Id: <20200429141126.85159-5-mreitz@redhat.com>

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

[kwolf: Fixed up reference output for 098]

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

---

block/commit.c | 16 +++++++++-------

qemu-img.c | 19 ++++++++++++++-----

tests/qemu-iotests/098.out | 8 ++++----

3 files changed, 27 insertions(+), 16 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/commit.c

+++ b/block/commit.c

@@ -XXX,XX +XXX,XX @@ int bdrv_commit(BlockDriverState *bs)

}

ctx = bdrv_get_aio_context(bs);

- src = blk_new(ctx, BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL);

+ /* WRITE_UNCHANGED is required for bdrv_make_empty() */

+ src = blk_new(ctx, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED,

+ BLK_PERM_ALL);

backing = blk_new(ctx, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);

ret = blk_insert_bs(src, bs, &local_err);

@@ -XXX,XX +XXX,XX @@ int bdrv_commit(BlockDriverState *bs)

}

}

- if (drv->bdrv_make_empty) {

- ret = drv->bdrv_make_empty(bs);

- if (ret < 0) {

- goto ro_cleanup;

- }

- blk_flush(src);

+ ret = blk_make_empty(src, NULL);

+ /* Ignore -ENOTSUP */

+ if (ret < 0 && ret != -ENOTSUP) {

+ goto ro_cleanup;

}

+ blk_flush(src);

+

/*

* Make sure all data we wrote to the backing device is actually

* stable on disk.

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 @@ static int img_commit(int argc, char **argv)

goto unref_backing;

}

- if (!drop && bs->drv->bdrv_make_empty) {

- ret = bs->drv->bdrv_make_empty(bs);

- if (ret) {

- error_setg_errno(&local_err, -ret, "Could not empty %s",

- filename);

+ if (!drop) {

+ BlockBackend *old_backing_blk;

+

+ old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,

+ &local_err);

+ if (!old_backing_blk) {

+ goto unref_backing;

+ }

+ ret = blk_make_empty(old_backing_blk, &local_err);

+ blk_unref(old_backing_blk);

+ if (ret == -ENOTSUP) {

+ error_free(local_err);

+ local_err = NULL;

+ } else if (ret < 0) {

goto unref_backing;

}

}

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

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/098.out

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