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

[Nir Soffer]

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.

> 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.

> 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?

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