From: Pan Nengyuan <pannengyuan@huawei.com>

'type/id' forgot to free in qmp_object_add, this patch fix that.

The leak stack:

Direct leak of 84 byte(s) in 6 object(s) allocated from:

#0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768)

#1 0x7fe2a5044445 in g_malloc (/lib64/libglib-2.0.so.0+0x52445)

#2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92)

#3 0x56344954e692 in qmp_object_add /mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:258

#4 0x563449960f5a in do_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132

#5 0x563449960f5a in qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175

#6 0x563449498a30 in monitor_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145

#7 0x56344949a64f in monitor_qmp_bh_dispatcher /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234

#8 0x563449a92a3a in aio_bh_call /mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136

Direct leak of 54 byte(s) in 6 object(s) allocated from:

#0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768)

#1 0x7fe2a5044445 in g_malloc (/lib64/libglib-2.0.so.0+0x52445)

#2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92)

#3 0x56344954e6c4 in qmp_object_add /mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:267

#4 0x563449960f5a in do_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132

#5 0x563449960f5a in qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175

#6 0x563449498a30 in monitor_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145

#7 0x56344949a64f in monitor_qmp_bh_dispatcher /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234

#8 0x563449a92a3a in aio_bh_call /mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136

Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e

Reported-by: Euler Robot <euler.robot@huawei.com>

Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>

Message-Id: <20200310064640.5059-1-pannengyuan@huawei.com>

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

Acked-by: Igor Mammedov <imammedo@redhat.com>

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

---

qom/qom-qmp-cmds.c | 16 ++++++----------

1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c

index XXXXXXX..XXXXXXX 100644

--- a/qom/qom-qmp-cmds.c

+++ b/qom/qom-qmp-cmds.c

@@ -XXX,XX +XXX,XX @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)

QDict *pdict;

Visitor *v;

Object *obj;

- const char *type;

- const char *id;

+ g_autofree char *type = NULL;

+ g_autofree char *id = NULL;

- type = qdict_get_try_str(qdict, "qom-type");

+ type = g_strdup(qdict_get_try_str(qdict, "qom-type"));

if (!type) {

error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");

return;

- } else {

- type = g_strdup(type);

- qdict_del(qdict, "qom-type");

}

+ qdict_del(qdict, "qom-type");

- id = qdict_get_try_str(qdict, "id");

+ id = g_strdup(qdict_get_try_str(qdict, "id"));

if (!id) {

error_setg(errp, QERR_MISSING_PARAMETER, "id");

return;

- } else {

- id = g_strdup(id);

- qdict_del(qdict, "id");

}

+ qdict_del(qdict, "id");

props = qdict_get(qdict, "props");

if (props) {

--

2.20.1

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

Message-Id: <20200310113831.27293-2-kwolf@redhat.com>

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

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

---

include/block/block_int.h | 3 +++

block.c | 6 ++----

2 files changed, 5 insertions(+), 4 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

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

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

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,

void *opaque, Error **errp);

void bdrv_root_unref_child(BdrvChild *child);

+void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,

+ uint64_t *shared_perm);

+

/**

* Sets a BdrvChild's permissions. Avoid if the parent is a BDS; use

* bdrv_child_refresh_perms() instead and make the parent's

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,

bool *tighten_restrictions, Error **errp);

static void bdrv_child_abort_perm_update(BdrvChild *c);

static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);

-static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,

- uint64_t *shared_perm);

typedef struct BlockReopenQueueEntry {

bool prepared;

@@ -XXX,XX +XXX,XX @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,

}

}

-static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,

- uint64_t *shared_perm)

+void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,

+ uint64_t *shared_perm)

{

BdrvChild *c;

uint64_t cumulative_perms = 0;

--

2.20.1

blockdev-snapshot returned an error if the overlay was already in use,

which it defined as having any BlockBackend parent. This is in fact both

too strict (some parents can tolerate the change of visible data caused

by attaching a backing file) and too loose (some non-BlockBackend

parents may not be happy with it).

One important use case that is prevented by the too strict check is live

storage migration with blockdev-mirror. Here, the target node is

usually opened without a backing file so that the active layer is

mirrored while its backing chain can be copied in the background.

The backing chain should be attached to the mirror target node when

finalising the job, just before switching the users of the source node

to the new copy (at which point the mirror job still has a reference to

the node). drive-mirror did this automatically, but with blockdev-mirror

this is the job of the QMP client, so it needs a way to do this.

blockdev-snapshot is the obvious way, so this patch makes it work in

this scenario. The new condition is that no parent uses CONSISTENT_READ

permissions. This will ensure that the operation will still be blocked

when the node is attached to the guest device, so blockdev-snapshot

remains safe.

(For the sake of completeness, x-blockdev-reopen can be used to achieve

the same, however it is a big hammer, performs the graph change

completely unchecked and is still experimental. So even with the option

of using x-blockdev-reopen, there are reasons why blockdev-snapshot

should be able to perform this operation.)

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

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

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

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

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

---

blockdev.c | 14 ++++++++------

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

2 files changed, 10 insertions(+), 8 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/blockdev.c

+++ b/blockdev.c

@@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common,

TransactionAction *action = common->action;

AioContext *aio_context;

AioContext *old_context;

+ uint64_t perm, shared;

int ret;

/* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar

@@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common,

goto out;

}

- if (bdrv_has_blk(state->new_bs)) {

+ /*

+ * Allow attaching a backing file to an overlay that's already in use only

+ * if the parents don't assume that they are already seeing a valid image.

+ * (Specifically, allow it as a mirror target, which is write-only access.)

+ */

+ bdrv_get_cumulative_perm(state->new_bs, &perm, &shared);

+ if (perm & BLK_PERM_CONSISTENT_READ) {

error_setg(errp, "The overlay is already in use");

goto out;

}

- if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,

- errp)) {

- goto out;

- }

-

if (state->new_bs->backing != NULL) {

error_setg(errp, "The overlay already has a backing image");

goto out;

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

index XXXXXXX..XXXXXXX 100644

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

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

@@ -XXX,XX +XXX,XX @@ Formatting 'TEST_DIR/12-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 backing_f

=== Invalid command - cannot create a snapshot using a file BDS ===

{ 'execute': 'blockdev-snapshot', 'arguments': { 'node':'virtio0', 'overlay':'file_12' } }

-{"error": {"class": "GenericError", "desc": "The overlay does not support backing images"}}

+{"error": {"class": "GenericError", "desc": "The overlay is already in use"}}

=== Invalid command - snapshot node used as active layer ===

@@ -XXX,XX +XXX,XX @@ Formatting 'TEST_DIR/12-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 backing_f

=== Invalid command - snapshot node used as backing hd ===

{ 'execute': 'blockdev-snapshot', 'arguments': { 'node': 'virtio0', 'overlay':'snap_11' } }

-{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is used as backing hd of 'snap_12'"}}

+{"error": {"class": "GenericError", "desc": "The overlay is already in use"}}

=== Invalid command - snapshot node has a backing image ===

--

2.20.1

The 'job-complete' QMP command should be run with qmp() rather than

qmp_log() if use_log=False is passed.

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

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

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

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

---

tests/qemu-iotests/iotests.py | 5 ++++-

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/iotests.py

+++ b/tests/qemu-iotests/iotests.py

@@ -XXX,XX +XXX,XX @@ class VM(qtest.QEMUQtestMachine):

if use_log:

log('Job failed: %s' % (j['error']))

elif status == 'ready':

- self.qmp_log('job-complete', id=job)

+ if use_log:

+ self.qmp_log('job-complete', id=job)

+ else:

+ self.qmp('job-complete', id=job)

elif status == 'pending' and not auto_finalize:

if pre_finalize:

pre_finalize()

--

2.20.1

The newly tested scenario is a common live storage migration scenario:

The target node is opened without a backing file so that the active

layer is mirrored while its backing chain can be copied in the

background.

The backing chain should be attached to the mirror target node when

finalising the job, just before switching the users of the source node

to the new copy (at which point the mirror job still has a reference to

the node). drive-mirror did this automatically, but with blockdev-mirror

this is the job of the QMP client.

This patch adds test cases for two ways to achieve the desired result,

using either x-blockdev-reopen or blockdev-snapshot.

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

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

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

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

---

tests/qemu-iotests/155 | 56 ++++++++++++++++++++++++++++++++++----

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

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

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/155

+++ b/tests/qemu-iotests/155

@@ -XXX,XX +XXX,XX @@ target_img = os.path.join(iotests.test_dir, 'target.' + iotests.imgfmt)

# image during runtime, only makes sense if

# target_blockdev_backing is not None

# (None: same as target_backing)

+# target_open_with_backing: If True, the target image is added with its backing

+# chain opened right away. If False, blockdev-add

+# opens it without a backing file and job completion

+# is supposed to open the backing chain.

class BaseClass(iotests.QMPTestCase):

target_blockdev_backing = None

target_real_backing = None

+ target_open_with_backing = True

def setUp(self):

qemu_img('create', '-f', iotests.imgfmt, back0_img, '1440K')

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

options = { 'node-name': 'target',

'driver': iotests.imgfmt,

'file': { 'driver': 'file',

+ 'node-name': 'target-file',

'filename': target_img } }

- if self.target_blockdev_backing:

- options['backing'] = self.target_blockdev_backing

+

+ if not self.target_open_with_backing:

+ options['backing'] = None

+ elif self.target_blockdev_backing:

+ options['backing'] = self.target_blockdev_backing

result = self.vm.qmp('blockdev-add', **options)

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

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

# cmd: Mirroring command to execute, either drive-mirror or blockdev-mirror

class MirrorBaseClass(BaseClass):

+ def openBacking(self):

+ pass

+

def runMirror(self, sync):

if self.cmd == 'blockdev-mirror':

result = self.vm.qmp(self.cmd, job_id='mirror-job', device='source',

- sync=sync, target='target')

+ sync=sync, target='target',

+ auto_finalize=False)

else:

if self.existing:

mode = 'existing'

@@ -XXX,XX +XXX,XX @@ class MirrorBaseClass(BaseClass):

result = self.vm.qmp(self.cmd, job_id='mirror-job', device='source',

sync=sync, target=target_img,

format=iotests.imgfmt, mode=mode,

- node_name='target')

+ node_name='target', auto_finalize=False)

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

- self.complete_and_wait('mirror-job')

+ self.vm.run_job('mirror-job', use_log=False, auto_finalize=False,

+ pre_finalize=self.openBacking, auto_dismiss=True)

def testFull(self):

self.runMirror('full')

@@ -XXX,XX +XXX,XX @@ class TestBlockdevMirrorForcedBacking(MirrorBaseClass):

target_blockdev_backing = { 'driver': 'null-co' }

target_real_backing = 'null-co://'

+# Attach the backing chain only during completion, with blockdev-reopen

+class TestBlockdevMirrorReopen(MirrorBaseClass):

+ cmd = 'blockdev-mirror'

+ existing = True

+ target_backing = 'null-co://'

+ target_open_with_backing = False

+

+ def openBacking(self):

+ if not self.target_open_with_backing:

+ result = self.vm.qmp('blockdev-add', node_name="backing",

+ driver="null-co")

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

+ result = self.vm.qmp('x-blockdev-reopen', node_name="target",

+ driver=iotests.imgfmt, file="target-file",

+ backing="backing")

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

+

+# Attach the backing chain only during completion, with blockdev-snapshot

+class TestBlockdevMirrorSnapshot(MirrorBaseClass):

+ cmd = 'blockdev-mirror'

+ existing = True

+ target_backing = 'null-co://'

+ target_open_with_backing = False

+

+ def openBacking(self):

+ if not self.target_open_with_backing:

+ result = self.vm.qmp('blockdev-add', node_name="backing",

+ driver="null-co")

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

+ result = self.vm.qmp('blockdev-snapshot', node="backing",

+ overlay="target")

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

class TestCommit(BaseClass):

existing = False

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

index XXXXXXX..XXXXXXX 100644

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

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

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

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

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

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

-Ran 19 tests

+Ran 25 tests

OK

--

2.20.1

external_snapshot_prepare() tries to move the overlay to the AioContext

of the backing file (the snapshotted node). However, it's possible that

this doesn't work, but the backing file can instead be moved to the

overlay's AioContext (e.g. opening the backing chain for a mirror

target).

bdrv_append() already indirectly uses bdrv_attach_node(), which takes

care to move nodes to make sure they use the same AioContext and which

tries both directions.

So the problem has a simple fix: Just delete the unnecessary extra

bdrv_try_set_aio_context() call in external_snapshot_prepare() and

instead assert in bdrv_append() that both nodes were indeed moved to the

same AioContext.

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

Message-Id: <20200310113831.27293-6-kwolf@redhat.com>

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

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

---

block.c | 1 +

blockdev.c | 16 ----------------

2 files changed, 1 insertion(+), 16 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,

bdrv_ref(from);

assert(qemu_get_current_aio_context() == qemu_get_aio_context());

+ assert(bdrv_get_aio_context(from) == bdrv_get_aio_context(to));

bdrv_drained_begin(from);

/* Put all parents into @list and calculate their cumulative permissions */

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

index XXXXXXX..XXXXXXX 100644

--- a/blockdev.c

+++ b/blockdev.c

@@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common,

DO_UPCAST(ExternalSnapshotState, common, common);

TransactionAction *action = common->action;

AioContext *aio_context;

- AioContext *old_context;

uint64_t perm, shared;

- int ret;

/* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar

* purpose but a different set of parameters */

@@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common,

goto out;

}

- /* Honor bdrv_try_set_aio_context() context acquisition requirements. */

- old_context = bdrv_get_aio_context(state->new_bs);

- aio_context_release(aio_context);

- aio_context_acquire(old_context);

-

- ret = bdrv_try_set_aio_context(state->new_bs, aio_context, errp);

-

- aio_context_release(old_context);

- aio_context_acquire(aio_context);

-

- if (ret < 0) {

- goto out;

- }

-

/* This removes our old bs and adds the new bs. This is an operation that

* can fail, so we need to do it in .prepare; undoing it for abort is

* always possible. */

--

2.20.1

This patch adds test cases for attaching the backing chain to a mirror

job target right before finalising the job, where the image is in a

non-mainloop AioContext (i.e. the backing chain needs to be moved to the

AioContext of the mirror target).

This requires switching the test case from virtio-blk to virtio-scsi

because virtio-blk only actually starts using the iothreads when the

guest driver initialises the device (which never happens in a test case

without a guest OS). virtio-scsi always keeps its block nodes in the

AioContext of the the requested iothread without guest interaction.

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

Message-Id: <20200310113831.27293-7-kwolf@redhat.com>

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

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

---

tests/qemu-iotests/155 | 32 +++++++++++++++++++++++---------

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

2 files changed, 25 insertions(+), 11 deletions(-)

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/155

+++ b/tests/qemu-iotests/155

@@ -XXX,XX +XXX,XX @@ target_img = os.path.join(iotests.test_dir, 'target.' + iotests.imgfmt)

# chain opened right away. If False, blockdev-add

# opens it without a backing file and job completion

# is supposed to open the backing chain.

+# use_iothread: If True, an iothread is configured for the virtio-blk device

+# that uses the image being mirrored

class BaseClass(iotests.QMPTestCase):

target_blockdev_backing = None

target_real_backing = None

target_open_with_backing = True

+ use_iothread = False

def setUp(self):

qemu_img('create', '-f', iotests.imgfmt, back0_img, '1440K')

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

'file': {'driver': 'file',

'filename': source_img}}

self.vm.add_blockdev(self.vm.qmp_to_opts(blockdev))

- self.vm.add_device('virtio-blk,id=qdev0,drive=source')

+

+ if self.use_iothread:

+ self.vm.add_object('iothread,id=iothread0')

+ iothread = ",iothread=iothread0"

+ else:

+ iothread = ""

+

+ self.vm.add_device('virtio-scsi%s' % iothread)

+ self.vm.add_device('scsi-hd,id=qdev0,drive=source')

+

self.vm.launch()

self.assertIntactSourceBackingChain()

@@ -XXX,XX +XXX,XX @@ class MirrorBaseClass(BaseClass):

def testFull(self):

self.runMirror('full')

- node = self.findBlockNode('target',

- '/machine/peripheral/qdev0/virtio-backend')

+ node = self.findBlockNode('target', 'qdev0')

self.assertCorrectBackingImage(node, None)

self.assertIntactSourceBackingChain()

def testTop(self):

self.runMirror('top')

- node = self.findBlockNode('target',

- '/machine/peripheral/qdev0/virtio-backend')

+ node = self.findBlockNode('target', 'qdev0')

self.assertCorrectBackingImage(node, back2_img)

self.assertIntactSourceBackingChain()

def testNone(self):

self.runMirror('none')

- node = self.findBlockNode('target',

- '/machine/peripheral/qdev0/virtio-backend')

+ node = self.findBlockNode('target', 'qdev0')

self.assertCorrectBackingImage(node, source_img)

self.assertIntactSourceBackingChain()

@@ -XXX,XX +XXX,XX @@ class TestBlockdevMirrorReopen(MirrorBaseClass):

backing="backing")

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

+class TestBlockdevMirrorReopenIothread(TestBlockdevMirrorReopen):

+ use_iothread = True

+

# Attach the backing chain only during completion, with blockdev-snapshot

class TestBlockdevMirrorSnapshot(MirrorBaseClass):

cmd = 'blockdev-mirror'

@@ -XXX,XX +XXX,XX @@ class TestBlockdevMirrorSnapshot(MirrorBaseClass):

overlay="target")

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

+class TestBlockdevMirrorSnapshotIothread(TestBlockdevMirrorSnapshot):

+ use_iothread = True

+

class TestCommit(BaseClass):

existing = False

@@ -XXX,XX +XXX,XX @@ class TestCommit(BaseClass):

self.vm.event_wait('BLOCK_JOB_COMPLETED')

- node = self.findBlockNode(None,

- '/machine/peripheral/qdev0/virtio-backend')

+ node = self.findBlockNode(None, 'qdev0')

self.assert_qmp(node, 'image' + '/backing-image' * 0 + '/filename',

back1_img)

self.assert_qmp(node, 'image' + '/backing-image' * 1 + '/filename',

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

index XXXXXXX..XXXXXXX 100644

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

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

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

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

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

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

-Ran 25 tests

+Ran 31 tests

OK

--

2.20.1

From: Peter Krempa <pkrempa@redhat.com>

Anounce that 'blockdev-snapshot' command's permissions allow changing

of the backing file if the 'consistent_read' permission is not required.

This is useful for libvirt to allow late opening of the backing chain

during a blockdev-mirror.

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

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

Message-Id: <20200310113831.27293-8-kwolf@redhat.com>

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

---

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

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

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 @@

#

# For the arguments, see the documentation of BlockdevSnapshot.

#

+# Features:

+# @allow-write-only-overlay: If present, the check whether this operation is safe

+# was relaxed so that it can be used to change

+# backing file of a destination of a blockdev-mirror.

+# (since 5.0)

+#

# Since: 2.5

#

# Example:

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

#

##

{ 'command': 'blockdev-snapshot',

- 'data': 'BlockdevSnapshot' }

+ 'data': 'BlockdevSnapshot',

+ 'features': [ 'allow-write-only-overlay' ] }

##

# @change-backing-file:

--

2.20.1

From: Philippe Mathieu-Daudé <philmd@redhat.com>

The socket_scm_helper path got corrupted during the mechanical

refactor moving the qtests files into their own sub-directory.

Fixes: 1e8a1fae7 ("test: Move qtests to a separate directory")

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Message-Id: <20200306165751.18986-1-philmd@redhat.com>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

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

---

tests/Makefile.include | 1 +

tests/qtest/Makefile.include | 1 -

2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include

index XXXXXXX..XXXXXXX 100644

--- a/tests/Makefile.include

+++ b/tests/Makefile.include

@@ -XXX,XX +XXX,XX @@ include $(SRC_PATH)/tests/qtest/Makefile.include

tests/test-qga$(EXESUF): qemu-ga$(EXESUF)

tests/test-qga$(EXESUF): tests/test-qga.o $(qtest-obj-y)

tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o $(test-util-obj-y) libvhost-user.a

+tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o

SPEED = quick

diff --git a/tests/qtest/Makefile.include b/tests/qtest/Makefile.include

index XXXXXXX..XXXXXXX 100644

--- a/tests/qtest/Makefile.include

+++ b/tests/qtest/Makefile.include

@@ -XXX,XX +XXX,XX @@ tests/qtest/usb-hcd-ehci-test$(EXESUF): tests/qtest/usb-hcd-ehci-test.o $(libqos

tests/qtest/usb-hcd-xhci-test$(EXESUF): tests/qtest/usb-hcd-xhci-test.o $(libqos-usb-obj-y)

tests/qtest/cpu-plug-test$(EXESUF): tests/qtest/cpu-plug-test.o

tests/qtest/migration-test$(EXESUF): tests/qtest/migration-test.o tests/qtest/migration-helpers.o

-tests/qtest/qemu-iotests/qtest/socket_scm_helper$(EXESUF): tests/qtest/qemu-iotests/qtest/socket_scm_helper.o

tests/qtest/test-netfilter$(EXESUF): tests/qtest/test-netfilter.o $(qtest-obj-y)

tests/qtest/test-filter-mirror$(EXESUF): tests/qtest/test-filter-mirror.o $(qtest-obj-y)

tests/qtest/test-filter-redirector$(EXESUF): tests/qtest/test-filter-redirector.o $(qtest-obj-y)

--

2.20.1

From: Daniel Henrique Barboza <danielhb413@gmail.com>

Adding to Block Drivers the capability of being able to clean up

its created files can be useful in certain situations. For the

LUKS driver, for instance, a failure in one of its authentication

steps can leave files in the host that weren't there before.

This patch adds the 'bdrv_co_delete_file' interface to block

drivers and add it to the 'file' driver in file-posix.c. The

implementation is given by 'raw_co_delete_file'.

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

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Message-Id: <20200130213907.2830642-2-danielhb413@gmail.com>

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

---

include/block/block_int.h | 4 ++++

block/file-posix.c | 23 +++++++++++++++++++++++

2 files changed, 27 insertions(+)

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

index XXXXXXX..XXXXXXX 100644

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

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

@@ -XXX,XX +XXX,XX @@ struct BlockDriver {

*/

int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);

+ /* Delete a created file. */

+ int coroutine_fn (*bdrv_co_delete_file)(BlockDriverState *bs,

+ Error **errp);

+

/*

* Flushes all data that was already written to the OS all the way down to

* the disk (for example file-posix.c calls fsync()).

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

index XXXXXXX..XXXXXXX 100644

--- a/block/file-posix.c

+++ b/block/file-posix.c

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,

return raw_co_create(&options, errp);

}

+static int coroutine_fn raw_co_delete_file(BlockDriverState *bs,

+ Error **errp)

+{

+ struct stat st;

+ int ret;

+

+ if (!(stat(bs->filename, &st) == 0) || !S_ISREG(st.st_mode)) {

+ error_setg_errno(errp, ENOENT, "%s is not a regular file",

+ bs->filename);

+ return -ENOENT;

+ }

+

+ ret = unlink(bs->filename);

+ if (ret < 0) {

+ ret = -errno;

+ error_setg_errno(errp, -ret, "Error when deleting file %s",

+ bs->filename);

+ }

+

+ return ret;

+}

+

/*

* Find allocation range in @bs around offset @start.

* May change underlying file descriptor's file offset.

@@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_file = {

.bdrv_co_block_status = raw_co_block_status,

.bdrv_co_invalidate_cache = raw_co_invalidate_cache,

.bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,

+ .bdrv_co_delete_file = raw_co_delete_file,

.bdrv_co_preadv = raw_co_preadv,

.bdrv_co_pwritev = raw_co_pwritev,

--

2.20.1

From: Daniel Henrique Barboza <danielhb413@gmail.com>

Using the new 'bdrv_co_delete_file' interface, a pure co_routine function

'bdrv_co_delete_file' inside block.c can can be used in a way similar of

the existing bdrv_create_file to to clean up a created file.

We're creating a pure co_routine because the only caller of

'bdrv_co_delete_file' will be already in co_routine context, thus there

is no need to add all the machinery to check for qemu_in_coroutine() and

create a separated co_routine to do the job.

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

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Message-Id: <20200130213907.2830642-3-danielhb413@gmail.com>

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

---

include/block/block.h | 1 +

block.c | 26 ++++++++++++++++++++++++++

2 files changed, 27 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 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,

int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,

Error **errp);

void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);

+int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp);

typedef struct BdrvCheckResult {

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)

}

}

+int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp)

+{

+ Error *local_err = NULL;

+ int ret;

+

+ assert(bs != NULL);

+

+ if (!bs->drv) {

+ error_setg(errp, "Block node '%s' is not opened", bs->filename);

+ return -ENOMEDIUM;

+ }

+

+ if (!bs->drv->bdrv_co_delete_file) {

+ error_setg(errp, "Driver '%s' does not support image deletion",

+ bs->drv->format_name);

+ return -ENOTSUP;

+ }

+

+ ret = bs->drv->bdrv_co_delete_file(bs, &local_err);

+ if (ret < 0) {

+ error_propagate(errp, local_err);

+ }

+

+ return ret;

+}

+

/**

* Try to get @bs's logical and physical block size.

* On success, store them in @bsz struct and return 0.

--

2.20.1

From: Daniel Henrique Barboza <danielhb413@gmail.com>

When using a non-UTF8 secret to create a volume using qemu-img, the

following error happens:

$ qemu-img create -f luks --object secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K

Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760 key-secret=vol_1_encrypt0

qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not valid UTF-8

However, the created file '/var/tmp/pool_target/vol_1' is left behind in the

file system after the failure. This behavior can be observed when creating

the volume using Libvirt, via 'virsh vol-create', and then getting "volume

target path already exist" errors when trying to re-create the volume.

The volume file is created inside block_crypto_co_create_opts_luks(), in

block/crypto.c. If the bdrv_create_file() call is successful but any

succeeding step fails*, the existing 'fail' label does not take into

account the created file, leaving it behind.

This patch changes block_crypto_co_create_opts_luks() to delete

'filename' in case of failure. A failure in this point means that

the volume is now truncated/corrupted, so even if 'filename' was an

existing volume before calling qemu-img, it is now unusable. Deleting

the file it is not much worse than leaving it in the filesystem in

this scenario, and we don't have to deal with checking the file

pre-existence in the code.

* in our case, block_crypto_co_create_generic calls qcrypto_block_create,

which calls qcrypto_block_luks_create, and this function fails when

calling qcrypto_secret_lookup_as_utf8.

Reported-by: Srikanth Aithal <bssrikanth@in.ibm.com>

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

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Message-Id: <20200130213907.2830642-4-danielhb413@gmail.com>

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

---

block/crypto.c | 18 ++++++++++++++++++

1 file changed, 18 insertions(+)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/crypto.c

+++ b/block/crypto.c

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

#include "qapi/error.h"

#include "qemu/module.h"

#include "qemu/option.h"

+#include "qemu/cutils.h"

#include "crypto.h"

typedef struct BlockCrypto BlockCrypto;

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,

ret = 0;

fail:

+ /*

+ * If an error occurred, delete 'filename'. Even if the file existed

+ * beforehand, it has been truncated and corrupted in the process.

+ */

+ if (ret && bs) {

+ Error *local_delete_err = NULL;

+ int r_del = bdrv_co_delete_file(bs, &local_delete_err);

+ /*

+ * ENOTSUP will happen if the block driver doesn't support

+ * the 'bdrv_co_delete_file' interface. This is a predictable

+ * scenario and shouldn't be reported back to the user.

+ */

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

+ error_report_err(local_delete_err);

+ }

+ }

+

bdrv_unref(bs);

qapi_free_QCryptoBlockCreateOptions(create_opts);

qobject_unref(cryptoopts);

--

2.20.1

From: Daniel Henrique Barboza <danielhb413@gmail.com>

This patch adds a new test file to exercise the case where

qemu-img fails to complete for the LUKS format when a non-UTF8

secret is used.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Message-Id: <20200130213907.2830642-5-danielhb413@gmail.com>

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

---

tests/qemu-iotests/282 | 67 ++++++++++++++++++++++++++++++++++++++

tests/qemu-iotests/282.out | 11 +++++++

tests/qemu-iotests/group | 1 +

3 files changed, 79 insertions(+)

create mode 100755 tests/qemu-iotests/282

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

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

new file mode 100755

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/282

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

+#!/usr/bin/env bash

+#

+# Test qemu-img file cleanup for LUKS when using a non-UTF8 secret

+#

+# Copyright (C) 2020, IBM Corporation.

+#

+# 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/>.

+#

+

+seq=`basename $0`

+echo "QA output created by $seq"

+

+status=1    # failure is the default!

+TEST_IMAGE_FILE='vol.img'

+

+_cleanup()

+{

+ _cleanup_test_img

+ rm non_utf8_secret

+ rm -f $TEST_IMAGE_FILE

+}

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

+

+# get standard environment, filters and checks

+. ./common.rc

+. ./common.filter

+

+_supported_fmt luks

+_supported_proto generic

+_unsupported_proto vxhs

+

+echo "== Create non-UTF8 secret =="

+echo -n -e '\x3a\x3c\x3b\xff' > non_utf8_secret

+SECRET="secret,id=sec0,file=non_utf8_secret"

+

+echo "== Throws an error because of invalid UTF-8 secret =="

+$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M

+

+echo "== Image file should not exist after the error =="

+if test -f "$TEST_IMAGE_FILE"; then

+ exit 1

+fi

+

+echo "== Create a stub image file and run qemu-img again =="

+touch $TEST_IMAGE_FILE

+$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M

+

+echo "== Pre-existing image file should also be deleted after the error =="

+if test -f "$TEST_IMAGE_FILE"; then

+ exit 1

+fi

+

+# success, all done

+echo "*** done"

+rm -f $seq.full

+status=0

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

new file mode 100644

index XXXXXXX..XXXXXXX

--- /dev/null

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

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

+QA output created by 282

+== Create non-UTF8 secret ==

+== Throws an error because of invalid UTF-8 secret ==

+qemu-img: vol.img: Data from secret sec0 is not valid UTF-8

+Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0

+== Image file should not exist after the error ==

+== Create a stub image file and run qemu-img again ==

+qemu-img: vol.img: Data from secret sec0 is not valid UTF-8

+Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0

+== Pre-existing image file should also be deleted after the error ==

+ *** 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 @@

279 rw backing quick

280 rw migration quick

281 rw quick

+282 rw img quick

283 auto quick

284 rw

286 rw quick

--

2.20.1