Re: [Qemu-devel] [PATCH v2 03/23] block: Connect BlockBackend to BlockDriverState

[Markus Armbruster <armbru@redhat.com>]

Max Reitz <mreitz@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 <armbru@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!