Re: [Qemu-devel] [PATCH v2 03/23] block: Connect BlockBackend to BlockDriverState

[Max Reitz <mreitz@redhat.com>]

On 13.09.2014 17:00, Markus Armbruster wrote:
> The pointer from BlockBackend to BlockDriverState is a strong
> reference, managed with bdrv_ref() / bdrv_unref(), the back-pointer is
> a weak one.
>
> Convenience function blk_new_with_bs() creates a BlockBackend with its
> BlockDriverState.  Callers have to unref both.  The commit after next
> will relieve them of the need to unref the BlockDriverState.
>
> Complication: due to the silly way drive_del works, we need a way to
> hide a BlockBackend, just like bdrv_make_anon().  To emphasize its
> "special" status, give the function a suitably off-putting name:

So you're trying to force people with a sense of aesthetics to solve 
this problem? I don't know why this isn't taught in Software Engineering 
in university, but it definitely should be.

> blk_hide_on_behalf_of_do_drive_del().  Unfortunately, hiding turns the
> BlockBackend's name into the empty string.  Can't avoid that without
> breaking the blk->bs->device_name equals blk->name invariant.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block.c                        |  10 ++--
>   block/block-backend.c          |  70 ++++++++++++++++++++++-
>   blockdev.c                     |  26 +++------
>   hw/block/xen_disk.c            |   8 +--
>   include/block/block_int.h      |   2 +
>   include/sysemu/block-backend.h |   5 ++
>   qemu-img.c                     | 125 +++++++++++++++++++----------------------
>   qemu-io.c                      |   4 +-
>   qemu-nbd.c                     |   2 +-
>   9 files changed, 152 insertions(+), 100 deletions(-)

[snip]

> diff --git a/block/block-backend.c b/block/block-backend.c
> index 919dd4c..b118b38 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -16,10 +16,11 @@
>   struct BlockBackend {
>       char *name;
>       int refcnt;
> +    BlockDriverState *bs;
>       QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
>   };
>   
> -/* All the BlockBackends */
> +/* All the BlockBackends (except for hidden ones) */
>   static QTAILQ_HEAD(, BlockBackend) blk_backends =
>       QTAILQ_HEAD_INITIALIZER(blk_backends);
>   
> @@ -47,10 +48,43 @@ BlockBackend *blk_new(const char *name, Error **errp)
>       return blk;
>   }
>   
> +/*
> + * Create a new BlockBackend with a new BlockDriverState attached.
> + * Both have a reference count of one.  Caller owns *both* references.
> + * TODO Let caller own only the BlockBackend reference
> + * Otherwise just like blk_new(), which see.

Could be my lack of profoundness in English, but I don't understand what 
"which see" is supposed to mean or how its grammar works. An imperative?

> + */
> +BlockBackend *blk_new_with_bs(const char *name, Error **errp)
> +{
> +    BlockBackend *blk;
> +    BlockDriverState *bs;
> +
> +    blk = blk_new(name, errp);
> +    if (!blk) {
> +        return NULL;
> +    }
> +
> +    bs = bdrv_new_root(name, errp);
> +    if (!bs) {
> +        blk_unref(blk);
> +        return NULL;
> +    }
> +
> +    blk->bs = bs;
> +    bs->blk = blk;
> +    return blk;
> +}
> +

[snip]

> diff --git a/blockdev.c b/blockdev.c
> index 5873205..21f4c67 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -229,14 +229,7 @@ void drive_info_del(DriveInfo *dinfo)
>           qemu_opts_del(dinfo->opts);
>       }
>   
> -    /*
> -     * Hairy special case: if do_drive_del() has made dinfo->bdrv
> -     * anonymous, it also unref'ed the associated BlockBackend.
> -     */
> -    if (dinfo->bdrv->device_name[0]) {
> -        blk_unref(blk_by_name(dinfo->bdrv->device_name));
> -    }
> -
> +    blk_unref(blk_by_name(dinfo->bdrv->device_name));

So if !device_name[0], the BB is hidden. Hidden BBs are removed from the 
BB list and therefore not returned by blk_by_name(). Therefore, the BB 
is leaked here.

I guess this will be fixed up in a later patch, though...

>       g_free(dinfo->id);
>       QTAILQ_REMOVE(&drives, dinfo, next);
>       g_free(dinfo->serial);

[snip]

> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index ff95da6..fa8a7d0 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -689,7 +689,7 @@ int main(int argc, char **argv)
>       }
>   
>       blk = blk_new("hda", &error_abort);
> -    bs = bdrv_new_root("hda", &error_abort);
> +    bs = blk_bs(blk);

Shouldn't that be a blk_new_with_bs() then, just like every other case?

>       srcpath = argv[optind];
>       ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);

Max