Re: [PATCH 00/10] image-fuzzer: Port to Python 3

[John Snow]

On 10/17/19 5:29 PM, Eduardo Habkost wrote:
> On Thu, Oct 17, 2019 at 05:11:29PM -0400, John Snow wrote:
>>
>>
>> On 10/16/19 3:24 PM, Eduardo Habkost wrote:
>>> This series ports image-fuzzer to Python 3.
>>>
>>> Eduardo Habkost (10):
>>>   image-fuzzer: Open image files in binary mode
>>>   image-fuzzer: Write bytes instead of string to image file
>>>   image-fuzzer: Explicitly use integer division operator
>>>   image-fuzzer: Use io.StringIO
>>>   image-fuzzer: Use %r for all fiels at Field.__repr__()
>>>   image-fuzzer: Return bytes objects on string fuzzing functions
>>>   image-fuzzer: Use bytes constant for field values
>>>   image-fuzzer: Encode file name and file format to bytes
>>>   image-fuzzer: Run using python3
>>>   image-fuzzer: Use errors parameter of subprocess.Popen()
>>>
>>>  tests/image-fuzzer/qcow2/__init__.py |  1 -
>>>  tests/image-fuzzer/qcow2/fuzz.py | 54 +-
>>>  tests/image-fuzzer/qcow2/layout.py   | 57 ++--
>>>  tests/image-fuzzer/runner.py | 12 +++---
>>>  4 files changed, 61 insertions(+), 63 deletions(-)
>>>
>>
>> When I gave my try at converting this to python3 I noticed that the
>> "except OSError as e" segments used e[1] in a way that was not seemingly
>> supported.
>>
>> Did you fix that in this series or did I miss it?
> 
> Good catch, I hadn't noticed that.  I didn't fix it.
> 

I recommend using pylint(3) with a bunch of the style issues turned off,
e.g.;

--disable=missing-docstring --disable=invalid-name

it will still whine about a lot of reasonably harmless stuff, but
sometimes it has a few errors to show.

--js

[PULL v3 19/19] dirty-bitmaps: remove deprecated autoload parameter

[John Snow]

This parameter has been deprecated since 2.12.0 and is eligible for
removal. Remove this parameter as it is actually completely ignored;
let's not give false hope.

Signed-off-by: John Snow 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20191002232411.29968-1-js...@redhat.com
---
 qemu-deprecated.texi | 20 +++-
 qapi/block-core.json |  6 +-
 blockdev.c   |  6 --
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 01245e0b1c..7239e0959d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -149,11 +149,6 @@ QEMU 4.1 has three options, please migrate to one of these 
three:
 
 @section QEMU Machine Protocol (QMP) commands
 
-@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
-
-"autoload" parameter is now ignored. All bitmaps are automatically loaded
-from qcow2 images.
-
 @subsection query-block result field dirty-bitmaps[i].status (since 4.0)
 
 The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
@@ -356,3 +351,18 @@ existing CPU models.  Management software that needs 
runnability
 guarantees must resolve the CPU model aliases using te
 ``alias-of'' field returned by the ``query-cpu-definitions'' QMP
 command.
+
+
+@node Recently removed features
+@appendix Recently removed features
+
+What follows is a record of recently removed, formerly deprecated
+features that serves as a record for users who have encountered
+trouble after a recent upgrade.
+
+@section QEMU Machine Protocol (QMP) commands
+
+@subsection block-dirty-bitmap-add "autoload" parameter (since 4.2.0)
+
+The "autoload" parameter has been ignored since 2.12.0. All bitmaps
+are automatically loaded from qcow2 images.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f66553aac7..b274aef713 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2052,10 +2052,6 @@
 #  Qcow2 disks support persistent bitmaps. Default is false for
 #  block-dirty-bitmap-add. (Since: 2.10)
 #
-# @autoload: ignored and deprecated since 2.12.
-#Currently, all dirty tracking bitmaps are loaded from Qcow2 on
-#open.
-#
 # @disabled: the bitmap is created in the disabled state, which means that
 #it will not track drive changes. The bitmap may be enabled with
 #block-dirty-bitmap-enable. Default is false. (Since: 4.0)
@@ -2064,7 +2060,7 @@
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-'*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } }
+'*persistent': 'bool', '*disabled': 'bool' } }
 
 ##
 # @BlockDirtyBitmapMergeSource:
diff --git a/blockdev.c b/blockdev.c
index d77e809623..03c7cd7651 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1966,7 +1966,6 @@ static void block_dirty_bitmap_add_prepare(BlkActionState 
*common,
 qmp_block_dirty_bitmap_add(action->node, action->name,
action->has_granularity, action->granularity,
action->has_persistent, action->persistent,
-   action->has_autoload, action->autoload,
action->has_disabled, action->disabled,
_err);
 
@@ -2858,7 +2857,6 @@ out:
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 bool has_granularity, uint32_t granularity,
 bool has_persistent, bool persistent,
-bool has_autoload, bool autoload,
 bool has_disabled, bool disabled,
 Error **errp)
 {
@@ -2890,10 +2888,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 persistent = false;
 }
 
-if (has_autoload) {
-warn_report("Autoload option is deprecated and its value is ignored");
-}
-
 if (!has_disabled) {
 disabled = false;
 }
-- 
2.21.0

[PULL v3 18/19] MAINTAINERS: Add Vladimir as a reviewer for bitmaps

[John Snow]

I already try to make sure all bitmaps patches have been reviewed by both
Red Hat and Virtuozzo anyway, so this formalizes the arrangement.

Fam meanwhile is no longer as active, so I am removing him as a co-maintainer
simply to reflect the current practice.

Signed-off-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20191005194448.16629-2-js...@redhat.com
---
 MAINTAINERS | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index fe4dc51b08..250ce8e7a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1816,8 +1816,8 @@ F: qapi/transaction.json
 T: git https://repo.or.cz/qemu/armbru.git block-next
 
 Dirty Bitmaps
-M: Fam Zheng 
 M: John Snow 
+R: Vladimir Sementsov-Ogievskiy 
 L: qemu-block@nongnu.org
 S: Supported
 F: util/hbitmap.c
@@ -1826,7 +1826,6 @@ F: include/qemu/hbitmap.h
 F: include/block/dirty-bitmap.h
 F: tests/test-hbitmap.c
 F: docs/interop/bitmaps.rst
-T: git https://github.com/famz/qemu.git bitmaps
 T: git https://github.com/jnsnow/qemu.git bitmaps
 
 Character device backends
-- 
2.21.0

[PULL v3 17/19] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

The only reason I can imagine for this strange code at the very-end of
bdrv_reopen_commit is the fact that bs->read_only updated after
calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same
time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong
check for being writable, when actually it only need writable file
child not self.

So, as it's fixed, let's move things to correct place.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Acked-by: Max Reitz 
Message-id: 20190927122355.7344-10-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/block/block_int.h |  6 --
 block.c   | 19 ---
 block/qcow2.c | 15 ++-
 3 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 32fb493cbb..ca4ccac4c1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -547,12 +547,6 @@ struct BlockDriver {
  uint64_t parent_perm, uint64_t parent_shared,
  uint64_t *nperm, uint64_t *nshared);
 
-/**
- * Bitmaps should be marked as 'IN_USE' in the image on reopening image
- * as rw. This handler should realize it. It also should unset readonly
- * field of BlockDirtyBitmap's in case of success.
- */
-int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
 bool (*bdrv_co_can_store_new_dirty_bitmap)(BlockDriverState *bs,
const char *name,
uint32_t granularity,
diff --git a/block.c b/block.c
index cf312258a9..dad5a3d8e0 100644
--- a/block.c
+++ b/block.c
@@ -3935,16 +3935,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 BlockDriver *drv;
 BlockDriverState *bs;
 BdrvChild *child;
-bool old_can_write, new_can_write;
 
 assert(reopen_state != NULL);
 bs = reopen_state->bs;
 drv = bs->drv;
 assert(drv != NULL);
 
-old_can_write =
-!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
-
 /* If there are any driver level actions to take */
 if (drv->bdrv_reopen_commit) {
 drv->bdrv_reopen_commit(reopen_state);
@@ -3988,21 +3984,6 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 }
 
 bdrv_refresh_limits(bs, NULL);
-
-new_can_write =
-!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
-if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
-Error *local_err = NULL;
-if (drv->bdrv_reopen_bitmaps_rw(bs, _err) < 0) {
-/* This is not fatal, bitmaps just left read-only, so all following
- * writes will fail. User can remove read-only bitmaps to unblock
- * writes.
- */
-error_reportf_err(local_err,
-  "%s: Failed to make dirty bitmaps writable: ",
-  bdrv_get_node_name(bs));
-}
-}
 }
 
 /*
diff --git a/block/qcow2.c b/block/qcow2.c
index 53a025703e..8b05933565 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1835,6 +1835,20 @@ fail:
 static void qcow2_reopen_commit(BDRVReopenState *state)
 {
 qcow2_update_options_commit(state->bs, state->opaque);
+if (state->flags & BDRV_O_RDWR) {
+Error *local_err = NULL;
+
+if (qcow2_reopen_bitmaps_rw(state->bs, _err) < 0) {
+/*
+ * This is not fatal, bitmaps just left read-only, so all following
+ * writes will fail. User can remove read-only bitmaps to unblock
+ * writes or retry reopen.
+ */
+error_reportf_err(local_err,
+  "%s: Failed to make dirty bitmaps writable: ",
+  bdrv_get_node_name(state->bs));
+}
+}
 g_free(state->opaque);
 }
 
@@ -5406,7 +5420,6 @@ BlockDriver bdrv_qcow2 = {
 .bdrv_detach_aio_context  = qcow2_detach_aio_context,
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
-.bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
 .bdrv_co_can_store_new_dirty_bitmap = qcow2_co_can_store_new_dirty_bitmap,
 .bdrv_co_remove_persistent_dirty_bitmap =
 qcow2_co_remove_persistent_dirty_bitmap,
-- 
2.21.0

[PULL v3 15/19] iotests: add test 260 to check bitmap life after snapshot + commit

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190927122355.7344-8-vsement...@virtuozzo.com
[Maintainer edit: removed 260 from auto group per Peter Maydell. --js]
Signed-off-by: John Snow 
---
 tests/qemu-iotests/260 | 89 ++
 tests/qemu-iotests/260.out | 52 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 142 insertions(+)
 create mode 100755 tests/qemu-iotests/260
 create mode 100644 tests/qemu-iotests/260.out

diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
new file mode 100755
index 00..4f6082c9d2
--- /dev/null
+++ b/tests/qemu-iotests/260
@@ -0,0 +1,89 @@
+#!/usr/bin/env python
+#
+# Tests for temporary external snapshot when we have bitmaps.
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# 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 .
+#
+
+import iotests
+from iotests import qemu_img_create, file_path, log, filter_qmp_event
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+base, top = file_path('base', 'top')
+size = 64 * 1024 * 3
+
+
+def print_bitmap(msg, vm):
+result = vm.qmp('query-block')['return'][0]
+if 'dirty-bitmaps' in result:
+bitmap = result['dirty-bitmaps'][0]
+log('{}: name={} dirty-clusters={}'.format(msg, bitmap['name'],
+bitmap['count'] // 64 // 1024))
+else:
+log(msg + ': not found')
+
+
+def test(persistent, restart):
+assert persistent or not restart
+log("\nTestcase {}persistent {} restart\n".format(
+'' if persistent else 'non-', 'with' if restart else 'without'))
+
+qemu_img_create('-f', iotests.imgfmt, base, str(size))
+
+vm = iotests.VM().add_drive(base)
+vm.launch()
+
+vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0',
+   persistent=persistent)
+vm.hmp_qemu_io('drive0', 'write 0 64K')
+print_bitmap('initial bitmap', vm)
+
+vm.qmp_log('blockdev-snapshot-sync', device='drive0', snapshot_file=top,
+   format=iotests.imgfmt, filters=[iotests.filter_qmp_testfiles])
+vm.hmp_qemu_io('drive0', 'write 64K 512')
+print_bitmap('check that no bitmaps are in snapshot', vm)
+
+if restart:
+log("... Restart ...")
+vm.shutdown()
+vm = iotests.VM().add_drive(top)
+vm.launch()
+
+vm.qmp_log('block-commit', device='drive0', top=top,
+   filters=[iotests.filter_qmp_testfiles])
+ev = vm.events_wait((('BLOCK_JOB_READY', None),
+ ('BLOCK_JOB_COMPLETED', None)))
+log(filter_qmp_event(ev))
+if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
+vm.shutdown()
+log(vm.get_log())
+exit()
+
+vm.qmp_log('block-job-complete', device='drive0')
+ev = vm.event_wait('BLOCK_JOB_COMPLETED')
+log(filter_qmp_event(ev))
+print_bitmap('check bitmap after commit', vm)
+
+vm.hmp_qemu_io('drive0', 'write 128K 64K')
+print_bitmap('check updated bitmap', vm)
+
+vm.shutdown()
+
+
+test(persistent=False, restart=False)
+test(persistent=True, restart=False)
+test(persistent=True, restart=True)
diff --git a/tests/qemu-iotests/260.out b/tests/qemu-iotests/260.out
new file mode 100644
index 00..2f0d98d036
--- /dev/null
+++ b/tests/qemu-iotests/260.out
@@ -0,0 +1,52 @@
+
+Testcase non-persistent without restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": 
"drive0", "persistent": false}}
+{"return": {}}
+initial bitmap: name=bitmap0 dirty-clusters=1
+{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", 
"format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
+{"return": {}}
+check that no bitmaps are in snapshot: not found
+{"execute": "block-commit", "arguments": {"device": "drive0", "top": 
"TEST_DIR/PID-top"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": 
"USECS", "seconds": "SECS"}}
+{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+check bitmap after commit: name=bitmap0 dirty-clusters=2

[PULL v3 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
readonly. But the latter don't work, as
qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
It's OK for inactivation but bad idea for reopen-ro. And this leads to
the following bug:

Assume we have persistent bitmap 'bitmap0'.
Create external snapshot
  bitmap0 is stored and therefore removed
Commit snapshot
  now we have no bitmaps
Do some writes from guest (*)
  they are not marked in bitmap
Shutdown
Start
  bitmap0 is loaded as valid, but it is actually broken! It misses
  writes (*)
Incremental backup
  it will be inconsistent

So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
reopening bitmaps to rw is broken too, so the whole scenario will not
work after this patch and we can't enable corresponding test cases in
260 iotests still. Reopening bitmaps rw will be fixed in the following
patches.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190927122355.7344-7-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/qcow2.h|  3 ++-
 block/qcow2-bitmap.c | 49 ++--
 block/qcow2.c|  2 +-
 3 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 23a9898a54..5cccd87162 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -742,7 +742,8 @@ Qcow2BitmapInfoList 
*qcow2_get_bitmap_info_list(BlockDriverState *bs,
 Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+  bool release_stored, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
  const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index ebc1afccd3..f7dfb40256 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1440,7 +1440,32 @@ out:
 return ret;
 }
 
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/*
+ * qcow2_store_persistent_dirty_bitmaps
+ *
+ * Stores persistent BdrvDirtyBitmap objects.
+ *
+ * @release_stored: if true, release BdrvDirtyBitmap's after storing to the
+ * image. This is used in two cases, both via qcow2_inactivate:
+ * 1. bdrv_close: It's correct to remove bitmaps on close.
+ * 2. migration: If bitmaps are migrated through migration channel via
+ *'dirty-bitmaps' migration capability they are not handled by this code.
+ *Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
+ *invalidation.
+ *
+ * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
+ * inactivation means that we lose control on disk, and therefore on bitmaps,
+ * we should sync them and do not touch more.
+ *
+ * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro,
+ * when we need to store them, as image is still under our control, and it's
+ * good to keep all the bitmaps in read-only mode. Moreover, keeping them
+ * read-only is correct because this is what would happen if we opened the node
+ * readonly to begin with, and whether we opened directly or reopened to that
+ * state shouldn't matter for the state we get afterward.
+ */
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+  bool release_stored, Error **errp)
 {
 BdrvDirtyBitmap *bitmap;
 BDRVQcow2State *s = bs->opaque;
@@ -1551,20 +1576,14 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 g_free(tb);
 }
 
-QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-/* For safety, we remove bitmap after storing.
- * We may be here in two cases:
- * 1. bdrv_close. It's ok to drop bitmap.
- * 2. inactivation. It means migration without 'dirty-bitmaps'
- *capability, so bitmaps are not marked with
- *BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
- *and reload on invalidation.
- */
-if (bm->dirty_bitmap == NULL) {
-continue;
-}
+if (release_stored) {
+QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+if (bm->dirty_bitmap == NULL) {
+continue;
+}
 
-bdrv_release_dirty_bitmap(bm->dirty_bitmap);
+bdrv_release_dirty_bitmap(bm->dirty_bitmap);
+}
 }
 
 success:
@@ -1592,7 +1611,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error 
**errp)
 BdrvDirtyBitmap *bitmap;
 Error *local_err = NULL;
 
-qcow2_store_persistent_dirty_bitmaps(bs, _err);
+

[PULL v3 16/19] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

- Correct check for write access to file child, and in correct place
  (only if we want to write).
- Support reopen rw -> rw (which will be used in following commit),
  for example, !bdrv_dirty_bitmap_readonly() is not a corruption if
  bitmap is marked IN_USE in the image.
- Consider unexpected bitmap as a corruption and check other
  combinations of in-image and in-RAM bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190927122355.7344-9-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/qcow2-bitmap.c | 77 +---
 1 file changed, 58 insertions(+), 19 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f7dfb40256..98294a7696 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1108,18 +1108,14 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 Qcow2BitmapList *bm_list;
 Qcow2Bitmap *bm;
 GSList *ro_dirty_bitmaps = NULL;
-int ret = 0;
+int ret = -EINVAL;
+bool need_header_update = false;
 
 if (s->nb_bitmaps == 0) {
 /* No bitmaps - nothing to do */
 return 0;
 }
 
-if (!can_write(bs)) {
-error_setg(errp, "Can't write to the image on reopening bitmaps rw");
-return -EINVAL;
-}
-
 bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
 if (bm_list == NULL) {
@@ -1128,32 +1124,75 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
 BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
-if (bitmap == NULL) {
-continue;
-}
 
-if (!bdrv_dirty_bitmap_readonly(bitmap)) {
-error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was 
"
-   "not marked as readonly. This is a bug, something went "
-   "wrong. All of the bitmaps may be corrupted", bm->name);
-ret = -EINVAL;
+if (!bitmap) {
+error_setg(errp, "Unexpected bitmap '%s' in image '%s'",
+   bm->name, bs->filename);
 goto out;
 }
 
-bm->flags |= BME_FLAG_IN_USE;
-ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+if (!(bm->flags & BME_FLAG_IN_USE)) {
+if (!bdrv_dirty_bitmap_readonly(bitmap)) {
+error_setg(errp, "Corruption: bitmap '%s' is not marked IN_USE 
"
+   "in the image '%s' and not marked readonly in RAM",
+   bm->name, bs->filename);
+goto out;
+}
+if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
+error_setg(errp, "Corruption: bitmap '%s' is inconsistent but "
+   "is not marked IN_USE in the image '%s'", bm->name,
+   bs->filename);
+goto out;
+}
+
+bm->flags |= BME_FLAG_IN_USE;
+need_header_update = true;
+} else {
+/*
+ * What if flags already has BME_FLAG_IN_USE ?
+ *
+ * 1. if we are reopening RW -> RW it's OK, of course.
+ * 2. if we are reopening RO -> RW:
+ *   2.1 if @bitmap is inconsistent, it's OK. It means that it was
+ *   inconsistent (IN_USE) when we loaded it
+ *   2.2 if @bitmap is not inconsistent. This seems to be 
impossible
+ *   and implies third party interaction. Let's error-out for
+ *   safety.
+ */
+if (bdrv_dirty_bitmap_readonly(bitmap) &&
+!bdrv_dirty_bitmap_inconsistent(bitmap))
+{
+error_setg(errp, "Corruption: bitmap '%s' is marked IN_USE "
+   "in the image '%s' but it is readonly and "
+   "consistent in RAM",
+   bm->name, bs->filename);
+goto out;
+}
+}
+
+if (bdrv_dirty_bitmap_readonly(bitmap)) {
+ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+}
 }
 
-if (ro_dirty_bitmaps != NULL) {
+if (need_header_update) {
+if (!can_write(bs->file->bs) || !(bs->file->perm & BLK_PERM_WRITE)) {
+error_setg(errp, "Failed to reopen bitmaps rw: no write access "
+   "the protocol file");
+goto out;
+}
+
 /* in_use flags must be updated */
 ret = update_ext_header_and_dir_in_place(bs, bm_list);
 if (ret < 0) {
-error_setg_errno(errp, -ret, "Can't update bitmap directory");
+error_setg_errno(errp, -ret, "Cannot update bitmap directory");
 goto out;
 }
-g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
 }
 
+

[PULL v3 13/19] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

The function is unused, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190927122355.7344-6-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/qcow2.h|  2 --
 block/qcow2-bitmap.c | 15 +--
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 0f3d9b088e..23a9898a54 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -740,8 +740,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
 bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
 Error **errp);
-int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
- Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6dfc083548..ebc1afccd3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1102,8 +1102,7 @@ Qcow2BitmapInfoList 
*qcow2_get_bitmap_info_list(BlockDriverState *bs,
 return list;
 }
 
-int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
- Error **errp)
+int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
 Qcow2BitmapList *bm_list;
@@ -,10 +1110,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, 
bool *header_updated,
 GSList *ro_dirty_bitmaps = NULL;
 int ret = 0;
 
-if (header_updated != NULL) {
-*header_updated = false;
-}
-
 if (s->nb_bitmaps == 0) {
 /* No bitmaps - nothing to do */
 return 0;
@@ -1156,9 +1151,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, 
bool *header_updated,
 error_setg_errno(errp, -ret, "Can't update bitmap directory");
 goto out;
 }
-if (header_updated != NULL) {
-*header_updated = true;
-}
 g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
 }
 
@@ -1169,11 +1161,6 @@ out:
 return ret;
 }
 
-int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
-{
-return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
-}
-
 /* Checks to see if it's safe to resize bitmaps */
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
 {
-- 
2.21.0

[PULL v3 07/19] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

mutex field is just a pointer to bs->dirty_bitmap_mutex, so no needs
to store it in BdrvDirtyBitmap when we have bs pointer in it (since
previous patch).

Drop mutex field. Constantly use bdrv_dirty_bitmaps_lock/unlock in
block/dirty-bitmap.c to make it more obvious that it's not per-bitmap
lock. Still, for simplicity, leave bdrv_dirty_bitmap_lock/unlock
functions as an external API.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190916141911.5255-4-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/dirty-bitmap.c | 84 +---
 1 file changed, 41 insertions(+), 43 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 44453ff824..4e5c87a907 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -29,7 +29,6 @@
 #include "qemu/main-loop.h"
 
 struct BdrvDirtyBitmap {
-QemuMutex *mutex;
 BlockDriverState *bs;
 HBitmap *bitmap;/* Dirty bitmap implementation */
 bool busy;  /* Bitmap is busy, it can't be used via QMP */
@@ -72,12 +71,12 @@ static inline void 
bdrv_dirty_bitmaps_unlock(BlockDriverState *bs)
 
 void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap)
 {
-qemu_mutex_lock(bitmap->mutex);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 }
 
 void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap)
 {
-qemu_mutex_unlock(bitmap->mutex);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called with BQL or dirty_bitmap lock taken.  */
@@ -117,7 +116,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 }
 bitmap = g_new0(BdrvDirtyBitmap, 1);
 bitmap->bs = bs;
-bitmap->mutex = >dirty_bitmap_mutex;
 bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity));
 bitmap->size = bitmap_size;
 bitmap->name = g_strdup(name);
@@ -151,9 +149,9 @@ static bool bdrv_dirty_bitmap_busy(const BdrvDirtyBitmap 
*bitmap)
 
 void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy)
 {
-qemu_mutex_lock(bitmap->mutex);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bitmap->busy = busy;
-qemu_mutex_unlock(bitmap->mutex);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called with BQL taken.  */
@@ -278,10 +276,10 @@ void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap 
*bitmap)
 /* Called with BQL taken. */
 void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
 {
-assert(bitmap->mutex == bitmap->successor->mutex);
-qemu_mutex_lock(bitmap->mutex);
+assert(bitmap->bs == bitmap->successor->bs);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bdrv_enable_dirty_bitmap_locked(bitmap->successor);
-qemu_mutex_unlock(bitmap->mutex);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.  */
@@ -361,9 +359,9 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap 
*parent,
 {
 BdrvDirtyBitmap *ret;
 
-qemu_mutex_lock(parent->mutex);
+bdrv_dirty_bitmaps_lock(parent->bs);
 ret = bdrv_reclaim_dirty_bitmap_locked(parent, errp);
-qemu_mutex_unlock(parent->mutex);
+bdrv_dirty_bitmaps_unlock(parent->bs);
 
 return ret;
 }
@@ -543,16 +541,16 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState 
*bs, const char *name,
 
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bitmap->disabled = true;
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bdrv_enable_dirty_bitmap_locked(bitmap);
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
@@ -593,9 +591,9 @@ bool bdrv_dirty_bitmap_get_locked(BdrvDirtyBitmap *bitmap, 
int64_t offset)
 bool bdrv_dirty_bitmap_get(BdrvDirtyBitmap *bitmap, int64_t offset)
 {
 bool ret;
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 ret = bdrv_dirty_bitmap_get_locked(bitmap, offset);
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 
 return ret;
 }
@@ -660,9 +658,9 @@ void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes)
 {
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bdrv_set_dirty_bitmap_locked(bitmap, offset, bytes);
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called within bdrv_dirty_bitmap_lock..unlock */
@@ -676,15 +674,15 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap 
*bitmap,
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  

[PULL v3 04/19] block/qcow2: proper locking on bitmap add/remove paths

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

qmp_block_dirty_bitmap_add and do_block_dirty_bitmap_remove do acquire
aio context since 0a6c86d024c52b. But this is not enough: we also must
lock qcow2 mutex when access in-image metadata. Especially it concerns
freeing qcow2 clusters.

To achieve this, move qcow2_can_store_new_dirty_bitmap and
qcow2_remove_persistent_dirty_bitmap to coroutine context.

Since we work in coroutines in correct aio context, we don't need
context acquiring in blockdev.c anymore, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190920082543.23444-4-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/qcow2.h |  11 ++--
 include/block/block_int.h |  10 ++--
 block/dirty-bitmap.c  | 102 +++---
 block/qcow2-bitmap.c  |  24 ++---
 block/qcow2.c |   5 +-
 blockdev.c|  27 +++---
 6 files changed, 131 insertions(+), 48 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 08b4c15dc4..0f3d9b088e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -746,12 +746,13 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
-bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
-  const char *name,
-  uint32_t granularity,
-  Error **errp);
-int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char 
*name,
+bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ uint32_t granularity,
  Error **errp);
+int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+const char *name,
+Error **errp);
 
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6b511dd889..32fb493cbb 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -553,13 +553,13 @@ struct BlockDriver {
  * field of BlockDirtyBitmap's in case of success.
  */
 int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
-bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
-const char *name,
-uint32_t granularity,
-Error **errp);
-int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+bool (*bdrv_co_can_store_new_dirty_bitmap)(BlockDriverState *bs,
const char *name,
+   uint32_t granularity,
Error **errp);
+int (*bdrv_co_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+  const char *name,
+  Error **errp);
 
 /**
  * Register/unregister a buffer for I/O. For example, when the driver is
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d1ae2e1922..03e0872b97 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -26,6 +26,7 @@
 #include "trace.h"
 #include "block/block_int.h"
 #include "block/blockjob.h"
+#include "qemu/main-loop.h"
 
 struct BdrvDirtyBitmap {
 QemuMutex *mutex;
@@ -455,18 +456,59 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState 
*bs)
  * not fail.
  * This function doesn't release corresponding BdrvDirtyBitmap.
  */
+static int coroutine_fn
+bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+   Error **errp)
+{
+if (bs->drv && bs->drv->bdrv_co_remove_persistent_dirty_bitmap) {
+return bs->drv->bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
+}
+
+return 0;
+}
+
+typedef struct BdrvRemovePersistentDirtyBitmapCo {
+BlockDriverState *bs;
+const char *name;
+Error **errp;
+int ret;
+} BdrvRemovePersistentDirtyBitmapCo;
+
+static void coroutine_fn
+bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque)
+{
+BdrvRemovePersistentDirtyBitmapCo *s = opaque;
+
+s->ret = bdrv_co_remove_persistent_dirty_bitmap(s->bs, s->name, s->errp);
+aio_wait_kick();
+}
+
 int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
 Error **errp)
 {
-if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
-return 

[PULL v3 09/19] block: switch reopen queue from QSIMPLEQ to QTAILQ

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

We'll need reverse-foreach in the following commit, QTAILQ support it,
so move to QTAILQ.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Message-id: 20190927122355.7344-2-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/block/block.h |  2 +-
 block.c   | 24 
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 792bb826db..89606bd9f8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -195,7 +195,7 @@ typedef struct HDGeometry {
 #define BDRV_BLOCK_EOF  0x20
 #define BDRV_BLOCK_RECURSE  0x40
 
-typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) 
BlockReopenQueue;
+typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
 
 typedef struct BDRVReopenState {
 BlockDriverState *bs;
diff --git a/block.c b/block.c
index 5721441697..0347632c6c 100644
--- a/block.c
+++ b/block.c
@@ -1719,7 +1719,7 @@ typedef struct BlockReopenQueueEntry {
  bool prepared;
  bool perms_checked;
  BDRVReopenState state;
- QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+ QTAILQ_ENTRY(BlockReopenQueueEntry) entry;
 } BlockReopenQueueEntry;
 
 /*
@@ -1732,7 +1732,7 @@ static int bdrv_reopen_get_flags(BlockReopenQueue *q, 
BlockDriverState *bs)
 BlockReopenQueueEntry *entry;
 
 if (q != NULL) {
-QSIMPLEQ_FOREACH(entry, q, entry) {
+QTAILQ_FOREACH(entry, q, entry) {
 if (entry->state.bs == bs) {
 return entry->state.flags;
 }
@@ -3249,7 +3249,7 @@ static bool bdrv_recurse_has_child(BlockDriverState *bs,
  * Adds a BlockDriverState to a simple queue for an atomic, transactional
  * reopen of multiple devices.
  *
- * bs_queue can either be an existing BlockReopenQueue that has had 
QSIMPLE_INIT
+ * bs_queue can either be an existing BlockReopenQueue that has had QTAILQ_INIT
  * already performed, or alternatively may be NULL a new BlockReopenQueue will
  * be created and initialized. This newly created BlockReopenQueue should be
  * passed back in for subsequent calls that are intended to be of the same
@@ -3290,7 +3290,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 if (bs_queue == NULL) {
 bs_queue = g_new0(BlockReopenQueue, 1);
-QSIMPLEQ_INIT(bs_queue);
+QTAILQ_INIT(bs_queue);
 }
 
 if (!options) {
@@ -3298,7 +3298,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 }
 
 /* Check if this BlockDriverState is already in the queue */
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 if (bs == bs_entry->state.bs) {
 break;
 }
@@ -3354,7 +3354,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 if (!bs_entry) {
 bs_entry = g_new0(BlockReopenQueueEntry, 1);
-QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+QTAILQ_INSERT_TAIL(bs_queue, bs_entry, entry);
 } else {
 qobject_unref(bs_entry->state.options);
 qobject_unref(bs_entry->state.explicit_options);
@@ -3455,7 +3455,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 
 assert(bs_queue != NULL);
 
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 assert(bs_entry->state.bs->quiesce_counter > 0);
 if (bdrv_reopen_prepare(_entry->state, bs_queue, errp)) {
 goto cleanup;
@@ -3463,7 +3463,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 bs_entry->prepared = true;
 }
 
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 BDRVReopenState *state = _entry->state;
 ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
   state->shared_perm, NULL, NULL, errp);
@@ -3489,13 +3489,13 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 /* If we reach this point, we have success and just need to apply the
  * changes
  */
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 bdrv_reopen_commit(_entry->state);
 }
 
 ret = 0;
 cleanup_perm:
-QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
 BDRVReopenState *state = _entry->state;
 
 if (!bs_entry->perms_checked) {
@@ -3512,7 +3512,7 @@ cleanup_perm:
 }
 }
 cleanup:
-QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
 if (ret) {
 if (bs_entry->prepared) {
 bdrv_reopen_abort(_entry->state);
@@ -3552,7 +3552,7 @@ static BlockReopenQueueEntry 

[PULL v3 03/19] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

It's more comfortable to not deal with local_err.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190920082543.23444-3-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/qcow2.h|  5 ++---
 include/block/block_int.h|  6 +++---
 include/block/dirty-bitmap.h |  5 ++---
 block/dirty-bitmap.c |  9 +
 block/qcow2-bitmap.c | 18 ++
 blockdev.c   |  7 +++
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index f51f478e34..08b4c15dc4 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -750,9 +750,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
   const char *name,
   uint32_t granularity,
   Error **errp);
-void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
-  const char *name,
-  Error **errp);
+int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char 
*name,
+ Error **errp);
 
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 05056b308a..6b511dd889 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -557,9 +557,9 @@ struct BlockDriver {
 const char *name,
 uint32_t granularity,
 Error **errp);
-void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
-const char *name,
-Error **errp);
+int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+   const char *name,
+   Error **errp);
 
 /**
  * Register/unregister a buffer for I/O. For example, when the driver is
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 4b4b731b46..07503b03b5 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, 
uint32_t flags,
 Error **errp);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
-void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp);
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+Error **errp);
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8f42015db9..d1ae2e1922 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState 
*bs)
  * not fail.
  * This function doesn't release corresponding BdrvDirtyBitmap.
  */
-void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp)
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+Error **errp)
 {
 if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
-bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
+return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
 }
+
+return 0;
 }
 
 bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b2487101ed..9821c1628f 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1404,9 +1404,8 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList 
*bm_list,
 return NULL;
 }
 
-void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
-  const char *name,
-  Error **errp)
+int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char 
*name,
+ Error **errp)
 {
 int ret;
 BDRVQcow2State *s = bs->opaque;
@@ -1416,18 +1415,19 @@ void 
qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
 if (s->nb_bitmaps == 0) {
 /* Absence of the bitmap is not an error: see explanation above
  * 

[PULL v3 08/19] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

bdrv_dirty_bitmap_next is always used in same pattern. So, split it
into _next and _first, instead of combining two functions into one and
add FOR_EACH_DIRTY_BITMAP macro.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190916141911.5255-5-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/block/dirty-bitmap.h   |  9 +++--
 block.c|  4 +---
 block/dirty-bitmap.c   | 11 +++
 block/qcow2-bitmap.c   |  8 ++--
 migration/block-dirty-bitmap.c |  4 +---
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 2f9b088e11..257f0f6704 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -96,8 +96,13 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap 
*bitmap);
 bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
-BdrvDirtyBitmap *bitmap);
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
+#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
+for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
+ bitmap = bdrv_dirty_bitmap_next(bitmap))
+
 char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
 int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
 uint64_t bytes);
diff --git a/block.c b/block.c
index d19a4781a3..5721441697 100644
--- a/block.c
+++ b/block.c
@@ -5390,9 +5390,7 @@ static void coroutine_fn 
bdrv_co_invalidate_cache(BlockDriverState *bs,
 }
 }
 
-for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
- bm = bdrv_dirty_bitmap_next(bs, bm))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bm) {
 bdrv_dirty_bitmap_skip_store(bm, false);
 }
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4e5c87a907..6065db8094 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -851,11 +851,14 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
*bs)
 return false;
 }
 
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
-BdrvDirtyBitmap *bitmap)
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs)
 {
-return bitmap == NULL ? QLIST_FIRST(>dirty_bitmaps) :
-QLIST_NEXT(bitmap, list);
+return QLIST_FIRST(>dirty_bitmaps);
+}
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap)
+{
+return QLIST_NEXT(bitmap, list);
 }
 
 char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 687087d2bc..99812b418b 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1488,9 +1488,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 }
 
 /* check constraints and names */
-for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 const char *name = bdrv_dirty_bitmap_name(bitmap);
 uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
 Qcow2Bitmap *bm;
@@ -1610,9 +1608,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error 
**errp)
 return -EINVAL;
 }
 
-for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
 bdrv_dirty_bitmap_set_readonly(bitmap, true);
 }
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 793f249aa5..7eafface61 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -283,9 +283,7 @@ static int init_dirty_bitmap_migration(void)
 for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
 const char *name = bdrv_get_device_or_node_name(bs);
 
-for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 if (!bdrv_dirty_bitmap_name(bitmap)) {
 continue;
 }
-- 
2.21.0

[PULL v3 11/19] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Reopening bitmaps to RW was broken prior to previous commit. Check that
it works now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190927122355.7344-4-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 tests/qemu-iotests/165 | 57 --
 tests/qemu-iotests/165.out |  4 +--
 2 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index 5650dc7c87..951ea011a2 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 os.remove(disk)
 
 def mkVm(self):
-return iotests.VM().add_drive(disk)
+return iotests.VM().add_drive(disk, opts='node-name=node0')
 
 def mkVmRo(self):
-return iotests.VM().add_drive(disk, opts='readonly=on')
+return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
 
 def getSha256(self):
 result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
@@ -102,6 +102,59 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 
 self.vm.shutdown()
 
+def test_reopen_rw(self):
+self.vm = self.mkVm()
+self.vm.launch()
+self.qmpAddBitmap()
+
+# Calculate hashes
+
+self.writeRegions(regions1)
+sha256_1 = self.getSha256()
+
+self.writeRegions(regions2)
+sha256_2 = self.getSha256()
+assert sha256_1 != sha256_2 # Otherwise, it's not very interesting.
+
+result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
+ name='bitmap0')
+self.assert_qmp(result, 'return', {})
+
+# Start with regions1
+
+self.writeRegions(regions1)
+assert sha256_1 == self.getSha256()
+
+self.vm.shutdown()
+
+self.vm = self.mkVmRo()
+self.vm.launch()
+
+assert sha256_1 == self.getSha256()
+
+# Check that we are in RO mode and can't modify bitmap.
+self.writeRegions(regions2)
+assert sha256_1 == self.getSha256()
+
+# Reopen to RW
+result = self.vm.qmp('x-blockdev-reopen', **{
+'node-name': 'node0',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': disk
+},
+'read-only': False
+})
+self.assert_qmp(result, 'return', {})
+
+# Check that bitmap is reopened to RW and we can write to it.
+self.writeRegions(regions2)
+assert sha256_2 == self.getSha256()
+
+self.vm.shutdown()
+
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2'],
  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
index ae1213e6f8..fbc63e62f8 100644
--- a/tests/qemu-iotests/165.out
+++ b/tests/qemu-iotests/165.out
@@ -1,5 +1,5 @@
-.
+..
 --
-Ran 1 tests
+Ran 2 tests
 
 OK
-- 
2.21.0

[PULL v3 06/19] block/dirty-bitmap: add bs link

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Add bs field to BdrvDirtyBitmap structure. Drop BlockDriverState
parameter from bitmap APIs where possible.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190916141911.5255-3-vsement...@virtuozzo.com
[Rebased on top of block-copy. --js]
Signed-off-by: John Snow 
---
 include/block/dirty-bitmap.h   | 14 +-
 block/backup.c |  8 
 block/block-copy.c |  2 +-
 block/dirty-bitmap.c   | 24 
 block/mirror.c |  4 ++--
 block/qcow2-bitmap.c   |  6 +++---
 blockdev.c |  6 +++---
 migration/block-dirty-bitmap.c |  7 +++
 migration/block.c  |  4 ++--
 9 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 973056778a..2f9b088e11 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -18,21 +18,18 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
   uint32_t granularity,
   const char *name,
   Error **errp);
-int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
-   BdrvDirtyBitmap *bitmap,
+int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap,
Error **errp);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
-BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap,
 Error **errp);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-   BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *bitmap,
Error **errp);
 void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 const char *name);
 int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
 Error **errp);
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
 int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
 Error **errp);
@@ -106,8 +103,7 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap 
*bitmap, uint64_t offset,
 uint64_t bytes);
 bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
uint64_t *offset, uint64_t *bytes);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
-  BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
   Error **errp);
 
 #endif
diff --git a/block/backup.c b/block/backup.c
index 46978c1785..dddcf77f53 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -98,13 +98,13 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, 
int ret)
  * We succeeded, or we always intended to sync the bitmap.
  * Delete this bitmap and install the child.
  */
-bm = bdrv_dirty_bitmap_abdicate(job->source_bs, job->sync_bitmap, 
NULL);
+bm = bdrv_dirty_bitmap_abdicate(job->sync_bitmap, NULL);
 } else {
 /*
  * We failed, or we never intended to sync the bitmap anyway.
  * Merge the successor back into the parent, keeping all data.
  */
-bm = bdrv_reclaim_dirty_bitmap(job->source_bs, job->sync_bitmap, NULL);
+bm = bdrv_reclaim_dirty_bitmap(job->sync_bitmap, NULL);
 }
 
 assert(bm);
@@ -402,7 +402,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 }
 
 /* Create a new bitmap, and freeze/disable this one. */
-if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
+if (bdrv_dirty_bitmap_create_successor(sync_bitmap, errp) < 0) {
 return NULL;
 }
 }
@@ -472,7 +472,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 
  error:
 if (sync_bitmap) {
-bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+bdrv_reclaim_dirty_bitmap(sync_bitmap, NULL);
 }
 if (job) {
 backup_clean(>common.job);
diff --git a/block/block-copy.c b/block/block-copy.c
index 0f76ea1e63..066e3a7274 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -60,7 +60,7 @@ void block_copy_state_free(BlockCopyState *s)
   

[PULL v3 12/19] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Firstly, no reason to optimize failure path. Then, function name is
ambiguous: it checks for readonly and similar things, but someone may
think that it will ignore normal bitmaps which was just unchanged, and
this is in bad relation with the fact that we should drop IN_USE flag
for unchanged bitmaps in the image.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190927122355.7344-5-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/block/dirty-bitmap.h |  1 -
 block/dirty-bitmap.c | 12 
 block/qcow2-bitmap.c | 23 +--
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 257f0f6704..958e7474fb 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -95,7 +95,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
-bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 
 BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 6065db8094..4bbb251b2c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -839,18 +839,6 @@ bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap 
*bitmap)
 return bitmap->inconsistent;
 }
 
-bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
-{
-BdrvDirtyBitmap *bm;
-QLIST_FOREACH(bm, >dirty_bitmaps, list) {
-if (bm->persistent && !bm->readonly && !bm->skip_store) {
-return true;
-}
-}
-
-return false;
-}
-
 BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs)
 {
 return QLIST_FIRST(>dirty_bitmaps);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 99812b418b..6dfc083548 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1464,16 +1464,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 Qcow2Bitmap *bm;
 QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;
 Qcow2BitmapTable *tb, *tb_next;
-
-if (!bdrv_has_changed_persistent_bitmaps(bs)) {
-/* nothing to do */
-return;
-}
-
-if (!can_write(bs)) {
-error_setg(errp, "No write access");
-return;
-}
+bool need_write = false;
 
 QSIMPLEQ_INIT(_tables);
 
@@ -1499,6 +1490,8 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 continue;
 }
 
+need_write = true;
+
 if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) {
 error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: 
",
   name);
@@ -1537,6 +1530,15 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 bm->dirty_bitmap = bitmap;
 }
 
+if (!need_write) {
+goto success;
+}
+
+if (!can_write(bs)) {
+error_setg(errp, "No write access");
+goto fail;
+}
+
 /* allocate clusters and store bitmaps */
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
 if (bm->dirty_bitmap == NULL) {
@@ -1578,6 +1580,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 bdrv_release_dirty_bitmap(bm->dirty_bitmap);
 }
 
+success:
 bitmap_list_free(bm_list);
 return;
 
-- 
2.21.0

[PULL v3 10/19] block: reverse order for reopen commits

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

It's needed to fix reopening qcow2 with bitmaps to RW. Currently it
can't work, as qcow2 needs write access to file child, to mark bitmaps
in-image with IN_USE flag. But usually children goes after parents in
reopen queue and file child is still RO on qcow2 reopen commit. Reverse
reopen order to fix it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Acked-by: Max Reitz 
Acked-by: John Snow 
Message-id: 20190927122355.7344-3-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 0347632c6c..cf312258a9 100644
--- a/block.c
+++ b/block.c
@@ -3486,10 +3486,16 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 bs_entry->perms_checked = true;
 }
 
-/* If we reach this point, we have success and just need to apply the
- * changes
+/*
+ * If we reach this point, we have success and just need to apply the
+ * changes.
+ *
+ * Reverse order is used to comfort qcow2 driver: on commit it need to 
write
+ * IN_USE flag to the image, to mark bitmaps in the image as invalid. But
+ * children are usually goes after parents in reopen-queue, so go from last
+ * to first element.
  */
-QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
 bdrv_reopen_commit(_entry->state);
 }
 
-- 
2.21.0

[PULL v3 05/19] block/dirty-bitmap: drop meta

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Drop meta bitmaps, as they are unused.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190916141911.5255-2-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/block/dirty-bitmap.h |  5 
 block/dirty-bitmap.c | 46 
 2 files changed, 51 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 07503b03b5..973056778a 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -18,9 +18,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
   uint32_t granularity,
   const char *name,
   Error **errp);
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-   int chunk_size);
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
@@ -54,7 +51,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  int64_t offset, int64_t bytes);
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
 
@@ -96,7 +92,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
 void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 03e0872b97..4ecf18d5df 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -31,7 +31,6 @@
 struct BdrvDirtyBitmap {
 QemuMutex *mutex;
 HBitmap *bitmap;/* Dirty bitmap implementation */
-HBitmap *meta;  /* Meta dirty bitmap */
 bool busy;  /* Bitmap is busy, it can't be used via QMP */
 BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
 char *name; /* Optional non-empty unique ID */
@@ -127,36 +126,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 return bitmap;
 }
 
-/* bdrv_create_meta_dirty_bitmap
- *
- * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. I.e.
- * when a dirty status bit in @bitmap is changed (either from reset to set or
- * the other way around), its respective meta dirty bitmap bit will be marked
- * dirty as well.
- *
- * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
- * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap
- * track.
- */
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-   int chunk_size)
-{
-assert(!bitmap->meta);
-qemu_mutex_lock(bitmap->mutex);
-bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
-   chunk_size * BITS_PER_BYTE);
-qemu_mutex_unlock(bitmap->mutex);
-}
-
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
-{
-assert(bitmap->meta);
-qemu_mutex_lock(bitmap->mutex);
-hbitmap_free_meta(bitmap->bitmap);
-bitmap->meta = NULL;
-qemu_mutex_unlock(bitmap->mutex);
-}
-
 int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
 {
 return bitmap->size;
@@ -320,7 +289,6 @@ static void 
bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 assert(!bitmap->active_iterators);
 assert(!bdrv_dirty_bitmap_busy(bitmap));
 assert(!bdrv_dirty_bitmap_has_successor(bitmap));
-assert(!bitmap->meta);
 QLIST_REMOVE(bitmap, list);
 hbitmap_free(bitmap->bitmap);
 g_free(bitmap->name);
@@ -666,15 +634,6 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap 
*bitmap)
 return iter;
 }
 
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap)
-{
-BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
-hbitmap_iter_init(>hbi, bitmap->meta, 0);
-iter->bitmap = bitmap;
-bitmap->active_iterators++;
-return iter;
-}
-
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
 {
 if (!iter) {
@@ -821,11 +780,6 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
 return hbitmap_count(bitmap->bitmap);
 }
 
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
-{
-return hbitmap_count(bitmap->meta);

[PULL v3 02/19] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

block/dirty-bitmap.c seems to be more appropriate for it and
bdrv_remove_persistent_dirty_bitmap already in it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190920082543.23444-2-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block.c  | 22 --
 block/dirty-bitmap.c | 22 ++
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 1946fc6f57..d19a4781a3 100644
--- a/block.c
+++ b/block.c
@@ -6582,25 +6582,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, 
BdrvChild *child, Error **errp)
 
 parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
 }
-
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
- uint32_t granularity, Error **errp)
-{
-BlockDriver *drv = bs->drv;
-
-if (!drv) {
-error_setg_errno(errp, ENOMEDIUM,
- "Can't store persistent bitmaps to %s",
- bdrv_get_device_or_node_name(bs));
-return false;
-}
-
-if (!drv->bdrv_can_store_new_dirty_bitmap) {
-error_setg_errno(errp, ENOTSUP,
- "Can't store persistent bitmaps to %s",
- bdrv_get_device_or_node_name(bs));
-return false;
-}
-
-return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
-}
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c..8f42015db9 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -464,6 +464,28 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState 
*bs,
 }
 }
 
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp)
+{
+BlockDriver *drv = bs->drv;
+
+if (!drv) {
+error_setg_errno(errp, ENOMEDIUM,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+return false;
+}
+
+if (!drv->bdrv_can_store_new_dirty_bitmap) {
+error_setg_errno(errp, ENOTSUP,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+return false;
+}
+
+return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+}
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
 bdrv_dirty_bitmap_lock(bitmap);
-- 
2.21.0

[PULL v3 00/19] Bitmaps patches

[John Snow]

The following changes since commit f22f553efffd083ff624be116726f843a39f1148:

  Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20191013' into 
staging (2019-10-17 16:48:56 +0100)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request

for you to fetch changes up to 3264ffced3d087bbe72d759639ef84fd5bd924cc:

  dirty-bitmaps: remove deprecated autoload parameter (2019-10-17 17:53:28 
-0400)


pull request



John Snow (2):
  MAINTAINERS: Add Vladimir as a reviewer for bitmaps
  dirty-bitmaps: remove deprecated autoload parameter

Vladimir Sementsov-Ogievskiy (17):
  util/hbitmap: strict hbitmap_reset
  block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c
  block/dirty-bitmap: return int from
bdrv_remove_persistent_dirty_bitmap
  block/qcow2: proper locking on bitmap add/remove paths
  block/dirty-bitmap: drop meta
  block/dirty-bitmap: add bs link
  block/dirty-bitmap: drop BdrvDirtyBitmap.mutex
  block/dirty-bitmap: refactor bdrv_dirty_bitmap_next
  block: switch reopen queue from QSIMPLEQ to QTAILQ
  block: reverse order for reopen commits
  iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
  block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps
  block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()
  block/qcow2-bitmap: do not remove bitmaps on reopen-ro
  iotests: add test 260 to check bitmap life after snapshot + commit
  block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
  qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

 qemu-deprecated.texi   |  20 ++-
 qapi/block-core.json   |   6 +-
 block/qcow2.h  |  19 +--
 include/block/block.h  |   2 +-
 include/block/block_int.h  |  20 +--
 include/block/dirty-bitmap.h   |  34 ++--
 include/qemu/hbitmap.h |   5 +
 block.c|  79 +++--
 block/backup.c |   8 +-
 block/block-copy.c |   2 +-
 block/dirty-bitmap.c   | 290 +++--
 block/mirror.c |   4 +-
 block/qcow2-bitmap.c   | 212 +++-
 block/qcow2.c  |  22 ++-
 blockdev.c |  40 ++---
 migration/block-dirty-bitmap.c |  11 +-
 migration/block.c  |   4 +-
 tests/test-hbitmap.c   |   2 +-
 util/hbitmap.c |   4 +
 MAINTAINERS|   3 +-
 tests/qemu-iotests/165 |  57 ++-
 tests/qemu-iotests/165.out |   4 +-
 tests/qemu-iotests/260 |  89 ++
 tests/qemu-iotests/260.out |  52 ++
 tests/qemu-iotests/group   |   1 +
 25 files changed, 623 insertions(+), 367 deletions(-)
 create mode 100755 tests/qemu-iotests/260
 create mode 100644 tests/qemu-iotests/260.out

-- 
2.21.0

[PULL v3 01/19] util/hbitmap: strict hbitmap_reset

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

hbitmap_reset has an unobvious property: it rounds requested region up.
It may provoke bugs, like in recently fixed write-blocking mode of
mirror: user calls reset on unaligned region, not keeping in mind that
there are possible unrelated dirty bytes, covered by rounded-up region
and information of this unrelated "dirtiness" will be lost.

Make hbitmap_reset strict: assert that arguments are aligned, allowing
only one exception when @start + @count == hb->orig_size. It's needed
to comfort users of hbitmap_next_dirty_area, which cares about
hb->orig_size.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Message-Id: <20190806152611.280389-1-vsement...@virtuozzo.com>
[Maintainer edit: Max's suggestions from on-list. --js]
[Maintainer edit: Eric's suggestion for aligned macro. --js]
Signed-off-by: John Snow 
---
 include/qemu/hbitmap.h | 5 +
 tests/test-hbitmap.c   | 2 +-
 util/hbitmap.c | 4 
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 4afbe6292e..1bf944ca3d 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -132,6 +132,11 @@ void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t 
count);
  * @count: Number of bits to reset.
  *
  * Reset a consecutive range of bits in an HBitmap.
+ * @start and @count must be aligned to bitmap granularity. The only exception
+ * is resetting the tail of the bitmap: @count may be equal to hb->orig_size -
+ * @start, in this case @count may be not aligned. The sum of @start + @count 
is
+ * allowed to be greater than hb->orig_size, but only if @start < hb->orig_size
+ * and @start + @count = ALIGN_UP(hb->orig_size, granularity).
  */
 void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count);
 
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index eed5d288cb..e1f867085f 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -423,7 +423,7 @@ static void test_hbitmap_granularity(TestHBitmapData *data,
 hbitmap_test_check(data, 0);
 hbitmap_test_set(data, 0, 3);
 g_assert_cmpint(hbitmap_count(data->hb), ==, 4);
-hbitmap_test_reset(data, 0, 1);
+hbitmap_test_reset(data, 0, 2);
 g_assert_cmpint(hbitmap_count(data->hb), ==, 2);
 }
 
diff --git a/util/hbitmap.c b/util/hbitmap.c
index fd44c897ab..66db87c6ff 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -476,6 +476,10 @@ void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t 
count)
 /* Compute range in the last layer.  */
 uint64_t first;
 uint64_t last = start + count - 1;
+uint64_t gran = 1ULL << hb->granularity;
+
+assert(QEMU_IS_ALIGNED(start, gran));
+assert(QEMU_IS_ALIGNED(count, gran) || (start + count == hb->orig_size));
 
 trace_hbitmap_reset(hb, start, count,
 start >> hb->granularity, last >> hb->granularity);
-- 
2.21.0

Re: [PATCH 00/10] image-fuzzer: Port to Python 3

[Eduardo Habkost]

On Thu, Oct 17, 2019 at 05:11:29PM -0400, John Snow wrote:
> 
> 
> On 10/16/19 3:24 PM, Eduardo Habkost wrote:
> > This series ports image-fuzzer to Python 3.
> > 
> > Eduardo Habkost (10):
> >   image-fuzzer: Open image files in binary mode
> >   image-fuzzer: Write bytes instead of string to image file
> >   image-fuzzer: Explicitly use integer division operator
> >   image-fuzzer: Use io.StringIO
> >   image-fuzzer: Use %r for all fiels at Field.__repr__()
> >   image-fuzzer: Return bytes objects on string fuzzing functions
> >   image-fuzzer: Use bytes constant for field values
> >   image-fuzzer: Encode file name and file format to bytes
> >   image-fuzzer: Run using python3
> >   image-fuzzer: Use errors parameter of subprocess.Popen()
> > 
> >  tests/image-fuzzer/qcow2/__init__.py |  1 -
> >  tests/image-fuzzer/qcow2/fuzz.py | 54 +-
> >  tests/image-fuzzer/qcow2/layout.py   | 57 ++--
> >  tests/image-fuzzer/runner.py | 12 +++---
> >  4 files changed, 61 insertions(+), 63 deletions(-)
> > 
> 
> When I gave my try at converting this to python3 I noticed that the
> "except OSError as e" segments used e[1] in a way that was not seemingly
> supported.
> 
> Did you fix that in this series or did I miss it?

Good catch, I hadn't noticed that.  I didn't fix it.

-- 
Eduardo

Re: [PATCH 00/10] image-fuzzer: Port to Python 3

[John Snow]

On 10/16/19 3:24 PM, Eduardo Habkost wrote:
> This series ports image-fuzzer to Python 3.
> 
> Eduardo Habkost (10):
>   image-fuzzer: Open image files in binary mode
>   image-fuzzer: Write bytes instead of string to image file
>   image-fuzzer: Explicitly use integer division operator
>   image-fuzzer: Use io.StringIO
>   image-fuzzer: Use %r for all fiels at Field.__repr__()
>   image-fuzzer: Return bytes objects on string fuzzing functions
>   image-fuzzer: Use bytes constant for field values
>   image-fuzzer: Encode file name and file format to bytes
>   image-fuzzer: Run using python3
>   image-fuzzer: Use errors parameter of subprocess.Popen()
> 
>  tests/image-fuzzer/qcow2/__init__.py |  1 -
>  tests/image-fuzzer/qcow2/fuzz.py | 54 +-
>  tests/image-fuzzer/qcow2/layout.py   | 57 ++--
>  tests/image-fuzzer/runner.py | 12 +++---
>  4 files changed, 61 insertions(+), 63 deletions(-)
> 

When I gave my try at converting this to python3 I noticed that the
"except OSError as e" segments used e[1] in a way that was not seemingly
supported.

Did you fix that in this series or did I miss it?

--js

Re: [PATCH 05/10] image-fuzzer: Use %r for all fiels at Field.__repr__()

[John Snow]

"fields" in the commit message.

On 10/16/19 3:24 PM, Eduardo Habkost wrote:
> This makes the formatting code simpler, and safer if we change
> the type of self.value from str to bytes.
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  tests/image-fuzzer/qcow2/layout.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/image-fuzzer/qcow2/layout.py 
> b/tests/image-fuzzer/qcow2/layout.py
> index 6501c9fd4b..0adcbd448d 100644
> --- a/tests/image-fuzzer/qcow2/layout.py
> +++ b/tests/image-fuzzer/qcow2/layout.py
> @@ -53,8 +53,8 @@ class Field(object):
>  return iter([self.fmt, self.offset, self.value, self.name])
>  
>  def __repr__(self):
> -return "Field(fmt='%s', offset=%d, value=%s, name=%s)" % \
> -(self.fmt, self.offset, str(self.value), self.name)
> +return "Field(fmt=%r, offset=%r, value=%r, name=%r)" % \
> +(self.fmt, self.offset, self.value, self.name)
>  
>  
>  class FieldsList(object):
> 

Re: [PULL v2 00/19] Bitmaps patches

[John Snow]

On 10/17/19 7:07 AM, Peter Maydell wrote:
> On Mon, 14 Oct 2019 at 20:29, John Snow  wrote:
>>
>> The following changes since commit c760cb77e511eb05094df67c1b30029a952efa35:
>>
>>   Merge remote-tracking branch 
>> 'remotes/dgilbert/tags/pull-migration-20191011a' into staging (2019-10-14 
>> 16:09:52 +0100)
>>
>> are available in the Git repository at:
>>
>>   https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>>
>> for you to fetch changes up to b2ca29ee390743c42a6062d44ee3b10fb51f9fa6:
>>
>>   dirty-bitmaps: remove deprecated autoload parameter (2019-10-14 15:28:17 
>> -0400)
>>
>> 
>> Pull request
>>
>> 
> 
> Hi; this pullreq fails on some hosts on the newly added iotest 260
> with an "AF_UNIX path too long" error:
> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg04063.html
> 
> Max tells me that this is a known problem (a fix is in the works)
> but that for the moment we've chosen not to add any python based
> tests to the 'auto' group, so that this AF_UNIX issue doesn't
> affect "make check". Could you respin the pullreq with the
> new iotest(s?) not in the 'auto' group, please?
> 
> thanks
> -- PMM
> 

Anguish and turmoil.

Alright, V3 coming up.

Re: [PATCH v4 4/4] tests/qemu-iotests: add case for block-stream compress

[Vladimir Sementsov-Ogievskiy]

16.10.2019 19:28, Andrey Shinkevich wrote:
> Add a case to the iotest #030 that tests the 'compress' option for a
> block-stream job.
> 
> Signed-off-by: Andrey Shinkevich 
> ---
>   tests/qemu-iotests/030 | 51 
> +-
>   tests/qemu-iotests/030.out |  4 ++--
>   2 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index f3766f2..f0f0e26 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -21,7 +21,8 @@
>   import time
>   import os
>   import iotests
> -from iotests import qemu_img, qemu_io
> +from iotests import qemu_img, qemu_io, qemu_img_pipe
> +import json
>   
>   backing_img = os.path.join(iotests.test_dir, 'backing.img')
>   mid_img = os.path.join(iotests.test_dir, 'mid.img')
> @@ -956,6 +957,54 @@ class TestSetSpeed(iotests.QMPTestCase):
>   
>   self.cancel_and_wait(resume=True)
>   
> +class TestCompressed(iotests.QMPTestCase):
> +test_img_init_size = 0
> +
> +def setUp(self):
> +qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
> +qemu_img('create', '-f', iotests.imgfmt, '-o',
> + 'backing_file=%s' % backing_img, mid_img)
> +qemu_img('create', '-f', iotests.imgfmt, '-o',
> + 'backing_file=%s' % mid_img, test_img)
> +qemu_io('-c', 'write -P 0x1 0 512k', backing_img)
> +top = json.loads(qemu_img_pipe('info', '--output=json', test_img))
> +self.test_img_init_size = top['actual-size']
> +self.vm = iotests.VM().add_drive(test_img, "backing.node-name=mid," +
> + "backing.backing.node-name=base," +

base node-name is unused actually

> + "compress=on")
> +self.vm.launch()
> +
> +def tearDown(self):
> +self.vm.shutdown()
> +os.remove(test_img)
> +os.remove(mid_img)
> +os.remove(backing_img)
> +
> +def test_stream_compress(self):
> +self.assert_no_active_block_jobs()
> +
> +result = self.vm.qmp('block-stream', device='mid', 
> job_id='stream-mid')
> +self.assert_qmp(result, 'return', {})
> +
> +self.wait_until_completed(drive='stream-mid')
> +# Remove other 'JOB_STATUS_CHANGE' events for the job 'stream-mid'
> +self.vm.get_qmp_events(wait=True)

Hmm, I think it's unsafe.. One more event may be still in pipe...

I think, better is just do
self.vm.wait_event(name='BLOCK_JOB_COMPLETED), and ignore JOB_STATUS_CHANGE 
events.

> +
> +result = self.vm.qmp('block-stream', device='drive0',
> + job_id='stream-top')
> +self.assert_qmp(result, 'return', {})
> +
> +self.wait_until_completed(drive='stream-top')
> +self.vm.shutdown()
> +
> +top = json.loads(qemu_img_pipe('info', '--output=json', test_img))
> +mid = json.loads(qemu_img_pipe('info', '--output=json', mid_img))
> +base = json.loads(qemu_img_pipe('info', '--output=json', 
> backing_img))
> +
> +self.assertEqual(mid['actual-size'], base['actual-size'])
> +self.assertLess(top['actual-size'], mid['actual-size'])
> +self.assertLess(self.test_img_init_size, top['actual-size'])

I think last assertion is covered by previous two.

And I'm still unsure about using actual-size. Direct checking number of 
compressed
clusters by qemu-img check maybe better.

> +
>   if __name__ == '__main__':
>   iotests.main(supported_fmts=['qcow2', 'qed'],
>supported_protocols=['file'])
> diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
> index 6d9bee1..af8dac1 100644
> --- a/tests/qemu-iotests/030.out
> +++ b/tests/qemu-iotests/030.out
> @@ -1,5 +1,5 @@
> -...
> +
>   --
> -Ran 27 tests
> +Ran 28 tests
>   
>   OK
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 2/4] qcow2: Allow writing compressed data of multiple clusters

[Vladimir Sementsov-Ogievskiy]

16.10.2019 19:28, Andrey Shinkevich wrote:
> QEMU currently supports writing compressed data of the size equal to
> one cluster. This patch allows writing QCOW2 compressed data that
> exceed one cluster. Now, we split buffered data into separate clusters
> and write them compressed using the existing functionality.
> 
> Suggested-by: Pavel Butsykin 
> Signed-off-by: Andrey Shinkevich 
> ---
>   block/qcow2.c | 102 
> ++
>   1 file changed, 75 insertions(+), 27 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6b29e16..9a85d73 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4156,10 +4156,8 @@ fail:
>   return ret;
>   }
>   
> -/* XXX: put compressed sectors first, then all the cluster aligned
> -   tables to avoid losing bytes in alignment */
>   static coroutine_fn int
> -qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
> +qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
>uint64_t offset, uint64_t bytes,
>QEMUIOVector *qiov, size_t qiov_offset)
>   {
> @@ -4169,32 +4167,11 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
>   uint8_t *buf, *out_buf;
>   uint64_t cluster_offset;
>   
> -if (has_data_file(bs)) {
> -return -ENOTSUP;
> -}
> -
> -if (bytes == 0) {
> -/* align end of file to a sector boundary to ease reading with
> -   sector based I/Os */
> -int64_t len = bdrv_getlength(bs->file->bs);
> -if (len < 0) {
> -return len;
> -}
> -return bdrv_co_truncate(bs->file, len, PREALLOC_MODE_OFF, NULL);
> -}
> -
> -if (offset_into_cluster(s, offset)) {
> -return -EINVAL;
> -}
> +assert(bytes == s->cluster_size || (bytes < s->cluster_size &&
> +   (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS)));
>   
>   buf = qemu_blockalign(bs, s->cluster_size);
> -if (bytes != s->cluster_size) {
> -if (bytes > s->cluster_size ||
> -offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS)
> -{
> -qemu_vfree(buf);
> -return -EINVAL;
> -}
> +if (bytes < s->cluster_size) {
>   /* Zero-pad last write if image size is not cluster aligned */
>   memset(buf + bytes, 0, s->cluster_size - bytes);
>   }
> @@ -4243,6 +4220,77 @@ fail:
>   return ret;
>   }
>   
> +static coroutine_fn int qcow2_co_pwritev_compressed_task_entry(AioTask *task)
> +{
> +Qcow2AioTask *t = container_of(task, Qcow2AioTask, task);
> +
> +assert(!t->cluster_type && !t->l2meta);
> +
> +return qcow2_co_pwritev_compressed_task(t->bs, t->offset, t->bytes, 
> t->qiov,
> +t->qiov_offset);
> +}
> +
> +/*
> + * XXX: put compressed sectors first, then all the cluster aligned
> +   tables to avoid losing bytes in alignment

missed asterisk

> + */
> +static coroutine_fn int
> +qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
> + uint64_t offset, uint64_t bytes,
> + QEMUIOVector *qiov, size_t qiov_offset)
> +{
> +BDRVQcow2State *s = bs->opaque;
> +AioTaskPool *aio = NULL;
> +int ret;
> +
> +if (has_data_file(bs)) {
> +return -ENOTSUP;
> +}
> +
> +if (bytes == 0) {
> +/*
> + * align end of file to a sector boundary to ease reading with
> + * sector based I/Os
> + */
> +int64_t len = bdrv_getlength(bs->file->bs);
> +if (len < 0) {
> +return len;
> +}
> +return bdrv_co_truncate(bs->file, len, PREALLOC_MODE_OFF, NULL);
> +}
> +
> +if (offset_into_cluster(s, offset)) {
> +return -EINVAL;
> +}
> +
> +while (bytes && aio_task_pool_status(aio) == 0) {
> +uint32_t chunk_size = MIN(bytes, s->cluster_size);

Hmm. cluster_size is int. bytes is uint64_t. qcow2_add_task argument type
is uint64_t. I think better to choose from these types.. Abd, I'd prefer to
use uint64_t for chunk_size.. But, uint32_t should work too, of course.

> +
> +if (!aio && chunk_size != bytes) {
> +aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
> +}
> +
> +ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
> + 0, 0, offset, chunk_size, qiov, qiov_offset, 
> NULL);
> +if (ret < 0) {
> +break;
> +}
> +qiov_offset += chunk_size;
> +offset += chunk_size;
> +bytes -= chunk_size;
> +}
> +
> +if (aio) {
> +aio_task_pool_wait_all(aio);
> +if (ret == 0) {
> +ret = aio_task_pool_status(aio);
> +}
> +g_free(aio);
> +}
> +
> +return ret;
> +}
> +
>   static int coroutine_fn
>   qcow2_co_preadv_compressed(BlockDriverState *bs,
>  uint64_t 

Re: [PATCH v4 3/4] tests/qemu-iotests: add case to write compressed data of multiple clusters

[Vladimir Sementsov-Ogievskiy]

16.10.2019 19:28, Andrey Shinkevich wrote:
> Add the test case to the iotest #214 that checks possibility of writing
> compressed data of more than one cluster size.
> 
> Signed-off-by: Andrey Shinkevich 
> ---
>   tests/qemu-iotests/214 | 35 +++
>   tests/qemu-iotests/214.out | 15 +++
>   2 files changed, 50 insertions(+)
> 
> diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
> index 21ec8a2..0003dc2 100755
> --- a/tests/qemu-iotests/214
> +++ b/tests/qemu-iotests/214
> @@ -89,6 +89,41 @@ _check_test_img -r all
>   $QEMU_IO -c "read  -P 0x11  0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | 
> _filter_testdir
>   $QEMU_IO -c "read  -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | 
> _filter_testdir
>   
> +echo
> +echo "=== Write compressed data of multiple clusters ==="
> +echo
> +cluster_size=0x1
> +_make_test_img 2M -o cluster_size=$cluster_size
> +
> +echo "Uncompressed data:"
> +let data_size="8 * $cluster_size"
> +$QEMU_IO -c "write -P 0xaa 0 $data_size" "$TEST_IMG" \
> + 2>&1 | _filter_qemu_io | _filter_testdir
> +$QEMU_IMG info "$TEST_IMG" | sed -n '/disk size:/ s/^ *//p'

hmm, I'm afraid, "disk size" is unstable thing for iotests. Consider
backporting: other size of metadata in qcow2 image, and we'll have to
fix this test often..

If you want to check, that clusters are compressed, you may use
qemu-img check (with or without --output=json), it shows amount of
compressed clusters.

> +
> +_make_test_img 2M -o cluster_size=$cluster_size
> +let data_size="3 * $cluster_size + ($cluster_size >> 1)"

I'd be a bit more happy with $cluster_size / 2, still, happiness and
bash scripts are incompatible anyway.

> +# Set compress=on. That will align the written data
> +# by the cluster size and will write them compressed.
> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
> +$QEMU_IO -c "write -P 0xbb 0 $data_size" --image-opts \
> + driver=$IMGFMT,compress=on,file.filename=$TEST_IMG \
> + 2>&1 | _filter_qemu_io | _filter_testdir
> +
> +let offset="4 * $cluster_size"
> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
> +$QEMU_IO -c "write -P 0xcc $offset $data_size" "json:{\
> +'driver': '$IMGFMT',
> +'file': {
> +'driver': 'file',
> +'filename': '$TEST_IMG'
> +},
> +'compress': true
> +}" | _filter_qemu_io | _filter_testdir
> +

It would be good to add case with unaligned offset as well. And, maybe,
check that we don't rewrite existing data in partial clusters when writing
unaligned compressed data over it.

> +echo "After the multiple cluster data have been written compressed,"
> +$QEMU_IMG info "$TEST_IMG" | sed -n '/disk size:/ s/^ *//p'
> +
>   # success, all done
>   echo '*** done'
>   rm -f $seq.full
> diff --git a/tests/qemu-iotests/214.out b/tests/qemu-iotests/214.out
> index 0fcd8dc..09a2e9a 100644
> --- a/tests/qemu-iotests/214.out
> +++ b/tests/qemu-iotests/214.out
> @@ -32,4 +32,19 @@ read 4194304/4194304 bytes at offset 0
>   4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   read 4194304/4194304 bytes at offset 4194304
>   4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +=== Write compressed data of multiple clusters ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
> +Uncompressed data:
> +wrote 524288/524288 bytes at offset 0
> +512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +disk size: 772 KiB
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
> +wrote 229376/229376 bytes at offset 0
> +224 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 229376/229376 bytes at offset 262144
> +224 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +After the multiple cluster data have been written compressed,
> +disk size: 268 KiB
>   *** done
> 


-- 
Best regards,
Vladimir

Re: iotest failure -- test possibly not using sufficiently unique temp filename?

[Peter Maydell]

On Fri, 27 Sep 2019 at 17:44, Max Reitz  wrote:
>
> On 27.09.19 18:39, Peter Maydell wrote:
> > Hi; I just saw this iotest failure (on an s390x box, as it happens):
> >
> >   TESTiotest-qcow2: 130 [fail]
> > QEMU  --
> > "/home/linux1/qemu/build/all/tests/qemu-iotests/../../s390x-softmmu/qemu-system-s390x"
> > -nodefaults -display none -machine accel=qtest
> > QEMU_IMG  -- 
> > "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-img"
> > QEMU_IO   --
> > "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-io"
> > --cache writeback -f qcow2
> > QEMU_NBD  -- 
> > "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-nbd"
> > IMGFMT-- qcow2 (compat=1.1)
> > IMGPROTO  -- file
> > PLATFORM  -- Linux/s390x lxub05 4.15.0-58-generic
> > TEST_DIR  -- /home/linux1/qemu/build/all/tests/qemu-iotests/scratch
> > SOCKET_SCM_HELPER --
> > /home/linux1/qemu/build/all/tests/qemu-iotests/socket_scm_helper
> >
> > --- /home/linux1/qemu/tests/qemu-iotests/130.out2019-05-10
> > 12:27:16.948075733 -0400
> > +++ /home/linux1/qemu/build/all/tests/qemu-iotests/130.out.bad
> > 2019-09-27 12:01:23.649722655 -0400
> > @@ -18,20 +18,22 @@
> >  QEMU X.Y.Z monitor - type 'help' for more information
> >  (qemu) commit testdisk
> >  (qemu)
> > -image: TEST_DIR/t.IMGFMT
> > -file format: IMGFMT
> > -virtual size: 64 MiB (67108864 bytes)
> > -backing file: TEST_DIR/t.IMGFMT.orig
> > -backing file format: raw
> > +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared "write" 
> > lock
> > +Is another process using the image [TEST_DIR/t.IMGFMT]?
> >
> >  === Marking image dirty (lazy refcounts) ===
> >
> > +qemu-img: TEST_DIR/t.IMGFMT: Failed to get "write" lock
> > +Is another process using the image [TEST_DIR/t.IMGFMT]?
> >  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> > -wrote 4096/4096 bytes at offset 0
> > -4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > +qemu-io: can't open device
> > /home/linux1/qemu/build/all/tests/qemu-iotests/scratch/t.qcow2: Failed
> > to get "write" lock
> > +Is another process using the image
> > [/home/linux1/qemu/build/all/tests/qemu-iotests/scratch/t.qcow2]?
> > +no file open, try 'help open'
> >  image: TEST_DIR/t.IMGFMT
> >  file format: IMGFMT
> >  virtual size: 64 MiB (67108864 bytes)
> > +backing file: TEST_DIR/t.IMGFMT.orig
> > +backing file format: raw
> >  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> > backing_file=TEST_DIR/t.IMGFMT.orig backing_fmt=raw
> >  wrote 4096/4096 bytes at offset 0
> >  4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >
> >
> >
> > This looks suspiciously like the test isn't using a unique
> > filename for its disk image: "qemu-iotests/scratch/t.qcow2"
> > in the build directory, and so perhaps it has collided with
> > another iotest ?
> >
> > If we run 'make check' with a -j option do the
> > iotests all get run serially anyway, or do they run in
> > parallel against each other ?
>
> As far as I know, all iotests are executed serially.  Anything else
> would not work with the same scratch directory.
>
> The only thing I suspect is that some tool has been accidentally left
> running by some previous test that still accesses its own image.  But I
> don’t know.

Just saw this one again with the same iotest 130 on the same
s390 box; only difference is that the log this time around
has the first part where qemu-img fails, but not the second part
where qemu-io fails:

--- /home/linux1/qemu/tests/qemu-iotests/130.out2019-05-10
12:27:16.948075733 -0400
+++ /home/linux1/qemu/build/all/tests/qemu-iotests/130.out.bad
2019-10-17 11:56:43.450750873 -0400
@@ -18,11 +18,8 @@
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) commit testdisk
 (qemu)
-image: TEST_DIR/t.IMGFMT
-file format: IMGFMT
-virtual size: 64 MiB (67108864 bytes)
-backing file: TEST_DIR/t.IMGFMT.orig
-backing file format: raw
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared "write" lock
+Is another process using the image [TEST_DIR/t.IMGFMT]?

 === Marking image dirty (lazy refcounts) ===

On the host machine there don't seem to be any stray
processes which might have held the file open, and
indeed the file doesn't exist at all, so it got removed
by some cleanup or other.

thanks
-- PMM

Re: [PATCH v2 21/23] iotests/240: Create socket in $SOCK_DIR

[Thomas Huth]

On 17/10/2019 15.31, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> ---
>  tests/qemu-iotests/240 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v2 20/23] iotests/223: Create socket in $SOCK_DIR

[Thomas Huth]

On 17/10/2019 15.31, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> ---
>  tests/qemu-iotests/223 | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v4 1/4] block: support compressed write at generic layer

[Vladimir Sementsov-Ogievskiy]

16.10.2019 19:28, Andrey Shinkevich wrote:
> To inform the block layer about writing all the data compressed, we
> introduce the 'compress' command line option. Based on that option, the
> written data will be aligned by the cluster size at the generic layer.
> 
> Suggested-by: Roman Kagan 
> Suggested-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Andrey Shinkevich 
> ---
>   block.c   | 20 +++-
>   block/io.c| 14 ++
>   block/qcow2.c |  4 
>   blockdev.c|  9 -
>   include/block/block.h |  1 +
>   include/block/block_int.h |  2 ++
>   qapi/block-core.json  |  6 +-
>   qemu-options.hx   |  6 --
>   8 files changed, 53 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1946fc6..a674920 100644
> --- a/block.c
> +++ b/block.c
> @@ -1418,6 +1418,11 @@ QemuOptsList bdrv_runtime_opts = {
>   .type = QEMU_OPT_BOOL,
>   .help = "always accept other writers (default: off)",
>   },
> +{
> +.name = BDRV_OPT_COMPRESS,
> +.type = QEMU_OPT_BOOL,
> +.help = "compress all writes to the image (default: off)",
> +},
>   { /* end of list */ }
>   },
>   };
> @@ -1545,6 +1550,14 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BlockBackend *file,
>   }
>   pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->filename);
>   
> +if (bs->all_write_compressed && !drv->bdrv_co_pwritev_compressed_part) {
> +error_setg(errp, "Compression is not supported for the driver '%s'",
> +   drv->format_name);
> +bs->all_write_compressed = false;
> +ret = -ENOTSUP;
> +goto fail_opts;
> +}
> +
>   /* Open the image, either directly or using a protocol */
>   open_flags = bdrv_open_flags(bs, bs->open_flags);
>   node_name = qemu_opt_get(opts, "node-name");
> @@ -2983,6 +2996,11 @@ static BlockDriverState *bdrv_open_inherit(const char 
> *filename,
>   flags &= ~BDRV_O_RDWR;
>   }
>   
> +if (!g_strcmp0(qdict_get_try_str(options, BDRV_OPT_COMPRESS), "on") ||
> +qdict_get_try_bool(options, BDRV_OPT_COMPRESS, false)) {
> +bs->all_write_compressed = true;
> +}
> +
>   if (flags & BDRV_O_SNAPSHOT) {
>   snapshot_options = qdict_new();
>   bdrv_temp_snapshot_options(_flags, snapshot_options,
> @@ -3208,7 +3226,7 @@ static int bdrv_reset_options_allowed(BlockDriverState 
> *bs,
>* in bdrv_reopen_prepare() so they can be left out of @new_opts */
>   const char *const common_options[] = {
>   "node-name", "discard", "cache.direct", "cache.no-flush",
> -"read-only", "auto-read-only", "detect-zeroes", NULL
> +"read-only", "auto-read-only", "detect-zeroes", "compress", NULL
>   };
>   
>   for (e = qdict_first(bs->options); e; e = qdict_next(bs->options, e)) {
> diff --git a/block/io.c b/block/io.c
> index f0b86c1..3743a13 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1360,9 +1360,15 @@ static int coroutine_fn 
> bdrv_co_do_copy_on_readv(BdrvChild *child,
>   /* This does not change the data on the disk, it is not
>* necessary to flush even in cache=writethrough mode.
>*/
> -ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
> -  _qiov, 0,
> -  BDRV_REQ_WRITE_UNCHANGED);
> +if (bs->all_write_compressed) {
> +ret = bdrv_driver_pwritev_compressed(bs, cluster_offset,
> + pnum, _qiov,
> + qiov_offset);

last argument must be 0, we are using local_qiov, so, it's qiov_offset is 0.

> +} else {
> +ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
> +  _qiov, 0,
> +  BDRV_REQ_WRITE_UNCHANGED);
> +}
>   }
>   
>   if (ret < 0) {
> @@ -1954,7 +1960,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
> *child,
>   } else if (flags & BDRV_REQ_ZERO_WRITE) {
>   bdrv_debug_event(bs, BLKDBG_PWRITEV_ZERO);
>   ret = bdrv_co_do_pwrite_zeroes(bs, offset, bytes, flags);
> -} else if (flags & BDRV_REQ_WRITE_COMPRESSED) {
> +} else if (flags & BDRV_REQ_WRITE_COMPRESSED || 
> bs->all_write_compressed) {
>   ret = bdrv_driver_pwritev_compressed(bs, offset, bytes,
>qiov, qiov_offset);
>   } else if (bytes <= max_transfer) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7961c05..6b29e16 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1787,6 +1787,10 @@ static void 

Re: [PATCH v2 15/23] iotests/201: Create socket in $SOCK_DIR

[Thomas Huth]

On 17/10/2019 15.31, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> ---
>  tests/qemu-iotests/201 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/201 b/tests/qemu-iotests/201
> index 7abf740fe4..86fa37e714 100755
> --- a/tests/qemu-iotests/201
> +++ b/tests/qemu-iotests/201
> @@ -24,7 +24,7 @@ echo "QA output created by $seq"
>  
>  status=1 # failure is the default!
>  
> -MIG_SOCKET="${TEST_DIR}/migrate"
> +MIG_SOCKET="${SOCK_DIR}/migrate"
>  
>  # get standard environment, filters and checks
>  . ./common.rc
> 

Reviewed-by: Thomas Huth 

Re: [PATCH v2 05/23] iotests: Let common.nbd create socket in $SOCK_DIR

[Thomas Huth]

On 17/10/2019 15.31, Max Reitz wrote:
> In addition, drop the nbd_unix_socket assignment in 241 because it does
> not really do anything.

Right, common.nbd is included afterwards, so it gets overwritten.

Reviewed-by: Thomas Huth 

Re: [PATCH v2 23/23] iotests: Drop TEST_DIR filter from _filter_nbd

[Thomas Huth]

On 17/10/2019 15.31, Max Reitz wrote:
> Sockets should be placed into $SOCK_DIR instead of $TEST_DIR, so remove
> the $TEST_DIR filter from _filter_nbd.
> 
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> ---
>  tests/qemu-iotests/common.filter | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/common.filter 
> b/tests/qemu-iotests/common.filter
> index 0ee6042524..f870e00e44 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -221,7 +221,6 @@ _filter_nbd()
>  # Filter out the TCP port number since this changes between runs.
>  $SED -e '/nbd\/.*\.c:/d' \
>  -e 's#127\.0\.0\.1:[0-9]*#127.0.0.1:PORT#g' \
> --e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \
>  -e "s#?socket=$SOCK_DIR#?socket=SOCK_DIR#g" \
>  -e 's#\(foo\|PORT/\?\|.sock\): Failed to .*$#\1#'
>  }
> 

Reviewed-by: Thomas Huth 

Re: [PATCH v2 22/23] iotests/267: Create socket in $SOCK_DIR

[Thomas Huth]

On 17/10/2019 15.31, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> ---
>  tests/qemu-iotests/267 | 4 ++--
>  tests/qemu-iotests/267.out | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v2 13/23] iotests/192: Create socket in $SOCK_DIR

[Thomas Huth]

On 17/10/2019 15.31, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> ---
>  tests/qemu-iotests/192 | 4 ++--
>  tests/qemu-iotests/192.out | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v2 16/23] iotests/205: Create socket in $SOCK_DIR

[Thomas Huth]

On 17/10/2019 15.31, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> ---
>  tests/qemu-iotests/205 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/205 b/tests/qemu-iotests/205
> index 76f6c5fa2b..4bb2c21e8b 100755
> --- a/tests/qemu-iotests/205
> +++ b/tests/qemu-iotests/205
> @@ -24,7 +24,7 @@ import iotests
>  import time
>  from iotests import qemu_img_create, qemu_io, filter_qemu_io, 
> QemuIoInteractive
>  
> -nbd_sock = os.path.join(iotests.test_dir, 'nbd_sock')
> +nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock')
>  nbd_uri = 'nbd+unix:///exp?socket=' + nbd_sock
>  disk = os.path.join(iotests.test_dir, 'disk')

Reviewed-by: Thomas Huth 

Re: [PATCH v2 04/23] iotests: Filter $SOCK_DIR

[Thomas Huth]

On 17/10/2019 15.31, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> ---
>  tests/qemu-iotests/common.filter | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v2 00/23] iotests: Add and use $SOCK_DIR

[Eric Blake]

On 10/17/19 8:31 AM, Max Reitz wrote:

Hi,

Perhaps the main reason we cannot run important tests such as 041 in CI
is that when they care Unix sockets in $TEST_DIR, the path may become
too long to connect to them.

To get by this problem, this series lets the check script create a new
temporary directory (mktemp -d) and then makes the iotests use it for
all Unix sockets.


v2:
- Patch 1: Use mkdir -p


I just thought of another potential concern there - what do you think of 
my response there?  Depending on how that thread goes, it may work to 
just amend the commit or take it as-is without having to go to a full v3 
posting...



- Patches 4/23: Only add the $SOCK_DIR replacement line in patch 4 and
 only drop the $TEST_DIR line in patch 23
   (Took Eric’s R-b on both because that’s how I interpreted his
   comments)


Yes, looks like you handled that well.  My R-b is okay there.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 01/23] iotests: Introduce $SOCK_DIR

[Eric Blake]

On 10/17/19 8:31 AM, Max Reitz wrote:

Unix sockets generally have a maximum path length.  Depending on your
$TEST_DIR, it may be exceeded and then all tests that create and use
Unix sockets there may fail.

Circumvent this by adding a new scratch directory specifically for
Unix socket files.  It defaults to a temporary directory (mktemp -d)
that is completely removed after the iotests are done.

(By default, mktemp -d creates a /tmp/tmp.XX directory, which
should be short enough for our use cases.)

Use mkdir -p to create the directory (because it seems right), and do
the same for $TEST_DIR (because there is no reason for that to be
created in any different way).

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/check | 15 +--
  1 file changed, 13 insertions(+), 2 deletions(-)



@@ -116,10 +117,14 @@ set_prog_path()
  if [ -z "$TEST_DIR" ]; then
  TEST_DIR=$PWD/scratch
  fi
+mkdir -p "$TEST_DIR" || _init_error 'Failed to create TEST_DIR'


This one seems fine. We are either using the user's name (and if it is 
pre-existing, not fail) or using a well-known name (if someone else 
slams in files into that directory in parallel with our test run, oh 
well).  But at least the well-known name is a directory that is probably 
already accessible only to the current user, not world-writable.


  
-if [ ! -e "$TEST_DIR" ]; then

-mkdir "$TEST_DIR"
+tmp_sock_dir=false
+if [ -z "$SOCK_DIR" ]; then
+SOCK_DIR=$(mktemp -d)
+tmp_sock_dir=true
  fi
+mkdir -p "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'


Thinking about this again: if the user passed in a name, we probably 
want to use it no matter whether the directory already exists (mkdir -p 
makes sense: either the directory did not exist, or the user is in 
charge of passing us a directory that they already secured).  But if we 
generate our own name in a world-writable location in /tmp, using mkdir 
-p means someone else can race us to the creation of the directory, and 
potentially populate it in a way to cause us a security hole while we 
execute our tests.


I would be a bit more comfortable with:

tmp_sock_dir=false
tmp_sock_opt=-p
if [ -z "$SOCK_DIR" ]; then
SOCK_DIR=$(mktemp -d)
tmp_sock_dir=true
tmp_sock_opt=  # disable -p for our generated name
fi
mkdir $tmp_sock_opt "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 2/3] iotests: Include QMP input in .out files

[Eric Blake]

On 10/17/19 7:59 AM, Max Reitz wrote:

On 15.10.19 21:35, Eric Blake wrote:

We generally include relevant HMP input in .out files, by virtue of
the fact that HMP echoes its input.  But QMP does not, so we have to
explicitly inject it in the output stream, in order to make it easier
to read .out files to see what behavior is being tested (especially
true where the output file is a sequence of {'return': {}}).

Suggested-by: Max Reitz 


That was actually not my intention. :-)

I was thinking of a new parameter that enables this behavior and is
disabled by default so that existing tests don’t change.

But then again I did see that you interpreted my suggestion in a
slightly different way, and thought this is probably better, actually.


I'm glad you like how it turned out.  Now to fix the problems ;)



+++ b/tests/qemu-iotests/common.qemu
@@ -123,6 +123,9 @@ _timed_wait_for()
  # until either timeout, or a response.  If it is not set, or <=0,
  # then the command is only sent once.
  #
+# If neither $silent nor $mismatch_only is set, and $cmd begins with '{',
+# echo the command before sending it the first time.
+#
  # If $qemu_error_no_exit is set, then even if the expected response
  # is not seen, we will not exit.  $QEMU_STATUS[$1] will be set it -1 in
  # that case.
@@ -152,6 +155,12 @@ _send_qemu_cmd()
  shift $(($# - 2))
  fi

+# Display QMP being sent, but not HMP (since HMP already echoes its
+# input back to output); decide based on leading '{'
+if [ -z "$silent" ] && [ -z "$mismatch_only" ] &&
+[ "$cmd" != "${cmd#{}" ]; then


It’s a shame that this breaks syntax highlighting in (my) vim.  (Also I
have to admit googling to understand ${cmd#{} wasn’t trivial.)

Can I persuade you to use "${cmd#\{}" instead?  That seems to work for me.


Yes.  That, or "${cmd#'{'}" should also work.




diff --git a/tests/qemu-iotests/094.out b/tests/qemu-iotests/094.out
index f3b9ecf22b73..f3e1a9ecf736 100644
--- a/tests/qemu-iotests/094.out
+++ b/tests/qemu-iotests/094.out
@@ -1,16 +1,20 @@
  QA output created by 094
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
  Formatting 'TEST_DIR/source.IMGFMT', fmt=IMGFMT size=67108864
+{'execute': 'qmp_capabilities'}
  {"return": {}}
+{'execute': 'drive-mirror', 'arguments': {'device': 'src', 'target': 
'nbd:127.0.0.1:10810', 'format': 'nbd', 'sync':'full', 'mode':'existing'}}


This reminds me that we need to fix nbd’s $TEST_IMG to not be fixed to
port 10810.  I get intermittent failures because of that.


And I should therefore fix the filter to display it as something more 
stable (perhaps nbd:HOST:PORT).  But I also agree that the hard-coded 
value is pre-existing broken, so a separate patch here to improve it is 
warranted.




[...]


diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
index 67fe44a3e390..3857675f7ebd 100644
--- a/tests/qemu-iotests/140.out
+++ b/tests/qemu-iotests/140.out
@@ -2,14 +2,19 @@ QA output created by 140
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
  wrote 65536/65536 bytes at offset 0
  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{ 'execute': 'qmp_capabilities' }
  {"return": {}}
+{ 'execute': 'nbd-server-start', 'arguments': { 'addr': { 'type': 'unix', 
'data': { 'path': 'TEST_DIR/nbd' 


Hm, this conflicts with my SOCK_DIR series.  common.qemu would then
also need a SOCK_DIR filter.  Well, or 140 should filter it (and the
other tests that are concerned).  I’m not 100 % sure, but a SOCK_DIR
filter in common.qemu probably can’t hurt.


Agreed. I will rebase a v3 on top of your pending series.



+++ b/tests/qemu-iotests/141.out
@@ -2,82 +2,108 @@ QA output created by 141
  Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=1048576
  Formatting 'TEST_DIR/m.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/b.IMGFMT
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/m.IMGFMT
+{'execute': 'qmp_capabilities'}
  {"return": {}}

  === Testing drive-backup ===

+{'execute': 'blockdev-add', 'arguments': { 'node-name': 'drv0', 'driver': 
'qcow2', 'file': { 'driver': 'file', 'filename': 'TEST_DIR/t.qcow2' }}}


141 also supports qed, so this then results in a mismatch.  I suppose
common.qemu should filter the image format.

(Same for 156, 161, and 229.)


Yep, I'll have to improve the filtering.  I'll make sure I run -qed 
tests before posting v3.




[...]


diff --git a/tests/qemu-iotests/156.out b/tests/qemu-iotests/156.out
index 4c391a760371..d1865044f81a 100644
--- a/tests/qemu-iotests/156.out
+++ b/tests/qemu-iotests/156.out
@@ -5,21 +5,27 @@ wrote 262144/262144 bytes at offset 0
  256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
  wrote 196608/196608 bytes at offset 65536
  192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{ 'execute': 'qmp_capabilities' }
  {"return": {}}
  Formatting 'TEST_DIR/t.IMGFMT.overlay', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.IMGFMT
+{ 'execute': 

[PATCH] block/backup: drop dead code from backup_job_create

[Vladimir Sementsov-Ogievskiy]

After commit 00e30f05de1d195, there is no more "goto error" points
after job creation, so after "error:" @job is always NULL and we don't
need roll-back job creation.

Reported-by: Coverity (CID 1406402)
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/backup.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 46978c1785..6e1497f7bb 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -474,10 +474,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 if (sync_bitmap) {
 bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
 }
-if (job) {
-backup_clean(>common.job);
-job_early_fail(>common.job);
-} else if (backup_top) {
+if (backup_top) {
 bdrv_backup_top_drop(backup_top);
 }
 
-- 
2.21.0

[PATCH v2 21/23] iotests/240: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/240 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240
index f73bc07d80..8b4337b58d 100755
--- a/tests/qemu-iotests/240
+++ b/tests/qemu-iotests/240
@@ -29,7 +29,7 @@ status=1  # failure is the default!
 
 _cleanup()
 {
-rm -f "$TEST_DIR/nbd"
+rm -f "$SOCK_DIR/nbd"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -135,7 +135,7 @@ echo
 run_qemu <

[PATCH v2 19/23] iotests/222: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/222 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index 0ead56d574..3f9f934ad8 100644
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -48,7 +48,7 @@ remainder = [("0xd5", "0x108000",  "32k"), # Right-end of 
partial-left [1]
 
 with iotests.FilePath('base.img') as base_img_path, \
  iotests.FilePath('fleece.img') as fleece_img_path, \
- iotests.FilePath('nbd.sock') as nbd_sock_path, \
+ iotests.FilePath('nbd.sock', iotests.sock_dir) as nbd_sock_path, \
  iotests.VM() as vm:
 
 log('--- Setting up images ---')
-- 
2.21.0

[PATCH v2 22/23] iotests/267: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/267 | 4 ++--
 tests/qemu-iotests/267.out | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
index d37a67c012..170e173c0a 100755
--- a/tests/qemu-iotests/267
+++ b/tests/qemu-iotests/267
@@ -29,7 +29,7 @@ status=1  # failure is the default!
 _cleanup()
 {
 _cleanup_test_img
-rm -f "$TEST_DIR/nbd"
+rm -f "$SOCK_DIR/nbd"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -143,7 +143,7 @@ echo
 
 IMGOPTS="backing_file=$TEST_IMG.base" _make_test_img $size
 cat <

[PATCH v2 10/23] iotests/181: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Thomas Huth 
---
 tests/qemu-iotests/181 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/181 b/tests/qemu-iotests/181
index e317e63422..378c2899d1 100755
--- a/tests/qemu-iotests/181
+++ b/tests/qemu-iotests/181
@@ -26,7 +26,7 @@ echo "QA output created by $seq"
 
 status=1   # failure is the default!
 
-MIG_SOCKET="${TEST_DIR}/migrate"
+MIG_SOCKET="${SOCK_DIR}/migrate"
 
 _cleanup()
 {
-- 
2.21.0

[PATCH v2 12/23] iotests/183: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Thomas Huth 
---
 tests/qemu-iotests/183 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
index 04fb344d08..bced83fae0 100755
--- a/tests/qemu-iotests/183
+++ b/tests/qemu-iotests/183
@@ -26,7 +26,7 @@ echo "QA output created by $seq"
 
 status=1 # failure is the default!
 
-MIG_SOCKET="${TEST_DIR}/migrate"
+MIG_SOCKET="${SOCK_DIR}/migrate"
 
 _cleanup()
 {
-- 
2.21.0

[PATCH v2 18/23] iotests/209: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/209 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/209 b/tests/qemu-iotests/209
index 259e991ec6..e0f464bcbe 100755
--- a/tests/qemu-iotests/209
+++ b/tests/qemu-iotests/209
@@ -24,7 +24,8 @@ from iotests import qemu_img_create, qemu_io, 
qemu_img_verbose, qemu_nbd, \
 
 iotests.verify_image_format(supported_fmts=['qcow2'])
 
-disk, nbd_sock = file_path('disk', 'nbd-sock')
+disk = file_path('disk')
+nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
 nbd_uri = 'nbd+unix:///exp?socket=' + nbd_sock
 
 qemu_img_create('-f', iotests.imgfmt, disk, '1M')
-- 
2.21.0

[PATCH v2 16/23] iotests/205: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/205 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/205 b/tests/qemu-iotests/205
index 76f6c5fa2b..4bb2c21e8b 100755
--- a/tests/qemu-iotests/205
+++ b/tests/qemu-iotests/205
@@ -24,7 +24,7 @@ import iotests
 import time
 from iotests import qemu_img_create, qemu_io, filter_qemu_io, QemuIoInteractive
 
-nbd_sock = os.path.join(iotests.test_dir, 'nbd_sock')
+nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock')
 nbd_uri = 'nbd+unix:///exp?socket=' + nbd_sock
 disk = os.path.join(iotests.test_dir, 'disk')
 
-- 
2.21.0

[PATCH v2 14/23] iotests/194: Create sockets in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/194 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index d746ab1e21..72e47e8833 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -26,8 +26,8 @@ iotests.verify_platform(['linux'])
 
 with iotests.FilePath('source.img') as source_img_path, \
  iotests.FilePath('dest.img') as dest_img_path, \
- iotests.FilePath('migration.sock') as migration_sock_path, \
- iotests.FilePath('nbd.sock') as nbd_sock_path, \
+ iotests.FilePaths(['migration.sock', 'nbd.sock'], iotests.sock_dir) as \
+ [migration_sock_path, nbd_sock_path], \
  iotests.VM('source') as source_vm, \
  iotests.VM('dest') as dest_vm:
 
-- 
2.21.0

Re: [PULL 33/36] block/backup: use backup-top instead of write notifiers

[Vladimir Sementsov-Ogievskiy]

17.10.2019 15:04, Peter Maydell wrote:
> On Thu, 10 Oct 2019 at 12:44, Max Reitz  wrote:
>>
>> From: Vladimir Sementsov-Ogievskiy 
>>
>> Drop write notifiers and use filter node instead.
> 
> Hi; after this change Coverity complains about dead code in
> backup_job_create() (CID 1406402):

Oops, I'm sorry. Will send a patch soon.

> 
>> @@ -382,6 +353,8 @@ BlockJob *backup_job_create(const char *job_id, 
>> BlockDriverState *bs,
>>   BackupBlockJob *job = NULL;
>>   int64_t cluster_size;
>>   BdrvRequestFlags write_flags;
>> +BlockDriverState *backup_top = NULL;
>> +BlockCopyState *bcs = NULL;
>>
>>   assert(bs);
>>   assert(target);
>> @@ -463,33 +436,35 @@ BlockJob *backup_job_create(const char *job_id, 
>> BlockDriverState *bs,
>>   write_flags = (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING 
>> : 0) |
>> (compress ? BDRV_REQ_WRITE_COMPRESSED : 0),
>>
>> +backup_top = bdrv_backup_top_append(bs, target, filter_node_name,
>> +cluster_size, write_flags, , 
>> errp);
>> +if (!backup_top) {
>> +goto error;
>> +}
>> +
>>   /* job->len is fixed, so we can't allow resize */
>> -job = block_job_create(job_id, _job_driver, txn, bs, 0, 
>> BLK_PERM_ALL,
>> +job = block_job_create(job_id, _job_driver, txn, backup_top,
>> +   0, BLK_PERM_ALL,
>>  speed, creation_flags, cb, opaque, errp);
>>   if (!job) {
>>   goto error;
>>   }
>>
>> +job->backup_top = backup_top;
>>   job->source_bs = bs;
>>   job->on_source_error = on_source_error;
>>   job->on_target_error = on_target_error;
>>   job->sync_mode = sync_mode;
>>   job->sync_bitmap = sync_bitmap;
>>   job->bitmap_mode = bitmap_mode;
>> -
>> -job->bcs = block_copy_state_new(bs, target, cluster_size, write_flags,
>> -errp);
>> -if (!job->bcs) {
>> -goto error;
>> -}
>> -
> 
> This code deletion has removed the only code path that can
> reach the 'error' label after the successful creation of the job...
> 
>> +job->bcs = bcs;
>>   job->cluster_size = cluster_size;
>>   job->len = len;
>>
>> -block_copy_set_callbacks(job->bcs, backup_progress_bytes_callback,
>> +block_copy_set_callbacks(bcs, backup_progress_bytes_callback,
>>backup_progress_reset_callback, job);
>>
>> -/* Required permissions are already taken by block-copy-state target */
>> +/* Required permissions are already taken by backup-top target */
>>   block_job_add_bdrv(>common, "target", target, 0, BLK_PERM_ALL,
>>  _abort);
>>
>> @@ -502,6 +477,8 @@ BlockJob *backup_job_create(const char *job_id, 
>> BlockDriverState *bs,
>>   if (job) {
>>   backup_clean(>common.job);
>>   job_early_fail(>common.job);
> 
> ...so down here in the 'error:' code the "if (job)" condition
> can never pass, and the code in this part of the if statement
> is dead.
> 
>> +} else if (backup_top) {
>> +bdrv_backup_top_drop(backup_top);
>>   }
>>
>>   return NULL;
>>   {"return": {}}
> 
> thanks
> -- PMM
> 


-- 
Best regards,
Vladimir

[PATCH v2 08/23] iotests/143: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Thomas Huth 
---
 tests/qemu-iotests/143 | 6 +++---
 tests/qemu-iotests/143.out | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143
index 92249ac8da..f649b36195 100755
--- a/tests/qemu-iotests/143
+++ b/tests/qemu-iotests/143
@@ -29,7 +29,7 @@ status=1  # failure is the default!
 _cleanup()
 {
 _cleanup_qemu
-rm -f "$TEST_DIR/nbd"
+rm -f "$SOCK_DIR/nbd"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -51,12 +51,12 @@ _send_qemu_cmd $QEMU_HANDLE \
 _send_qemu_cmd $QEMU_HANDLE \
 "{ 'execute': 'nbd-server-start',
'arguments': { 'addr': { 'type': 'unix',
-'data': { 'path': '$TEST_DIR/nbd' " \
+'data': { 'path': '$SOCK_DIR/nbd' " \
 'return'
 
 # This should just result in a client error, not in the server crashing
 $QEMU_IO_PROG -f raw -c quit \
-"nbd+unix:///no_such_export?socket=$TEST_DIR/nbd" 2>&1 \
+"nbd+unix:///no_such_export?socket=$SOCK_DIR/nbd" 2>&1 \
 | _filter_qemu_io | _filter_nbd
 
 _send_qemu_cmd $QEMU_HANDLE \
diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out
index ee71b5aa42..037d34a409 100644
--- a/tests/qemu-iotests/143.out
+++ b/tests/qemu-iotests/143.out
@@ -1,7 +1,7 @@
 QA output created by 143
 {"return": {}}
 {"return": {}}
-qemu-io: can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: 
Requested export not available
+qemu-io: can't open device nbd+unix:///no_such_export?socket=SOCK_DIR/nbd: 
Requested export not available
 server reported: export 'no_such_export' not present
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
-- 
2.21.0

[PATCH v2 15/23] iotests/201: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/201 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/201 b/tests/qemu-iotests/201
index 7abf740fe4..86fa37e714 100755
--- a/tests/qemu-iotests/201
+++ b/tests/qemu-iotests/201
@@ -24,7 +24,7 @@ echo "QA output created by $seq"
 
 status=1   # failure is the default!
 
-MIG_SOCKET="${TEST_DIR}/migrate"
+MIG_SOCKET="${SOCK_DIR}/migrate"
 
 # get standard environment, filters and checks
 . ./common.rc
-- 
2.21.0

[PATCH v2 09/23] iotests/147: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Thomas Huth 
---
 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 ab8480b9a4..03fc2fabcf 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -32,7 +32,7 @@ NBD_IPV6_PORT_START = NBD_PORT_END
 NBD_IPV6_PORT_END   = NBD_IPV6_PORT_START + 1024
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
-unix_socket = os.path.join(iotests.test_dir, 'nbd.socket')
+unix_socket = os.path.join(iotests.sock_dir, 'nbd.socket')
 
 
 def flatten_sock_addr(crumpled_address):
-- 
2.21.0

[PATCH v2 13/23] iotests/192: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/192 | 4 ++--
 tests/qemu-iotests/192.out | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/192 b/tests/qemu-iotests/192
index 034432272f..d2ba55dd90 100755
--- a/tests/qemu-iotests/192
+++ b/tests/qemu-iotests/192
@@ -31,7 +31,7 @@ _cleanup()
 {
 _cleanup_qemu
 _cleanup_test_img
-rm -f "$TEST_DIR/nbd"
+rm -f "$SOCK_DIR/nbd"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -66,7 +66,7 @@ else
 QEMU_COMM_TIMEOUT=1
 fi
 
-_send_qemu_cmd $h "nbd_server_start unix:$TEST_DIR/nbd" "(qemu)"
+_send_qemu_cmd $h "nbd_server_start unix:$SOCK_DIR/nbd" "(qemu)"
 _send_qemu_cmd $h "nbd_server_add -w drive0" "(qemu)"
 _send_qemu_cmd $h "q" "(qemu)"
 
diff --git a/tests/qemu-iotests/192.out b/tests/qemu-iotests/192.out
index 1e0be4c4d7..b9429dbe36 100644
--- a/tests/qemu-iotests/192.out
+++ b/tests/qemu-iotests/192.out
@@ -1,7 +1,7 @@
 QA output created by 192
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) nbd_server_start unix:TEST_DIR/nbd
+(qemu) nbd_server_start unix:SOCK_DIR/nbd
 (qemu) nbd_server_add -w drive0
 (qemu) q
 *** done
-- 
2.21.0

[PATCH v2 04/23] iotests: Filter $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/common.filter | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 9f418b4881..0ee6042524 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -43,7 +43,8 @@ _filter_qom_path()
 # replace occurrences of the actual TEST_DIR value with TEST_DIR
 _filter_testdir()
 {
-$SED -e "s#$TEST_DIR/#TEST_DIR/#g"
+$SED -e "s#$TEST_DIR/#TEST_DIR/#g" \
+ -e "s#$SOCK_DIR/#SOCK_DIR/#g"
 }
 
 # replace occurrences of the actual IMGFMT value with IMGFMT
@@ -124,6 +125,7 @@ _filter_img_create()
 $SED -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
 -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
 -e "s#$TEST_DIR#TEST_DIR#g" \
+-e "s#$SOCK_DIR#SOCK_DIR#g" \
 -e "s#$IMGFMT#IMGFMT#g" \
 -e 's#nbd:127.0.0.1:10810#TEST_DIR/t.IMGFMT#g' \
 -e "s# encryption=off##g" \
@@ -160,6 +162,7 @@ _filter_img_info()
 $SED -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
 -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
 -e "s#$TEST_DIR#TEST_DIR#g" \
+-e "s#$SOCK_DIR#SOCK_DIR#g" \
 -e "s#$IMGFMT#IMGFMT#g" \
 -e 's#nbd://127.0.0.1:10810$#TEST_DIR/t.IMGFMT#g' \
 -e 's#json.*vdisk-id.*vxhs"}}#TEST_DIR/t.IMGFMT#' \
@@ -219,6 +222,7 @@ _filter_nbd()
 $SED -e '/nbd\/.*\.c:/d' \
 -e 's#127\.0\.0\.1:[0-9]*#127.0.0.1:PORT#g' \
 -e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \
+-e "s#?socket=$SOCK_DIR#?socket=SOCK_DIR#g" \
 -e 's#\(foo\|PORT/\?\|.sock\): Failed to .*$#\1#'
 }
 
-- 
2.21.0

[PATCH v2 07/23] iotests/140: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Thomas Huth 
---
 tests/qemu-iotests/140 | 8 
 tests/qemu-iotests/140.out | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
index b965b1dd5d..8d2ce5d9e3 100755
--- a/tests/qemu-iotests/140
+++ b/tests/qemu-iotests/140
@@ -34,7 +34,7 @@ _cleanup()
 {
 _cleanup_qemu
 _cleanup_test_img
-rm -f "$TEST_DIR/nbd"
+rm -f "$SOCK_DIR/nbd"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -69,7 +69,7 @@ _send_qemu_cmd $QEMU_HANDLE \
 _send_qemu_cmd $QEMU_HANDLE \
 "{ 'execute': 'nbd-server-start',
'arguments': { 'addr': { 'type': 'unix',
-'data': { 'path': '$TEST_DIR/nbd' " \
+'data': { 'path': '$SOCK_DIR/nbd' " \
 'return'
 
 _send_qemu_cmd $QEMU_HANDLE \
@@ -78,7 +78,7 @@ _send_qemu_cmd $QEMU_HANDLE \
 'return'
 
 $QEMU_IO_PROG -f raw -r -c 'read -P 42 0 64k' \
-"nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
+"nbd+unix:///drv?socket=$SOCK_DIR/nbd" 2>&1 \
 | _filter_qemu_io | _filter_nbd
 
 _send_qemu_cmd $QEMU_HANDLE \
@@ -87,7 +87,7 @@ _send_qemu_cmd $QEMU_HANDLE \
 'return'
 
 $QEMU_IO_PROG -f raw -r -c close \
-"nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
+"nbd+unix:///drv?socket=$SOCK_DIR/nbd" 2>&1 \
 | _filter_qemu_io | _filter_nbd
 
 _send_qemu_cmd $QEMU_HANDLE \
diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
index 67fe44a3e3..2511eb7369 100644
--- a/tests/qemu-iotests/140.out
+++ b/tests/qemu-iotests/140.out
@@ -8,7 +8,7 @@ wrote 65536/65536 bytes at offset 0
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"return": {}}
-qemu-io: can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Requested 
export not available
+qemu-io: can't open device nbd+unix:///drv?socket=SOCK_DIR/nbd: Requested 
export not available
 server reported: export 'drv' not present
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
-- 
2.21.0

[PATCH v2 01/23] iotests: Introduce $SOCK_DIR

[Max Reitz]

Unix sockets generally have a maximum path length.  Depending on your
$TEST_DIR, it may be exceeded and then all tests that create and use
Unix sockets there may fail.

Circumvent this by adding a new scratch directory specifically for
Unix socket files.  It defaults to a temporary directory (mktemp -d)
that is completely removed after the iotests are done.

(By default, mktemp -d creates a /tmp/tmp.XX directory, which
should be short enough for our use cases.)

Use mkdir -p to create the directory (because it seems right), and do
the same for $TEST_DIR (because there is no reason for that to be
created in any different way).

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/check | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 588c453a94..71fe38834e 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -97,6 +97,7 @@ IMGFMT-- $FULL_IMGFMT_DETAILS
 IMGPROTO  -- $IMGPROTO
 PLATFORM  -- $FULL_HOST_DETAILS
 TEST_DIR  -- $TEST_DIR
+SOCK_DIR  -- $SOCK_DIR
 SOCKET_SCM_HELPER -- $SOCKET_SCM_HELPER
 
 EOF
@@ -116,10 +117,14 @@ set_prog_path()
 if [ -z "$TEST_DIR" ]; then
 TEST_DIR=$PWD/scratch
 fi
+mkdir -p "$TEST_DIR" || _init_error 'Failed to create TEST_DIR'
 
-if [ ! -e "$TEST_DIR" ]; then
-mkdir "$TEST_DIR"
+tmp_sock_dir=false
+if [ -z "$SOCK_DIR" ]; then
+SOCK_DIR=$(mktemp -d)
+tmp_sock_dir=true
 fi
+mkdir -p "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
 
 diff="diff -u"
 verbose=false
@@ -534,6 +539,7 @@ if [ -z "$SAMPLE_IMG_DIR" ]; then
 fi
 
 export TEST_DIR
+export SOCK_DIR
 export SAMPLE_IMG_DIR
 
 if [ -s $tmp.list ]
@@ -716,6 +722,11 @@ END{ if (NR > 0) {
 rm -f "${TEST_DIR}"/*.out "${TEST_DIR}"/*.err "${TEST_DIR}"/*.time
 rm -f "${TEST_DIR}"/check.pid "${TEST_DIR}"/check.sts
 rm -f $tmp.*
+
+if $tmp_sock_dir
+then
+rm -rf "$SOCK_DIR"
+fi
 }
 
 trap "_wrapup; exit \$status" 0 1 2 3 15
-- 
2.21.0

[PATCH v2 05/23] iotests: Let common.nbd create socket in $SOCK_DIR

[Max Reitz]

In addition, drop the nbd_unix_socket assignment in 241 because it does
not really do anything.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/241| 2 --
 tests/qemu-iotests/common.nbd | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/241 b/tests/qemu-iotests/241
index 58b64ebf41..8dae8d39e4 100755
--- a/tests/qemu-iotests/241
+++ b/tests/qemu-iotests/241
@@ -23,8 +23,6 @@ echo "QA output created by $seq"
 
 status=1 # failure is the default!
 
-nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
-
 _cleanup()
 {
 _cleanup_test_img
diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index 24b01b60aa..a8cae8fe2c 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -19,7 +19,7 @@
 # along with this program.  If not, see .
 #
 
-nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
+nbd_unix_socket="${SOCK_DIR}/qemu-nbd.sock"
 nbd_tcp_addr="127.0.0.1"
 nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
 nbd_stderr_fifo="${TEST_DIR}/qemu-nbd.fifo"
-- 
2.21.0

[PATCH v2 02/23] iotests.py: Store socket files in $SOCK_DIR

[Max Reitz]

iotests.py itself does not store socket files, but machine.py and
qtest.py do.  iotests.py needs to pass the respective path to them, and
they need to adhere to it.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Thomas Huth 
---
 python/qemu/machine.py| 15 ---
 python/qemu/qtest.py  |  9 ++---
 tests/qemu-iotests/iotests.py |  4 +++-
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 128a3d1dc2..2024e8b1b1 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -71,7 +71,7 @@ class QEMUMachine(object):
 
 def __init__(self, binary, args=None, wrapper=None, name=None,
  test_dir="/var/tmp", monitor_address=None,
- socket_scm_helper=None):
+ socket_scm_helper=None, sock_dir=None):
 '''
 Initialize a QEMUMachine
 
@@ -90,6 +90,8 @@ class QEMUMachine(object):
 wrapper = []
 if name is None:
 name = "qemu-%d" % os.getpid()
+if sock_dir is None:
+sock_dir = test_dir
 self._name = name
 self._monitor_address = monitor_address
 self._vm_monitor = None
@@ -106,12 +108,14 @@ class QEMUMachine(object):
 self._qemu_full_args = None
 self._test_dir = test_dir
 self._temp_dir = None
+self._sock_dir = sock_dir
 self._launched = False
 self._machine = None
 self._console_set = False
 self._console_device_type = None
 self._console_address = None
 self._console_socket = None
+self._remove_files = []
 
 # just in case logging wasn't configured by the main script:
 logging.basicConfig()
@@ -236,8 +240,9 @@ class QEMUMachine(object):
 if self._machine is not None:
 args.extend(['-machine', self._machine])
 if self._console_set:
-self._console_address = os.path.join(self._temp_dir,
+self._console_address = os.path.join(self._sock_dir,
  self._name + "-console.sock")
+self._remove_files.append(self._console_address)
 chardev = ('socket,id=console,path=%s,server,nowait' %
self._console_address)
 args.extend(['-chardev', chardev])
@@ -253,8 +258,9 @@ class QEMUMachine(object):
 if self._monitor_address is not None:
 self._vm_monitor = self._monitor_address
 else:
-self._vm_monitor = os.path.join(self._temp_dir,
+self._vm_monitor = os.path.join(self._sock_dir,
 self._name + "-monitor.sock")
+self._remove_files.append(self._vm_monitor)
 self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
 self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
@@ -279,6 +285,9 @@ class QEMUMachine(object):
 shutil.rmtree(self._temp_dir)
 self._temp_dir = None
 
+while len(self._remove_files) > 0:
+self._remove_if_exists(self._remove_files.pop())
+
 def launch(self):
 """
 Launch the VM and make sure we cleanup and expose the
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 3f1d2cb325..d24ad04256 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -84,14 +84,17 @@ class QEMUQtestMachine(QEMUMachine):
 '''A QEMU VM'''
 
 def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
- socket_scm_helper=None):
+ socket_scm_helper=None, sock_dir=None):
 if name is None:
 name = "qemu-%d" % os.getpid()
+if sock_dir is None:
+sock_dir = test_dir
 super(QEMUQtestMachine,
   self).__init__(binary, args, name=name, test_dir=test_dir,
- socket_scm_helper=socket_scm_helper)
+ socket_scm_helper=socket_scm_helper,
+ sock_dir=sock_dir)
 self._qtest = None
-self._qtest_path = os.path.join(test_dir, name + "-qtest.sock")
+self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
 
 def _base_args(self):
 args = super(QEMUQtestMachine, self)._base_args()
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 43759e4e27..0616129342 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -57,6 +57,7 @@ qemu_opts = os.environ.get('QEMU_OPTIONS', 
'').strip().split(' ')
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
 test_dir = os.environ.get('TEST_DIR')
+sock_dir = os.environ.get('SOCK_DIR')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
 cachemode = os.environ.get('CACHEMODE')
 qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
@@ -445,7 +446,8 @@ class 

[PATCH v2 03/23] iotests.py: Add @base_dir to FilePaths etc.

[Max Reitz]

Specifying this optional parameter allows creating temporary files in
other directories than the test_dir; for example in sock_dir.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/iotests.py | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0616129342..b61ff30961 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -375,10 +375,10 @@ class FilePaths(object):
 qemu_img('create', img_path, '1G')
 # migration_sock_path is automatically deleted
 """
-def __init__(self, names):
+def __init__(self, names, base_dir=test_dir):
 self.paths = []
 for name in names:
-self.paths.append(os.path.join(test_dir, file_pattern(name)))
+self.paths.append(os.path.join(base_dir, file_pattern(name)))
 
 def __enter__(self):
 return self.paths
@@ -395,8 +395,8 @@ class FilePath(FilePaths):
 """
 FilePath is a specialization of FilePaths that takes a single filename.
 """
-def __init__(self, name):
-super(FilePath, self).__init__([name])
+def __init__(self, name, base_dir=test_dir):
+super(FilePath, self).__init__([name], base_dir)
 
 def __enter__(self):
 return self.paths[0]
@@ -409,7 +409,7 @@ def file_path_remover():
 pass
 
 
-def file_path(*names):
+def file_path(*names, base_dir=test_dir):
 ''' Another way to get auto-generated filename that cleans itself up.
 
 Use is as simple as:
@@ -425,7 +425,7 @@ def file_path(*names):
 paths = []
 for name in names:
 filename = file_pattern(name)
-path = os.path.join(test_dir, filename)
+path = os.path.join(base_dir, filename)
 file_path_remover.paths.append(path)
 paths.append(path)
 
-- 
2.21.0

[PATCH v2 23/23] iotests: Drop TEST_DIR filter from _filter_nbd

[Max Reitz]

Sockets should be placed into $SOCK_DIR instead of $TEST_DIR, so remove
the $TEST_DIR filter from _filter_nbd.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/common.filter | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 0ee6042524..f870e00e44 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -221,7 +221,6 @@ _filter_nbd()
 # Filter out the TCP port number since this changes between runs.
 $SED -e '/nbd\/.*\.c:/d' \
 -e 's#127\.0\.0\.1:[0-9]*#127.0.0.1:PORT#g' \
--e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \
 -e "s#?socket=$SOCK_DIR#?socket=SOCK_DIR#g" \
 -e 's#\(foo\|PORT/\?\|.sock\): Failed to .*$#\1#'
 }
-- 
2.21.0

[PATCH v2 20/23] iotests/223: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/223 | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 2ba3d8124b..b5a80e50bb 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -28,7 +28,7 @@ _cleanup()
 nbd_server_stop
 _cleanup_test_img
 _cleanup_qemu
-rm -f "$TEST_DIR/nbd"
+rm -f "$SOCK_DIR/nbd"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -125,11 +125,11 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n"}}' "error" # Attempt add without server
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
   "arguments":{"addr":{"type":"unix",
-"data":{"path":"'"$TEST_DIR/nbd"'"' "return"
+"data":{"path":"'"$SOCK_DIR/nbd"'"' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
   "arguments":{"addr":{"type":"unix",
-"data":{"path":"'"$TEST_DIR/nbd"1'"' "error" # Attempt second server
-$QEMU_NBD_PROG -L -k "$TEST_DIR/nbd"
+"data":{"path":"'"$SOCK_DIR/nbd"1'"' "error" # Attempt second server
+$QEMU_NBD_PROG -L -k "$SOCK_DIR/nbd"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "bitmap":"b"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
@@ -145,14 +145,14 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "name":"n2", "writable":true,
   "bitmap":"b2"}}' "return"
-$QEMU_NBD_PROG -L -k "$TEST_DIR/nbd"
+$QEMU_NBD_PROG -L -k "$SOCK_DIR/nbd"
 
 echo
 echo "=== Contrast normal status to large granularity dirty-bitmap ==="
 echo
 
 QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
-IMG="driver=nbd,export=n,server.type=unix,server.path=$TEST_DIR/nbd"
+IMG="driver=nbd,export=n,server.type=unix,server.path=$SOCK_DIR/nbd"
 $QEMU_IO -r -c 'r -P 0x22 512 512' -c 'r -P 0 512k 512k' -c 'r -P 0x11 1m 1m' \
   -c 'r -P 0x33 2m 2m' --image-opts "$IMG" | _filter_qemu_io
 $QEMU_IMG map --output=json --image-opts \
@@ -164,7 +164,7 @@ echo
 echo "=== Contrast to small granularity dirty-bitmap ==="
 echo
 
-IMG="driver=nbd,export=n2,server.type=unix,server.path=$TEST_DIR/nbd"
+IMG="driver=nbd,export=n2,server.type=unix,server.path=$SOCK_DIR/nbd"
 $QEMU_IMG map --output=json --image-opts \
   "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
 
-- 
2.21.0

[PATCH v2 11/23] iotests/182: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Thomas Huth 
---
 tests/qemu-iotests/182 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/182 b/tests/qemu-iotests/182
index 7f494eb9bb..1ccb850055 100755
--- a/tests/qemu-iotests/182
+++ b/tests/qemu-iotests/182
@@ -31,7 +31,7 @@ _cleanup()
 {
 _cleanup_test_img
 rm -f "$TEST_IMG.overlay"
-rm -f "$TEST_DIR/nbd.socket"
+rm -f "$SOCK_DIR/nbd.socket"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -133,7 +133,7 @@ success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
   'addr': {
   'type': 'unix',
   'data': {
-  'path': '$TEST_DIR/nbd.socket'
+  'path': '$SOCK_DIR/nbd.socket'
   } } } }" \
 'return' \
 'error'
-- 
2.21.0

[PATCH v2 17/23] iotests/208: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/208 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 1e202388dc..546eb1de3e 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -26,7 +26,7 @@ iotests.verify_image_format(supported_fmts=['generic'])
 
 with iotests.FilePath('disk.img') as disk_img_path, \
  iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
- iotests.FilePath('nbd.sock') as nbd_sock_path, \
+ iotests.FilePath('nbd.sock', iotests.sock_dir) as nbd_sock_path, \
  iotests.VM() as vm:
 
 img_size = '10M'
-- 
2.21.0

[PATCH v2 06/23] iotests/083: Create socket in $SOCK_DIR

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Thomas Huth 
---
 tests/qemu-iotests/083 |  6 +++---
 tests/qemu-iotests/083.out | 34 +-
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index b270550d3e..10fdfc8ebb 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -28,7 +28,7 @@ status=1  # failure is the default!
 
 _cleanup()
 {
-   rm -f nbd.sock
+   rm -f "$SOCK_DIR/nbd.sock"
rm -f nbd-fault-injector.out
rm -f nbd-fault-injector.conf
 }
@@ -80,10 +80,10 @@ EOF
if [ "$proto" = "tcp" ]; then
nbd_addr="127.0.0.1:0"
else
-   nbd_addr="$TEST_DIR/nbd.sock"
+   nbd_addr="$SOCK_DIR/nbd.sock"
fi
 
-   rm -f "$TEST_DIR/nbd.sock"
+   rm -f "$SOCK_DIR/nbd.sock"
 
 echo > "$TEST_DIR/nbd-fault-injector.out"
$PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" 
"$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 &
diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
index eee6dd1379..2090ee693c 100644
--- a/tests/qemu-iotests/083.out
+++ b/tests/qemu-iotests/083.out
@@ -110,43 +110,43 @@ read failed: Input/output error
 
 === Check disconnect before neg1 ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect after neg1 ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect 8 neg1 ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect 16 neg1 ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect before export ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect after export ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect 4 export ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect 12 export ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect 16 export ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect before neg2 ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect after neg2 ===
 
@@ -154,11 +154,11 @@ read failed: Input/output error
 
 === Check disconnect 8 neg2 ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect 10 neg2 ===
 
-qemu-io: can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///foo?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect before request ===
 
@@ -195,23 +195,23 @@ read 512/512 bytes at offset 0
 
 === Check disconnect before neg-classic ===
 
-qemu-io: can't open device nbd+unix:///?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect 8 neg-classic ===
 
-qemu-io: can't open device nbd+unix:///?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect 16 neg-classic ===
 
-qemu-io: can't open device nbd+unix:///?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect 24 neg-classic ===
 
-qemu-io: can't open device nbd+unix:///?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect 28 neg-classic ===
 
-qemu-io: can't open device nbd+unix:///?socket=TEST_DIR/nbd.sock
+qemu-io: can't open device nbd+unix:///?socket=SOCK_DIR/nbd.sock
 
 === Check disconnect after neg-classic ===
 
-- 
2.21.0

[PATCH v2 00/23] iotests: Add and use $SOCK_DIR

[Max Reitz]

Hi,

Perhaps the main reason we cannot run important tests such as 041 in CI
is that when they care Unix sockets in $TEST_DIR, the path may become
too long to connect to them.

To get by this problem, this series lets the check script create a new
temporary directory (mktemp -d) and then makes the iotests use it for
all Unix sockets.


v2:
- Patch 1: Use mkdir -p
- Patches 4/23: Only add the $SOCK_DIR replacement line in patch 4 and
only drop the $TEST_DIR line in patch 23
  (Took Eric’s R-b on both because that’s how I interpreted his
  comments)


git-backport-diff against v2:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/23:[0010] [FC] 'iotests: Introduce $SOCK_DIR'
002/23:[] [--] 'iotests.py: Store socket files in $SOCK_DIR'
003/23:[] [--] 'iotests.py: Add @base_dir to FilePaths etc.'
004/23:[0002] [FC] 'iotests: Filter $SOCK_DIR'
005/23:[] [--] 'iotests: Let common.nbd create socket in $SOCK_DIR'
006/23:[] [--] 'iotests/083: Create socket in $SOCK_DIR'
007/23:[] [--] 'iotests/140: Create socket in $SOCK_DIR'
008/23:[] [--] 'iotests/143: Create socket in $SOCK_DIR'
009/23:[] [--] 'iotests/147: Create socket in $SOCK_DIR'
010/23:[] [--] 'iotests/181: Create socket in $SOCK_DIR'
011/23:[] [--] 'iotests/182: Create socket in $SOCK_DIR'
012/23:[] [--] 'iotests/183: Create socket in $SOCK_DIR'
013/23:[] [--] 'iotests/192: Create socket in $SOCK_DIR'
014/23:[] [--] 'iotests/194: Create sockets in $SOCK_DIR'
015/23:[] [--] 'iotests/201: Create socket in $SOCK_DIR'
016/23:[] [--] 'iotests/205: Create socket in $SOCK_DIR'
017/23:[] [--] 'iotests/208: Create socket in $SOCK_DIR'
018/23:[] [--] 'iotests/209: Create socket in $SOCK_DIR'
019/23:[] [--] 'iotests/222: Create socket in $SOCK_DIR'
020/23:[] [--] 'iotests/223: Create socket in $SOCK_DIR'
021/23:[] [--] 'iotests/240: Create socket in $SOCK_DIR'
022/23:[] [--] 'iotests/267: Create socket in $SOCK_DIR'
023/23:[0002] [FC] 'iotests: Drop TEST_DIR filter from _filter_nbd'


Max Reitz (23):
  iotests: Introduce $SOCK_DIR
  iotests.py: Store socket files in $SOCK_DIR
  iotests.py: Add @base_dir to FilePaths etc.
  iotests: Filter $SOCK_DIR
  iotests: Let common.nbd create socket in $SOCK_DIR
  iotests/083: Create socket in $SOCK_DIR
  iotests/140: Create socket in $SOCK_DIR
  iotests/143: Create socket in $SOCK_DIR
  iotests/147: Create socket in $SOCK_DIR
  iotests/181: Create socket in $SOCK_DIR
  iotests/182: Create socket in $SOCK_DIR
  iotests/183: Create socket in $SOCK_DIR
  iotests/192: Create socket in $SOCK_DIR
  iotests/194: Create sockets in $SOCK_DIR
  iotests/201: Create socket in $SOCK_DIR
  iotests/205: Create socket in $SOCK_DIR
  iotests/208: Create socket in $SOCK_DIR
  iotests/209: Create socket in $SOCK_DIR
  iotests/222: Create socket in $SOCK_DIR
  iotests/223: Create socket in $SOCK_DIR
  iotests/240: Create socket in $SOCK_DIR
  iotests/267: Create socket in $SOCK_DIR
  iotests: Drop TEST_DIR filter from _filter_nbd

 python/qemu/machine.py   | 15 +++---
 python/qemu/qtest.py |  9 ++---
 tests/qemu-iotests/083   |  6 +++---
 tests/qemu-iotests/083.out   | 34 
 tests/qemu-iotests/140   |  8 
 tests/qemu-iotests/140.out   |  2 +-
 tests/qemu-iotests/143   |  6 +++---
 tests/qemu-iotests/143.out   |  2 +-
 tests/qemu-iotests/147   |  2 +-
 tests/qemu-iotests/181   |  2 +-
 tests/qemu-iotests/182   |  4 ++--
 tests/qemu-iotests/183   |  2 +-
 tests/qemu-iotests/192   |  4 ++--
 tests/qemu-iotests/192.out   |  2 +-
 tests/qemu-iotests/194   |  4 ++--
 tests/qemu-iotests/201   |  2 +-
 tests/qemu-iotests/205   |  2 +-
 tests/qemu-iotests/208   |  2 +-
 tests/qemu-iotests/209   |  3 ++-
 tests/qemu-iotests/222   |  2 +-
 tests/qemu-iotests/223   | 14 ++---
 tests/qemu-iotests/240   |  4 ++--
 tests/qemu-iotests/241   |  2 --
 tests/qemu-iotests/267   |  4 ++--
 tests/qemu-iotests/267.out   |  2 +-
 tests/qemu-iotests/check | 15 --
 tests/qemu-iotests/common.filter |  7 +--
 tests/qemu-iotests/common.nbd|  2 +-
 tests/qemu-iotests/iotests.py| 16 ---
 29 files changed, 103 insertions(+), 76 deletions(-)

-- 
2.21.0

Re: [PATCH v2 3/3] tests: More iotest 223 improvements

[Max Reitz]

On 15.10.19 21:35, Eric Blake wrote:
> Run the core of the test twice, once without iothreads, and again
> with, for more coverage of both setups.
> 
> Suggested-by: Nir Soffer 
> Signed-off-by: Eric Blake 
> ---
>  tests/qemu-iotests/223 | 16 ++-
>  tests/qemu-iotests/223.out | 85 +-
>  2 files changed, 97 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

[RFC PATCH 13/18] qapi: Create module 'monitor'

[Kevin Wolf]

misc.json contains definitions that are related to the system emulator,
so it can't be used for the storage daemon. This patch moves basic
functionality that is related to the monitor itself into a new
monitor.json, which could be used in tools as well.

Signed-off-by: Kevin Wolf 
---
 qapi/misc.json | 212 
 qapi/monitor.json  | 218 +
 qapi/qapi-schema.json  |   1 +
 monitor/monitor-internal.h |   1 +
 monitor/hmp-cmds.c |   1 +
 monitor/qmp-cmds.c |   1 +
 monitor/qmp.c  |   2 +-
 tests/qmp-test.c   |   2 +-
 ui/gtk.c   |   1 +
 qapi/Makefile.objs |   4 +-
 10 files changed, 227 insertions(+), 216 deletions(-)
 create mode 100644 qapi/monitor.json

diff --git a/qapi/misc.json b/qapi/misc.json
index 6bd11f50e6..7a05c286d5 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -7,159 +7,6 @@
 
 { 'include': 'common.json' }
 
-##
-# @qmp_capabilities:
-#
-# Enable QMP capabilities.
-#
-# Arguments:
-#
-# @enable:   An optional list of QMPCapability values to enable.  The
-#client must not enable any capability that is not
-#mentioned in the QMP greeting message.  If the field is not
-#provided, it means no QMP capabilities will be enabled.
-#(since 2.12)
-#
-# Example:
-#
-# -> { "execute": "qmp_capabilities",
-#  "arguments": { "enable": [ "oob" ] } }
-# <- { "return": {} }
-#
-# Notes: This command is valid exactly when first connecting: it must be
-# issued before any other command will be accepted, and will fail once the
-# monitor is accepting other commands. (see qemu docs/interop/qmp-spec.txt)
-#
-# The QMP client needs to explicitly enable QMP capabilities, otherwise
-# all the QMP capabilities will be turned off by default.
-#
-# Since: 0.13
-#
-##
-{ 'command': 'qmp_capabilities',
-  'data': { '*enable': [ 'QMPCapability' ] },
-  'allow-preconfig': true }
-
-##
-# @QMPCapability:
-#
-# Enumeration of capabilities to be advertised during initial client
-# connection, used for agreeing on particular QMP extension behaviors.
-#
-# @oob:   QMP ability to support out-of-band requests.
-# (Please refer to qmp-spec.txt for more information on OOB)
-#
-# Since: 2.12
-#
-##
-{ 'enum': 'QMPCapability',
-  'data': [ 'oob' ] }
-
-##
-# @VersionTriple:
-#
-# A three-part version number.
-#
-# @major:  The major version number.
-#
-# @minor:  The minor version number.
-#
-# @micro:  The micro version number.
-#
-# Since: 2.4
-##
-{ 'struct': 'VersionTriple',
-  'data': {'major': 'int', 'minor': 'int', 'micro': 'int'} }
-
-
-##
-# @VersionInfo:
-#
-# A description of QEMU's version.
-#
-# @qemu:The version of QEMU.  By current convention, a micro
-#   version of 50 signifies a development branch.  A micro version
-#   greater than or equal to 90 signifies a release candidate for
-#   the next minor version.  A micro version of less than 50
-#   signifies a stable release.
-#
-# @package: QEMU will always set this field to an empty string.  Downstream
-#   versions of QEMU should set this to a non-empty string.  The
-#   exact format depends on the downstream however it highly
-#   recommended that a unique name is used.
-#
-# Since: 0.14.0
-##
-{ 'struct': 'VersionInfo',
-  'data': {'qemu': 'VersionTriple', 'package': 'str'} }
-
-##
-# @query-version:
-#
-# Returns the current version of QEMU.
-#
-# Returns:  A @VersionInfo object describing the current version of QEMU.
-#
-# Since: 0.14.0
-#
-# Example:
-#
-# -> { "execute": "query-version" }
-# <- {
-#   "return":{
-#  "qemu":{
-# "major":0,
-# "minor":11,
-# "micro":5
-#  },
-#  "package":""
-#   }
-#}
-#
-##
-{ 'command': 'query-version', 'returns': 'VersionInfo',
-  'allow-preconfig': true }
-
-##
-# @CommandInfo:
-#
-# Information about a QMP command
-#
-# @name: The command name
-#
-# Since: 0.14.0
-##
-{ 'struct': 'CommandInfo', 'data': {'name': 'str'} }
-
-##
-# @query-commands:
-#
-# Return a list of supported QMP commands by this server
-#
-# Returns: A list of @CommandInfo for all supported commands
-#
-# Since: 0.14.0
-#
-# Example:
-#
-# -> { "execute": "query-commands" }
-# <- {
-#  "return":[
-# {
-#"name":"query-balloon"
-# },
-# {
-#"name":"system_powerdown"
-# }
-#  ]
-#}
-#
-# Note: This example has been shortened as the real response is too long.
-#
-##
-{ 'command': 'query-commands', 'returns': ['CommandInfo'],
-  'allow-preconfig': true }
-
 ##
 # @LostTickPolicy:
 #
@@ -300,48 +147,6 @@
 ##
 { 'command': 'query-uuid', 'returns': 'UuidInfo', 'allow-preconfig': true }
 
-##
-# @EventInfo:
-#
-# Information about a QMP event
-#
-# @name: The event name
-#
-# Since: 1.2.0
-##
-{ 'struct': 'EventInfo', 

[RFC PATCH 18/18] qemu-storage-daemon: Add --monitor option

[Kevin Wolf]

This adds and parses the --monitor option, so that a QMP monitor can be
used in the storage daemon. The monitor offers commands defined in the
QAPI schema at storage-daemon/qapi/qapi-schema.json.

Signed-off-by: Kevin Wolf 
---
 storage-daemon/qapi/qapi-schema.json | 15 
 qemu-storage-daemon.c| 34 
 Makefile | 30 
 Makefile.objs|  4 ++--
 monitor/Makefile.objs|  2 ++
 qapi/Makefile.objs   |  5 
 qom/Makefile.objs|  1 +
 scripts/qapi/gen.py  |  5 
 storage-daemon/Makefile.objs |  1 +
 storage-daemon/qapi/Makefile.objs|  1 +
 10 files changed, 96 insertions(+), 2 deletions(-)
 create mode 100644 storage-daemon/qapi/qapi-schema.json
 create mode 100644 storage-daemon/Makefile.objs
 create mode 100644 storage-daemon/qapi/Makefile.objs

diff --git a/storage-daemon/qapi/qapi-schema.json 
b/storage-daemon/qapi/qapi-schema.json
new file mode 100644
index 00..58c561ebea
--- /dev/null
+++ b/storage-daemon/qapi/qapi-schema.json
@@ -0,0 +1,15 @@
+# -*- Mode: Python -*-
+
+{ 'include': '../../qapi/pragma.json' }
+
+{ 'include': '../../qapi/block.json' }
+{ 'include': '../../qapi/block-core.json' }
+{ 'include': '../../qapi/char.json' }
+{ 'include': '../../qapi/common.json' }
+{ 'include': '../../qapi/crypto.json' }
+{ 'include': '../../qapi/introspect.json' }
+{ 'include': '../../qapi/job.json' }
+{ 'include': '../../qapi/monitor.json' }
+{ 'include': '../../qapi/qom.json' }
+{ 'include': '../../qapi/sockets.json' }
+{ 'include': '../../qapi/transaction.json' }
diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index 46e0a6ea56..4939e6b41f 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -28,12 +28,16 @@
 #include "block/nbd.h"
 #include "chardev/char.h"
 #include "crypto/init.h"
+#include "monitor/monitor.h"
+#include "monitor/monitor-internal.h"
 
 #include "qapi/error.h"
 #include "qapi/qapi-commands-block.h"
 #include "qapi/qapi-commands-block-core.h"
+#include "qapi/qapi-commands-monitor.h"
 #include "qapi/qapi-visit-block.h"
 #include "qapi/qapi-visit-block-core.h"
+#include "qapi/qmp/qstring.h"
 #include "qapi/qobject-input-visitor.h"
 
 #include "qemu-common.h"
@@ -46,6 +50,8 @@
 #include "qemu/option.h"
 #include "qom/object_interfaces.h"
 
+#include "storage-daemon/qapi/qapi-commands.h"
+
 #include "sysemu/runstate.h"
 #include "trace/control.h"
 
@@ -58,6 +64,11 @@ void qemu_system_killed(int signal, pid_t pid)
 exit_requested = true;
 }
 
+void qmp_quit(Error **errp)
+{
+exit_requested = true;
+}
+
 static void help(void)
 {
 printf(
@@ -101,6 +112,7 @@ enum {
 OPTION_OBJECT = 256,
 OPTION_BLOCKDEV,
 OPTION_CHARDEV,
+OPTION_MONITOR,
 OPTION_NBD_SERVER,
 OPTION_EXPORT,
 };
@@ -116,6 +128,17 @@ static QemuOptsList qemu_object_opts = {
 },
 };
 
+static void init_qmp_commands(void)
+{
+qmp_init_marshal(_commands);
+qmp_register_command(_commands, "query-qmp-schema",
+ qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
+
+QTAILQ_INIT(_cap_negotiation_commands);
+qmp_register_command(_cap_negotiation_commands, "qmp_capabilities",
+ qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
+}
+
 static void init_export(BlockExport *export, Error **errp)
 {
 switch (export->type) {
@@ -138,6 +161,7 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 {"object", required_argument, 0, OPTION_OBJECT},
 {"blockdev", required_argument, 0, OPTION_BLOCKDEV},
 {"chardev", required_argument, 0, OPTION_CHARDEV},
+{"monitor", required_argument, 0, OPTION_MONITOR},
 {"nbd-server", required_argument, 0, OPTION_NBD_SERVER},
 {"export", required_argument, 0, OPTION_EXPORT},
 {"version", no_argument, 0, 'V'},
@@ -208,6 +232,14 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 qemu_opts_del(opts);
 break;
 }
+case OPTION_MONITOR:
+{
+QemuOpts *opts = qemu_opts_parse(_mon_opts,
+ optarg, true, _fatal);
+monitor_init_opts(opts, false, _fatal);
+qemu_opts_del(opts);
+break;
+}
 case OPTION_NBD_SERVER:
 {
 Visitor *v;
@@ -272,6 +304,8 @@ int main(int argc, char *argv[])
 qemu_add_opts(_trace_opts);
 qcrypto_init(_fatal);
 bdrv_init();
+monitor_init_globals_core();
+init_qmp_commands();
 
 if (qemu_init_main_loop(_err)) {
 error_report_err(local_err);
diff --git a/Makefile b/Makefile
index 0e3e98582d..e367d2b28a 100644
--- a/Makefile
+++ b/Makefile
@@ -121,7 +121,26 @@ GENERATED_QAPI_FILES += 
$(QAPI_MODULES:%=qapi/qapi-events-%.c)
 

[RFC PATCH 17/18] monitor: Move qmp_query_qmp_schema to qmp-cmds-monitor.c

[Kevin Wolf]

monitor/misc.c contains code that works only in the system emulator, so
it can't be linked to the storage daemon. In order to make schema
introspection available for the storage daemon, move the function to
monitor/qmp-cmds-monitor.c, which can be linked into the storage daemon.

Signed-off-by: Kevin Wolf 
---
 monitor/monitor-internal.h |  3 +++
 monitor/misc.c | 16 
 monitor/qmp-cmds-monitor.c | 16 
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 451aa64c1a..b91cb09ee5 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -180,4 +180,7 @@ void help_cmd(Monitor *mon, const char *name);
 void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
 int hmp_compare_cmd(const char *name, const char *list);
 
+void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
+ Error **errp);
+
 #endif
diff --git a/monitor/misc.c b/monitor/misc.c
index 95c87df09b..4c90c6813e 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -69,7 +69,6 @@
 #include "qapi/qapi-commands.h"
 #include "qapi/error.h"
 #include "qapi/qmp-event.h"
-#include "qapi/qapi-introspect.h"
 #include "sysemu/cpus.h"
 #include "qemu/cutils.h"
 #include "tcg/tcg.h"
@@ -229,21 +228,6 @@ static void hmp_info_help(Monitor *mon, const QDict *qdict)
 help_cmd(mon, "info");
 }
 
-/*
- * Minor hack: generated marshalling suppressed for this command
- * ('gen': false in the schema) so we can parse the JSON string
- * directly into QObject instead of first parsing it with
- * visit_type_SchemaInfoList() into a SchemaInfoList, then marshal it
- * to QObject with generated output marshallers, every time.  Instead,
- * we do it in test-qobject-input-visitor.c, just to make sure
- * qapi-gen.py's output actually conforms to the schema.
- */
-static void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
- Error **errp)
-{
-*ret_data = qobject_from_qlit(_schema_qlit);
-}
-
 static void monitor_init_qmp_commands(void)
 {
 /*
diff --git a/monitor/qmp-cmds-monitor.c b/monitor/qmp-cmds-monitor.c
index acebfd3716..7215392f3e 100644
--- a/monitor/qmp-cmds-monitor.c
+++ b/monitor/qmp-cmds-monitor.c
@@ -29,6 +29,7 @@
 #include "qapi/error.h"
 #include "qapi/qapi-commands-monitor.h"
 #include "qapi/qapi-emit-events.h"
+#include "qapi/qapi-introspect.h"
 
 /*
  * Accept QMP capabilities in @list for @mon.
@@ -151,3 +152,18 @@ EventInfoList *qmp_query_events(Error **errp)
 
 return ev_list;
 }
+
+/*
+ * Minor hack: generated marshalling suppressed for this command
+ * ('gen': false in the schema) so we can parse the JSON string
+ * directly into QObject instead of first parsing it with
+ * visit_type_SchemaInfoList() into a SchemaInfoList, then marshal it
+ * to QObject with generated output marshallers, every time.  Instead,
+ * we do it in test-qobject-input-visitor.c, just to make sure
+ * qapi-gen.py's output actually conforms to the schema.
+ */
+void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
+ Error **errp)
+{
+*ret_data = qobject_from_qlit(_schema_qlit);
+}
-- 
2.20.1

[RFC PATCH 14/18] monitor: Create monitor/qmp-cmds-monitor.c

[Kevin Wolf]

Move all of the QMP commands handlers to implement the 'monitor' module
(qapi/monitor.json) that can be shared between the system emulator and
the storage daemon to a new file monitor/qmp-cmds-monitor.c.

Signed-off-by: Kevin Wolf 
---
 monitor/misc.c | 110 --
 monitor/qmp-cmds-monitor.c | 153 +
 monitor/qmp-cmds.c |  14 
 monitor/Makefile.objs  |   3 +-
 4 files changed, 155 insertions(+), 125 deletions(-)
 create mode 100644 monitor/qmp-cmds-monitor.c

diff --git a/monitor/misc.c b/monitor/misc.c
index aef16f6cfb..95c87df09b 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -67,7 +67,6 @@
 #include "qemu/thread.h"
 #include "block/qapi.h"
 #include "qapi/qapi-commands.h"
-#include "qapi/qapi-emit-events.h"
 #include "qapi/error.h"
 #include "qapi/qmp-event.h"
 #include "qapi/qapi-introspect.h"
@@ -230,58 +229,6 @@ static void hmp_info_help(Monitor *mon, const QDict *qdict)
 help_cmd(mon, "info");
 }
 
-static void query_commands_cb(QmpCommand *cmd, void *opaque)
-{
-CommandInfoList *info, **list = opaque;
-
-if (!cmd->enabled) {
-return;
-}
-
-info = g_malloc0(sizeof(*info));
-info->value = g_malloc0(sizeof(*info->value));
-info->value->name = g_strdup(cmd->name);
-info->next = *list;
-*list = info;
-}
-
-CommandInfoList *qmp_query_commands(Error **errp)
-{
-CommandInfoList *list = NULL;
-MonitorQMP *mon;
-
-assert(monitor_is_qmp(cur_mon));
-mon = container_of(cur_mon, MonitorQMP, common);
-
-qmp_for_each_command(mon->commands, query_commands_cb, );
-
-return list;
-}
-
-EventInfoList *qmp_query_events(Error **errp)
-{
-/*
- * TODO This deprecated command is the only user of
- * QAPIEvent_str() and QAPIEvent_lookup[].  When the command goes,
- * they should go, too.
- */
-EventInfoList *info, *ev_list = NULL;
-QAPIEvent e;
-
-for (e = 0 ; e < QAPI_EVENT__MAX ; e++) {
-const char *event_name = QAPIEvent_str(e);
-assert(event_name != NULL);
-info = g_malloc0(sizeof(*info));
-info->value = g_malloc0(sizeof(*info->value));
-info->value->name = g_strdup(event_name);
-
-info->next = ev_list;
-ev_list = info;
-}
-
-return ev_list;
-}
-
 /*
  * Minor hack: generated marshalling suppressed for this command
  * ('gen': false in the schema) so we can parse the JSON string
@@ -320,63 +267,6 @@ static void monitor_init_qmp_commands(void)
  qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
 }
 
-/*
- * Accept QMP capabilities in @list for @mon.
- * On success, set mon->qmp.capab[], and return true.
- * On error, set @errp, and return false.
- */
-static bool qmp_caps_accept(MonitorQMP *mon, QMPCapabilityList *list,
-Error **errp)
-{
-GString *unavailable = NULL;
-bool capab[QMP_CAPABILITY__MAX];
-
-memset(capab, 0, sizeof(capab));
-
-for (; list; list = list->next) {
-if (!mon->capab_offered[list->value]) {
-if (!unavailable) {
-unavailable = g_string_new(QMPCapability_str(list->value));
-} else {
-g_string_append_printf(unavailable, ", %s",
-  QMPCapability_str(list->value));
-}
-}
-capab[list->value] = true;
-}
-
-if (unavailable) {
-error_setg(errp, "Capability %s not available", unavailable->str);
-g_string_free(unavailable, true);
-return false;
-}
-
-memcpy(mon->capab, capab, sizeof(capab));
-return true;
-}
-
-void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
-  Error **errp)
-{
-MonitorQMP *mon;
-
-assert(monitor_is_qmp(cur_mon));
-mon = container_of(cur_mon, MonitorQMP, common);
-
-if (mon->commands == _commands) {
-error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
-  "Capabilities negotiation is already complete, command "
-  "ignored");
-return;
-}
-
-if (!qmp_caps_accept(mon, enable, errp)) {
-return;
-}
-
-mon->commands = _commands;
-}
-
 /* Set the current CPU defined by the user. Callers must hold BQL. */
 int monitor_set_cpu(int cpu_index)
 {
diff --git a/monitor/qmp-cmds-monitor.c b/monitor/qmp-cmds-monitor.c
new file mode 100644
index 00..acebfd3716
--- /dev/null
+++ b/monitor/qmp-cmds-monitor.c
@@ -0,0 +1,153 @@
+/*
+ * QMP commands related to the monitor (common functions for sysemu and tools)
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of 

[RFC PATCH 16/18] qapi: Create 'pragma' module

[Kevin Wolf]

We want to share the whitelists between the system emulator schema and
the storage daemon schema, so move all the pragmas from the main schema
file into a separate file that can be included from both.

Signed-off-by: Kevin Wolf 
---
 qapi/pragma.json  | 24 
 qapi/qapi-schema.json | 25 +
 qapi/Makefile.objs|  2 +-
 3 files changed, 26 insertions(+), 25 deletions(-)
 create mode 100644 qapi/pragma.json

diff --git a/qapi/pragma.json b/qapi/pragma.json
new file mode 100644
index 00..cffae27666
--- /dev/null
+++ b/qapi/pragma.json
@@ -0,0 +1,24 @@
+{ 'pragma': { 'doc-required': true } }
+
+# Whitelists to permit QAPI rule violations; think twice before you
+# add to them!
+{ 'pragma': {
+# Commands allowed to return a non-dictionary:
+'returns-whitelist': [
+'human-monitor-command',
+'qom-get',
+'query-migrate-cache-size',
+'query-tpm-models',
+'query-tpm-types',
+'ringbuf-read' ],
+'name-case-whitelist': [
+'ACPISlotType', # DIMM, visible through 
query-acpi-ospm-status
+'CpuInfoMIPS',  # PC, visible through query-cpu
+'CpuInfoTricore',   # PC, visible through query-cpu
+'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings
+'BlockdevVmdkAdapterType',  # legacyESX, to match VMDK spec spellings
+'QapiErrorClass',   # all members, visible through errors
+'UuidInfo', # UUID, visible through query-uuid
+'X86CPURegister32', # all members, visible indirectly through 
qom-get
+'CpuInfo'   # CPU, visible through query-cpu
+] } }
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index be90422ffe..85b4048535 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -49,30 +49,7 @@
 #
 ##
 
-{ 'pragma': { 'doc-required': true } }
-
-# Whitelists to permit QAPI rule violations; think twice before you
-# add to them!
-{ 'pragma': {
-# Commands allowed to return a non-dictionary:
-'returns-whitelist': [
-'human-monitor-command',
-'qom-get',
-'query-migrate-cache-size',
-'query-tpm-models',
-'query-tpm-types',
-'ringbuf-read' ],
-'name-case-whitelist': [
-'ACPISlotType', # DIMM, visible through 
query-acpi-ospm-status
-'CpuInfoMIPS',  # PC, visible through query-cpu
-'CpuInfoTricore',   # PC, visible through query-cpu
-'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings
-'BlockdevVmdkAdapterType',  # legacyESX, to match VMDK spec spellings
-'QapiErrorClass',   # all members, visible through errors
-'UuidInfo', # UUID, visible through query-uuid
-'X86CPURegister32', # all members, visible indirectly through 
qom-get
-'CpuInfo'   # CPU, visible through query-cpu
-] } }
+{ 'include': 'pragma.json' }
 
 # Documentation generated with qapi-gen.py is in source order, with
 # included sub-schemas inserted at the first include directive
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index 519b6f1a8e..3e04e299ed 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -7,7 +7,7 @@ util-obj-y += qapi-util.o
 
 QAPI_COMMON_MODULES = audio authz block-core block char common crypto
 QAPI_COMMON_MODULES += dump error introspect job machine migration misc monitor
-QAPI_COMMON_MODULES += net qdev qom rdma rocker run-state sockets tpm
+QAPI_COMMON_MODULES += net pragma qdev qom rdma rocker run-state sockets tpm
 QAPI_COMMON_MODULES += trace transaction ui
 QAPI_TARGET_MODULES = machine-target misc-target
 QAPI_MODULES = $(QAPI_COMMON_MODULES) $(QAPI_TARGET_MODULES)
-- 
2.20.1

[RFC PATCH 15/18] qapi: Support empty modules

[Kevin Wolf]

If you added an include file that doesn't contain any definitions, no
source files would be generated for it. However, in other source files,
you would still get an #include for the header files of the empty
module.

The intended behaviour is that empty source files are created for empty
modules. This patch makes QAPISchema keep a list of all modules
(including empty ones) and modifies visit() to first visit all modules
in that list.

Some test reference outputs need to be updated due to the additional
visitor calls.

Signed-off-by: Kevin Wolf 
---
 scripts/qapi/schema.py   | 9 +
 tests/qapi-schema/comments.out   | 2 ++
 tests/qapi-schema/doc-bad-section.out| 2 ++
 tests/qapi-schema/doc-good.out   | 2 ++
 tests/qapi-schema/empty.out  | 2 ++
 tests/qapi-schema/event-case.out | 2 ++
 tests/qapi-schema/include-repetition.out | 4 
 tests/qapi-schema/include-simple.out | 3 +++
 tests/qapi-schema/indented-expr.out  | 2 ++
 tests/qapi-schema/qapi-schema-test.out   | 4 
 10 files changed, 32 insertions(+)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 38041098bd..e1b034d67d 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -749,6 +749,7 @@ class QAPISchema(object):
 self.docs = parser.docs
 self._entity_list = []
 self._entity_dict = {}
+self._modules = [os.path.basename(fname)]
 self._predefining = True
 self._def_predefineds()
 self._predefining = False
@@ -800,6 +801,8 @@ class QAPISchema(object):
 main_info = main_info.parent
 fname = os.path.relpath(include, os.path.dirname(main_info.fname))
 self._def_entity(QAPISchemaInclude(fname, info))
+if fname not in self._modules:
+self._modules.append(fname)
 
 def _def_builtin_type(self, name, json_type, c_type):
 self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
@@ -1033,6 +1036,12 @@ class QAPISchema(object):
 visitor.visit_begin(self)
 module = None
 visitor.visit_module(module)
+
+# Make sure that all modules are visited, even if they contain no
+# entities
+for module in self._modules:
+visitor.visit_module(module)
+
 for entity in self._entity_list:
 if visitor.visit_needed(entity):
 if entity.module != module:
diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index 273f0f54e1..fa7e95d1cc 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,4 +1,6 @@
 module None
+module comments.json
+module None
 object q_empty
 enum QType
 prefix QTYPE
diff --git a/tests/qapi-schema/doc-bad-section.out 
b/tests/qapi-schema/doc-bad-section.out
index 367e2a1c3e..331237cfbe 100644
--- a/tests/qapi-schema/doc-bad-section.out
+++ b/tests/qapi-schema/doc-bad-section.out
@@ -1,4 +1,6 @@
 module None
+module doc-bad-section.json
+module None
 object q_empty
 enum QType
 prefix QTYPE
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index d3bca343eb..8f3577bb21 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -1,4 +1,6 @@
 module None
+module doc-good.json
+module None
 object q_empty
 enum QType
 prefix QTYPE
diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out
index 5b53d00702..3671cbbe59 100644
--- a/tests/qapi-schema/empty.out
+++ b/tests/qapi-schema/empty.out
@@ -1,4 +1,6 @@
 module None
+module empty.json
+module None
 object q_empty
 enum QType
 prefix QTYPE
diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out
index ec8a1406e4..2b2d8548e9 100644
--- a/tests/qapi-schema/event-case.out
+++ b/tests/qapi-schema/event-case.out
@@ -1,4 +1,6 @@
 module None
+module event-case.json
+module None
 object q_empty
 enum QType
 prefix QTYPE
diff --git a/tests/qapi-schema/include-repetition.out 
b/tests/qapi-schema/include-repetition.out
index 5423983239..ebaac1813d 100644
--- a/tests/qapi-schema/include-repetition.out
+++ b/tests/qapi-schema/include-repetition.out
@@ -1,4 +1,8 @@
 module None
+module include-repetition.json
+module comments.json
+module include-repetition-sub.json
+module None
 object q_empty
 enum QType
 prefix QTYPE
diff --git a/tests/qapi-schema/include-simple.out 
b/tests/qapi-schema/include-simple.out
index 061f81e509..dea51f9738 100644
--- a/tests/qapi-schema/include-simple.out
+++ b/tests/qapi-schema/include-simple.out
@@ -1,4 +1,7 @@
 module None
+module include-simple.json
+module include-simple-sub.json
+module None
 object q_empty
 enum QType
 prefix QTYPE
diff --git a/tests/qapi-schema/indented-expr.out 
b/tests/qapi-schema/indented-expr.out
index bffdf6756d..d4cffb9c1b 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,4 +1,6 @@
 module None
+module indented-expr.json
+module None

[RFC PATCH 07/18] blockdev-nbd: Boxed argument type for nbd-server-add

[Kevin Wolf]

Move the arguments of nbd-server-add to a new struct BlockExportNbd and
convert the command to 'boxed': true. This makes it easier to share code
with the storage daemon.

Signed-off-by: Kevin Wolf 
---
 qapi/block.json| 20 +++-
 blockdev-nbd.c | 25 -
 monitor/hmp-cmds.c | 21 +
 3 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 7fe0cf6538..567f116875 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -259,9 +259,9 @@
 '*tls-authz': 'str'} }
 
 ##
-# @nbd-server-add:
+# @BlockExportNbd:
 #
-# Export a block node to QEMU's embedded NBD server.
+# An NBD block export.
 #
 # @device: The device name or node name of the node to be exported
 #
@@ -270,19 +270,29 @@
 #
 # @writable: Whether clients should be able to write to the device via the
 # NBD connection (default false).
-
+#
 # @bitmap: Also export the dirty bitmap reachable from @device, so the
 #  NBD client can use NBD_OPT_SET_META_CONTEXT with
 #  "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
 #
+# Since: 4.2
+##
+{ 'struct': 'BlockExportNbd',
+  'data': {'device': 'str', '*name': 'str', '*writable': 'bool',
+   '*bitmap': 'str' } }
+
+##
+# @nbd-server-add:
+#
+# Export a block node to QEMU's embedded NBD server.
+#
 # Returns: error if the server is not running, or export with the same name
 #  already exists.
 #
 # Since: 1.3.0
 ##
 { 'command': 'nbd-server-add',
-  'data': {'device': 'str', '*name': 'str', '*writable': 'bool',
-   '*bitmap': 'str' } }
+  'data': 'BlockExportNbd', 'boxed': true }
 
 ##
 # @NbdServerRemoveMode:
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index d4c1fd4166..ee8262cd4c 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -148,9 +148,7 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
 qapi_free_SocketAddress(addr_flat);
 }
 
-void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
-bool has_writable, bool writable,
-bool has_bitmap, const char *bitmap, Error **errp)
+void qmp_nbd_server_add(BlockExportNbd *arg, Error **errp)
 {
 BlockDriverState *bs = NULL;
 BlockBackend *on_eject_blk;
@@ -163,18 +161,18 @@ void qmp_nbd_server_add(const char *device, bool 
has_name, const char *name,
 return;
 }
 
-if (!has_name) {
-name = device;
+if (!arg->has_name) {
+arg->name = arg->device;
 }
 
-if (nbd_export_find(name)) {
-error_setg(errp, "NBD server already has export named '%s'", name);
+if (nbd_export_find(arg->name)) {
+error_setg(errp, "NBD server already has export named '%s'", 
arg->name);
 return;
 }
 
-on_eject_blk = blk_by_name(device);
+on_eject_blk = blk_by_name(arg->device);
 
-bs = bdrv_lookup_bs(device, device, errp);
+bs = bdrv_lookup_bs(arg->device, arg->device, errp);
 if (!bs) {
 return;
 }
@@ -188,14 +186,15 @@ void qmp_nbd_server_add(const char *device, bool 
has_name, const char *name,
 goto out;
 }
 
-if (!has_writable) {
-writable = false;
+if (!arg->has_writable) {
+arg->writable = false;
 }
 if (bdrv_is_read_only(bs)) {
-writable = false;
+arg->writable = false;
 }
 
-exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable,
+exp = nbd_export_new(bs, 0, len, arg->name, NULL, arg->bitmap,
+ !arg->writable, !arg->writable,
  NULL, false, on_eject_blk, errp);
 if (!exp) {
 goto out;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b2551c16d1..22d7e6e222 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2320,6 +2320,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict 
*qdict)
 Error *local_err = NULL;
 BlockInfoList *block_list, *info;
 SocketAddress *addr;
+BlockExportNbd export;
 
 if (writable && !all) {
 error_setg(_err, "-w only valid together with -a");
@@ -2352,8 +2353,13 @@ void hmp_nbd_server_start(Monitor *mon, const QDict 
*qdict)
 continue;
 }
 
-qmp_nbd_server_add(info->value->device, false, NULL,
-   true, writable, false, NULL, _err);
+export = (BlockExportNbd) {
+.device = info->value->device,
+.has_writable   = true,
+.writable   = writable,
+};
+
+qmp_nbd_server_add(, _err);
 
 if (local_err != NULL) {
 qmp_nbd_server_stop(NULL);
@@ -2374,8 +2380,15 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
 bool writable = qdict_get_try_bool(qdict, "writable", false);
 Error *local_err = NULL;
 
-qmp_nbd_server_add(device, !!name, name, true, writable,
-   false, NULL, _err);
+BlockExportNbd export = {
+.device

[RFC PATCH 09/18] qemu-storage-daemon: Add main loop

[Kevin Wolf]

Instead of exiting after processing all command line options, start a
main loop and keep processing events until exit is requested with a
signal (e.g. SIGINT).

Now qemu-storage-daemon can be used as an alternative for qemu-nbd that
provides a few features that were previously only available from QMP,
such as access to options only available with -blockdev and the socket
types 'vsock' and 'fd'.

Signed-off-by: Kevin Wolf 
---
 qemu-storage-daemon.c | 13 +
 Makefile.objs |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index 9e5f474fd0..099388f645 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -45,10 +45,18 @@
 #include "qemu/option.h"
 #include "qom/object_interfaces.h"
 
+#include "sysemu/runstate.h"
 #include "trace/control.h"
 
 #include 
 
+static bool exit_requested = false;
+
+void qemu_system_killed(int signal, pid_t pid)
+{
+exit_requested = true;
+}
+
 static void help(void)
 {
 printf(
@@ -238,6 +246,7 @@ int main(int argc, char *argv[])
 
 error_init(argv[0]);
 qemu_init_exec_dir(argv[0]);
+os_setup_signal_handling();
 
 module_call_init(MODULE_INIT_QOM);
 module_call_init(MODULE_INIT_TRACE);
@@ -256,5 +265,9 @@ int main(int argc, char *argv[])
 return EXIT_FAILURE;
 }
 
+while (!exit_requested) {
+main_loop_wait(false);
+}
+
 return EXIT_SUCCESS;
 }
diff --git a/Makefile.objs b/Makefile.objs
index cc262e445f..b667d3f07b 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -43,6 +43,8 @@ io-obj-y = io/
 
 storage-daemon-obj-y = block/
 storage-daemon-obj-y += blockdev.o blockdev-nbd.o iothread.o
+storage-daemon-obj-$(CONFIG_WIN32) += os-win32.o
+storage-daemon-obj-$(CONFIG_POSIX) += os-posix.o
 
 ##
 # Target independent part of system emulation. The long term path is to
-- 
2.20.1

[RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option

[Kevin Wolf]

Add a --nbd-server option to qemu-storage-daemon to start the built-in
NBD server right away. It maps the arguments for nbd-server-start to the
command line. Example (only with required options):

--nbd-server addr.type=inet,addr.host=localhost,addr.port=10809

Signed-off-by: Kevin Wolf 
---
 qapi/block.json   | 18 ++
 include/block/nbd.h   |  1 +
 blockdev-nbd.c|  5 +
 qemu-storage-daemon.c | 26 +-
 Makefile.objs |  2 +-
 5 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 145c268bb6..7fe0cf6538 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -215,6 +215,24 @@
 '*id': 'str',
 '*force': 'bool' } }
 
+##
+# @NbdServerOptions:
+#
+# @addr: Address on which to listen.
+# @tls-creds: ID of the TLS credentials object (since 2.6).
+# @tls-authz: ID of the QAuthZ authorization object used to validate
+# the client's x509 distinguished name. This object is
+# is only resolved at time of use, so can be deleted and
+# recreated on the fly while the NBD server is active.
+# If missing, it will default to denying access (since 4.0).
+#
+# Since: 4.2
+##
+{ 'struct': 'NbdServerOptions',
+  'data': { 'addr': 'SocketAddress',
+'*tls-creds': 'str',
+'*tls-authz': 'str'} }
+
 ##
 # @nbd-server-start:
 #
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 316fd705a9..2a7441491a 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -353,6 +353,7 @@ void nbd_client_put(NBDClient *client);
 
 void nbd_server_start(SocketAddress *addr, const char *tls_creds,
   const char *tls_authz, Error **errp);
+void nbd_server_start_options(NbdServerOptions *arg, Error **errp);
 
 /* nbd_read
  * Reads @size bytes from @ioc. Returns 0 on success.
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 6a8b206e1d..d4c1fd4166 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -132,6 +132,11 @@ void nbd_server_start(SocketAddress *addr, const char 
*tls_creds,
 nbd_server = NULL;
 }
 
+void nbd_server_start_options(NbdServerOptions *arg, Error **errp)
+{
+nbd_server_start(arg->addr, arg->tls_creds, arg->tls_authz, errp);
+}
+
 void qmp_nbd_server_start(SocketAddressLegacy *addr,
   bool has_tls_creds, const char *tls_creds,
   bool has_tls_authz, const char *tls_authz,
diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index 904e3c3a46..51882452f3 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -25,11 +25,14 @@
 #include "qemu/osdep.h"
 
 #include "block/block.h"
+#include "block/nbd.h"
 #include "crypto/init.h"
 
 #include "qapi/error.h"
-#include "qapi/qapi-visit-block-core.h"
+#include "qapi/qapi-commands-block.h"
 #include "qapi/qapi-commands-block-core.h"
+#include "qapi/qapi-visit-block.h"
+#include "qapi/qapi-visit-block-core.h"
 #include "qapi/qobject-input-visitor.h"
 
 #include "qemu-common.h"
@@ -64,6 +67,12 @@ static void help(void)
 " [,driver specific parameters...]\n"
 " configure a block backend\n"
 "\n"
+"  --nbd-server addr.type=inet,addr.host=,addr.port=\n"
+"   [,tls-creds=][,tls-authz=]\n"
+"  --nbd-server addr.type=unix,addr.path=\n"
+"   [,tls-creds=][,tls-authz=]\n"
+" start an NBD server for exporting block nodes\n"
+"\n"
 "  --object   define a QOM object such as 'secret' for\n"
 " passwords and/or encryption keys\n"
 "\n"
@@ -74,6 +83,7 @@ QEMU_HELP_BOTTOM "\n",
 enum {
 OPTION_OBJECT = 256,
 OPTION_BLOCKDEV,
+OPTION_NBD_SERVER,
 };
 
 static QemuOptsList qemu_object_opts = {
@@ -95,6 +105,7 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 {"help", no_argument, 0, 'h'},
 {"object", required_argument, 0, OPTION_OBJECT},
 {"blockdev", required_argument, 0, OPTION_BLOCKDEV},
+{"nbd-server", required_argument, 0, OPTION_NBD_SERVER},
 {"version", no_argument, 0, 'V'},
 {"trace", required_argument, NULL, 'T'},
 {0, 0, 0, 0}
@@ -152,6 +163,19 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 qapi_free_BlockdevOptions(options);
 break;
 }
+case OPTION_NBD_SERVER:
+{
+Visitor *v;
+NbdServerOptions *options;
+
+v = qobject_input_visitor_new_str(optarg, NULL, _fatal);
+visit_type_NbdServerOptions(v, NULL, , _fatal);
+visit_free(v);
+
+nbd_server_start_options(options, _fatal);
+qapi_free_NbdServerOptions(options);
+break;
+}
 }
 }
 if (optind != argc) {
diff --git a/Makefile.objs b/Makefile.objs
index 00fdf54500..cc262e445f 100644
--- a/Makefile.objs
+++ 

[RFC PATCH 10/18] qemu-storage-daemon: Add --chardev option

[Kevin Wolf]

This adds a --chardev option to the storage daemon that works the same
as the -chardev option of the system emulator.

Signed-off-by: Kevin Wolf 
---
 qemu-storage-daemon.c | 19 +++
 Makefile  |  2 +-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index 099388f645..46e0a6ea56 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -26,6 +26,7 @@
 
 #include "block/block.h"
 #include "block/nbd.h"
+#include "chardev/char.h"
 #include "crypto/init.h"
 
 #include "qapi/error.h"
@@ -75,6 +76,9 @@ static void help(void)
 " [,driver specific parameters...]\n"
 " configure a block backend\n"
 "\n"
+"  --chardev configure a character device backend\n"
+" (see the qemu(1) man page for possible options)\n"
+"\n"
 "  --export [type=]nbd,device=[,name=]\n"
 "   [,writable=on|off][,bitmap=]\n"
 " export the specified block node over NBD\n"
@@ -96,10 +100,13 @@ QEMU_HELP_BOTTOM "\n",
 enum {
 OPTION_OBJECT = 256,
 OPTION_BLOCKDEV,
+OPTION_CHARDEV,
 OPTION_NBD_SERVER,
 OPTION_EXPORT,
 };
 
+extern QemuOptsList qemu_chardev_opts;
+
 static QemuOptsList qemu_object_opts = {
 .name = "object",
 .implied_opt_name = "qom-type",
@@ -130,6 +137,7 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 {"help", no_argument, 0, 'h'},
 {"object", required_argument, 0, OPTION_OBJECT},
 {"blockdev", required_argument, 0, OPTION_BLOCKDEV},
+{"chardev", required_argument, 0, OPTION_CHARDEV},
 {"nbd-server", required_argument, 0, OPTION_NBD_SERVER},
 {"export", required_argument, 0, OPTION_EXPORT},
 {"version", no_argument, 0, 'V'},
@@ -189,6 +197,17 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 qapi_free_BlockdevOptions(options);
 break;
 }
+case OPTION_CHARDEV:
+{
+QemuOpts *opts = qemu_opts_parse(_chardev_opts,
+ optarg, true, _fatal);
+if (!qemu_chr_new_from_opts(opts, NULL, _fatal)) {
+/* No error, but NULL returned means help was printed */
+exit(EXIT_SUCCESS);
+}
+qemu_opts_del(opts);
+break;
+}
 case OPTION_NBD_SERVER:
 {
 Visitor *v;
diff --git a/Makefile b/Makefile
index b913d4d736..0e3e98582d 100644
--- a/Makefile
+++ b/Makefile
@@ -561,7 +561,7 @@ qemu-img.o: qemu-img-cmds.h
 qemu-img$(EXESUF): qemu-img.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) 
$(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 qemu-nbd$(EXESUF): qemu-nbd.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) 
$(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 qemu-io$(EXESUF): qemu-io.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) 
$(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
-qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) 
$(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(storage-daemon-obj-y) 
$(COMMON_LDADDS)
+qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) 
$(block-obj-y) $(crypto-obj-y) $(chardev-obj-y) $(io-obj-y) $(qom-obj-y) 
$(storage-daemon-obj-y) $(COMMON_LDADDS)
 
 qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o $(COMMON_LDADDS)
 
-- 
2.20.1

[RFC PATCH 11/18] monitor: Move monitor option parsing to monitor/monitor.c

[Kevin Wolf]

Both the system emulators and the storage daemon will need to parse
monitor options, so move that code to monitor/monitor.c, which can be
linked into binaries that aren't a system emulator.

This patch moves the monitor option parsing from vl.c and adds an
allow_hmp parameter so that callers can support QMP without HMP.

Signed-off-by: Kevin Wolf 
---
 include/monitor/monitor.h |  4 +++
 include/sysemu/sysemu.h   |  1 -
 monitor/monitor.c | 52 +++
 vl.c  | 45 +
 4 files changed, 57 insertions(+), 45 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index a81eeff5f8..d3e8da36a5 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -3,6 +3,7 @@
 
 #include "block/block.h"
 #include "qapi/qapi-types-misc.h"
+#include "qemu/option.h"
 #include "qemu/readline.h"
 
 extern __thread Monitor *cur_mon;
@@ -10,12 +11,15 @@ typedef struct MonitorHMP MonitorHMP;
 
 #define QMP_REQ_QUEUE_LEN_MAX 8
 
+extern QemuOptsList qemu_mon_opts;
+
 bool monitor_cur_is_qmp(void);
 
 void monitor_init_globals(void);
 void monitor_init_globals_core(void);
 void monitor_init_qmp(Chardev *chr, bool pretty);
 void monitor_init_hmp(Chardev *chr, bool use_readline);
+int monitor_init_opts(QemuOpts *opts, bool allow_hmp, Error **errp);
 void monitor_cleanup(void);
 
 int monitor_suspend(Monitor *mon);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 44f18eb739..e54934c311 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -124,7 +124,6 @@ extern QemuOptsList qemu_netdev_opts;
 extern QemuOptsList qemu_nic_opts;
 extern QemuOptsList qemu_net_opts;
 extern QemuOptsList qemu_global_opts;
-extern QemuOptsList qemu_mon_opts;
 extern QemuOptsList qemu_semihosting_config_opts;
 
 #endif
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 12898b6448..316b71b928 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -609,6 +609,58 @@ void monitor_init_globals_core(void)
NULL);
 }
 
+int monitor_init_opts(QemuOpts *opts, bool allow_hmp, Error **errp)
+{
+Chardev *chr;
+bool qmp;
+bool pretty = false;
+const char *chardev;
+const char *mode;
+
+mode = qemu_opt_get(opts, "mode");
+if (mode == NULL) {
+mode = allow_hmp ? "readline" : "control";
+}
+if (strcmp(mode, "readline") == 0) {
+qmp = false;
+} else if (strcmp(mode, "control") == 0) {
+qmp = true;
+} else {
+error_setg(errp, "unknown monitor mode \"%s\"", mode);
+return -1;
+}
+if (!allow_hmp && !qmp) {
+error_setg(errp, "Only QMP is supported");
+return -1;
+}
+
+if (!qmp && qemu_opt_get(opts, "pretty")) {
+warn_report("'pretty' is deprecated for HMP monitors, it has no effect 
"
+"and will be removed in future versions");
+}
+if (qemu_opt_get_bool(opts, "pretty", 0)) {
+pretty = true;
+}
+
+chardev = qemu_opt_get(opts, "chardev");
+if (!chardev) {
+error_report("chardev is required");
+exit(1);
+}
+chr = qemu_chr_find(chardev);
+if (chr == NULL) {
+error_setg(errp, "chardev \"%s\" not found", chardev);
+return -1;
+}
+
+if (qmp) {
+monitor_init_qmp(chr, pretty);
+} else {
+monitor_init_hmp(chr, true);
+}
+return 0;
+}
+
 QemuOptsList qemu_mon_opts = {
 .name = "mon",
 .implied_opt_name = "chardev",
diff --git a/vl.c b/vl.c
index 4489cfb2bb..e9d9a44acf 100644
--- a/vl.c
+++ b/vl.c
@@ -2234,50 +2234,7 @@ static int fsdev_init_func(void *opaque, QemuOpts *opts, 
Error **errp)
 
 static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
 {
-Chardev *chr;
-bool qmp;
-bool pretty = false;
-const char *chardev;
-const char *mode;
-
-mode = qemu_opt_get(opts, "mode");
-if (mode == NULL) {
-mode = "readline";
-}
-if (strcmp(mode, "readline") == 0) {
-qmp = false;
-} else if (strcmp(mode, "control") == 0) {
-qmp = true;
-} else {
-error_setg(errp, "unknown monitor mode \"%s\"", mode);
-return -1;
-}
-
-if (!qmp && qemu_opt_get(opts, "pretty")) {
-warn_report("'pretty' is deprecated for HMP monitors, it has no effect 
"
-"and will be removed in future versions");
-}
-if (qemu_opt_get_bool(opts, "pretty", 0)) {
-pretty = true;
-}
-
-chardev = qemu_opt_get(opts, "chardev");
-if (!chardev) {
-error_report("chardev is required");
-exit(1);
-}
-chr = qemu_chr_find(chardev);
-if (chr == NULL) {
-error_setg(errp, "chardev \"%s\" not found", chardev);
-return -1;
-}
-
-if (qmp) {
-monitor_init_qmp(chr, pretty);
-} else {
-monitor_init_hmp(chr, true);
-}
-return 0;
+return 

[RFC PATCH 12/18] stubs: Update monitor stubs for qemu-storage-daemon

[Kevin Wolf]

Before we can add the monitor to qemu-storage-daemon, we need to add a
few monitor stubs, and we need to make sure that stubs that are actually
implemented in the monitor core aren't linked so that we don't get
linker errors because of duplicate symbols.

Signed-off-by: Kevin Wolf 
---
 stubs/monitor-core.c | 21 +
 stubs/monitor.c  | 15 ++-
 stubs/Makefile.objs  |  1 +
 3 files changed, 24 insertions(+), 13 deletions(-)
 create mode 100644 stubs/monitor-core.c

diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
new file mode 100644
index 00..403c00a6d0
--- /dev/null
+++ b/stubs/monitor-core.c
@@ -0,0 +1,21 @@
+#include "qemu/osdep.h"
+#include "monitor/monitor.h"
+#include "qemu-common.h"
+#include "qapi/qapi-emit-events.h"
+
+__thread Monitor *cur_mon;
+
+void monitor_init_qmp(Chardev *chr, bool pretty)
+{
+}
+
+void qapi_event_emit(QAPIEvent event, QDict *qdict)
+{
+}
+
+int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
+{
+abort();
+}
+
+
diff --git a/stubs/monitor.c b/stubs/monitor.c
index c3e9a2e4dc..9403f8e72c 100644
--- a/stubs/monitor.c
+++ b/stubs/monitor.c
@@ -1,14 +1,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "qapi/qapi-emit-events.h"
 #include "monitor/monitor.h"
-
-__thread Monitor *cur_mon;
-
-int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
-{
-abort();
-}
+#include "../monitor/monitor-internal.h"
 
 int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
 {
@@ -16,14 +9,10 @@ int monitor_get_fd(Monitor *mon, const char *name, Error 
**errp)
 return -1;
 }
 
-void monitor_init_qmp(Chardev *chr, bool pretty)
-{
-}
-
 void monitor_init_hmp(Chardev *chr, bool use_readline)
 {
 }
 
-void qapi_event_emit(QAPIEvent event, QDict *qdict)
+void monitor_fdsets_cleanup(void)
 {
 }
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 77fbf72576..ad4515ac70 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -19,6 +19,7 @@ stub-obj-y += machine-init-done.o
 stub-obj-y += migr-blocker.o
 stub-obj-y += change-state-handler.o
 stub-obj-y += monitor.o
+stub-obj-y += monitor-core.o
 stub-obj-y += notify-event.o
 stub-obj-y += qtest.o
 stub-obj-y += replay.o
-- 
2.20.1

[RFC PATCH 08/18] qemu-storage-daemon: Add --export option

[Kevin Wolf]

Add a --export option to qemu-storage-daemon to export a block node. For
now, only NBD exports are implemented. Apart from the 'type' option
(which is the implied key), it maps the arguments for nbd-server-add to
the command line. Example:

--export nbd,device=disk,name=test-export,writable=on

Signed-off-by: Kevin Wolf 
---
 qapi/block.json   | 27 +++
 qemu-storage-daemon.c | 31 +++
 2 files changed, 58 insertions(+)

diff --git a/qapi/block.json b/qapi/block.json
index 567f116875..d9b1f16fbf 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -481,3 +481,30 @@
 { 'event': 'QUORUM_REPORT_BAD',
   'data': { 'type': 'QuorumOpType', '*error': 'str', 'node-name': 'str',
 'sector-num': 'int', 'sectors-count': 'int' } }
+
+##
+# @BlockExportType:
+#
+# An enumeration of block export types
+#
+# @nbd: NBD export
+#
+# Since: 4.2
+##
+{ 'enum': 'BlockExportType',
+  'data': [ 'nbd' ] }
+
+##
+# @BlockExport:
+#
+# Describes a block export, i.e. how single node should be exported on an
+# external interface.
+#
+# Since: 4.2
+##
+{ 'union': 'BlockExport',
+  'base': { 'type': 'BlockExportType' },
+  'discriminator': 'type',
+  'data': {
+  'nbd': 'BlockExportNbd'
+   } }
diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index 51882452f3..9e5f474fd0 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -67,6 +67,11 @@ static void help(void)
 " [,driver specific parameters...]\n"
 " configure a block backend\n"
 "\n"
+"  --export [type=]nbd,device=[,name=]\n"
+"   [,writable=on|off][,bitmap=]\n"
+" export the specified block node over NBD\n"
+" (requires --nbd-server)\n"
+"\n"
 "  --nbd-server addr.type=inet,addr.host=,addr.port=\n"
 "   [,tls-creds=][,tls-authz=]\n"
 "  --nbd-server addr.type=unix,addr.path=\n"
@@ -84,6 +89,7 @@ enum {
 OPTION_OBJECT = 256,
 OPTION_BLOCKDEV,
 OPTION_NBD_SERVER,
+OPTION_EXPORT,
 };
 
 static QemuOptsList qemu_object_opts = {
@@ -95,6 +101,17 @@ static QemuOptsList qemu_object_opts = {
 },
 };
 
+static void init_export(BlockExport *export, Error **errp)
+{
+switch (export->type) {
+case BLOCK_EXPORT_TYPE_NBD:
+qmp_nbd_server_add(>u.nbd, errp);
+break;
+default:
+g_assert_not_reached();
+}
+}
+
 static int process_options(int argc, char *argv[], Error **errp)
 {
 int c;
@@ -106,6 +123,7 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 {"object", required_argument, 0, OPTION_OBJECT},
 {"blockdev", required_argument, 0, OPTION_BLOCKDEV},
 {"nbd-server", required_argument, 0, OPTION_NBD_SERVER},
+{"export", required_argument, 0, OPTION_EXPORT},
 {"version", no_argument, 0, 'V'},
 {"trace", required_argument, NULL, 'T'},
 {0, 0, 0, 0}
@@ -176,6 +194,19 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 qapi_free_NbdServerOptions(options);
 break;
 }
+case OPTION_EXPORT:
+{
+Visitor *v;
+BlockExport *export;
+
+v = qobject_input_visitor_new_str(optarg, "type", 
_fatal);
+visit_type_BlockExport(v, NULL, , _fatal);
+visit_free(v);
+
+init_export(export, _fatal);
+qapi_free_BlockExport(export);
+break;
+}
 }
 }
 if (optind != argc) {
-- 
2.20.1

[RFC PATCH 00/18] Add qemu-storage-daemon

[Kevin Wolf]

This series adds a new tool 'qemu-storage-daemon', which can be used to
export and perform operations on block devices. There is some overlap
between qemu-img/qemu-nbd and the new qemu-storage-daemon, but there are
a few important differences:

* The qemu-storage-daemon has QMP support. The command set is obviously
  restricted compared to the system emulator because there is no guest,
  but all of the block operations are present.

  This means that it can access advanced options or operations that the
  qemu-img command line doesn't expose. For example, blockdev-create is
  a lot more powerful than 'qemu-img create', and qemu-storage-daemon
  allows to execute it without starting a guest.

  Compared to qemu-nbd it means that, for example, block jobs can now be
  executed on the server side, and backing chains shared by multiple VMs
  can be modified this way.

* The existing tools all have a separately invented one-off syntax for
  the job at hand, which usually comes with restrictions compared to the
  system emulator. qemu-storage-daemon shares the same syntax with the
  system emulator for most options and prefers QAPI based interfaces
  where possible (such as --blockdev), so it should be easy to make use
  of in libvirt.

* While this series implements only NBD exports, the storage daemon is
  intended to serve multiple protocols and its syntax reflects this. In
  the past, we had proposals to add new one-off tools for exporting over
  new protocols like FUSE or TCMU.

  With a generic storage daemon, additional export methods have a home
  without adding a new tool for each of them.

I'm posting this as an RFC mainly for two reasons:

1. The monitor integration, which could be argued to be a little hackish
   (some generated QAPI source files are built from a separate QAPI
   schema, but the per-module ones are taken from the system emulator)
   and Markus will want to have a closer look there. But from the IRC
   discussions we had, we seem to agree on the general approach here.

2. I'm not completely sure if the command line syntax is the final
   version that we want to support long-term. Many options directly use
   QAPI visitors (--blockdev, --export, --nbd-server) and should be
   fine. However, others use QemuOpts (--chardev, --monitor, --object).

   This is the same as in the system emulator, so we wouldn't be adding
   a new problem, but as there was talk about QAPIfying the command
   line, and I wouldn't want later syntax changes or adding lots of
   compatibility code to a new tool, I thought we should probably
   discuss whether QAPIfying from the start would be an option.

I would like to have something merged for 4.2, but I'm considering
marking the whole tool as experimental and unstable ABI by requiring a
command line option like --experimental for the tool to even start if we
know that we want to change some things later.

This series is based on:
* [PATCH] blockdev: Use error_report() in hmp_commit()
* [PATCH 0/7] qapi: Cleanups and test speedup

Based-on: <20191015123932.12214-1-kw...@redhat.com>
Based-on: <20191001191514.11208-1-arm...@redhat.com>

Kevin Wolf (18):
  qemu-storage-daemon: Add barebone tool
  qemu-storage-daemon: Add --object option
  stubs: Add arch_type
  stubs: Add blk_by_qdev_id()
  qemu-storage-daemon: Add --blockdev option
  qemu-storage-daemon: Add --nbd-server option
  blockdev-nbd: Boxed argument type for nbd-server-add
  qemu-storage-daemon: Add --export option
  qemu-storage-daemon: Add main loop
  qemu-storage-daemon: Add --chardev option
  monitor: Move monitor option parsing to monitor/monitor.c
  stubs: Update monitor stubs for qemu-storage-daemon
  qapi: Create module 'monitor'
  monitor: Create monitor/qmp-cmds-monitor.c
  qapi: Support empty modules
  qapi: Create 'pragma' module
  monitor: Move qmp_query_qmp_schema to qmp-cmds-monitor.c
  qemu-storage-daemon: Add --monitor option

 qapi/block.json  |  65 -
 qapi/misc.json   | 212 ---
 qapi/monitor.json| 218 +++
 qapi/pragma.json |  24 ++
 qapi/qapi-schema.json|  26 +-
 storage-daemon/qapi/qapi-schema.json |  15 ++
 configure|   2 +-
 include/block/nbd.h  |   1 +
 include/monitor/monitor.h|   4 +
 include/sysemu/arch_init.h   |   2 +
 include/sysemu/sysemu.h  |   1 -
 monitor/monitor-internal.h   |   4 +
 blockdev-nbd.c   |  30 ++-
 monitor/hmp-cmds.c   |  22 +-
 monitor/misc.c   | 126 -
 monitor/monitor.c|  52 
 monitor/qmp-cmds-monitor.c   | 169 
 monitor/qmp-cmds.c   |  15 +-
 monitor/qmp.c|   2 +-
 qemu-storage-daemon.c| 326 

[RFC PATCH 05/18] qemu-storage-daemon: Add --blockdev option

[Kevin Wolf]

This adds a --blockdev option to the storage daemon that works the same
as the -blockdev option of the system emulator.

In order to be able to link with blockdev.o, we also need to change
stream.o from common-obj to block-obj, which is where all other block
jobs already are.

Signed-off-by: Kevin Wolf 
---
 qemu-storage-daemon.c | 29 +
 Makefile  |  5 -
 Makefile.objs |  7 +++
 block/Makefile.objs   |  2 +-
 4 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index 48d6af43a6..904e3c3a46 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -28,6 +28,10 @@
 #include "crypto/init.h"
 
 #include "qapi/error.h"
+#include "qapi/qapi-visit-block-core.h"
+#include "qapi/qapi-commands-block-core.h"
+#include "qapi/qobject-input-visitor.h"
+
 #include "qemu-common.h"
 #include "qemu-version.h"
 #include "qemu/config-file.h"
@@ -53,6 +57,13 @@ static void help(void)
 " specify tracing options\n"
 "  -V, --version  output version information and exit\n"
 "\n"
+"  --blockdev [driver=][,node-name=][,discard=ignore|unmap]\n"
+" [,cache.direct=on|off][,cache.no-flush=on|off]\n"
+" [,read-only=on|off][,auto-read-only=on|off]\n"
+" [,force-share=on|off][,detect-zeroes=on|off|unmap]\n"
+" [,driver specific parameters...]\n"
+" configure a block backend\n"
+"\n"
 "  --object   define a QOM object such as 'secret' for\n"
 " passwords and/or encryption keys\n"
 "\n"
@@ -62,6 +73,7 @@ QEMU_HELP_BOTTOM "\n",
 
 enum {
 OPTION_OBJECT = 256,
+OPTION_BLOCKDEV,
 };
 
 static QemuOptsList qemu_object_opts = {
@@ -82,6 +94,7 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 static const struct option long_options[] = {
 {"help", no_argument, 0, 'h'},
 {"object", required_argument, 0, OPTION_OBJECT},
+{"blockdev", required_argument, 0, OPTION_BLOCKDEV},
 {"version", no_argument, 0, 'V'},
 {"trace", required_argument, NULL, 'T'},
 {0, 0, 0, 0}
@@ -123,6 +136,22 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 qemu_opts_del(opts);
 break;
 }
+break;
+case OPTION_BLOCKDEV:
+{
+Visitor *v;
+BlockdevOptions *options;
+
+v = qobject_input_visitor_new_str(optarg, "driver",
+  _fatal);
+
+visit_type_BlockdevOptions(v, NULL, , _fatal);
+visit_free(v);
+
+qmp_blockdev_add(options, _fatal);
+qapi_free_BlockdevOptions(options);
+break;
+}
 }
 }
 if (optind != argc) {
diff --git a/Makefile b/Makefile
index 76338d0ab4..b913d4d736 100644
--- a/Makefile
+++ b/Makefile
@@ -432,6 +432,8 @@ dummy := $(call unnest-vars,, \
 qga-vss-dll-obj-y \
 block-obj-y \
 block-obj-m \
+storage-daemon-obj-y \
+storage-daemon-obj-m \
 crypto-obj-y \
 crypto-user-obj-y \
 qom-obj-y \
@@ -469,6 +471,7 @@ TARGET_DIRS_RULES := $(foreach t, all clean install, 
$(addsuffix /$(t), $(TARGET
 SOFTMMU_ALL_RULES=$(filter %-softmmu/all, $(TARGET_DIRS_RULES))
 $(SOFTMMU_ALL_RULES): $(authz-obj-y)
 $(SOFTMMU_ALL_RULES): $(block-obj-y)
+$(SOFTMMU_ALL_RULES): $(storage-daemon-obj-y)
 $(SOFTMMU_ALL_RULES): $(chardev-obj-y)
 $(SOFTMMU_ALL_RULES): $(crypto-obj-y)
 $(SOFTMMU_ALL_RULES): $(io-obj-y)
@@ -558,7 +561,7 @@ qemu-img.o: qemu-img-cmds.h
 qemu-img$(EXESUF): qemu-img.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) 
$(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 qemu-nbd$(EXESUF): qemu-nbd.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) 
$(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 qemu-io$(EXESUF): qemu-io.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) 
$(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
-qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) 
$(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
+qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) 
$(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(storage-daemon-obj-y) 
$(COMMON_LDADDS)
 
 qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o $(COMMON_LDADDS)
 
diff --git a/Makefile.objs b/Makefile.objs
index abcbd89654..00fdf54500 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -37,6 +37,13 @@ qom-obj-y = qom/
 
 io-obj-y = io/
 
+###
+# storage-daemon-obj-y is code used by qemu-storage-daemon (these objects are
+# used for system emulation, too, but specified separately there)
+
+storage-daemon-obj-y = block/
+storage-daemon-obj-y += blockdev.o iothread.o
+
 

[RFC PATCH 03/18] stubs: Add arch_type

[Kevin Wolf]

blockdev.c uses the arch_type constant, so before we can use the file in
tools (i.e. outside of the system emulator), we need to add a stub for
it. A new QEMU_ARCH_NONE is introduced for this case.

Signed-off-by: Kevin Wolf 
---
 include/sysemu/arch_init.h | 2 ++
 stubs/arch_type.c  | 4 
 stubs/Makefile.objs| 1 +
 3 files changed, 7 insertions(+)
 create mode 100644 stubs/arch_type.c

diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index 62c6fe4cf1..01392dc945 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -24,6 +24,8 @@ enum {
 QEMU_ARCH_NIOS2 = (1 << 17),
 QEMU_ARCH_HPPA = (1 << 18),
 QEMU_ARCH_RISCV = (1 << 19),
+
+QEMU_ARCH_NONE = (1 << 31),
 };
 
 extern const uint32_t arch_type;
diff --git a/stubs/arch_type.c b/stubs/arch_type.c
new file mode 100644
index 00..fc5423bc98
--- /dev/null
+++ b/stubs/arch_type.c
@@ -0,0 +1,4 @@
+#include "qemu/osdep.h"
+#include "sysemu/arch_init.h"
+
+const uint32_t arch_type = QEMU_ARCH_NONE;
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 4a50e95ec3..9f4eb25e02 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,3 +1,4 @@
+stub-obj-y += arch_type.o
 stub-obj-y += bdrv-next-monitor-owned.o
 stub-obj-y += blk-commit-all.o
 stub-obj-y += blockdev-close-all-bdrv-states.o
-- 
2.20.1

[RFC PATCH 01/18] qemu-storage-daemon: Add barebone tool

[Kevin Wolf]

This adds a new binary qemu-storage-daemon that doesn't yet do more than
some typical initialisation for tools and parsing the basic command
options --version, --help and --trace.

Signed-off-by: Kevin Wolf 
---
 configure |   2 +-
 qemu-storage-daemon.c | 141 ++
 Makefile  |   1 +
 3 files changed, 143 insertions(+), 1 deletion(-)
 create mode 100644 qemu-storage-daemon.c

diff --git a/configure b/configure
index 08ca4bcb46..bb3d55fb25 100755
--- a/configure
+++ b/configure
@@ -6034,7 +6034,7 @@ tools=""
 if test "$want_tools" = "yes" ; then
   tools="qemu-img\$(EXESUF) qemu-io\$(EXESUF) qemu-edid\$(EXESUF) $tools"
   if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
-tools="qemu-nbd\$(EXESUF) $tools"
+tools="qemu-nbd\$(EXESUF) qemu-storage-daemon\$(EXESUF) $tools"
   fi
   if [ "$ivshmem" = "yes" ]; then
 tools="ivshmem-client\$(EXESUF) ivshmem-server\$(EXESUF) $tools"
diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
new file mode 100644
index 00..a251dc255c
--- /dev/null
+++ b/qemu-storage-daemon.c
@@ -0,0 +1,141 @@
+/*
+ * QEMU storage daemon
+ *
+ * Copyright (c) 2019 Kevin Wolf 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+
+#include "block/block.h"
+#include "crypto/init.h"
+
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "qemu-version.h"
+#include "qemu/config-file.h"
+#include "qemu/error-report.h"
+#include "qemu/log.h"
+#include "qemu/main-loop.h"
+#include "qemu/module.h"
+
+#include "trace/control.h"
+
+#include 
+
+static void help(void)
+{
+printf(
+"Usage: %s [options]\n"
+"QEMU storage daemon\n"
+"\n"
+"  -h, --help display this help and exit\n"
+"  -T, --trace [[enable=]][,events=][,file=]\n"
+" specify tracing options\n"
+"  -V, --version  output version information and exit\n"
+"\n"
+QEMU_HELP_BOTTOM "\n",
+error_get_progname());
+}
+
+static int process_options(int argc, char *argv[], Error **errp)
+{
+int c;
+char *trace_file = NULL;
+int ret = -EINVAL;
+
+static const struct option long_options[] = {
+{"help", no_argument, 0, 'h'},
+{"version", no_argument, 0, 'V'},
+{"trace", required_argument, NULL, 'T'},
+{0, 0, 0, 0}
+};
+
+while ((c = getopt_long(argc, argv, ":hT:V", long_options, NULL)) != -1) {
+switch (c) {
+case '?':
+error_setg(errp, "Unknown option '%s'", argv[optind - 1]);
+goto out;
+case ':':
+error_setg(errp, "Missing option argument for '%s'",
+   argv[optind - 1]);
+goto out;
+case 'h':
+help();
+exit(EXIT_SUCCESS);
+case 'V':
+printf("qemu-storage-daemon version "
+   QEMU_FULL_VERSION "\n" QEMU_COPYRIGHT "\n");
+exit(EXIT_SUCCESS);
+case 'T':
+g_free(trace_file);
+trace_file = trace_opt_parse(optarg);
+break;
+}
+}
+if (optind != argc) {
+error_setg(errp, "Unexpected argument: %s", argv[optind]);
+goto out;
+}
+
+if (!trace_init_backends()) {
+error_setg(errp, "Could not initialize trace backends");
+goto out;
+}
+trace_init_file(trace_file);
+qemu_set_log(LOG_TRACE);
+
+ret = 0;
+out:
+g_free(trace_file);
+return ret;
+}
+
+int main(int argc, char *argv[])
+{
+Error *local_err = NULL;
+int ret;
+
+#ifdef CONFIG_POSIX
+signal(SIGPIPE, SIG_IGN);
+#endif
+
+error_init(argv[0]);
+qemu_init_exec_dir(argv[0]);
+
+module_call_init(MODULE_INIT_QOM);
+module_call_init(MODULE_INIT_TRACE);
+qemu_add_opts(_trace_opts);
+qcrypto_init(_fatal);
+bdrv_init();
+
+if (qemu_init_main_loop(_err)) {
+error_report_err(local_err);
+return 

[RFC PATCH 02/18] qemu-storage-daemon: Add --object option

[Kevin Wolf]

Add a command line option to create user-creatable QOM objects.

Signed-off-by: Kevin Wolf 
---
 qemu-storage-daemon.c | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index a251dc255c..48d6af43a6 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -35,6 +35,8 @@
 #include "qemu/log.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
+#include "qemu/option.h"
+#include "qom/object_interfaces.h"
 
 #include "trace/control.h"
 
@@ -51,10 +53,26 @@ static void help(void)
 " specify tracing options\n"
 "  -V, --version  output version information and exit\n"
 "\n"
+"  --object   define a QOM object such as 'secret' for\n"
+" passwords and/or encryption keys\n"
+"\n"
 QEMU_HELP_BOTTOM "\n",
 error_get_progname());
 }
 
+enum {
+OPTION_OBJECT = 256,
+};
+
+static QemuOptsList qemu_object_opts = {
+.name = "object",
+.implied_opt_name = "qom-type",
+.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+.desc = {
+{ }
+},
+};
+
 static int process_options(int argc, char *argv[], Error **errp)
 {
 int c;
@@ -63,6 +81,7 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 
 static const struct option long_options[] = {
 {"help", no_argument, 0, 'h'},
+{"object", required_argument, 0, OPTION_OBJECT},
 {"version", no_argument, 0, 'V'},
 {"trace", required_argument, NULL, 'T'},
 {0, 0, 0, 0}
@@ -88,6 +107,22 @@ static int process_options(int argc, char *argv[], Error 
**errp)
 g_free(trace_file);
 trace_file = trace_opt_parse(optarg);
 break;
+case OPTION_OBJECT:
+{
+QemuOpts *opts;
+const char *type;
+
+opts = qemu_opts_parse(_object_opts,
+   optarg, true, _fatal);
+type = qemu_opt_get(opts, "qom-type");
+
+if (user_creatable_print_help(type, opts)) {
+exit(EXIT_SUCCESS);
+}
+user_creatable_add_opts(opts, _fatal);
+qemu_opts_del(opts);
+break;
+}
 }
 }
 if (optind != argc) {
-- 
2.20.1

[RFC PATCH 04/18] stubs: Add blk_by_qdev_id()

[Kevin Wolf]

blockdev.c uses the blk_by_qdev_id() function, so before we can use the
file in tools (i.e. outside of the system emulator), we need to add a
stub for it. The function always returns an error.

Signed-off-by: Kevin Wolf 
---
 stubs/blk-by-qdev-id.c | 9 +
 stubs/Makefile.objs| 1 +
 2 files changed, 10 insertions(+)
 create mode 100644 stubs/blk-by-qdev-id.c

diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c
new file mode 100644
index 00..0b6160fefa
--- /dev/null
+++ b/stubs/blk-by-qdev-id.c
@@ -0,0 +1,9 @@
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "sysemu/block-backend.h"
+
+BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
+{
+error_setg(errp, "qdev IDs/QOM paths exist only in the system emulator");
+return NULL;
+}
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 9f4eb25e02..77fbf72576 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,5 +1,6 @@
 stub-obj-y += arch_type.o
 stub-obj-y += bdrv-next-monitor-owned.o
+stub-obj-y += blk-by-qdev-id.o
 stub-obj-y += blk-commit-all.o
 stub-obj-y += blockdev-close-all-bdrv-states.o
 stub-obj-y += clock-warp.o
-- 
2.20.1

Re: [PATCH v2 2/3] iotests: Include QMP input in .out files

[Max Reitz]

On 15.10.19 21:35, Eric Blake wrote:
> We generally include relevant HMP input in .out files, by virtue of
> the fact that HMP echoes its input.  But QMP does not, so we have to
> explicitly inject it in the output stream, in order to make it easier
> to read .out files to see what behavior is being tested (especially
> true where the output file is a sequence of {'return': {}}).
> 
> Suggested-by: Max Reitz 

That was actually not my intention. :-)

I was thinking of a new parameter that enables this behavior and is
disabled by default so that existing tests don’t change.

But then again I did see that you interpreted my suggestion in a
slightly different way, and thought this is probably better, actually.

> Signed-off-by: Eric Blake 
> ---
>  tests/qemu-iotests/common.qemu |  9 
>  tests/qemu-iotests/085.out | 26 ++
>  tests/qemu-iotests/094.out |  4 ++
>  tests/qemu-iotests/095.out |  2 +
>  tests/qemu-iotests/109.out | 88 ++
>  tests/qemu-iotests/117.out |  5 ++
>  tests/qemu-iotests/127.out |  4 ++
>  tests/qemu-iotests/140.out |  5 ++
>  tests/qemu-iotests/141.out | 26 ++
>  tests/qemu-iotests/143.out |  3 ++
>  tests/qemu-iotests/144.out |  5 ++
>  tests/qemu-iotests/153.out | 11 +
>  tests/qemu-iotests/156.out | 11 +
>  tests/qemu-iotests/161.out |  8 
>  tests/qemu-iotests/173.out |  4 ++
>  tests/qemu-iotests/182.out |  8 
>  tests/qemu-iotests/183.out | 11 +
>  tests/qemu-iotests/185.out | 18 +++
>  tests/qemu-iotests/191.out |  8 
>  tests/qemu-iotests/200.out |  1 +
>  tests/qemu-iotests/223.out | 19 
>  tests/qemu-iotests/229.out |  3 ++
>  tests/qemu-iotests/249.out |  6 +++
>  23 files changed, 285 insertions(+)
> diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
> index 8d2021a7eb0c..abc231743e82 100644
> --- a/tests/qemu-iotests/common.qemu
> +++ b/tests/qemu-iotests/common.qemu
> @@ -123,6 +123,9 @@ _timed_wait_for()
>  # until either timeout, or a response.  If it is not set, or <=0,
>  # then the command is only sent once.
>  #
> +# If neither $silent nor $mismatch_only is set, and $cmd begins with '{',
> +# echo the command before sending it the first time.
> +#
>  # If $qemu_error_no_exit is set, then even if the expected response
>  # is not seen, we will not exit.  $QEMU_STATUS[$1] will be set it -1 in
>  # that case.
> @@ -152,6 +155,12 @@ _send_qemu_cmd()
>  shift $(($# - 2))
>  fi
> 
> +# Display QMP being sent, but not HMP (since HMP already echoes its
> +# input back to output); decide based on leading '{'
> +if [ -z "$silent" ] && [ -z "$mismatch_only" ] &&
> +[ "$cmd" != "${cmd#{}" ]; then

It’s a shame that this breaks syntax highlighting in (my) vim.  (Also I
have to admit googling to understand ${cmd#{} wasn’t trivial.)

Can I persuade you to use "${cmd#\{}" instead?  That seems to work for me.

> diff --git a/tests/qemu-iotests/094.out b/tests/qemu-iotests/094.out
> index f3b9ecf22b73..f3e1a9ecf736 100644
> --- a/tests/qemu-iotests/094.out
> +++ b/tests/qemu-iotests/094.out
> @@ -1,16 +1,20 @@
>  QA output created by 094
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  Formatting 'TEST_DIR/source.IMGFMT', fmt=IMGFMT size=67108864
> +{'execute': 'qmp_capabilities'}
>  {"return": {}}
> +{'execute': 'drive-mirror', 'arguments': {'device': 'src', 'target': 
> 'nbd:127.0.0.1:10810', 'format': 'nbd', 'sync':'full', 'mode':'existing'}}

This reminds me that we need to fix nbd’s $TEST_IMG to not be fixed to
port 10810.  I get intermittent failures because of that.

[...]

> diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
> index 67fe44a3e390..3857675f7ebd 100644
> --- a/tests/qemu-iotests/140.out
> +++ b/tests/qemu-iotests/140.out
> @@ -2,14 +2,19 @@ QA output created by 140
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
>  wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +{ 'execute': 'qmp_capabilities' }
>  {"return": {}}
> +{ 'execute': 'nbd-server-start', 'arguments': { 'addr': { 'type': 'unix', 
> 'data': { 'path': 'TEST_DIR/nbd' 

Hm, this conflicts with my SOCK_DIR series.  common.qemu would then
also need a SOCK_DIR filter.  Well, or 140 should filter it (and the
other tests that are concerned).  I’m not 100 % sure, but a SOCK_DIR
filter in common.qemu probably can’t hurt.

>  {"return": {}}
> +{ 'execute': 'nbd-server-add', 'arguments': { 'device': 'drv' }}
>  {"return": {}}
>  read 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +{ 'execute': 'eject', 'arguments': { 'device': 'drv' }}
>  {"return": {}}
>  qemu-io: can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Requested 
> export not available
>  server reported: export 'drv' not present
> +{ 'execute': 'quit' }
>  {"return": {}}
>  

Re: [PATCH v2 1/3] iotests: Fix 173

[Max Reitz]

On 17.10.19 13:46, Max Reitz wrote:
> On 15.10.19 21:35, Eric Blake wrote:
>> This test has been broken since 3.0.  It used TEST_IMG to influence
>> the name of a file created during _make_test_img, but commit 655ae6bb
>> changed things so that the wrong file name is being created, which
>> then caused _launch_qemu to fail.  In the meantime, the set of events
>> issued for the actions of the test has increased.
>>
>> Why haven't we noticed the failure? Because the test rarely gets run:
>> './check -qcow2 173' is insufficient (that defaults to using file protocol)
>> './check -nfs 173' is insufficient (that defaults to using raw format)
>> so the test is only run with:
>> ./check -qcow2 -nfs 173
>>
>> Note that we already have a number of other problems with -nfs:
>> ./check -nfs (fails 18/30)
>> ./check -qcow2 -nfs (fails 45/76 after this patch)
>> and it's not on my priority list to fix those.  Rather, I found this
>> because of my next patch's work on tests using _send_qemu_cmd.
>>
>> Fixes: 655ae6b
>> Signed-off-by: Eric Blake 
>> ---
>>  tests/qemu-iotests/173 | 4 ++--
>>  tests/qemu-iotests/173.out | 6 +-
>>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> On second thought, I wonder whether this test actually does anything
> with NFS.  It doesn’t look like it to me.
> 
> I wonder because for some reason I can’t get NFS to work with qemu at
> all.  I don’t think the iotests are at fault why so many tests fail,
> actually.

OK, I was just missing an “insecure” in my exports.  I hate debugging NFS.

Now I’m down to 16/76 for qcow2, and most of those look benign.  (As in,
they simply don’t support nfs.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PULL 33/36] block/backup: use backup-top instead of write notifiers

[Peter Maydell]

On Thu, 10 Oct 2019 at 12:44, Max Reitz  wrote:
>
> From: Vladimir Sementsov-Ogievskiy 
>
> Drop write notifiers and use filter node instead.

Hi; after this change Coverity complains about dead code in
backup_job_create() (CID 1406402):

> @@ -382,6 +353,8 @@ BlockJob *backup_job_create(const char *job_id, 
> BlockDriverState *bs,
>  BackupBlockJob *job = NULL;
>  int64_t cluster_size;
>  BdrvRequestFlags write_flags;
> +BlockDriverState *backup_top = NULL;
> +BlockCopyState *bcs = NULL;
>
>  assert(bs);
>  assert(target);
> @@ -463,33 +436,35 @@ BlockJob *backup_job_create(const char *job_id, 
> BlockDriverState *bs,
>  write_flags = (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING : 
> 0) |
>(compress ? BDRV_REQ_WRITE_COMPRESSED : 0),
>
> +backup_top = bdrv_backup_top_append(bs, target, filter_node_name,
> +cluster_size, write_flags, , 
> errp);
> +if (!backup_top) {
> +goto error;
> +}
> +
>  /* job->len is fixed, so we can't allow resize */
> -job = block_job_create(job_id, _job_driver, txn, bs, 0, 
> BLK_PERM_ALL,
> +job = block_job_create(job_id, _job_driver, txn, backup_top,
> +   0, BLK_PERM_ALL,
> speed, creation_flags, cb, opaque, errp);
>  if (!job) {
>  goto error;
>  }
>
> +job->backup_top = backup_top;
>  job->source_bs = bs;
>  job->on_source_error = on_source_error;
>  job->on_target_error = on_target_error;
>  job->sync_mode = sync_mode;
>  job->sync_bitmap = sync_bitmap;
>  job->bitmap_mode = bitmap_mode;
> -
> -job->bcs = block_copy_state_new(bs, target, cluster_size, write_flags,
> -errp);
> -if (!job->bcs) {
> -goto error;
> -}
> -

This code deletion has removed the only code path that can
reach the 'error' label after the successful creation of the job...

> +job->bcs = bcs;
>  job->cluster_size = cluster_size;
>  job->len = len;
>
> -block_copy_set_callbacks(job->bcs, backup_progress_bytes_callback,
> +block_copy_set_callbacks(bcs, backup_progress_bytes_callback,
>   backup_progress_reset_callback, job);
>
> -/* Required permissions are already taken by block-copy-state target */
> +/* Required permissions are already taken by backup-top target */
>  block_job_add_bdrv(>common, "target", target, 0, BLK_PERM_ALL,
> _abort);
>
> @@ -502,6 +477,8 @@ BlockJob *backup_job_create(const char *job_id, 
> BlockDriverState *bs,
>  if (job) {
>  backup_clean(>common.job);
>  job_early_fail(>common.job);

...so down here in the 'error:' code the "if (job)" condition
can never pass, and the code in this part of the if statement
is dead.

> +} else if (backup_top) {
> +bdrv_backup_top_drop(backup_top);
>  }
>
>  return NULL;
>  {"return": {}}

thanks
-- PMM

Re: [PATCH v2 1/3] iotests: Fix 173

[Max Reitz]

On 15.10.19 21:35, Eric Blake wrote:
> This test has been broken since 3.0.  It used TEST_IMG to influence
> the name of a file created during _make_test_img, but commit 655ae6bb
> changed things so that the wrong file name is being created, which
> then caused _launch_qemu to fail.  In the meantime, the set of events
> issued for the actions of the test has increased.
> 
> Why haven't we noticed the failure? Because the test rarely gets run:
> './check -qcow2 173' is insufficient (that defaults to using file protocol)
> './check -nfs 173' is insufficient (that defaults to using raw format)
> so the test is only run with:
> ./check -qcow2 -nfs 173
> 
> Note that we already have a number of other problems with -nfs:
> ./check -nfs (fails 18/30)
> ./check -qcow2 -nfs (fails 45/76 after this patch)
> and it's not on my priority list to fix those.  Rather, I found this
> because of my next patch's work on tests using _send_qemu_cmd.
> 
> Fixes: 655ae6b
> Signed-off-by: Eric Blake 
> ---
>  tests/qemu-iotests/173 | 4 ++--
>  tests/qemu-iotests/173.out | 6 +-
>  2 files changed, 7 insertions(+), 3 deletions(-)

On second thought, I wonder whether this test actually does anything
with NFS.  It doesn’t look like it to me.

I wonder because for some reason I can’t get NFS to work with qemu at
all.  I don’t think the iotests are at fault why so many tests fail,
actually.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 1/3] iotests: Fix 173

[Max Reitz]

On 15.10.19 21:35, Eric Blake wrote:
> This test has been broken since 3.0.  It used TEST_IMG to influence
> the name of a file created during _make_test_img, but commit 655ae6bb
> changed things so that the wrong file name is being created, which
> then caused _launch_qemu to fail.  In the meantime, the set of events
> issued for the actions of the test has increased.
> 
> Why haven't we noticed the failure? Because the test rarely gets run:
> './check -qcow2 173' is insufficient (that defaults to using file protocol)
> './check -nfs 173' is insufficient (that defaults to using raw format)
> so the test is only run with:
> ./check -qcow2 -nfs 173
> 
> Note that we already have a number of other problems with -nfs:
> ./check -nfs (fails 18/30)
> ./check -qcow2 -nfs (fails 45/76 after this patch)
> and it's not on my priority list to fix those.  Rather, I found this
> because of my next patch's work on tests using _send_qemu_cmd.
> 
> Fixes: 655ae6b
> Signed-off-by: Eric Blake 
> ---
>  tests/qemu-iotests/173 | 4 ++--
>  tests/qemu-iotests/173.out | 6 +-
>  2 files changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PULL v2 00/19] Bitmaps patches

[Peter Maydell]

On Mon, 14 Oct 2019 at 20:29, John Snow  wrote:
>
> The following changes since commit c760cb77e511eb05094df67c1b30029a952efa35:
>
>   Merge remote-tracking branch 
> 'remotes/dgilbert/tags/pull-migration-20191011a' into staging (2019-10-14 
> 16:09:52 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>
> for you to fetch changes up to b2ca29ee390743c42a6062d44ee3b10fb51f9fa6:
>
>   dirty-bitmaps: remove deprecated autoload parameter (2019-10-14 15:28:17 
> -0400)
>
> 
> Pull request
>
> 

Hi; this pullreq fails on some hosts on the newly added iotest 260
with an "AF_UNIX path too long" error:
https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg04063.html

Max tells me that this is a known problem (a fix is in the works)
but that for the moment we've chosen not to add any python based
tests to the 'auto' group, so that this AF_UNIX issue doesn't
affect "make check". Could you respin the pullreq with the
new iotest(s?) not in the 'auto' group, please?

thanks
-- PMM

Re: iotest 260 failure (linux host): "OSError: AF_UNIX path too long"

[Peter Maydell]

On Thu, 17 Oct 2019 at 11:20, Max Reitz  wrote:
>
> On 17.10.19 12:04, Peter Maydell wrote:
> > On Thu, 17 Oct 2019 at 10:53, Max Reitz  wrote:
> >>
> >> On 17.10.19 11:51, Peter Maydell wrote:
> >>> I just got this iotest 260 failure processing an
> >>> unrelated merge on my x86-64 Ubuntu box. I assume
> >>> it's an intermittent (have just kicked off a retry) but
> >>> post the backtrace in case it's of interest:
> >>
> >> I hope it’s intermittent, although I presume it might not be.
> >
> > It has indeed failed again. Is there some way to get the
> > build to pick a shorter test directory name, or to get
> > 'make check' to skip the iotests, so I can proceed with
> > testing until that fix lands?
>
> Yes, you can set the $TEST_DIR environment variable to a shorter scratch
> path.

Thanks, that works. It turns out that it wasn't "an unrelated
merge", I was confused. It's a merge that adds iotest 260,
which is why I hadn't seen the failure before.

-- PMM

Re: iotest 260 failure (linux host): "OSError: AF_UNIX path too long"

[Max Reitz]

On 17.10.19 12:04, Peter Maydell wrote:
> On Thu, 17 Oct 2019 at 10:53, Max Reitz  wrote:
>>
>> On 17.10.19 11:51, Peter Maydell wrote:
>>> I just got this iotest 260 failure processing an
>>> unrelated merge on my x86-64 Ubuntu box. I assume
>>> it's an intermittent (have just kicked off a retry) but
>>> post the backtrace in case it's of interest:
>>
>> I hope it’s intermittent, although I presume it might not be.
> 
> It has indeed failed again. Is there some way to get the
> build to pick a shorter test directory name, or to get
> 'make check' to skip the iotests, so I can proceed with
> testing until that fix lands?

Yes, you can set the $TEST_DIR environment variable to a shorter scratch
path.

Max



signature.asc
Description: OpenPGP digital signature

Re: iotest 260 failure (linux host): "OSError: AF_UNIX path too long"

[Peter Maydell]

On Thu, 17 Oct 2019 at 10:53, Max Reitz  wrote:
>
> On 17.10.19 11:51, Peter Maydell wrote:
> > I just got this iotest 260 failure processing an
> > unrelated merge on my x86-64 Ubuntu box. I assume
> > it's an intermittent (have just kicked off a retry) but
> > post the backtrace in case it's of interest:
>
> I hope it’s intermittent, although I presume it might not be.

It has indeed failed again. Is there some way to get the
build to pick a shorter test directory name, or to get
'make check' to skip the iotests, so I can proceed with
testing until that fix lands?

thanks
-- PMM

Re: [PATCH 02/10] image-fuzzer: Write bytes instead of string to image file

[Philippe Mathieu-Daudé]

On 10/16/19 9:24 PM, Eduardo Habkost wrote:

This is necessary for Python 3 compatibility.

Signed-off-by: Eduardo Habkost 
---
  tests/image-fuzzer/qcow2/layout.py | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index c57418fa15..fe273d4143 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -518,7 +518,7 @@ class Image(object):
  rounded = (size + self.cluster_size - 1) & ~(self.cluster_size - 1)
  if rounded > size:
  image_file.seek(rounded - 1)
-image_file.write("\0")
+image_file.write(b'\x00')
  image_file.close()
  
  @staticmethod




Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 07/10] image-fuzzer: Use bytes constant for field values

[Philippe Mathieu-Daudé]

On 10/16/19 9:24 PM, Eduardo Habkost wrote:

Field values are supposed to be bytes objects, not unicode
strings.  Change two constants that were declared as strings.

Signed-off-by: Eduardo Habkost 
---
  tests/image-fuzzer/qcow2/layout.py | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index 0adcbd448d..a0fd53c7ad 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -122,7 +122,7 @@ class Image(object):
  def create_header(self, cluster_bits, backing_file_name=None):
  """Generate a random valid header."""
  meta_header = [
-['>4s', 0, "QFI\xfb", 'magic'],
+['>4s', 0, b"QFI\xfb", 'magic'],
  ['>I', 4, random.randint(2, 3), 'version'],
  ['>Q', 8, 0, 'backing_file_offset'],
  ['>I', 16, 0, 'backing_file_size'],
@@ -231,7 +231,7 @@ class Image(object):
  feature_tables = []
  feature_ids = []
  inner_offset = self.ext_offset + ext_header_len
-feat_name = 'some cool feature'
+feat_name = b'some cool feature'
  while len(feature_tables) < num_fnt_entries * 3:
  feat_type, feat_bit = gen_feat_ids()
  # Remove duplicates



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 01/10] image-fuzzer: Open image files in binary mode

[Philippe Mathieu-Daudé]

On 10/16/19 9:24 PM, Eduardo Habkost wrote:

This probably never caused problems because on Linux there's no
actual newline conversion happening, but on Python 3 the
binary/text distinction is stronger and we must explicitly open
the image file in binary mode.

Signed-off-by: Eduardo Habkost 
---
  tests/image-fuzzer/qcow2/layout.py | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index 675877da96..c57418fa15 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -503,7 +503,7 @@ class Image(object):
  
  def write(self, filename):

  """Write an entire image to the file."""
-image_file = open(filename, 'w')
+image_file = open(filename, 'wb')
  for field in self:
  image_file.seek(field.offset)
  image_file.write(struct.pack(field.fmt, field.value))



Reviewed-by: Philippe Mathieu-Daudé