[PATCH v4 0/7] Exposing backing-chain allocation over NBD

[PATCH v4 0/7] Exposing backing-chain allocation over NBD

[Eric Blake]

v3 was here:
https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02305.html

Since then:
rebase to master
- patch 1 is new, fixing a theoretical bug in QAPI interaction and
 simplifying later patches
- patch 2, 4, and 6 are renamed to favor the term 'metadata context'
 [Markus], sadly 'git backport-diff' can't see through renames

Based-on: <20201009200720.1169904-1-eblake@redhat.com>
([PULL v3 0/8] NBD patches through 2020-10-08)

Also available at:
https://repo.or.cz/qemu/ericb.git/shortlog/refs/tags/nbd-alloc-depth-v4

001/7:[down] 'nbd: Utilize QAPI_CLONE for type conversion'
002/7:[down] 'nbd: Add new qemu:allocation-depth metadata context'
003/7:[0042] [FC] 'nbd: Add 'qemu-nbd -A' to expose allocation depth'
004/7:[down] 'nbd: Update qapi to support exporting multiple bitmaps'
005/7:[----] [-C] 'nbd: Simplify qemu bitmap context name'
006/7:[down] 'nbd: Refactor counting of metadata contexts'
007/7:[----] [-C] 'nbd: Allow export of multiple bitmaps for one device'

Eric Blake (7):
  nbd: Utilize QAPI_CLONE for type conversion
  nbd: Add new qemu:allocation-depth metadata context
  nbd: Add 'qemu-nbd -A' to expose allocation depth
  nbd: Update qapi to support exporting multiple bitmaps
  nbd: Simplify qemu bitmap context name
  nbd: Refactor counting of metadata contexts
  nbd: Allow export of multiple bitmaps for one device

 docs/interop/nbd.txt       |  27 ++++-
 docs/tools/qemu-nbd.rst    |   8 +-
 qapi/block-core.json       |   7 +-
 qapi/block-export.json     |  22 +++-
 include/block/nbd.h        |  12 ++-
 blockdev-nbd.c             |  29 +++--
 nbd/server.c               | 210 +++++++++++++++++++++++++++++--------
 qemu-nbd.c                 |  33 ++++--
 tests/qemu-iotests/291     |   6 +-
 tests/qemu-iotests/309     |  73 +++++++++++++
 tests/qemu-iotests/309.out |  22 ++++
 tests/qemu-iotests/group   |   1 +
 12 files changed, 364 insertions(+), 86 deletions(-)
 create mode 100755 tests/qemu-iotests/309
 create mode 100644 tests/qemu-iotests/309.out

-- 
2.28.0

[PATCH v4 1/7] nbd: Utilize QAPI_CLONE for type conversion

[Eric Blake]

Rather than open-coding the translation from the deprecated
NbdServerAddOptions type to the preferred BlockExportOptionsNbd, it's
better to utilize QAPI_CLONE_MEMBERS.  This solves a couple of issues:
first, if we do any more refactoring of the base type (which an
upcoming patch plans to do), we don't have to revisit the open-coding.
Second, our assignment to arg->name is fishy: the generated QAPI code
currently does not visit it if arg->has_name is false, but if it DID
visit it, we would have introduced a double-free situation when arg is
finally freed.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 blockdev-nbd.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 8174023e5c47..43856c1058a5 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -14,6 +14,8 @@
 #include "sysemu/block-backend.h"
 #include "hw/block/block.h"
 #include "qapi/error.h"
+#include "qapi/clone-visitor.h"
+#include "qapi/qapi-visit-block-export.h"
 #include "qapi/qapi-commands-block-export.h"
 #include "block/nbd.h"
 #include "io/channel-socket.h"
@@ -195,7 +197,8 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
      * the device name as a default here for compatibility.
      */
     if (!arg->has_name) {
-        arg->name = arg->device;
+        arg->has_name = true;
+        arg->name = g_steal_pointer(&arg->device);
     }

     export_opts = g_new(BlockExportOptions, 1);
@@ -205,15 +208,9 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
         .node_name              = g_strdup(bdrv_get_node_name(bs)),
         .has_writable           = arg->has_writable,
         .writable               = arg->writable,
-        .u.nbd = {
-            .has_name           = true,
-            .name               = g_strdup(arg->name),
-            .has_description    = arg->has_description,
-            .description        = g_strdup(arg->description),
-            .has_bitmap         = arg->has_bitmap,
-            .bitmap             = g_strdup(arg->bitmap),
-        },
     };
+    QAPI_CLONE_MEMBERS(BlockExportOptionsNbd, &export_opts->u.nbd,
+                       qapi_NbdServerAddOptions_base(arg));

     /*
      * nbd-server-add doesn't complain when a read-only device should be
-- 
2.28.0

[PATCH v4 2/7] nbd: Add new qemu:allocation-depth metadata context

[Eric Blake]

'qemu-img map' provides a way to determine which extents of an image
come from the top layer vs. inherited from a backing chain.  This is
useful information worth exposing over NBD.  There is a proposal to
add a QMP command block-dirty-bitmap-populate which can create a dirty
bitmap that reflects allocation information, at which point the
qemu:dirty-bitmap:NAME metadata context can expose that information
via the creation of a temporary bitmap, but we can shorten the effort
by adding a new qemu:allocation-depth metadata context that does the
same thing without an intermediate bitmap (this patch does not
eliminate the need for that proposal, as it will have other uses as
well).

For this patch, I just encoded a tri-state value (unallocated, from
this layer, from any of the backing layers); an obvious extension
would be to provide the actual depth in bits 31-4 while keeping bits
1-0 as a tri-state (leaving bits 3-2 unused, for ease of reading depth
from a hex number).  But adding this extension would require
bdrv_is_allocated_above to return a depth number.

While documenting things, remember that although the NBD protocol has
NBD_OPT_SET_META_CONTEXT, the rest of its documentation refers to
'metadata context', which is a more apt description of what is
actually being used by NBD_CMD_BLOCK_STATUS: the user is requesting
metadata by passing one or more context names.  So I also touched up
some existing wording to prefer the term 'metadata context' where it
makes sense.

Note that this patch does not actually enable any way to request a
server to enable this context; that will come in the next patch.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/interop/nbd.txt | 27 ++++++++++---
 include/block/nbd.h  | 12 ++++--
 nbd/server.c         | 92 ++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 116 insertions(+), 15 deletions(-)

diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
index f3b3cacc9621..a55e5a8776c8 100644
--- a/docs/interop/nbd.txt
+++ b/docs/interop/nbd.txt
@@ -17,19 +17,35 @@ namespace "qemu".

 == "qemu" namespace ==

-The "qemu" namespace currently contains only one type of context,
-related to exposing the contents of a dirty bitmap alongside the
-associated disk contents.  That context has the following form:
+The "qemu" namespace currently contains two available metadata context
+types.  The first is related to exposing the contents of a dirty
+bitmap alongside the associated disk contents.  That metadata context
+is named with the following form:

     qemu:dirty-bitmap:<dirty-bitmap-export-name>

 Each dirty-bitmap metadata context defines only one flag for extents
 in reply for NBD_CMD_BLOCK_STATUS:

-    bit 0: NBD_STATE_DIRTY, means that the extent is "dirty"
+    bit 0: NBD_STATE_DIRTY, set when the extent is "dirty"
+
+The second is related to exposing the source of various extents within
+the image, with a single metadata context named:
+
+    qemu:allocation-depth
+
+In the allocation depth context, bits 0 and 1 form a tri-state value:
+
+    bits 0-1: 00: NBD_STATE_DEPTH_UNALLOC, the extent is unallocated
+              01: NBD_STATE_DEPTH_LOCAL, the extent is allocated in the
+                  top level of the image
+              10: NBD_STATE_DEPTH_BACKING, the extent is inherited from a
+                  backing layer
+              11: invalid, never returned

 For NBD_OPT_LIST_META_CONTEXT the following queries are supported
-in addition to "qemu:dirty-bitmap:<dirty-bitmap-export-name>":
+in addition to the specific "qemu:allocation-depth" and
+"qemu:dirty-bitmap:<dirty-bitmap-export-name>":

 * "qemu:" - returns list of all available metadata contexts in the
             namespace.
@@ -55,3 +71,4 @@ the operation of that feature.
 NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE
 * 4.2: NBD_FLAG_CAN_MULTI_CONN for shareable read-only exports,
 NBD_CMD_FLAG_FAST_ZERO
+* 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth"
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 3dd9a04546ec..0bbf92f02951 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -1,5 +1,5 @@
 /*
- *  Copyright (C) 2016-2019 Red Hat, Inc.
+ *  Copyright (C) 2016-2020 Red Hat, Inc.
  *  Copyright (C) 2005  Anthony Liguori <anthony@codemonkey.ws>
  *
  *  Network Block Device
@@ -47,7 +47,7 @@ typedef struct NBDOptionReply NBDOptionReply;
 typedef struct NBDOptionReplyMetaContext {
     NBDOptionReply h; /* h.type = NBD_REP_META_CONTEXT, h.length > 4 */
     uint32_t context_id;
-    /* meta context name follows */
+    /* metadata context name follows */
 } QEMU_PACKED NBDOptionReplyMetaContext;

 /* Transmission phase structs
@@ -229,7 +229,7 @@ enum {
 #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)

 /*
- * Maximum size of a protocol string (export name, meta context name,
+ * Maximum size of a protocol string (export name, metadata context name,
  * etc.).  Use malloc rather than stack allocation for storage of a
  * string.
  */
@@ -259,6 +259,12 @@ enum {
 /* Extent flags for qemu:dirty-bitmap in NBD_REPLY_TYPE_BLOCK_STATUS */
 #define NBD_STATE_DIRTY (1 << 0)

+/* Extent flags for qemu:allocation-depth in NBD_REPLY_TYPE_BLOCK_STATUS */
+#define NBD_STATE_DEPTH_MASK    0x3
+#define NBD_STATE_DEPTH_UNALLOC 0x0
+#define NBD_STATE_DEPTH_LOCAL   0x1
+#define NBD_STATE_DEPTH_BACKING 0x2
+
 static inline bool nbd_reply_type_is_error(int type)
 {
     return type & (1 << 15);
diff --git a/nbd/server.c b/nbd/server.c
index e75c825879aa..ae6f8a8e5429 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -27,7 +27,8 @@
 #include "qemu/units.h"

 #define NBD_META_ID_BASE_ALLOCATION 0
-#define NBD_META_ID_DIRTY_BITMAP 1
+#define NBD_META_ID_ALLOCATION_DEPTH 1
+#define NBD_META_ID_DIRTY_BITMAP 2

 /*
  * NBD_MAX_BLOCK_STATUS_EXTENTS: 1 MiB of extents data. An empirical
@@ -94,6 +95,7 @@ struct NBDExport {
     BlockBackend *eject_notifier_blk;
     Notifier eject_notifier;

+    bool allocation_depth;
     BdrvDirtyBitmap *export_bitmap;
     char *export_bitmap_context;
 };
@@ -108,6 +110,7 @@ typedef struct NBDExportMetaContexts {
     bool valid; /* means that negotiation of the option finished without
                    errors */
     bool base_allocation; /* export base:allocation context (block status) */
+    bool allocation_depth; /* export qemu:allocation-depth */
     bool bitmap; /* export qemu:dirty-bitmap:<export bitmap name> */
 } NBDExportMetaContexts;

@@ -852,7 +855,8 @@ static bool nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
 /* nbd_meta_qemu_query
  *
  * Handle queries to 'qemu' namespace. For now, only the qemu:dirty-bitmap:
- * context is available.  Return true if @query has been handled.
+ * and qemu:allocation-depth contexts are available.  Return true if @query
+ * has been handled.
  */
 static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
                                 const char *query)
@@ -864,12 +868,19 @@ static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,

     if (!*query) {
         if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
+            meta->allocation_depth = meta->exp->allocation_depth;
             meta->bitmap = !!meta->exp->export_bitmap;
         }
         trace_nbd_negotiate_meta_query_parse("empty");
         return true;
     }

+    if (strcmp(query, "allocation-depth") == 0) {
+        trace_nbd_negotiate_meta_query_parse("allocation-depth");
+        meta->allocation_depth = meta->exp->allocation_depth;
+        return true;
+    }
+
     if (nbd_strshift(&query, "dirty-bitmap:")) {
         trace_nbd_negotiate_meta_query_parse("dirty-bitmap:");
         if (!meta->exp->export_bitmap) {
@@ -884,7 +895,7 @@ static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
         return true;
     }

-    trace_nbd_negotiate_meta_query_skip("not dirty-bitmap");
+    trace_nbd_negotiate_meta_query_skip("unknown qemu context");
     return true;
 }

@@ -984,6 +995,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
     if (client->opt == NBD_OPT_LIST_META_CONTEXT && !nb_queries) {
         /* enable all known contexts */
         meta->base_allocation = true;
+        meta->allocation_depth = meta->exp->allocation_depth;
         meta->bitmap = !!meta->exp->export_bitmap;
     } else {
         for (i = 0; i < nb_queries; ++i) {
@@ -1003,6 +1015,15 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         }
     }

+    if (meta->allocation_depth) {
+        ret = nbd_negotiate_send_meta_context(client, "qemu:allocation-depth",
+                                              NBD_META_ID_ALLOCATION_DEPTH,
+                                              errp);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
     if (meta->bitmap) {
         ret = nbd_negotiate_send_meta_context(client,
                                               meta->exp->export_bitmap_context,
@@ -1961,6 +1982,40 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
     return 0;
 }

+static int blockalloc_to_extents(BlockDriverState *bs, uint64_t offset,
+                                 uint64_t bytes, NBDExtentArray *ea)
+{
+    while (bytes) {
+        uint32_t flags;
+        int64_t num;
+        int ret = bdrv_is_allocated(bs, offset, bytes, &num);
+
+        if (ret < 0) {
+            return ret;
+        }
+
+        if (ret == 1) {
+            flags = NBD_STATE_DEPTH_LOCAL;
+        } else {
+            ret = bdrv_is_allocated_above(bs, NULL, false, offset, num,
+                                          &num);
+            if (ret < 0) {
+                return ret;
+            }
+            flags = ret ? NBD_STATE_DEPTH_BACKING : NBD_STATE_DEPTH_UNALLOC;
+        }
+
+        if (nbd_extent_array_add(ea, num, flags) < 0) {
+            return 0;
+        }
+
+        offset += num;
+        bytes -= num;
+    }
+
+    return 0;
+}
+
 /*
  * nbd_co_send_extents
  *
@@ -2000,7 +2055,11 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
     unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
     g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);

-    ret = blockstatus_to_extents(bs, offset, length, ea);
+    if (context_id == NBD_META_ID_BASE_ALLOCATION) {
+        ret = blockstatus_to_extents(bs, offset, length, ea);
+    } else {
+        ret = blockalloc_to_extents(bs, offset, length, ea);
+    }
     if (ret < 0) {
         return nbd_co_send_structured_error(
                 client, handle, -ret, "can't get block status", errp);
@@ -2335,16 +2394,20 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
         }
         if (client->export_meta.valid &&
             (client->export_meta.base_allocation ||
+             client->export_meta.allocation_depth ||
              client->export_meta.bitmap))
         {
             bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE;
+            int contexts_remaining = client->export_meta.base_allocation +
+                client->export_meta.allocation_depth +
+                client->export_meta.bitmap;

             if (client->export_meta.base_allocation) {
                 ret = nbd_co_send_block_status(client, request->handle,
                                                blk_bs(exp->common.blk),
                                                request->from,
                                                request->len, dont_fragment,
-                                               !client->export_meta.bitmap,
+                                               !--contexts_remaining,
                                                NBD_META_ID_BASE_ALLOCATION,
                                                errp);
                 if (ret < 0) {
@@ -2352,17 +2415,32 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
                 }
             }

+            if (client->export_meta.allocation_depth) {
+                ret = nbd_co_send_block_status(client, request->handle,
+                                               blk_bs(exp->common.blk),
+                                               request->from, request->len,
+                                               dont_fragment,
+                                               !--contexts_remaining,
+                                               NBD_META_ID_ALLOCATION_DEPTH,
+                                               errp);
+                if (ret < 0) {
+                    return ret;
+                }
+            }
+
             if (client->export_meta.bitmap) {
                 ret = nbd_co_send_bitmap(client, request->handle,
                                          client->exp->export_bitmap,
                                          request->from, request->len,
-                                         dont_fragment,
-                                         true, NBD_META_ID_DIRTY_BITMAP, errp);
+                                         dont_fragment, !--contexts_remaining,
+                                         NBD_META_ID_DIRTY_BITMAP, errp);
                 if (ret < 0) {
                     return ret;
                 }
             }

+            assert(!contexts_remaining);
+
             return 0;
         } else {
             return nbd_send_generic_reply(client, request->handle, -EINVAL,
-- 
2.28.0

[PATCH v4 3/7] nbd: Add 'qemu-nbd -A' to expose allocation depth

[Eric Blake]

Allow the server to expose an additional metacontext to be requested
by savvy clients.  qemu-nbd adds a new option -A to expose the
qemu:allocation-depth metacontext through NBD_CMD_BLOCK_STATUS; this
can also be set via QMP when using block-export-add.

qemu as client can be hacked into viewing this new context by using
the now-misnamed x-dirty-bitmap option when creating an NBD blockdev
(even though our x- naming means we could rename it, I did not think
it worth breaking back-compat of tools that have been using it while
waiting for a better solution).  It is worth noting the decoding of
how such context information will appear in 'qemu-img map
--output=json':

NBD_STATE_DEPTH_UNALLOC => "zero":false, "data":true
NBD_STATE_DEPTH_LOCAL   => "zero":false, "data":false
NBD_STATE_DEPTH_BACKING => "zero":true,  "data":true

libnbd as client is probably a nicer way to get at the information
without having to decipher such hacks in qemu as client. ;)

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20200930121105.667049-6-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[eblake: comment tweak suggested by Vladimir]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/tools/qemu-nbd.rst    |  8 ++++-
 qapi/block-core.json       |  7 ++--
 qapi/block-export.json     | 12 +++++--
 nbd/server.c               |  2 ++
 qemu-nbd.c                 | 26 +++++++++-----
 tests/qemu-iotests/309     | 73 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/309.out | 22 ++++++++++++
 tests/qemu-iotests/group   |  1 +
 8 files changed, 136 insertions(+), 15 deletions(-)
 create mode 100755 tests/qemu-iotests/309
 create mode 100644 tests/qemu-iotests/309.out

diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
index 667861cb22e9..fe41336dc550 100644
--- a/docs/tools/qemu-nbd.rst
+++ b/docs/tools/qemu-nbd.rst
@@ -72,10 +72,16 @@ driver options if ``--image-opts`` is specified.

   Export the disk as read-only.

+.. option:: -A, --allocation-depth
+
+  Expose allocation depth information via the
+  ``qemu:allocation-depth`` metadata context accessible through
+  NBD_OPT_SET_META_CONTEXT.
+
 .. option:: -B, --bitmap=NAME

   If *filename* has a qcow2 persistent bitmap *NAME*, expose
-  that bitmap via the ``qemu:dirty-bitmap:NAME`` context
+  that bitmap via the ``qemu:dirty-bitmap:NAME`` metadata context
   accessible through NBD_OPT_SET_META_CONTEXT.

 .. option:: -s, --snapshot
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ee5ebef7f2ce..b6368f5fd9a1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3883,9 +3883,12 @@
 #
 # @tls-creds: TLS credentials ID
 #
-# @x-dirty-bitmap: A "qemu:dirty-bitmap:NAME" string to query in place of
+# @x-dirty-bitmap: A metadata context name such as "qemu:dirty-bitmap:NAME"
+#                  or "qemu:allocation-depth" to query in place of the
 #                  traditional "base:allocation" block status (see
-#                  NBD_OPT_LIST_META_CONTEXT in the NBD protocol) (since 3.0)
+#                  NBD_OPT_LIST_META_CONTEXT in the NBD protocol; and
+#                  yes, naming this option x-context would have made
+#                  more sense) (since 3.0)
 #
 # @reconnect-delay: On an unexpected disconnect, the nbd client tries to
 #                   connect again until succeeding or encountering a serious
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 65804834d905..893d5cde5dfe 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -75,14 +75,20 @@
 #               (Since 5.0)
 #
 # @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)
+#          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)
+#
+# @allocation-depth: Also export the allocation depth map for @device, so
+#                    the NBD client can use NBD_OPT_SET_META_CONTEXT with
+#                    the metadata context name "qemu:allocation-depth" to
+#                    inspect allocation details. (since 5.2)
 #
 # Since: 5.0
 ##
 { 'struct': 'BlockExportOptionsNbd',
   'data': { '*name': 'str', '*description': 'str',
-            '*bitmap': 'str' } }
+            '*bitmap': 'str', '*allocation-depth': 'bool' } }

 ##
 # @NbdServerAddOptions:
diff --git a/nbd/server.c b/nbd/server.c
index ae6f8a8e5429..30cfe0eee467 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1597,6 +1597,8 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
         assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
     }

+    exp->allocation_depth = arg->allocation_depth;
+
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);

     QTAILQ_INSERT_TAIL(&exports, exp, next);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index bc644a0670b6..847fde435a7f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -99,6 +99,7 @@ static void usage(const char *name)
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET       offset into the image\n"
+"  -A, --allocation-depth    expose the allocation depth\n"
 "  -B, --bitmap=NAME         expose a persistent dirty bitmap\n"
 "\n"
 "General purpose options:\n"
@@ -519,7 +520,7 @@ int main(int argc, char **argv)
     char *device = NULL;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnc:dvk:e:f:tl:x:T:D:B:L";
+    const char *sopt = "hVb:o:p:rsnc:dvk:e:f:tl:x:T:D:AB:L";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -528,6 +529,7 @@ int main(int argc, char **argv)
         { "socket", required_argument, NULL, 'k' },
         { "offset", required_argument, NULL, 'o' },
         { "read-only", no_argument, NULL, 'r' },
+        { "allocation-depth", no_argument, NULL, 'A' },
         { "bitmap", required_argument, NULL, 'B' },
         { "connect", required_argument, NULL, 'c' },
         { "disconnect", no_argument, NULL, 'd' },
@@ -569,6 +571,7 @@ int main(int argc, char **argv)
     QDict *options = NULL;
     const char *export_name = NULL; /* defaults to "" later for server mode */
     const char *export_description = NULL;
+    bool alloc_depth = false;
     const char *bitmap = NULL;
     const char *tlscredsid = NULL;
     bool imageOpts = false;
@@ -694,6 +697,9 @@ int main(int argc, char **argv)
             readonly = true;
             flags &= ~BDRV_O_RDWR;
             break;
+        case 'A':
+            alloc_depth = true;
+            break;
         case 'B':
             bitmap = optarg;
             break;
@@ -791,8 +797,8 @@ int main(int argc, char **argv)
             exit(EXIT_FAILURE);
         }
         if (export_name || export_description || dev_offset ||
-            device || disconnect || fmt || sn_id_or_name || bitmap ||
-            seen_aio || seen_discard || seen_cache) {
+            device || disconnect || fmt || sn_id_or_name || alloc_depth ||
+            bitmap || seen_aio || seen_discard || seen_cache) {
             error_report("List mode is incompatible with per-device settings");
             exit(EXIT_FAILURE);
         }
@@ -1072,12 +1078,14 @@ int main(int argc, char **argv)
         .has_writable       = true,
         .writable           = !readonly,
         .u.nbd = {
-            .has_name           = true,
-            .name               = g_strdup(export_name),
-            .has_description    = !!export_description,
-            .description        = g_strdup(export_description),
-            .has_bitmap         = !!bitmap,
-            .bitmap             = g_strdup(bitmap),
+            .has_name             = true,
+            .name                 = g_strdup(export_name),
+            .has_description      = !!export_description,
+            .description          = g_strdup(export_description),
+            .has_bitmap           = !!bitmap,
+            .bitmap               = g_strdup(bitmap),
+            .has_allocation_depth = alloc_depth,
+            .allocation_depth     = alloc_depth,
         },
     };
     blk_exp_add(export_opts, &error_fatal);
diff --git a/tests/qemu-iotests/309 b/tests/qemu-iotests/309
new file mode 100755
index 000000000000..b6734794bb68
--- /dev/null
+++ b/tests/qemu-iotests/309
@@ -0,0 +1,73 @@
+#!/usr/bin/env bash
+#
+# Test qemu-nbd -A
+#
+# Copyright (C) 2018-2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    nbd_server_stop
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.nbd
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+_require_command QEMU_NBD
+
+echo
+echo "=== Initial image setup ==="
+echo
+
+TEST_IMG="$TEST_IMG.base" _make_test_img 4M
+$QEMU_IO -c 'w 0 2M' -f $IMGFMT "$TEST_IMG.base" | _filter_qemu_io
+_make_test_img -b "$TEST_IMG.base" -F $IMGFMT 4M
+$QEMU_IO -c 'w 1M 2M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "=== Check allocation over NBD ==="
+echo
+
+$QEMU_IMG map --output=json -f qcow2 "$TEST_IMG"
+IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
+nbd_server_start_unix_socket -r -f qcow2 -A "$TEST_IMG"
+# Normal -f raw NBD block status loses access to allocation information
+$QEMU_IMG map --output=json --image-opts \
+    "$IMG" | _filter_qemu_img_map
+# But since we used -A, and use x-dirty-bitmap as a hack for reading bitmaps,
+# we can reconstruct it, by abusing block status to report:
+#    NBD_STATE_DEPTH_UNALLOC => "zero":false, "data":true
+#    NBD_STATE_DEPTH_LOCAL   => "zero":false, "data":false
+#    NBD_STATE_DEPTH_BACKING => "zero":true,  "data":true
+$QEMU_IMG map --output=json --image-opts \
+    "$IMG,x-dirty-bitmap=qemu:allocation-depth" | _filter_qemu_img_map
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/309.out b/tests/qemu-iotests/309.out
new file mode 100644
index 000000000000..db75bb6b0df9
--- /dev/null
+++ b/tests/qemu-iotests/309.out
@@ -0,0 +1,22 @@
+QA output created by 309
+
+=== Initial image setup ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=4194304
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+wrote 2097152/2097152 bytes at offset 1048576
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Check allocation over NBD ===
+
+[{ "start": 0, "length": 1048576, "depth": 1, "zero": false, "data": true, "offset": 327680},
+{ "start": 1048576, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": 327680},
+{ "start": 3145728, "length": 1048576, "depth": 1, "zero": true, "data": false}]
+[{ "start": 0, "length": 3145728, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 3145728, "length": 1048576, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "zero": true, "data": true, "offset": OFFSET},
+{ "start": 1048576, "length": 2097152, "depth": 0, "zero": false, "data": false},
+{ "start": 3145728, "length": 1048576, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 9e4f7c01530d..a567fa97d7e5 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -315,3 +315,4 @@
 304 rw quick
 305 rw quick
 307 rw quick export
+309 rw auto quick
-- 
2.28.0

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

[Eric Blake]

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(-)

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,
         },
-- 
2.28.0

[PATCH v4 5/7] nbd: Simplify qemu bitmap context name

[Eric Blake]

Each dirty bitmap already knows its name; by reducing the scope of the
places where we construct "qemu:dirty-bitmap:NAME" strings, tracking
the name is more localized, and there are fewer per-export fields to
worry about.  This in turn will make it easier for an upcoming patch
to export more than one bitmap at once.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 884ffa00f1bd..05a8154975a1 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -97,7 +97,6 @@ struct NBDExport {

     bool allocation_depth;
     BdrvDirtyBitmap *export_bitmap;
-    char *export_bitmap_context;
 };

 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -882,14 +881,15 @@ static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
     }

     if (nbd_strshift(&query, "dirty-bitmap:")) {
+        const char *bm_name;
+
         trace_nbd_negotiate_meta_query_parse("dirty-bitmap:");
         if (!meta->exp->export_bitmap) {
             trace_nbd_negotiate_meta_query_skip("no dirty-bitmap exported");
             return true;
         }
-        if (nbd_meta_empty_or_pattern(client,
-                                      meta->exp->export_bitmap_context +
-                                      strlen("qemu:dirty-bitmap:"), query)) {
+        bm_name = bdrv_dirty_bitmap_name(meta->exp->export_bitmap);
+        if (nbd_meta_empty_or_pattern(client, bm_name, query)) {
             meta->bitmap = true;
         }
         return true;
@@ -1025,8 +1025,11 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
     }

     if (meta->bitmap) {
-        ret = nbd_negotiate_send_meta_context(client,
-                                              meta->exp->export_bitmap_context,
+        const char *bm_name = bdrv_dirty_bitmap_name(meta->exp->export_bitmap);
+        g_autofree char *context = g_strdup_printf("qemu:dirty-bitmap:%s",
+                                                   bm_name);
+
+        ret = nbd_negotiate_send_meta_context(client, context,
                                               NBD_META_ID_DIRTY_BITMAP,
                                               errp);
         if (ret < 0) {
@@ -1599,9 +1602,6 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
         bdrv_dirty_bitmap_set_busy(bm, true);
         exp->export_bitmap = bm;
         assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
-        exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
-                                                     bitmap);
-        assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
     }

     exp->allocation_depth = arg->allocation_depth;
@@ -1681,7 +1681,6 @@ static void nbd_export_delete(BlockExport *blk_exp)

     if (exp->export_bitmap) {
         bdrv_dirty_bitmap_set_busy(exp->export_bitmap, false);
-        g_free(exp->export_bitmap_context);
     }
 }

-- 
2.28.0

[PATCH v4 6/7] nbd: Refactor counting of metadata contexts

[Eric Blake]

Rather than open-code the count of negotiated contexts at several
sites, embed it directly into the struct.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 05a8154975a1..27d943529409 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -106,8 +106,7 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
  * NBD_OPT_LIST_META_CONTEXT. */
 typedef struct NBDExportMetaContexts {
     NBDExport *exp;
-    bool valid; /* means that negotiation of the option finished without
-                   errors */
+    size_t count; /* number of negotiated contexts */
     bool base_allocation; /* export base:allocation context (block status) */
     bool allocation_depth; /* export qemu:allocation-depth */
     bool bitmap; /* export qemu:dirty-bitmap:<export bitmap name> */
@@ -448,7 +447,9 @@ static int nbd_negotiate_handle_list(NBDClient *client, Error **errp)

 static void nbd_check_meta_export(NBDClient *client)
 {
-    client->export_meta.valid &= client->exp == client->export_meta.exp;
+    if (client->exp != client->export_meta.exp) {
+        client->export_meta.count = 0;
+    }
 }

 /* Send a reply to NBD_OPT_EXPORT_NAME.
@@ -956,6 +957,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
     NBDExportMetaContexts local_meta;
     uint32_t nb_queries;
     int i;
+    size_t count = 0;

     if (!client->structured_reply) {
         return nbd_opt_invalid(client, errp,
@@ -1013,6 +1015,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         if (ret < 0) {
             return ret;
         }
+        count++;
     }

     if (meta->allocation_depth) {
@@ -1022,6 +1025,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         if (ret < 0) {
             return ret;
         }
+        count++;
     }

     if (meta->bitmap) {
@@ -1035,11 +1039,12 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         if (ret < 0) {
             return ret;
         }
+        count++;
     }

     ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
     if (ret == 0) {
-        meta->valid = true;
+        meta->count = count;
     }

     return ret;
@@ -2400,15 +2405,9 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
             return nbd_send_generic_reply(client, request->handle, -EINVAL,
                                           "need non-zero length", errp);
         }
-        if (client->export_meta.valid &&
-            (client->export_meta.base_allocation ||
-             client->export_meta.allocation_depth ||
-             client->export_meta.bitmap))
-        {
+        if (client->export_meta.count) {
             bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE;
-            int contexts_remaining = client->export_meta.base_allocation +
-                client->export_meta.allocation_depth +
-                client->export_meta.bitmap;
+            int contexts_remaining = client->export_meta.count;

             if (client->export_meta.base_allocation) {
                 ret = nbd_co_send_block_status(client, request->handle,
-- 
2.28.0

[PATCH v4 7/7] nbd: Allow export of multiple bitmaps for one device

[Eric Blake]

With this, 'qemu-nbd -B b0 -B b1 -f qcow2 img.qcow2' can let you sniff
out multiple bitmaps from one server.  qemu-img as client can still
only read one bitmap per client connection, but other NBD clients
(hello libnbd) can now read multiple bitmaps in a single pass.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c           | 89 +++++++++++++++++++++++++++++-------------
 tests/qemu-iotests/291 |  6 +--
 2 files changed, 63 insertions(+), 32 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 27d943529409..8a9455627d25 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -96,7 +96,8 @@ struct NBDExport {
     Notifier eject_notifier;

     bool allocation_depth;
-    BdrvDirtyBitmap *export_bitmap;
+    BdrvDirtyBitmap **export_bitmaps;
+    size_t nr_export_bitmaps;
 };

 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -109,7 +110,10 @@ typedef struct NBDExportMetaContexts {
     size_t count; /* number of negotiated contexts */
     bool base_allocation; /* export base:allocation context (block status) */
     bool allocation_depth; /* export qemu:allocation-depth */
-    bool bitmap; /* export qemu:dirty-bitmap:<export bitmap name> */
+    bool *bitmaps; /*
+                    * export qemu:dirty-bitmap:<export bitmap name>,
+                    * sized by exp->nr_export_bitmaps
+                    */
 } NBDExportMetaContexts;

 struct NBDClient {
@@ -861,6 +865,8 @@ static bool nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
 static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
                                 const char *query)
 {
+    size_t i;
+
     if (!nbd_strshift(&query, "qemu:")) {
         return false;
     }
@@ -869,7 +875,7 @@ static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
     if (!*query) {
         if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
             meta->allocation_depth = meta->exp->allocation_depth;
-            meta->bitmap = !!meta->exp->export_bitmap;
+            memset(meta->bitmaps, 1, meta->exp->nr_export_bitmaps);
         }
         trace_nbd_negotiate_meta_query_parse("empty");
         return true;
@@ -882,17 +888,26 @@ static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
     }

     if (nbd_strshift(&query, "dirty-bitmap:")) {
-        const char *bm_name;
-
         trace_nbd_negotiate_meta_query_parse("dirty-bitmap:");
-        if (!meta->exp->export_bitmap) {
-            trace_nbd_negotiate_meta_query_skip("no dirty-bitmap exported");
+        if (!*query) {
+            if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
+                memset(meta->bitmaps, 1, meta->exp->nr_export_bitmaps);
+            }
+            trace_nbd_negotiate_meta_query_parse("empty");
             return true;
         }
-        bm_name = bdrv_dirty_bitmap_name(meta->exp->export_bitmap);
-        if (nbd_meta_empty_or_pattern(client, bm_name, query)) {
-            meta->bitmap = true;
+
+        for (i = 0; i < meta->exp->nr_export_bitmaps; i++) {
+            const char *bm_name;
+
+            bm_name = bdrv_dirty_bitmap_name(meta->exp->export_bitmaps[i]);
+            if (strcmp(bm_name, query) == 0) {
+                meta->bitmaps[i] = true;
+                trace_nbd_negotiate_meta_query_parse(query);
+                return true;
+            }
         }
+        trace_nbd_negotiate_meta_query_skip("no dirty-bitmap match");
         return true;
     }

@@ -954,9 +969,10 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
 {
     int ret;
     g_autofree char *export_name = NULL;
-    NBDExportMetaContexts local_meta;
+    g_autofree bool *bitmaps = NULL;
+    NBDExportMetaContexts local_meta = {0};
     uint32_t nb_queries;
-    int i;
+    size_t i;
     size_t count = 0;

     if (!client->structured_reply) {
@@ -971,6 +987,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         meta = &local_meta;
     }

+    g_free(meta->bitmaps);
     memset(meta, 0, sizeof(*meta));

     ret = nbd_opt_read_name(client, &export_name, NULL, errp);
@@ -985,6 +1002,10 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         return nbd_opt_drop(client, NBD_REP_ERR_UNKNOWN, errp,
                             "export '%s' not present", sane_name);
     }
+    meta->bitmaps = g_new0(bool, meta->exp->nr_export_bitmaps);
+    if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
+        bitmaps = meta->bitmaps;
+    }

     ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), false, errp);
     if (ret <= 0) {
@@ -998,7 +1019,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         /* enable all known contexts */
         meta->base_allocation = true;
         meta->allocation_depth = meta->exp->allocation_depth;
-        meta->bitmap = !!meta->exp->export_bitmap;
+        memset(meta->bitmaps, 1, meta->exp->nr_export_bitmaps);
     } else {
         for (i = 0; i < nb_queries; ++i) {
             ret = nbd_negotiate_meta_query(client, meta, errp);
@@ -1028,13 +1049,19 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         count++;
     }

-    if (meta->bitmap) {
-        const char *bm_name = bdrv_dirty_bitmap_name(meta->exp->export_bitmap);
-        g_autofree char *context = g_strdup_printf("qemu:dirty-bitmap:%s",
-                                                   bm_name);
+    for (i = 0; i < meta->exp->nr_export_bitmaps; i++) {
+        const char *bm_name;
+        g_autofree char *context = NULL;
+
+        if (!meta->bitmaps[i]) {
+            continue;
+        }
+
+        bm_name = bdrv_dirty_bitmap_name(meta->exp->export_bitmaps[i]);
+        context = g_strdup_printf("qemu:dirty-bitmap:%s", bm_name);

         ret = nbd_negotiate_send_meta_context(client, context,
-                                              NBD_META_ID_DIRTY_BITMAP,
+                                              NBD_META_ID_DIRTY_BITMAP + i,
                                               errp);
         if (ret < 0) {
             return ret;
@@ -1388,6 +1415,7 @@ void nbd_client_put(NBDClient *client)
             QTAILQ_REMOVE(&client->exp->clients, client, next);
             blk_exp_unref(&client->exp->common);
         }
+        g_free(client->export_meta.bitmaps);
         g_free(client);
     }
 }
@@ -1565,11 +1593,11 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
     }
     exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);

-    /* 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) {
+        exp->nr_export_bitmaps++;
     }
+    exp->export_bitmaps = g_new0(BdrvDirtyBitmap *, exp->nr_export_bitmaps);
+    exp->nr_export_bitmaps = 0;
     for (bitmaps = arg->bitmaps; bitmaps; bitmaps = bitmaps->next) {
         const char *bitmap = bitmaps->value;
         BlockDriverState *bs = blk_bs(blk);
@@ -1605,7 +1633,7 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
         }

         bdrv_dirty_bitmap_set_busy(bm, true);
-        exp->export_bitmap = bm;
+        exp->export_bitmaps[exp->nr_export_bitmaps++] = bm;
         assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
     }

@@ -1667,6 +1695,7 @@ static void nbd_export_request_shutdown(BlockExport *blk_exp)

 static void nbd_export_delete(BlockExport *blk_exp)
 {
+    size_t i;
     NBDExport *exp = container_of(blk_exp, NBDExport, common);

     assert(exp->name == NULL);
@@ -1684,8 +1713,8 @@ static void nbd_export_delete(BlockExport *blk_exp)
                                         blk_aio_detach, exp);
     }

-    if (exp->export_bitmap) {
-        bdrv_dirty_bitmap_set_busy(exp->export_bitmap, false);
+    for (i = 0; i < exp->nr_export_bitmaps; i++) {
+        bdrv_dirty_bitmap_set_busy(exp->export_bitmaps[i], false);
     }
 }

@@ -2332,6 +2361,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
     int flags;
     NBDExport *exp = client->exp;
     char *msg;
+    size_t i;

     switch (request->type) {
     case NBD_CMD_CACHE:
@@ -2435,12 +2465,15 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
                 }
             }

-            if (client->export_meta.bitmap) {
+            for (i = 0; i < client->exp->nr_export_bitmaps; i++) {
+                if (!client->export_meta.bitmaps[i]) {
+                    continue;
+                }
                 ret = nbd_co_send_bitmap(client, request->handle,
-                                         client->exp->export_bitmap,
+                                         client->exp->export_bitmaps[i],
                                          request->from, request->len,
                                          dont_fragment, !--contexts_remaining,
-                                         NBD_META_ID_DIRTY_BITMAP, errp);
+                                         NBD_META_ID_DIRTY_BITMAP + i, errp);
                 if (ret < 0) {
                     return ret;
                 }
diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
index 4f837b205655..37848ac60bba 100755
--- a/tests/qemu-iotests/291
+++ b/tests/qemu-iotests/291
@@ -107,16 +107,14 @@ echo
 # x-dirty-bitmap is a hack for reading bitmaps; it abuses block status to
 # report "data":false for portions of the bitmap which are set
 IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
-nbd_server_start_unix_socket -r -f qcow2 -B b0 "$TEST_IMG"
+nbd_server_start_unix_socket -r -f qcow2 \
+    -B b0 -B b1 -B b2 -B b3 "$TEST_IMG"
 $QEMU_IMG map --output=json --image-opts \
     "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b0" | _filter_qemu_img_map
-nbd_server_start_unix_socket -r -f qcow2 -B b1 "$TEST_IMG"
 $QEMU_IMG map --output=json --image-opts \
     "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b1" | _filter_qemu_img_map
-nbd_server_start_unix_socket -r -f qcow2 -B b2 "$TEST_IMG"
 $QEMU_IMG map --output=json --image-opts \
     "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
-nbd_server_start_unix_socket -r -f qcow2 -B b3 "$TEST_IMG"
 $QEMU_IMG map --output=json --image-opts \
     "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b3" | _filter_qemu_img_map

-- 
2.28.0

Re: [PATCH v4 1/7] nbd: Utilize QAPI_CLONE for type conversion

[Vladimir Sementsov-Ogievskiy]

10.10.2020 00:55, Eric Blake wrote:
> Rather than open-coding the translation from the deprecated
> NbdServerAddOptions type to the preferred BlockExportOptionsNbd, it's
> better to utilize QAPI_CLONE_MEMBERS.  This solves a couple of issues:
> first, if we do any more refactoring of the base type (which an
> upcoming patch plans to do), we don't have to revisit the open-coding.
> Second, our assignment to arg->name is fishy: the generated QAPI code
> currently does not visit it if arg->has_name is false, but if it DID
> visit it, we would have introduced a double-free situation when arg is
> finally freed.
> 
> Signed-off-by: Eric Blake<eblake@redhat.com>

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

-- 
Best regards,
Vladimir

Re: [PATCH v4 2/7] nbd: Add new qemu:allocation-depth metadata context

[Vladimir Sementsov-Ogievskiy]

10.10.2020 00:55, Eric Blake wrote:
> 'qemu-img map' provides a way to determine which extents of an image
> come from the top layer vs. inherited from a backing chain.  This is
> useful information worth exposing over NBD.  There is a proposal to
> add a QMP command block-dirty-bitmap-populate which can create a dirty
> bitmap that reflects allocation information, at which point the
> qemu:dirty-bitmap:NAME metadata context can expose that information
> via the creation of a temporary bitmap, but we can shorten the effort
> by adding a new qemu:allocation-depth metadata context that does the
> same thing without an intermediate bitmap (this patch does not
> eliminate the need for that proposal, as it will have other uses as
> well).
> 
> For this patch, I just encoded a tri-state value (unallocated, from
> this layer, from any of the backing layers); an obvious extension
> would be to provide the actual depth in bits 31-4 while keeping bits
> 1-0 as a tri-state (leaving bits 3-2 unused, for ease of reading depth
> from a hex number).  But adding this extension would require
> bdrv_is_allocated_above to return a depth number.
> 
> While documenting things, remember that although the NBD protocol has
> NBD_OPT_SET_META_CONTEXT, the rest of its documentation refers to
> 'metadata context', which is a more apt description of what is
> actually being used by NBD_CMD_BLOCK_STATUS: the user is requesting
> metadata by passing one or more context names.  So I also touched up
> some existing wording to prefer the term 'metadata context' where it
> makes sense.
> 
> Note that this patch does not actually enable any way to request a
> server to enable this context; that will come in the next patch.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

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

> ---
>   docs/interop/nbd.txt | 27 ++++++++++---

[..]

> +In the allocation depth context, bits 0 and 1 form a tri-state value:
> +
> +    bits 0-1: 00: NBD_STATE_DEPTH_UNALLOC, the extent is unallocated
> +              01: NBD_STATE_DEPTH_LOCAL, the extent is allocated in the
> +                  top level of the image

Hmm. I always thought that "image" == file, so backing chain is a chain of images,
not a several levels of one image. If it is so, than it should be "the top level image".
And "levels of the image" may designate internal qcow2 snapshots unrelated here..


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v4 5/7] nbd: Simplify qemu bitmap context name

[Vladimir Sementsov-Ogievskiy]

10.10.2020 00:55, Eric Blake wrote:
> Each dirty bitmap already knows its name; by reducing the scope of the
> places where we construct "qemu:dirty-bitmap:NAME" strings, tracking
> the name is more localized, and there are fewer per-export fields to
> worry about.  This in turn will make it easier for an upcoming patch
> to export more than one bitmap at once.
> 
> Signed-off-by: Eric Blake<eblake@redhat.com>

Thanks for updating this!

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

-- 
Best regards,
Vladimir

Re: [PATCH v4 6/7] nbd: Refactor counting of metadata contexts

[Vladimir Sementsov-Ogievskiy]

10.10.2020 00:55, Eric Blake wrote:
> Rather than open-code the count of negotiated contexts at several
> sites, embed it directly into the struct.
> 
> Signed-off-by: Eric Blake<eblake@redhat.com>

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

-- 
Best regards,
Vladimir

Re: [PATCH v4 7/7] nbd: Allow export of multiple bitmaps for one device

[Vladimir Sementsov-Ogievskiy]

10.10.2020 00:55, Eric Blake wrote:
> With this, 'qemu-nbd -B b0 -B b1 -f qcow2 img.qcow2' can let you sniff
> out multiple bitmaps from one server.  qemu-img as client can still
> only read one bitmap per client connection, but other NBD clients
> (hello libnbd) can now read multiple bitmaps in a single pass.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>


You didn't update nbd_export_create failure patch, I suggest:

@@ -1533,6 +1537,7 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
      bool shared = !exp_args->writable;
      strList *bitmaps;
      int ret;
+    size_t i;
  
      assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
  
@@ -1632,11 +1637,15 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
              goto fail;
          }
  
-        bdrv_dirty_bitmap_set_busy(bm, true);
          exp->export_bitmaps[exp->nr_export_bitmaps++] = bm;
          assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
      }
  
+    /* Mark bitmaps busy in a separate loop, to not bother with roll-back */
+    for (i = 0; i < exp->nr_export_bitmaps; i++) {
+        bdrv_dirty_bitmap_set_busy(bm, true);
+    }
+
      exp->allocation_depth = arg->allocation_depth;
  
      blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
@@ -1646,6 +1655,7 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
      return 0;
  
  fail:
+    g_free(exp->export_bitmaps);
      g_free(exp->name);
      g_free(exp->description);
      return ret;

and with it:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


Also, would be good to add a comment:

@@ -29,6 +29,10 @@
  #define NBD_META_ID_BASE_ALLOCATION 0
  #define NBD_META_ID_ALLOCATION_DEPTH 1
  #define NBD_META_ID_DIRTY_BITMAP 2
+/*
+ * NBD_META_ID_DIRTY_BITMAP+i are reserved for dirty bitmaps, so keep
+ * NBD_META_ID_DIRTY_BITMAP the last one.
+ */
  
  /*
   * NBD_MAX_BLOCK_STATUS_EXTENTS: 1 MiB of extents data. An empirical



-- 
Best regards,
Vladimir

Re: [PATCH v4 7/7] nbd: Allow export of multiple bitmaps for one device

[Eric Blake]

On 10/14/20 9:42 AM, Vladimir Sementsov-Ogievskiy wrote:
> 10.10.2020 00:55, Eric Blake wrote:
>> With this, 'qemu-nbd -B b0 -B b1 -f qcow2 img.qcow2' can let you sniff
>> out multiple bitmaps from one server.  qemu-img as client can still
>> only read one bitmap per client connection, but other NBD clients
>> (hello libnbd) can now read multiple bitmaps in a single pass.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> 
> You didn't update nbd_export_create failure patch, I suggest:

Good catch. I'm happy to fold in both of your suggestions.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

[Eric Blake]

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

[Markus Armbruster]

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.


[...]

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

[Eric Blake]

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

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

[Markus Armbruster]

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.

Re: [PATCH v4 2/7] nbd: Add new qemu:allocation-depth metadata context

[Eric Blake]

On 10/14/20 6:52 AM, Vladimir Sementsov-Ogievskiy wrote:

>>   docs/interop/nbd.txt | 27 ++++++++++---
> 
> [..]
> 
>> +In the allocation depth context, bits 0 and 1 form a tri-state value:
>> +
>> +    bits 0-1: 00: NBD_STATE_DEPTH_UNALLOC, the extent is unallocated
>> +              01: NBD_STATE_DEPTH_LOCAL, the extent is allocated in the
>> +                  top level of the image
> 
> Hmm. I always thought that "image" == file, so backing chain is a chain
> of images,
> not a several levels of one image. If it is so, than it should be "the
> top level image".
> And "levels of the image" may designate internal qcow2 snapshots
> unrelated here..

It's fuzzy.  From the guest point of view, we are serving a single guest
image by use of multiple files in the host.  I will do s/level/layer/,
to match the wording I already had on the next line:

>               10: NBD_STATE_DEPTH_BACKING, the extent is inherited from a
>                   backing layer

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v4 1/7] nbd: Utilize QAPI_CLONE for type conversion

[Eric Blake]

On 10/9/20 4:55 PM, Eric Blake wrote:
> Rather than open-coding the translation from the deprecated
> NbdServerAddOptions type to the preferred BlockExportOptionsNbd, it's
> better to utilize QAPI_CLONE_MEMBERS.  This solves a couple of issues:
> first, if we do any more refactoring of the base type (which an
> upcoming patch plans to do), we don't have to revisit the open-coding.
> Second, our assignment to arg->name is fishy: the generated QAPI code
> currently does not visit it if arg->has_name is false, but if it DID
> visit it, we would have introduced a double-free situation when arg is
> finally freed.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  blockdev-nbd.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

v5 will fix this nasty bug:


> @@ -195,7 +197,8 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
>       * the device name as a default here for compatibility.
>       */
>      if (!arg->has_name) {
> -        arg->name = arg->device;
> +        arg->has_name = true;
> +        arg->name = g_steal_pointer(&arg->device);
>      }

This causes assertion failures visible in at least iotest 149 and 192,
because arg->device was left NULL.  Using g_strdup() instead fixes that.

> 
>      export_opts = g_new(BlockExportOptions, 1);
> @@ -205,15 +208,9 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
>          .node_name              = g_strdup(bdrv_get_node_name(bs)),
>          .has_writable           = arg->has_writable,
>          .writable               = arg->writable,
> -        .u.nbd = {
> -            .has_name           = true,
> -            .name               = g_strdup(arg->name),
> -            .has_description    = arg->has_description,
> -            .description        = g_strdup(arg->description),
> -            .has_bitmap         = arg->has_bitmap,
> -            .bitmap             = g_strdup(arg->bitmap),
> -        },
>      };
> +    QAPI_CLONE_MEMBERS(BlockExportOptionsNbd, &export_opts->u.nbd,
> +                       qapi_NbdServerAddOptions_base(arg));
> 
>      /*
>       * nbd-server-add doesn't complain when a read-only device should be
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Libguestfs] [libnbd PATCH] info: Add support for new 'qemu-nbd -A' qemu:allocation-depth

[Eric Blake]

On 10/16/20 10:23 AM, Eric Blake wrote:
> A rather trivial decoding; we may enhance it further if qemu extends
> things to give an integer depth alongside its tri-state encoding.
> ---
> 
> I'll wait to push this to libnbd until the counterpart qemu patches
> land upstream, although it looks like I've got positive review.

Whoops, I accidentally pushed this before qemu stuff landed upstream,
and in the meantime, we changed our minds on what to expose over
qemu:allocation-depth to be a bare integer rather than a tri-state.
I'll push this followup (but this time, wait for the actual qemu patch
to land).  In fact, I should probably add test-suite coverage...


From eba8734654e6fd340e18b3e07c3213ed1a0ab9e8 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Tue, 27 Oct 2020 10:27:25 -0500
Subject: [libnbd PATCH] info: Adjust to actual 'qemu-nbd -A' semantics

Review on the qemu list has led to an altered definition of what
'qemu:allocation-depth' should report: rather than a tri-state value,
it is an actual depth.  It's time to match what actually got committed
into qemu, which in turn means a slight refactoring to use a malloc'd
string for a description.

Fixes: 71455c021
---
 info/nbdinfo.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/info/nbdinfo.c b/info/nbdinfo.c
index 2b22f51..b152f28 100644
--- a/info/nbdinfo.c
+++ b/info/nbdinfo.c
@@ -767,28 +767,30 @@ get_content (struct nbd_handle *nbd, int64_t size)
 }

 /* Callback handling --map. */
-static const char *
+static char *
 extent_description (const char *metacontext, uint32_t type)
 {
+  char *ret;
+
   if (strcmp (metacontext, "base:allocation") == 0) {
     switch (type) {
-    case 0: return "allocated";
-    case 1: return "hole";
-    case 2: return "zero";
-    case 3: return "hole,zero";
+    case 0: return strdup ("allocated");
+    case 1: return strdup ("hole");
+    case 2: return strdup ("zero");
+    case 3: return strdup ("hole,zero");
     }
   }
   else if (strncmp (metacontext, "qemu:dirty-bitmap:", 18) == 0) {
     switch (type) {
-    case 0: return "clean";
-    case 1: return "dirty";
+    case 0: return strdup ("clean");
+    case 1: return strdup ("dirty");
     }
   }
   else if (strcmp (metacontext, "qemu:allocation-depth") == 0) {
-    switch (type & 3) {
-    case 0: return "unallocated";
-    case 1: return "local";
-    case 2: return "backing";
+    switch (type) {
+    case 0: return strdup ("unallocated");
+    case 1: return strdup ("local");
+    case 2: asprintf (&ret, "backing depth %d", type); return ret;
     }
   }

@@ -810,7 +812,7 @@ extent_callback (void *user_data, const char
*metacontext,

   /* Print the entries received. */
   for (i = 0; i < nr_entries; i += 2) {
-    const char *descr = extent_description (map, entries[i+1]);
+    char *descr = extent_description (map, entries[i+1]);

     if (!json_output) {
       fprintf (fp, "%10" PRIu64 "  "
@@ -837,6 +839,7 @@ extent_callback (void *user_data, const char
*metacontext,
       comma = true;
     }

+    free (descr);
     offset += entries[i];
   }

-- 
2.29.0



-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Libguestfs] [libnbd PATCH] info: Add support for new 'qemu-nbd -A' qemu:allocation-depth

[Richard W.M. Jones]

On Tue, Oct 27, 2020 at 10:33:48AM -0500, Eric Blake wrote:
> On 10/16/20 10:23 AM, Eric Blake wrote:
> > A rather trivial decoding; we may enhance it further if qemu extends
> > things to give an integer depth alongside its tri-state encoding.
> > ---
> > 
> > I'll wait to push this to libnbd until the counterpart qemu patches
> > land upstream, although it looks like I've got positive review.
> 
> Whoops, I accidentally pushed this before qemu stuff landed upstream,
> and in the meantime, we changed our minds on what to expose over
> qemu:allocation-depth to be a bare integer rather than a tri-state.
> I'll push this followup (but this time, wait for the actual qemu patch
> to land).  In fact, I should probably add test-suite coverage...

ACK.  I have a patch which touches this file but it's a simple merge
to combine the two changes.

Rich.

> >From eba8734654e6fd340e18b3e07c3213ed1a0ab9e8 Mon Sep 17 00:00:00 2001
> From: Eric Blake <eblake@redhat.com>
> Date: Tue, 27 Oct 2020 10:27:25 -0500
> Subject: [libnbd PATCH] info: Adjust to actual 'qemu-nbd -A' semantics
> 
> Review on the qemu list has led to an altered definition of what
> 'qemu:allocation-depth' should report: rather than a tri-state value,
> it is an actual depth.  It's time to match what actually got committed
> into qemu, which in turn means a slight refactoring to use a malloc'd
> string for a description.
> 
> Fixes: 71455c021
> ---
>  info/nbdinfo.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/info/nbdinfo.c b/info/nbdinfo.c
> index 2b22f51..b152f28 100644
> --- a/info/nbdinfo.c
> +++ b/info/nbdinfo.c
> @@ -767,28 +767,30 @@ get_content (struct nbd_handle *nbd, int64_t size)
>  }
> 
>  /* Callback handling --map. */
> -static const char *
> +static char *
>  extent_description (const char *metacontext, uint32_t type)
>  {
> +  char *ret;
> +
>    if (strcmp (metacontext, "base:allocation") == 0) {
>      switch (type) {
> -    case 0: return "allocated";
> -    case 1: return "hole";
> -    case 2: return "zero";
> -    case 3: return "hole,zero";
> +    case 0: return strdup ("allocated");
> +    case 1: return strdup ("hole");
> +    case 2: return strdup ("zero");
> +    case 3: return strdup ("hole,zero");
>      }
>    }
>    else if (strncmp (metacontext, "qemu:dirty-bitmap:", 18) == 0) {
>      switch (type) {
> -    case 0: return "clean";
> -    case 1: return "dirty";
> +    case 0: return strdup ("clean");
> +    case 1: return strdup ("dirty");
>      }
>    }
>    else if (strcmp (metacontext, "qemu:allocation-depth") == 0) {
> -    switch (type & 3) {
> -    case 0: return "unallocated";
> -    case 1: return "local";
> -    case 2: return "backing";
> +    switch (type) {
> +    case 0: return strdup ("unallocated");
> +    case 1: return strdup ("local");
> +    case 2: asprintf (&ret, "backing depth %d", type); return ret;
>      }
>    }
> 
> @@ -810,7 +812,7 @@ extent_callback (void *user_data, const char
> *metacontext,
> 
>    /* Print the entries received. */
>    for (i = 0; i < nr_entries; i += 2) {
> -    const char *descr = extent_description (map, entries[i+1]);
> +    char *descr = extent_description (map, entries[i+1]);
> 
>      if (!json_output) {
>        fprintf (fp, "%10" PRIu64 "  "
> @@ -837,6 +839,7 @@ extent_callback (void *user_data, const char
> *metacontext,
>        comma = true;
>      }
> 
> +    free (descr);
>      offset += entries[i];
>    }
> 
> -- 
> 2.29.0
> 
> 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org