Re: [Qemu-devel] [PATCH 05/23] block: Make BlockBackend own its BlockDriverState

[Kevin Wolf]

Am 10.09.2014 um 10:13 hat Markus Armbruster geschrieben:
> On BlockBackend destruction, unref its BlockDriverState.  Replaces the
> callers' unrefs.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block/block-backend.c |  9 ++-------
>  blockdev.c            | 11 +++--------
>  hw/block/xen_disk.c   |  6 +++---
>  qemu-img.c            | 35 +----------------------------------
>  qemu-io.c             |  5 -----
>  5 files changed, 9 insertions(+), 57 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 2a22660..ae51f7f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -58,10 +58,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
>   * @errp: return location for an error to be set on failure, or %NULL
>   *
>   * Create a new BlockBackend, with a reference count of one, and
> - * attach a new BlockDriverState to it, also with a reference count of
> - * one.  Caller owns *both* references.
> - * TODO Let caller own only the BlockBackend reference
> - * Fail if @name already exists.
> + * a new BlockDriverState attached.  Fail if @name already exists.
>   *
>   * Returns: the BlockBackend on success, %NULL on error
>   */
> @@ -88,6 +85,7 @@ BlockBackend *blk_new_with_bs(const char *name, Error 
> **errp)
>  static void blk_delete(BlockBackend *blk)
>  {
>      assert(!blk->refcnt);
> +    bdrv_unref(blk->bs);
>      blk_detach_bs(blk);

I think the bdrv_unref() should really be part of blk_detach_bs().

The same way it would be more logical to have bdrv_ref() as part of
blk_attach_bs(). For blk_new_with_bs() this might mean bdrv_new,
blk_attach_bs, bdrv_unref, which looks a bit odd, but if blk_attach_bs()
is ever called from somewhere else, it probably makes more sense (if it
isn't, it should be static).

Kevin