[PATCH v2 0/4] mirror: Do not dereference invalid pointers

[PATCH v2 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)?


v2:
- Patch 2: Dropped BlockdevPermission, we have BlockPermission already
- Patch 4:
  - Use list comprehension instead of map() + lambda
    (Though I notice that over time my aversion of list comprehensions
    only grows)

  - Use a tuple instead of a list for a collection of values that isn’t
    modified

git-backport-diff against v1:

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/4:[----] [--] 'mirror: Do not dereference invalid pointers'
002/4:[0027] [FC] 'blkdebug: Allow taking/unsharing permissions'
003/4:[----] [--] 'iotests: Add @error to wait_until_completed'
004/4:[0004] [FC] 'iotests: Add test for failing mirror complete'


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          |  14 ++++-
 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, 185 insertions(+), 14 deletions(-)

-- 
2.21.0

[PATCH v2 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>
Reviewed-by: John Snow <jsnow@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);
     }
-- 
2.21.0

[PATCH v2 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 |  14 +++++-
 block/blkdebug.c     | 106 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index b5cd00c361..572c5756f1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3394,6 +3394,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',
@@ -3403,7 +3413,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:
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5ae96c52b0..f3c1e4ad7b 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;
+        BlockPermission 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(&BlockPermission_lookup, perm_name,
+                                   BLOCK_PERMISSION__MAX, errp);
+        if (perm_bit == BLOCK_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

[PATCH v2 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>
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 3a8f378f90..5d44183f8f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -770,15 +770,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':
@@ -796,7 +801,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)
@@ -804,7 +810,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

[PATCH v2 4/4] 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.21.0

Re: [PATCH v2 2/4] blkdebug: Allow taking/unsharing permissions

[Vladimir Sementsov-Ogievskiy]

23.09.2019 14:57, 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 |  14 +++++-
>   block/blkdebug.c     | 106 ++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 118 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b5cd00c361..572c5756f1 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3394,6 +3394,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',
> @@ -3403,7 +3413,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:
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 5ae96c52b0..f3c1e4ad7b 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;
> +        BlockPermission 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(&BlockPermission_lookup, perm_name,
> +                                   BLOCK_PERMISSION__MAX, errp);
> +        if (perm_bit == BLOCK_PERMISSION__MAX) {
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        *dest |= UINT64_C(1) << perm_bit;
> +    }


I still think that parsing it all by hand is a bad idea. We already have
functions which does it, why to opencode? The following works for me:

static int blkdebug_parse_perm_list(uint64_t *dest, QDict *options,
                                     const char *prefix, Error **errp)
{
     Error *local_err = NULL;
     Visitor *v = NULL;
     BlockPermissionList *list = NULL, *perm;
     int ret = 0;
     QDict *subqdict = NULL;
     QObject *crumpled_subqdict = NULL;
     uint64_t perm_map[] = {
         [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,
     };

     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(QOBJECT(crumpled_subqdict));
     visit_type_BlockPermissionList(v, NULL, &list, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
     }

     for (perm = list; perm; perm = perm->next) {
         *dest |= perm_map[perm->value];
     }

     qapi_free_BlockPermissionList(list);

out:
     visit_free(v);
     qobject_unref(subqdict);
     qobject_unref(crumpled_subqdict);
     return ret;
}


Probably, we can do similar thing for the whole blkdebug_open and drop runtime_opts,
but it's for another series..

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

-- 
Best regards,
Vladimir

Re: [PATCH v2 2/4] blkdebug: Allow taking/unsharing permissions

[Max Reitz]

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

On 25.09.19 12:12, Vladimir Sementsov-Ogievskiy wrote:
> 23.09.2019 14:57, 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 |  14 +++++-
>>   block/blkdebug.c     | 106 ++++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 118 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index b5cd00c361..572c5756f1 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3394,6 +3394,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',
>> @@ -3403,7 +3413,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:
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 5ae96c52b0..f3c1e4ad7b 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;
>> +        BlockPermission 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(&BlockPermission_lookup, perm_name,
>> +                                   BLOCK_PERMISSION__MAX, errp);
>> +        if (perm_bit == BLOCK_PERMISSION__MAX) {
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +
>> +        *dest |= UINT64_C(1) << perm_bit;
>> +    }
> 
> 
> I still think that parsing it all by hand is a bad idea. We already have
> functions which does it, why to opencode? The following works for me:

Ah, yes.  I don’t know why I didn’t think of just using a visitor for
the crumpled sub-QDict.  Will do, thanks.

Max


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