[PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete

[PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete

[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

[PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm()

[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

Re: [PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm()

[Vladimir Sementsov-Ogievskiy]

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

[PATCH for-5.0 v4 2/5] block: Use bdrv_qapi_perm_to_blk_perm()

[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

Re: [PATCH for-5.0 v4 2/5] block: Use bdrv_qapi_perm_to_blk_perm()

[Vladimir Sementsov-Ogievskiy]

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

[PATCH for-5.0 v4 3/5] blkdebug: Allow taking/unsharing permissions

[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

Re: [PATCH for-5.0 v4 3/5] blkdebug: Allow taking/unsharing permissions

[Vladimir Sementsov-Ogievskiy]

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

[PATCH for-5.0 v4 4/5] iotests: Add @error to wait_until_completed

[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

[PATCH for-5.0 v4 5/5] iotests: Add test for failing mirror complete

[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

Re: [PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1032 bytes --]

On 08.11.19 13:34, Max Reitz wrote:
> 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

Thanks for the review, applied to my block-next branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]