[v2,04/33] block: Add BdrvChildRole to BdrvChild

Commit Message

Max Reitz Feb. 4, 2020, 5:08 p.m. UTC

For now, it is always set to 0.  Later patches in this series will
ensure that all callers pass an appropriate combination of flags.

Signed-off-by: Max Reitz <[e-mail]>
---
 block.c                     | 11 ++++++++---
 block/backup-top.c          |  3 ++-
 block/blkdebug.c            |  2 +-
 block/blklogwrites.c        |  6 +++---
 block/blkreplay.c           |  2 +-
 block/blkverify.c           |  4 ++--
 block/block-backend.c       |  4 ++--
 block/bochs.c               |  2 +-
 block/cloop.c               |  2 +-
 block/copy-on-read.c        |  2 +-
 block/crypto.c              |  2 +-
 block/dmg.c                 |  2 +-
 block/filter-compress.c     |  2 +-
 block/parallels.c           |  2 +-
 block/qcow.c                |  2 +-
 block/qcow2.c               |  6 +++---
 block/qed.c                 |  2 +-
 block/quorum.c              |  4 ++--
 block/raw-format.c          |  2 +-
 block/replication.c         |  2 +-
 block/throttle.c            |  2 +-
 block/vdi.c                 |  2 +-
 block/vhdx.c                |  2 +-
 block/vmdk.c                |  4 ++--
 block/vpc.c                 |  2 +-
 block/vvfat.c               |  2 +-
 blockjob.c                  |  5 +++--
 include/block/block.h       |  2 ++
 include/block/block_int.h   |  2 ++
 tests/test-bdrv-drain.c     | 20 +++++++++++---------
 tests/test-bdrv-graph-mod.c |  4 ++--
 31 files changed, 62 insertions(+), 49 deletions(-)

Comments

Eric Blake Feb. 5, 2020, 3:33 p.m. UTC | #1

On 2/4/20 11:08 AM, Max Reitz wrote:
> For now, it is always set to 0.  Later patches in this series will
> ensure that all callers pass an appropriate combination of flags.

Sneaky - this re-adds the field you dropped as part of a rename in 2/33. 
  Anyone doing backport had better be aware that they would need this 
whole series, rather than cherry-picking later patches without the 
earlier ones.  But that observation does not affect the patch validity.

> 
> Signed-off-by: Max Reitz <[e-mail]>
> ---

> +++ b/block.c
> @@ -2381,6 +2381,7 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>   BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
>                                     const char *child_name,
>                                     const BdrvChildClass *child_class,
> +                                  BdrvChildRole child_role,

Hmm - C is loose enough to allow the declaration of a parameter as an 
enum type even when its intended usage is to receive a bitwise-or 
derived from that enum but not declared in the enum.  For example, if I 
understand intent correctly, a caller might pass in 0x3 
(BDRV_CHILD_DATA|BDRV_CHILD_METADATA) which does NOT appear in 
BdrvChildRole.  It feels like it might be cleaner to document the 
interface as taking an unsigned int (although then you've lost the 
documentation that the int is derived from BdrvChildRole), than to abuse 
the typesystem to pass values that are not BdrvChildRole through the 
parameter.

Otherwise, this patch is a mechanical addition.
Reviewed-by: Eric Blake <[e-mail]>

Max Reitz Feb. 6, 2020, 10:53 a.m. UTC | #2

On 05.02.20 16:33, Eric Blake wrote:
> On 2/4/20 11:08 AM, Max Reitz wrote:
>> For now, it is always set to 0.  Later patches in this series will
>> ensure that all callers pass an appropriate combination of flags.
> 
> Sneaky - this re-adds the field you dropped as part of a rename in 2/33.
>  Anyone doing backport had better be aware that they would need this
> whole series, rather than cherry-picking later patches without the
> earlier ones.  But that observation does not affect the patch validity.
> 
>>
>> Signed-off-by: Max Reitz <[e-mail]>
>> ---
> 
>> +++ b/block.c
>> @@ -2381,6 +2381,7 @@ static void bdrv_replace_child(BdrvChild *child,
>> BlockDriverState *new_bs)
>>   BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
>>                                     const char *child_name,
>>                                     const BdrvChildClass *child_class,
>> +                                  BdrvChildRole child_role,
> 
> Hmm - C is loose enough to allow the declaration of a parameter as an
> enum type even when its intended usage is to receive a bitwise-or
> derived from that enum but not declared in the enum.  For example, if I
> understand intent correctly, a caller might pass in 0x3
> (BDRV_CHILD_DATA|BDRV_CHILD_METADATA) which does NOT appear in
> BdrvChildRole.  It feels like it might be cleaner to document the
> interface as taking an unsigned int (although then you've lost the
> documentation that the int is derived from BdrvChildRole), than to abuse
> the typesystem to pass values that are not BdrvChildRole through the
> parameter.

I don’t necessarily disagree, but we have pre-existing examples of such
abuse, like BdrvRequestFlags.

The advantage of using BdrvChildRole as a type here is to show that we
expect values from that enum.  I personally prefer that.

I mean, we could do something else entirely and name the enum
“BdrvChildRoleBits” and add a “typedef unsigned int BdrvChildRole;”.  I
don’t think we’ve ever done that before but maybe it’d be the cleanest way?

Max

Eric Blake Feb. 11, 2020, 3:37 p.m. UTC | #3

On 2/6/20 4:53 AM, Max Reitz wrote:

>>> +++ b/block.c
>>> @@ -2381,6 +2381,7 @@ static void bdrv_replace_child(BdrvChild *child,
>>> BlockDriverState *new_bs)
>>>    BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
>>>                                      const char *child_name,
>>>                                      const BdrvChildClass *child_class,
>>> +                                  BdrvChildRole child_role,
>>
>> Hmm - C is loose enough to allow the declaration of a parameter as an
>> enum type even when its intended usage is to receive a bitwise-or
>> derived from that enum but not declared in the enum.  For example, if I
>> understand intent correctly, a caller might pass in 0x3
>> (BDRV_CHILD_DATA|BDRV_CHILD_METADATA) which does NOT appear in
>> BdrvChildRole.  It feels like it might be cleaner to document the
>> interface as taking an unsigned int (although then you've lost the
>> documentation that the int is derived from BdrvChildRole), than to abuse
>> the typesystem to pass values that are not BdrvChildRole through the
>> parameter.
> 
> I don’t necessarily disagree, but we have pre-existing examples of such
> abuse, like BdrvRequestFlags.
> 
> The advantage of using BdrvChildRole as a type here is to show that we
> expect values from that enum.  I personally prefer that.

Yeah, the self-documenting aspect is nice.

> 
> I mean, we could do something else entirely and name the enum
> “BdrvChildRoleBits” and add a “typedef unsigned int BdrvChildRole;”.  I
> don’t think we’ve ever done that before but maybe it’d be the cleanest way?

You're right that we haven't done it, but it is also the slickest 
solution that preserves documentation intent.  In C, such a typedef 
serves only as documentation (and the compiler doesn't enforce it, 
compared to what a strongly-typed language would do), but I do like the 
idea.

Patch

diff --git a/block.c b/block.c
index 5835161172..c576377650 100644
--- a/block.c
+++ b/block.c
@@ -2381,6 +2381,7 @@  static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
 BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   const char *child_name,
                                   const BdrvChildClass *child_class,
+                                  BdrvChildRole child_role,
                                   AioContext *ctx,
                                   uint64_t perm, uint64_t shared_perm,
                                   void *opaque, Error **errp)
@@ -2402,6 +2403,7 @@  BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
         .bs             = NULL,
         .name           = g_strdup(child_name),
         .klass          = child_class,
+        .role           = child_role,
         .perm           = perm,
         .shared_perm    = shared_perm,
         .opaque         = opaque,
@@ -2453,6 +2455,7 @@  BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                              BlockDriverState *child_bs,
                              const char *child_name,
                              const BdrvChildClass *child_class,
+                             BdrvChildRole child_role,
                              Error **errp)
 {
     BdrvChild *child;
@@ -2465,7 +2468,7 @@  BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                     perm, shared_perm, &perm, &shared_perm);
 
     child = bdrv_root_attach_child(child_bs, child_name, child_class,
-                                   bdrv_get_aio_context(parent_bs),
+                                   child_role, bdrv_get_aio_context(parent_bs),
                                    perm, shared_perm, parent_bs, errp);
     if (child == NULL) {
         return NULL;
@@ -2586,7 +2589,7 @@  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
     }
 
     bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,
-                                    errp);
+                                    0, errp);
     /* If backing_hd was already part of bs's backing chain, and
      * inherits_from pointed recursively to bs then let's update it to
      * point directly to bs (else it will become NULL). */
@@ -2777,6 +2780,7 @@  BdrvChild *bdrv_open_child(const char *filename,
                            QDict *options, const char *bdref_key,
                            BlockDriverState *parent,
                            const BdrvChildClass *child_class,
+                           BdrvChildRole child_role,
                            bool allow_none, Error **errp)
 {
     BlockDriverState *bs;
@@ -2787,7 +2791,8 @@  BdrvChild *bdrv_open_child(const char *filename,
         return NULL;
     }
 
-    return bdrv_attach_child(parent, bs, bdref_key, child_class, errp);
+    return bdrv_attach_child(parent, bs, bdref_key, child_class, child_role,
+                             errp);
 }
 
 /*
diff --git a/block/backup-top.c b/block/backup-top.c
index 21a75f1422..3c812358ce 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -200,7 +200,8 @@  BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
     state = top->opaque;
 
     bdrv_ref(target);
-    state->target = bdrv_attach_child(top, target, "target", &child_file, errp);
+    state->target = bdrv_attach_child(top, target, "target", &child_file, 0,
+                                      errp);
     if (!state->target) {
         bdrv_unref(target);
         bdrv_unref(top);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index f369d54ee4..c91e78d5c8 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -497,7 +497,7 @@  static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* Open the image file */
     bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
-                               bs, &child_file, false, &local_err);
+                               bs, &child_file, 0, false, &local_err);
     if (local_err) {
         ret = -EINVAL;
         error_propagate(errp, local_err);
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index f3b3259d8d..739db6dcf6 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -157,7 +157,7 @@  static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Open the file */
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0, false,
                                &local_err);
     if (local_err) {
         ret = -EINVAL;
@@ -166,8 +166,8 @@  static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Open the log file */
-    s->log_file = bdrv_open_child(NULL, options, "log", bs, &child_file, false,
-                                  &local_err);
+    s->log_file = bdrv_open_child(NULL, options, "log", bs, &child_file, 0,
+                                  false, &local_err);
     if (local_err) {
         ret = -EINVAL;
         error_propagate(errp, local_err);
diff --git a/block/blkreplay.c b/block/blkreplay.c
index c96ac8f4bc..f97493f45a 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -28,7 +28,7 @@  static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* Open the image file */
     bs->file = bdrv_open_child(NULL, options, "image",
-                               bs, &child_file, false, &local_err);
+                               bs, &child_file, 0, false, &local_err);
     if (local_err) {
         ret = -EINVAL;
         error_propagate(errp, local_err);
diff --git a/block/blkverify.c b/block/blkverify.c
index ba6b1853ae..ba4f6d7b7c 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -125,7 +125,7 @@  static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* Open the raw file */
     bs->file = bdrv_open_child(qemu_opt_get(opts, "x-raw"), options, "raw",
-                               bs, &child_file, false, &local_err);
+                               bs, &child_file, 0, false, &local_err);
     if (local_err) {
         ret = -EINVAL;
         error_propagate(errp, local_err);
@@ -134,7 +134,7 @@  static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* Open the test file */
     s->test_file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options,
-                                   "test", bs, &child_format, false,
+                                   "test", bs, &child_format, 0, false,
                                    &local_err);
     if (local_err) {
         ret = -EINVAL;
diff --git a/block/block-backend.c b/block/block-backend.c
index 3d7e96e0d4..4b79b2abb1 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -400,7 +400,7 @@  BlockBackend *blk_new_open(const char *filename, const char *reference,
         return NULL;
     }
 
-    blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk->ctx,
+    blk->root = bdrv_root_attach_child(bs, "root", &child_root, 0, blk->ctx,
                                        perm, BLK_PERM_ALL, blk, errp);
     if (!blk->root) {
         blk_unref(blk);
@@ -811,7 +811,7 @@  int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
 {
     ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
     bdrv_ref(bs);
-    blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk->ctx,
+    blk->root = bdrv_root_attach_child(bs, "root", &child_root, 0, blk->ctx,
                                        blk->perm, blk->shared_perm, blk, errp);
     if (blk->root == NULL) {
         return -EPERM;
diff --git a/block/bochs.c b/block/bochs.c
index e7bbeaa1c4..b013e73063 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -110,7 +110,7 @@  static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         return ret;
     }
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/cloop.c b/block/cloop.c
index f90f1a4b4c..3ed9fa63cc 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -71,7 +71,7 @@  static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
         return ret;
     }
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 7504ca6ffc..a2c4e6dc58 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -28,7 +28,7 @@ 
 static int cor_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0, false,
                                errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/crypto.c b/block/crypto.c
index b81f673a51..4da74a7737 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -200,7 +200,7 @@  static int block_crypto_open_generic(QCryptoBlockFormat format,
     unsigned int cflags = 0;
     QDict *cryptoopts = NULL;
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/dmg.c b/block/dmg.c
index ef3c6e771d..af8188638c 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -439,7 +439,7 @@  static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         return ret;
     }
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/filter-compress.c b/block/filter-compress.c
index 82c315b298..4dc5f9fb8c 100644
--- a/block/filter-compress.c
+++ b/block/filter-compress.c
@@ -30,7 +30,7 @@ 
 static int compress_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0, false,
                                errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/parallels.c b/block/parallels.c
index 30eda982bd..3d5b3b7c63 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -728,7 +728,7 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     Error *local_err = NULL;
     char *buf;
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/qcow.c b/block/qcow.c
index 0e4f09934c..2bf8e8eb36 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -130,7 +130,7 @@  static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     qdict_extract_subqdict(options, &encryptopts, "encrypt.");
     encryptfmt = qdict_get_try_str(encryptopts, "format");
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         ret = -EINVAL;
diff --git a/block/qcow2.c b/block/qcow2.c
index 8e057556eb..94e394d608 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1535,7 +1535,7 @@  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
 
     /* Open external data file */
     s->data_file = bdrv_open_child(NULL, options, "data-file", bs, &child_file,
-                                   true, &local_err);
+                                   0, true, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1545,7 +1545,7 @@  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
         if (!s->data_file && s->image_data_file) {
             s->data_file = bdrv_open_child(s->image_data_file, options,
-                                           "data-file", bs, &child_file,
+                                           "data-file", bs, &child_file, 0,
                                            false, errp);
             if (!s->data_file) {
                 ret = -EINVAL;
@@ -1805,7 +1805,7 @@  static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         .ret = -EINPROGRESS
     };
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/qed.c b/block/qed.c
index eb6139acbc..d15ba11959 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -547,7 +547,7 @@  static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
         .ret = -EINPROGRESS
     };
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/quorum.c b/block/quorum.c
index f354d9bf69..e53d02bbcd 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -992,7 +992,7 @@  static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
         assert(ret < 32);
 
         s->children[i].child = bdrv_open_child(NULL, options, indexstr, bs,
-                                               &child_format, false,
+                                               &child_format, 0, false,
                                                &local_err);
         if (local_err) {
             ret = -EINVAL;
@@ -1069,7 +1069,7 @@  static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
     /* We can safely add the child now */
     bdrv_ref(child_bs);
 
-    child = bdrv_attach_child(bs, child_bs, indexstr, &child_format, errp);
+    child = bdrv_attach_child(bs, child_bs, indexstr, &child_format, 0, errp);
     if (child == NULL) {
         s->next_child_index--;
         goto out;
diff --git a/block/raw-format.c b/block/raw-format.c
index 2429ff0d23..b600dae209 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -431,7 +431,7 @@  static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVRawState *s = bs->opaque;
     int ret;
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/replication.c b/block/replication.c
index e268a6e5c5..c05df1bc16 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -90,7 +90,7 @@  static int replication_open(BlockDriverState *bs, QDict *options,
     const char *mode;
     const char *top_id;
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/throttle.c b/block/throttle.c
index 71f4bb0ad1..2dea913be7 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -82,7 +82,7 @@  static int throttle_open(BlockDriverState *bs, QDict *options,
     int ret;
 
     bs->file = bdrv_open_child(NULL, options, "file", bs,
-                               &child_file, false, errp);
+                               &child_file, 0, false, errp);
     if (!bs->file) {
         return -EINVAL;
     }
diff --git a/block/vdi.c b/block/vdi.c
index 7e87b205b5..987e2ed592 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -378,7 +378,7 @@  static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
     Error *local_err = NULL;
     QemuUUID uuid_link, uuid_parent;
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/vhdx.c b/block/vhdx.c
index d3c1619026..be23290ac8 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -997,7 +997,7 @@  static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
     uint64_t signature;
     Error *local_err = NULL;
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/vmdk.c b/block/vmdk.c
index 631fcd15ab..84f99d881f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1152,7 +1152,7 @@  static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         assert(ret < 32);
 
         extent_file = bdrv_open_child(extent_path, options, extent_opt_prefix,
-                                      bs, &child_file, false, &local_err);
+                                      bs, &child_file, 0, false, &local_err);
         g_free(extent_path);
         if (local_err) {
             error_propagate(errp, local_err);
@@ -1257,7 +1257,7 @@  static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
     uint32_t magic;
     Error *local_err = NULL;
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/vpc.c b/block/vpc.c
index 7cf0f87a16..5378e32d1d 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -228,7 +228,7 @@  static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
     int64_t bs_size;
 
-    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, 0,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
diff --git a/block/vvfat.c b/block/vvfat.c
index 70f6985fbb..0c0b224947 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3192,7 +3192,7 @@  static int enable_write_target(BlockDriverState *bs, Error **errp)
     options = qdict_new();
     qdict_put_str(options, "write-target.driver", "qcow");
     s->qcow = bdrv_open_child(s->qcow_filename, options, "write-target", bs,
-                              &child_vvfat_qcow, false, errp);
+                              &child_vvfat_qcow, 0, false, errp);
     qobject_unref(options);
     if (!s->qcow) {
         ret = -EINVAL;
diff --git a/blockjob.c b/blockjob.c
index 79f303822b..533ef0af30 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -217,8 +217,9 @@  int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
     if (job->job.aio_context != qemu_get_aio_context()) {
         aio_context_release(job->job.aio_context);
     }
-    c = bdrv_root_attach_child(bs, name, &child_job, job->job.aio_context,
-                               perm, shared_perm, job, errp);
+    c = bdrv_root_attach_child(bs, name, &child_job, 0,
+                               job->job.aio_context, perm, shared_perm, job,
+                               errp);
     if (job->job.aio_context != qemu_get_aio_context()) {
         aio_context_acquire(job->job.aio_context);
     }
diff --git a/include/block/block.h b/include/block/block.h
index 0f7e8caa5b..7ad819f045 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -333,6 +333,7 @@  BdrvChild *bdrv_open_child(const char *filename,
                            QDict *options, const char *bdref_key,
                            BlockDriverState* parent,
                            const BdrvChildClass *child_class,
+                           BdrvChildRole child_role,
                            bool allow_none, Error **errp);
 BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
@@ -576,6 +577,7 @@  BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                              BlockDriverState *child_bs,
                              const char *child_name,
                              const BdrvChildClass *child_class,
+                             BdrvChildRole child_role,
                              Error **errp);
 
 bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index bbd933d782..4817c2d38c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -746,6 +746,7 @@  struct BdrvChild {
     BlockDriverState *bs;
     char *name;
     const BdrvChildClass *klass;
+    BdrvChildRole role;
     void *opaque;
 
     /**
@@ -1228,6 +1229,7 @@  void hmp_drive_add_node(Monitor *mon, const char *optstr);
 BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   const char *child_name,
                                   const BdrvChildClass *child_class,
+                                  BdrvChildRole child_role,
                                   AioContext *ctx,
                                   uint64_t perm, uint64_t shared_perm,
                                   void *opaque, Error **errp);
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 9d683a6c11..c03705ea37 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -1202,7 +1202,7 @@  static void do_test_delete_by_drain(bool detach_instead_of_delete,
 
     null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
                         &error_abort);
-    bdrv_attach_child(bs, null_bs, "null-child", &child_file, &error_abort);
+    bdrv_attach_child(bs, null_bs, "null-child", &child_file, 0, &error_abort);
 
     /* This child will be the one to pass to requests through to, and
      * it will stall until a drain occurs */
@@ -1211,13 +1211,13 @@  static void do_test_delete_by_drain(bool detach_instead_of_delete,
     child_bs->total_sectors = 65536 >> BDRV_SECTOR_BITS;
     /* Takes our reference to child_bs */
     tts->wait_child = bdrv_attach_child(bs, child_bs, "wait-child", &child_file,
-                                        &error_abort);
+                                        0, &error_abort);
 
     /* This child is just there to be deleted
      * (for detach_instead_of_delete == true) */
     null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
                         &error_abort);
-    bdrv_attach_child(bs, null_bs, "null-child", &child_file, &error_abort);
+    bdrv_attach_child(bs, null_bs, "null-child", &child_file, 0, &error_abort);
 
     blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
     blk_insert_bs(blk, bs, &error_abort);
@@ -1314,7 +1314,7 @@  static void detach_indirect_bh(void *opaque)
 
     bdrv_ref(data->c);
     data->child_c = bdrv_attach_child(data->parent_b, data->c, "PB-C",
-                                      &child_file, &error_abort);
+                                      &child_file, 0, &error_abort);
 }
 
 static void detach_by_parent_aio_cb(void *opaque, int ret)
@@ -1396,13 +1396,15 @@  static void test_detach_indirect(bool by_parent_cb)
     /* Set child relationships */
     bdrv_ref(b);
     bdrv_ref(a);
-    child_b = bdrv_attach_child(parent_b, b, "PB-B", &child_file, &error_abort);
-    child_a = bdrv_attach_child(parent_b, a, "PB-A", &child_backing, &error_abort);
+    child_b = bdrv_attach_child(parent_b, b, "PB-B", &child_file, 0,
+                                &error_abort);
+    child_a = bdrv_attach_child(parent_b, a, "PB-A", &child_backing, 0,
+                                &error_abort);
 
     bdrv_ref(a);
     bdrv_attach_child(parent_a, a, "PA-A",
                       by_parent_cb ? &child_file : &detach_by_driver_cb_class,
-                      &error_abort);
+                      0, &error_abort);
 
     g_assert_cmpint(parent_a->refcnt, ==, 1);
     g_assert_cmpint(parent_b->refcnt, ==, 1);
@@ -1813,7 +1815,7 @@  static void test_drop_intermediate_poll(void)
             /* Takes the reference to chain[i - 1] */
             chain[i]->backing = bdrv_attach_child(chain[i], chain[i - 1],
                                                   "chain", &chain_child_class,
-                                                  &error_abort);
+                                                  0, &error_abort);
         }
     }
 
@@ -2031,7 +2033,7 @@  static void do_test_replace_child_mid_drain(int old_drain_count,
 
     bdrv_ref(old_child_bs);
     parent_bs->backing = bdrv_attach_child(parent_bs, old_child_bs, "child",
-                                           &child_backing, &error_abort);
+                                           &child_backing, 0, &error_abort);
 
     for (i = 0; i < old_drain_count; i++) {
         bdrv_drained_begin(old_child_bs);
diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c
index fef42cb294..8b8c186c9f 100644
--- a/tests/test-bdrv-graph-mod.c
+++ b/tests/test-bdrv-graph-mod.c
@@ -111,7 +111,7 @@  static void test_update_perm_tree(void)
 
     blk_insert_bs(root, bs, &error_abort);
 
-    bdrv_attach_child(filter, bs, "child", &child_file, &error_abort);
+    bdrv_attach_child(filter, bs, "child", &child_file, 0, &error_abort);
 
     bdrv_append(filter, bs, &local_err);
 
@@ -177,7 +177,7 @@  static void test_should_update_child(void)
     bdrv_set_backing_hd(target, bs, &error_abort);
 
     g_assert(target->backing->bs == bs);
-    bdrv_attach_child(filter, target, "target", &child_file, &error_abort);
+    bdrv_attach_child(filter, target, "target", &child_file, 0, &error_abort);
     bdrv_append(filter, bs, &error_abort);
     g_assert(target->backing->bs == bs);