Re: [PATCH] block: nbd: Fix dirty bitmap context name

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
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