| Message ID | 1410891148-28849-24-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <armbru@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.canet@nodalink.com>
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 <armbru@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
Max Reitz <mreitz@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 <armbru@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.
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 <armbru@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 <kwolf@redhat.com>
Kevin Wolf <kwolf@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 <armbru@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 <kwolf@redhat.com> Thanks!
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); }
Doesn't make a difference just yet, but it's the right thing to do. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/block-backend.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)