Re: [Qemu-devel] [PATCH v3 23/23] block: Make device model's references to BlockBackend strong

2014-09-30 Thread Kevin Wolf 
Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
 Doesn't make a difference just yet, but it's the right thing to do.
 
 Signed-off-by: Markus Armbruster arm...@redhat.com
 ---
  block/block-backend.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/block/block-backend.c b/block/block-backend.c
 index d49c988..5646628 100644
 --- a/block/block-backend.c
 +++ b/block/block-backend.c
 @@ -253,6 +253,7 @@ int blk_attach_dev(BlockBackend *blk, void *dev)
  if (blk-dev) {
  return -EBUSY;
  }
 +blk_ref(blk);
  blk-dev = dev;
  bdrv_iostatus_reset(blk-bs);
  
 @@ -281,9 +282,10 @@ void blk_detach_dev(BlockBackend *blk, void *dev)
  /* TODO change to DeviceState *dev when all users are qdevified */
  {
  assert(blk-dev == dev);
 -blk-dev = NULL;
  blk-dev_ops = NULL;
  blk-dev_opaque = NULL;
 +blk-dev = NULL;

Is the move of blk-dev intentional or a rebase artifact?

 +blk_unref(blk);
  bdrv_set_guest_block_size(blk-bs, 512);
  qemu_coroutine_adjust_pool_size(-COROUTINE_POOL_RESERVATION);
  }

hw/sd/sd.c calls blk_attach_dev_nofail(), but never detaches the BB
again. The reference count will therefore never become zero. Probably
okay for a device that isn't unpluggable, bdrv_close_all() should still
do everything that is important for a clean shutdown.

Reviewed-by: Kevin Wolf kw...@redhat.com

Re: [Qemu-devel] [PATCH v3 23/23] block: Make device model's references to BlockBackend strong

2014-09-30 Thread Markus Armbruster 
Kevin Wolf kw...@redhat.com writes:

 Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
 Doesn't make a difference just yet, but it's the right thing to do.
 
 Signed-off-by: Markus Armbruster arm...@redhat.com
 ---
  block/block-backend.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/block/block-backend.c b/block/block-backend.c
 index d49c988..5646628 100644
 --- a/block/block-backend.c
 +++ b/block/block-backend.c
 @@ -253,6 +253,7 @@ int blk_attach_dev(BlockBackend *blk, void *dev)
  if (blk-dev) {
  return -EBUSY;
  }
 +blk_ref(blk);
  blk-dev = dev;
  bdrv_iostatus_reset(blk-bs);
  
 @@ -281,9 +282,10 @@ void blk_detach_dev(BlockBackend *blk, void *dev)
  /* TODO change to DeviceState *dev when all users are qdevified */
  {
  assert(blk-dev == dev);
 -blk-dev = NULL;
  blk-dev_ops = NULL;
  blk-dev_opaque = NULL;
 +blk-dev = NULL;

 Is the move of blk-dev intentional or a rebase artifact?

Artifact, already cleaned up in my tree.

 +blk_unref(blk);
  bdrv_set_guest_block_size(blk-bs, 512);
  qemu_coroutine_adjust_pool_size(-COROUTINE_POOL_RESERVATION);
  }

 hw/sd/sd.c calls blk_attach_dev_nofail(), but never detaches the BB
 again. The reference count will therefore never become zero.

Feature!

  Probably
 okay for a device that isn't unpluggable, bdrv_close_all() should still
 do everything that is important for a clean shutdown.

Yes, that's its mission.

 Reviewed-by: Kevin Wolf kw...@redhat.com

Thanks!

Re: [Qemu-devel] [PATCH v3 23/23] block: Make device model's references to BlockBackend strong

2014-09-22 Thread Benoît Canet 
On Tue, Sep 16, 2014 at 08:12:28PM +0200, Markus Armbruster wrote:
 Doesn't make a difference just yet, but it's the right thing to do.
 
 Signed-off-by: Markus Armbruster arm...@redhat.com
 ---
  block/block-backend.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/block/block-backend.c b/block/block-backend.c
 index d49c988..5646628 100644
 --- a/block/block-backend.c
 +++ b/block/block-backend.c
 @@ -253,6 +253,7 @@ int blk_attach_dev(BlockBackend *blk, void *dev)
  if (blk-dev) {
  return -EBUSY;
  }
 +blk_ref(blk);
  blk-dev = dev;
  bdrv_iostatus_reset(blk-bs);
  
 @@ -281,9 +282,10 @@ void blk_detach_dev(BlockBackend *blk, void *dev)
  /* TODO change to DeviceState *dev when all users are qdevified */
  {
  assert(blk-dev == dev);
 -blk-dev = NULL;
  blk-dev_ops = NULL;
  blk-dev_opaque = NULL;
 +blk-dev = NULL;
 +blk_unref(blk);
  bdrv_set_guest_block_size(blk-bs, 512);
  qemu_coroutine_adjust_pool_size(-COROUTINE_POOL_RESERVATION);
  }
 -- 
 1.9.3
 
Reviewed-by: Benoit Canet benoit.ca...@nodalink.com

Re: [Qemu-devel] [PATCH v3 23/23] block: Make device model's references to BlockBackend strong

2014-09-22 Thread Max Reitz 

On 16.09.2014 20:12, Markus Armbruster wrote:

Doesn't make a difference just yet, but it's the right thing to do.

Signed-off-by: Markus Armbruster arm...@redhat.com
---
  block/block-backend.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index d49c988..5646628 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -253,6 +253,7 @@ int blk_attach_dev(BlockBackend *blk, void *dev)
  if (blk-dev) {
  return -EBUSY;
  }
+blk_ref(blk);
  blk-dev = dev;
  bdrv_iostatus_reset(blk-bs);
  
@@ -281,9 +282,10 @@ void blk_detach_dev(BlockBackend *blk, void *dev)

  /* TODO change to DeviceState *dev when all users are qdevified */
  {
  assert(blk-dev == dev);
-blk-dev = NULL;
  blk-dev_ops = NULL;
  blk-dev_opaque = NULL;
+blk-dev = NULL;
+blk_unref(blk);
  bdrv_set_guest_block_size(blk-bs, 512);
  qemu_coroutine_adjust_pool_size(-COROUTINE_POOL_RESERVATION);
  }


I'd put the blk_unref() call at the very end of this function. It 
probably won't happen but theoretically blk_unref() can free the 
BlockBackend object and I don't like the risk of use-after-free in blk-bs.


Max

Re: [Qemu-devel] [PATCH v3 23/23] block: Make device model's references to BlockBackend strong

2014-09-22 Thread Markus Armbruster 
Max Reitz mre...@redhat.com writes:

 On 16.09.2014 20:12, Markus Armbruster wrote:
 Doesn't make a difference just yet, but it's the right thing to do.

 Signed-off-by: Markus Armbruster arm...@redhat.com
 ---
   block/block-backend.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)

 diff --git a/block/block-backend.c b/block/block-backend.c
 index d49c988..5646628 100644
 --- a/block/block-backend.c
 +++ b/block/block-backend.c
 @@ -253,6 +253,7 @@ int blk_attach_dev(BlockBackend *blk, void *dev)
   if (blk-dev) {
   return -EBUSY;
   }
 +blk_ref(blk);
   blk-dev = dev;
   bdrv_iostatus_reset(blk-bs);
   @@ -281,9 +282,10 @@ void blk_detach_dev(BlockBackend *blk, void
 *dev)
   /* TODO change to DeviceState *dev when all users are qdevified */
   {
   assert(blk-dev == dev);
 -blk-dev = NULL;
   blk-dev_ops = NULL;
   blk-dev_opaque = NULL;
 +blk-dev = NULL;
 +blk_unref(blk);
   bdrv_set_guest_block_size(blk-bs, 512);
   qemu_coroutine_adjust_pool_size(-COROUTINE_POOL_RESERVATION);
   }

 I'd put the blk_unref() call at the very end of this function. It
 probably won't happen but theoretically blk_unref() can free the
 BlockBackend object and I don't like the risk of use-after-free in
 blk-bs.

Even if it can't happen, putting it at the end is more obviously
correct.  I'll do it.