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

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

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                     | 196 +++++++++++++----------------------------
 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, 158 insertions(+), 194 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v4 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                     | 32 ++++++++++++++++++++------------
 include/sysemu/block-backend.h |  1 +
 qapi/block-core.json           |  5 +----
 qmp-commands.hx                |  2 +-
 tests/qemu-iotests/030         |  2 +-
 6 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index f9cea1b..4a00f8a 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 384ad3b..739906e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1174,6 +1174,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_is_root_node(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");
@@ -3014,7 +3031,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;
@@ -3025,22 +3041,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 3c3e82f..e1c89d8 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 3444a9b..d9958d5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1434,7 +1434,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
 #
@@ -1459,9 +1459,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 c46c65c..68fb925 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 v4 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 |  2 +-
 qmp-commands.hx      |  2 +-
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 739906e..22bf03d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3094,7 +3094,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;
@@ -3113,22 +3112,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 d9958d5..61f8fd7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1016,7 +1016,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 68fb925..047b6f1 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 v4 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 22bf03d..e5a5642 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1954,38 +1954,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->has_job_id ? backup->job_id : NULL,
@@ -3361,7 +3354,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;
@@ -3377,21 +3369,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 61f8fd7..728b5f7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -921,7 +921,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 name of the backup target device.
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 047b6f1..e7608f2 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 v4 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 e5a5642..7701f73 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3663,21 +3663,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 728b5f7..6266748 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1261,7 +1261,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 e7608f2..4f6a474 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 v4 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 7701f73..8e024ff 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1301,21 +1301,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) {
@@ -1331,12 +1327,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 4f6a474..d495b73 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 v4 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 8e024ff..05f7ad0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1506,7 +1506,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;
@@ -1529,23 +1528,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 d495b73..0283387 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 v4 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 05f7ad0..6b1f2f0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3791,7 +3791,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;
@@ -3800,22 +3799,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 6266748..a73cd0d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -994,7 +994,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 0283387..7e643f3 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 v4 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 6b1f2f0..b89b5f8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1851,30 +1851,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,
@@ -3179,7 +3172,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;
@@ -3203,24 +3195,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 a73cd0d..ab885e8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -872,7 +872,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
@@ -1083,7 +1083,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 7e643f3..5586546 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 v4 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 b89b5f8..2d38537 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3467,7 +3467,6 @@ void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
                       Error **errp)
 {
     BlockDriverState *bs;
-    BlockBackend *blk;
     BlockDriverState *source, *target_bs;
     AioContext *aio_context;
     BlockMirrorBackingMode backing_mode;
@@ -3476,21 +3475,14 @@ void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
     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 ab885e8..8c92918 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1124,7 +1124,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
@@ -1171,7 +1172,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 5586546..2c90d23 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 v4 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 67894e0..9f037db 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 fbc82de..0feabc2 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 v4 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 v4 01/11] block: Accept node-name for block-stream

[Eric Blake]

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

On 07/14/2016 07:28 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>
> ---
>  block/block-backend.c          | 16 ++++++++++++++++
>  blockdev.c                     | 32 ++++++++++++++++++++------------
>  include/sysemu/block-backend.h |  1 +
>  qapi/block-core.json           |  5 +----
>  qmp-commands.hx                |  2 +-
>  tests/qemu-iotests/030         |  2 +-
>  6 files changed, 40 insertions(+), 18 deletions(-)

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

> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index f9cea1b..4a00f8a 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;
> +}

I think you've settled on a good approach here.  Thanks for persisting
at it.

-- 
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 v4 02/11] block: Accept node-name for block-commit

[Eric Blake]

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

On 07/14/2016 07:28 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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c           | 23 +++++++++++------------
>  qapi/block-core.json |  2 +-
>  qmp-commands.hx      |  2 +-
>  3 files changed, 13 insertions(+), 14 deletions(-)
> 

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

-- 
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 v4 03/11] block: Accept node-name for blockdev-backup

[Eric Blake]

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

On 07/14/2016 07:28 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
> 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(-)
> 

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

-- 
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 v4 04/11] block: Accept node-name for blockdev-mirror

[Eric Blake]

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

On 07/14/2016 07:28 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
> 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(-)
> 

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

-- 
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 v4 05/11] block: Accept node-name for blockdev-snapshot-delete-internal-sync

[Eric Blake]

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

On 07/14/2016 07:28 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
> 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(-)
> 

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

-- 
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 v4 06/11] block: Accept node-name for blockdev-snapshot-internal-sync

[Eric Blake]

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

On 07/14/2016 07:28 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
> 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(-)
> 

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

-- 
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 v4 07/11] block: Accept node-name for change-backing-file

[Eric Blake]

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

On 07/14/2016 07:28 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
> 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(-)
> 

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

Sometimes I wonder why this command is limited to only the root node -
it seems like tweaking the backing file text of an intermediate node in
a chain would be useful to someone.  But that's not the problem for this
series.

-- 
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 v4 08/11] block: Accept node-name for drive-backup

[Eric Blake]

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

On 07/14/2016 07:28 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
> 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(-)
> 

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

-- 
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 v4 09/11] block: Accept node-name for drive-mirror

[Eric Blake]

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

On 07/14/2016 07:28 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
> 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(-)
> 

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

-- 
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 v4 10/11] nbd-server: Use a separate BlockBackend

[Eric Blake]

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

On 07/14/2016 07:28 AM, 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>
> ---
>  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: Eric Blake <eblake@redhat.com>

-- 
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 v4 11/11] nbd-server: Allow node name for nbd-server-add

[Eric Blake]

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

On 07/14/2016 07:28 AM, 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(-)
> 
> 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;

Do we want to do any sanity checking that writing should only be
permitted on a root, and that when using a node name that is not a root
that writable must be false so as not to negatively change the BDS out
of under the feet of the other root?  Do op-blockers already cover that?


-- 
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 v4 11/11] nbd-server: Allow node name for nbd-server-add

[Max Reitz]

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

On 14.07.2016 23:36, Eric Blake wrote:
> On 07/14/2016 07:28 AM, 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(-)
>>
>> 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;
> 
> Do we want to do any sanity checking that writing should only be
> permitted on a root, and that when using a node name that is not a root
> that writable must be false so as not to negatively change the BDS out
> of under the feet of the other root?  Do op-blockers already cover that?

Well, one could argue that it's possible to create an NBD server on a
non-root node today anyway, since creating BBs is not restricted to root
nodes:

blockdev-add(id=foo, other arguments...)
blockdev-add(id=bar, backing=foo, other arguments...)

And then you can create an NBD server on bar. I agree that this is not
how it should be, though. However, I think that the fact that you need
to specify a BB name for now deters people from doing stuff like that.
If you can specify a node name, people will think it's completely fine
to do so.

Also note that only allowing NBD servers to be created on a root node
doesn't really help you:

blockdev-add(node-name=foo, ...)
nbd-server-add(device=foo)
blockdev-add(id=bar, backing=foo, ...)

So, yeah, I think we just need the new op-blockers for this, I don't
think the current op blockers cover this.

Max


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

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

[Eric Blake]

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

On 07/15/2016 07:36 AM, Max Reitz wrote:
> On 14.07.2016 23:36, Eric Blake wrote:
>> On 07/14/2016 07:28 AM, 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.
>>>

>> Do we want to do any sanity checking that writing should only be
>> permitted on a root, and that when using a node name that is not a root
>> that writable must be false so as not to negatively change the BDS out
>> of under the feet of the other root?  Do op-blockers already cover that?
> 
> Well, one could argue that it's possible to create an NBD server on a
> non-root node today anyway, since creating BBs is not restricted to root
> nodes:
> 
> blockdev-add(id=foo, other arguments...)
> blockdev-add(id=bar, backing=foo, other arguments...)
> 
> And then you can create an NBD server on bar. I agree that this is not
> how it should be, though. However, I think that the fact that you need
> to specify a BB name for now deters people from doing stuff like that.
> If you can specify a node name, people will think it's completely fine
> to do so.

Creating a server on bar doesn't change the contents of foo, so I see
that as safe (foo can still be in use by other chains, and the server on
bar won't invalidate those chains).

> 
> Also note that only allowing NBD servers to be created on a root node
> doesn't really help you:
> 
> blockdev-add(node-name=foo, ...)
> nbd-server-add(device=foo)
> blockdev-add(id=bar, backing=foo, ...)

But THAT is indeed unsafe, if the server allows writes, because now the
contents of bar are at risk of being silently changed by any edits made
to foo.

So the real restriction we want is that if foo is owned by a read-write
BB (the NBD server in this case), then creating another BDS bar that
uses foo as a backing is undesirable.

> 
> So, yeah, I think we just need the new op-blockers for this, I don't
> think the current op blockers cover this.

I'm not sure either, which is why we're discussing it on list to make
sure we think about the restrictions and their 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] [PATCH v4 11/11] nbd-server: Allow node name for nbd-server-add

[Max Reitz]

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

On 15.07.2016 17:18, Eric Blake wrote:
> On 07/15/2016 07:36 AM, Max Reitz wrote:
>> On 14.07.2016 23:36, Eric Blake wrote:
>>> On 07/14/2016 07:28 AM, 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.
>>>>
> 
>>> Do we want to do any sanity checking that writing should only be
>>> permitted on a root, and that when using a node name that is not a root
>>> that writable must be false so as not to negatively change the BDS out
>>> of under the feet of the other root?  Do op-blockers already cover that?
>>
>> Well, one could argue that it's possible to create an NBD server on a
>> non-root node today anyway, since creating BBs is not restricted to root
>> nodes:
>>
>> blockdev-add(id=foo, other arguments...)
>> blockdev-add(id=bar, backing=foo, other arguments...)
>>
>> And then you can create an NBD server on bar. I agree that this is not
>> how it should be, though. However, I think that the fact that you need
>> to specify a BB name for now deters people from doing stuff like that.
>> If you can specify a node name, people will think it's completely fine
>> to do so.
> 
> Creating a server on bar doesn't change the contents of foo, so I see
> that as safe (foo can still be in use by other chains, and the server on
> bar won't invalidate those chains).

Errr, I meant foo instead of bar. Curse me for using placeholder names.

>> Also note that only allowing NBD servers to be created on a root node
>> doesn't really help you:
>>
>> blockdev-add(node-name=foo, ...)
>> nbd-server-add(device=foo)
>> blockdev-add(id=bar, backing=foo, ...)
> 
> But THAT is indeed unsafe, if the server allows writes, because now the
> contents of bar are at risk of being silently changed by any edits made
> to foo.
> 
> So the real restriction we want is that if foo is owned by a read-write
> BB (the NBD server in this case), then creating another BDS bar that
> uses foo as a backing is undesirable.

Well, it's exactly what the new op blockers should be for.

>> So, yeah, I think we just need the new op-blockers for this, I don't
>> think the current op blockers cover this.
> 
> I'm not sure either, which is why we're discussing it on list to make
> sure we think about the restrictions and their implications.

Considering that Kevin is on PTO as of today, I think the best course of
action would be put the last two patches of this series off until after
2.7 is released.

Max


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

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

[Max Reitz]

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

On 14.07.2016 15:28, 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>
> ---
>  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 739906e..22bf03d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3094,7 +3094,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;
> @@ -3113,22 +3112,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);
> +        }

I would have liked a comment here why this code exists; including an
explanation what libvirt uses DeviceNotFound for (i.e. for probing
whether qemu supports active commit).

Care to add that later?

Max

>          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 d9958d5..61f8fd7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1016,7 +1016,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 68fb925..047b6f1 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)
> 



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

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

[Max Reitz]

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

On 14.07.2016 15:28, 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>
> ---
>  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 22bf03d..e5a5642 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1954,38 +1954,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);

As of recently (0d978913), do_blockdev_backup() uses bdrv_lookup_bs()
for the target. While I think it was a mistake for that commit not to
change this place to bdrv_lookup_bs() as well, I think this patch should
either not touch this place at all, or it should fix it by using
bdrv_lookup_bs().

Max

>      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->has_job_id ? backup->job_id : NULL,
> @@ -3361,7 +3354,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;
> @@ -3377,21 +3369,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 61f8fd7..728b5f7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -921,7 +921,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 name of the backup target device.
>  #
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 047b6f1..e7608f2 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;
> 



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

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

[Max Reitz]

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

On 14.07.2016 22:44, Eric Blake wrote:
> On 07/14/2016 07:28 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
>> 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(-)
>>
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Sometimes I wonder why this command is limited to only the root node -
> it seems like tweaking the backing file text of an intermediate node in
> a chain would be useful to someone.  But that's not the problem for this
> series.

Is it? The main argument of change-backing-file is not @device but
@image-node-name. @device is just used for a sanity check.

Max


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

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

[Max Reitz]

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

On 14.07.2016 15:28, 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>
> ---
>  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 6b1f2f0..b89b5f8 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> @@ -3203,24 +3195,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. */

This comment makes a good point, bs->drv may be NULL. I think you should
either call bdrv_is_inserted() or check bs->drv directly.

Max

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


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

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

[Max Reitz]

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

On 14.07.2016 15:28, 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>
> ---
>  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 b89b5f8..2d38537 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3467,7 +3467,6 @@ void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
>                        Error **errp)
>  {
>      BlockDriverState *bs;
> -    BlockBackend *blk;
>      BlockDriverState *source, *target_bs;
>      AioContext *aio_context;
>      BlockMirrorBackingMode backing_mode;
> @@ -3476,21 +3475,14 @@ void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
>      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;
>      }

After this, bs->drv may be used. So I think we should keep a
bdrv_is_inserted() or bs->drv check here.

Max


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

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

[Eric Blake]

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

On 07/18/2016 07:38 AM, Max Reitz wrote:
> On 14.07.2016 15:28, 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.
>>

>> @@ -3113,22 +3112,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. */

Isn't this the [tail end of the] comment...

>> -    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);
>> +        }
> 
> I would have liked a comment here why this code exists; including an
> explanation what libvirt uses DeviceNotFound for (i.e. for probing
> whether qemu supports active commit).

...you are wanting?

-- 
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 v4 02/11] block: Accept node-name for block-commit

[Max Reitz]

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

On 18.07.2016 18:13, Eric Blake wrote:
> On 07/18/2016 07:38 AM, Max Reitz wrote:
>> On 14.07.2016 15:28, 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.
>>>
> 
>>> @@ -3113,22 +3112,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. */
> 
> Isn't this the [tail end of the] comment...
> 
>>> -    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);
>>> +        }
>>
>> I would have liked a comment here why this code exists; including an
>> explanation what libvirt uses DeviceNotFound for (i.e. for probing
>> whether qemu supports active commit).
> 
> ...you are wanting?

Hm! Indeed it is. No complaints left for this patch, then.

Max


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

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

[Alberto Garcia]

On Thu 14 Jul 2016 03:28:04 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 v4 02/11] block: Accept node-name for block-commit

[Alberto Garcia]

On Thu 14 Jul 2016 03:28:05 PM CEST, Kevin Wolf wrote:
> -    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;

It seems that you're calling bdrv_lookup_bs() here twice, once in
qmp_get_root_bs() and then again directly. If the purpose is to see
whether the error is "device not found" or "device is not a root node" I
think the code would be clearer if you do everything here.

On a related note, you're keeping the DeviceNotFound error here, but not
in block-stream. Wouldn't it be better to keep both APIs consistent?

Berto

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

[Alberto Garcia]

On Thu 14 Jul 2016 03:28:07 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
> 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: Alberto Garcia <berto@igalia.com>

Berto

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

[Alberto Garcia]

On Thu 14 Jul 2016 03:28:08 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
> 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: Alberto Garcia <berto@igalia.com>

Berto

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

[Alberto Garcia]

On Thu 14 Jul 2016 03:28:09 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
> 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: Alberto Garcia <berto@igalia.com>

Berto

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

[Alberto Garcia]

On Thu 14 Jul 2016 03:28:11 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
> 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: Alberto Garcia <berto@igalia.com>

Berto

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

[Alberto Garcia]

On Thu 14 Jul 2016 03:28:10 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
> 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: Alberto Garcia <berto@igalia.com>

Berto

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

[Kevin Wolf]

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

Am 18.07.2016 um 16:30 hat Max Reitz geschrieben:
> On 14.07.2016 15:28, 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>

> > -    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;
> >      }
> 
> After this, bs->drv may be used. So I think we should keep a
> bdrv_is_inserted() or bs->drv check here.

Do BDSes with bs->drv == NULL still happen normally, i.e. other than in
cases where a driver gives up on a broken image? If not, maybe doing
that check in qmp_get_root_bs() would be best.

Kevin

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

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

[Kevin Wolf]

Am 01.08.2016 um 15:35 hat Alberto Garcia geschrieben:
> On Thu 14 Jul 2016 03:28:05 PM CEST, Kevin Wolf wrote:
> > -    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;
> 
> It seems that you're calling bdrv_lookup_bs() here twice, once in
> qmp_get_root_bs() and then again directly. If the purpose is to see
> whether the error is "device not found" or "device is not a root node" I
> think the code would be clearer if you do everything here.

Hm, I would like every command that needs a root node to go through
qmp_get_root_bs() for consistency. You're right that the extra code is
just for checking which error class we need to use.

> On a related note, you're keeping the DeviceNotFound error here, but not
> in block-stream. Wouldn't it be better to keep both APIs consistent?

This is the only instance that libvirt actually makes use of, so we have
to keep it. The others just happened to use the error class for no
specific reason, so they should have been GenericError from the start.

Kevin

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

[Kevin Wolf]

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

Am 18.07.2016 um 15:59 hat Max Reitz geschrieben:
> On 14.07.2016 15:28, 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>
> > ---
> >  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 22bf03d..e5a5642 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1954,38 +1954,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);
> 
> As of recently (0d978913), do_blockdev_backup() uses bdrv_lookup_bs()
> for the target. While I think it was a mistake for that commit not to
> change this place to bdrv_lookup_bs() as well, I think this patch should
> either not touch this place at all, or it should fix it by using
> bdrv_lookup_bs().

I'll first fix target in a separate patch and also update the
documentation which 0d978913 neglected to do, and then I'll leave target
alone in this patch.

Kevin

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

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

[Max Reitz]

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

On 02.08.2016 18:19, Kevin Wolf wrote:
> Am 18.07.2016 um 16:30 hat Max Reitz geschrieben:
>> On 14.07.2016 15:28, 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>
> 
>>> -    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;
>>>      }
>>
>> After this, bs->drv may be used. So I think we should keep a
>> bdrv_is_inserted() or bs->drv check here.
> 
> Do BDSes with bs->drv == NULL still happen normally, i.e. other than in
> cases where a driver gives up on a broken image? If not, maybe doing
> that check in qmp_get_root_bs() would be best.

Well, while corrupt qcow2 images aren't strictly normal, it can happen
and I don't think qemu should crash if one is encountered.

Max


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