Max Reitz <mre...@redhat.com> writes:
> On 13.09.2014 17:00, Markus Armbruster wrote:
>> 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:
>
> So you're trying to force people with a sense of aesthetics to solve
> this problem? I don't know why this isn't taught in Software
> Engineering in university, but it definitely should be.
Ha!
Actually, I only want to deter additional use of this bad idea.
Once we have a decent solution for the problem drive_del is trying to
solve, we can deprecate drive_del, then remove it after a couple of
releases (it's only HMP, not a stable interface). So the solution to
the problem will hopefully be "throw it away".
>> 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.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> block.c | 10 ++--
>> block/block-backend.c | 70 ++++++++++++++++++++++-
>> blockdev.c | 26 +++------
>> 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 | 2 +-
>> 9 files changed, 152 insertions(+), 100 deletions(-)
>
> [snip]
>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 919dd4c..b118b38 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -16,10 +16,11 @@
>> struct BlockBackend {
>> char *name;
>> int refcnt;
>> + BlockDriverState *bs;
>> QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
>> };
>> -/* All the BlockBackends */
>> +/* All the BlockBackends (except for hidden ones) */
>> static QTAILQ_HEAD(, BlockBackend) blk_backends =
>> QTAILQ_HEAD_INITIALIZER(blk_backends);
>> @@ -47,10 +48,43 @@ BlockBackend *blk_new(const char *name, Error
>> **errp)
>> return blk;
>> }
>> +/*
>> + * Create a new BlockBackend with a new BlockDriverState attached.
>> + * Both have a reference count of one. Caller owns *both* references.
>> + * TODO Let caller own only the BlockBackend reference
>> + * Otherwise just like blk_new(), which see.
>
> Could be my lack of profoundness in English, but I don't understand
> what "which see" is supposed to mean or how its grammar works. An
> imperative?
I guess I picked this up in my reading. Wikipedia recognizes it, so it
must be okay ;-}
https://en.wikipedia.org/wiki/Which_see
>> + */
>> +BlockBackend *blk_new_with_bs(const char *name, Error **errp)
>> +{
>> + BlockBackend *blk;
>> + BlockDriverState *bs;
>> +
>> + blk = blk_new(name, errp);
>> + if (!blk) {
>> + return NULL;
>> + }
>> +
>> + bs = bdrv_new_root(name, errp);
>> + if (!bs) {
>> + blk_unref(blk);
>> + return NULL;
>> + }
>> +
>> + blk->bs = bs;
>> + bs->blk = blk;
>> + return blk;
>> +}
>> +
>
> [snip]
>
>> diff --git a/blockdev.c b/blockdev.c
>> index 5873205..21f4c67 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -229,14 +229,7 @@ void drive_info_del(DriveInfo *dinfo)
>> qemu_opts_del(dinfo->opts);
>> }
>> - /*
>> - * Hairy special case: if do_drive_del() has made dinfo->bdrv
>> - * anonymous, it also unref'ed the associated BlockBackend.
>> - */
>> - if (dinfo->bdrv->device_name[0]) {
>> - blk_unref(blk_by_name(dinfo->bdrv->device_name));
>> - }
>> -
>> + blk_unref(blk_by_name(dinfo->bdrv->device_name));
>
> So if !device_name[0], the BB is hidden. Hidden BBs are removed from
> the BB list and therefore not returned by blk_by_name(). Therefore,
> the BB is leaked here.
You're right. This part of the patches went through a couple of
iterations already, and at some point I zapped device_name without
realizing it introduces a leak here. I'll try to avoid the zapping.
> I guess this will be fixed up in a later patch, though...
Right again.
>> g_free(dinfo->id);
>> QTAILQ_REMOVE(&drives, dinfo, next);
>> g_free(dinfo->serial);
>
> [snip]
>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index ff95da6..fa8a7d0 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -689,7 +689,7 @@ int main(int argc, char **argv)
>> }
>> blk = blk_new("hda", &error_abort);
>> - bs = bdrv_new_root("hda", &error_abort);
>> + bs = blk_bs(blk);
>
> Shouldn't that be a blk_new_with_bs() then, just like every other case?
Fixing...
>> srcpath = argv[optind];
>> ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);
Thanks!
