[Qemu-devel] [PATCH for-2.13 0/4] NBD export bitmaps

[Qemu-devel] [PATCH for-2.13 0/4] NBD export bitmaps

[Vladimir Sementsov-Ogievskiy]

Hi all.

This is a proposal and realization of new NBD meta context:
qemu-dirty-bitmap. (I'll send corresponding proposal to NBD protocol
after some negotiation here)

Corresponding queries will look like:
qemu-dirty-bitmap:<export-bitmap-name>

Mapping from export-bitmap-name to BdrvDirtyBitmap is done through qmp
command nbd-server-add-bitmap. For now, only one bitmap export is
allowed per NBD export, however it may be easily improved if needed 
(we don't have such cases for now)

Client and testing.
I wrote client code for Virtuozzo, but it turned out to be unused,
actually it's used only for tests. We don't have cases, where we need
to import dirty bitmap through qemu nbd-client. All this done for
exporting dirty bitmaps to the third tool. So, I think, it is not worth
refactoring, rebasing and merging client part upstream, if there are no
real usage cases.

Vladimir Sementsov-Ogievskiy (4):
  nbd/server: refactor nbd_negotiate_meta_query for several namespaces
  nbd/server: add nbd_meta_single_query helper
  nbd/server: implement dirty bitmap export
  qapi: new qmp command nbd-server-add-bitmap

 qapi/block.json     |  27 +++++
 include/block/nbd.h |   2 +
 blockdev-nbd.c      |  23 ++++
 nbd/server.c        | 308 ++++++++++++++++++++++++++++++++++++++++++++++------
 4 files changed, 324 insertions(+), 36 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH 1/4] nbd/server: refactor nbd_negotiate_meta_query for several namespaces

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 17 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index cea158913b..b830997114 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -771,13 +771,19 @@ static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
     return 1;
 }
 
+struct {
+    const char *ns;
+    int (*func)(NBDClient *, NBDExportMetaContexts *, uint32_t, Error **);
+} meta_namespace_handlers[] = {
+    /* namespaces should go in non-decreasing order by name length */
+    {.ns = "base:", .func = nbd_meta_base_query},
+};
+
 /* nbd_negotiate_meta_query
  *
  * Parse namespace name and call corresponding function to parse body of the
  * query.
  *
- * The only supported namespace now is 'base'.
- *
  * The function aims not wasting time and memory to read long unknown namespace
  * names.
  *
@@ -787,9 +793,12 @@ static int nbd_negotiate_meta_query(NBDClient *client,
                                     NBDExportMetaContexts *meta, Error **errp)
 {
     int ret;
-    char query[sizeof("base:") - 1];
-    size_t baselen = strlen("base:");
+    int i;
     uint32_t len;
+    int bytes_done = 0;
+    char *query;
+    int nb_ns = sizeof(meta_namespace_handlers) /
+                sizeof(meta_namespace_handlers[0]);
 
     ret = nbd_opt_read(client, &len, sizeof(len), errp);
     if (ret <= 0) {
@@ -797,22 +806,39 @@ static int nbd_negotiate_meta_query(NBDClient *client,
     }
     cpu_to_be32s(&len);
 
-    /* The only supported namespace for now is 'base'. So query should start
-     * with 'base:'. Otherwise, we can ignore it and skip the remainder. */
-    if (len < baselen) {
-        return nbd_opt_skip(client, len, errp);
-    }
+    query = g_malloc(strlen(meta_namespace_handlers[nb_ns - 1].ns));
 
-    len -= baselen;
-    ret = nbd_opt_read(client, query, baselen, errp);
-    if (ret <= 0) {
-        return ret;
-    }
-    if (strncmp(query, "base:", baselen) != 0) {
-        return nbd_opt_skip(client, len, errp);
+    for (i = 0; i < nb_ns; i++) {
+        const char *ns = meta_namespace_handlers[i].ns;
+        int ns_len = strlen(ns);
+        int diff_len = strlen(ns) - bytes_done;
+
+        assert(diff_len >= 0);
+
+        if (diff_len > 0) {
+            if (len < diff_len) {
+                ret = nbd_opt_skip(client, len, errp);
+                goto out;
+            }
+
+            len -= diff_len;
+            ret = nbd_opt_read(client, query + bytes_done, diff_len, errp);
+            if (ret <= 0) {
+                goto out;
+            }
+        }
+
+        if (!strncmp(query, ns, ns_len)) {
+            ret = meta_namespace_handlers[i].func(client, meta, len, errp);
+            goto out;
+        }
     }
 
-    return nbd_meta_base_query(client, meta, len, errp);
+    ret = nbd_opt_skip(client, len, errp);
+
+out:
+    g_free(query);
+    return ret;
 }
 
 /* nbd_negotiate_meta_queries
-- 
2.11.1

[Qemu-devel] [PATCH 2/4] nbd/server: add nbd_meta_single_query helper

[Vladimir Sementsov-Ogievskiy]

The helper will be reused for bitmaps namespace.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index b830997114..8fe53ffd4b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -733,44 +733,59 @@ static int nbd_negotiate_send_meta_context(NBDClient *client,
     return qio_channel_writev_all(client->ioc, iov, 2, errp) < 0 ? -EIO : 0;
 }
 
-/* nbd_meta_base_query
- *
- * Handle query to 'base' namespace. For now, only base:allocation context is
- * available in it.  'len' is the amount of text remaining to be read from
- * the current name, after the 'base:' portion has been stripped.
+/* Read len bytes and check matching to the pattern.
+ * @match is set to true on empty query for _LIST_ and for query matching the
+ * @pattern. @match is never set to false.
  *
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success. */
-static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
-                               uint32_t len, Error **errp)
+static int nbd_meta_single_query(NBDClient *client, const char *pattern,
+                                 uint32_t len, bool *match, Error **errp)
 {
     int ret;
-    char query[sizeof("allocation") - 1];
-    size_t alen = strlen("allocation");
+    char *query;
 
     if (len == 0) {
         if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
-            meta->base_allocation = true;
+            *match = true;
         }
         return 1;
     }
 
-    if (len != alen) {
+    if (len != strlen(pattern)) {
         return nbd_opt_skip(client, len, errp);
     }
 
+    query = g_malloc(len);
     ret = nbd_opt_read(client, query, len, errp);
     if (ret <= 0) {
+        g_free(query);
         return ret;
     }
 
-    if (strncmp(query, "allocation", alen) == 0) {
-        meta->base_allocation = true;
+    if (strncmp(query, pattern, len) == 0) {
+        *match = true;
     }
+    g_free(query);
 
     return 1;
 }
 
+/* nbd_meta_base_query
+ *
+ * Handle query to 'base' namespace. For now, only base:allocation context is
+ * available in it.  'len' is the amount of text remaining to be read from
+ * the current name, after the 'base:' portion has been stripped.
+ *
+ * Return -errno on I/O error, 0 if option was completely handled by
+ * sending a reply about inconsistent lengths, or 1 on success. */
+static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
+                               uint32_t len, Error **errp)
+{
+    return nbd_meta_single_query(client, "allocation", len,
+                                 &meta->base_allocation, errp);
+}
+
 struct {
     const char *ns;
     int (*func)(NBDClient *, NBDExportMetaContexts *, uint32_t, Error **);
-- 
2.11.1

[Qemu-devel] [PATCH 3/4] nbd/server: implement dirty bitmap export

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/nbd.h |   2 +
 nbd/server.c        | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 203 insertions(+), 6 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index fcdcd54502..f0b459283f 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -315,6 +315,8 @@ void nbd_client_put(NBDClient *client);
 void nbd_server_start(SocketAddress *addr, const char *tls_creds,
                       Error **errp);
 
+void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
+                       const char *bitmap_export_name, Error **errp);
 
 /* nbd_read
  * Reads @size bytes from @ioc. Returns 0 on success.
diff --git a/nbd/server.c b/nbd/server.c
index 8fe53ffd4b..6554919ef2 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -23,6 +23,12 @@
 #include "nbd-internal.h"
 
 #define NBD_META_ID_BASE_ALLOCATION 0
+#define NBD_META_ID_DIRTY_BITMAP 1
+
+#define NBD_MAX_BITMAP_EXTENTS (0x100000 / 8) /* 1 mb of extents data */
+#define MAX_EXTENT_LENGTH QEMU_ALIGN_DOWN(INT32_MAX, 512)
+
+#define NBD_STATE_DIRTY 1
 
 static int system_errno_to_nbd_errno(int err)
 {
@@ -80,6 +86,9 @@ struct NBDExport {
 
     BlockBackend *eject_notifier_blk;
     Notifier eject_notifier;
+
+    BdrvDirtyBitmap *export_bitmap;
+    char *export_bitmap_name;
 };
 
 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -92,6 +101,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 dirty_bitmap; /* export qemu-dirty-bitmap:<export_bitmap_name> */
 } NBDExportMetaContexts;
 
 struct NBDClient {
@@ -786,12 +796,32 @@ static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
                                  &meta->base_allocation, errp);
 }
 
+/* nbd_meta_bitmap_query
+ *
+ * Handle query to 'qemu-dirty-bitmap' namespace.
+ * 'len' is the amount of text remaining to be read from the current name, after
+ * the 'qemu-dirty-bitmap:' portion has been stripped.
+ *
+ * Return -errno on I/O error, 0 if option was completely handled by
+ * sending a reply about inconsistent lengths, or 1 on success. */
+static int nbd_meta_bitmap_query(NBDClient *client, NBDExportMetaContexts *meta,
+                                 uint32_t len, Error **errp)
+{
+    if (!client->exp->export_bitmap) {
+        return nbd_opt_skip(client, len, errp);
+    }
+
+    return nbd_meta_single_query(client, client->exp->export_bitmap_name, len,
+                                 &meta->dirty_bitmap, errp);
+}
+
 struct {
     const char *ns;
     int (*func)(NBDClient *, NBDExportMetaContexts *, uint32_t, Error **);
 } meta_namespace_handlers[] = {
     /* namespaces should go in non-decreasing order by name length */
     {.ns = "base:", .func = nbd_meta_base_query},
+    {.ns = "qemu-dirty-bitmap:", .func = nbd_meta_bitmap_query},
 };
 
 /* nbd_negotiate_meta_query
@@ -921,6 +951,17 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         }
     }
 
+    if (meta->dirty_bitmap) {
+        char *context = g_strdup_printf("qemu-dirty-bitmap:%s",
+                                        exp->export_bitmap_name);
+        ret = nbd_negotiate_send_meta_context(client, context,
+                                              NBD_META_ID_DIRTY_BITMAP,
+                                              errp);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
     ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
     if (ret == 0) {
         meta->valid = true;
@@ -1549,6 +1590,11 @@ void nbd_export_put(NBDExport *exp)
             exp->blk = NULL;
         }
 
+        if (exp->export_bitmap) {
+            bdrv_dirty_bitmap_set_qmp_locked(exp->export_bitmap, false);
+            g_free(exp->export_bitmap_name);
+        }
+
         g_free(exp);
     }
 }
@@ -1790,6 +1836,9 @@ static int blockstatus_to_extent_be(BlockDriverState *bs, uint64_t offset,
 }
 
 /* nbd_co_send_extents
+ *
+ * NBD_REPLY_FLAG_DONE is not set, don't forget to send it.
+ *
  * @extents should be in big-endian */
 static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
                                NBDExtent *extents, unsigned nb_extents,
@@ -1802,7 +1851,7 @@ static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
         {.iov_base = extents, .iov_len = nb_extents * sizeof(extents[0])}
     };
 
-    set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_BLOCK_STATUS,
+    set_be_chunk(&chunk.h, 0, NBD_REPLY_TYPE_BLOCK_STATUS,
                  handle, sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
     stl_be_p(&chunk.context_id, context_id);
 
@@ -1827,6 +1876,91 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
     return nbd_co_send_extents(client, handle, &extent, 1, context_id, errp);
 }
 
+/* Set several extents, describing region of given @length with given @flags.
+ * Do not set more than @nb_extents, return number of set extents.
+ */
+static unsigned add_extents(NBDExtent *extents, unsigned nb_extents,
+                            uint64_t length, uint32_t flags)
+{
+    unsigned i = 0;
+    uint32_t max_extent_be = cpu_to_be32(MAX_EXTENT_LENGTH);
+    uint32_t flags_be = cpu_to_be32(flags);
+
+    for (i = 0; i < nb_extents && length > MAX_EXTENT_LENGTH;
+         i++, length -= MAX_EXTENT_LENGTH)
+    {
+        extents[i].length = max_extent_be;
+        extents[i].flags = flags_be;
+    }
+
+    if (length > 0 && i < nb_extents) {
+        extents[i].length = cpu_to_be32(length);
+        extents[i].flags = flags_be;
+        i++;
+    }
+
+    return i;
+}
+
+static unsigned bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
+                                  uint64_t length, NBDExtent *extents,
+                                  unsigned nb_extents)
+{
+    uint64_t begin = offset, end;
+    uint64_t overall_end = offset + length;
+    unsigned i = 0;
+    BdrvDirtyBitmapIter *it;
+    bool dirty;
+
+    bdrv_dirty_bitmap_lock(bitmap);
+
+    it = bdrv_dirty_iter_new(bitmap);
+    dirty = bdrv_get_dirty_locked(NULL, bitmap, offset);
+
+    while (begin < overall_end && i < nb_extents) {
+        if (dirty) {
+            end = bdrv_dirty_bitmap_next_zero(bitmap, begin);
+        } else {
+            bdrv_set_dirty_iter(it, begin);
+            end = bdrv_dirty_iter_next(it);
+        }
+        if (end == -1) {
+            end = overall_end;
+        }
+
+        i += add_extents(extents + i, nb_extents - i, end - begin,
+                         dirty ? NBD_STATE_DIRTY : 0);
+        begin = end;
+        dirty = !dirty;
+    }
+
+    bdrv_dirty_iter_free(it);
+
+    bdrv_dirty_bitmap_unlock(bitmap);
+
+    return i;
+}
+
+static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
+                              BdrvDirtyBitmap *bitmap, uint64_t offset,
+                              uint64_t length, uint32_t context_id,
+                              Error **errp)
+{
+    int ret;
+    unsigned nb_extents;
+    NBDExtent *extents = g_new(NBDExtent, NBD_MAX_BITMAP_EXTENTS);
+
+    nb_extents = bitmap_to_extents(bitmap, offset, length, extents,
+                                   NBD_MAX_BITMAP_EXTENTS);
+
+    ret = nbd_co_send_extents(client, handle, extents, nb_extents, context_id,
+                              errp);
+
+    g_free(extents);
+
+    return ret;
+}
+
 /* nbd_co_receive_request
  * Collect a client request. Return 0 if request looks valid, -EIO to drop
  * connection right away, and any other negative value to report an error to
@@ -2040,11 +2174,32 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
                                       "discard failed", errp);
 
     case NBD_CMD_BLOCK_STATUS:
-        if (client->export_meta.valid && client->export_meta.base_allocation) {
-            return nbd_co_send_block_status(client, request->handle,
-                                            blk_bs(exp->blk), request->from,
-                                            request->len,
-                                            NBD_META_ID_BASE_ALLOCATION, errp);
+        if (client->export_meta.valid &&
+            (client->export_meta.base_allocation ||
+             client->export_meta.dirty_bitmap))
+        {
+            if (client->export_meta.base_allocation) {
+                ret = nbd_co_send_block_status(client, request->handle,
+                                               blk_bs(exp->blk), request->from,
+                                               request->len,
+                                               NBD_META_ID_BASE_ALLOCATION,
+                                               errp);
+                if (ret < 0) {
+                    return ret;
+                }
+            }
+
+            if (client->export_meta.dirty_bitmap) {
+                ret = nbd_co_send_bitmap(client, request->handle,
+                                         client->exp->export_bitmap,
+                                         request->from, request->len,
+                                         NBD_META_ID_DIRTY_BITMAP, errp);
+                if (ret < 0) {
+                    return ret;
+                }
+            }
+
+            return nbd_co_send_structured_done(client, request->handle, errp);
         } else {
             return nbd_send_generic_reply(client, request->handle, -EINVAL,
                                           "CMD_BLOCK_STATUS not negotiated",
@@ -2196,3 +2351,43 @@ void nbd_client_new(NBDExport *exp,
     co = qemu_coroutine_create(nbd_co_client_start, client);
     qemu_coroutine_enter(co);
 }
+
+void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
+                       const char *bitmap_export_name, Error **errp)
+{
+    BdrvDirtyBitmap *bm = NULL;
+    BlockDriverState *bs = blk_bs(exp->blk);
+
+    if (exp->export_bitmap) {
+        error_setg(errp, "Export bitmap is already set");
+        return;
+    }
+
+    while (true) {
+        bm = bdrv_find_dirty_bitmap(bs, bitmap);
+        if (bm != NULL || bs->backing == NULL) {
+            break;
+        }
+
+        bs = bs->backing->bs;
+    }
+
+    if (bm == NULL) {
+        error_setg(errp, "Bitmap '%s' is not found", bitmap);
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_enabled(bm)) {
+        error_setg(errp, "Bitmap '%s' is enabled", bitmap);
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_qmp_locked(bm)) {
+        error_setg(errp, "Bitmap '%s' is locked", bitmap);
+        return;
+    }
+
+    bdrv_dirty_bitmap_set_qmp_locked(bm, true);
+    exp->export_bitmap = bm;
+    exp->export_bitmap_name = g_strdup(bitmap_export_name);
+}
-- 
2.11.1

[Qemu-devel] [PATCH 4/4] qapi: new qmp command nbd-server-add-bitmap

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block.json | 27 +++++++++++++++++++++++++++
 blockdev-nbd.c  | 23 +++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/qapi/block.json b/qapi/block.json
index c694524002..4afbbcd7b7 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -269,6 +269,33 @@
   'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }
 
 ##
+# @nbd-server-add-bitmap:
+#
+# Export dirty bitmap through selected export. Bitmaps are searched for in
+# device attached to the export and in all its backings. Exported bitmap
+# is locked until NBD export is removed.
+#
+# @name: Export name.
+#
+# @bitmap: Bitmap name to search.
+#
+# @bitmap-export-name: How the bitmap will be seen by nbd clients
+#                      (default @bitmap)
+#
+#
+# Returns: error on one of the following conditions:
+#           - the server is not running
+#           - export is not found
+#           - bitmap is not found
+#           - bitmap is disabled
+#           - bitmap is locked
+#
+# Since: 2.13
+##
+  { 'command': 'nbd-server-add-bitmap',
+    'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 'str'} }
+
+##
 # @nbd-server-stop:
 #
 # Stop QEMU's embedded NBD server, and unregister all devices previously
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 65a84739ed..6b0c50732c 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -220,3 +220,26 @@ void qmp_nbd_server_stop(Error **errp)
     nbd_server_free(nbd_server);
     nbd_server = NULL;
 }
+
+void qmp_nbd_server_add_bitmap(const char *name, const char *bitmap,
+                               bool has_bitmap_export_name,
+                               const char *bitmap_export_name,
+                               Error **errp)
+{
+    NBDExport *exp;
+
+    if (!nbd_server) {
+        error_setg(errp, "NBD server not running");
+        return;
+    }
+
+    exp = nbd_export_find(name);
+    if (exp == NULL) {
+        error_setg(errp, "Export '%s' is not found", name);
+        return;
+    }
+
+    nbd_export_bitmap(exp, bitmap,
+                      has_bitmap_export_name ? bitmap_export_name : bitmap,
+                      errp);
+}
-- 
2.11.1

Re: [Qemu-devel] [PATCH 1/4] nbd/server: refactor nbd_negotiate_meta_query for several namespaces

[Eric Blake]

[adding NBD list]

On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   nbd/server.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 43 insertions(+), 17 deletions(-)

> +struct {
> +    const char *ns;
> +    int (*func)(NBDClient *, NBDExportMetaContexts *, uint32_t, Error **);
> +} meta_namespace_handlers[] = {
> +    /* namespaces should go in non-decreasing order by name length */
> +    {.ns = "base:", .func = nbd_meta_base_query},
> +};
> +

> @@ -787,9 +793,12 @@ static int nbd_negotiate_meta_query(NBDClient *client,
>                                       NBDExportMetaContexts *meta, Error **errp)
>   {
>       int ret;
> -    char query[sizeof("base:") - 1];
> -    size_t baselen = strlen("base:");
> +    int i;
>       uint32_t len;
> +    int bytes_done = 0;
> +    char *query;
> +    int nb_ns = sizeof(meta_namespace_handlers) /
> +                sizeof(meta_namespace_handlers[0]);

Use the ARRAY_SIZE() macro here.

> +    query = g_malloc(strlen(meta_namespace_handlers[nb_ns - 1].ns));

So this sizes a buffer according to the largest namespace we expect to 
handle,...

>   
> -    len -= baselen;
> -    ret = nbd_opt_read(client, query, baselen, errp);
> -    if (ret <= 0) {
> -        return ret;
> -    }
> -    if (strncmp(query, "base:", baselen) != 0) {
> -        return nbd_opt_skip(client, len, errp);
> +    for (i = 0; i < nb_ns; i++) {
> +        const char *ns = meta_namespace_handlers[i].ns;
> +        int ns_len = strlen(ns);
> +        int diff_len = strlen(ns) - bytes_done;
> +
> +        assert(diff_len >= 0);
> +
> +        if (diff_len > 0) {
> +            if (len < diff_len) {
> +                ret = nbd_opt_skip(client, len, errp);
> +                goto out;
> +            }
> +
> +            len -= diff_len;
> +            ret = nbd_opt_read(client, query + bytes_done, diff_len, errp);
> +            if (ret <= 0) {
> +                goto out;
> +            }
> +        }
> +
> +        if (!strncmp(query, ns, ns_len)) {
> +            ret = meta_namespace_handlers[i].func(client, meta, len, errp);
> +            goto out;
> +        }


...then you do multiple iterative reads as you step through the list. 
You know, if you encounter a ':' at any point in the iterative reads, 
you don't have to continue through the rest of the handlers (the query 
belongs to a short-named namespace we didn't recognize).

Is it any smarter to just blindly do a single read of MIN(querylen, 
maxlen), then strchr() for ':' (if not found, it's not a namespace we 
recognize), and then look up if the name matches, at which point we then 
read the rest of the query and refactor the namespace handler to be 
passed the already-parsed leafname, rather than having to parse the 
leafname off the wire in the handler?

I'm STILL wondering if the NBD spec should specify namespace and 
leafname as separate fields rather than requiring the server to parse 
for ':'.  We have only a couple of weeks before the qemu 2.12 release 
cements in place an implementation of the BLOCK_STATUS extension.  And 
we've already discussed that if we make a change, we have to consider 
using a different constant for NBD_OPT_SET_META_CONTEXT to play nicely 
with the existing Virtuozzo implementation that currently matches what 
is slated to land in qemu 2.12 if no further qemu patches are submitted. 
  Is it worth me proposing a doc change to demonstrate what the 
difference would look like, along with corresponding qemu changes to 
match, to decide if including it in 2.12 is worth it?

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

Re: [Qemu-devel] [PATCH 2/4] nbd/server: add nbd_meta_single_query helper

[Eric Blake]

On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> The helper will be reused for bitmaps namespace.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   nbd/server.c | 41 ++++++++++++++++++++++++++++-------------
>   1 file changed, 28 insertions(+), 13 deletions(-)
> 

> +/* Read len bytes and check matching to the pattern.
> + * @match is set to true on empty query for _LIST_ and for query matching the
> + * @pattern. @match is never set to false.

How about:

Read @len bytes, and set @match to true if they match @pattern, or if 
@len is 0 and the client is performing _LIST_.  @match is never set to 
false.

At any rate, the refactoring is sane; and comment touchups are trivial, so

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH 3/4] nbd/server: implement dirty bitmap export

[Eric Blake]

On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Rather sparse on the details in the commit message; I had to read the 
patch to even learn what the new namespace is.

> ---
>   include/block/nbd.h |   2 +
>   nbd/server.c        | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 203 insertions(+), 6 deletions(-)
> 

> +++ b/nbd/server.c
> @@ -23,6 +23,12 @@
>   #include "nbd-internal.h"
>   
>   #define NBD_META_ID_BASE_ALLOCATION 0
> +#define NBD_META_ID_DIRTY_BITMAP 1
> +
> +#define NBD_MAX_BITMAP_EXTENTS (0x100000 / 8) /* 1 mb of extents data */
> +#define MAX_EXTENT_LENGTH QEMU_ALIGN_DOWN(INT32_MAX, 512)

Worth comments on these two limits?

> +
> +#define NBD_STATE_DIRTY 1

Does this belong better in nbd.h, alongside NBD_STATE_HOLE?  (And 
defining it as (1 << 0) might be nicer, too).

>   
>   static int system_errno_to_nbd_errno(int err)
>   {
> @@ -80,6 +86,9 @@ struct NBDExport {
>   
>       BlockBackend *eject_notifier_blk;
>       Notifier eject_notifier;
> +
> +    BdrvDirtyBitmap *export_bitmap;
> +    char *export_bitmap_name;
>   };
>   
>   static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
> @@ -92,6 +101,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 dirty_bitmap; /* export qemu-dirty-bitmap:<export_bitmap_name> */

That's a LONG namespace name, and it does not lend itself well to other 
qemu extensions; so future qemu BLOCK_STATUS additions would require 
consuming yet another namespace and additional upstream NBD 
registration.  Wouldn't it be better to have the namespace be 'qemu:' 
(which we register with upstream NBD just once), where we are then free 
to document leafnames to look like 'dirty-bitmap:name' or 'foo:bar'?

>   } NBDExportMetaContexts;
>   
>   struct NBDClient {
> @@ -786,12 +796,32 @@ static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
>                                    &meta->base_allocation, errp);
>   }
>   
> +/* nbd_meta_bitmap_query
> + *
> + * Handle query to 'qemu-dirty-bitmap' namespace.
> + * 'len' is the amount of text remaining to be read from the current name, after
> + * the 'qemu-dirty-bitmap:' portion has been stripped.
> + *
> + * Return -errno on I/O error, 0 if option was completely handled by
> + * sending a reply about inconsistent lengths, or 1 on success. */
> +static int nbd_meta_bitmap_query(NBDClient *client, NBDExportMetaContexts *meta,
> +                                 uint32_t len, Error **errp)
> +{
> +    if (!client->exp->export_bitmap) {
> +        return nbd_opt_skip(client, len, errp);
> +    }
> +
> +    return nbd_meta_single_query(client, client->exp->export_bitmap_name, len,
> +                                 &meta->dirty_bitmap, errp);

So if I'm reading this right, a client can do _LIST_ 
'qemu-dirty-bitmap:' (or 'qemu:dirty-bitmap:' if we choose a shorter 
initial namespace) and get back the exported bitmap name; or the user 
already knows the bitmap name and both _LIST_ and _SET_ 
'qemu-dirty-bitmap:name' return the one supported bitmap for that NBD 
server.


>   /* nbd_co_send_extents
> + *
> + * NBD_REPLY_FLAG_DONE is not set, don't forget to send it.
> + *
>    * @extents should be in big-endian */
>   static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
>                                  NBDExtent *extents, unsigned nb_extents,

Worth a bool flag that the caller can inform this function whether to 
include FLAG_DONE?
> +
> +static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
> +                              BdrvDirtyBitmap *bitmap, uint64_t offset,
> +                              uint64_t length, uint32_t context_id,
> +                              Error **errp)
> +{
> +    int ret;
> +    unsigned nb_extents;
> +    NBDExtent *extents = g_new(NBDExtent, NBD_MAX_BITMAP_EXTENTS);

Is this correct even when the client used NBD_CMD_FLAG_REQ_ONE?

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

Re: [Qemu-devel] [PATCH 1/4] nbd/server: refactor nbd_negotiate_meta_query for several namespaces

[Wouter Verhelst]

On Wed, Mar 21, 2018 at 09:56:36AM -0500, Eric Blake wrote:
> no further qemu patches are submitted.  Is it worth me proposing a doc
> change to demonstrate what the difference would look like, along with
> corresponding qemu changes to match, to decide if including it in 2.12 is
> worth it?

If you're willing to spend the effort, I won't stop you :-) but I think the
pain of parsing a string once (during negotiation) is not that bad, and
I'm not too bothered about it.

Put otherwise, I'm not convinced that the downside (which I agree
exists) outweighs the downside of having to introduce yet another
command, or add some ugliness for backwards compat reasons.

But hey, go ahead, prove me wrong ;-)

-- 
Could you people please use IRC like normal people?!?

  -- Amaya Rodrigo Sastre, trying to quiet down the buzz in the DebConf 2008
     Hacklab

Re: [Qemu-devel] [PATCH 4/4] qapi: new qmp command nbd-server-add-bitmap

[Eric Blake]

On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block.json | 27 +++++++++++++++++++++++++++
>   blockdev-nbd.c  | 23 +++++++++++++++++++++++
>   2 files changed, 50 insertions(+)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index c694524002..4afbbcd7b7 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -269,6 +269,33 @@
>     'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }
>   
>   ##
> +# @nbd-server-add-bitmap:
> +#
> +# Export dirty bitmap through selected export. Bitmaps are searched for in
> +# device attached to the export and in all its backings. Exported bitmap
> +# is locked until NBD export is removed.

Expose a dirty bitmap associated with the selected export.  The bitmap 
search starts at the device attached to the export, and includes all 
backing files.  The exported bitmap is then locked until the NBD export 
is removed.

> +#
> +# @name: Export name.
> +#
> +# @bitmap: Bitmap name to search.

s/search./search for./

> +#
> +# @bitmap-export-name: How the bitmap will be seen by nbd clients
> +#                      (default @bitmap)

Maybe mention that the client must use NBD_OPT_SET_META_CONTEXT with a 
query of "qemu-dirty-bitmap:NAME" (where NAME matches 
bitmap-export-name) to access the exposed bitmap.  (May need to be 
adjusted by my suggestion to use just the namespace "qemu:")

> +#
> +#
> +# Returns: error on one of the following conditions:
> +#           - the server is not running
> +#           - export is not found
> +#           - bitmap is not found
> +#           - bitmap is disabled
> +#           - bitmap is locked

Do we really need to list all the error conditions?  My worry is that a 
list this specific might go stale, compared to the obvious default that 
the command succeeds only if it was able to expose the bitmap and that 
the error message is specific enough for a human to figure out what to 
fix if it failed.

> +#
> +# Since: 2.13
> +##
> +  { 'command': 'nbd-server-add-bitmap',
> +    'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 'str'} }
> +

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

Re: [Qemu-devel] [PATCH 1/4] nbd/server: refactor nbd_negotiate_meta_query for several namespaces

[Vladimir Sementsov-Ogievskiy]

21.03.2018 17:56, Eric Blake wrote:
> [adding NBD list]
>
> On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/server.c | 60 
>> +++++++++++++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 43 insertions(+), 17 deletions(-)
>
>> +struct {
>> +    const char *ns;
>> +    int (*func)(NBDClient *, NBDExportMetaContexts *, uint32_t, 
>> Error **);
>> +} meta_namespace_handlers[] = {
>> +    /* namespaces should go in non-decreasing order by name length */
>> +    {.ns = "base:", .func = nbd_meta_base_query},
>> +};
>> +
>
>> @@ -787,9 +793,12 @@ static int nbd_negotiate_meta_query(NBDClient 
>> *client,
>>                                       NBDExportMetaContexts *meta, 
>> Error **errp)
>>   {
>>       int ret;
>> -    char query[sizeof("base:") - 1];
>> -    size_t baselen = strlen("base:");
>> +    int i;
>>       uint32_t len;
>> +    int bytes_done = 0;
>> +    char *query;
>> +    int nb_ns = sizeof(meta_namespace_handlers) /
>> +                sizeof(meta_namespace_handlers[0]);
>
> Use the ARRAY_SIZE() macro here.
>
>> +    query = g_malloc(strlen(meta_namespace_handlers[nb_ns - 1].ns));
>
> So this sizes a buffer according to the largest namespace we expect to 
> handle,...
>
>>   -    len -= baselen;
>> -    ret = nbd_opt_read(client, query, baselen, errp);
>> -    if (ret <= 0) {
>> -        return ret;
>> -    }
>> -    if (strncmp(query, "base:", baselen) != 0) {
>> -        return nbd_opt_skip(client, len, errp);
>> +    for (i = 0; i < nb_ns; i++) {
>> +        const char *ns = meta_namespace_handlers[i].ns;
>> +        int ns_len = strlen(ns);
>> +        int diff_len = strlen(ns) - bytes_done;
>> +
>> +        assert(diff_len >= 0);
>> +
>> +        if (diff_len > 0) {
>> +            if (len < diff_len) {
>> +                ret = nbd_opt_skip(client, len, errp);
>> +                goto out;
>> +            }
>> +
>> +            len -= diff_len;
>> +            ret = nbd_opt_read(client, query + bytes_done, diff_len, 
>> errp);
>> +            if (ret <= 0) {
>> +                goto out;
>> +            }
>> +        }
>> +
>> +        if (!strncmp(query, ns, ns_len)) {
>> +            ret = meta_namespace_handlers[i].func(client, meta, len, 
>> errp);
>> +            goto out;
>> +        }
>
>
> ...then you do multiple iterative reads as you step through the list. 
> You know, if you encounter a ':' at any point in the iterative reads, 
> you don't have to continue through the rest of the handlers (the query 
> belongs to a short-named namespace we didn't recognize).
>
> Is it any smarter to just blindly do a single read of MIN(querylen, 
> maxlen), then strchr() for ':' (if not found, it's not a namespace we 
> recognize), and then look up if the name matches, at which point we 
> then read the rest of the query and refactor the namespace handler to 
> be passed the already-parsed leafname, rather than having to parse the 
> leafname off the wire in the handler?

but what if we get base:very-long-garbage? Only namespace handler knows, 
should we read leafname to the memory or just drop. So, in your case, 
we'll have to refactor it to handle partly read query..
for now, we can do it as simple as:

1. read first letter.
2. if first_letter == 'b' and len == strlen(base:alloction): read, 
check, set meta.base_allocation if match
3. if first_letter == 'q' and len == strlen(qemu-drity-bitmap:) + 
strlen(exp.export_bitmap_name): read, check, set meta.dirty_bitmap if match

and forget about generic code, until we really need it

>
> I'm STILL wondering if the NBD spec should specify namespace and 
> leafname as separate fields rather than requiring the server to parse 
> for ':'.  We have only a couple of weeks before the qemu 2.12 release 
> cements in place an implementation of the BLOCK_STATUS extension.  And 
> we've already discussed that if we make a change, we have to consider 
> using a different constant for NBD_OPT_SET_META_CONTEXT to play nicely 
> with the existing Virtuozzo implementation that currently matches what 
> is slated to land in qemu 2.12 if no further qemu patches are 
> submitted.  Is it worth me proposing a doc change to demonstrate what 
> the difference would look like, along with corresponding qemu changes 
> to match, to decide if including it in 2.12 is worth it?
>

Personally, I'd prefer to avoid it.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/4] nbd/server: implement dirty bitmap export

[Vladimir Sementsov-Ogievskiy]

21.03.2018 19:57, Eric Blake wrote:
> On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Rather sparse on the details in the commit message; I had to read the 
> patch to even learn what the new namespace is.

oh, yes, sorry :(

>> @@ -92,6 +101,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 dirty_bitmap; /* export 
>> qemu-dirty-bitmap:<export_bitmap_name> */
>
> That's a LONG namespace name, and it does not lend itself well to 
> other qemu extensions; so future qemu BLOCK_STATUS additions would 
> require consuming yet another namespace and additional upstream NBD 
> registration.  Wouldn't it be better to have the namespace be 'qemu:' 
> (which we register with upstream NBD just once), where we are then 
> free to document leafnames to look like 'dirty-bitmap:name' or 'foo:bar'?

No doubts. (and this shows, that we may want namespaces in namespaces, 
so we'll parse ':' anyway).

Only one note here (I'm ashamed): 'qemu-dirty-bitmap' namespace is used 
in Virtuozzo for the feature, yes, without 'x-' prefix. It's my fault, 
and it should not influence final solution. The probability of problems 
is near to zero (especially if we decide now to use 'qemu:' namespace 
for all qemu-specific things. But I'm not sure about, should we mention 
this fact in the spec.
[cc NBD list]

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/4] nbd/server: implement dirty bitmap export

[Vladimir Sementsov-Ogievskiy]

21.03.2018 19:57, Eric Blake wrote:
> On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>

[...]

>
>> +
>> +#define NBD_STATE_DIRTY 1
>
> Does this belong better in nbd.h, alongside NBD_STATE_HOLE?  (And 
> defining it as (1 << 0) might be nicer, too).

It's not used anywhere else, but it may be worth moving, for consistency 
and for future users. Ok, I'll move.


[...]

>>   +/* nbd_meta_bitmap_query
>> + *
>> + * Handle query to 'qemu-dirty-bitmap' namespace.
>> + * 'len' is the amount of text remaining to be read from the current 
>> name, after
>> + * the 'qemu-dirty-bitmap:' portion has been stripped.
>> + *
>> + * Return -errno on I/O error, 0 if option was completely handled by
>> + * sending a reply about inconsistent lengths, or 1 on success. */
>> +static int nbd_meta_bitmap_query(NBDClient *client, 
>> NBDExportMetaContexts *meta,
>> +                                 uint32_t len, Error **errp)
>> +{
>> +    if (!client->exp->export_bitmap) {
>> +        return nbd_opt_skip(client, len, errp);
>> +    }
>> +
>> +    return nbd_meta_single_query(client, 
>> client->exp->export_bitmap_name, len,
>> +                                 &meta->dirty_bitmap, errp);
>
> So if I'm reading this right, a client can do _LIST_ 
> 'qemu-dirty-bitmap:' (or 'qemu:dirty-bitmap:' if we choose a shorter 
> initial namespace) and get back the exported bitmap name; or the user 
> already knows the bitmap name and both _LIST_ and _SET_ 
> 'qemu-dirty-bitmap:name' return the one supported bitmap for that NBD 
> server.

yes

>
>
>>   /* nbd_co_send_extents
>> + *
>> + * NBD_REPLY_FLAG_DONE is not set, don't forget to send it.
>> + *
>>    * @extents should be in big-endian */
>>   static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
>>                                  NBDExtent *extents, unsigned 
>> nb_extents,
>
> Worth a bool flag that the caller can inform this function whether to 
> include FLAG_DONE?

It was simpler to just send it separately, after all BLOCK_STATUS 
replies. So, I didn't need it.

>> +
>> +static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
>> +                              BdrvDirtyBitmap *bitmap, uint64_t offset,
>> +                              uint64_t length, uint32_t context_id,
>> +                              Error **errp)
>> +{
>> +    int ret;
>> +    unsigned nb_extents;
>> +    NBDExtent *extents = g_new(NBDExtent, NBD_MAX_BITMAP_EXTENTS);
>
> Is this correct even when the client used NBD_CMD_FLAG_REQ_ONE?
>

Oops, looks like a bug.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/4] qapi: new qmp command nbd-server-add-bitmap

[Vladimir Sementsov-Ogievskiy]

21.03.2018 20:33, Eric Blake wrote:
> On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block.json | 27 +++++++++++++++++++++++++++
>>   blockdev-nbd.c  | 23 +++++++++++++++++++++++
>>   2 files changed, 50 insertions(+)
>>
>> diff --git a/qapi/block.json b/qapi/block.json
>> index c694524002..4afbbcd7b7 100644
>> --- a/qapi/block.json
>> +++ b/qapi/block.json
>> @@ -269,6 +269,33 @@
>>     'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }
>>     ##
>> +# @nbd-server-add-bitmap:
>> +#
>> +# Export dirty bitmap through selected export. Bitmaps are searched 
>> for in
>> +# device attached to the export and in all its backings. Exported 
>> bitmap
>> +# is locked until NBD export is removed.
>
> Expose a dirty bitmap associated with the selected export.  The bitmap 
> search starts at the device attached to the export, and includes all 
> backing files.  The exported bitmap is then locked until the NBD 
> export is removed.
>
>> +#
>> +# @name: Export name.
>> +#
>> +# @bitmap: Bitmap name to search.
>
> s/search./search for./
>
>> +#
>> +# @bitmap-export-name: How the bitmap will be seen by nbd clients
>> +#                      (default @bitmap)
>
> Maybe mention that the client must use NBD_OPT_SET_META_CONTEXT with a 
> query of "qemu-dirty-bitmap:NAME" (where NAME matches 
> bitmap-export-name) to access the exposed bitmap.  (May need to be 
> adjusted by my suggestion to use just the namespace "qemu:")

ok

>
>> +#
>> +#
>> +# Returns: error on one of the following conditions:
>> +#           - the server is not running
>> +#           - export is not found
>> +#           - bitmap is not found
>> +#           - bitmap is disabled
>> +#           - bitmap is locked
>
> Do we really need to list all the error conditions?  My worry is that 
> a list this specific might go stale, compared to the obvious default 
> that the command succeeds only if it was able to expose the bitmap and 
> that the error message is specific enough for a human to figure out 
> what to fix if it failed.


Hmm, I've just doing it similar with other commands in the file. Is 
there any requirements on this part of qapi documentation? I can write 
only "# Returns: nothing on success" line, is it appropriate? 
blockdev-mirror do so, but other commands tries to describe errors. 
Looks like we lack some specified format for return code description 
like we have for parameters..

>
>> +#
>> +# Since: 2.13
>> +##
>> +  { 'command': 'nbd-server-add-bitmap',
>> +    'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 
>> 'str'} }
>> +
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/4] qapi: new qmp command nbd-server-add-bitmap

[Eric Blake]

On 03/22/2018 10:43 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> +# Returns: error on one of the following conditions:
>>> +#           - the server is not running
>>> +#           - export is not found
>>> +#           - bitmap is not found
>>> +#           - bitmap is disabled
>>> +#           - bitmap is locked
>>
>> Do we really need to list all the error conditions?  My worry is that 
>> a list this specific might go stale, compared to the obvious default 
>> that the command succeeds only if it was able to expose the bitmap and 
>> that the error message is specific enough for a human to figure out 
>> what to fix if it failed.
> 
> 
> Hmm, I've just doing it similar with other commands in the file. Is 
> there any requirements on this part of qapi documentation? I can write 
> only "# Returns: nothing on success" line, is it appropriate? 
> blockdev-mirror do so, but other commands tries to describe errors. 
> Looks like we lack some specified format for return code description 
> like we have for parameters..

Yeah, for returns, it's been very ad hoc.  My personal feel (although 
it's not very well documented and certainly not enforced): all commands 
can reasonably return errors, presumably for a good reason; but 
exhaustively auditing WHICH errors is a huge task with little benefits. 
A few commands return non-generic errors, but if all error paths used 
error_setg(), there's nothing that a machine can do to tell the 
difference between the errors, so documenting different error reasons 
doesn't add much.

Thus, if a command returns nothing on success, I'm fine with omitting 
'Returns:' entirely, and the doc generator permits that.  But if you 
have bothered to list Returns: for certain errors, I'm not going to 
blindly throw away the documentation work, even though the list may 
become incomplete over time.

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

Re: [Qemu-devel] [PATCH 4/4] qapi: new qmp command nbd-server-add-bitmap

[Vladimir Sementsov-Ogievskiy]

22.03.2018 19:19, Eric Blake wrote:
> On 03/22/2018 10:43 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>> +# Returns: error on one of the following conditions:
>>>> +#           - the server is not running
>>>> +#           - export is not found
>>>> +#           - bitmap is not found
>>>> +#           - bitmap is disabled
>>>> +#           - bitmap is locked
>>>
>>> Do we really need to list all the error conditions?  My worry is 
>>> that a list this specific might go stale, compared to the obvious 
>>> default that the command succeeds only if it was able to expose the 
>>> bitmap and that the error message is specific enough for a human to 
>>> figure out what to fix if it failed.
>>
>>
>> Hmm, I've just doing it similar with other commands in the file. Is 
>> there any requirements on this part of qapi documentation? I can 
>> write only "# Returns: nothing on success" line, is it appropriate? 
>> blockdev-mirror do so, but other commands tries to describe errors. 
>> Looks like we lack some specified format for return code description 
>> like we have for parameters..
>
> Yeah, for returns, it's been very ad hoc.  My personal feel (although 
> it's not very well documented and certainly not enforced): all 
> commands can reasonably return errors, presumably for a good reason; 
> but exhaustively auditing WHICH errors is a huge task with little 
> benefits. A few commands return non-generic errors, but if all error 
> paths used error_setg(), there's nothing that a machine can do to tell 
> the difference between the errors, so documenting different error 
> reasons doesn't add much.
>
> Thus, if a command returns nothing on success, I'm fine with omitting 
> 'Returns:' entirely, and the doc generator permits that. But if you 
> have bothered to list Returns: for certain errors, I'm not going to 
> blindly throw away the documentation work, even though the list may 
> become incomplete over time.
>

So, only something interesting worth documenting.

Hmm, interesting: consider for example bloc-dirty-bitmap-remove. It says:
# Returns: nothing on success
#          If @node is not a valid block device or node, DeviceNotFound

But the code uses bdrv_lookup_bs, which uses simple error_setg, so it 
will return GenericError, isn't it? And, it's not the only place..

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/4] nbd/server: implement dirty bitmap export

[Vladimir Sementsov-Ogievskiy]

22.03.2018 18:26, Vladimir Sementsov-Ogievskiy wrote:
> 21.03.2018 19:57, Eric Blake wrote:
>> On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Rather sparse on the details in the commit message; I had to read the 
>> patch to even learn what the new namespace is.
>
> oh, yes, sorry :(
>
>>> @@ -92,6 +101,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 dirty_bitmap; /* export 
>>> qemu-dirty-bitmap:<export_bitmap_name> */
>>
>> That's a LONG namespace name, and it does not lend itself well to 
>> other qemu extensions; so future qemu BLOCK_STATUS additions would 
>> require consuming yet another namespace and additional upstream NBD 
>> registration.  Wouldn't it be better to have the namespace be 'qemu:' 
>> (which we register with upstream NBD just once), where we are then 
>> free to document leafnames to look like 'dirty-bitmap:name' or 
>> 'foo:bar'?
>
> No doubts. (and this shows, that we may want namespaces in namespaces, 
> so we'll parse ':' anyway).
>
> Only one note here (I'm ashamed): 'qemu-dirty-bitmap' namespace is 
> used in Virtuozzo for the feature, yes, without 'x-' prefix. It's my 
> fault, and it should not influence final solution. The probability of 
> problems is near to zero (especially if we decide now to use 'qemu:' 
> namespace for all qemu-specific things. But I'm not sure about, should 
> we mention this fact in the spec.
> [cc NBD list]
>

Looks like at this point, we can safely move to qemu:dirty-bitmap:name, 
or any other thing, and we don't need to support my first variant.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/4] nbd/server: add nbd_meta_single_query helper

[Vladimir Sementsov-Ogievskiy]

21.03.2018 18:05, Eric Blake wrote:
> On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The helper will be reused for bitmaps namespace.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/server.c | 41 ++++++++++++++++++++++++++++-------------
>>   1 file changed, 28 insertions(+), 13 deletions(-)
>>
>
>> +/* Read len bytes and check matching to the pattern.
>> + * @match is set to true on empty query for _LIST_ and for query 
>> matching the
>> + * @pattern. @match is never set to false.
>
> How about:
>
> Read @len bytes, and set @match to true if they match @pattern, or if 
> @len is 0 and the client is performing _LIST_.  @match is never set to 
> false.

will add, as always, thank you for natural rewording) Hm, I have a 
question: why do you often use double white-space "  " between 
sentences? Is it something meaningful?

>
> At any rate, the refactoring is sane; and comment touchups are 
> trivial, so
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] nbd/server: add nbd_meta_single_query helper

[John Snow]

On 04/13/2018 01:44 PM, Vladimir Sementsov-Ogievskiy wrote:
> 
> will add, as always, thank you for natural rewording) Hm, I have a
> question: why do you often use double white-space "  " between
> sentences? Is it something meaningful?

There is some GREAT DEBATE in the English-speaking world over whether or
not this is correct typography. At one point, it was thought that there
should be two spaces after every full stop (".") to improve readability.
Allegedly, this was most important for physical typesetting on typewriters.

Since digital typography has taken off, some people argue that this is a
relic and that semantically we ought to be using only one literal space
after the full stop, and using various kerning and display parameters to
extend the physical buffer between two sentences if desired.

Famously in my mind, PEP8 mandates the two spaces after a period style.

MLA says "One, unless your professor prefers two."
https://style.mla.org/number-of-spaces-after-period/

Chicago Manual of Style mandates one space after the full stop.
http://www.chicagomanualofstyle.org/qanda/data/faq/topics/OneSpaceorTwo.html

Strunk & White uses one space after the period:
https://www.legalwatercoolerblog.com/tag/strunk-and-white/

APA style (American Psychiatric Association) actually requests two
spaces after a period for *manuscripts*:
https://owl.english.purdue.edu/owl/resource/560/24/

I think it used to be the pedagogical norm to instruct students to type
two spaces after a period. Most institutions (Python documentation
excluded) do not recommend the practice currently.



*cough*



So it's not just the programming world that argues about things like
"tabs vs spaces." Literary nerds do it too.

I'm sure this email will be entirely without controversy. :)


Happy Friday,
--js

Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] nbd/server: add nbd_meta_single_query helper

[Vladimir Sementsov-Ogievskiy]

14.04.2018 00:06, John Snow wrote:
>
> On 04/13/2018 01:44 PM, Vladimir Sementsov-Ogievskiy wrote:
>> will add, as always, thank you for natural rewording) Hm, I have a
>> question: why do you often use double white-space "  " between
>> sentences? Is it something meaningful?
> There is some GREAT DEBATE in the English-speaking world over whether or
> not this is correct typography. At one point, it was thought that there
> should be two spaces after every full stop (".") to improve readability.
> Allegedly, this was most important for physical typesetting on typewriters.
>
> Since digital typography has taken off, some people argue that this is a
> relic and that semantically we ought to be using only one literal space
> after the full stop, and using various kerning and display parameters to
> extend the physical buffer between two sentences if desired.
>
> Famously in my mind, PEP8 mandates the two spaces after a period style.
>
> MLA says "One, unless your professor prefers two."
> https://style.mla.org/number-of-spaces-after-period/
>
> Chicago Manual of Style mandates one space after the full stop.
> http://www.chicagomanualofstyle.org/qanda/data/faq/topics/OneSpaceorTwo.html
>
> Strunk & White uses one space after the period:
> https://www.legalwatercoolerblog.com/tag/strunk-and-white/
>
> APA style (American Psychiatric Association) actually requests two
> spaces after a period for *manuscripts*:
> https://owl.english.purdue.edu/owl/resource/560/24/
>
> I think it used to be the pedagogical norm to instruct students to type
> two spaces after a period. Most institutions (Python documentation
> excluded) do not recommend the practice currently.
>
>
>
> *cough*
>
>
>
> So it's not just the programming world that argues about things like
> "tabs vs spaces." Literary nerds do it too.
>
> I'm sure this email will be entirely without controversy. :)
>
>
> Happy Friday,
> --js

Got it, thank you!


-- 
Best regards,
Vladimir