Am 30.09.2014 um 12:56 hat Markus Armbruster geschrieben: > 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.
When run for raw, it's only 052 that catches it. For qcow2, I got some more failures: 039 040 041 051 052 085 I see the problem: Only 039 and 052 are marked as 'quick', i.e. the rest is already excluded from 'make check-block'. 039 and 052 don't work with cache=none and 'make check-block' uses -nocache, so those are skipped as well. I'll send a patch to remove the -nocache option and let it run with the default options. Kevin
