Patchwork [5/8] block: use BlockDriverState refcnt for device attach/detach

login
register
mail settings
Submitter Fam Zheng
Date July 25, 2013, 9:01 a.m.
Message ID <1374742906-4489-6-git-send-email-famz@redhat.com>
Download mbox | patch
Permalink /patch/261631/
State New
Headers show

Comments

Fam Zheng - July 25, 2013, 9:01 a.m.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Jeff Cody - July 25, 2013, 12:49 p.m.
On Thu, Jul 25, 2013 at 05:01:43PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  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
> 
>

Patch

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;