[Qemu-devel] [PATCH 0/4] mirror: Do not dereference invalid pointers

[Qemu-devel] [PATCH 0/4] mirror: Do not dereference invalid pointers

[Max Reitz]

Hi,

The fix (patch 1) is pretty straightforward; patch 2 (which I need for
the test) may not be.

The biggest problem with patch 2 is that you can use it to uncover where
our permission handling is broken.  For example, devising the test case
(patch 4) was very difficult because I kept running into the
&error_abort that mirror_exit_common() passes when dropping the
mirror_top_bs.

The problem is that mirror_top_bs does not take the same permissions
that its parent takes.  Ergo using &error_abort when dropping it is
wrong: The parent may require more permissions that mirror_top_bs did,
and so dropping mirror_top_bs may fail.

Now what’s really bad is that this cannot be fixed with our current
permission system.  mirror_top_bs was introduced precisely so it does
not take CONSISTENT_READ, but can still allow parents to take it (for
active commits).  But what if there is actually something besides the
mirror job that unshares CONSISTENT_READ?


Imagine this:

      mirror target BB       mirror source BB
                  |             |
                  v             v
mirror_top_bs -> top -> mid -> base
                  ^
                  |
             other_parent

The source BB unshares CONSISTENT_READ on the base.  mirror_top_bs
ensures that its parents can read from top even though top itself cannot
allow CONSISTENT_READ to be taken.  So far so good.

But what if other_parent also unshares CONSISTENT_READ?  Then,
mirror_top_bs has no business allowing its parents to take it.

No idea how to fix that.  (I suppose mirror_top_bs would need some way
to verify that there is no other party that has unshared CONSISTENT_READ
but its associated source BB.  In the future, we want the source BB to
go away and instead have the source be an immediate BdrvChild of
mirror_top_bs.  Maybe we can then build something into the block layer
so that a node can only restore CONSISTENT_READ when it was that node
that broke it?)


Anyway.  You can see something arising from this problem simply by
unsharing CONSISTENT_READ on the target node.  (Just drop the src-perm
node from the test I add in patch 4.)  Replacing the source with the
target will then work fine (because mirror_top_bs doesn’t care about
CONSISTENT_READ being removed), but then you cannot drop mirror_top_bs –
because its parent does want CONSISTENT_READ.  Thus, the &error_abort
aborts.


While this is a more special case, I have no idea how to fix this one
either.


Soo...  This series just fixes one thing, and leaves another unfixed
because I have no idea how to fix it.  Worse, it adds parameters to
blkdebug to actually see the problem.  Do we want to let blkdebug be
able to crash qemu (because of a bug in qemu)?


Max Reitz (4):
  mirror: Do not dereference invalid pointers
  blkdebug: Allow taking/unsharing permissions
  iotests: Add @error to wait_until_completed
  iotests: Add test for failing mirror complete

 qapi/block-core.json          |  29 +++++++++-
 block/blkdebug.c              | 106 +++++++++++++++++++++++++++++++++-
 block/mirror.c                |  13 +++--
 tests/qemu-iotests/041        |  44 ++++++++++++++
 tests/qemu-iotests/041.out    |   4 +-
 tests/qemu-iotests/iotests.py |  18 ++++--
 6 files changed, 200 insertions(+), 14 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/4] mirror: Do not dereference invalid pointers

[Max Reitz]

mirror_exit_common() may be called twice (if it is called from
mirror_prepare() and fails, it will be called from mirror_abort()
again).

In such a case, many of the pointers in the MirrorBlockJob object will
already be freed.  This can be seen most reliably for s->target, which
is set to NULL (and then dereferenced by blk_bs()).

Cc: qemu-stable@nongnu.org
Fixes: 737efc1eda23b904fbe0e66b37715fb0e5c3e58b
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index fe984efb90..706d80fced 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -620,11 +620,11 @@ static int mirror_exit_common(Job *job)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
     BlockJob *bjob = &s->common;
-    MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
+    MirrorBDSOpaque *bs_opaque;
     AioContext *replace_aio_context = NULL;
-    BlockDriverState *src = s->mirror_top_bs->backing->bs;
-    BlockDriverState *target_bs = blk_bs(s->target);
-    BlockDriverState *mirror_top_bs = s->mirror_top_bs;
+    BlockDriverState *src;
+    BlockDriverState *target_bs;
+    BlockDriverState *mirror_top_bs;
     Error *local_err = NULL;
     bool abort = job->ret < 0;
     int ret = 0;
@@ -634,6 +634,11 @@ static int mirror_exit_common(Job *job)
     }
     s->prepared = true;
 
+    mirror_top_bs = s->mirror_top_bs;
+    bs_opaque = mirror_top_bs->opaque;
+    src = mirror_top_bs->backing->bs;
+    target_bs = blk_bs(s->target);
+
     if (bdrv_chain_contains(src, target_bs)) {
         bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
     }
-- 
2.21.0

[Qemu-devel] [PATCH 2/4] 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>
---
 qapi/block-core.json |  29 +++++++++++-
 block/blkdebug.c     | 106 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 133 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index e6edd641f1..336043e02c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3347,6 +3347,21 @@
             '*state': 'int',
             'new_state': 'int' } }
 
+##
+# @BlockdevPermission:
+#
+# Permissions that an edge in the block graph can take or share.
+#
+# Since: 4.2
+##
+{ 'enum': 'BlockdevPermission',
+  'data': [
+      'consistent-read',
+      'write',
+      'write-unchanged',
+      'resize',
+      'graph-mod' ] }
+
 ##
 # @BlockdevOptionsBlkdebug:
 #
@@ -3388,6 +3403,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 4.2)
+#
+# @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 4.2)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsBlkdebug',
@@ -3397,7 +3422,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': ['BlockdevPermission'],
+            '*unshare-child-perms': ['BlockdevPermission'] } }
 
 ##
 # @BlockdevOptionsBlklogwrites:
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5ae96c52b0..ec24a5e4e5 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -28,9 +28,11 @@
 #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/qmp/qdict.h"
+#include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qstring.h"
 #include "sysemu/qtest.h"
 
@@ -44,6 +46,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 +349,84 @@ 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;
+    QList *perm_list;
+    const QListEntry *perm;
+
+    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;
+    }
+
+    perm_list = qobject_to(QList, crumpled_subqdict);
+    if (!perm_list) {
+        /* Omit the trailing . from the prefix */
+        error_setg(errp, "%.*s expects a list",
+                   (int)(strlen(prefix) - 1), prefix);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    for (perm = qlist_first(perm_list); perm; perm = qlist_next(perm)) {
+        const char *perm_name;
+        BlockdevPermission perm_bit;
+
+        perm_name = qstring_get_try_str(qobject_to(QString, perm->value));
+        if (!perm_name) {
+            /* Omit the trailing . from the prefix */
+            error_setg(errp, "%.*s expects a list of enum strings",
+                       (int)(strlen(prefix) - 1), prefix);
+            ret = -EINVAL;
+            goto out;
+        }
+
+        perm_bit = qapi_enum_parse(&BlockdevPermission_lookup, perm_name,
+                                   BLOCKDEV_PERMISSION__MAX, errp);
+        if (perm_bit == BLOCKDEV_PERMISSION__MAX) {
+            ret = -EINVAL;
+            goto out;
+        }
+
+        *dest |= UINT64_C(1) << perm_bit;
+    }
+
+out:
+    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 +502,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 +1005,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 +1044,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,
-- 
2.21.0

[Qemu-devel] [PATCH 3/4] 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>
---
 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 b26271187c..300347c7c8 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -745,15 +745,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':
@@ -771,7 +776,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)
@@ -779,7 +785,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.21.0

[Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@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..84bc6d6581 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 = list(map(lambda image: image['node-name'], 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.21.0

Re: [Qemu-devel] [PATCH 1/4] mirror: Do not dereference invalid pointers

[John Snow]

On 9/12/19 9:56 AM, Max Reitz wrote:
> mirror_exit_common() may be called twice (if it is called from
> mirror_prepare() and fails, it will be called from mirror_abort()
> again).
> 
> In such a case, many of the pointers in the MirrorBlockJob object will
> already be freed.  This can be seen most reliably for s->target, which
> is set to NULL (and then dereferenced by blk_bs()).
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 737efc1eda23b904fbe0e66b37715fb0e5c3e58b
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Sorry that I left the mirror callbacks such a mess. I think I got the
design for the completion callbacks completely wrong, because of how
hard it is to disentangle mirror into something reasonable here.

The original idea was that it calls .prepare, then either .commit or
.abort, then .clean. Ideally, there would be no exit_common for mirror;
everything would be sorted into the proper little callback silos.

The problem with the existing design is that if it has already failed by
.prepare time, we jump straight to .abort, so it has this uneven,
unbalanced design. The code in abort and cleanup therefore has to work
doubly-hard to figure out what exactly it needs to do.

It's a mess.

I wonder if it can be improved by always calling prepare, even when
we've already failed. We could just posit that it now means "prepare to
commit" or "prepare to abort" but we can do all of the work that can
still fail there in one shot.

Maybe that will make the work that needs to happen in abort/commit
easier to digest.

> ---
>  block/mirror.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index fe984efb90..706d80fced 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -620,11 +620,11 @@ static int mirror_exit_common(Job *job)
>  {
>      MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>      BlockJob *bjob = &s->common;
> -    MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
> +    MirrorBDSOpaque *bs_opaque;
>      AioContext *replace_aio_context = NULL;
> -    BlockDriverState *src = s->mirror_top_bs->backing->bs;
> -    BlockDriverState *target_bs = blk_bs(s->target);
> -    BlockDriverState *mirror_top_bs = s->mirror_top_bs;
> +    BlockDriverState *src;
> +    BlockDriverState *target_bs;
> +    BlockDriverState *mirror_top_bs;
>      Error *local_err = NULL;
>      bool abort = job->ret < 0;
>      int ret = 0;
> @@ -634,6 +634,11 @@ static int mirror_exit_common(Job *job)
>      }
>      s->prepared = true;
>  
> +    mirror_top_bs = s->mirror_top_bs;
> +    bs_opaque = mirror_top_bs->opaque;
> +    src = mirror_top_bs->backing->bs;
> +    target_bs = blk_bs(s->target);
> +
>      if (bdrv_chain_contains(src, target_bs)) {
>          bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
>      }
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 3/4] iotests: Add @error to wait_until_completed

[John Snow]

On 9/12/19 9:56 AM, Max Reitz wrote:
> Callers can use this new parameter to expect failure during the
> completion process.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.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 b26271187c..300347c7c8 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -745,15 +745,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':
> @@ -771,7 +776,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)
> @@ -779,7 +785,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'):
> 

toot toot more optional parameters. lay them at the altar of
noncommittal python design.

I completely forget what the difference between unittest.TestCase and
qtest.QEMUQtestMachine is and why they each have job management methods.

Well, OK: the VM one is a simple subclass of the general-purpose VM
machine to add some more useful stuff. the unittest one implements some
general-purpose behavior with asserts that only work in the unittest world.

Still,

It's a little fun that we've got run_job as well as cancel_and_wait,
wait_until_completed, wait_ready, wait_ready_and_cancel, pause_wait and
pause_job and they all seem to implement job run-state logic management
a little differently.

Probably no bugs there, I bet.

*cough* Not your fault, anyway, so please take this accolade:

Reviewed-by: John Snow <jsnow@redhat.com>


(it's probably my fault)

Re: [Qemu-devel] [PATCH 3/4] iotests: Add @error to wait_until_completed

[Max Reitz]

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

On 14.09.19 00:53, John Snow wrote:
> 
> 
> On 9/12/19 9:56 AM, Max Reitz wrote:
>> Callers can use this new parameter to expect failure during the
>> completion process.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.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 b26271187c..300347c7c8 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -745,15 +745,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':
>> @@ -771,7 +776,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)
>> @@ -779,7 +785,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'):
>>
> 
> toot toot more optional parameters. lay them at the altar of
> noncommittal python design.
> 
> I completely forget what the difference between unittest.TestCase and
> qtest.QEMUQtestMachine is and why they each have job management methods.
> 
> Well, OK: the VM one is a simple subclass of the general-purpose VM
> machine to add some more useful stuff. the unittest one implements some
> general-purpose behavior with asserts that only work in the unittest world.

Yes.  run_job doesn’t assert anything (well, I could check the returned
error, but that’s it), it’s just based on comparison to the reference
output.  That’s not so useful for unittest-style tests, so it’s better
to use complete_and_wait() etc. there.

> Still,
> 
> It's a little fun that we've got run_job as well as cancel_and_wait,
> wait_until_completed, wait_ready, wait_ready_and_cancel, pause_wait and
> pause_job and they all seem to implement job run-state logic management
> a little differently.

At the end of the day, it’s all just test code.  It doesn’t hurt too
much for it to have duplicated code and be generally messy.

> Probably no bugs there, I bet.

Hm.  The functions are not that complex.

> *cough* Not your fault, anyway, so please take this accolade:
> 
> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks!

> (it's probably my fault)

I can only imagine by nonassistance of code in danger.

Max


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

Re: [Qemu-devel] [PATCH 1/4] mirror: Do not dereference invalid pointers

[Vladimir Sementsov-Ogievskiy]

12.09.2019 16:56, Max Reitz wrote:
> mirror_exit_common() may be called twice (if it is called from
> mirror_prepare() and fails, it will be called from mirror_abort()
> again).
> 
> In such a case, many of the pointers in the MirrorBlockJob object will
> already be freed.  This can be seen most reliably for s->target, which
> is set to NULL (and then dereferenced by blk_bs()).
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 737efc1eda23b904fbe0e66b37715fb0e5c3e58b
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

> ---
>   block/mirror.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index fe984efb90..706d80fced 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -620,11 +620,11 @@ static int mirror_exit_common(Job *job)
>   {
>       MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>       BlockJob *bjob = &s->common;
> -    MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
> +    MirrorBDSOpaque *bs_opaque;
>       AioContext *replace_aio_context = NULL;
> -    BlockDriverState *src = s->mirror_top_bs->backing->bs;
> -    BlockDriverState *target_bs = blk_bs(s->target);
> -    BlockDriverState *mirror_top_bs = s->mirror_top_bs;
> +    BlockDriverState *src;
> +    BlockDriverState *target_bs;
> +    BlockDriverState *mirror_top_bs;
>       Error *local_err = NULL;
>       bool abort = job->ret < 0;
>       int ret = 0;
> @@ -634,6 +634,11 @@ static int mirror_exit_common(Job *job)
>       }
>       s->prepared = true;
>   
> +    mirror_top_bs = s->mirror_top_bs;
> +    bs_opaque = mirror_top_bs->opaque;
> +    src = mirror_top_bs->backing->bs;
> +    target_bs = blk_bs(s->target);
> +
>       if (bdrv_chain_contains(src, target_bs)) {
>           bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
>       }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/4] mirror: Do not dereference invalid pointers

[Vladimir Sementsov-Ogievskiy]

12.09.2019 16:56, Max Reitz wrote:
> Hi,
> 
> The fix (patch 1) is pretty straightforward; patch 2 (which I need for
> the test) may not be.
> 
> The biggest problem with patch 2 is that you can use it to uncover where
> our permission handling is broken.  For example, devising the test case
> (patch 4) was very difficult because I kept running into the
> &error_abort that mirror_exit_common() passes when dropping the
> mirror_top_bs.
> 
> The problem is that mirror_top_bs does not take the same permissions
> that its parent takes.  Ergo using &error_abort when dropping it is
> wrong: The parent may require more permissions that mirror_top_bs did,
> and so dropping mirror_top_bs may fail.
> 
> Now what’s really bad is that this cannot be fixed with our current
> permission system.  mirror_top_bs was introduced precisely so it does
> not take CONSISTENT_READ, but can still allow parents to take it (for
> active commits).  But what if there is actually something besides the
> mirror job that unshares CONSISTENT_READ?
> 
> 
> Imagine this:
> 
>        mirror target BB       mirror source BB
>                    |             |
>                    v             v
> mirror_top_bs -> top -> mid -> base
>                    ^
>                    |
>               other_parent
> 
> The source BB unshares CONSISTENT_READ on the base.  mirror_top_bs
> ensures that its parents can read from top even though top itself cannot
> allow CONSISTENT_READ to be taken.  So far so good.
> 
> But what if other_parent also unshares CONSISTENT_READ?  Then,
> mirror_top_bs has no business allowing its parents to take it.
> 
> No idea how to fix that.  (I suppose mirror_top_bs would need some way
> to verify that there is no other party that has unshared CONSISTENT_READ
> but its associated source BB.

May be we need grouped permissions?

Some way to define group of children, which may unshare read permission
for other children (out of the group), but still children in group may
have read permission?

But it don't work here as we are saying about children on different
nodes.. And propagated through backing chain permissions..


>  In the future, we want the source BB to
> go away and instead have the source be an immediate BdrvChild of
> mirror_top_bs.  Maybe we can then build something into the block layer
> so that a node can only restore CONSISTENT_READ when it was that node
> that broke it?)
> 
> 
> Anyway.  You can see something arising from this problem simply by
> unsharing CONSISTENT_READ on the target node.  (Just drop the src-perm
> node from the test I add in patch 4.)  Replacing the source with the
> target will then work fine (because mirror_top_bs doesn’t care about
> CONSISTENT_READ being removed), but then you cannot drop mirror_top_bs –
> because its parent does want CONSISTENT_READ.  Thus, the &error_abort
> aborts.
> 
> 
> While this is a more special case, I have no idea how to fix this one
> either.
> 
> 
> Soo...  This series just fixes one thing, and leaves another unfixed
> because I have no idea how to fix it.  Worse, it adds parameters to
> blkdebug to actually see the problem.  Do we want to let blkdebug be
> able to crash qemu (because of a bug in qemu)?
> 

blkdebug is for debugging and not used by end users like libvirt, yes?

> 
> Max Reitz (4):
>    mirror: Do not dereference invalid pointers
>    blkdebug: Allow taking/unsharing permissions
>    iotests: Add @error to wait_until_completed
>    iotests: Add test for failing mirror complete
> 
>   qapi/block-core.json          |  29 +++++++++-
>   block/blkdebug.c              | 106 +++++++++++++++++++++++++++++++++-
>   block/mirror.c                |  13 +++--
>   tests/qemu-iotests/041        |  44 ++++++++++++++
>   tests/qemu-iotests/041.out    |   4 +-
>   tests/qemu-iotests/iotests.py |  18 ++++--
>   6 files changed, 200 insertions(+), 14 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/4] blkdebug: Allow taking/unsharing permissions

[Vladimir Sementsov-Ogievskiy]

12.09.2019 16:56, 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>
> ---
>   qapi/block-core.json |  29 +++++++++++-
>   block/blkdebug.c     | 106 ++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 133 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e6edd641f1..336043e02c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3347,6 +3347,21 @@
>               '*state': 'int',
>               'new_state': 'int' } }
>   
> +##
> +# @BlockdevPermission:
> +#
> +# Permissions that an edge in the block graph can take or share.
> +#
> +# Since: 4.2
> +##
> +{ 'enum': 'BlockdevPermission',
> +  'data': [
> +      'consistent-read',
> +      'write',
> +      'write-unchanged',
> +      'resize',
> +      'graph-mod' ] }

:)

BlockPermission is already here since 4.0 and has exactly same content. (And better documented)

> +
>   ##
>   # @BlockdevOptionsBlkdebug:
>   #
> @@ -3388,6 +3403,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 4.2)
> +#
> +# @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 4.2)
> +#
>   # Since: 2.9
>   ##
>   { 'struct': 'BlockdevOptionsBlkdebug',
> @@ -3397,7 +3422,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': ['BlockdevPermission'],
> +            '*unshare-child-perms': ['BlockdevPermission'] } }
>   
>   ##
>   # @BlockdevOptionsBlklogwrites:
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 5ae96c52b0..ec24a5e4e5 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -28,9 +28,11 @@
>   #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/qmp/qdict.h"
> +#include "qapi/qmp/qlist.h"
>   #include "qapi/qmp/qstring.h"
>   #include "sysemu/qtest.h"
>   
> @@ -44,6 +46,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 +349,84 @@ 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;
> +    QList *perm_list;
> +    const QListEntry *perm;
> +
> +    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;
> +    }
> +
> +    perm_list = qobject_to(QList, crumpled_subqdict);
> +    if (!perm_list) {
> +        /* Omit the trailing . from the prefix */
> +        error_setg(errp, "%.*s expects a list",
> +                   (int)(strlen(prefix) - 1), prefix);
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    for (perm = qlist_first(perm_list); perm; perm = qlist_next(perm)) {
> +        const char *perm_name;
> +        BlockdevPermission perm_bit;
> +
> +        perm_name = qstring_get_try_str(qobject_to(QString, perm->value));
> +        if (!perm_name) {
> +            /* Omit the trailing . from the prefix */
> +            error_setg(errp, "%.*s expects a list of enum strings",
> +                       (int)(strlen(prefix) - 1), prefix);
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        perm_bit = qapi_enum_parse(&BlockdevPermission_lookup, perm_name,
> +                                   BLOCKDEV_PERMISSION__MAX, errp);
> +        if (perm_bit == BLOCKDEV_PERMISSION__MAX) {
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        *dest |= UINT64_C(1) << perm_bit;
> +    }
> +
> +out:
> +    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;
> +}


It's a pity that being described in json, these new parameters still not parsed automatically..

> +
>   static QemuOptsList runtime_opts = {

and that we have to keep these runtime_opts everywhere, which duplicates json definitions..

>       .name = "blkdebug",
>       .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> @@ -419,6 +502,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 +1005,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 +1044,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,
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/4] iotests: Add @error to wait_until_completed

[Vladimir Sementsov-Ogievskiy]

12.09.2019 16:56, Max Reitz wrote:
> Callers can use this new parameter to expect failure during the
> completion process.
> 
> Signed-off-by: Max Reitz<mreitz@redhat.com>

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

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete

[Vladimir Sementsov-Ogievskiy]

12.09.2019 16:56, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@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..84bc6d6581 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 = list(map(lambda image: image['node-name'], nodes))

using list comprehension is a bit more pythonic:
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
> 


With or without my suggestion:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

I checked, that it pass, and that fails (generates segfault) if drop patch 01:
Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete

[John Snow]

On 9/12/19 9:56 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@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..84bc6d6581 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 = list(map(lambda image: image['node-name'], nodes))

Sadly, the list comprehension is a good suggestion. Squash it in if 
you'd like.

> +
> +        for expect in ['src', 'src-perm', 'dest', 'dest-perm']:
> +            self.assertTrue(expect in nodes, '%s disappeared' % expect)

I could be a real weenie and say "why not use a tuple here?"

but, I'll only pretend I didn't say that instead of couching it in a 
self-deprecating wrapper to the same end effect.

(I'm a weenie.)

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

Or don't do anything, because I'm just being obnoxious, and I owe you 
one for giving you bad advice last round.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 0/4] mirror: Do not dereference invalid pointers

[Max Reitz]

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

On 18.09.19 17:38, Vladimir Sementsov-Ogievskiy wrote:
> 12.09.2019 16:56, Max Reitz wrote:
>> Hi,
>>
>> The fix (patch 1) is pretty straightforward; patch 2 (which I need for
>> the test) may not be.
>>
>> The biggest problem with patch 2 is that you can use it to uncover where
>> our permission handling is broken.  For example, devising the test case
>> (patch 4) was very difficult because I kept running into the
>> &error_abort that mirror_exit_common() passes when dropping the
>> mirror_top_bs.
>>
>> The problem is that mirror_top_bs does not take the same permissions
>> that its parent takes.  Ergo using &error_abort when dropping it is
>> wrong: The parent may require more permissions that mirror_top_bs did,
>> and so dropping mirror_top_bs may fail.
>>
>> Now what’s really bad is that this cannot be fixed with our current
>> permission system.  mirror_top_bs was introduced precisely so it does
>> not take CONSISTENT_READ, but can still allow parents to take it (for
>> active commits).  But what if there is actually something besides the
>> mirror job that unshares CONSISTENT_READ?
>>
>>
>> Imagine this:
>>
>>        mirror target BB       mirror source BB
>>                    |             |
>>                    v             v
>> mirror_top_bs -> top -> mid -> base
>>                    ^
>>                    |
>>               other_parent
>>
>> The source BB unshares CONSISTENT_READ on the base.  mirror_top_bs
>> ensures that its parents can read from top even though top itself cannot
>> allow CONSISTENT_READ to be taken.  So far so good.
>>
>> But what if other_parent also unshares CONSISTENT_READ?  Then,
>> mirror_top_bs has no business allowing its parents to take it.
>>
>> No idea how to fix that.  (I suppose mirror_top_bs would need some way
>> to verify that there is no other party that has unshared CONSISTENT_READ
>> but its associated source BB.
> 
> May be we need grouped permissions?
> 
> Some way to define group of children, which may unshare read permission
> for other children (out of the group), but still children in group may
> have read permission?

Hm, is that different from my idea below where one of mirror_top's
children unshares the read permission, and another is allowed to take it
still?

(The problem is always that if some BDS has a parent that unshares this
permission, this condition propagates upwards through its other parents,
and we need to keep track of who unshared it in the first place.)

> But it don't work here as we are saying about children on different
> nodes.. And propagated through backing chain permissions..

Yep.

>>  In the future, we want the source BB to
>> go away and instead have the source be an immediate BdrvChild of
>> mirror_top_bs.  Maybe we can then build something into the block layer
>> so that a node can only restore CONSISTENT_READ when it was that node
>> that broke it?)
>>
>>
>> Anyway.  You can see something arising from this problem simply by
>> unsharing CONSISTENT_READ on the target node.  (Just drop the src-perm
>> node from the test I add in patch 4.)  Replacing the source with the
>> target will then work fine (because mirror_top_bs doesn’t care about
>> CONSISTENT_READ being removed), but then you cannot drop mirror_top_bs –
>> because its parent does want CONSISTENT_READ.  Thus, the &error_abort
>> aborts.
>>
>>
>> While this is a more special case, I have no idea how to fix this one
>> either.
>>
>>
>> Soo...  This series just fixes one thing, and leaves another unfixed
>> because I have no idea how to fix it.  Worse, it adds parameters to
>> blkdebug to actually see the problem.  Do we want to let blkdebug be
>> able to crash qemu (because of a bug in qemu)?
>>
> 
> blkdebug is for debugging and not used by end users like libvirt, yes?

Correct.

Max

>>
>> Max Reitz (4):
>>    mirror: Do not dereference invalid pointers
>>    blkdebug: Allow taking/unsharing permissions
>>    iotests: Add @error to wait_until_completed
>>    iotests: Add test for failing mirror complete
>>
>>   qapi/block-core.json          |  29 +++++++++-
>>   block/blkdebug.c              | 106 +++++++++++++++++++++++++++++++++-
>>   block/mirror.c                |  13 +++--
>>   tests/qemu-iotests/041        |  44 ++++++++++++++
>>   tests/qemu-iotests/041.out    |   4 +-
>>   tests/qemu-iotests/iotests.py |  18 ++++--
>>   6 files changed, 200 insertions(+), 14 deletions(-)
>>
> 
> 



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

Re: [Qemu-devel] [PATCH 2/4] blkdebug: Allow taking/unsharing permissions

[Max Reitz]

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

On 18.09.19 18:01, Vladimir Sementsov-Ogievskiy wrote:
> 12.09.2019 16:56, 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>
>> ---
>>   qapi/block-core.json |  29 +++++++++++-
>>   block/blkdebug.c     | 106 ++++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 133 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e6edd641f1..336043e02c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3347,6 +3347,21 @@
>>               '*state': 'int',
>>               'new_state': 'int' } }
>>   
>> +##
>> +# @BlockdevPermission:
>> +#
>> +# Permissions that an edge in the block graph can take or share.
>> +#
>> +# Since: 4.2
>> +##
>> +{ 'enum': 'BlockdevPermission',
>> +  'data': [
>> +      'consistent-read',
>> +      'write',
>> +      'write-unchanged',
>> +      'resize',
>> +      'graph-mod' ] }
> 
> :)
> 
> BlockPermission is already here since 4.0 and has exactly same content. (And better documented)

Oops.  Very good, then, thanks. :-)

[...]

>> @@ -344,6 +349,84 @@ 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;
>> +    QList *perm_list;
>> +    const QListEntry *perm;
>> +
>> +    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;
>> +    }
>> +
>> +    perm_list = qobject_to(QList, crumpled_subqdict);
>> +    if (!perm_list) {
>> +        /* Omit the trailing . from the prefix */
>> +        error_setg(errp, "%.*s expects a list",
>> +                   (int)(strlen(prefix) - 1), prefix);
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    for (perm = qlist_first(perm_list); perm; perm = qlist_next(perm)) {
>> +        const char *perm_name;
>> +        BlockdevPermission perm_bit;
>> +
>> +        perm_name = qstring_get_try_str(qobject_to(QString, perm->value));
>> +        if (!perm_name) {
>> +            /* Omit the trailing . from the prefix */
>> +            error_setg(errp, "%.*s expects a list of enum strings",
>> +                       (int)(strlen(prefix) - 1), prefix);
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +
>> +        perm_bit = qapi_enum_parse(&BlockdevPermission_lookup, perm_name,
>> +                                   BLOCKDEV_PERMISSION__MAX, errp);
>> +        if (perm_bit == BLOCKDEV_PERMISSION__MAX) {
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +
>> +        *dest |= UINT64_C(1) << perm_bit;
>> +    }
>> +
>> +out:
>> +    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;
>> +}
> 
> 
> It's a pity that being described in json, these new parameters still not parsed automatically..

That would require changing the whole bdrv_open() infrastructure to use
QAPI types.

>> +
>>   static QemuOptsList runtime_opts = {
> 
> and that we have to keep these runtime_opts everywhere, which duplicates json definitions..

Well, it duplicates some json definitions.  I don’t add the new
parameters added in this patch to the list, because there is no point in
supporting them outside of blockdev-add.

Max


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

Re: [Qemu-devel] [PATCH 0/4] mirror: Do not dereference invalid pointers

[Vladimir Sementsov-Ogievskiy]

19.09.2019 19:45, Max Reitz wrote:
> On 18.09.19 17:38, Vladimir Sementsov-Ogievskiy wrote:
>> 12.09.2019 16:56, Max Reitz wrote:
>>> Hi,
>>>
>>> The fix (patch 1) is pretty straightforward; patch 2 (which I need for
>>> the test) may not be.
>>>
>>> The biggest problem with patch 2 is that you can use it to uncover where
>>> our permission handling is broken.  For example, devising the test case
>>> (patch 4) was very difficult because I kept running into the
>>> &error_abort that mirror_exit_common() passes when dropping the
>>> mirror_top_bs.
>>>
>>> The problem is that mirror_top_bs does not take the same permissions
>>> that its parent takes.  Ergo using &error_abort when dropping it is
>>> wrong: The parent may require more permissions that mirror_top_bs did,
>>> and so dropping mirror_top_bs may fail.
>>>
>>> Now what’s really bad is that this cannot be fixed with our current
>>> permission system.  mirror_top_bs was introduced precisely so it does
>>> not take CONSISTENT_READ, but can still allow parents to take it (for
>>> active commits).  But what if there is actually something besides the
>>> mirror job that unshares CONSISTENT_READ?
>>>
>>>
>>> Imagine this:
>>>
>>>         mirror target BB       mirror source BB
>>>                     |             |
>>>                     v             v
>>> mirror_top_bs -> top -> mid -> base
>>>                     ^
>>>                     |
>>>                other_parent
>>>
>>> The source BB unshares CONSISTENT_READ on the base.  mirror_top_bs
>>> ensures that its parents can read from top even though top itself cannot
>>> allow CONSISTENT_READ to be taken.  So far so good.
>>>
>>> But what if other_parent also unshares CONSISTENT_READ?  Then,
>>> mirror_top_bs has no business allowing its parents to take it.
>>>
>>> No idea how to fix that.  (I suppose mirror_top_bs would need some way
>>> to verify that there is no other party that has unshared CONSISTENT_READ
>>> but its associated source BB.
>>
>> May be we need grouped permissions?
>>
>> Some way to define group of children, which may unshare read permission
>> for other children (out of the group), but still children in group may
>> have read permission?
> 
> Hm, is that different from my idea below where one of mirror_top's
> children unshares the read permission, and another is allowed to take it
> still?

I just tried to imagine something generic

> 
> (The problem is always that if some BDS has a parent that unshares this
> permission, this condition propagates upwards through its other parents,
> and we need to keep track of who unshared it in the first place.)
> 
>> But it don't work here as we are saying about children on different
>> nodes.. And propagated through backing chain permissions..
> 
> Yep.
> 
>>>   In the future, we want the source BB to
>>> go away and instead have the source be an immediate BdrvChild of
>>> mirror_top_bs.  Maybe we can then build something into the block layer
>>> so that a node can only restore CONSISTENT_READ when it was that node
>>> that broke it?)
>>>
>>>
>>> Anyway.  You can see something arising from this problem simply by
>>> unsharing CONSISTENT_READ on the target node.  (Just drop the src-perm
>>> node from the test I add in patch 4.)  Replacing the source with the
>>> target will then work fine (because mirror_top_bs doesn’t care about
>>> CONSISTENT_READ being removed), but then you cannot drop mirror_top_bs –
>>> because its parent does want CONSISTENT_READ.  Thus, the &error_abort
>>> aborts.
>>>
>>>
>>> While this is a more special case, I have no idea how to fix this one
>>> either.
>>>
>>>
>>> Soo...  This series just fixes one thing, and leaves another unfixed
>>> because I have no idea how to fix it.  Worse, it adds parameters to
>>> blkdebug to actually see the problem.  Do we want to let blkdebug be
>>> able to crash qemu (because of a bug in qemu)?
>>>
>>
>> blkdebug is for debugging and not used by end users like libvirt, yes?
> 
> Correct.
> 
> 
>>>
>>> Max Reitz (4):
>>>     mirror: Do not dereference invalid pointers
>>>     blkdebug: Allow taking/unsharing permissions
>>>     iotests: Add @error to wait_until_completed
>>>     iotests: Add test for failing mirror complete
>>>
>>>    qapi/block-core.json          |  29 +++++++++-
>>>    block/blkdebug.c              | 106 +++++++++++++++++++++++++++++++++-
>>>    block/mirror.c                |  13 +++--
>>>    tests/qemu-iotests/041        |  44 ++++++++++++++
>>>    tests/qemu-iotests/041.out    |   4 +-
>>>    tests/qemu-iotests/iotests.py |  18 ++++--
>>>    6 files changed, 200 insertions(+), 14 deletions(-)
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete

[Max Reitz]

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

On 18.09.19 18:30, Vladimir Sementsov-Ogievskiy wrote:
> 12.09.2019 16:56, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@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..84bc6d6581 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 = list(map(lambda image: image['node-name'], nodes))
> 
> using list comprehension is a bit more pythonic:
> nodes = [node['node-name'] for node in nodes]

OK.  I can never remember, so I rarely bother using list/dict
comprehensions and instead use what I would do in any other language.
(Hence the “Sadly” from John.)

(And then wait for some kind reviewer to tell me how to do it right. :-))

Max


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

Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete

[Max Reitz]

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

On 18.09.19 20:46, John Snow wrote:
> 
> 
> On 9/12/19 9:56 AM, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@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..84bc6d6581 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 = list(map(lambda image: image['node-name'], nodes))
> 
> Sadly, the list comprehension is a good suggestion. Squash it in if
> you'd like.

You know I don’t, but I’ll do it anyway.

>> +
>> +        for expect in ['src', 'src-perm', 'dest', 'dest-perm']:
>> +            self.assertTrue(expect in nodes, '%s disappeared' % expect)
> 
> I could be a real weenie and say "why not use a tuple here?"

OK.

> but, I'll only pretend I didn't say that instead of couching it in a
> self-deprecating wrapper to the same end effect.
> 
> (I'm a weenie.)
> 
>> +        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
>>
> 
> Or don't do anything, because I'm just being obnoxious, and I owe you
> one for giving you bad advice last round.

Come on, I have better (more selfish) reasons.

For the list comprehension: I want to introduce as many instances of
map/filter as I can so using those functions becomes normal.

For the tuple: This is a test, nobody cares whether it uses 60 bytes
more memory or is 10 µs slower or lets something be mutable when it is
actually never changed.  As such, I like to use the most general
solution which is simply a list.

Max


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

Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete

[John Snow]

On 9/19/19 12:58 PM, Max Reitz wrote:
> On 18.09.19 20:46, John Snow wrote:
>>
>>
>> On 9/12/19 9:56 AM, Max Reitz wrote:
>>> Signed-off-by: Max Reitz <mreitz@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..84bc6d6581 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 = list(map(lambda image: image['node-name'], nodes))
>>
>> Sadly, the list comprehension is a good suggestion. Squash it in if
>> you'd like.
> 
> You know I don’t, but I’ll do it anyway.
> 

Don't you dare make me feel bad by re-spinning, though.

>>> +
>>> +        for expect in ['src', 'src-perm', 'dest', 'dest-perm']:
>>> +            self.assertTrue(expect in nodes, '%s disappeared' % expect)
>>
>> I could be a real weenie and say "why not use a tuple here?"
> 
> OK.
> 
>> but, I'll only pretend I didn't say that instead of couching it in a
>> self-deprecating wrapper to the same end effect.
>>
>> (I'm a weenie.)
>>
>>> +        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
>>>
>>
>> Or don't do anything, because I'm just being obnoxious, and I owe you
>> one for giving you bad advice last round.
> 
> Come on, I have better (more selfish) reasons.
> 
> For the list comprehension: I want to introduce as many instances of
> map/filter as I can so using those functions becomes normal.
> 

They have their uses! But also they're usually just simply longer and
aren't worth using where list comprehensions do.

> For the tuple: This is a test, nobody cares whether it uses 60 bytes
> more memory or is 10 µs slower or lets something be mutable when it is
> actually never changed.  As such, I like to use the most general
> solution which is simply a list.
> 

I did say I was being a weenie. You really can take the reviewed-by!

Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete

[Max Reitz]

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

On 19.09.19 19:02, John Snow wrote:
> 
> 
> On 9/19/19 12:58 PM, Max Reitz wrote:
>> On 18.09.19 20:46, John Snow wrote:
>>>
>>>
>>> On 9/12/19 9:56 AM, Max Reitz wrote:
>>>> Signed-off-by: Max Reitz <mreitz@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..84bc6d6581 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 = list(map(lambda image: image['node-name'], nodes))
>>>
>>> Sadly, the list comprehension is a good suggestion. Squash it in if
>>> you'd like.
>>
>> You know I don’t, but I’ll do it anyway.
>>
> 
> Don't you dare make me feel bad by re-spinning, though.

I have to respin anyway. ;-)

>>>> +
>>>> +        for expect in ['src', 'src-perm', 'dest', 'dest-perm']:
>>>> +            self.assertTrue(expect in nodes, '%s disappeared' % expect)
>>>
>>> I could be a real weenie and say "why not use a tuple here?"
>>
>> OK.
>>
>>> but, I'll only pretend I didn't say that instead of couching it in a
>>> self-deprecating wrapper to the same end effect.
>>>
>>> (I'm a weenie.)
>>>
>>>> +        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
>>>>
>>>
>>> Or don't do anything, because I'm just being obnoxious, and I owe you
>>> one for giving you bad advice last round.
>>
>> Come on, I have better (more selfish) reasons.
>>
>> For the list comprehension: I want to introduce as many instances of
>> map/filter as I can so using those functions becomes normal.
>>
> 
> They have their uses! But also they're usually just simply longer and
> aren't worth using where list comprehensions do.

The point is that they’re special language constructs whereas map/filter
are available in basically any decent language.

>> For the tuple: This is a test, nobody cares whether it uses 60 bytes
>> more memory or is 10 µs slower or lets something be mutable when it is
>> actually never changed.  As such, I like to use the most general
>> solution which is simply a list.
>>
> 
> I did say I was being a weenie. You really can take the reviewed-by!

I’m just saying those would be my reasons if I rejected the changes
suggested by you both.  I don’t reject them, though. :-)

Max


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