[Qemu-devel] [PATCH v5 00/11] block: Accept node-name in all node level QMP commands

[Qemu-devel] [PATCH v5 00/11] block: Accept node-name in all node level QMP commands

[Kevin Wolf]

As stated in the RFC I sent two weeks ago:

   * Node level commands: We need to complete the conversion that makes
     commands accept node names instead of BlockBackend names. In some places
     we intentionally allow only BlockBackends because we don't know if the
     command works in other places than the root. This is okay, but we can
     accept node names anyway. We just need to check that the node is a root
     node as expected.

This part of the RFC is implemented by this series.

v5:
- Sent separate patch "block: Accept any target node for transactional
  blockdev-backup" and rebased this series on it [Max]
- Check bdrv_is_inserted() in qmp_get_root_bs() [Max]
- Resolved merge conflicts with QAPI boxed type conversion

v4:
- Changed the way to decide whether a root-node-only QMP command is allowed on
  a specific node. Instead of checking that it has a BB on top, the new check
  is that there is no non-BB parent on top. This allows using unattached nodes.
- Fixed incorrect documentation change for block-commit [Eric]

v3:
- Now with updated documentation

Kevin Wolf (11):
  block: Accept node-name for block-stream
  block: Accept node-name for block-commit
  block: Accept node-name for blockdev-backup
  block: Accept node-name for blockdev-mirror
  block: Accept node-name for blockdev-snapshot-delete-internal-sync
  block: Accept node-name for blockdev-snapshot-internal-sync
  block: Accept node-name for change-backing-file
  block: Accept node-name for drive-backup
  block: Accept node-name for drive-mirror
  nbd-server: Use a separate BlockBackend
  nbd-server: Allow node name for nbd-server-add

 block.c                        |   2 +
 block/block-backend.c          |  16 ++++
 blockdev-nbd.c                 |  21 ++---
 blockdev.c                     | 197 ++++++++++++++---------------------------
 include/block/nbd.h            |   3 +-
 include/sysemu/block-backend.h |   1 +
 nbd/server.c                   |  25 ++++--
 qapi/block-core.json           |  24 ++---
 qapi/block.json                |  14 +--
 qemu-nbd.c                     |   4 +-
 qmp-commands.hx                |  25 +++---
 tests/qemu-iotests/030         |   2 +-
 tests/qemu-iotests/041         |   8 +-
 tests/qemu-iotests/055         |   7 +-
 tests/qemu-iotests/057         |   4 +-
 15 files changed, 161 insertions(+), 192 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v5 01/11] block: Accept node-name for block-stream

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This converts
block-stream to accept a node-name without lifting the restriction that
we're operating at a root node.

In case of an invalid device name, the command returns the GenericError
error class now instead of DeviceNotFound, because this is what
qmp_get_root_bs() returns.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 16 ++++++++++++++++
 blockdev.c                     | 37 +++++++++++++++++++++++++------------
 include/sysemu/block-backend.h |  1 +
 qapi/block-core.json           |  5 +----
 qmp-commands.hx                |  2 +-
 tests/qemu-iotests/030         |  2 +-
 6 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index effa038..dc2bc60 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -410,6 +410,22 @@ bool bdrv_has_blk(BlockDriverState *bs)
 }
 
 /*
+ * Returns true if @bs has only BlockBackends as parents.
+ */
+bool bdrv_is_root_node(BlockDriverState *bs)
+{
+    BdrvChild *c;
+
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        if (c->role != &child_root) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
+/*
  * Return @blk's DriveInfo if any, else null.
  */
 DriveInfo *blk_legacy_dinfo(BlockBackend *blk)
diff --git a/blockdev.c b/blockdev.c
index 2161400..68b6741 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1174,6 +1174,28 @@ fail:
     return dinfo;
 }
 
+static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_lookup_bs(name, name, errp);
+    if (bs == NULL) {
+        return NULL;
+    }
+
+    if (!bdrv_is_root_node(bs)) {
+        error_setg(errp, "Need a root block node");
+        return NULL;
+    }
+
+    if (!bdrv_is_inserted(bs)) {
+        error_setg(errp, "Device has no medium");
+        return NULL;
+    }
+
+    return bs;
+}
+
 void hmp_commit(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
@@ -2983,7 +3005,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
     AioContext *aio_context;
@@ -2994,22 +3015,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
+    bs = qmp_get_root_bs(device, errp);
+    if (!bs) {
         return;
     }
 
-    aio_context = blk_get_aio_context(blk);
+    aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    if (!blk_is_available(blk)) {
-        error_setg(errp, "Device '%s' has no medium", device);
-        goto out;
-    }
-    bs = blk_bs(blk);
-
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
         goto out;
     }
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 2da4905..bb4fa82 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -98,6 +98,7 @@ BlockDriverState *blk_bs(BlockBackend *blk);
 void blk_remove_bs(BlockBackend *blk);
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
 bool bdrv_has_blk(BlockDriverState *bs);
+bool bdrv_is_root_node(BlockDriverState *bs);
 
 void blk_set_allow_write_beyond_eof(BlockBackend *blk, bool allow);
 void blk_iostatus_enable(BlockBackend *blk);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5e2d7d7..a2d1834 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1462,7 +1462,7 @@
 # @job-id: #optional identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)
 #
-# @device: the device name
+# @device: the device name or node-name of a root node
 #
 # @base:   #optional the common backing file name
 #
@@ -1487,9 +1487,6 @@
 #            'stop' and 'enospc' can only be used if the block device
 #            supports io-status (see BlockInfo).  Since 1.3.
 #
-# Returns: Nothing on success
-#          If @device does not exist, DeviceNotFound
-#
 # Since: 1.1
 ##
 { 'command': 'block-stream',
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c8d360a..1176056 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1120,7 +1120,7 @@ Arguments:
 
 - "job-id": Identifier for the newly-created block job. If omitted,
             the device name will be used. (json-string, optional)
-- "device": The device's ID, must be unique (json-string)
+- "device": The device name or node-name of a root node (json-string)
 - "base": The file name of the backing image above which copying starts
           (json-string, optional)
 - "backing-file": The backing file string to write into the active layer. This
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 3ac2443..107049b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -126,7 +126,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 
     def test_device_not_found(self):
         result = self.vm.qmp('block-stream', device='nonexistent')
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
 
 class TestSmallerBackingFile(iotests.QMPTestCase):
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 02/11] block: Accept node-name for block-commit

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This converts
block-commit to accept a node-name without lifting the restriction that
we're operating at a root node.

As libvirt makes use of the DeviceNotFound error class, we must add
explicit code to retain this behaviour because qmp_get_root_bs() only
returns GenericErrors.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c           | 23 +++++++++++------------
 qapi/block-core.json |  2 +-
 qmp-commands.hx      |  2 +-
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 68b6741..7619ad4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3068,7 +3068,6 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *base_bs, *top_bs;
     AioContext *aio_context;
@@ -3087,22 +3086,22 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
      *  live commit feature versions; for this to work, we must make sure to
      *  perform the device lookup before any generic errors that may occur in a
      *  scenario in which all optional arguments are omitted. */
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
+    bs = qmp_get_root_bs(device, &local_err);
+    if (!bs) {
+        bs = bdrv_lookup_bs(device, device, NULL);
+        if (!bs) {
+            error_free(local_err);
+            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                      "Device '%s' not found", device);
+        } else {
+            error_propagate(errp, local_err);
+        }
         return;
     }
 
-    aio_context = blk_get_aio_context(blk);
+    aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    if (!blk_is_available(blk)) {
-        error_setg(errp, "Device '%s' has no medium", device);
-        goto out;
-    }
-    bs = blk_bs(blk);
-
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, errp)) {
         goto out;
     }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a2d1834..04076c8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1020,7 +1020,7 @@
 # @job-id: #optional identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)
 #
-# @device:  the name of the device
+# @device:  the device name or node-name of a root node
 #
 # @base:   #optional The file name of the backing image to write data into.
 #                    If not specified, this is the deepest backing image
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1176056..55425e9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1166,7 +1166,7 @@ Arguments:
 
 - "job-id": Identifier for the newly-created block job. If omitted,
             the device name will be used. (json-string, optional)
-- "device": The device's ID, must be unique (json-string)
+- "device": The device name or node-name of a root node (json-string)
 - "base": The file name of the backing image to write data into.
           If not specified, this is the deepest backing image
           (json-string, optional)
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 03/11] block: Accept node-name for blockdev-backup

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This converts
blockdev-backup and the corresponding transaction action to accept a
node-name without lifting the restriction that we're operating at a root
node.

In case of an invalid device name, the command returns the GenericError
error class now instead of DeviceNotFound, because this is what
qmp_get_root_bs() returns.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c           | 31 ++++++++-----------------------
 qapi/block-core.json |  2 +-
 qmp-commands.hx      |  2 +-
 3 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 7619ad4..46beafd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1959,21 +1959,14 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
     BlockdevBackup *backup;
-    BlockBackend *blk;
-    BlockDriverState *target;
+    BlockDriverState *bs, *target;
     Error *local_err = NULL;
 
     assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
     backup = common->action->u.blockdev_backup.data;
 
-    blk = blk_by_name(backup->device);
-    if (!blk) {
-        error_setg(errp, "Device '%s' not found", backup->device);
-        return;
-    }
-
-    if (!blk_is_available(blk)) {
-        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, backup->device);
+    bs = qmp_get_root_bs(backup->device, errp);
+    if (!bs) {
         return;
     }
 
@@ -1983,14 +1976,14 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
     }
 
     /* AioContext is released in .clean() */
-    state->aio_context = blk_get_aio_context(blk);
+    state->aio_context = bdrv_get_aio_context(bs);
     if (state->aio_context != bdrv_get_aio_context(target)) {
         state->aio_context = NULL;
         error_setg(errp, "Backup between two IO threads is not implemented");
         return;
     }
     aio_context_acquire(state->aio_context);
-    state->bs = blk_bs(blk);
+    state->bs = bs;
     bdrv_drained_begin(state->bs);
 
     do_blockdev_backup(backup->has_job_id ? backup->job_id : NULL,
@@ -3335,7 +3328,6 @@ void do_blockdev_backup(const char *job_id, const char *device,
                          BlockdevOnError on_target_error,
                          BlockJobTxn *txn, Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     Error *local_err = NULL;
@@ -3351,21 +3343,14 @@ void do_blockdev_backup(const char *job_id, const char *device,
         on_target_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_setg(errp, "Device '%s' not found", device);
+    bs = qmp_get_root_bs(device, errp);
+    if (!bs) {
         return;
     }
 
-    aio_context = blk_get_aio_context(blk);
+    aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    if (!blk_is_available(blk)) {
-        error_setg(errp, "Device '%s' has no medium", device);
-        goto out;
-    }
-    bs = blk_bs(blk);
-
     target_bs = bdrv_lookup_bs(target, target, errp);
     if (!target_bs) {
         goto out;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 04076c8..53f46b6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -925,7 +925,7 @@
 # @job-id: #optional identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)
 #
-# @device: the name of the device which should be copied.
+# @device: the device name or node-name of a root node which should be copied.
 #
 # @target: the device name or node-name of the backup target node.
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 55425e9..d3ba48d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1288,7 +1288,7 @@ Arguments:
 
 - "job-id": Identifier for the newly-created block job. If omitted,
             the device name will be used. (json-string, optional)
-- "device": the name of the device which should be copied.
+- "device": the device name or node-name of a root node which should be copied.
             (json-string)
 - "target": the name of the backup target device. (json-string)
 - "sync": what parts of the disk image should be copied to the destination;
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 04/11] block: Accept node-name for blockdev-mirror

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This converts
blockdev-mirror to accept a node-name without lifting the restriction
that we're operating at a root node.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 10 +---------
 qapi/block-core.json |  3 ++-
 qmp-commands.hx      |  3 ++-
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 46beafd..ccff1f7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3627,21 +3627,13 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
                          Error **errp)
 {
     BlockDriverState *bs;
-    BlockBackend *blk;
     BlockDriverState *target_bs;
     AioContext *aio_context;
     BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN;
     Error *local_err = NULL;
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_setg(errp, "Device '%s' not found", device);
-        return;
-    }
-    bs = blk_bs(blk);
-
+    bs = qmp_get_root_bs(device, errp);
     if (!bs) {
-        error_setg(errp, "Device '%s' has no media", device);
         return;
     }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 53f46b6..bcb37db 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1277,7 +1277,8 @@
 # @job-id: #optional identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)
 #
-# @device: the name of the device whose writes should be mirrored.
+# @device: The device name or node-name of a root node whose writes should be
+#          mirrored.
 #
 # @target: the id or node-name of the block device to mirror to. This mustn't be
 #          attached to guest.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d3ba48d..6af1e58 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1747,7 +1747,8 @@ Arguments:
 
 - "job-id": Identifier for the newly-created block job. If omitted,
             the device name will be used. (json-string, optional)
-- "device": device name to operate on (json-string)
+- "device": The device name or node-name of a root node whose writes should be
+            mirrored (json-string)
 - "target": device name to mirror to (json-string)
 - "replaces": the block driver node name to replace when finished
               (json-string, optional)
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 05/11] block: Accept node-name for blockdev-snapshot-delete-internal-sync

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This converts
blockdev-snapshot-delete-internal-sync to accept a node-name without
lifting the restriction that we're operating at a root node.

In case of an invalid device name, the command returns the GenericError
error class now instead of DeviceNotFound, because this is what
qmp_get_root_bs() returns.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c             | 16 +++-------------
 qapi/block.json        |  5 +++--
 qmp-commands.hx        |  2 +-
 tests/qemu-iotests/057 |  2 +-
 4 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index ccff1f7..9feffcd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1306,21 +1306,17 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
                                                          Error **errp)
 {
     BlockDriverState *bs;
-    BlockBackend *blk;
     AioContext *aio_context;
     QEMUSnapshotInfo sn;
     Error *local_err = NULL;
     SnapshotInfo *info = NULL;
     int ret;
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
+    bs = qmp_get_root_bs(device, errp);
+    if (!bs) {
         return NULL;
     }
-
-    aio_context = blk_get_aio_context(blk);
+    aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
     if (!has_id) {
@@ -1336,12 +1332,6 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
         goto out_aio_context;
     }
 
-    if (!blk_is_available(blk)) {
-        error_setg(errp, "Device '%s' has no medium", device);
-        goto out_aio_context;
-    }
-    bs = blk_bs(blk);
-
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, errp)) {
         goto out_aio_context;
     }
diff --git a/qapi/block.json b/qapi/block.json
index 937337d..c0e831f 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -99,14 +99,15 @@
 # both. One of the name or id is required. Return SnapshotInfo for the
 # successfully deleted snapshot.
 #
-# @device: the name of the device to delete the snapshot from
+# @device: the device name or node-name of a root node to delete the snapshot
+#          from
 #
 # @id: optional the snapshot's ID to be deleted
 #
 # @name: optional the snapshot's name to be deleted
 #
 # Returns: SnapshotInfo on success
-#          If @device is not a valid block device, DeviceNotFound
+#          If @device is not a valid block device, GenericError
 #          If snapshot not found, GenericError
 #          If the format of the image used does not support it,
 #          BlockFormatFeatureNotSupported
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6af1e58..33e75c2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1639,7 +1639,7 @@ fail.
 
 Arguments:
 
-- "device": device name (json-string)
+- "device": the device name or node-name of a root node (json-string)
 - "id": ID of the snapshot (json-string, optional)
 - "name": name of the snapshot (json-string, optional)
 
diff --git a/tests/qemu-iotests/057 b/tests/qemu-iotests/057
index 9cdd582..bb6c06a 100755
--- a/tests/qemu-iotests/057
+++ b/tests/qemu-iotests/057
@@ -239,7 +239,7 @@ class TestSnapshotDelete(ImageSnapshotTestCase):
         result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
                               device = 'drive_error',
                               id = '0')
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_error_no_id_and_name(self):
         result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 06/11] block: Accept node-name for blockdev-snapshot-internal-sync

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This converts
blockdev-snapshot-internal-sync to accept a node-name without lifting
the restriction that we're operating at a root node.

In case of an invalid device name, the command returns the GenericError
error class now instead of DeviceNotFound, because this is what
qmp_get_root_bs() returns.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c             | 15 +++------------
 qapi/block.json        |  5 +++--
 qmp-commands.hx        |  6 ++++--
 tests/qemu-iotests/057 |  2 +-
 4 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 9feffcd..f775bbd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1511,7 +1511,6 @@ static void internal_snapshot_prepare(BlkActionState *common,
     Error *local_err = NULL;
     const char *device;
     const char *name;
-    BlockBackend *blk;
     BlockDriverState *bs;
     QEMUSnapshotInfo old_sn, *sn;
     bool ret;
@@ -1534,23 +1533,15 @@ static void internal_snapshot_prepare(BlkActionState *common,
         return;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
+    bs = qmp_get_root_bs(device, errp);
+    if (!bs) {
         return;
     }
 
     /* AioContext is released in .clean() */
-    state->aio_context = blk_get_aio_context(blk);
+    state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
 
-    if (!blk_is_available(blk)) {
-        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-        return;
-    }
-    bs = blk_bs(blk);
-
     state->bs = bs;
     bdrv_drained_begin(bs);
 
diff --git a/qapi/block.json b/qapi/block.json
index c0e831f..db05169 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -58,7 +58,8 @@
 ##
 # @BlockdevSnapshotInternal
 #
-# @device: the name of the device to generate the snapshot from
+# @device: the device name or node-name of a root node to generate the snapshot
+#          from
 #
 # @name: the name of the internal snapshot to be created
 #
@@ -80,7 +81,7 @@
 # For the arguments, see the documentation of BlockdevSnapshotInternal.
 #
 # Returns: nothing on success
-#          If @device is not a valid block device, DeviceNotFound
+#          If @device is not a valid block device, GenericError
 #          If any snapshot matching @name exists, or @name is empty,
 #          GenericError
 #          If the format of the image used does not support it,
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 33e75c2..8a53539 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1407,7 +1407,8 @@ actions array:
       - "mode": whether and how QEMU should create the snapshot file
         (NewImageMode, optional, default "absolute-paths")
       When "type" is "blockdev-snapshot-internal-sync":
-      - "device": device name to snapshot (json-string)
+      - "device": the device name or node-name of a root node to snapshot
+                  (json-string)
       - "name": name of the new snapshot (json-string)
 
 Example:
@@ -1608,7 +1609,8 @@ name already exists, the operation will fail.
 
 Arguments:
 
-- "device": device name to snapshot (json-string)
+- "device": the device name or node-name of a root node to snapshot
+            (json-string)
 - "name": name of the new snapshot (json-string)
 
 Example:
diff --git a/tests/qemu-iotests/057 b/tests/qemu-iotests/057
index bb6c06a..9f0a5a3 100755
--- a/tests/qemu-iotests/057
+++ b/tests/qemu-iotests/057
@@ -182,7 +182,7 @@ class TestSingleTransaction(ImageSnapshotTestCase):
                               'name': 'a' },
                   }]
         result = self.vm.qmp('transaction', actions = actions)
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_error_exist(self):
         self.createSnapshotInTransaction(1)
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 07/11] block: Accept node-name for change-backing-file

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This converts
change-backing-file to accept a node-name without lifting the
restriction that we're operating at a root node.

In case of an invalid device name, the command returns the GenericError
error class now instead of DeviceNotFound, because this is what
qmp_get_root_bs() returns.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 15 +++------------
 qapi/block-core.json |  3 ++-
 qmp-commands.hx      |  3 ++-
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index f775bbd..a42c802 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3755,7 +3755,6 @@ void qmp_change_backing_file(const char *device,
                              const char *backing_file,
                              Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs = NULL;
     AioContext *aio_context;
     BlockDriverState *image_bs = NULL;
@@ -3764,22 +3763,14 @@ void qmp_change_backing_file(const char *device,
     int open_flags;
     int ret;
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
+    bs = qmp_get_root_bs(device, errp);
+    if (!bs) {
         return;
     }
 
-    aio_context = blk_get_aio_context(blk);
+    aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    if (!blk_is_available(blk)) {
-        error_setg(errp, "Device '%s' has no medium", device);
-        goto out;
-    }
-    bs = blk_bs(blk);
-
     image_bs = bdrv_lookup_bs(NULL, image_node_name, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bcb37db..d25ba46 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -998,7 +998,8 @@
 # @image-node-name: The name of the block driver state node of the
 #                   image to modify.
 #
-# @device:          The name of the device that owns image-node-name.
+# @device:          The device name or node-name of the root node that owns
+#                   image-node-name.
 #
 # @backing-file:    The string to write as the backing file.  This
 #                   string is not validated, so care should be taken
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8a53539..bd54b34 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1806,7 +1806,8 @@ Arguments:
                         "device".
                         (json-string, optional)
 
-- "device":             The name of the device.
+- "device":             The device name or node-name of the root node that owns
+                        image-node-name.
                         (json-string)
 
 - "backing-file":       The string to write as the backing file.  This string is
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 08/11] block: Accept node-name for drive-backup

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This converts
drive-backup and the corresponding transaction action to accept a
node-name without lifting the restriction that we're operating at a root
node.

In case of an invalid device name, the command returns the GenericError
error class now instead of DeviceNotFound, because this is what
qmp_get_root_bs() returns.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c             | 36 +++++++++---------------------------
 qapi/block-core.json   |  4 ++--
 qmp-commands.hx        |  2 +-
 tests/qemu-iotests/055 |  7 ++-----
 4 files changed, 14 insertions(+), 35 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index a42c802..773336c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1856,30 +1856,23 @@ static void do_drive_backup(const char *job_id, const char *device,
 static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
-    BlockBackend *blk;
+    BlockDriverState *bs;
     DriveBackup *backup;
     Error *local_err = NULL;
 
     assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
     backup = common->action->u.drive_backup.data;
 
-    blk = blk_by_name(backup->device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", backup->device);
-        return;
-    }
-
-    if (!blk_is_available(blk)) {
-        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, backup->device);
+    bs = qmp_get_root_bs(backup->device, errp);
+    if (!bs) {
         return;
     }
 
     /* AioContext is released in .clean() */
-    state->aio_context = blk_get_aio_context(blk);
+    state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
-    bdrv_drained_begin(blk_bs(blk));
-    state->bs = blk_bs(blk);
+    bdrv_drained_begin(bs);
+    state->bs = bs;
 
     do_drive_backup(backup->has_job_id ? backup->job_id : NULL,
                     backup->device, backup->target,
@@ -3153,7 +3146,6 @@ static void do_drive_backup(const char *job_id, const char *device,
                             BlockdevOnError on_target_error,
                             BlockJobTxn *txn, Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     BlockDriverState *source = NULL;
@@ -3177,24 +3169,14 @@ static void do_drive_backup(const char *job_id, const char *device,
         mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
+    bs = qmp_get_root_bs(device, errp);
+    if (!bs) {
         return;
     }
 
-    aio_context = blk_get_aio_context(blk);
+    aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    /* Although backup_run has this check too, we need to use bs->drv below, so
-     * do an early check redundantly. */
-    if (!blk_is_available(blk)) {
-        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-        goto out;
-    }
-    bs = blk_bs(blk);
-
     if (!has_format) {
         format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
     }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d25ba46..f081eb8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -876,7 +876,7 @@
 # @job-id: #optional identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)
 #
-# @device: the name of the device which should be copied.
+# @device: the device name or node-name of a root node which should be copied.
 #
 # @target: the target of the new image. If the file exists, or if it
 #          is a device, the existing file/device will be used as the new
@@ -1087,7 +1087,7 @@
 # For the arguments, see the documentation of DriveBackup.
 #
 # Returns: nothing on success
-#          If @device is not a valid block device, DeviceNotFound
+#          If @device is not a valid block device, GenericError
 #
 # Since 1.6
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index bd54b34..6b89b36 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1235,7 +1235,7 @@ Arguments:
 
 - "job-id": Identifier for the newly-created block job. If omitted,
             the device name will be used. (json-string, optional)
-- "device": the name of the device which should be copied.
+- "device": the device name or node-name of a root node which should be copied.
             (json-string)
 - "target": the target of the new image. If the file exists, or if it is a
             device, the existing file/device will be used as the new
diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index c8e3578..8113c61 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -134,10 +134,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 
     def do_test_device_not_found(self, cmd, **args):
         result = self.vm.qmp(cmd, **args)
-        if cmd == 'drive-backup':
-            self.assert_qmp(result, 'error/class', 'DeviceNotFound')
-        else:
-            self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_device_not_found(self):
         self.do_test_device_not_found('drive-backup', device='nonexistent',
@@ -371,7 +368,7 @@ class TestSingleTransaction(iotests.QMPTestCase):
                           'sync': 'full' },
             }
         ])
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
         result = self.vm.qmp('transaction', actions=[{
                 'type': 'blockdev-backup',
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 09/11] block: Accept node-name for drive-mirror

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This converts
drive-mirror to accept a node-name without lifting the restriction that
we're operating at a root node.

In case of an invalid device name, the command returns the GenericError
error class now instead of DeviceNotFound, because this is what
qmp_get_root_bs() returns.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c             | 14 +++-----------
 qapi/block-core.json   |  5 +++--
 qmp-commands.hx        |  3 ++-
 tests/qemu-iotests/041 |  8 +++-----
 4 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 773336c..a392e08 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3429,7 +3429,6 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
 void qmp_drive_mirror(DriveMirror *arg, Error **errp)
 {
     BlockDriverState *bs;
-    BlockBackend *blk;
     BlockDriverState *source, *target_bs;
     AioContext *aio_context;
     BlockMirrorBackingMode backing_mode;
@@ -3439,21 +3438,14 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
     int64_t size;
     const char *format = arg->format;
 
-    blk = blk_by_name(arg->device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", arg->device);
+    bs = qmp_get_root_bs(arg->device, errp);
+    if (!bs) {
         return;
     }
 
-    aio_context = blk_get_aio_context(blk);
+    aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    if (!blk_is_available(blk)) {
-        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, arg->device);
-        goto out;
-    }
-    bs = blk_bs(blk);
     if (!arg->has_mode) {
         arg->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f081eb8..fba1718 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1128,7 +1128,7 @@
 # See DriveMirror for parameter descriptions
 #
 # Returns: nothing on success
-#          If @device is not a valid block device, DeviceNotFound
+#          If @device is not a valid block device, GenericError
 #
 # Since 1.3
 ##
@@ -1143,7 +1143,8 @@
 # @job-id: #optional identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)
 #
-# @device:  the name of the device whose writes should be mirrored.
+# @device:  the device name or node-name of a root node whose writes should be
+#           mirrored.
 #
 # @target: the target of the new image. If the file exists, or if it
 #          is a device, the existing file/device will be used as the new
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6b89b36..22bdb41 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1689,7 +1689,8 @@ Arguments:
 
 - "job-id": Identifier for the newly-created block job. If omitted,
             the device name will be used. (json-string, optional)
-- "device": device name to operate on (json-string)
+- "device": the device name or node-name of a root node whose writes should be
+            mirrored.(json-string)
 - "target": name of new image file (json-string)
 - "format": format of new image (json-string, optional)
 - "node-name": the name of the new block driver state in the node graph
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index cbf5e0b..80939c0 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -38,7 +38,6 @@ class TestSingleDrive(iotests.QMPTestCase):
     image_len = 1 * 1024 * 1024 # MB
     qmp_cmd = 'drive-mirror'
     qmp_target = target_img
-    not_found_error = 'DeviceNotFound'
 
     def setUp(self):
         iotests.create_image(backing_img, self.image_len)
@@ -176,7 +175,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 
         result = self.vm.qmp(self.qmp_cmd, device='ide1-cd0', sync='full',
                              target=self.qmp_target)
-        self.assert_qmp(result, 'error/class', self.not_found_error)
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_image_not_found(self):
         result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
@@ -186,12 +185,11 @@ class TestSingleDrive(iotests.QMPTestCase):
     def test_device_not_found(self):
         result = self.vm.qmp(self.qmp_cmd, device='nonexistent', sync='full',
                              target=self.qmp_target)
-        self.assert_qmp(result, 'error/class', self.not_found_error)
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
 class TestSingleBlockdev(TestSingleDrive):
     qmp_cmd = 'blockdev-mirror'
     qmp_target = 'node1'
-    not_found_error = 'GenericError'
 
     def setUp(self):
         TestSingleDrive.setUp(self)
@@ -922,7 +920,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
                              node_name='repair0',
                              replaces='img1',
                              target=quorum_repair_img, format=iotests.imgfmt)
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_wrong_sync_mode(self):
         if not self.has_quorum():
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 10/11] nbd-server: Use a separate BlockBackend

[Kevin Wolf]

The builtin NBD server uses its own BlockBackend now instead of reusing
the monitor/guest device one.

This means that it has its own writethrough setting now. The builtin
NBD server always uses writeback caching now regardless of whether the
guest device has WCE enabled. qemu-nbd respects the cache mode given on
the command line.

We still need to keep a reference to the monitor BB because we put an
eject notifier on it, but we don't use it for any I/O.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c             |  2 ++
 blockdev-nbd.c      |  4 ++--
 include/block/nbd.h |  3 ++-
 nbd/server.c        | 25 ++++++++++++++++++++-----
 qemu-nbd.c          |  4 ++--
 5 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 30d64e6..101f8c6 100644
--- a/block.c
+++ b/block.c
@@ -25,6 +25,7 @@
 #include "trace.h"
 #include "block/block_int.h"
 #include "block/blockjob.h"
+#include "block/nbd.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "qapi/qmp/qerror.h"
@@ -2206,6 +2207,7 @@ static void bdrv_close(BlockDriverState *bs)
 void bdrv_close_all(void)
 {
     block_job_cancel_sync_all();
+    nbd_export_close_all();
 
     /* Drop references from requests still in flight, such as canceled block
      * jobs whose AIO context has not been polled yet */
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 12cae0e..c437d32 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -176,8 +176,8 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         writable = false;
     }
 
-    exp = nbd_export_new(blk, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL,
-                         errp);
+    exp = nbd_export_new(blk_bs(blk), 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
+                         NULL, false, blk, errp);
     if (!exp) {
         return;
     }
diff --git a/include/block/nbd.h b/include/block/nbd.h
index cb91820..ebdba0d 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -103,8 +103,9 @@ int nbd_disconnect(int fd);
 typedef struct NBDExport NBDExport;
 typedef struct NBDClient NBDClient;
 
-NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
+NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
                           uint32_t nbdflags, void (*close)(NBDExport *),
+                          bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp);
 void nbd_export_close(NBDExport *exp);
 void nbd_export_get(NBDExport *exp);
diff --git a/nbd/server.c b/nbd/server.c
index 29e2099..207625e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -69,6 +69,7 @@ struct NBDExport {
 
     AioContext *ctx;
 
+    BlockBackend *eject_notifier_blk;
     Notifier eject_notifier;
 };
 
@@ -809,11 +810,18 @@ static void nbd_eject_notifier(Notifier *n, void *data)
     nbd_export_close(exp);
 }
 
-NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
+NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
                           uint32_t nbdflags, void (*close)(NBDExport *),
+                          bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp)
 {
+    BlockBackend *blk;
     NBDExport *exp = g_malloc0(sizeof(NBDExport));
+
+    blk = blk_new();
+    blk_insert_bs(blk, bs);
+    blk_set_enable_write_cache(blk, !writethrough);
+
     exp->refcount = 1;
     QTAILQ_INIT(&exp->clients);
     exp->blk = blk;
@@ -829,11 +837,14 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
 
     exp->close = close;
     exp->ctx = blk_get_aio_context(blk);
-    blk_ref(blk);
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
 
-    exp->eject_notifier.notify = nbd_eject_notifier;
-    blk_add_remove_bs_notifier(blk, &exp->eject_notifier);
+    if (on_eject_blk) {
+        blk_ref(on_eject_blk);
+        exp->eject_notifier_blk = on_eject_blk;
+        exp->eject_notifier.notify = nbd_eject_notifier;
+        blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
+    }
 
     /*
      * NBD exports are used for non-shared storage migration.  Make sure
@@ -846,6 +857,7 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
     return exp;
 
 fail:
+    blk_unref(blk);
     g_free(exp);
     return NULL;
 }
@@ -916,7 +928,10 @@ void nbd_export_put(NBDExport *exp)
         }
 
         if (exp->blk) {
-            notifier_remove(&exp->eject_notifier);
+            if (exp->eject_notifier_blk) {
+                notifier_remove(&exp->eject_notifier);
+                blk_unref(exp->eject_notifier_blk);
+            }
             blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
                                             blk_aio_detach, exp);
             blk_unref(exp->blk);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 321f02b..9e6aaa9 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -910,8 +910,8 @@ int main(int argc, char **argv)
         }
     }
 
-    exp = nbd_export_new(blk, dev_offset, fd_size, nbdflags, nbd_export_closed,
-                         &local_err);
+    exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, nbd_export_closed,
+                         writethrough, NULL, &local_err);
     if (!exp) {
         error_report_err(local_err);
         exit(EXIT_FAILURE);
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 11/11] nbd-server: Allow node name for nbd-server-add

[Kevin Wolf]

There is no reason why an NBD server couldn't be started for any node,
even if it's not on the top level. This converts nbd-server-add to
accept a node-name.

Note that there is a semantic difference between using a BlockBackend
name and the node name of its root: In the former case, the NBD server
is closed on eject; in the latter case, the NBD server doesn't drop its
reference and keeps the image file open this way.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev-nbd.c  | 21 +++++++++------------
 qapi/block.json |  4 ++--
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index c437d32..ca41cc6 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -145,7 +145,8 @@ void qmp_nbd_server_start(SocketAddress *addr,
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
                         Error **errp)
 {
-    BlockBackend *blk;
+    BlockDriverState *bs = NULL;
+    BlockBackend *on_eject_blk;
     NBDExport *exp;
 
     if (!nbd_server) {
@@ -158,26 +159,22 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         return;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
-        return;
-    }
-    if (!blk_is_inserted(blk)) {
-        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+    on_eject_blk = blk_by_name(device);
+
+    bs = bdrv_lookup_bs(device, device, errp);
+    if (!bs) {
         return;
     }
 
     if (!has_writable) {
         writable = false;
     }
-    if (blk_is_read_only(blk)) {
+    if (bdrv_is_read_only(bs)) {
         writable = false;
     }
 
-    exp = nbd_export_new(blk_bs(blk), 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
-                         NULL, false, blk, errp);
+    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
+                         NULL, false, on_eject_blk, errp);
     if (!exp) {
         return;
     }
diff --git a/qapi/block.json b/qapi/block.json
index db05169..8b08bd2 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -161,9 +161,9 @@
 ##
 # @nbd-server-add:
 #
-# Export a device to QEMU's embedded NBD server.
+# Export a block node to QEMU's embedded NBD server.
 #
-# @device: Block device to be exported
+# @device: The device name or node name of the node to be exported
 #
 # @writable: Whether clients should be able to write to the device via the
 #     NBD connection (default false). #optional
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v5 01/11] block: Accept node-name for block-stream

[Alberto Garcia]

On Wed 03 Aug 2016 01:21:24 PM CEST, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> block-stream to accept a node-name without lifting the restriction that
> we're operating at a root node.
>
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v5 02/11] block: Accept node-name for block-commit

[Alberto Garcia]

On Wed 03 Aug 2016 01:21:25 PM CEST, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> block-commit to accept a node-name without lifting the restriction that
> we're operating at a root node.
>
> As libvirt makes use of the DeviceNotFound error class, we must add
> explicit code to retain this behaviour because qmp_get_root_bs() only
> returns GenericErrors.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v5 01/11] block: Accept node-name for block-stream

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> block-stream to accept a node-name without lifting the restriction that
> we're operating at a root node.
> 
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 16 ++++++++++++++++
>  blockdev.c                     | 37 +++++++++++++++++++++++++------------
>  include/sysemu/block-backend.h |  1 +
>  qapi/block-core.json           |  5 +----
>  qmp-commands.hx                |  2 +-
>  tests/qemu-iotests/030         |  2 +-
>  6 files changed, 45 insertions(+), 18 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 02/11] block: Accept node-name for block-commit

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> block-commit to accept a node-name without lifting the restriction that
> we're operating at a root node.
> 
> As libvirt makes use of the DeviceNotFound error class, we must add
> explicit code to retain this behaviour because qmp_get_root_bs() only
> returns GenericErrors.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  blockdev.c           | 23 +++++++++++------------
>  qapi/block-core.json |  2 +-
>  qmp-commands.hx      |  2 +-
>  3 files changed, 13 insertions(+), 14 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 03/11] block: Accept node-name for blockdev-backup

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> blockdev-backup and the corresponding transaction action to accept a
> node-name without lifting the restriction that we're operating at a root
> node.
> 
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  blockdev.c           | 31 ++++++++-----------------------
>  qapi/block-core.json |  2 +-
>  qmp-commands.hx      |  2 +-
>  3 files changed, 10 insertions(+), 25 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 04/11] block: Accept node-name for blockdev-mirror

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> blockdev-mirror to accept a node-name without lifting the restriction
> that we're operating at a root node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 10 +---------
>  qapi/block-core.json |  3 ++-
>  qmp-commands.hx      |  3 ++-
>  3 files changed, 5 insertions(+), 11 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 05/11] block: Accept node-name for blockdev-snapshot-delete-internal-sync

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> blockdev-snapshot-delete-internal-sync to accept a node-name without
> lifting the restriction that we're operating at a root node.
> 
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c             | 16 +++-------------
>  qapi/block.json        |  5 +++--
>  qmp-commands.hx        |  2 +-
>  tests/qemu-iotests/057 |  2 +-
>  4 files changed, 8 insertions(+), 17 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 06/11] block: Accept node-name for blockdev-snapshot-internal-sync

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> blockdev-snapshot-internal-sync to accept a node-name without lifting
> the restriction that we're operating at a root node.
> 
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c             | 15 +++------------
>  qapi/block.json        |  5 +++--
>  qmp-commands.hx        |  6 ++++--
>  tests/qemu-iotests/057 |  2 +-
>  4 files changed, 11 insertions(+), 17 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 07/11] block: Accept node-name for change-backing-file

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> change-backing-file to accept a node-name without lifting the
> restriction that we're operating at a root node.
> 
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 15 +++------------
>  qapi/block-core.json |  3 ++-
>  qmp-commands.hx      |  3 ++-
>  3 files changed, 7 insertions(+), 14 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 08/11] block: Accept node-name for drive-backup

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> drive-backup and the corresponding transaction action to accept a
> node-name without lifting the restriction that we're operating at a root
> node.
> 
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c             | 36 +++++++++---------------------------
>  qapi/block-core.json   |  4 ++--
>  qmp-commands.hx        |  2 +-
>  tests/qemu-iotests/055 |  7 ++-----
>  4 files changed, 14 insertions(+), 35 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 09/11] block: Accept node-name for drive-mirror

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> drive-mirror to accept a node-name without lifting the restriction that
> we're operating at a root node.
> 
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  blockdev.c             | 14 +++-----------
>  qapi/block-core.json   |  5 +++--
>  qmp-commands.hx        |  3 ++-
>  tests/qemu-iotests/041 |  8 +++-----
>  4 files changed, 11 insertions(+), 19 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 09/11] block: Accept node-name for drive-mirror

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> drive-mirror to accept a node-name without lifting the restriction that
> we're operating at a root node.
> 
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  blockdev.c             | 14 +++-----------
>  qapi/block-core.json   |  5 +++--
>  qmp-commands.hx        |  3 ++-
>  tests/qemu-iotests/041 |  8 +++-----
>  4 files changed, 11 insertions(+), 19 deletions(-)

[...]

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6b89b36..22bdb41 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1689,7 +1689,8 @@ Arguments:
>  
>  - "job-id": Identifier for the newly-created block job. If omitted,
>              the device name will be used. (json-string, optional)
> -- "device": device name to operate on (json-string)
> +- "device": the device name or node-name of a root node whose writes should be
> +            mirrored.(json-string)

Critical issue spotted in the last minute: Space missing after the period!

(Or there should probably not be a period at all, considering that the
"sentence" is not really one and that it starts with a lower letter.)

Max

>  - "target": name of new image file (json-string)
>  - "format": format of new image (json-string, optional)
>  - "node-name": the name of the new block driver state in the node graph


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

Re: [Qemu-devel] [PATCH v5 10/11] nbd-server: Use a separate BlockBackend

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> The builtin NBD server uses its own BlockBackend now instead of reusing
> the monitor/guest device one.
> 
> This means that it has its own writethrough setting now. The builtin
> NBD server always uses writeback caching now regardless of whether the
> guest device has WCE enabled. qemu-nbd respects the cache mode given on
> the command line.
> 
> We still need to keep a reference to the monitor BB because we put an
> eject notifier on it, but we don't use it for any I/O.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c             |  2 ++
>  blockdev-nbd.c      |  4 ++--
>  include/block/nbd.h |  3 ++-
>  nbd/server.c        | 25 ++++++++++++++++++++-----
>  qemu-nbd.c          |  4 ++--
>  5 files changed, 28 insertions(+), 10 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 11/11] nbd-server: Allow node name for nbd-server-add

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> There is no reason why an NBD server couldn't be started for any node,
> even if it's not on the top level. This converts nbd-server-add to
> accept a node-name.
> 
> Note that there is a semantic difference between using a BlockBackend
> name and the node name of its root: In the former case, the NBD server
> is closed on eject; in the latter case, the NBD server doesn't drop its
> reference and keeps the image file open this way.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev-nbd.c  | 21 +++++++++------------
>  qapi/block.json |  4 ++--
>  2 files changed, 11 insertions(+), 14 deletions(-)

Well, while I concede that it's possible to create R/W NBD servers on
basically any node even today, I share Eric's concern that we shouldn't
make it so simple.

On the other hand, given COLO, I can't think of any simple solution to
this. As far as I'm aware, COLO will make use of R/W NBD servers on
backing nodes, so we can't just disallow creation of R/W servers on
non-root nodes.

So I guess your take on this is that it was already possible to create
NBD servers wherever you so pleased and that we should just put this off
until we have better op blockers?

Max


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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 11/11] nbd-server: Allow node name for nbd-server-add

[Max Reitz]

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

On 03.08.2016 13:21, Kevin Wolf wrote:
> There is no reason why an NBD server couldn't be started for any node,
> even if it's not on the top level. This converts nbd-server-add to
> accept a node-name.
> 
> Note that there is a semantic difference between using a BlockBackend
> name and the node name of its root: In the former case, the NBD server
> is closed on eject; in the latter case, the NBD server doesn't drop its
> reference and keeps the image file open this way.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev-nbd.c  | 21 +++++++++------------
>  qapi/block.json |  4 ++--
>  2 files changed, 11 insertions(+), 14 deletions(-)

As per the terms of surrender as stated on IRC:

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 00/11] block: Accept node-name in all node level QMP commands

[Kevin Wolf]

Am 03.08.2016 um 13:21 hat Kevin Wolf geschrieben:
> As stated in the RFC I sent two weeks ago:
> 
>    * Node level commands: We need to complete the conversion that makes
>      commands accept node names instead of BlockBackend names. In some places
>      we intentionally allow only BlockBackends because we don't know if the
>      command works in other places than the root. This is okay, but we can
>      accept node names anyway. We just need to check that the node is a root
>      node as expected.
> 
> This part of the RFC is implemented by this series.

Applied to block-next.

Kevin