[Qemu-devel] [PATCH v3 0/8] Promote x-nbd-server-add-bitmap to stable

[Qemu-devel] [PATCH v3 0/8] Promote x-nbd-server-add-bitmap to stable

[Eric Blake]

Or rather, move its functionality into nbd-server-add.  And as
a side effect, teach qemu-nbd how to export a persistent bitmap
without having to go through a qemu process and several QMP
commands.

Based-on: <20181221093529.23855-1-jsnow@redhat.com>
[jsnow: 0/11 bitmaps: remove x- prefix from QMP api]
Based-on: <20190111163519.11457-1-philmd@redhat.com>
[philmd: qemu-nbd: Rename 'exp' variable clashing with math::exp() symbol]

Available at: https://repo.or.cz/qemu/ericb.git nbd-bitmap-add-v3

Since v2:
- split old patch 1 into 3 parts
- add even more tests of expected error messages
- rebase on top of 'exp' rename
- improve commit messages
- add some R-b where it made sense

001/8:[down] 'nbd: Add some error case testing to iotests 223'
002/8:[down] 'nbd: Forbid nbd-server-stop when server is not running'
003/8:[0011] [FC] 'nbd: Only require disabled bitmap for read-only exports'
004/8:[0020] [FC] 'nbd: Merge nbd_export_set_name into nbd_export_new'
005/8:[0008] [FC] 'nbd: Allow bitmap export during QMP nbd-server-add'
006/8:[----] [--] 'nbd: Remove x-nbd-server-add-bitmap'
007/8:[0009] [FC] 'nbd: Merge nbd_export_bitmap into nbd_export_new'
008/8:[0004] [FC] 'qemu-nbd: Add --bitmap=NAME option'

Eric Blake (8):
  nbd: Add some error case testing to iotests 223
  nbd: Forbid nbd-server-stop when server is not running
  nbd: Only require disabled bitmap for read-only exports
  nbd: Merge nbd_export_set_name into nbd_export_new
  nbd: Allow bitmap export during QMP nbd-server-add
  nbd: Remove x-nbd-server-add-bitmap
  nbd: Merge nbd_export_bitmap into nbd_export_new
  qemu-nbd: Add --bitmap=NAME option

 qemu-nbd.texi              |   4 ++
 qapi/block.json            |  30 ++------
 include/block/nbd.h        |  12 ++--
 blockdev-nbd.c             |  36 +++-------
 hmp.c                      |   5 +-
 nbd/server.c               | 136 +++++++++++++++++--------------------
 qemu-nbd.c                 |  17 +++--
 tests/qemu-iotests/223     |  50 +++++++++++---
 tests/qemu-iotests/223.out |  23 +++++--
 9 files changed, 160 insertions(+), 153 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v3 1/8] nbd: Add some error case testing to iotests 223

[Eric Blake]

Testing success paths is important, but it's also nice to highlight
expected failure handling, to show that we don't crash, and so that
upcoming tests that change behavior can demonstrate the resulting
effects on error paths.

Add the following errors:
Attempting to export without a running server
Attempting to start a second server
Attempting to export a bad node name
Attempting to export a name that is already exported
Attempting to export an enabled bitmap
Attempting to clean an already cleaned export
Attempting to quit server a second time

All of these properly complain except for a second server-stop,
which will be fixed next.

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

---
v3: split out unrelated error path testing and expand [Vladimir]
---
 tests/qemu-iotests/223     | 19 +++++++++++++++++--
 tests/qemu-iotests/223.out |  7 +++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 5513dc62159..61b46a2f066 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -107,6 +107,7 @@ echo

 _launch_qemu 2> >(_filter_nbd)

+# Intentionally provoke some errors as well, to check error handling
 silent=
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"qmp_capabilities"}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
@@ -114,17 +115,28 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
   "arguments":{"node":"n", "name":"b"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
-  "arguments":{"node":"n", "name":"b2"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}' "error" # Attempt add without server
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
   "arguments":{"addr":{"type":"unix",
     "data":{"path":"'"$TEST_DIR/nbd"'"}}}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
+  "arguments":{"addr":{"type":"unix",
+    "data":{"path":"'"$TEST_DIR/nbd"1'"}}}}' "error" # Attempt second server
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments":{"device":"nosuch"}}' "error" # Attempt to export missing node
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}' "error" # Attempt to export same name twice
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
   "arguments":{"name":"n", "bitmap":"b"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "name":"n2"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
+  "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Attempt enabled bitmap
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
+  "arguments":{"node":"n", "name":"b2"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
   "arguments":{"name":"n2", "bitmap":"b2"}}' "return"

@@ -157,7 +169,10 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
   "arguments":{"name":"n"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
   "arguments":{"name":"n2"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
+  "arguments":{"name":"n2"}}' "error" # Attempt duplicate clean
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return" # Oops
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"

 # success, all done
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 99ca172fbb8..e6ede0591cd 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -27,10 +27,15 @@ wrote 2097152/2097152 bytes at offset 2097152
 {"return": {}}
 {"return": {}}
 {"return": {}}
+{"error": {"class": "GenericError", "desc": "NBD server not running"}}
 {"return": {}}
+{"error": {"class": "GenericError", "desc": "NBD server already running"}}
 {"return": {}}
+{"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}}
+{"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
 {"return": {}}
 {"return": {}}
+{"error": {"class": "GenericError", "desc": "Bitmap 'b2' is enabled"}}
 {"return": {}}
 {"return": {}}

@@ -62,6 +67,8 @@ read 2097152/2097152 bytes at offset 2097152

 {"return": {}}
 {"return": {}}
+{"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
+{"return": {}}
 {"return": {}}
 {"return": {}}
 *** done
-- 
2.20.1

[Qemu-devel] [PATCH v3 2/8] nbd: Forbid nbd-server-stop when server is not running

[Eric Blake]

Since we already forbid other nbd-server commands when not
in the right state, it is unlikely that any caller was relying
on a second stop to behave as a silent no-op.  Update iotest
223 to show the improved behavior.

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

---
v3: new patch
---
 blockdev-nbd.c             | 5 +++++
 tests/qemu-iotests/223     | 2 +-
 tests/qemu-iotests/223.out | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 1d170c80b82..ca584919194 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -214,6 +214,11 @@ void qmp_nbd_server_remove(const char *name,

 void qmp_nbd_server_stop(Error **errp)
 {
+    if (!nbd_server) {
+        error_setg(errp, "NBD server not running");
+        return;
+    }
+
     nbd_export_close_all();

     nbd_server_free(nbd_server);
diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 61b46a2f066..a4016091b21 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -172,7 +172,7 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
   "arguments":{"name":"n2"}}' "error" # Attempt duplicate clean
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return" # Oops
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "error" # Again
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"

 # success, all done
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index e6ede0591cd..8a4d63a4fc2 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -69,6 +69,6 @@ read 2097152/2097152 bytes at offset 2097152
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
 {"return": {}}
-{"return": {}}
+{"error": {"class": "GenericError", "desc": "NBD server not running"}}
 {"return": {}}
 *** done
-- 
2.20.1

[Qemu-devel] [PATCH v3 3/8] nbd: Only require disabled bitmap for read-only exports

[Eric Blake]

Our initial implementation of x-nbd-server-add-bitmap put
in a restriction because of incremental backups: in that
usage, we are exporting one qcow2 file (the temporary overlay
target of a blockdev-backup sync:none job) and a dirty bitmap
owned by a second qcow2 file (the source of the
blockdev-backup, which is the backing file of the temporary).
While both qcow2 files are still writable (the target in
order to capture copy-on-write of old contents, and the
source in order to track live guest writes in the meantime),
the NBD client expects to see constant data, including the
dirty bitmap.  An enabled bitmap in the source would be
modified by guest writes, which is at odds with the NBD
export being a read-only constant view, hence the initial
code choice of enforcing a disabled bitmap (the intent is
that the exposed bitmap was disabled in the same transaction
that started the blockdev-backup job, although we don't want
to track enough state to actually enforce that).

However, consider the case of a bitmap contained in a read-only
node (including when the bitmap is found in a backing layer of
the active image).  Because the node can't be modified, the
bitmap won't change due to writes, regardless of whether it is
still enabled.  Forbidding the export unless the bitmap is
disabled is awkward, paritcularly since we can't change the
bitmap to be disabled (because the node is read-only).

Alternatively, consider the case of live storage migration,
where management directs the destination to create a writable
NBD server, then performs a drive-mirror from the source to
the mirror, prior to doing the rest of the live migration.
Since storage migration can be time-consuming, it may be wise
to let the destination include a dirty bitmap to track which
portions it has already received, where even if the migration
is interrupted and restarted, the source can query the
destination block status in order to minimize re-sending
data that has not changed in the meantime on a second attempt.
Such code has not been written, but it makes sense that
letting an active dirty bitmap be exposed and changing
alongside writes may prove useful in the future.

Solve both issues by gating the restriction against a
disabled bitmap to only happen when the caller has requested
a read-only export, and where the BDS that owns the bitmap
(whether or not it is the BDS handed to nbd_export_new() or
from its backing chain) is still writable.

Update iotest 223 to show the looser behavior by leaving
a bitmap enabled the whole run; note that we have to tear
down and re-export a node when handling an error.

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

---
v3: split out unrelated test changes, improve commit message [Vladimir]
v2: new patch
---
 nbd/server.c               |  7 +++++--
 tests/qemu-iotests/223     | 10 +++++++---
 tests/qemu-iotests/223.out |  3 ++-
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 7af0ddffb20..98327088cb4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2456,8 +2456,11 @@ void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
         return;
     }

-    if (bdrv_dirty_bitmap_enabled(bm)) {
-        error_setg(errp, "Bitmap '%s' is enabled", bitmap);
+    if ((exp->nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) &&
+        bdrv_dirty_bitmap_enabled(bm)) {
+        error_setg(errp,
+                   "Enabled bitmap '%s' incompatible with readonly export",
+                   bitmap);
         return;
     }

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index a4016091b21..f200e313c06 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -61,6 +61,8 @@ echo "=== Create partially sparse image, then add dirty bitmaps ==="
 echo

 # Two bitmaps, to contrast granularity issues
+# Also note that b will be disabled, while b2 is left enabled, to
+# check for read-only interactions
 _make_test_img -o cluster_size=4k 4M
 $QEMU_IO -c 'w -P 0x11 1M 2M' "$TEST_IMG" | _filter_qemu_io
 run_qemu <<EOF
@@ -134,9 +136,11 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "name":"n2"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
-  "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Attempt enabled bitmap
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
-  "arguments":{"node":"n", "name":"b2"}}' "return"
+  "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Enabled vs. read-only
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
+  "arguments":{"name":"n2"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2", "writable":true}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
   "arguments":{"name":"n2", "bitmap":"b2"}}' "return"

diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 8a4d63a4fc2..7d147291d48 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -35,7 +35,8 @@ wrote 2097152/2097152 bytes at offset 2097152
 {"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
 {"return": {}}
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "Bitmap 'b2' is enabled"}}
+{"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}}
+{"return": {}}
 {"return": {}}
 {"return": {}}

-- 
2.20.1

[Qemu-devel] [PATCH v3 4/8] nbd: Merge nbd_export_set_name into nbd_export_new

[Eric Blake]

The existing NBD code had a weird split where nbd_export_new()
created an export but did not add it to the list of exported
names until a later nbd_export_set_name() came along and grabbed
a second reference on the object; later, the first call to
nbd_export_close() drops the second reference while removing
the export from the list.  This is in part because the QAPI
NbdServerRemoveNode enum documents the possibility of adding a
mode where we could do a soft disconnect: preventing new clients,
but waiting for existing clients to gracefully quit, based on
the mode used when calling nbd_export_close().

But in spite of all that, note that we never change the name of
an NBD export while it is exposed, which means it is easier to
just inline the process of setting the name as part of creating
the export.

Inline the contents of nbd_export_set_name() and
nbd_export_set_description() into the two points in an export
lifecycle where they matter, then adjust both callers to pass
the name up front.  Note that for creation, all callers pass a
non-NULL name, (passing NULL at creation was for old style
servers, but we removed support for that in commit 7f7dfe2a),
so we can add an assert and do things unconditionally; but for
cleanup, because of the dual nature of nbd_export_close(), we
still have to be careful to avoid use-after-free.  Along the
way, add a comment reminding ourselves of the potential of
adding a middle mode disconnect.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

---
v3: add comment on potential close mode semantics [Vladimir],
rebase to 'exp' rename [Phillipe]; no major code change so R-b added
---
 include/block/nbd.h |  3 +--
 blockdev-nbd.c      |  5 ++---
 nbd/server.c        | 52 ++++++++++++++++++++-------------------------
 qemu-nbd.c          |  8 +++----
 4 files changed, 29 insertions(+), 39 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 65402d33964..2f9a2aeb73c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -295,6 +295,7 @@ typedef struct NBDExport NBDExport;
 typedef struct NBDClient NBDClient;

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
+                          const char *name, const char *description,
                           uint16_t nbdflags, void (*close)(NBDExport *),
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp);
@@ -306,8 +307,6 @@ void nbd_export_put(NBDExport *exp);
 BlockBackend *nbd_export_get_blockdev(NBDExport *exp);

 NBDExport *nbd_export_find(const char *name);
-void nbd_export_set_name(NBDExport *exp, const char *name);
-void nbd_export_set_description(NBDExport *exp, const char *description);
 void nbd_export_close_all(void);

 void nbd_client_new(QIOChannelSocket *sioc,
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index ca584919194..582ffded77f 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -174,14 +174,13 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         writable = false;
     }

-    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
+    exp = nbd_export_new(bs, 0, -1, name, NULL,
+                         writable ? 0 : NBD_FLAG_READ_ONLY,
                          NULL, false, on_eject_blk, errp);
     if (!exp) {
         return;
     }

-    nbd_export_set_name(exp, name);
-
     /* The list of named exports has a strong reference to this export now and
      * our only way of accessing it is through nbd_export_find(), so we can drop
      * the strong reference that is @exp. */
diff --git a/nbd/server.c b/nbd/server.c
index 98327088cb4..bb5438c448b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1456,6 +1456,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
 }

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
+                          const char *name, const char *description,
                           uint16_t nbdflags, void (*close)(NBDExport *),
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp)
@@ -1471,6 +1472,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
      * that BDRV_O_INACTIVE is cleared and the image is ready for write
      * access since the export could be available before migration handover.
      */
+    assert(name);
     ctx = bdrv_get_aio_context(bs);
     aio_context_acquire(ctx);
     bdrv_invalidate_cache(bs, NULL);
@@ -1494,6 +1496,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
     QTAILQ_INIT(&exp->clients);
     exp->blk = blk;
     exp->dev_offset = dev_offset;
+    exp->name = g_strdup(name);
+    exp->description = g_strdup(description);
     exp->nbdflags = nbdflags;
     exp->size = size < 0 ? blk_getlength(blk) : size;
     if (exp->size < 0) {
@@ -1513,10 +1517,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
         exp->eject_notifier.notify = nbd_eject_notifier;
         blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
     }
+    QTAILQ_INSERT_TAIL(&exports, exp, next);
+    nbd_export_get(exp);
     return exp;

 fail:
     blk_unref(blk);
+    g_free(exp->name);
+    g_free(exp->description);
     g_free(exp);
     return NULL;
 }
@@ -1533,43 +1541,29 @@ NBDExport *nbd_export_find(const char *name)
     return NULL;
 }

-void nbd_export_set_name(NBDExport *exp, const char *name)
-{
-    if (exp->name == name) {
-        return;
-    }
-
-    nbd_export_get(exp);
-    if (exp->name != NULL) {
-        g_free(exp->name);
-        exp->name = NULL;
-        QTAILQ_REMOVE(&exports, exp, next);
-        nbd_export_put(exp);
-    }
-    if (name != NULL) {
-        nbd_export_get(exp);
-        exp->name = g_strdup(name);
-        QTAILQ_INSERT_TAIL(&exports, exp, next);
-    }
-    nbd_export_put(exp);
-}
-
-void nbd_export_set_description(NBDExport *exp, const char *description)
-{
-    g_free(exp->description);
-    exp->description = g_strdup(description);
-}
-
 void nbd_export_close(NBDExport *exp)
 {
     NBDClient *client, *next;

     nbd_export_get(exp);
+    /*
+     * TODO: Should we expand QMP NbdServerRemoveNode enum to allow a
+     * close mode that stops advertising the export to new clients but
+     * still permits existing clients to run to completion? Because of
+     * that possibility, nbd_export_close() can be called more than
+     * once on an export.
+     */
     QTAILQ_FOREACH_SAFE(client, &exp->clients, next, next) {
         client_close(client, true);
     }
-    nbd_export_set_name(exp, NULL);
-    nbd_export_set_description(exp, NULL);
+    if (exp->name) {
+        nbd_export_put(exp);
+        g_free(exp->name);
+        exp->name = NULL;
+        QTAILQ_REMOVE(&exports, exp, next);
+    }
+    g_free(exp->description);
+    exp->description = NULL;
     nbd_export_put(exp);
 }

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 6ca02b6d87b..b93fa196dac 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1015,11 +1015,9 @@ int main(int argc, char **argv)
         }
     }

-    export = nbd_export_new(bs, dev_offset, fd_size, nbdflags,
-                            nbd_export_closed, writethrough,
-                            NULL, &error_fatal);
-    nbd_export_set_name(export, export_name);
-    nbd_export_set_description(export, export_description);
+    export = nbd_export_new(bs, dev_offset, fd_size, export_name,
+                            export_description, nbdflags, nbd_export_closed,
+                            writethrough, NULL, &error_fatal);

     if (device) {
 #if HAVE_NBD_DEVICE
-- 
2.20.1

[Qemu-devel] [PATCH v3 5/8] nbd: Allow bitmap export during QMP nbd-server-add

[Eric Blake]

With the experimental x-nbd-server-add-bitmap command, there was
a window of time where an NBD client could see the export but not
the associated dirty bitmap, which can cause a client that planned
on using the dirty bitmap to be forced to treat the entire image
as dirty as a safety fallback.  Furthermore, if the QMP client
successfully exports a disk but then fails to add the bitmap, it
has to take on the burden of removing the export.  Since we don't
allow changing the exposed dirty bitmap (whether to a different
bitmap, or removing advertisement of the bitmap), it is nicer to
make the bitmap tied to the export at the time the export is
created, with automatic failure to export if the bitmap is not
available.

The experimental command included an optional 'bitmap-export-name'
field for remapping the name exposed over NBD to be different from
the bitmap name stored on disk.  However, my libvirt demo code
for implementing differential backups on top of persistent bitmaps
did not need to take advantage of that feature (it is instead
possible to create a new temporary bitmap with the desired name,
use block-dirty-bitmap-merge to merge one or more persistent
bitmaps into the temporary, then associate the temporary with the
NBD export, if control is needed over the exported bitmap name).
Hence, I'm not copying that part of the experiment over to the
stable addition. For more details on the libvirt demo, see
https://www.redhat.com/archives/libvir-list/2018-October/msg01254.html,
https://kvmforum2018.sched.com/event/FzuB/facilitating-incremental-backup-eric-blake-red-hat

This patch focuses on the user interface, and reduces (but does
not completely eliminate) the window where an NBD client can see
the export but not the dirty bitmap, with less work to clean up
after errors.  Later patches will add further cleanups now that
this interface is declared stable via a single QMP command,
including removing the race window.

Update test 223 to use the new interface.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block.json            |  7 ++++++-
 blockdev-nbd.c             | 12 +++++++++++-
 hmp.c                      |  5 +++--
 tests/qemu-iotests/223     | 19 ++++++++-----------
 tests/qemu-iotests/223.out |  5 +----
 5 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 11f01f28efe..3d70420f763 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -246,6 +246,10 @@
 #
 # @writable: Whether clients should be able to write to the device via the
 #     NBD connection (default false).
+
+# @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)
 #
 # Returns: error if the server is not running, or export with the same name
 #          already exists.
@@ -253,7 +257,8 @@
 # Since: 1.3.0
 ##
 { 'command': 'nbd-server-add',
-  'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
+  'data': {'device': 'str', '*name': 'str', '*writable': 'bool',
+           '*bitmap': 'str' } }

 ##
 # @NbdServerRemoveMode:
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 582ffded77f..ec8cf0ab8c3 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -140,7 +140,8 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
 }

 void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
-                        bool has_writable, bool writable, Error **errp)
+                        bool has_writable, bool writable,
+                        bool has_bitmap, const char *bitmap, Error **errp)
 {
     BlockDriverState *bs = NULL;
     BlockBackend *on_eject_blk;
@@ -185,6 +186,15 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
      * our only way of accessing it is through nbd_export_find(), so we can drop
      * the strong reference that is @exp. */
     nbd_export_put(exp);
+
+    if (has_bitmap) {
+        Error *err = NULL;
+        nbd_export_bitmap(exp, bitmap, bitmap, &err);
+        if (err) {
+            error_propagate(errp, err);
+            nbd_export_remove(exp, NBD_SERVER_REMOVE_MODE_HARD, NULL);
+        }
+    }
 }

 void qmp_nbd_server_remove(const char *name,
diff --git a/hmp.c b/hmp.c
index 80aa5ab504b..8da5fd8760a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2326,7 +2326,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
         }

         qmp_nbd_server_add(info->value->device, false, NULL,
-                           true, writable, &local_err);
+                           true, writable, false, NULL, &local_err);

         if (local_err != NULL) {
             qmp_nbd_server_stop(NULL);
@@ -2347,7 +2347,8 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
     bool writable = qdict_get_try_bool(qdict, "writable", false);
     Error *local_err = NULL;

-    qmp_nbd_server_add(device, !!name, name, true, writable, &local_err);
+    qmp_nbd_server_add(device, !!name, name, true, writable,
+                       false, NULL, &local_err);
     hmp_handle_error(mon, &local_err);
 }

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index f200e313c06..0bcc98a8677 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -126,23 +126,20 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
   "arguments":{"addr":{"type":"unix",
     "data":{"path":"'"$TEST_DIR/nbd"1'"}}}}' "error" # Attempt second server
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
-  "arguments":{"device":"n"}}' "return"
+  "arguments":{"device":"n", "bitmap":"b"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"nosuch"}}' "error" # Attempt to export missing node
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n"}}' "error" # Attempt to export same name twice
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
-  "arguments":{"name":"n", "bitmap":"b"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
-  "arguments":{"device":"n", "name":"n2"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
-  "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Enabled vs. read-only
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
-  "arguments":{"name":"n2"}}' "return"
+  "arguments":{"device":"n", "name":"n2",
+  "bitmap":"b2"}}' "error" # enabled vs. read-only
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
-  "arguments":{"device":"n", "name":"n2", "writable":true}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
-  "arguments":{"name":"n2", "bitmap":"b2"}}' "return"
+  "arguments":{"device":"n", "name":"n2",
+  "bitmap":"b3"}}' "error" # Missing bitmap
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2", "writable":true,
+  "bitmap":"b2"}}' "return"

 echo
 echo "=== Contrast normal status to large granularity dirty-bitmap ==="
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 7d147291d48..ebd3935c974 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -33,11 +33,8 @@ wrote 2097152/2097152 bytes at offset 2097152
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}}
 {"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
-{"return": {}}
-{"return": {}}
 {"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}}
-{"return": {}}
-{"return": {}}
+{"error": {"class": "GenericError", "desc": "Bitmap 'b3' is not found"}}
 {"return": {}}

 === Contrast normal status to large granularity dirty-bitmap ===
-- 
2.20.1

[Qemu-devel] [PATCH v3 6/8] nbd: Remove x-nbd-server-add-bitmap

[Eric Blake]

Now that nbd-server-add can do the same functionality (well, other
than making the exported bitmap name different than the underlying
bitamp - but we argued that was not essential, since it is just as
easy to create a new non-persistent bitmap with the desired name),
we no longer need the experimental separate command.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block.json | 23 -----------------------
 blockdev-nbd.c  | 23 -----------------------
 2 files changed, 46 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 3d70420f763..5a79d639e8c 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -301,29 +301,6 @@
 { 'command': 'nbd-server-remove',
   'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }

-##
-# @x-nbd-server-add-bitmap:
-#
-# 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 for.
-#
-# @bitmap-export-name: How the bitmap will be seen by nbd clients
-#                      (default @bitmap)
-#
-# Note: the client must use NBD_OPT_SET_META_CONTEXT with a query of
-# "qemu:dirty-bitmap:NAME" (where NAME matches @bitmap-export-name) to access
-# the exposed bitmap.
-#
-# Since: 3.0
-##
-  { 'command': 'x-nbd-server-add-bitmap',
-    'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 'str'} }
-
 ##
 # @nbd-server-stop:
 #
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index ec8cf0ab8c3..cd86b38cdaa 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -233,26 +233,3 @@ void qmp_nbd_server_stop(Error **errp)
     nbd_server_free(nbd_server);
     nbd_server = NULL;
 }
-
-void qmp_x_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.20.1

[Qemu-devel] [PATCH v3 7/8] nbd: Merge nbd_export_bitmap into nbd_export_new

[Eric Blake]

We only have one caller that wants to export a bitmap name,
which it does right after creation of the export. But there is
still a brief window of time where an NBD client could see the
export but not the dirty bitmap, which a robust client would
have to interpret as meaning the entire image should be treated
as dirty.  Better is to eliminate the window entirely, by
inlining nbd_export_bitmap() into nbd_export_new(), and refusing
to create the bitmap in the first place if the requested bitmap
can't be located.

We also no longer need logic for setting a different bitmap
name compared to the bitmap being exported.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

---
v3: rebase to 'exp' rename [Phillipe]
---
 include/block/nbd.h |  9 ++---
 blockdev-nbd.c      | 11 +-----
 nbd/server.c        | 87 +++++++++++++++++++++------------------------
 qemu-nbd.c          |  5 +--
 4 files changed, 47 insertions(+), 65 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 2f9a2aeb73c..1971b557896 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -296,9 +296,9 @@ typedef struct NBDClient NBDClient;

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
                           const char *name, const char *description,
-                          uint16_t nbdflags, void (*close)(NBDExport *),
-                          bool writethrough, BlockBackend *on_eject_blk,
-                          Error **errp);
+                          const char *bitmap, uint16_t nbdflags,
+                          void (*close)(NBDExport *), bool writethrough,
+                          BlockBackend *on_eject_blk, Error **errp);
 void nbd_export_close(NBDExport *exp);
 void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
 void nbd_export_get(NBDExport *exp);
@@ -319,9 +319,6 @@ 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/blockdev-nbd.c b/blockdev-nbd.c
index cd86b38cdaa..c76d5416b90 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -175,7 +175,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         writable = false;
     }

-    exp = nbd_export_new(bs, 0, -1, name, NULL,
+    exp = nbd_export_new(bs, 0, -1, name, NULL, bitmap,
                          writable ? 0 : NBD_FLAG_READ_ONLY,
                          NULL, false, on_eject_blk, errp);
     if (!exp) {
@@ -186,15 +186,6 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
      * our only way of accessing it is through nbd_export_find(), so we can drop
      * the strong reference that is @exp. */
     nbd_export_put(exp);
-
-    if (has_bitmap) {
-        Error *err = NULL;
-        nbd_export_bitmap(exp, bitmap, bitmap, &err);
-        if (err) {
-            error_propagate(errp, err);
-            nbd_export_remove(exp, NBD_SERVER_REMOVE_MODE_HARD, NULL);
-        }
-    }
 }

 void qmp_nbd_server_remove(const char *name,
diff --git a/nbd/server.c b/nbd/server.c
index bb5438c448b..e8c56607eff 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1457,9 +1457,9 @@ static void nbd_eject_notifier(Notifier *n, void *data)

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
                           const char *name, const char *description,
-                          uint16_t nbdflags, void (*close)(NBDExport *),
-                          bool writethrough, BlockBackend *on_eject_blk,
-                          Error **errp)
+                          const char *bitmap, uint16_t nbdflags,
+                          void (*close)(NBDExport *), bool writethrough,
+                          BlockBackend *on_eject_blk, Error **errp)
 {
     AioContext *ctx;
     BlockBackend *blk;
@@ -1507,6 +1507,43 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
     }
     exp->size -= exp->size % BDRV_SECTOR_SIZE;

+    if (bitmap) {
+        BdrvDirtyBitmap *bm = NULL;
+        BlockDriverState *bs = blk_bs(blk);
+
+        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);
+            goto fail;
+        }
+
+        if ((nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) &&
+            bdrv_dirty_bitmap_enabled(bm)) {
+            error_setg(errp,
+                       "Enabled bitmap '%s' incompatible with readonly export",
+                       bitmap);
+            goto fail;
+        }
+
+        if (bdrv_dirty_bitmap_user_locked(bm)) {
+            error_setg(errp, "Bitmap '%s' is in use", bitmap);
+            goto fail;
+        }
+
+        bdrv_dirty_bitmap_set_qmp_locked(bm, true);
+        exp->export_bitmap = bm;
+        exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
+                                                     bitmap);
+    }
+
     exp->close = close;
     exp->ctx = blk_get_aio_context(blk);
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
@@ -2424,47 +2461,3 @@ void nbd_client_new(QIOChannelSocket *sioc,
     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 ((exp->nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) &&
-        bdrv_dirty_bitmap_enabled(bm)) {
-        error_setg(errp,
-                   "Enabled bitmap '%s' incompatible with readonly export",
-                   bitmap);
-        return;
-    }
-
-    if (bdrv_dirty_bitmap_user_locked(bm)) {
-        error_setg(errp, "Bitmap '%s' is in use", bitmap);
-        return;
-    }
-
-    bdrv_dirty_bitmap_set_qmp_locked(bm, true);
-    exp->export_bitmap = bm;
-    exp->export_bitmap_context =
-            g_strdup_printf("qemu:dirty-bitmap:%s", bitmap_export_name);
-}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index b93fa196dac..1552274c189 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1016,8 +1016,9 @@ int main(int argc, char **argv)
     }

     export = nbd_export_new(bs, dev_offset, fd_size, export_name,
-                            export_description, nbdflags, nbd_export_closed,
-                            writethrough, NULL, &error_fatal);
+                            export_description, NULL, nbdflags,
+                            nbd_export_closed, writethrough, NULL,
+                            &error_fatal);

     if (device) {
 #if HAVE_NBD_DEVICE
-- 
2.20.1

[Qemu-devel] [PATCH v3 8/8] qemu-nbd: Add --bitmap=NAME option

[Eric Blake]

Having to fire up qemu, then use QMP commands for nbd-server-start
and nbd-server-add, just to expose a persistent dirty bitmap, is
rather tedious.  Make it possible to expose a dirty bitmap using
just qemu-nbd (of course, for now this only works when qemu-nbd is
visiting a BDS formatted as qcow2).

Of course, any good feature also needs unit testing, so expand
iotest 223 to cover it.

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

---
v3: rebase to earlier changes
---
 qemu-nbd.texi              |  4 ++++
 qemu-nbd.c                 | 10 ++++++++--
 tests/qemu-iotests/223     | 18 +++++++++++++++++-
 tests/qemu-iotests/223.out | 12 +++++++++++-
 4 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9a84e81eed9..96b1546006a 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -45,6 +45,10 @@ auto-detecting
 Export the disk as read-only
 @item -P, --partition=@var{num}
 Only expose partition @var{num}
+@item -B, --bitmap=@var{name}
+If @var{filename} has a qcow2 persistent bitmap @var{name}, expose
+that bitmap via the ``qemu:dirty-bitmap:@var{name}'' context
+accessible through NBD_OPT_SET_META_CONTEXT.
 @item -s, --snapshot
 Use @var{filename} as an external snapshot, create a temporary
 file with backing_file=@var{filename}, redirect the write to
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 1552274c189..51b55f2e066 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -95,6 +95,7 @@ static void usage(const char *name)
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET       offset into the image\n"
 "  -P, --partition=NUM       only expose partition NUM\n"
+"  -B, --bitmap=NAME         expose a persistent dirty bitmap\n"
 "\n"
 "General purpose options:\n"
 "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
@@ -509,7 +510,7 @@ int main(int argc, char **argv)
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:";
+    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -519,6 +520,7 @@ int main(int argc, char **argv)
         { "offset", required_argument, NULL, 'o' },
         { "read-only", no_argument, NULL, 'r' },
         { "partition", required_argument, NULL, 'P' },
+        { "bitmap", required_argument, NULL, 'B' },
         { "connect", required_argument, NULL, 'c' },
         { "disconnect", no_argument, NULL, 'd' },
         { "snapshot", no_argument, NULL, 's' },
@@ -558,6 +560,7 @@ int main(int argc, char **argv)
     QDict *options = NULL;
     const char *export_name = ""; /* Default export name */
     const char *export_description = NULL;
+    const char *bitmap = NULL;
     const char *tlscredsid = NULL;
     bool imageOpts = false;
     bool writethrough = true;
@@ -695,6 +698,9 @@ int main(int argc, char **argv)
                 exit(EXIT_FAILURE);
             }
             break;
+        case 'B':
+            bitmap = optarg;
+            break;
         case 'k':
             sockpath = optarg;
             if (sockpath[0] != '/') {
@@ -1016,7 +1022,7 @@ int main(int argc, char **argv)
     }

     export = nbd_export_new(bs, dev_offset, fd_size, export_name,
-                            export_description, NULL, nbdflags,
+                            export_description, bitmap, nbdflags,
                             nbd_export_closed, writethrough, NULL,
                             &error_fatal);

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 0bcc98a8677..773892dbe60 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -25,6 +25,7 @@ status=1 # failure is the default!

 _cleanup()
 {
+    nbd_server_stop
     _cleanup_test_img
     _cleanup_qemu
     rm -f "$TEST_DIR/nbd"
@@ -35,6 +36,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 . ./common.qemu
+. ./common.nbd

 _supported_fmt qcow2
 _supported_proto file # uses NBD as well
@@ -163,7 +165,7 @@ $QEMU_IMG map --output=json --image-opts \
   "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map

 echo
-echo "=== End NBD server ==="
+echo "=== End qemu NBD server ==="
 echo

 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
@@ -176,6 +178,20 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "error" # Again
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"

+echo
+echo "=== Use qemu-nbd as server ==="
+echo
+
+nbd_server_start_unix_socket -r -f $IMGFMT -B b "$TEST_IMG"
+IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
+$QEMU_IMG map --output=json --image-opts \
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
+
+nbd_server_start_unix_socket -f $IMGFMT -B b2 "$TEST_IMG"
+IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
+$QEMU_IMG map --output=json --image-opts \
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index ebd3935c974..0de5240a75e 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -61,7 +61,7 @@ read 2097152/2097152 bytes at offset 2097152
 { "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true},
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]

-=== End NBD server ===
+=== End qemu NBD server ===

 {"return": {}}
 {"return": {}}
@@ -69,4 +69,14 @@ read 2097152/2097152 bytes at offset 2097152
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "NBD server not running"}}
 {"return": {}}
+
+=== Use qemu-nbd as server ===
+
+[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": false},
+{ "start": 65536, "length": 2031616, "depth": 0, "zero": false, "data": true},
+{ "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
+[{ "start": 0, "length": 512, "depth": 0, "zero": false, "data": true},
+{ "start": 512, "length": 512, "depth": 0, "zero": false, "data": false},
+{ "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true},
+{ "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
 *** done
-- 
2.20.1

Re: [Qemu-devel] [PATCH v3 0/8] Promote x-nbd-server-add-bitmap to stable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On 1/11/19 1:47 PM, Eric Blake wrote:
> Or rather, move its functionality into nbd-server-add.  And as
> a side effect, teach qemu-nbd how to export a persistent bitmap
> without having to go through a qemu process and several QMP
> commands.
> 
> Based-on: <20181221093529.23855-1-jsnow@redhat.com>
> [jsnow: 0/11 bitmaps: remove x- prefix from QMP api]
> Based-on: <20190111163519.11457-1-philmd@redhat.com>
> [philmd: qemu-nbd: Rename 'exp' variable clashing with math::exp() symbol]
> 
> Available at: https://repo.or.cz/qemu/ericb.git nbd-bitmap-add-v3
> 
> Since v2:
> - split old patch 1 into 3 parts
> - add even more tests of expected error messages
> - rebase on top of 'exp' rename
> - improve commit messages
> - add some R-b where it made sense
> 
> 001/8:[down] 'nbd: Add some error case testing to iotests 223'
> 002/8:[down] 'nbd: Forbid nbd-server-stop when server is not running'
> 003/8:[0011] [FC] 'nbd: Only require disabled bitmap for read-only exports'
> 004/8:[0020] [FC] 'nbd: Merge nbd_export_set_name into nbd_export_new'
> 005/8:[0008] [FC] 'nbd: Allow bitmap export during QMP nbd-server-add'
> 006/8:[----] [--] 'nbd: Remove x-nbd-server-add-bitmap'
> 007/8:[0009] [FC] 'nbd: Merge nbd_export_bitmap into nbd_export_new'
> 008/8:[0004] [FC] 'qemu-nbd: Add --bitmap=NAME option'

Queuing on my NBD tree; I'm planning on sending a pull request Monday
morning that includes these patches, unless a last-minute review finding
changes things.

https://repo.or.cz/qemu/ericb.git nbd

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 1/8] nbd: Add some error case testing to iotests 223

[Vladimir Sementsov-Ogievskiy]

11.01.2019 22:47, Eric Blake wrote:
> Testing success paths is important, but it's also nice to highlight
> expected failure handling, to show that we don't crash, and so that
> upcoming tests that change behavior can demonstrate the resulting
> effects on error paths.
> 
> Add the following errors:
> Attempting to export without a running server
> Attempting to start a second server
> Attempting to export a bad node name
> Attempting to export a name that is already exported
> Attempting to export an enabled bitmap
> Attempting to clean an already cleaned export

For me "clean" sounds not the same as "remove" (I used to the thought that
remove means to remove object from larger object, and clean is clean the
object internally (remove its content?), but it continues to exist and all
its relations with parents are not touched), but I'm really far from being
an expert in English.

> Attempting to quit server a second time
> 
> All of these properly complain except for a second server-stop,
> which will be fixed next.

hm, looks a bit strange to add test first and then fix bug, but on the
other hand it's more demonstrative, OK for me.

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


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


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 2/8] nbd: Forbid nbd-server-stop when server is not running

[Vladimir Sementsov-Ogievskiy]

11.01.2019 22:47, Eric Blake wrote:
> Since we already forbid other nbd-server commands when not
> in the right state, it is unlikely that any caller was relying
> on a second stop to behave as a silent no-op.  Update iotest
> 223 to show the improved behavior.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>


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

> 
> ---
> v3: new patch
> ---
>   blockdev-nbd.c             | 5 +++++
>   tests/qemu-iotests/223     | 2 +-
>   tests/qemu-iotests/223.out | 2 +-
>   3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 1d170c80b82..ca584919194 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -214,6 +214,11 @@ void qmp_nbd_server_remove(const char *name,
> 
>   void qmp_nbd_server_stop(Error **errp)
>   {
> +    if (!nbd_server) {
> +        error_setg(errp, "NBD server not running");
> +        return;
> +    }

yes, exactly like in qmp_nbd_server_remove

> +
>       nbd_export_close_all();
> 
>       nbd_server_free(nbd_server);
> diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
> index 61b46a2f066..a4016091b21 100755
> --- a/tests/qemu-iotests/223
> +++ b/tests/qemu-iotests/223
> @@ -172,7 +172,7 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
>     "arguments":{"name":"n2"}}' "error" # Attempt duplicate clean
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return"
> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return" # Oops
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "error" # Again

hm, interesting, but looks like the latest "return" or "error" is an extra thing,
we can use "}" for all qmp commands, and it worth adding _send_qmp_cmd wrapper..
But, of course, it's not about these series.

>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"
> 
>   # success, all done
> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
> index e6ede0591cd..8a4d63a4fc2 100644
> --- a/tests/qemu-iotests/223.out
> +++ b/tests/qemu-iotests/223.out
> @@ -69,6 +69,6 @@ read 2097152/2097152 bytes at offset 2097152
>   {"return": {}}
>   {"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
>   {"return": {}}
> -{"return": {}}
> +{"error": {"class": "GenericError", "desc": "NBD server not running"}}
>   {"return": {}}
>   *** done
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 3/8] nbd: Only require disabled bitmap for read-only exports

[Vladimir Sementsov-Ogievskiy]

11.01.2019 22:47, Eric Blake wrote:
> Our initial implementation of x-nbd-server-add-bitmap put
> in a restriction because of incremental backups: in that
> usage, we are exporting one qcow2 file (the temporary overlay
> target of a blockdev-backup sync:none job) and a dirty bitmap
> owned by a second qcow2 file (the source of the
> blockdev-backup, which is the backing file of the temporary).
> While both qcow2 files are still writable (the target in
> order to capture copy-on-write of old contents, and the
> source in order to track live guest writes in the meantime),
> the NBD client expects to see constant data, including the
> dirty bitmap.  An enabled bitmap in the source would be
> modified by guest writes, which is at odds with the NBD
> export being a read-only constant view, hence the initial
> code choice of enforcing a disabled bitmap (the intent is
> that the exposed bitmap was disabled in the same transaction
> that started the blockdev-backup job, although we don't want
> to track enough state to actually enforce that).
> 
> However, consider the case of a bitmap contained in a read-only
> node (including when the bitmap is found in a backing layer of
> the active image).  Because the node can't be modified, the
> bitmap won't change due to writes, regardless of whether it is
> still enabled.  Forbidding the export unless the bitmap is
> disabled is awkward, paritcularly since we can't change the
> bitmap to be disabled (because the node is read-only).
> 
> Alternatively, consider the case of live storage migration,
> where management directs the destination to create a writable
> NBD server, then performs a drive-mirror from the source to
> the mirror, prior to doing the rest of the live migration.

s/mirror/target ?

> Since storage migration can be time-consuming, it may be wise
> to let the destination include a dirty bitmap to track which
> portions it has already received, where even if the migration
> is interrupted and restarted, the source can query the
> destination block status in order to minimize re-sending
> data that has not changed in the meantime on a second attempt.

hmm, dirty bitmap dirtiness don't guarantee that data was written,
due to:

1. philosofy: we generally don't care, when something is reported
    as dirty, when actually it is clean, we assume that it is always
    safe to consider things to be dirty.

2. granularity: really, at least granularity of target bitmap should
    correspond to mirror cluster size and target block size, if one of
    these three things is out of sync, we definitely get [1]

3. errors: bitmaps are set dirty for failed writes too, so [1]

I'm not totally against, I just note that the feature you mean is
at least not as simple to implement. And for me it looks like it is more
correct to track progress of mirror on source, assuming all success
nbd write requests as successful story. At least, as all vm metadata
is on source, when storage migration not finished yet.

> Such code has not been written, but it makes sense that
> letting an active dirty bitmap be exposed and changing
> alongside writes may prove useful in the future.
> 
> Solve both issues by gating the restriction against a
> disabled bitmap to only happen when the caller has requested
> a read-only export, and where the BDS that owns the bitmap
> (whether or not it is the BDS handed to nbd_export_new() or
> from its backing chain) is still writable.
> 
> Update iotest 223 to show the looser behavior by leaving
> a bitmap enabled the whole run; note that we have to tear
> down and re-export a node when handling an error.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

anyway, I'm OK with new if-condition, and for me something like

   No real reasons to forbid export of enabled bitmap, user should understand,
   that block-status result may not correspond to reality if bitmap may be
   changed by guest-write or something in parallel (including client's self requests).
   So, it should be OK to drop the restriction at all, but let's keep a smaller
   one to prevent dangerous wrong management error..

would be enough description so,
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Or, we can allow exporting enabled bitmaps only for read-only nodes, until we
don't have real reasoning for your second case.

> 
> ---
> v3: split out unrelated test changes, improve commit message [Vladimir]
> v2: new patch
> ---
>   nbd/server.c               |  7 +++++--
>   tests/qemu-iotests/223     | 10 +++++++---
>   tests/qemu-iotests/223.out |  3 ++-
>   3 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 7af0ddffb20..98327088cb4 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2456,8 +2456,11 @@ void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
>           return;
>       }
> 
> -    if (bdrv_dirty_bitmap_enabled(bm)) {
> -        error_setg(errp, "Bitmap '%s' is enabled", bitmap);
> +    if ((exp->nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) &&
> +        bdrv_dirty_bitmap_enabled(bm)) {
> +        error_setg(errp,
> +                   "Enabled bitmap '%s' incompatible with readonly export",
> +                   bitmap);
>           return;
>       }
> 
> diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
> index a4016091b21..f200e313c06 100755
> --- a/tests/qemu-iotests/223
> +++ b/tests/qemu-iotests/223
> @@ -61,6 +61,8 @@ echo "=== Create partially sparse image, then add dirty bitmaps ==="
>   echo
> 
>   # Two bitmaps, to contrast granularity issues
> +# Also note that b will be disabled, while b2 is left enabled, to
> +# check for read-only interactions
>   _make_test_img -o cluster_size=4k 4M
>   $QEMU_IO -c 'w -P 0x11 1M 2M' "$TEST_IMG" | _filter_qemu_io
>   run_qemu <<EOF
> @@ -134,9 +136,11 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
>     "arguments":{"device":"n", "name":"n2"}}' "return"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
> -  "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Attempt enabled bitmap
> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
> -  "arguments":{"node":"n", "name":"b2"}}' "return"
> +  "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Enabled vs. read-only
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
> +  "arguments":{"name":"n2"}}' "return"
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
> +  "arguments":{"device":"n", "name":"n2", "writable":true}}' "return"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
>     "arguments":{"name":"n2", "bitmap":"b2"}}' "return"
> 
> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
> index 8a4d63a4fc2..7d147291d48 100644
> --- a/tests/qemu-iotests/223.out
> +++ b/tests/qemu-iotests/223.out
> @@ -35,7 +35,8 @@ wrote 2097152/2097152 bytes at offset 2097152
>   {"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
>   {"return": {}}
>   {"return": {}}
> -{"error": {"class": "GenericError", "desc": "Bitmap 'b2' is enabled"}}
> +{"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}}
> +{"return": {}}
>   {"return": {}}
>   {"return": {}}
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 1/8] nbd: Add some error case testing to iotests 223

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]

On 1/14/19 2:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.01.2019 22:47, Eric Blake wrote:
>> Testing success paths is important, but it's also nice to highlight
>> expected failure handling, to show that we don't crash, and so that
>> upcoming tests that change behavior can demonstrate the resulting
>> effects on error paths.
>>
>> Add the following errors:
>> Attempting to export without a running server
>> Attempting to start a second server
>> Attempting to export a bad node name
>> Attempting to export a name that is already exported
>> Attempting to export an enabled bitmap
>> Attempting to clean an already cleaned export
> 
> For me "clean" sounds not the same as "remove" (I used to the thought that
> remove means to remove object from larger object, and clean is clean the
> object internally (remove its content?), but it continues to exist and all
> its relations with parents are not touched), but I'm really far from being
> an expert in English.

Sure, I'll s/clean/remove/ in that text.

> 
>> Attempting to quit server a second time
>>
>> All of these properly complain except for a second server-stop,
>> which will be fixed next.
> 
> hm, looks a bit strange to add test first and then fix bug, but on the
> other hand it's more demonstrative, OK for me.
> 
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 3/8] nbd: Only require disabled bitmap for read-only exports

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4878 bytes --]

On 1/14/19 3:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.01.2019 22:47, Eric Blake wrote:
>> Our initial implementation of x-nbd-server-add-bitmap put
>> in a restriction because of incremental backups: in that
>> usage, we are exporting one qcow2 file (the temporary overlay
>> target of a blockdev-backup sync:none job) and a dirty bitmap
>> owned by a second qcow2 file (the source of the
>> blockdev-backup, which is the backing file of the temporary).
>> While both qcow2 files are still writable (the target in
>> order to capture copy-on-write of old contents, and the
>> source in order to track live guest writes in the meantime),
>> the NBD client expects to see constant data, including the
>> dirty bitmap.  An enabled bitmap in the source would be
>> modified by guest writes, which is at odds with the NBD
>> export being a read-only constant view, hence the initial
>> code choice of enforcing a disabled bitmap (the intent is
>> that the exposed bitmap was disabled in the same transaction
>> that started the blockdev-backup job, although we don't want
>> to track enough state to actually enforce that).
>>
>> However, consider the case of a bitmap contained in a read-only
>> node (including when the bitmap is found in a backing layer of
>> the active image).  Because the node can't be modified, the
>> bitmap won't change due to writes, regardless of whether it is
>> still enabled.  Forbidding the export unless the bitmap is
>> disabled is awkward, paritcularly since we can't change the
>> bitmap to be disabled (because the node is read-only).
>>
>> Alternatively, consider the case of live storage migration,
>> where management directs the destination to create a writable
>> NBD server, then performs a drive-mirror from the source to
>> the mirror, prior to doing the rest of the live migration.
> 
> s/mirror/target ?

Yes.

> 
>> Since storage migration can be time-consuming, it may be wise
>> to let the destination include a dirty bitmap to track which
>> portions it has already received, where even if the migration
>> is interrupted and restarted, the source can query the
>> destination block status in order to minimize re-sending
>> data that has not changed in the meantime on a second attempt.
> 
> hmm, dirty bitmap dirtiness don't guarantee that data was written,
> due to:
> 
> 1. philosofy: we generally don't care, when something is reported
>     as dirty, when actually it is clean, we assume that it is always
>     safe to consider things to be dirty.
> 
> 2. granularity: really, at least granularity of target bitmap should
>     correspond to mirror cluster size and target block size, if one of
>     these three things is out of sync, we definitely get [1]
> 
> 3. errors: bitmaps are set dirty for failed writes too, so [1]

Good points.

> 
> I'm not totally against, I just note that the feature you mean is
> at least not as simple to implement. And for me it looks like it is more
> correct to track progress of mirror on source, assuming all success
> nbd write requests as successful story. At least, as all vm metadata
> is on source, when storage migration not finished yet.

I'm fine adding a caveat that while allowing reads from a modifying
dirty bitmap may help, they may not be trivial to use correctly.

> 
>> Such code has not been written, but it makes sense that
>> letting an active dirty bitmap be exposed and changing
>> alongside writes may prove useful in the future.

to go along with my caveat that such code does not yet exist anyways :)

>>
>> Solve both issues by gating the restriction against a
>> disabled bitmap to only happen when the caller has requested
>> a read-only export, and where the BDS that owns the bitmap
>> (whether or not it is the BDS handed to nbd_export_new() or
>> from its backing chain) is still writable.
>>
>> Update iotest 223 to show the looser behavior by leaving
>> a bitmap enabled the whole run; note that we have to tear
>> down and re-export a node when handling an error.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> anyway, I'm OK with new if-condition, and for me something like
> 
>    No real reasons to forbid export of enabled bitmap, user should understand,
>    that block-status result may not correspond to reality if bitmap may be
>    changed by guest-write or something in parallel (including client's self requests).
>    So, it should be OK to drop the restriction at all, but let's keep a smaller
>    one to prevent dangerous wrong management error..
> 
> would be enough description so,
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Okay. Thanks for the review!

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]