Re: [PATCH v5 06/12] nbd: Update qapi to support exporting multiple bitmaps

[Peter Krempa]

On Fri, Oct 23, 2020 at 13:36:46 -0500, Eric Blake wrote:
> Since 'nbd-server-add' is deprecated, and 'block-export-add' is new to
> 5.2, we can still tweak the interface.  Allowing 'bitmaps':['str'] is
> nicer than 'bitmap':'str'.  This wires up the qapi and qemu-nbd
> changes to permit passing multiple bitmaps as distinct metadata
> contexts that the NBD client may request, but the actual support for
> more than one will require a further patch to the server.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/system/deprecated.rst |  4 +++-
>  qapi/block-export.json     | 18 ++++++++++++------
>  blockdev-nbd.c             | 13 +++++++++++++
>  nbd/server.c               | 19 +++++++++++++------
>  qemu-nbd.c                 | 10 +++++-----
>  5 files changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 905628f3a0cb..d6cd027ac740 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -268,7 +268,9 @@ the 'wait' field, which is only applicable to sockets in 
> server mode
>  ''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> 
>  Use the more generic commands ``block-export-add`` and ``block-export-del``
> -instead.
> +instead.  As part of this deprecation, it is now preferred to export a
> +list of dirty bitmaps via ``bitmaps``, rather than a single bitmap via
> +``bitmap``.
> 
>  Human Monitor Protocol (HMP) commands
>  -------------------------------------
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index 893d5cde5dfe..c7c749d61097 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -74,10 +74,10 @@
>  # @description: Free-form description of the export, up to 4096 bytes.
>  #               (Since 5.0)
>  #
> -# @bitmap: Also export the dirty bitmap reachable from @device, so the
> -#          NBD client can use NBD_OPT_SET_META_CONTEXT with the
> -#          metadata context name "qemu:dirty-bitmap:NAME" to inspect the
> -#          bitmap. (since 4.0)
> +# @bitmaps: Also export each of the named dirty bitmaps reachable from
> +#           @device, so the NBD client can use NBD_OPT_SET_META_CONTEXT with
> +#           the metadata context name "qemu:dirty-bitmap:BITMAP" to inspect
> +#           each bitmap. (since 5.2)

Given unsynchronised release cycles between qemu and management apps
it's not cool to deprecate an interface without having at least one
release where the replacement interface is already stable.

This means that any project wanting to stay up to date will either have
to use deprecated interfaces for at least one release and develop the
replacement only when there's a stable interface or hope that they don't
have to change the interfaces too often.

This specifically impacts libvirt as we have validators which notify us
that a deprecated interface is used and we want to stay in sync, so that
there's one less group of users to worry about at the point qemu will
want to delete the interface.

>  # @allocation-depth: Also export the allocation depth map for @device, so
>  #                    the NBD client can use NBD_OPT_SET_META_CONTEXT with
> @@ -88,7 +88,7 @@
>  ##
>  { 'struct': 'BlockExportOptionsNbd',
>    'data': { '*name': 'str', '*description': 'str',
> -            '*bitmap': 'str', '*allocation-depth': 'bool' } }
> +            '*bitmaps': ['str'], '*allocation-depth': 'bool' } }

This adds 'bitmaps' also to nbd-server-add, which should not happen. You
probably want to stop using 'base' for 'NbdServerAddOptions' and just
duplicate everything else.

>  # @NbdServerAddOptions:
> @@ -100,12 +100,18 @@
>  # @writable: Whether clients should be able to write to the device via the
>  #            NBD connection (default false).
>  #
> +# @bitmap: Also export a single dirty bitmap reachable from @device, so the
> +#          NBD client can use NBD_OPT_SET_META_CONTEXT with the metadata
> +#          context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap
> +#          (since 4.0).  Mutually exclusive with @bitmaps, and newer
> +#          clients should use that instead.

This doesn't make sense, nbd-server-add never had @bitmaps. Also adding
features to a deprecated interface doesn't IMO make sense if you want to
motivate users switch to thne new one.

> +#
>  # Since: 5.0
>  ##
>  { 'struct': 'NbdServerAddOptions',
>    'base': 'BlockExportOptionsNbd',
>    'data': { 'device': 'str',
> -            '*writable': 'bool' } }
> +            '*writable': 'bool', '*bitmap': 'str' } }
> 
>  ##
>  # @nbd-server-add: