Re: [Qemu-devel] [PATCH v5 21/38] block: Add blk_insert_bs()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2855 bytes --]

On 22.09.2015 16:42, Kevin Wolf wrote:
> Am 18.09.2015 um 17:22 hat Max Reitz geschrieben:
>> This function associates the given BlockDriverState with the given
>> BlockBackend.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/block-backend.c          | 16 ++++++++++++++++
>>  include/sysemu/block-backend.h |  1 +
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 33145f8..652385e 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -314,6 +314,22 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
>>  }
>>  
>>  /*
>> + * Associates a new BlockDriverState with @blk.
>> + */
>> +void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
>> +{
>> +    if (bs->blk == blk) {
>> +        return;
>> +    }
>> +
>> +    assert(!blk->bs);
>> +    assert(!bs->blk);
> 
> Why is it useful to allow reconnecting a BDS to a BB it's already
> connected to? I would have expected that we can assert that this is not
> the case.

We can do that, too, there is no use case. It's just that I in principle
don't like making things an error that do make sense and can trivially
be handled gracefully. But I can see people wanting to hit an assertion
as soon as something looks fishy.

So I'm fine either way. I think Eric asked about the same before, so
that makes two against one now. :-)

>> +    bdrv_ref(bs);
>> +    blk->bs = bs;
>> +    bs->blk = blk;
>> +}
> 
> My series to remove bdrv_swap() introduces a blk_set_bs() function,
> which looks suspiciously similar to this one, except that it allows
> passing a BB that already had a BDS (it gets unrefed then) and that I
> don't assert that the BDS isn't attached to a BB yet (I should do that).
> 
> Do you think that's similar enough to have only one function?

Well, yours looks like blk_remove_bs()+blk_insert_bs() combined. In case
we have called blk_remove_bs() before, it will effectively be the same,
yes. But that difference still bothers me a bit... I'd prefer
implementing blk_set_bs() by calling blk_remove_bs() and then
blk_insert_bs() instead, but since these functions are not available in
your bdrv_swap() series, that would prove rather difficult.

I don't know. Maybe implement it separately for now, and trust that this
series will stay in flight long enough for the bdrv_swap() series to get
merged so I can include a commit cleaning up blk_set_bs() in this series
later on?

Or I can base this series on your bdrv_swap() series. Your call, as I
haven't looked at it yet and thus don't know how long it'll take to get
merged (albeit judging from your series in the past, it won't be too long).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]