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 <arm...@redhat.com> > --- > block.c | 10 ++-- > block/block-backend.c | 71 ++++++++++++++++++++++- > blockdev.c | 21 ++++--- > hw/block/xen_disk.c | 8 +-- > include/block/block_int.h | 2 + > include/sysemu/block-backend.h | 5 ++ > qemu-img.c | 125 > +++++++++++++++++++---------------------- > qemu-io.c | 4 +- > qemu-nbd.c | 4 +- > 9 files changed, 156 insertions(+), 94 deletions(-) > > diff --git a/block.c b/block.c > index 934881f..7ccf443 100644 > --- a/block.c > +++ b/block.c > @@ -2032,7 +2032,7 @@ static void bdrv_move_feature_fields(BlockDriverState > *bs_dest, > * This will modify the BlockDriverState fields, and swap contents > * between bs_new and bs_old. Both bs_new and bs_old are modified. > * > - * bs_new is required to be anonymous. > + * bs_new must be nameless and not attached to a BlockBackend. > * > * This function does not create any image files. > */ > @@ -2051,8 +2051,9 @@ void bdrv_swap(BlockDriverState *bs_new, > BlockDriverState *bs_old) > QTAILQ_REMOVE(&graph_bdrv_states, bs_old, node_list); > } > > - /* bs_new must be anonymous and shouldn't have anything fancy enabled */ > + /* bs_new must be nameless and shouldn't have anything fancy enabled */ > assert(bs_new->device_name[0] == '\0'); > + assert(!bs_new->blk); > assert(QLIST_EMPTY(&bs_new->dirty_bitmaps)); > assert(bs_new->job == NULL); > assert(bs_new->dev == NULL); > @@ -2068,8 +2069,9 @@ void bdrv_swap(BlockDriverState *bs_new, > BlockDriverState *bs_old) > bdrv_move_feature_fields(bs_old, bs_new); > bdrv_move_feature_fields(bs_new, &tmp); > > - /* bs_new shouldn't be in bdrv_states even after the swap! */ > + /* bs_new must remain nameless and unattached */ > assert(bs_new->device_name[0] == '\0'); > + assert(!bs_new->blk);
Taking back my R-b: You tricked us, this assertion doesn't hold true. Easy to reproduce by taking a live snapshot. qemu-iotests case 052 catches it. Didn't you run it? You probably need to swap bs->blk in bdrv_move_feature_fields(). Kevin
