QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete
@ 2019-11-08 12:34 Max Reitz
  2019-11-08 12:34 ` [PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm() Max Reitz
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Max Reitz @ 2019-11-08 12:34 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz

Hi,

v3 of this series was this:

https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg00868.html

In the meantime, I’ve merged the first patch, so the subject of the
series has changed.

In v4, I’ve tried to address Vladimir’s concern of how to map QAPI
BlockPermission values to BLK_PERM_* flags with the new patches 1 and 2,
and a minor change to patch 3:
- Patch 1: Added, this new function will translate QAPI BlockPermission
           values to BLK_PERM_* flags
- Patch 2: Added; we can make use of the new function in
           xdbg_graph_add_edge() to save some LoC
- Patch 3:
  - %s/4.2/5.0/
  - Let blkdebug_parse_perm_list() initialize *dest
  - Use bdrv_qapi_perm_to_blk_perm() to translate QAPI BlockPermission
    values instead of blindly assuming an x -> 2^x mapping


git-backport-diff against v3:

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/5:[down] 'block: Add bdrv_qapi_perm_to_blk_perm()'
002/5:[down] 'block: Use bdrv_qapi_perm_to_blk_perm()'
003/5:[0008] [FC] 'blkdebug: Allow taking/unsharing permissions'
004/5:[----] [--] 'iotests: Add @error to wait_until_completed'
005/5:[----] [--] 'iotests: Add test for failing mirror complete'


Max Reitz (5):
  block: Add bdrv_qapi_perm_to_blk_perm()
  block: Use bdrv_qapi_perm_to_blk_perm()
  blkdebug: Allow taking/unsharing permissions
  iotests: Add @error to wait_until_completed
  iotests: Add test for failing mirror complete

 block.c                       | 47 ++++++++++--------
 block/blkdebug.c              | 93 ++++++++++++++++++++++++++++++++++-
 include/block/block.h         |  1 +
 qapi/block-core.json          | 14 +++++-
 tests/qemu-iotests/041        | 44 +++++++++++++++++
 tests/qemu-iotests/041.out    |  4 +-
 tests/qemu-iotests/iotests.py | 18 ++++---
 7 files changed, 190 insertions(+), 31 deletions(-)

-- 
2.23.0



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm()
  2019-11-08 12:34 [PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete Max Reitz
@ 2019-11-08 12:34 ` Max Reitz
  2019-11-08 14:05   ` Vladimir Sementsov-Ogievskiy
  2019-11-08 12:34 ` [PATCH for-5.0 v4 2/5] block: Use bdrv_qapi_perm_to_blk_perm() Max Reitz
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2019-11-08 12:34 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz

We need some way to correlate QAPI BlockPermission values with
BLK_PERM_* flags.  We could:

(1) have the same order in the QAPI definition as the the BLK_PERM_*
    flags are in LSb-first order.  However, then there is no guarantee
    that they actually match (e.g. when someone modifies the QAPI schema
    without thinking of the BLK_PERM_* definitions).
    We could add static assertions, but these would break what’s good
    about this solution, namely its simplicity.

(2) define the BLK_PERM_* flags based on the BlockPermission values.
    But this way whenever someone were to modify the QAPI order
    (perfectly sensible in theory), the BLK_PERM_* values would change.
    Because these values are used for file locking, this might break
    file locking between different qemu versions.

Therefore, go the slightly more cumbersome way: Add a function to
translate from the QAPI constants to the BLK_PERM_* flags.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 18 ++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/block.c b/block.c
index 4cffc2bc35..066433f3e2 100644
--- a/block.c
+++ b/block.c
@@ -2227,6 +2227,24 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
     *nshared = shared;
 }
 
+uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
+{
+    static const uint64_t permissions[] = {
+        [BLOCK_PERMISSION_CONSISTENT_READ]  = BLK_PERM_CONSISTENT_READ,
+        [BLOCK_PERMISSION_WRITE]            = BLK_PERM_WRITE,
+        [BLOCK_PERMISSION_WRITE_UNCHANGED]  = BLK_PERM_WRITE_UNCHANGED,
+        [BLOCK_PERMISSION_RESIZE]           = BLK_PERM_RESIZE,
+        [BLOCK_PERMISSION_GRAPH_MOD]        = BLK_PERM_GRAPH_MOD,
+    };
+
+    QEMU_BUILD_BUG_ON(ARRAY_SIZE(permissions) != BLOCK_PERMISSION__MAX);
+    QEMU_BUILD_BUG_ON(1UL << ARRAY_SIZE(permissions) != BLK_PERM_ALL + 1);
+
+    assert(qapi_perm < BLOCK_PERMISSION__MAX);
+
+    return permissions[qapi_perm];
+}
+
 static void bdrv_replace_child_noperm(BdrvChild *child,
                                       BlockDriverState *new_bs)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 1df9848e74..e9dcfef7fa 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -280,6 +280,7 @@ enum {
 };
 
 char *bdrv_perm_names(uint64_t perm);
+uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm);
 
 /* disk I/O throttling */
 void bdrv_init(void);
-- 
2.23.0



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH for-5.0 v4 2/5] block: Use bdrv_qapi_perm_to_blk_perm()
  2019-11-08 12:34 [PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete Max Reitz
  2019-11-08 12:34 ` [PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm() Max Reitz
@ 2019-11-08 12:34 ` Max Reitz
  2019-11-08 14:11   ` Vladimir Sementsov-Ogievskiy
  2019-11-08 12:34 ` [PATCH for-5.0 v4 3/5] blkdebug: Allow taking/unsharing permissions Max Reitz
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2019-11-08 12:34 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz

We can save some LoC in xdbg_graph_add_edge() by using
bdrv_qapi_perm_to_blk_perm().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index 066433f3e2..ae279ff21f 100644
--- a/block.c
+++ b/block.c
@@ -4870,36 +4870,23 @@ static void xdbg_graph_add_node(XDbgBlockGraphConstructor *gr, void *node,
 static void xdbg_graph_add_edge(XDbgBlockGraphConstructor *gr, void *parent,
                                 const BdrvChild *child)
 {
-    typedef struct {
-        unsigned int flag;
-        BlockPermission num;
-    } PermissionMap;
-
-    static const PermissionMap permissions[] = {
-        { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
-        { BLK_PERM_WRITE,           BLOCK_PERMISSION_WRITE },
-        { BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
-        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
-        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
-        { 0, 0 }
-    };
-    const PermissionMap *p;
+    BlockPermission qapi_perm;
     XDbgBlockGraphEdge *edge;
 
-    QEMU_BUILD_BUG_ON(1UL << (ARRAY_SIZE(permissions) - 1) != BLK_PERM_ALL + 1);
-
     edge = g_new0(XDbgBlockGraphEdge, 1);
 
     edge->parent = xdbg_graph_node_num(gr, parent);
     edge->child = xdbg_graph_node_num(gr, child->bs);
     edge->name = g_strdup(child->name);
 
-    for (p = permissions; p->flag; p++) {
-        if (p->flag & child->perm) {
-            QAPI_LIST_ADD(edge->perm, p->num);
+    for (qapi_perm = 0; qapi_perm < BLOCK_PERMISSION__MAX; qapi_perm++) {
+        uint64_t flag = bdrv_qapi_perm_to_blk_perm(qapi_perm);
+
+        if (flag & child->perm) {
+            QAPI_LIST_ADD(edge->perm, qapi_perm);
         }
-        if (p->flag & child->shared_perm) {
-            QAPI_LIST_ADD(edge->shared_perm, p->num);
+        if (flag & child->shared_perm) {
+            QAPI_LIST_ADD(edge->shared_perm, qapi_perm);
         }
     }
 
-- 
2.23.0



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH for-5.0 v4 3/5] blkdebug: Allow taking/unsharing permissions
  2019-11-08 12:34 [PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete Max Reitz
  2019-11-08 12:34 ` [PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm() Max Reitz
  2019-11-08 12:34 ` [PATCH for-5.0 v4 2/5] block: Use bdrv_qapi_perm_to_blk_perm() Max Reitz
@ 2019-11-08 12:34 ` Max Reitz
  2019-11-08 15:19   ` Vladimir Sementsov-Ogievskiy
  2019-11-08 12:34 ` [PATCH for-5.0 v4 4/5] iotests: Add @error to wait_until_completed Max Reitz
  2019-11-08 12:34 ` [PATCH for-5.0 v4 5/5] iotests: Add test for failing mirror complete Max Reitz
  4 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2019-11-08 12:34 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz

Sometimes it is useful to be able to add a node to the block graph that
takes or unshare a certain set of permissions for debugging purposes.
This patch adds this capability to blkdebug.

(Note that you cannot make blkdebug release or share permissions that it
needs to take or cannot share, because this might result in assertion
failures in the block layer.  But if the blkdebug node has no parents,
it will not take any permissions and share everything by default, so you
can then freely choose what permissions to take and share.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/blkdebug.c     | 93 +++++++++++++++++++++++++++++++++++++++++++-
 qapi/block-core.json | 14 ++++++-
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5ae96c52b0..af44aa973f 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -28,10 +28,14 @@
 #include "qemu/cutils.h"
 #include "qemu/config-file.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qapi/qapi-visit-block-core.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
 #include "sysemu/qtest.h"
 
 typedef struct BDRVBlkdebugState {
@@ -44,6 +48,9 @@ typedef struct BDRVBlkdebugState {
     uint64_t opt_discard;
     uint64_t max_discard;
 
+    uint64_t take_child_perms;
+    uint64_t unshare_child_perms;
+
     /* For blkdebug_refresh_filename() */
     char *config_file;
 
@@ -344,6 +351,69 @@ static void blkdebug_parse_filename(const char *filename, QDict *options,
     qdict_put_str(options, "x-image", filename);
 }
 
+static int blkdebug_parse_perm_list(uint64_t *dest, QDict *options,
+                                    const char *prefix, Error **errp)
+{
+    int ret = 0;
+    QDict *subqdict = NULL;
+    QObject *crumpled_subqdict = NULL;
+    Visitor *v = NULL;
+    BlockPermissionList *perm_list = NULL, *element;
+    Error *local_err = NULL;
+
+    *dest = 0;
+
+    qdict_extract_subqdict(options, &subqdict, prefix);
+    if (!qdict_size(subqdict)) {
+        goto out;
+    }
+
+    crumpled_subqdict = qdict_crumple(subqdict, errp);
+    if (!crumpled_subqdict) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    v = qobject_input_visitor_new(crumpled_subqdict);
+    visit_type_BlockPermissionList(v, NULL, &perm_list, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    for (element = perm_list; element; element = element->next) {
+        *dest |= bdrv_qapi_perm_to_blk_perm(element->value);
+    }
+
+out:
+    qapi_free_BlockPermissionList(perm_list);
+    visit_free(v);
+    qobject_unref(subqdict);
+    qobject_unref(crumpled_subqdict);
+    return ret;
+}
+
+static int blkdebug_parse_perms(BDRVBlkdebugState *s, QDict *options,
+                                Error **errp)
+{
+    int ret;
+
+    ret = blkdebug_parse_perm_list(&s->take_child_perms, options,
+                                   "take-child-perms.", errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    ret = blkdebug_parse_perm_list(&s->unshare_child_perms, options,
+                                   "unshare-child-perms.", errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return 0;
+}
+
 static QemuOptsList runtime_opts = {
     .name = "blkdebug",
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
@@ -419,6 +489,12 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     /* Set initial state */
     s->state = 1;
 
+    /* Parse permissions modifiers before opening the image file */
+    ret = blkdebug_parse_perms(s, options, errp);
+    if (ret < 0) {
+        goto out;
+    }
+
     /* Open the image file */
     bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
                                bs, &child_file, false, &local_err);
@@ -916,6 +992,21 @@ static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state,
     return 0;
 }
 
+static void blkdebug_child_perm(BlockDriverState *bs, BdrvChild *c,
+                                const BdrvChildRole *role,
+                                BlockReopenQueue *reopen_queue,
+                                uint64_t perm, uint64_t shared,
+                                uint64_t *nperm, uint64_t *nshared)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+
+    bdrv_filter_default_perms(bs, c, role, reopen_queue, perm, shared,
+                              nperm, nshared);
+
+    *nperm |= s->take_child_perms;
+    *nshared &= ~s->unshare_child_perms;
+}
+
 static const char *const blkdebug_strong_runtime_opts[] = {
     "config",
     "inject-error.",
@@ -940,7 +1031,7 @@ static BlockDriver bdrv_blkdebug = {
     .bdrv_file_open         = blkdebug_open,
     .bdrv_close             = blkdebug_close,
     .bdrv_reopen_prepare    = blkdebug_reopen_prepare,
-    .bdrv_child_perm        = bdrv_filter_default_perms,
+    .bdrv_child_perm        = blkdebug_child_perm,
 
     .bdrv_getlength         = blkdebug_getlength,
     .bdrv_refresh_filename  = blkdebug_refresh_filename,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index aa97ee2641..46f3491878 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3450,6 +3450,16 @@
 #
 # @set-state:       array of state-change descriptions
 #
+# @take-child-perms: Permissions to take on @image in addition to what
+#                    is necessary anyway (which depends on how the
+#                    blkdebug node is used).  Defaults to none.
+#                    (since 5.0)
+#
+# @unshare-child-perms: Permissions not to share on @image in addition
+#                       to what cannot be shared anyway (which depends
+#                       on how the blkdebug node is used).  Defaults
+#                       to none.  (since 5.0)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsBlkdebug',
@@ -3459,7 +3469,9 @@
             '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
             '*opt-discard': 'int32', '*max-discard': 'int32',
             '*inject-error': ['BlkdebugInjectErrorOptions'],
-            '*set-state': ['BlkdebugSetStateOptions'] } }
+            '*set-state': ['BlkdebugSetStateOptions'],
+            '*take-child-perms': ['BlockPermission'],
+            '*unshare-child-perms': ['BlockPermission'] } }
 
 ##
 # @BlockdevOptionsBlklogwrites:
-- 
2.23.0



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH for-5.0 v4 4/5] iotests: Add @error to wait_until_completed
  2019-11-08 12:34 [PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete Max Reitz
                   ` (2 preceding siblings ...)
  2019-11-08 12:34 ` [PATCH for-5.0 v4 3/5] blkdebug: Allow taking/unsharing permissions Max Reitz
@ 2019-11-08 12:34 ` Max Reitz
  2019-11-08 12:34 ` [PATCH for-5.0 v4 5/5] iotests: Add test for failing mirror complete Max Reitz
  4 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2019-11-08 12:34 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz

Callers can use this new parameter to expect failure during the
completion process.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/iotests.py | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 075f4739da..2010b71d4b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -783,15 +783,20 @@ class QMPTestCase(unittest.TestCase):
         self.assert_no_active_block_jobs()
         return result
 
-    def wait_until_completed(self, drive='drive0', check_offset=True, wait=60.0):
+    def wait_until_completed(self, drive='drive0', check_offset=True, wait=60.0,
+                             error=None):
         '''Wait for a block job to finish, returning the event'''
         while True:
             for event in self.vm.get_qmp_events(wait=wait):
                 if event['event'] == 'BLOCK_JOB_COMPLETED':
                     self.assert_qmp(event, 'data/device', drive)
-                    self.assert_qmp_absent(event, 'data/error')
-                    if check_offset:
-                        self.assert_qmp(event, 'data/offset', event['data']['len'])
+                    if error is None:
+                        self.assert_qmp_absent(event, 'data/error')
+                        if check_offset:
+                            self.assert_qmp(event, 'data/offset',
+                                            event['data']['len'])
+                    else:
+                        self.assert_qmp(event, 'data/error', error)
                     self.assert_no_active_block_jobs()
                     return event
                 elif event['event'] == 'JOB_STATUS_CHANGE':
@@ -809,7 +814,8 @@ class QMPTestCase(unittest.TestCase):
         self.assert_qmp(event, 'data/type', 'mirror')
         self.assert_qmp(event, 'data/offset', event['data']['len'])
 
-    def complete_and_wait(self, drive='drive0', wait_ready=True):
+    def complete_and_wait(self, drive='drive0', wait_ready=True,
+                          completion_error=None):
         '''Complete a block job and wait for it to finish'''
         if wait_ready:
             self.wait_ready(drive=drive)
@@ -817,7 +823,7 @@ class QMPTestCase(unittest.TestCase):
         result = self.vm.qmp('block-job-complete', device=drive)
         self.assert_qmp(result, 'return', {})
 
-        event = self.wait_until_completed(drive=drive)
+        event = self.wait_until_completed(drive=drive, error=completion_error)
         self.assert_qmp(event, 'data/type', 'mirror')
 
     def pause_wait(self, job_id='job0'):
-- 
2.23.0



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH for-5.0 v4 5/5] iotests: Add test for failing mirror complete
  2019-11-08 12:34 [PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete Max Reitz
                   ` (3 preceding siblings ...)
  2019-11-08 12:34 ` [PATCH for-5.0 v4 4/5] iotests: Add @error to wait_until_completed Max Reitz
@ 2019-11-08 12:34 ` Max Reitz
  4 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2019-11-08 12:34 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/041     | 44 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out |  4 ++--
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 8568426311..d7be30b62b 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1121,6 +1121,50 @@ class TestOrphanedSource(iotests.QMPTestCase):
                              target='dest-ro')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
+    def test_failing_permission_in_complete(self):
+        self.assert_no_active_block_jobs()
+
+        # Unshare consistent-read on the target
+        # (The mirror job does not care)
+        result = self.vm.qmp('blockdev-add',
+                             driver='blkdebug',
+                             node_name='dest-perm',
+                             image='dest',
+                             unshare_child_perms=['consistent-read'])
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-mirror', job_id='job', device='src',
+                             sync='full', target='dest',
+                             filter_node_name='mirror-filter')
+        self.assert_qmp(result, 'return', {})
+
+        # Require consistent-read on the source
+        # (We can only add this node once the job has started, or it
+        # will complain that it does not want to run on non-root nodes)
+        result = self.vm.qmp('blockdev-add',
+                             driver='blkdebug',
+                             node_name='src-perm',
+                             image='src',
+                             take_child_perms=['consistent-read'])
+        self.assert_qmp(result, 'return', {})
+
+        # While completing, mirror will attempt to replace src by
+        # dest, which must fail because src-perm requires
+        # consistent-read but dest-perm does not share it; thus
+        # aborting the job when it is supposed to complete
+        self.complete_and_wait('job',
+                               completion_error='Operation not permitted')
+
+        # Assert that all of our nodes are still there (except for the
+        # mirror filter, which should be gone despite the failure)
+        nodes = self.vm.qmp('query-named-block-nodes')['return']
+        nodes = [node['node-name'] for node in nodes]
+
+        for expect in ('src', 'src-perm', 'dest', 'dest-perm'):
+            self.assertTrue(expect in nodes, '%s disappeared' % expect)
+        self.assertFalse('mirror-filter' in nodes,
+                         'Mirror filter node did not disappear')
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'],
                  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 2c448b4239..f496be9197 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-..........................................................................................
+...........................................................................................
 ----------------------------------------------------------------------
-Ran 90 tests
+Ran 91 tests
 
 OK
-- 
2.23.0



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm()
  2019-11-08 12:34 ` [PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm() Max Reitz
@ 2019-11-08 14:05   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-11-08 14:05 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel

08.11.2019 15:34, Max Reitz wrote:
> We need some way to correlate QAPI BlockPermission values with
> BLK_PERM_* flags.  We could:
> 
> (1) have the same order in the QAPI definition as the the BLK_PERM_*
>      flags are in LSb-first order.  However, then there is no guarantee
>      that they actually match (e.g. when someone modifies the QAPI schema
>      without thinking of the BLK_PERM_* definitions).
>      We could add static assertions, but these would break what’s good
>      about this solution, namely its simplicity.
> 
> (2) define the BLK_PERM_* flags based on the BlockPermission values.
>      But this way whenever someone were to modify the QAPI order
>      (perfectly sensible in theory), the BLK_PERM_* values would change.
>      Because these values are used for file locking, this might break
>      file locking between different qemu versions.
> 
> Therefore, go the slightly more cumbersome way: Add a function to
> translate from the QAPI constants to the BLK_PERM_* flags.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

> ---
>   block.c               | 18 ++++++++++++++++++
>   include/block/block.h |  1 +
>   2 files changed, 19 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 4cffc2bc35..066433f3e2 100644
> --- a/block.c
> +++ b/block.c
> @@ -2227,6 +2227,24 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
>       *nshared = shared;
>   }
>   
> +uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
> +{
> +    static const uint64_t permissions[] = {
> +        [BLOCK_PERMISSION_CONSISTENT_READ]  = BLK_PERM_CONSISTENT_READ,
> +        [BLOCK_PERMISSION_WRITE]            = BLK_PERM_WRITE,
> +        [BLOCK_PERMISSION_WRITE_UNCHANGED]  = BLK_PERM_WRITE_UNCHANGED,
> +        [BLOCK_PERMISSION_RESIZE]           = BLK_PERM_RESIZE,
> +        [BLOCK_PERMISSION_GRAPH_MOD]        = BLK_PERM_GRAPH_MOD,
> +    };
> +
> +    QEMU_BUILD_BUG_ON(ARRAY_SIZE(permissions) != BLOCK_PERMISSION__MAX);
> +    QEMU_BUILD_BUG_ON(1UL << ARRAY_SIZE(permissions) != BLK_PERM_ALL + 1);

safe enough)

> +
> +    assert(qapi_perm < BLOCK_PERMISSION__MAX);
> +
> +    return permissions[qapi_perm];
> +}
> +
>   static void bdrv_replace_child_noperm(BdrvChild *child,
>                                         BlockDriverState *new_bs)
>   {
> diff --git a/include/block/block.h b/include/block/block.h
> index 1df9848e74..e9dcfef7fa 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -280,6 +280,7 @@ enum {
>   };
>   
>   char *bdrv_perm_names(uint64_t perm);
> +uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm);
>   
>   /* disk I/O throttling */
>   void bdrv_init(void);
> 


-- 
Best regards,
Vladimir

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH for-5.0 v4 2/5] block: Use bdrv_qapi_perm_to_blk_perm()
  2019-11-08 12:34 ` [PATCH for-5.0 v4 2/5] block: Use bdrv_qapi_perm_to_blk_perm() Max Reitz
@ 2019-11-08 14:11   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-11-08 14:11 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel

08.11.2019 15:34, Max Reitz wrote:
> We can save some LoC in xdbg_graph_add_edge() by using
> bdrv_qapi_perm_to_blk_perm().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

> ---
>   block.c | 29 ++++++++---------------------
>   1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 066433f3e2..ae279ff21f 100644
> --- a/block.c
> +++ b/block.c
> @@ -4870,36 +4870,23 @@ static void xdbg_graph_add_node(XDbgBlockGraphConstructor *gr, void *node,
>   static void xdbg_graph_add_edge(XDbgBlockGraphConstructor *gr, void *parent,
>                                   const BdrvChild *child)
>   {
> -    typedef struct {
> -        unsigned int flag;
> -        BlockPermission num;
> -    } PermissionMap;
> -
> -    static const PermissionMap permissions[] = {
> -        { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
> -        { BLK_PERM_WRITE,           BLOCK_PERMISSION_WRITE },
> -        { BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
> -        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
> -        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
> -        { 0, 0 }
> -    };
> -    const PermissionMap *p;
> +    BlockPermission qapi_perm;
>       XDbgBlockGraphEdge *edge;
>   
> -    QEMU_BUILD_BUG_ON(1UL << (ARRAY_SIZE(permissions) - 1) != BLK_PERM_ALL + 1);
> -
>       edge = g_new0(XDbgBlockGraphEdge, 1);
>   
>       edge->parent = xdbg_graph_node_num(gr, parent);
>       edge->child = xdbg_graph_node_num(gr, child->bs);
>       edge->name = g_strdup(child->name);
>   
> -    for (p = permissions; p->flag; p++) {
> -        if (p->flag & child->perm) {
> -            QAPI_LIST_ADD(edge->perm, p->num);
> +    for (qapi_perm = 0; qapi_perm < BLOCK_PERMISSION__MAX; qapi_perm++) {
> +        uint64_t flag = bdrv_qapi_perm_to_blk_perm(qapi_perm);
> +
> +        if (flag & child->perm) {
> +            QAPI_LIST_ADD(edge->perm, qapi_perm);
>           }
> -        if (p->flag & child->shared_perm) {
> -            QAPI_LIST_ADD(edge->shared_perm, p->num);
> +        if (flag & child->shared_perm) {
> +            QAPI_LIST_ADD(edge->shared_perm, qapi_perm);
>           }
>       }
>   
> 


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH for-5.0 v4 3/5] blkdebug: Allow taking/unsharing permissions
  2019-11-08 12:34 ` [PATCH for-5.0 v4 3/5] blkdebug: Allow taking/unsharing permissions Max Reitz
@ 2019-11-08 15:19   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-11-08 15:19 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel

08.11.2019 15:34, Max Reitz wrote:
> Sometimes it is useful to be able to add a node to the block graph that
> takes or unshare a certain set of permissions for debugging purposes.
> This patch adds this capability to blkdebug.
> 
> (Note that you cannot make blkdebug release or share permissions that it
> needs to take or cannot share, because this might result in assertion
> failures in the block layer.  But if the blkdebug node has no parents,
> it will not take any permissions and share everything by default, so you
> can then freely choose what permissions to take and share.)
> 
> Signed-off-by: Max Reitz<mreitz@redhat.com>


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

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 12:34 [PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete Max Reitz
2019-11-08 12:34 ` [PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm() Max Reitz
2019-11-08 14:05   ` Vladimir Sementsov-Ogievskiy
2019-11-08 12:34 ` [PATCH for-5.0 v4 2/5] block: Use bdrv_qapi_perm_to_blk_perm() Max Reitz
2019-11-08 14:11   ` Vladimir Sementsov-Ogievskiy
2019-11-08 12:34 ` [PATCH for-5.0 v4 3/5] blkdebug: Allow taking/unsharing permissions Max Reitz
2019-11-08 15:19   ` Vladimir Sementsov-Ogievskiy
2019-11-08 12:34 ` [PATCH for-5.0 v4 4/5] iotests: Add @error to wait_until_completed Max Reitz
2019-11-08 12:34 ` [PATCH for-5.0 v4 5/5] iotests: Add test for failing mirror complete Max Reitz

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git