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

[Kevin Wolf]

Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
> 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:
> 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.
> 
> The patch adds a memory leak: drive_del while a device model is
> connected leaks the BlockBackend.  Avoiding the leak here is rather
> hairy, but it'll become straightforward in a few commits, so I mark it
> FIXME in the code now, and plug it when it's easy.
> 
> Signed-off-by: Markus Armbruster <address@hidden>

> +/*
> + * Hide @blk.
> + * @blk must not have been hidden already.
> + * Make attached BlockDriverState, if any, anonymous.
> + * Once hidden, @blk is invisible to all functions that don't receive
> + * it as argument.  For example, blk_by_name() won't return it.
> + * Strictly for use by do_drive_del().
> + * TODO get rid of it!
> + */
> +void blk_hide_on_behalf_of_do_drive_del(BlockBackend *blk)
> +{
> +    QTAILQ_REMOVE(&blk_backends, blk, link);
> +    blk->name[0] = 0;

Style nit: I prefer '\0' when dealing with strings.

> +    if (blk->bs) {
> +        bdrv_make_anon(blk->bs);
> +    }
> +}
> diff --git a/blockdev.c b/blockdev.c
> index 583235a..5da6028 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -228,6 +228,7 @@ void drive_info_del(DriveInfo *dinfo)
>      if (dinfo->opts) {
>          qemu_opts_del(dinfo->opts);
>      }
> +
>      g_free(dinfo->id);
>      QTAILQ_REMOVE(&drives, dinfo, next);
>      g_free(dinfo->serial);

This hunk is a rebasing artifact, I guess?

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8d86a6c..14e0b7c 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -324,6 +324,8 @@ struct BlockDriverState {
>      BlockDriver *drv; /* NULL means no media */
>      void *opaque;
>  
> +    BlockBackend *blk;          /* owning backend, if any */
> +
>      void *dev;                  /* attached device model, if any */
>      /* TODO change to DeviceState when all users are qdevified */
>      const BlockDevOps *dev_ops;

Just to make sure that we agree on where we're going: This makes the
assumption that a BDS has at most one BB that owns it. Which is not the
final state that we want to have, so this will have to go away later.
(Where "later" isn't necessarily part of this series.)

For now, the use of the field is limited to callbacks and
bdrv_get_device_name(). Callbacks could always only serve a single
device, so nothing became worse here.

I'm not entirely sure about bdrv_get_device_name(), whether it needs to
go or to be rewritten to get the name of any BB pointing to it (I
suspect for most callers we want to replace it by something that uses
node-name by default if there is one and only fall back to BB names if
there isn't), but that's not an issue to block this patch.

What I would consider, however, is adding a TODO comment that tells
people that this field needs to go and if you need to use it, something
is wrong with your design (which happens to be true for the existing
design of some code).


Nothing critical in this patch, so with or without addressing the
comments:

Reviewed-by: Kevin Wolf <address@hidden>