I'd not call it a "fix".. As it implies something broken. [edit: OK, now I see that something is broken, and why you called it "fix", see below]
19.12.2019 15:51, Nir Soffer wrote: > When adding an export with a dirty bitmap, expose the bitmap at: > > qemu:dirty-bitmap:export-name export-name? But it would be extra information, as client already knows with which export it works. NBD commands NBD_OPT_GET/SET_META_CONTEXT includes export name as a parameter, so, any queried metadata (bitmaps, etc) is always bound to specified export. > > This matches qapi documentation, and user expectations. Hmmm, "qemu" namespace is documented in docs/interop/nbd.txt, not in Qapi, which is also mention in official NBD spec. Ahh, I see, it's documented as +# @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) and it is logical to assume that export name (which is @name argument) is mentioned. But we never mentioned it. This is just documented after removed experimenatl command x-nbd-server-add-bitmap, look at commit 7dc570b3806e5b0a4c9219061556ed5a4a0de80c Author: Eric Blake <[email protected]> Date: Fri Jan 11 13:47:18 2019 -0600 nbd: Remove x-nbd-server-add-bitmap ... -# @bitmap-export-name: How the bitmap will be seen by nbd clients -# (default @bitmap) -# -# Note: the client must use NBD_OPT_SET_META_CONTEXT with a query of -# "qemu:dirty-bitmap:NAME" (where NAME matches @bitmap-export-name) to access -# the exposed bitmap. So, this "NAME" is saved and now looks incorrect. What should be fixed, is Qapi documentation. > > Without this, qemu leaks libvirt implementations details to clients by > exposing the bitmap using the actual bitmap name: > > qemu:dirty-bitmap:bitmap-name Yes, "qemu" namespace specification says: qemu:dirty-bitmap:<dirty-bitmap-export-name> so, <dirty-bitmap-export-name> may be exact bitmap name or may be something other. We just don't have an interface to set such name. It was in removed x-nbd-server-add-bitmap So, if you need this possibility now, the correct way is to add 'export-bitmap-name' optional parameter to nbd-server-add, like it was in x-nbd-server-add-bitmap > > And all clients need to duplicate code like: > > meta_context = "qemu:dirty-bitmap:backup-" + export_name > > NBD allows exposing multiple bitmaps under "qemu:dirty-bitmap:" > namespace, and clients can query the available bitmaps, but it is not > clear what a client should do if a server provides multiple bitmaps. > --- > nbd/server.c | 2 +- > tests/qemu-iotests/223 | 16 ++++++++-------- > tests/qemu-iotests/223.out | 8 ++++---- > 3 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/nbd/server.c b/nbd/server.c > index 24ebc1a805..f20f2994c0 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -1574,7 +1574,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, > uint64_t dev_offset, > exp->export_bitmap = bm; > assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE); > exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s", > - bitmap); > + name); I think it's a bad idea to automatically name bitmap after export. Actually export may have several bitmaps (we just don't support it). "NAME" in Qapi spec is a mistake. > assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE); > } > > diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223 > index ea69cd4b8b..3068a7c280 100755 > --- a/tests/qemu-iotests/223 > +++ b/tests/qemu-iotests/223 > @@ -167,7 +167,7 @@ $QEMU_IO -r -c 'r -P 0x22 512 512' -c 'r -P 0 512k 512k' > -c 'r -P 0x11 1m 1m' \ > $QEMU_IMG map --output=json --image-opts \ > "$IMG" | _filter_qemu_img_map > $QEMU_IMG map --output=json --image-opts \ > - "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map > + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map > > echo > echo "=== Contrast to small granularity dirty-bitmap ===" > @@ -175,7 +175,7 @@ echo > > IMG="driver=nbd,export=n2,server.type=unix,server.path=$SOCK_DIR/nbd" > $QEMU_IMG map --output=json --image-opts \ > - "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map > + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n2" | _filter_qemu_img_map > > echo > echo "=== End qemu NBD server ===" > @@ -199,15 +199,15 @@ echo > echo "=== Use qemu-nbd as server ===" > echo > > -nbd_server_start_unix_socket -r -f $IMGFMT -B b "$TEST_IMG" > -IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket" > +nbd_server_start_unix_socket -r -f $IMGFMT -x n -B b "$TEST_IMG" > +IMG="driver=nbd,export=n,server.type=unix,server.path=$nbd_unix_socket" > $QEMU_IMG map --output=json --image-opts \ > - "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map > + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map > > -nbd_server_start_unix_socket -f $IMGFMT -B b2 "$TEST_IMG" > -IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket" > +nbd_server_start_unix_socket -f $IMGFMT -x n -B b2 "$TEST_IMG" > +IMG="driver=nbd,export=n,server.type=unix,server.path=$nbd_unix_socket" > $QEMU_IMG map --output=json --image-opts \ > - "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map > + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map > > # success, all done > echo '*** done' > diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out > index f175598802..9f879add60 100644 > --- a/tests/qemu-iotests/223.out > +++ b/tests/qemu-iotests/223.out > @@ -61,7 +61,7 @@ exports available: 2 > max block: 33554432 > available meta contexts: 2 > base:allocation > - qemu:dirty-bitmap:b > + qemu:dirty-bitmap:n > export: 'n2' > size: 4194304 > flags: 0xced ( flush fua trim zeroes df cache fast-zero ) > @@ -70,7 +70,7 @@ exports available: 2 > max block: 33554432 > available meta contexts: 2 > base:allocation > - qemu:dirty-bitmap:b2 > + qemu:dirty-bitmap:n2 > > === Contrast normal status to large granularity dirty-bitmap === > > @@ -141,7 +141,7 @@ exports available: 2 > max block: 33554432 > available meta contexts: 2 > base:allocation > - qemu:dirty-bitmap:b > + qemu:dirty-bitmap:n > export: 'n2' > size: 4194304 > flags: 0xced ( flush fua trim zeroes df cache fast-zero ) > @@ -150,7 +150,7 @@ exports available: 2 > max block: 33554432 > available meta contexts: 2 > base:allocation > - qemu:dirty-bitmap:b2 > + qemu:dirty-bitmap:n2 > > === Contrast normal status to large granularity dirty-bitmap === > > -- Best regards, Vladimir
