[Qemu-devel] [PATCH for-2.4 0/5] block: Fix backing file child when modifying graph

[Qemu-devel] [PATCH for-2.4 0/5] block: Fix backing file child when modifying graph

[Kevin Wolf]

This series is extracted from my work towards removing bdrv_swap(),
which is targeted for 2.5. It contains a fix for dangling pointers when
modifying the BDS graph and its dependencies.

I didn't bother to split patches of which only a part is required, nor
did I remove references to the future bdrv_swap removal (after all, it
will happen, even if the patches will be delayed by the 2.4 freeze).

Specifically, bdrv_open/unref_child() are yet unused in this series; the
respective patches are included because of bdrv_attach/detach_child().

Kevin Wolf (5):
  block: Move bdrv_attach_child() calls up the call chain
  block: Introduce bdrv_open_child()
  block: Introduce bdrv_unref_child()
  block: Reorder cleanups in bdrv_close()
  block: Fix backing file child when modifying graph

 block.c                   | 144 ++++++++++++++++++++++++++++++++--------------
 include/block/block.h     |   7 +++
 include/block/block_int.h |   1 +
 3 files changed, 108 insertions(+), 44 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.4 1/5] block: Move bdrv_attach_child() calls up the call chain

[Kevin Wolf]

Let the callers of bdrv_open_inherit() call bdrv_attach_child(). It
needs to be called in all cases where bdrv_open_inherit() succeeds (i.e.
returns 0) and a child_role is given.

bdrv_attach_child() is moved upwards to avoid a forward declaration.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/block.c b/block.c
index 5e80336..0398bff 100644
--- a/block.c
+++ b/block.c
@@ -1102,6 +1102,19 @@ static int bdrv_fill_options(QDict **options, const char **pfilename,
     return 0;
 }
 
+static void bdrv_attach_child(BlockDriverState *parent_bs,
+                              BlockDriverState *child_bs,
+                              const BdrvChildRole *child_role)
+{
+    BdrvChild *child = g_new(BdrvChild, 1);
+    *child = (BdrvChild) {
+        .bs     = child_bs,
+        .role   = child_role,
+    };
+
+    QLIST_INSERT_HEAD(&parent_bs->children, child, next);
+}
+
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
 {
 
@@ -1202,6 +1215,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
         error_free(local_err);
         goto free_exit;
     }
+
+    bdrv_attach_child(bs, backing_hd, &child_backing);
     bdrv_set_backing_hd(bs, backing_hd);
 
 free_exit:
@@ -1237,6 +1252,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
 
     assert(pbs);
     assert(*pbs == NULL);
+    assert(child_role != NULL);
 
     bdref_key_dot = g_strdup_printf("%s.", bdref_key);
     qdict_extract_subqdict(options, &image_options, bdref_key_dot);
@@ -1257,6 +1273,11 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
 
     ret = bdrv_open_inherit(pbs, filename, reference, image_options, 0,
                             parent, child_role, NULL, errp);
+    if (ret < 0) {
+        goto done;
+    }
+
+    bdrv_attach_child(parent, *pbs, child_role);
 
 done:
     qdict_del(options, bdref_key);
@@ -1328,19 +1349,6 @@ out:
     return ret;
 }
 
-static void bdrv_attach_child(BlockDriverState *parent_bs,
-                              BlockDriverState *child_bs,
-                              const BdrvChildRole *child_role)
-{
-    BdrvChild *child = g_new(BdrvChild, 1);
-    *child = (BdrvChild) {
-        .bs     = child_bs,
-        .role   = child_role,
-    };
-
-    QLIST_INSERT_HEAD(&parent_bs->children, child, next);
-}
-
 /*
  * Opens a disk image (raw, qcow2, vmdk, ...)
  *
@@ -1393,9 +1401,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
             return -ENODEV;
         }
         bdrv_ref(bs);
-        if (child_role) {
-            bdrv_attach_child(parent, bs, child_role);
-        }
         *pbs = bs;
         return 0;
     }
@@ -1540,10 +1545,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
         goto close_and_fail;
     }
 
-    if (child_role) {
-        bdrv_attach_child(parent, bs, child_role);
-    }
-
     QDECREF(options);
     *pbs = bs;
     return 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.4 2/5] block: Introduce bdrv_open_child()

[Kevin Wolf]

It is the same as bdrv_open_image(), except that it doesn't only return
success or failure, but the newly created BdrvChild object for the new
child node.

As the BdrvChild object already contains a BlockDriverState pointer (and
this is supposed to become the only pointer so that bdrv_append() and
friends can just change a single pointer in BdrvChild), the pbs
parameter is removed for bdrv_open_child().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               | 71 +++++++++++++++++++++++++++++++++++++--------------
 include/block/block.h |  6 +++++
 2 files changed, 58 insertions(+), 19 deletions(-)

diff --git a/block.c b/block.c
index 0398bff..029feeb 100644
--- a/block.c
+++ b/block.c
@@ -1102,9 +1102,9 @@ static int bdrv_fill_options(QDict **options, const char **pfilename,
     return 0;
 }
 
-static void bdrv_attach_child(BlockDriverState *parent_bs,
-                              BlockDriverState *child_bs,
-                              const BdrvChildRole *child_role)
+static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+                                    BlockDriverState *child_bs,
+                                    const BdrvChildRole *child_role)
 {
     BdrvChild *child = g_new(BdrvChild, 1);
     *child = (BdrvChild) {
@@ -1113,6 +1113,8 @@ static void bdrv_attach_child(BlockDriverState *parent_bs,
     };
 
     QLIST_INSERT_HEAD(&parent_bs->children, child, next);
+
+    return child;
 }
 
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
@@ -1229,7 +1231,7 @@ free_exit:
  * device's options.
  *
  * If allow_none is true, no image will be opened if filename is false and no
- * BlockdevRef is given. *pbs will remain unchanged and 0 will be returned.
+ * BlockdevRef is given. NULL will be returned, but errp remains unset.
  *
  * bdrev_key specifies the key for the image's BlockdevRef in the options QDict.
  * That QDict has to be flattened; therefore, if the BlockdevRef is a QDict
@@ -1237,21 +1239,20 @@ free_exit:
  * BlockdevRef.
  *
  * The BlockdevRef will be removed from the options QDict.
- *
- * To conform with the behavior of bdrv_open(), *pbs has to be NULL.
  */
-int bdrv_open_image(BlockDriverState **pbs, const char *filename,
-                    QDict *options, const char *bdref_key,
-                    BlockDriverState* parent, const BdrvChildRole *child_role,
-                    bool allow_none, Error **errp)
+BdrvChild *bdrv_open_child(const char *filename,
+                           QDict *options, const char *bdref_key,
+                           BlockDriverState* parent,
+                           const BdrvChildRole *child_role,
+                           bool allow_none, Error **errp)
 {
+    BdrvChild *c = NULL;
+    BlockDriverState *bs;
     QDict *image_options;
     int ret;
     char *bdref_key_dot;
     const char *reference;
 
-    assert(pbs);
-    assert(*pbs == NULL);
     assert(child_role != NULL);
 
     bdref_key_dot = g_strdup_printf("%s.", bdref_key);
@@ -1260,28 +1261,60 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
 
     reference = qdict_get_try_str(options, bdref_key);
     if (!filename && !reference && !qdict_size(image_options)) {
-        if (allow_none) {
-            ret = 0;
-        } else {
+        if (!allow_none) {
             error_setg(errp, "A block device must be specified for \"%s\"",
                        bdref_key);
-            ret = -EINVAL;
         }
         QDECREF(image_options);
         goto done;
     }
 
-    ret = bdrv_open_inherit(pbs, filename, reference, image_options, 0,
+    bs = NULL;
+    ret = bdrv_open_inherit(&bs, filename, reference, image_options, 0,
                             parent, child_role, NULL, errp);
     if (ret < 0) {
         goto done;
     }
 
-    bdrv_attach_child(parent, *pbs, child_role);
+    c = bdrv_attach_child(parent, bs, child_role);
 
 done:
     qdict_del(options, bdref_key);
-    return ret;
+    return c;
+}
+
+/*
+ * This is a version of bdrv_open_child() that returns 0/-EINVAL instead of
+ * a BdrvChild object.
+ *
+ * If allow_none is true, no image will be opened if filename is false and no
+ * BlockdevRef is given. *pbs will remain unchanged and 0 will be returned.
+ *
+ * To conform with the behavior of bdrv_open(), *pbs has to be NULL.
+ */
+int bdrv_open_image(BlockDriverState **pbs, const char *filename,
+                    QDict *options, const char *bdref_key,
+                    BlockDriverState* parent, const BdrvChildRole *child_role,
+                    bool allow_none, Error **errp)
+{
+    Error *local_err = NULL;
+    BdrvChild *c;
+
+    assert(pbs);
+    assert(*pbs == NULL);
+
+    c = bdrv_open_child(filename, options, bdref_key, parent, child_role,
+                        allow_none, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return -EINVAL;
+    }
+
+    if (c != NULL) {
+        *pbs = c->bs;
+    }
+
+    return 0;
 }
 
 int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
diff --git a/include/block/block.h b/include/block/block.h
index 06e4137..5048772 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -12,6 +12,7 @@
 /* block.c */
 typedef struct BlockDriver BlockDriver;
 typedef struct BlockJob BlockJob;
+typedef struct BdrvChild BdrvChild;
 typedef struct BdrvChildRole BdrvChildRole;
 
 typedef struct BlockDriverInfo {
@@ -208,6 +209,11 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
                     QDict *options, const char *bdref_key,
                     BlockDriverState* parent, const BdrvChildRole *child_role,
                     bool allow_none, Error **errp);
+BdrvChild *bdrv_open_child(const char *filename,
+                           QDict *options, const char *bdref_key,
+                           BlockDriverState* parent,
+                           const BdrvChildRole *child_role,
+                           bool allow_none, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
 int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.4 3/5] block: Introduce bdrv_unref_child()

[Kevin Wolf]

This is the counterpart for bdrv_open_child(). It decreases the
reference count of the child BDS and removes it from the list of
children of the given parent BDS.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               | 23 +++++++++++++++++++++--
 include/block/block.h |  1 +
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 029feeb..b723cf2 100644
--- a/block.c
+++ b/block.c
@@ -1117,6 +1117,24 @@ static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
     return child;
 }
 
+static void bdrv_detach_child(BdrvChild *child)
+{
+    QLIST_REMOVE(child, next);
+    g_free(child);
+}
+
+void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
+{
+    BlockDriverState *child_bs = child->bs;
+
+    if (child->bs->inherits_from == parent) {
+        child->bs->inherits_from = NULL;
+    }
+
+    bdrv_detach_child(child);
+    bdrv_unref(child_bs);
+}
+
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
 {
 
@@ -1884,11 +1902,12 @@ void bdrv_close(BlockDriverState *bs)
         BdrvChild *child, *next;
 
         QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
+            /* TODO Remove bdrv_unref() from drivers' close function and use
+             * bdrv_unref_child() here */
             if (child->bs->inherits_from == bs) {
                 child->bs->inherits_from = NULL;
             }
-            QLIST_REMOVE(child, next);
-            g_free(child);
+            bdrv_detach_child(child);
         }
 
         if (bs->backing_hd) {
diff --git a/include/block/block.h b/include/block/block.h
index 5048772..37916f7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -513,6 +513,7 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 
 void bdrv_ref(BlockDriverState *bs);
 void bdrv_unref(BlockDriverState *bs);
+void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
 
 bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
 void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.4 4/5] block: Reorder cleanups in bdrv_close()

[Kevin Wolf]

Block drivers may still want to access their child nodes in their
.bdrv_close handler. If they unref and/or detach a child by themselves,
this should not result in a double free.

There is additional code for backing files, which are just a special
case of child nodes. The same applies for them.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index b723cf2..d5c9f03 100644
--- a/block.c
+++ b/block.c
@@ -1901,6 +1901,14 @@ void bdrv_close(BlockDriverState *bs)
     if (bs->drv) {
         BdrvChild *child, *next;
 
+        bs->drv->bdrv_close(bs);
+
+        if (bs->backing_hd) {
+            BlockDriverState *backing_hd = bs->backing_hd;
+            bdrv_set_backing_hd(bs, NULL);
+            bdrv_unref(backing_hd);
+        }
+
         QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
             /* TODO Remove bdrv_unref() from drivers' close function and use
              * bdrv_unref_child() here */
@@ -1910,12 +1918,6 @@ void bdrv_close(BlockDriverState *bs)
             bdrv_detach_child(child);
         }
 
-        if (bs->backing_hd) {
-            BlockDriverState *backing_hd = bs->backing_hd;
-            bdrv_set_backing_hd(bs, NULL);
-            bdrv_unref(backing_hd);
-        }
-        bs->drv->bdrv_close(bs);
         g_free(bs->opaque);
         bs->opaque = NULL;
         bs->drv = NULL;
-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.4 5/5] block: Fix backing file child when modifying graph

[Kevin Wolf]

This patch moves bdrv_attach_child() from the individual places that add
a backing file to a BDS to bdrv_set_backing_hd(), which is called by all
of them. It also adds bdrv_detach_child() there.

For normal operation (starting with one backing file chain and not
changing it until the topmost image is closed) and live snapshots, this
constitutes no change in behaviour.

For all other cases, this is a fix for the bug that the old backing file
was still referenced as a child, and the new one wasn't referenced.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 5 +++--
 include/block/block_int.h | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index d5c9f03..d088ee0 100644
--- a/block.c
+++ b/block.c
@@ -1141,6 +1141,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
     if (bs->backing_hd) {
         assert(bs->backing_blocker);
         bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
+        bdrv_detach_child(bs->backing_child);
     } else if (backing_hd) {
         error_setg(&bs->backing_blocker,
                    "node is used as backing hd of '%s'",
@@ -1151,8 +1152,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
     if (!backing_hd) {
         error_free(bs->backing_blocker);
         bs->backing_blocker = NULL;
+        bs->backing_child = NULL;
         goto out;
     }
+    bs->backing_child = bdrv_attach_child(bs, backing_hd, &child_backing);
     bs->open_flags &= ~BDRV_O_NO_BACKING;
     pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
     pstrcpy(bs->backing_format, sizeof(bs->backing_format),
@@ -1236,7 +1239,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
         goto free_exit;
     }
 
-    bdrv_attach_child(bs, backing_hd, &child_backing);
     bdrv_set_backing_hd(bs, backing_hd);
 
 free_exit:
@@ -2171,7 +2173,6 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
     /* The contents of 'tmp' will become bs_top, as we are
      * swapping bs_new and bs_top contents. */
     bdrv_set_backing_hd(bs_top, bs_new);
-    bdrv_attach_child(bs_top, bs_new, &child_backing);
 }
 
 static void bdrv_delete(BlockDriverState *bs)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8996baf..2cc973c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -379,6 +379,7 @@ struct BlockDriverState {
     char exact_filename[PATH_MAX];
 
     BlockDriverState *backing_hd;
+    BdrvChild *backing_child;
     BlockDriverState *file;
 
     NotifierList close_notifiers;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 1/5] block: Move bdrv_attach_child() calls up the call chain

[Max Reitz]

On 08.07.2015 21:36, Kevin Wolf wrote:
> Let the callers of bdrv_open_inherit() call bdrv_attach_child(). It
> needs to be called in all cases where bdrv_open_inherit() succeeds (i.e.
> returns 0) and a child_role is given.
>
> bdrv_attach_child() is moved upwards to avoid a forward declaration.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c | 41 +++++++++++++++++++++--------------------
>   1 file changed, 21 insertions(+), 20 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH for-2.4 2/5] block: Introduce bdrv_open_child()

[Max Reitz]

On 08.07.2015 21:36, Kevin Wolf wrote:
> It is the same as bdrv_open_image(), except that it doesn't only return
> success or failure, but the newly created BdrvChild object for the new
> child node.
>
> As the BdrvChild object already contains a BlockDriverState pointer (and
> this is supposed to become the only pointer so that bdrv_append() and
> friends can just change a single pointer in BdrvChild), the pbs
> parameter is removed for bdrv_open_child().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c               | 71 +++++++++++++++++++++++++++++++++++++--------------
>   include/block/block.h |  6 +++++
>   2 files changed, 58 insertions(+), 19 deletions(-)

Are you planning on removing bdrv_open_image() later on? Because the 
version introduced here behaves differently than the one before this 
patch, in that before the error value returned by bdrv_open() was preserved.

I don't think this is noticeable at all, though, since as long as there 
is an Error object involved, the exact value returned doesn't really 
matter (but I can't verify that assumption, since the value returned by 
bdrv_open_image() seems to be preserved by a lot of nested function calls).

So:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH for-2.4 3/5] block: Introduce bdrv_unref_child()

[Max Reitz]

On 08.07.2015 21:36, Kevin Wolf wrote:
> This is the counterpart for bdrv_open_child(). It decreases the
> reference count of the child BDS and removes it from the list of
> children of the given parent BDS.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c               | 23 +++++++++++++++++++++--
>   include/block/block.h |  1 +
>   2 files changed, 22 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH for-2.4 4/5] block: Reorder cleanups in bdrv_close()

[Max Reitz]

On 08.07.2015 21:36, Kevin Wolf wrote:
> Block drivers may still want to access their child nodes in their
> .bdrv_close handler. If they unref and/or detach a child by themselves,
> this should not result in a double free.
>
> There is additional code for backing files, which are just a special
> case of child nodes. The same applies for them.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH for-2.4 5/5] block: Fix backing file child when modifying graph

[Max Reitz]

On 08.07.2015 21:36, Kevin Wolf wrote:
> This patch moves bdrv_attach_child() from the individual places that add
> a backing file to a BDS to bdrv_set_backing_hd(), which is called by all
> of them. It also adds bdrv_detach_child() there.
>
> For normal operation (starting with one backing file chain and not
> changing it until the topmost image is closed) and live snapshots, this
> constitutes no change in behaviour.
>
> For all other cases, this is a fix for the bug that the old backing file
> was still referenced as a child, and the new one wasn't referenced.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c                   | 5 +++--
>   include/block/block_int.h | 1 +
>   2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index d5c9f03..d088ee0 100644
> --- a/block.c
> +++ b/block.c
> @@ -1141,6 +1141,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>       if (bs->backing_hd) {
>           assert(bs->backing_blocker);
>           bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> +        bdrv_detach_child(bs->backing_child);
>       } else if (backing_hd) {
>           error_setg(&bs->backing_blocker,
>                      "node is used as backing hd of '%s'",
> @@ -1151,8 +1152,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>       if (!backing_hd) {
>           error_free(bs->backing_blocker);
>           bs->backing_blocker = NULL;
> +        bs->backing_child = NULL;

I'd prefer this to be directly below the bdrv_detach_child() call, but 
that's just a question of personal preference.

Reviewed-by: Max Reitz <mreitz@redhat.com>

>           goto out;
>       }
> +    bs->backing_child = bdrv_attach_child(bs, backing_hd, &child_backing);
>       bs->open_flags &= ~BDRV_O_NO_BACKING;
>       pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
>       pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> @@ -1236,7 +1239,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>           goto free_exit;
>       }
>
> -    bdrv_attach_child(bs, backing_hd, &child_backing);
>       bdrv_set_backing_hd(bs, backing_hd);
>
>   free_exit:
> @@ -2171,7 +2173,6 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>       /* The contents of 'tmp' will become bs_top, as we are
>        * swapping bs_new and bs_top contents. */
>       bdrv_set_backing_hd(bs_top, bs_new);
> -    bdrv_attach_child(bs_top, bs_new, &child_backing);
>   }
>
>   static void bdrv_delete(BlockDriverState *bs)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8996baf..2cc973c 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -379,6 +379,7 @@ struct BlockDriverState {
>       char exact_filename[PATH_MAX];
>
>       BlockDriverState *backing_hd;
> +    BdrvChild *backing_child;
>       BlockDriverState *file;
>
>       NotifierList close_notifiers;
>

Re: [Qemu-devel] [PATCH for-2.4 2/5] block: Introduce bdrv_open_child()

[Kevin Wolf]

Am 10.07.2015 um 17:51 hat Max Reitz geschrieben:
> On 08.07.2015 21:36, Kevin Wolf wrote:
> >It is the same as bdrv_open_image(), except that it doesn't only return
> >success or failure, but the newly created BdrvChild object for the new
> >child node.
> >
> >As the BdrvChild object already contains a BlockDriverState pointer (and
> >this is supposed to become the only pointer so that bdrv_append() and
> >friends can just change a single pointer in BdrvChild), the pbs
> >parameter is removed for bdrv_open_child().
> >
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >---
> >  block.c               | 71 +++++++++++++++++++++++++++++++++++++--------------
> >  include/block/block.h |  6 +++++
> >  2 files changed, 58 insertions(+), 19 deletions(-)
> 
> Are you planning on removing bdrv_open_image() later on? Because the
> version introduced here behaves differently than the one before this
> patch, in that before the error value returned by bdrv_open() was
> preserved.

Yes. As I wrote in the cover letter, this is just the start of my
bdrv_swap series. The current status of it can be seen here:

http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/bdrv_swap

All callers of bdrv_open_image() are converted to bdrv_open_child()
there and bdrv_open_image() is removed eventually. (The -EINVAL stays,
however, it's just moved to the callers.)

> I don't think this is noticeable at all, though, since as long as
> there is an Error object involved, the exact value returned doesn't
> really matter (but I can't verify that assumption, since the value
> returned by bdrv_open_image() seems to be preserved by a lot of
> nested function calls).

Right, that's my assumption as well. If .bdrv_open() returns an Error,
the exact return code shouldn't matter as long as it's negative.

Kevin