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