Re: [PATCH] qapi/block: fix nbd-server-add spec

[Vladimir Sementsov-Ogievskiy]

19.12.2019 17:42, Nir Soffer wrote:
> On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy
> <address@hidden> wrote:
>>
>> "NAME" here may be interpreted like it should match @name, which is
>> export name. But it was never mentioned in such way. Make it obvious,
>> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt)
>> will match @bitmap parameter.
> 
> But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap
> name but the name exposed to the NBD client, which can be anything.

Yes. What is wrong? It can be enything. Currently by default it is bitmap name.
It purely documented (okay, even confusingly documented), but it was so since
4.0. And existing users obviously knows how it work (otherwise, they can't use
the feature)

So, I think it's OK to fix spec to directly show implementation, that was here
since feature introducing.

> 
>> Fixes: 5fcbeb06812685a2
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>
>> Hi all.
>>
>> This patch follows discussion on Nir's patch
>>   [PATCH] block: nbd: Fix dirty bitmap context name
>>   ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html )
>>
>> Let's just fix qapi spec now.
> 
> But qapi documents a better behavior for users. We should fix the code instead
> to mach the docs.

1. Using disk name as a bitmap name is a bad behavior, as they are completely
different concepts. Especially keeping in mind that user already knows disk 
name anyway
and no reason to write this export name inside metadata context of this export.

2. It's not directly documented. You assume that NAME == @name. I understand 
that
it may be assumed.. But it's not documented.

3. It's never worked like you write. So if we change the behavior, we'll break
existing users.


> 
> With this we still have the issue of leaking internal bitmap name to
> users who do not
> control the name, and do not care about it.
> 
>>   qapi/block.json | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/block.json b/qapi/block.json
>> index 145c268bb6..8042ef78f0 100644
>> --- a/qapi/block.json
>> +++ b/qapi/block.json
>> @@ -255,7 +255,8 @@
>>
>>   # @bitmap: Also export the dirty bitmap reachable from @device, so the
>>   #          NBD client can use NBD_OPT_SET_META_CONTEXT with
>> -#          "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
>> +#          "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here
>> +#          matches @bitmap parameter). (since 4.0)
>>   #
>>   # Returns: error if the server is not running, or export with the same name
>>   #          already exists.
>> --
>> 2.21.0
>>
> 


-- 
Best regards,
Vladimir