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

[Nir Soffer]

On Thu, Dec 19, 2019 at 5:25 PM Vladimir Sementsov-Ogievskiy
<address@hidden> wrote:
>
> 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?

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