diff mbox series

[v3,2/2] block: Make bdrv_root_attach_child()unref child_bs on failure

Message ID 20dfb3d9ccec559cdd1a9690146abad5d204a186.1557754872.git.berto@igalia.com
State New
Headers show
Series block: Use bdrv_unref_child() for allchildren in bdrv_close() | expand

Commit Message

Alberto Garcia May 13, 2019, 1:46 p.m. UTC 
A consequence of the previous patch is that bdrv_attach_child()
transfers the reference to child_bs from the caller to parent_bs,
which will drop it on bdrv_close() or when someone calls
bdrv_unref_child().

But this only happens when bdrv_attach_child() succeeds. If it fails
then the caller is responsible for dropping the reference to child_bs.

This patch makes bdrv_attach_child() take the reference also when
there is an error, freeing the caller for having to do it.

A similar situation happens with bdrv_root_attach_child(), so the
changes on this patch affect both functions.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c               | 25 +++++++++++++++++--------
 block/block-backend.c |  3 +--
 block/quorum.c        |  1 -
 blockjob.c            |  2 +-
 4 files changed, 19 insertions(+), 12 deletions(-)

Comments

Max Reitz May 13, 2019, 2:31 p.m. UTC | #1 
On 13.05.19 15:46, Alberto Garcia wrote:
> A consequence of the previous patch is that bdrv_attach_child()
> transfers the reference to child_bs from the caller to parent_bs,
> which will drop it on bdrv_close() or when someone calls
> bdrv_unref_child().
> 
> But this only happens when bdrv_attach_child() succeeds. If it fails
> then the caller is responsible for dropping the reference to child_bs.
> 
> This patch makes bdrv_attach_child() take the reference also when
> there is an error, freeing the caller for having to do it.
> 
> A similar situation happens with bdrv_root_attach_child(), so the
> changes on this patch affect both functions.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c               | 25 +++++++++++++++++--------
>  block/block-backend.c |  3 +--
>  block/quorum.c        |  1 -
>  blockjob.c            |  2 +-
>  4 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3c3bd0f8d2..df727314ff 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -2569,10 +2582,6 @@ BdrvChild *bdrv_open_child(const char *filename,
>      }
>  
>      c = bdrv_attach_child(parent, bs, bdref_key, child_role, errp);
> -    if (!c) {
> -        bdrv_unref(bs);
> -        return NULL;
> -    }
>  
>      return c;
>  }

(That could have been simplified even further. *shrug*)
Alberto Garcia May 13, 2019, 3:52 p.m. UTC | #2 
On Mon 13 May 2019 04:31:16 PM CEST, Max Reitz wrote:
>> @@ -2569,10 +2582,6 @@ BdrvChild *bdrv_open_child(const char *filename,
>>      }
>>  
>>      c = bdrv_attach_child(parent, bs, bdref_key, child_role, errp);
>> -    if (!c) {
>> -        bdrv_unref(bs);
>> -        return NULL;
>> -    }
>>  
>>      return c;
>>  }
>
> (That could have been simplified even further. *shrug*)

Right, we can remove the 'c' variable altogether. Feel free to edit the
commit if you want.

Berto
diff mbox series

Patch

diff --git a/block.c b/block.c
index 3c3bd0f8d2..df727314ff 100644
--- a/block.c
+++ b/block.c
@@ -2208,6 +2208,13 @@  static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
     }
 }
 
+/*
+ * This function steals the reference to child_bs from the caller.
+ * That reference is later dropped by bdrv_root_unref_child().
+ *
+ * On failure NULL is returned, errp is set and the reference to
+ * child_bs is also dropped.
+ */
 BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   const char *child_name,
                                   const BdrvChildRole *child_role,
@@ -2220,6 +2227,7 @@  BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
     ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp);
     if (ret < 0) {
         bdrv_abort_perm_update(child_bs);
+        bdrv_unref(child_bs);
         return NULL;
     }
 
@@ -2239,6 +2247,14 @@  BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
     return child;
 }
 
+/*
+ * This function transfers the reference to child_bs from the caller
+ * to parent_bs. That reference is later dropped by parent_bs on
+ * bdrv_close() or if someone calls bdrv_unref_child().
+ *
+ * On failure NULL is returned, errp is set and the reference to
+ * child_bs is also dropped.
+ */
 BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                              BlockDriverState *child_bs,
                              const char *child_name,
@@ -2366,12 +2382,9 @@  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
     /* 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). */
-    if (update_inherits_from) {
+    if (bs->backing && update_inherits_from) {
         backing_hd->inherits_from = bs;
     }
-    if (!bs->backing) {
-        bdrv_unref(backing_hd);
-    }
 
 out:
     bdrv_refresh_limits(bs, NULL);
@@ -2569,10 +2582,6 @@  BdrvChild *bdrv_open_child(const char *filename,
     }
 
     c = bdrv_attach_child(parent, bs, bdref_key, child_role, errp);
-    if (!c) {
-        bdrv_unref(bs);
-        return NULL;
-    }
 
     return c;
 }
diff --git a/block/block-backend.c b/block/block-backend.c
index f78e82a707..7a621597e7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -383,7 +383,6 @@  BlockBackend *blk_new_open(const char *filename, const char *reference,
     blk->root = bdrv_root_attach_child(bs, "root", &child_root,
                                        perm, BLK_PERM_ALL, blk, errp);
     if (!blk->root) {
-        bdrv_unref(bs);
         blk_unref(blk);
         return NULL;
     }
@@ -791,12 +790,12 @@  void blk_remove_bs(BlockBackend *blk)
 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->perm, blk->shared_perm, blk, errp);
     if (blk->root == NULL) {
         return -EPERM;
     }
-    bdrv_ref(bs);
 
     notifier_list_notify(&blk->insert_bs_notifiers, blk);
     if (tgm->throttle_state) {
diff --git a/block/quorum.c b/block/quorum.c
index 352f729136..133ee18204 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1019,7 +1019,6 @@  static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
     child = bdrv_attach_child(bs, child_bs, indexstr, &child_format, errp);
     if (child == NULL) {
         s->next_child_index--;
-        bdrv_unref(child_bs);
         goto out;
     }
     s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
diff --git a/blockjob.c b/blockjob.c
index 730101d282..b68fa4b13e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -208,6 +208,7 @@  int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
 {
     BdrvChild *c;
 
+    bdrv_ref(bs);
     c = bdrv_root_attach_child(bs, name, &child_job, perm, shared_perm,
                                job, errp);
     if (c == NULL) {
@@ -215,7 +216,6 @@  int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
     }
 
     job->nodes = g_slist_prepend(job->nodes, c);
-    bdrv_ref(bs);
     bdrv_op_block_all(bs, job->blocker);
 
     return 0;