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

[Qemu-devel] [PATCH v3 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.

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 +
 blockdev-nbd.c         |  21 +++---
 blockdev.c             | 196 ++++++++++++++++---------------------------------
 include/block/nbd.h    |   3 +-
 nbd/server.c           |  25 +++++--
 qapi/block-core.json   |  27 ++++---
 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 +-
 13 files changed, 142 insertions(+), 196 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 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>
---
 blockdev.c             | 32 ++++++++++++++++++++------------
 qapi/block-core.json   |  5 +----
 qmp-commands.hx        |  2 +-
 tests/qemu-iotests/030 |  2 +-
 4 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0f8065c..01e57c9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1172,6 +1172,23 @@ 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_has_blk(bs)) {
+        error_setg(errp, "Need a root block node");
+        return NULL;
+    }
+
+    return bs;
+}
+
 void hmp_commit(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
@@ -3011,7 +3028,6 @@ void qmp_block_stream(const char *device,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
     AioContext *aio_context;
@@ -3022,22 +3038,14 @@ void qmp_block_stream(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/qapi/block-core.json b/qapi/block-core.json
index ac8f5f6..f069e40 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1413,7 +1413,7 @@
 # On successful completion the image file is updated to drop the backing file
 # and the BLOCK_JOB_COMPLETED event is emitted.
 #
-# @device: the device name
+# @device: the device name or node-name of a root node
 #
 # @base:   #optional the common backing file name
 #
@@ -1438,9 +1438,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 6937e83..0588fd6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1118,7 +1118,7 @@ Copy data from a backing file into a block device.
 
 Arguments:
 
-- "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 v3 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>
---
 blockdev.c           | 23 +++++++++++------------
 qapi/block-core.json |  5 ++---
 qmp-commands.hx      |  2 +-
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 01e57c9..e3586f7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3091,7 +3091,6 @@ void qmp_block_commit(const char *device,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *base_bs, *top_bs;
     AioContext *aio_context;
@@ -3110,22 +3109,22 @@ void qmp_block_commit(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 f069e40..6baf6cc 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1004,7 +1004,7 @@
 # Live commit of data from overlay image nodes into backing nodes - i.e.,
 # writes data between 'top' and 'base' into 'base'.
 #
-# @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
@@ -1046,9 +1046,8 @@
 #
 # Returns: Nothing on success
 #          If commit or stream is already active on this device, DeviceInUse
-#          If @device does not exist, DeviceNotFound
 #          If image commit is not supported by this device, NotSupported
-#          If @base or @top is invalid, a generic error is returned
+#          If @device, @base or @top is invalid, a generic error is returned
 #          If @speed is invalid, InvalidParameter
 #
 # Since: 1.3
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 0588fd6..c6df92c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1162,7 +1162,7 @@ data between 'top' and 'base' into 'base'.
 
 Arguments:
 
-- "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 v3 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>
---
 blockdev.c           | 35 ++++++++++-------------------------
 qapi/block-core.json |  2 +-
 qmp-commands.hx      |  2 +-
 3 files changed, 12 insertions(+), 27 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e3586f7..d9909d0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1951,38 +1951,31 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
     BlockdevBackup *backup;
-    BlockBackend *blk, *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;
     }
 
-    target = blk_by_name(backup->target);
+    target = qmp_get_root_bs(backup->target, errp);
     if (!target) {
-        error_setg(errp, "Device '%s' not found", backup->target);
         return;
     }
 
     /* AioContext is released in .clean() */
-    state->aio_context = blk_get_aio_context(blk);
-    if (state->aio_context != blk_get_aio_context(target)) {
+    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->device, backup->target,
@@ -3355,7 +3348,6 @@ void do_blockdev_backup(const char *device, const char *target,
                          BlockdevOnError on_target_error,
                          BlockJobTxn *txn, Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     Error *local_err = NULL;
@@ -3371,21 +3363,14 @@ void do_blockdev_backup(const char *device, const char *target,
         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 6baf6cc..98bbd49 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -912,7 +912,7 @@
 ##
 # @BlockdevBackup
 #
-# @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 name of the backup target device.
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c6df92c..e24d573 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1280,7 +1280,7 @@ as backup target.
 
 Arguments:
 
-- "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 v3 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>
---
 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 d9909d0..7e9c7a4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3654,21 +3654,13 @@ void qmp_blockdev_mirror(const char *device, const char *target,
                          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 98bbd49..a64b598 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1242,7 +1242,8 @@
 #
 # Start mirroring a block device's writes to a new destination.
 #
-# @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 e24d573..45b445f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1735,7 +1735,8 @@ specifies the target of mirror operation.
 
 Arguments:
 
-- "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 v3 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>
---
 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 7e9c7a4..8e38d2d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1299,21 +1299,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) {
@@ -1329,12 +1325,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 45b445f..b16ba35 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1631,7 +1631,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 v3 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>
---
 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 8e38d2d..7a0890f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1504,7 +1504,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;
@@ -1527,23 +1526,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 b16ba35..4aebb32 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1399,7 +1399,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:
@@ -1600,7 +1601,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 v3 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>
---
 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 7a0890f..ca28ff7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3796,7 +3796,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;
@@ -3805,22 +3804,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 a64b598..6ab3c91 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -985,7 +985,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 4aebb32..e15bcf5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1794,7 +1794,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 v3 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>
---
 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 ca28ff7..5438b3c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1849,30 +1849,23 @@ static void do_drive_backup(const char *device, const char *target,
 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->device, backup->target,
                     backup->has_format, backup->format,
@@ -3175,7 +3168,6 @@ static void do_drive_backup(const char *device, const char *target,
                             BlockdevOnError on_target_error,
                             BlockJobTxn *txn, Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     BlockDriverState *source = NULL;
@@ -3199,24 +3191,14 @@ static void do_drive_backup(const char *device, const char *target,
         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 6ab3c91..09cab22 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -866,7 +866,7 @@
 ##
 # @DriveBackup
 #
-# @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
@@ -1070,7 +1070,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 e15bcf5..940ff69 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1229,7 +1229,7 @@ block-job-cancel command.
 
 Arguments:
 
-- "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 v3 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>
---
 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 5438b3c..2a2eb83 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3459,7 +3459,6 @@ void qmp_drive_mirror(const char *device, const char *target,
                       Error **errp)
 {
     BlockDriverState *bs;
-    BlockBackend *blk;
     BlockDriverState *source, *target_bs;
     AioContext *aio_context;
     BlockMirrorBackingMode backing_mode;
@@ -3468,21 +3467,14 @@ void qmp_drive_mirror(const char *device, const char *target,
     int flags;
     int64_t size;
 
-    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, QERR_DEVICE_HAS_NO_MEDIUM, device);
-        goto out;
-    }
-    bs = blk_bs(blk);
     if (!has_mode) {
         mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 09cab22..01bce7f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1108,7 +1108,8 @@
 #
 # Start mirroring a block device's writes to a new destination.
 #
-# @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
@@ -1155,7 +1156,7 @@
 #         Default is true. (Since 2.4)
 #
 # 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
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 940ff69..6da5c03 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1679,7 +1679,8 @@ of the source.
 
 Arguments:
 
-- "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 v3 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>
---
 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 823ff1d..c083829 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"
@@ -2204,6 +2205,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 eeda3eb..d89f092 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -105,8 +105,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 a677e26..1cd6d60 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 v3 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] [Qemu-block] [PATCH v3 01/11] block: Accept node-name for block-stream

[Alberto Garcia]

On Thu 07 Jul 2016 02:11:27 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>
> ---
>  blockdev.c             | 32 ++++++++++++++++++++------------
>  qapi/block-core.json   |  5 +----
>  qmp-commands.hx        |  2 +-
>  tests/qemu-iotests/030 |  2 +-
>  4 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0f8065c..01e57c9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1172,6 +1172,23 @@ 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_has_blk(bs)) {
> +        error_setg(errp, "Need a root block node");
> +        return NULL;
> +    }

Since b6d2e59995f when you create a block job a new BlockBackend is
created on top of the BDS. What happens with this check if we allow
creating jobs in a non-root BDS? 

Berto

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

[Kevin Wolf]

Am 07.07.2016 um 14:59 hat Alberto Garcia geschrieben:
> On Thu 07 Jul 2016 02:11:27 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>
> > ---
> >  blockdev.c             | 32 ++++++++++++++++++++------------
> >  qapi/block-core.json   |  5 +----
> >  qmp-commands.hx        |  2 +-
> >  tests/qemu-iotests/030 |  2 +-
> >  4 files changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 0f8065c..01e57c9 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1172,6 +1172,23 @@ 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_has_blk(bs)) {
> > +        error_setg(errp, "Need a root block node");
> > +        return NULL;
> > +    }
> 
> Since b6d2e59995f when you create a block job a new BlockBackend is
> created on top of the BDS. What happens with this check if we allow
> creating jobs in a non-root BDS? 

Hm, you mean I'd start first an intermediate streaming job and then I
can call commands on the target node that I shouldn't be able to call?
It's a good point, but I think op blockers would prevent that this
actually works.

If we wanted to keep exactly the old set of nodes that is allowed, we
could make qmp_get_root_bs() look for a _named_ BlockBackend. But that
would kind of defeat the purpose of this series, which wants to allow
these commands on named nodes that are directly used for -device.

Another option - and maybe that makes more sense than the old rule
anyway because you already can have a BB anywhere in the middle of the
graph - would be to check that the node doesn't have any non-BB parent.
This would restrict some cases that are possible today.

Or, considering that requiring a device name didn't really work as a
check because you can have BBs anywhere, just leave it. Sooner or later
we'll want all commands to work on any node anyway. I think the main
reason to require root nodes today is just op blockers.

Kevin

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

[Alberto Garcia]

On Thu 07 Jul 2016 04:17:21 PM CEST, Kevin Wolf wrote:
>> > +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_has_blk(bs)) {
>> > +        error_setg(errp, "Need a root block node");
>> > +        return NULL;
>> > +    }
>> 
>> Since b6d2e59995f when you create a block job a new BlockBackend is
>> created on top of the BDS. What happens with this check if we allow
>> creating jobs in a non-root BDS?
>
> Hm, you mean I'd start first an intermediate streaming job and then I
> can call commands on the target node that I shouldn't be able to call?
> It's a good point, but I think op blockers would prevent that this
> actually works.

Yes, they would but

a) the user would get a misleading error message ("you cannot start that
   job because the device is temporarily being used" rather than "you
   cannot start that block job there at all"). Probably not so
   important.

b) all the code after the qmp_get_root_bs() call would run under the
   (incorrect) assumption that the node is a root BDS. This probably
   doesn't break anything at the moment but leaves a door open for
   surprises.

> Another option - and maybe that makes more sense than the old rule
> anyway because you already can have a BB anywhere in the middle of the
> graph - would be to check that the node doesn't have any non-BB
> parent.  This would restrict some cases that are possible today.

Which ones?

Berto

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

[Kevin Wolf]

Am 07.07.2016 um 16:39 hat Alberto Garcia geschrieben:
> On Thu 07 Jul 2016 04:17:21 PM CEST, Kevin Wolf wrote:
> >> > +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_has_blk(bs)) {
> >> > +        error_setg(errp, "Need a root block node");
> >> > +        return NULL;
> >> > +    }
> >> 
> >> Since b6d2e59995f when you create a block job a new BlockBackend is
> >> created on top of the BDS. What happens with this check if we allow
> >> creating jobs in a non-root BDS?
> >
> > Hm, you mean I'd start first an intermediate streaming job and then I
> > can call commands on the target node that I shouldn't be able to call?
> > It's a good point, but I think op blockers would prevent that this
> > actually works.
> 
> Yes, they would but
> 
> a) the user would get a misleading error message ("you cannot start that
>    job because the device is temporarily being used" rather than "you
>    cannot start that block job there at all"). Probably not so
>    important.
> 
> b) all the code after the qmp_get_root_bs() call would run under the
>    (incorrect) assumption that the node is a root BDS. This probably
>    doesn't break anything at the moment but leaves a door open for
>    surprises.

Yes, I understand that. The truth is that our current op blockers just
don't quite cut it and we need to move away from them so that we can
finally allow jobs on every node without giving up safety.

> > Another option - and maybe that makes more sense than the old rule
> > anyway because you already can have a BB anywhere in the middle of the
> > graph - would be to check that the node doesn't have any non-BB
> > parent.  This would restrict some cases that are possible today.
> 
> Which ones?

-drive if=none,file=backing.qcow2,id=backing
-drive if=virtio,file=overlay.qcow2,backing=backing

You got a BB name for the backing file node, so can run any command that
wants a root node on it. The backing file blocker will still prevent
most actions, but that's the same situation as with node names.

So I guess the new surprises won't be any worse. :-)

Kevin

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

[Eric Blake]

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

On 07/07/2016 06:11 AM, 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>
> ---
>  blockdev.c             | 32 ++++++++++++++++++++------------
>  qapi/block-core.json   |  5 +----
>  qmp-commands.hx        |  2 +-
>  tests/qemu-iotests/030 |  2 +-
>  4 files changed, 23 insertions(+), 18 deletions(-)
> 

The interface change looks okay; but due to Berto's comments, I'm not
sure it is worth giving R-b yet if you plan on changing the check for
whether a node name properly qualifies as a root name.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Eric Blake]

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

On 07/07/2016 06:11 AM, 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.

Indeed, and good thing we had the comment in the code to remind us.

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

> +++ b/qapi/block-core.json
> @@ -1004,7 +1004,7 @@
>  # Live commit of data from overlay image nodes into backing nodes - i.e.,
>  # writes data between 'top' and 'base' into 'base'.
>  #
> -# @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
> @@ -1046,9 +1046,8 @@
>  #
>  # Returns: Nothing on success
>  #          If commit or stream is already active on this device, DeviceInUse
> -#          If @device does not exist, DeviceNotFound

Except deleting this line is at odds with the special casing you added.

>  #          If image commit is not supported by this device, NotSupported
> -#          If @base or @top is invalid, a generic error is returned
> +#          If @device, @base or @top is invalid, a generic error is returned
>  #          If @speed is invalid, InvalidParameter
>  #

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Kevin Wolf]

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

Am 08.07.2016 um 00:45 hat Eric Blake geschrieben:
> On 07/07/2016 06:11 AM, 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>
> > ---
> >  blockdev.c             | 32 ++++++++++++++++++++------------
> >  qapi/block-core.json   |  5 +----
> >  qmp-commands.hx        |  2 +-
> >  tests/qemu-iotests/030 |  2 +-
> >  4 files changed, 23 insertions(+), 18 deletions(-)
> > 
> 
> The interface change looks okay; but due to Berto's comments, I'm not
> sure it is worth giving R-b yet if you plan on changing the check for
> whether a node name properly qualifies as a root name.

Initially I intended to address the comment with some change, but since
I realised that you already can put a BB everywhere and therefore this
doesn't protect anything against intentional actions anyway, I'm not so
sure any more.

Do you have an opintion on this? More input would be appreciated.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Eric Blake]

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

On 07/08/2016 04:01 AM, Kevin Wolf wrote:
> Am 08.07.2016 um 00:45 hat Eric Blake geschrieben:
>> On 07/07/2016 06:11 AM, 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>
>>> ---
>>>  blockdev.c             | 32 ++++++++++++++++++++------------
>>>  qapi/block-core.json   |  5 +----
>>>  qmp-commands.hx        |  2 +-
>>>  tests/qemu-iotests/030 |  2 +-
>>>  4 files changed, 23 insertions(+), 18 deletions(-)
>>>
>>
>> The interface change looks okay; but due to Berto's comments, I'm not
>> sure it is worth giving R-b yet if you plan on changing the check for
>> whether a node name properly qualifies as a root name.
> 
> Initially I intended to address the comment with some change, but since
> I realised that you already can put a BB everywhere and therefore this
> doesn't protect anything against intentional actions anyway, I'm not so
> sure any more.
> 
> Do you have an opintion on this? More input would be appreciated.

I still need to re-read the other sub-thread closely, but yes, I'll try
to chime in after I've had a chance to think about implications.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Kevin Wolf]

Am 07.07.2016 um 16:17 hat Kevin Wolf geschrieben:
> Am 07.07.2016 um 14:59 hat Alberto Garcia geschrieben:
> > On Thu 07 Jul 2016 02:11:27 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>
> > > ---
> > >  blockdev.c             | 32 ++++++++++++++++++++------------
> > >  qapi/block-core.json   |  5 +----
> > >  qmp-commands.hx        |  2 +-
> > >  tests/qemu-iotests/030 |  2 +-
> > >  4 files changed, 23 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/blockdev.c b/blockdev.c
> > > index 0f8065c..01e57c9 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -1172,6 +1172,23 @@ 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_has_blk(bs)) {
> > > +        error_setg(errp, "Need a root block node");
> > > +        return NULL;
> > > +    }
> > 
> > Since b6d2e59995f when you create a block job a new BlockBackend is
> > created on top of the BDS. What happens with this check if we allow
> > creating jobs in a non-root BDS? 
> 
> Hm, you mean I'd start first an intermediate streaming job and then I
> can call commands on the target node that I shouldn't be able to call?
> It's a good point, but I think op blockers would prevent that this
> actually works.
> 
> If we wanted to keep exactly the old set of nodes that is allowed, we
> could make qmp_get_root_bs() look for a _named_ BlockBackend. But that
> would kind of defeat the purpose of this series, which wants to allow
> these commands on named nodes that are directly used for -device.
> 
> Another option - and maybe that makes more sense than the old rule
> anyway because you already can have a BB anywhere in the middle of the
> graph - would be to check that the node doesn't have any non-BB parent.

This is what I'm implementing now. The reason for this is that
bdrv_has_blk() obviously rejects configurations where you have only a
node name, but no BB. And the whole point of the series is to move
towards a model without named BBs, so this would mean that you can only
run block job on attached nodes, which doesn't make a lot of sense (and
gives qemu-iotests some trouble).

With this option implemented, a node that isn't attached anywhere can be
used for root node commands, as it should.

Kevin