Kevin Wolf <kw...@redhat.com> writes: > 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?
I run "make check-qtest check-block" on every commit before I submit. No idea what went wrong with this one. > You probably need to swap bs->blk in bdrv_move_feature_fields(). I'll look into it, thanks!
