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

[Vladimir Sementsov-Ogievskiy]

21.12.2019 1:04, Eric Blake wrote:
> On 12/19/19 10:14 AM, Nir Soffer wrote:
> 
>>>>> 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?
>>
>> Yes - the name of the bitmap does not have any meaning.
>> But for nbd_server_add we allow only single bitmap for export.
> 
> Just because qemu is currently limited to only exposing one bitmap at the 
> moment does not mean that a future version can't expose multiple bitmaps. It 
> may very well be that we have reason to expose both "qemu:dirty-bitmap:timeA" 
> and "qemu:dirty-bitmap:timeB" on the same export, for exposing two bitmaps at 
> once.  To get to that point, we'd have to refactor the QAPI command to allow 
> attaching more than one bitmap at the time of creating the NBD export, but 
> it's not impossible.
> 
>>
>>> Metadata context is always owned by some export.
>>
>> Of course.
>>
>>> Do you mean that there will bemetadata contexts
>>>
>>> qemu:dirty-bitmap:export-A
>>> qemu:dirty-bitmap:export-B
>>>
>>> both defined for export-A?
>>
>> It does not make sense, but it is valid.
> 
> If an image has multiple bitmaps, exposing all of those as separate contexts 
> at the same time for a single export can indeed make sense.
> 
>>
>>>>> 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.
>>
>> If we cannot change the current behavior since it will break current users,
>> I agree fixing the spec to describe the current behavior is a good idea.
> 
> We need the doc fix. 

Do you like the patch starting this thread? It's a fix we are talking about..

> Whether we also want an additional fix adding an optional parameter allowing 
> user control over the export name is also under debate (the fact that the old 
> x-nbd-server-add-bitmap supported it shows that it may be useful, but it is 
> not minimal, and as I pointed out at the time of removing x-, libvirt can 
> always control what name is exposed by creating a temporary bitmap and 
> merging from other bitmaps into the temporary).
> 
> We also have to think about a future of parallel backup jobs: libvirt can 
> create a single temporary bitmap to expose whatever name it wants under one 
> job, but if libvirt wants to expose the SAME user-visible name to two 
> parallel jobs, it cannot create two bitmaps with the same name, so having a 
> way to set the user-visible name of an arbitrary bitmap when producing the 
> NBD export makes sense on that front.
> 
> 
>>>>
>>>>> 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
> 
> As long as altering the exported name is controlled by a new optional 
> parameter, it does not hurt older 4.0 clients that do not use the new 
> parameter.
> 


-- 
Best regards,
Vladimir