diff mbox series

[v4,4/7] nbd: Update qapi to support exporting multiple bitmaps

Message ID 20201009215533.1194742-5-eblake@redhat.com
State Superseded
Headers show
Series Exposing backing-chain allocation over NBD | expand

Commit Message

Eric Blake Oct. 9, 2020, 9:55 p.m. UTC
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>
---
 qapi/block-export.json | 18 ++++++++++++------
 blockdev-nbd.c         | 14 ++++++++++++++
 nbd/server.c           | 19 +++++++++++++------
 qemu-nbd.c             | 13 ++++++++-----
 4 files changed, 47 insertions(+), 17 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Oct. 14, 2020, 12:15 p.m. UTC | #1
10.10.2020 00:55, 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>

> ---


[..]

>               break;

>           case 'B':

> -            bitmap = optarg;

> +            tmp = g_new(strList, 1);

> +            tmp->value = g_strdup(optarg);

> +            tmp->next = bitmaps;

> +            bitmaps = tmp;


If publish QAPI_LIST_ADD, defined in block.c, it would look like:

     QAPI_LIST_ADD(bitmaps, g_strdup(optarg));

anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir
Eric Blake Oct. 19, 2020, 9:45 p.m. UTC | #2
On 10/14/20 7:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> 10.10.2020 00:55, 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>
>> ---
> 
> [..]
> 
>>               break;
>>           case 'B':
>> -            bitmap = optarg;
>> +            tmp = g_new(strList, 1);
>> +            tmp->value = g_strdup(optarg);
>> +            tmp->next = bitmaps;
>> +            bitmaps = tmp;
> 
> If publish QAPI_LIST_ADD, defined in block.c, it would look like:
> 
>      QAPI_LIST_ADD(bitmaps, g_strdup(optarg));

#define QAPI_LIST_ADD(list, element) do { \
     typeof(list) _tmp = g_new(typeof(*(list)), 1); \
     _tmp->value = (element); \
     _tmp->next = (list); \
     (list) = _tmp; \
} while (0)


Markus, thoughts on if we should publish this macro, and if so, which 
header would be best?


> 
> anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
Markus Armbruster Oct. 20, 2020, 8:51 a.m. UTC | #3
Eric Blake <eblake@redhat.com> writes:

> On 10/14/20 7:15 AM, Vladimir Sementsov-Ogievskiy wrote:

>> 10.10.2020 00:55, 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>

>>> ---

>> [..]

>> 

>>>               break;

>>>           case 'B':

>>> -            bitmap = optarg;

>>> +            tmp = g_new(strList, 1);

>>> +            tmp->value = g_strdup(optarg);

>>> +            tmp->next = bitmaps;

>>> +            bitmaps = tmp;

>> If publish QAPI_LIST_ADD, defined in block.c, it would look like:

>>      QAPI_LIST_ADD(bitmaps, g_strdup(optarg));

>

> #define QAPI_LIST_ADD(list, element) do { \

>     typeof(list) _tmp = g_new(typeof(*(list)), 1); \

>     _tmp->value = (element); \

>     _tmp->next = (list); \

>     (list) = _tmp; \

> } while (0)

>

>

> Markus, thoughts on if we should publish this macro,


If it's widely useful.

"git-grep -- '->value ='" matches ~200 times.  A patch converting these
to the macro where possible would make a strong case for having the
macro.

>                                                      and if so, which

> header would be best?


The macro is generic: @list's type may be any of the struct TYPEList we
generate for the QAPI type ['TYPE'].

We don't want to generate this macro next to each of these struct
definitions.  A non-generic macro would go there, but let's not generate
almost a hundred non-generic macros where a single generic one can do
the job.

The closest we have to a common base is GenericList.  It's in in
visitor.h because it's only used by visitors so far.  Adding the macro
next it is not so smart, because we don't want non-visitor code to
include visitor.h just for this macro.

Perhaps the macro should go into qapi/util.h, and perhaps GenericList
and GenericAlternate should move there, too.


[...]
Eric Blake Oct. 20, 2020, 6:26 p.m. UTC | #4
On 10/20/20 3:51 AM, Markus Armbruster wrote:

>> #define QAPI_LIST_ADD(list, element) do { \

>>     typeof(list) _tmp = g_new(typeof(*(list)), 1); \

>>     _tmp->value = (element); \

>>     _tmp->next = (list); \

>>     (list) = _tmp; \

>> } while (0)

>>

>>

>> Markus, thoughts on if we should publish this macro,

> 

> If it's widely useful.

> 

> "git-grep -- '->value ='" matches ~200 times.  A patch converting these

> to the macro where possible would make a strong case for having the

> macro.

> 

>>                                                      and if so, which

>> header would be best?

> 

> The macro is generic: @list's type may be any of the struct TYPEList we

> generate for the QAPI type ['TYPE'].

> 

> We don't want to generate this macro next to each of these struct

> definitions.  A non-generic macro would go there, but let's not generate

> almost a hundred non-generic macros where a single generic one can do

> the job.


Agreed.

> 

> The closest we have to a common base is GenericList.  It's in in

> visitor.h because it's only used by visitors so far.  Adding the macro

> next it is not so smart, because we don't want non-visitor code to

> include visitor.h just for this macro.


Also agreed.

> 

> Perhaps the macro should go into qapi/util.h, and perhaps GenericList

> and GenericAlternate should move there, too.


GenericList is easy, but GenericAlternate is harder: it would introduce
a cyclic declaration dependency (generated qapi-builtin-types.h includes
qapi/util.h for the definition of QEnumLookup, but qapi/util.h declaring
GenericAlternate would depend on including qapi-builtin-types.h for the
definition of QType).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
Markus Armbruster Oct. 21, 2020, 4:19 a.m. UTC | #5
Eric Blake <eblake@redhat.com> writes:

> On 10/20/20 3:51 AM, Markus Armbruster wrote:
>
>>> #define QAPI_LIST_ADD(list, element) do { \
>>>     typeof(list) _tmp = g_new(typeof(*(list)), 1); \
>>>     _tmp->value = (element); \
>>>     _tmp->next = (list); \
>>>     (list) = _tmp; \
>>> } while (0)
>>>
>>>
>>> Markus, thoughts on if we should publish this macro,
>> 
>> If it's widely useful.
>> 
>> "git-grep -- '->value ='" matches ~200 times.  A patch converting these
>> to the macro where possible would make a strong case for having the
>> macro.
>> 
>>>                                                      and if so, which
>>> header would be best?
>> 
>> The macro is generic: @list's type may be any of the struct TYPEList we
>> generate for the QAPI type ['TYPE'].
>> 
>> We don't want to generate this macro next to each of these struct
>> definitions.  A non-generic macro would go there, but let's not generate
>> almost a hundred non-generic macros where a single generic one can do
>> the job.
>
> Agreed.
>
>> 
>> The closest we have to a common base is GenericList.  It's in in
>> visitor.h because it's only used by visitors so far.  Adding the macro
>> next it is not so smart, because we don't want non-visitor code to
>> include visitor.h just for this macro.
>
> Also agreed.
>
>> 
>> Perhaps the macro should go into qapi/util.h, and perhaps GenericList
>> and GenericAlternate should move there, too.
>
> GenericList is easy, but GenericAlternate is harder: it would introduce
> a cyclic declaration dependency (generated qapi-builtin-types.h includes
> qapi/util.h for the definition of QEnumLookup, but qapi/util.h declaring
> GenericAlternate would depend on including qapi-builtin-types.h for the
> definition of QType).

You're right.

QType is a built-in QAPI type.  The typedef is generated into
qapi-builtin-types.h.

It needs to be a QAPI type because it's the type of alternates'
(implicit) member @type.

I figure the easiest way to move GenericAlternate (if we want to), is
creating a new header, or rather splitting qapi/util.h into the part
needed by qapi-builtin-types.h and the part that needs
qapi-builtin-types.h.

Doesn't seem to be worth our while now.  We can simply put the macro
into qapi/util.h and call it a day.
diff mbox series

Patch

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)
 #
 # @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' } }

 ##
 # @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.
+#
 # Since: 5.0
 ##
 { 'struct': 'NbdServerAddOptions',
   'base': 'BlockExportOptionsNbd',
   'data': { 'device': 'str',
-            '*writable': 'bool' } }
+            '*writable': 'bool', '*bitmap': 'str' } }

 ##
 # @nbd-server-add:
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 43856c1058a5..359a198de2c7 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -192,6 +192,20 @@  void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
         return;
     }

+    /*
+     * New code should use the list 'bitmaps'; but until this code is
+     * gone, we must support the older single 'bitmap'.  Use only one.
+     */
+    if (arg->has_bitmap) {
+        if (arg->has_bitmaps) {
+            error_setg(errp, "Can't mix 'bitmap' and 'bitmaps'");
+            return;
+        }
+        arg->has_bitmaps = true;
+        arg->bitmaps = g_new0(strList, 1);
+        arg->bitmaps->value = g_strdup(arg->bitmap);
+    }
+
     /*
      * block-export-add would default to the node-name, but we may have to use
      * the device name as a default here for compatibility.
diff --git a/nbd/server.c b/nbd/server.c
index 30cfe0eee467..884ffa00f1bd 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1495,6 +1495,7 @@  static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
     uint64_t perm, shared_perm;
     bool readonly = !exp_args->writable;
     bool shared = !exp_args->writable;
+    strList *bitmaps;
     int ret;

     assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
@@ -1556,12 +1557,18 @@  static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
     }
     exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);

-    if (arg->bitmap) {
+    /* XXX Allow more than one bitmap */
+    if (arg->bitmaps && arg->bitmaps->next) {
+        error_setg(errp, "multiple bitmaps per export not supported yet");
+        return -EOPNOTSUPP;
+    }
+    for (bitmaps = arg->bitmaps; bitmaps; bitmaps = bitmaps->next) {
+        const char *bitmap = bitmaps->value;
         BlockDriverState *bs = blk_bs(blk);
         BdrvDirtyBitmap *bm = NULL;

         while (bs) {
-            bm = bdrv_find_dirty_bitmap(bs, arg->bitmap);
+            bm = bdrv_find_dirty_bitmap(bs, bitmap);
             if (bm != NULL) {
                 break;
             }
@@ -1571,7 +1578,7 @@  static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,

         if (bm == NULL) {
             ret = -ENOENT;
-            error_setg(errp, "Bitmap '%s' is not found", arg->bitmap);
+            error_setg(errp, "Bitmap '%s' is not found", bitmap);
             goto fail;
         }

@@ -1585,15 +1592,15 @@  static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
             ret = -EINVAL;
             error_setg(errp,
                        "Enabled bitmap '%s' incompatible with readonly export",
-                       arg->bitmap);
+                       bitmap);
             goto fail;
         }

         bdrv_dirty_bitmap_set_busy(bm, true);
         exp->export_bitmap = bm;
-        assert(strlen(arg->bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
+        assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
         exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
-                                                     arg->bitmap);
+                                                     bitmap);
         assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
     }

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 847fde435a7f..fa5c68749e8f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -572,7 +572,7 @@  int main(int argc, char **argv)
     const char *export_name = NULL; /* defaults to "" later for server mode */
     const char *export_description = NULL;
     bool alloc_depth = false;
-    const char *bitmap = NULL;
+    strList *bitmaps = NULL, *tmp;
     const char *tlscredsid = NULL;
     bool imageOpts = false;
     bool writethrough = true;
@@ -701,7 +701,10 @@  int main(int argc, char **argv)
             alloc_depth = true;
             break;
         case 'B':
-            bitmap = optarg;
+            tmp = g_new(strList, 1);
+            tmp->value = g_strdup(optarg);
+            tmp->next = bitmaps;
+            bitmaps = tmp;
             break;
         case 'k':
             sockpath = optarg;
@@ -798,7 +801,7 @@  int main(int argc, char **argv)
         }
         if (export_name || export_description || dev_offset ||
             device || disconnect || fmt || sn_id_or_name || alloc_depth ||
-            bitmap || seen_aio || seen_discard || seen_cache) {
+            bitmaps || seen_aio || seen_discard || seen_cache) {
             error_report("List mode is incompatible with per-device settings");
             exit(EXIT_FAILURE);
         }
@@ -1082,8 +1085,8 @@  int main(int argc, char **argv)
             .name                 = g_strdup(export_name),
             .has_description      = !!export_description,
             .description          = g_strdup(export_description),
-            .has_bitmap           = !!bitmap,
-            .bitmap               = g_strdup(bitmap),
+            .has_bitmaps          = !!bitmaps,
+            .bitmaps              = bitmaps,
             .has_allocation_depth = alloc_depth,
             .allocation_depth     = alloc_depth,
         },