On Thu, Dec 19, 2019 at 5:25 PM Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> wrote: > > 19.12.2019 18:08, Nir Soffer wrote: > > On Thu, Dec 19, 2019 at 5:00 PM Vladimir Sementsov-Ogievskiy > > <vsement...@virtuozzo.com> wrote: > >> > >> 19.12.2019 17:42, Nir Soffer wrote: > >>> On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy > >>> <vsement...@virtuozzo.com> 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 <vsement...@virtuozzo.com> > >>>> --- > >>>> > >>>> 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?
Yes - the name of the bitmap does not have any meaning. But for nbd_server_add we allow only single bitmap for export. > 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. > >> 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. > > > >> 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
