Re: [Qemu-devel] [PATCH 5/8] block: use BlockDriverState refcnt for device attach/detach

[Jeff Cody]

On Thu, Jul 25, 2013 at 05:01:43PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index dfa4be0..ce4d94b 100644
> --- a/block.c
> +++ b/block.c
> @@ -1620,11 +1620,13 @@ int bdrv_attach_dev(BlockDriverState *bs, void *dev)
>          return -EBUSY;
>      }
>      bs->dev = dev;
> +    bdrv_ref(bs);
>      bdrv_iostatus_reset(bs);
>      return 0;
>  }
>  
> -/* TODO qdevified devices don't use this, remove when devices are qdevified 
> */
> +/* Attach a bs to dev, and increase its refcnt.
> + * TODO qdevified devices don't use this, remove when devices are qdevified 
> */
>  void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev)
>  {
>      if (bdrv_attach_dev(bs, dev) < 0) {
> @@ -1632,10 +1634,13 @@ void bdrv_attach_dev_nofail(BlockDriverState *bs, 
> void *dev)
>      }
>  }
>  
> +/* Detach bs from device. This decreases its refcnt, and may consequently
> + * deletes it make bs an invalid pointer */
>  void bdrv_detach_dev(BlockDriverState *bs, void *dev)
>  /* TODO change to DeviceState *dev when all users are qdevified */
>  {
>      assert(bs->dev == dev);
> +    bdrv_unref(bs);
>      bs->dev = NULL;
>      bs->dev_ops = NULL;
>      bs->dev_opaque = NULL;

This won't work, since we are dereferencing bs shortly after
(potentially) freeing it.  I would say just move the bdrv_unref() to
the end of the function, but I have another concern as well.  

If bs is freed, then BDS pointer is now invalid, but not NULL.  So
there is no way for callers of bdrv_detach_dev() to know if the BDS
pointer they passed into bdrv_detach_dev() is still valid; in fact, I
think some call bdrv_close(bs) afterwards (piix).  Qdev also still
uses it, although just for pointer comparison and not dereferencing.

Jeff

> -- 
> 1.8.3.2
> 
>