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

[Vladimir Sementsov-Ogievskiy]

19.12.2019 18:08, Nir Soffer wrote:
> On Thu, Dec 19, 2019 at 5:00 PM Vladimir Sementsov-Ogievskiy
> <address@hidden> wrote:
>>
>> 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.
> 
> The different concept is expressed by the "qemu:dirty-bitmap:" prefix.
> "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export.

Why do you think so? Did you read NBD specification?

Metadata context is always owned by some export. Do you mean that there will be
metadata contexts

qemu:dirty-bitmap:export-A
qemu:dirty-bitmap:export-B

both defined for export-A?

> 
>> 2. It's not directly documented. You assume that NAME == @name. I understand 
>> that
>> it may be assumed.. But it's not documented.
> 
> But NAME is likely to be understood as the name argument, and unlikely to be 
> the
> bitmap name.

Yes likely. But it's still bad specification, which should be fixed.

> 
>> 3. It's never worked like you write. So if we change the behavior, we'll 
>> break
>> existing users.
> 
> Do we have existing users? isn't this new feature in 4.2?

No, it's since 4.0

> 
> Before we had experimental x-block-dirty-bitmap APIs, which are stable, so 
> users
> could not depend on them.
> 
>>> 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
> 


-- 
Best regards,
Vladimir