[Qemu-devel] [PATCH 00/10] block: Accept qdev IDs in device level QMP commands

[Qemu-devel] [PATCH 00/10] block: Accept qdev IDs in device level QMP commands

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the external API,
we already converted all block layer QMP commands on the node level to accept
node names instead of BlockBackend names. This series converts the second part,
device level commands, to allow qdev device names instead of BlockBackend
names.

Kevin Wolf (10):
  block: Add blk_by_dev()
  qdev-monitor: Factor out find_device_state()
  qdev-monitor: Add blk_by_qdev_id()
  block: Accept device model name for blockdev-open/close-tray
  block: Accept device model name for x-blockdev-insert-medium
  block: Accept device model name for x-blockdev-remove-medium
  block: Accept device model name for eject
  block: Accept device model name for blockdev-change-medium
  block: Accept device model name for block_set_io_throttle
  qemu-iotests/118: Test media change with qdev name

 block/block-backend.c          |  19 ++++++
 blockdev.c                     | 146 +++++++++++++++++++++++++++--------------
 hmp.c                          |   7 +-
 include/sysemu/block-backend.h |   2 +
 qapi/block-core.json           |  39 +++++++----
 qapi/block.json                |   7 +-
 qdev-monitor.c                 |  34 ++++++++--
 qmp-commands.hx                |  44 ++++++++-----
 qmp.c                          |   4 +-
 tests/qemu-iotests/118         |  85 +++++++++++++++++++-----
 tests/qemu-iotests/iotests.py  |   5 ++
 11 files changed, 288 insertions(+), 104 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/10] block: Add blk_by_dev()

[Kevin Wolf]

This finds a BlockBackend given the device model that is attached to it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/block-backend.c          | 19 +++++++++++++++++++
 include/sysemu/block-backend.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index d1349d9..0bd19ab 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -560,6 +560,25 @@ void *blk_get_attached_dev(BlockBackend *blk)
 }
 
 /*
+ * Return the BlockBackend which has the device model @dev attached if it
+ * exists, else null.
+ *
+ * @dev must not be null.
+ */
+BlockBackend *blk_by_dev(void *dev)
+{
+    BlockBackend *blk = NULL;
+
+    assert(dev != NULL);
+    while ((blk = blk_all_next(blk)) != NULL) {
+        if (blk->dev == dev) {
+            return blk;
+        }
+    }
+    return NULL;
+}
+
+/*
  * Set @blk's device model callbacks to @ops.
  * @opaque is the opaque argument to pass to the callbacks.
  * This is for use by device models.
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 4808a96..410eb68 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -111,6 +111,7 @@ int blk_attach_dev(BlockBackend *blk, void *dev);
 void blk_attach_dev_nofail(BlockBackend *blk, void *dev);
 void blk_detach_dev(BlockBackend *blk, void *dev);
 void *blk_get_attached_dev(BlockBackend *blk);
+BlockBackend *blk_by_dev(void *dev);
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
 int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
                           int count);
-- 
1.8.3.1

[Qemu-devel] [PATCH 02/10] qdev-monitor: Factor out find_device_state()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qdev-monitor.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index e19617f..bc0213f 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -801,7 +801,7 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
     object_unref(OBJECT(dev));
 }
 
-void qmp_device_del(const char *id, Error **errp)
+static DeviceState *find_device_state(const char *id, Error **errp)
 {
     Object *obj;
 
@@ -819,15 +819,23 @@ void qmp_device_del(const char *id, Error **errp)
     if (!obj) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", id);
-        return;
+        return NULL;
     }
 
     if (!object_dynamic_cast(obj, TYPE_DEVICE)) {
         error_setg(errp, "%s is not a hotpluggable device", id);
-        return;
+        return NULL;
     }
 
-    qdev_unplug(DEVICE(obj), errp);
+    return DEVICE(obj);
+}
+
+void qmp_device_del(const char *id, Error **errp)
+{
+    DeviceState *dev = find_device_state(id, errp);
+    if (dev != NULL) {
+        qdev_unplug(dev, errp);
+    }
 }
 
 void qdev_machine_init(void)
-- 
1.8.3.1

[Qemu-devel] [PATCH 03/10] qdev-monitor: Add blk_by_qdev_id()

[Kevin Wolf]

This finds the BlockBackend attached to the device model identified by
its qdev ID.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/sysemu/block-backend.h |  1 +
 qdev-monitor.c                 | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 410eb68..3b29317 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -112,6 +112,7 @@ void blk_attach_dev_nofail(BlockBackend *blk, void *dev);
 void blk_detach_dev(BlockBackend *blk, void *dev);
 void *blk_get_attached_dev(BlockBackend *blk);
 BlockBackend *blk_by_dev(void *dev);
+BlockBackend *blk_by_qdev_id(const char *id, Error **errp);
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
 int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
                           int count);
diff --git a/qdev-monitor.c b/qdev-monitor.c
index bc0213f..4f78ecb 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -28,6 +28,7 @@
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "qemu/help_option.h"
+#include "sysemu/block-backend.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -838,6 +839,23 @@ void qmp_device_del(const char *id, Error **errp)
     }
 }
 
+BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
+{
+    DeviceState *dev;
+    BlockBackend *blk;
+
+    dev = find_device_state(id, errp);
+    if (dev == NULL) {
+        return NULL;
+    }
+
+    blk = blk_by_dev(dev);
+    if (!blk) {
+        error_setg(errp, "Device does not have a block device backend");
+    }
+    return blk;
+}
+
 void qdev_machine_init(void)
 {
     qdev_get_peripheral_anon();
-- 
1.8.3.1

[Qemu-devel] [PATCH 04/10] block: Accept device model name for blockdev-open/close-tray

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow qdev device names in all device related
commands.

This converts blockdev-open/close-tray to accept a qdev device name.

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

diff --git a/blockdev.c b/blockdev.c
index 97062e3..ee3a153 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -56,7 +56,8 @@
 static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
     QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states);
 
-static int do_open_tray(const char *device, bool force, Error **errp);
+static int do_open_tray(const char *blk_name, const char *qdev_id,
+                        bool force, Error **errp);
 
 static const char *const if_name[IF_COUNT] = {
     [IF_NONE] = "none",
@@ -1196,6 +1197,29 @@ static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp)
     return bs;
 }
 
+static BlockBackend *qmp_get_blk(const char *blk_name, const char *qdev_id,
+                                 Error **errp)
+{
+    BlockBackend *blk;
+
+    if (!blk_name == !qdev_id) {
+        error_setg(errp, "Need exactly one of 'device' and 'id'");
+        return NULL;
+    }
+
+    if (qdev_id) {
+        blk = blk_by_qdev_id(qdev_id, errp);
+    } else {
+        blk = blk_by_name(blk_name);
+        if (blk == NULL) {
+            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                      "Device '%s' not found", blk_name);
+        }
+    }
+
+    return blk;
+}
+
 void hmp_commit(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
@@ -2248,7 +2272,7 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
         force = false;
     }
 
-    rc = do_open_tray(device, force, &local_err);
+    rc = do_open_tray(device, NULL, force, &local_err);
     if (rc && rc != -ENOSYS) {
         error_propagate(errp, local_err);
         return;
@@ -2293,15 +2317,15 @@ void qmp_block_passwd(bool has_device, const char *device,
  * If the guest was asked to open the tray, return -EINPROGRESS.
  * Else, return 0.
  */
-static int do_open_tray(const char *device, bool force, Error **errp)
+static int do_open_tray(const char *blk_name, const char *qdev_id,
+                        bool force, Error **errp)
 {
     BlockBackend *blk;
+    const char *device = qdev_id ?: blk_name;
     bool locked;
 
-    blk = blk_by_name(device);
+    blk = qmp_get_blk(blk_name, qdev_id, errp);
     if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
         return -ENODEV;
     }
 
@@ -2337,7 +2361,9 @@ static int do_open_tray(const char *device, bool force, Error **errp)
     return 0;
 }
 
-void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
+void qmp_blockdev_open_tray(bool has_device, const char *device,
+                            bool has_id, const char *id,
+                            bool has_force, bool force,
                             Error **errp)
 {
     Error *local_err = NULL;
@@ -2346,7 +2372,9 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
     if (!has_force) {
         force = false;
     }
-    rc = do_open_tray(device, force, &local_err);
+    rc = do_open_tray(has_device ? device : NULL,
+                      has_id ? id : NULL,
+                      force, &local_err);
     if (rc && rc != -ENOSYS && rc != -EINPROGRESS) {
         error_propagate(errp, local_err);
         return;
@@ -2354,19 +2382,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
     error_free(local_err);
 }
 
-void qmp_blockdev_close_tray(const char *device, Error **errp)
+void qmp_blockdev_close_tray(bool has_device, const char *device,
+                             bool has_id, const char *id,
+                             Error **errp)
 {
     BlockBackend *blk;
 
-    blk = blk_by_name(device);
+    blk = qmp_get_blk(has_device ? device : NULL,
+                      has_id ? id : NULL,
+                      errp);
     if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
         return;
     }
 
     if (!blk_dev_has_removable_media(blk)) {
-        error_setg(errp, "Device '%s' is not removable", device);
+        error_setg(errp, "Device '%s' is not removable", device ?: id);
         return;
     }
 
@@ -2562,7 +2592,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
         goto fail;
     }
 
-    rc = do_open_tray(device, false, &err);
+    rc = do_open_tray(device, NULL, false, &err);
     if (rc && rc != -ENOSYS) {
         error_propagate(errp, err);
         goto fail;
@@ -2584,7 +2614,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
 
     blk_apply_root_state(blk, medium_bs);
 
-    qmp_blockdev_close_tray(device, errp);
+    qmp_blockdev_close_tray(true, device, false, NULL, errp);
 
 fail:
     /* If the medium has been inserted, the device has its own reference, so
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 31f9990..069c699 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2316,7 +2316,9 @@
 #   to it
 # - if the guest device does not have an actual tray
 #
-# @device: block device name
+# @device:  block device name (deprecated, use @id instead)
+#
+# @id:      the name or QOM path of the guest device (since: 2.8)
 #
 # @force:  #optional if false (the default), an eject request will be sent to
 #          the guest if it has locked the tray (and the tray will not be opened
@@ -2326,7 +2328,8 @@
 # Since: 2.5
 ##
 { 'command': 'blockdev-open-tray',
-  'data': { 'device': 'str',
+  'data': { '*device': 'str',
+            '*id': 'str',
             '*force': 'bool' } }
 
 ##
@@ -2338,12 +2341,15 @@
 #
 # If the tray was already closed before, this will be a no-op.
 #
-# @device: block device name
+# @device:  block device name (deprecated, use @id instead)
+#
+# @id:      the name or QOM path of the guest device (since: 2.8)
 #
 # Since: 2.5
 ##
 { 'command': 'blockdev-close-tray',
-  'data': { 'device': 'str' } }
+  'data': { '*device': 'str',
+            '*id': 'str' } }
 
 ##
 # @x-blockdev-remove-medium:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ba2a916..0fd679c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4277,7 +4277,7 @@ EQMP
 
     {
         .name       = "blockdev-open-tray",
-        .args_type  = "device:s,force:b?",
+        .args_type  = "device:s?,id:s?,force:b?",
         .mhandler.cmd_new = qmp_marshal_blockdev_open_tray,
     },
 
@@ -4302,7 +4302,9 @@ which no such event will be generated, these include:
 
 Arguments:
 
-- "device": block device name (json-string)
+- "device": block device name (deprecated, use @id instead)
+            (json-string, optional)
+- "id": the name or QOM path of the guest device (json-string, optional)
 - "force": if false (the default), an eject request will be sent to the guest if
            it has locked the tray (and the tray will not be opened immediately);
            if true, the tray will be opened regardless of whether it is locked
@@ -4325,7 +4327,7 @@ EQMP
 
     {
         .name       = "blockdev-close-tray",
-        .args_type  = "device:s",
+        .args_type  = "device:s?,id:s?",
         .mhandler.cmd_new = qmp_marshal_blockdev_close_tray,
     },
 
@@ -4341,7 +4343,9 @@ If the tray was already closed before, this will be a no-op.
 
 Arguments:
 
-- "device": block device name (json-string)
+- "device": block device name (deprecated, use @id instead)
+            (json-string, optional)
+- "id": the name or QOM path of the guest device (json-string, optional)
 
 Example:
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 05/10] block: Accept device model name for x-blockdev-insert-medium

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow qdev device names in all device related
commands.

This converts x-blockdev-insert-medium to accept a qdev device name.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 33 +++++++++++++++++----------------
 qapi/block-core.json |  7 +++++--
 qmp-commands.hx      |  6 ++++--
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index ee3a153..645c639 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2465,34 +2465,26 @@ out:
     aio_context_release(aio_context);
 }
 
-static void qmp_blockdev_insert_anon_medium(const char *device,
+static void qmp_blockdev_insert_anon_medium(BlockBackend *blk,
                                             BlockDriverState *bs, Error **errp)
 {
-    BlockBackend *blk;
     bool has_device;
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
-        return;
-    }
-
     /* For BBs without a device, we can exchange the BDS tree at will */
     has_device = blk_get_attached_dev(blk);
 
     if (has_device && !blk_dev_has_removable_media(blk)) {
-        error_setg(errp, "Device '%s' is not removable", device);
+        error_setg(errp, "Device is not removable");
         return;
     }
 
     if (has_device && blk_dev_has_tray(blk) && !blk_dev_is_tray_open(blk)) {
-        error_setg(errp, "Tray of device '%s' is not open", device);
+        error_setg(errp, "Tray of the device is not open");
         return;
     }
 
     if (blk_bs(blk)) {
-        error_setg(errp, "There already is a medium in device '%s'", device);
+        error_setg(errp, "There already is a medium in the device");
         return;
     }
 
@@ -2508,11 +2500,20 @@ static void qmp_blockdev_insert_anon_medium(const char *device,
     }
 }
 
-void qmp_x_blockdev_insert_medium(const char *device, const char *node_name,
-                                  Error **errp)
+void qmp_x_blockdev_insert_medium(bool has_device, const char *device,
+                                  bool has_id, const char *id,
+                                  const char *node_name, Error **errp)
 {
+    BlockBackend *blk;
     BlockDriverState *bs;
 
+    blk = qmp_get_blk(has_device ? device : NULL,
+                      has_id ? id : NULL,
+                      errp);
+    if (!blk) {
+        return;
+    }
+
     bs = bdrv_find_node(node_name);
     if (!bs) {
         error_setg(errp, "Node '%s' not found", node_name);
@@ -2525,7 +2526,7 @@ void qmp_x_blockdev_insert_medium(const char *device, const char *node_name,
         return;
     }
 
-    qmp_blockdev_insert_anon_medium(device, bs, errp);
+    qmp_blockdev_insert_anon_medium(blk, bs, errp);
 }
 
 void qmp_blockdev_change_medium(const char *device, const char *filename,
@@ -2606,7 +2607,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
         goto fail;
     }
 
-    qmp_blockdev_insert_anon_medium(device, medium_bs, &err);
+    qmp_blockdev_insert_anon_medium(blk, medium_bs, &err);
     if (err) {
         error_propagate(errp, err);
         goto fail;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 069c699..2625bf8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2380,14 +2380,17 @@
 # This command is still a work in progress and is considered experimental.
 # Stay away from it unless you want to help with its development.
 #
-# @device:    block device name
+# @device:    block device name (deprecated, use @id instead)
+#
+# @id:        the name or QOM path of the guest device (since: 2.8)
 #
 # @node-name: name of a node in the block driver state graph
 #
 # Since: 2.5
 ##
 { 'command': 'x-blockdev-insert-medium',
-  'data': { 'device': 'str',
+  'data': { '*device': 'str',
+            '*id': 'str',
             'node-name': 'str'} }
 
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 0fd679c..b3aed7f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4412,7 +4412,7 @@ EQMP
 
     {
         .name       = "x-blockdev-insert-medium",
-        .args_type  = "device:s,node-name:s",
+        .args_type  = "device:s?,id:s?,node-name:s",
         .mhandler.cmd_new = qmp_marshal_x_blockdev_insert_medium,
     },
 
@@ -4429,7 +4429,9 @@ Stay away from it unless you want to help with its development.
 
 Arguments:
 
-- "device": block device name (json-string)
+- "device": block device name (deprecated, use @id instead)
+            (json-string, optional)
+- "id": the name or QOM path of the guest device (json-string, optional)
 - "node-name": root node of the BDS tree to insert into the block device
 
 Example:
-- 
1.8.3.1

[Qemu-devel] [PATCH 06/10] block: Accept device model name for x-blockdev-remove-medium

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow qdev device names in all device related
commands.

This converts x-blockdev-remove-medium to accept a qdev device name.

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

diff --git a/blockdev.c b/blockdev.c
index 645c639..77cf8db 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2279,7 +2279,7 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
     }
     error_free(local_err);
 
-    qmp_x_blockdev_remove_medium(device, errp);
+    qmp_x_blockdev_remove_medium(true, device, false, NULL, errp);
 }
 
 void qmp_block_passwd(bool has_device, const char *device,
@@ -2412,29 +2412,32 @@ void qmp_blockdev_close_tray(bool has_device, const char *device,
     blk_dev_change_media_cb(blk, true);
 }
 
-void qmp_x_blockdev_remove_medium(const char *device, Error **errp)
+void qmp_x_blockdev_remove_medium(bool has_device, const char *device,
+                                  bool has_id, const char *id, Error **errp)
 {
     BlockBackend *blk;
     BlockDriverState *bs;
     AioContext *aio_context;
-    bool has_device;
+    bool has_attached_device;
 
-    blk = blk_by_name(device);
+    blk = qmp_get_blk(has_device ? device : NULL,
+                      has_id ? id : NULL,
+                      errp);
     if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
         return;
     }
 
     /* For BBs without a device, we can exchange the BDS tree at will */
-    has_device = blk_get_attached_dev(blk);
+    has_attached_device = blk_get_attached_dev(blk);
 
-    if (has_device && !blk_dev_has_removable_media(blk)) {
+    if (has_attached_device && !blk_dev_has_removable_media(blk)) {
         error_setg(errp, "Device '%s' is not removable", device);
         return;
     }
 
-    if (has_device && blk_dev_has_tray(blk) && !blk_dev_is_tray_open(blk)) {
+    if (has_attached_device && blk_dev_has_tray(blk) &&
+        !blk_dev_is_tray_open(blk))
+    {
         error_setg(errp, "Tray of device '%s' is not open", device);
         return;
     }
@@ -2601,7 +2604,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
     error_free(err);
     err = NULL;
 
-    qmp_x_blockdev_remove_medium(device, &err);
+    qmp_x_blockdev_remove_medium(true, device, false, NULL, errp);
     if (err) {
         error_propagate(errp, err);
         goto fail;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2625bf8..c3177f4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2363,12 +2363,15 @@
 # This command is still a work in progress and is considered experimental.
 # Stay away from it unless you want to help with its development.
 #
-# @device: block device name
+# @device: block device name (deprecated, use @id instead)
+#
+# @id:     the name or QOM path of the guest device (since: 2.8)
 #
 # Since: 2.5
 ##
 { 'command': 'x-blockdev-remove-medium',
-  'data': { 'device': 'str' } }
+  'data': { '*device': 'str',
+            '*id': 'str' } }
 
 ##
 # @x-blockdev-insert-medium:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b3aed7f..6f10cdf 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4364,7 +4364,7 @@ EQMP
 
     {
         .name       = "x-blockdev-remove-medium",
-        .args_type  = "device:s",
+        .args_type  = "device:s?,id:s?",
         .mhandler.cmd_new = qmp_marshal_x_blockdev_remove_medium,
     },
 
@@ -4382,7 +4382,9 @@ Stay away from it unless you want to help with its development.
 
 Arguments:
 
-- "device": block device name (json-string)
+- "device": block device name (deprecated, use @id instead)
+            (json-string, optional)
+- "id": the name or QOM path of the guest device (json-string, optional)
 
 Example:
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 07/10] block: Accept device model name for eject

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow qdev device names in all device related
commands.

This converts eject to accept a qdev device name.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c      | 10 +++++++---
 hmp.c           |  2 +-
 qapi/block.json |  7 ++++++-
 qmp-commands.hx |  8 +++++---
 4 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 77cf8db..7bed3f6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2263,7 +2263,9 @@ exit:
     block_job_txn_unref(block_job_txn);
 }
 
-void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
+void qmp_eject(bool has_device, const char *device,
+               bool has_id, const char *id,
+               bool has_force, bool force, Error **errp)
 {
     Error *local_err = NULL;
     int rc;
@@ -2272,14 +2274,16 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
         force = false;
     }
 
-    rc = do_open_tray(device, NULL, force, &local_err);
+    rc = do_open_tray(has_device ? device : NULL,
+                      has_id ? id : NULL,
+                      force, &local_err);
     if (rc && rc != -ENOSYS) {
         error_propagate(errp, local_err);
         return;
     }
     error_free(local_err);
 
-    qmp_x_blockdev_remove_medium(true, device, false, NULL, errp);
+    qmp_x_blockdev_remove_medium(has_device, device, has_id, id, errp);
 }
 
 void qmp_block_passwd(bool has_device, const char *device,
diff --git a/hmp.c b/hmp.c
index ad33b44..fbd451a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1376,7 +1376,7 @@ void hmp_eject(Monitor *mon, const QDict *qdict)
     const char *device = qdict_get_str(qdict, "device");
     Error *err = NULL;
 
-    qmp_eject(device, true, force, &err);
+    qmp_eject(true, device, false, NULL, true, force, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/qapi/block.json b/qapi/block.json
index 8b08bd2..8234cca 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -127,6 +127,8 @@
 #
 # @device:  The name of the device
 #
+# @id:      The name or QOM path of the guest device (since: 2.8)
+#
 # @force:   @optional If true, eject regardless of whether the drive is locked.
 #           If not specified, the default value is false.
 #
@@ -137,7 +139,10 @@
 #
 # Since: 0.14.0
 ##
-{ 'command': 'eject', 'data': {'device': 'str', '*force': 'bool'} }
+{ 'command': 'eject',
+  'data': { '*device': 'str',
+            '*id': 'str',
+            '*force': 'bool' } }
 
 ##
 # @nbd-server-start:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6f10cdf..8822fd5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -83,7 +83,7 @@ EQMP
 
     {
         .name       = "eject",
-        .args_type  = "force:-f,device:B",
+        .args_type  = "force:-f,device:B?,id:s?",
         .mhandler.cmd_new = qmp_marshal_eject,
     },
 
@@ -95,8 +95,10 @@ Eject a removable medium.
 
 Arguments: 
 
-- force: force ejection (json-bool, optional)
-- device: device name (json-string)
+- "force": force ejection (json-bool, optional)
+- "device": block device name (deprecated, use @id instead)
+            (json-string, optional)
+- "id": the name or QOM path of the guest device (json-string, optional)
 
 Example:
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 08/10] block: Accept device model name for blockdev-change-medium

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow qdev device names in all device related
commands.

This converts blockdev-change-medium to accept a qdev device name.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 18 +++++++++++-------
 hmp.c                |  5 +++--
 qapi/block-core.json |  5 ++++-
 qmp-commands.hx      |  6 ++++--
 qmp.c                |  4 ++--
 5 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 7bed3f6..6f81b07 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2536,7 +2536,9 @@ void qmp_x_blockdev_insert_medium(bool has_device, const char *device,
     qmp_blockdev_insert_anon_medium(blk, bs, errp);
 }
 
-void qmp_blockdev_change_medium(const char *device, const char *filename,
+void qmp_blockdev_change_medium(bool has_device, const char *device,
+                                bool has_id, const char *id,
+                                const char *filename,
                                 bool has_format, const char *format,
                                 bool has_read_only,
                                 BlockdevChangeReadOnlyMode read_only,
@@ -2549,10 +2551,10 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
     QDict *options = NULL;
     Error *err = NULL;
 
-    blk = blk_by_name(device);
+    blk = qmp_get_blk(has_device ? device : NULL,
+                      has_id ? id : NULL,
+                      errp);
     if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
         goto fail;
     }
 
@@ -2600,7 +2602,9 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
         goto fail;
     }
 
-    rc = do_open_tray(device, NULL, false, &err);
+    rc = do_open_tray(has_device ? device : NULL,
+                      has_id ? id : NULL,
+                      false, &err);
     if (rc && rc != -ENOSYS) {
         error_propagate(errp, err);
         goto fail;
@@ -2608,7 +2612,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
     error_free(err);
     err = NULL;
 
-    qmp_x_blockdev_remove_medium(true, device, false, NULL, errp);
+    qmp_x_blockdev_remove_medium(has_device, device, has_id, id, errp);
     if (err) {
         error_propagate(errp, err);
         goto fail;
@@ -2622,7 +2626,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
 
     blk_apply_root_state(blk, medium_bs);
 
-    qmp_blockdev_close_tray(true, device, false, NULL, errp);
+    qmp_blockdev_close_tray(has_device, device, has_id, id, errp);
 
 fail:
     /* If the medium has been inserted, the device has its own reference, so
diff --git a/hmp.c b/hmp.c
index fbd451a..dbe426a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1422,8 +1422,9 @@ void hmp_change(Monitor *mon, const QDict *qdict)
             }
         }
 
-        qmp_blockdev_change_medium(device, target, !!arg, arg,
-                                   !!read_only, read_only_mode, &err);
+        qmp_blockdev_change_medium(true, device, false, NULL, target,
+                                   !!arg, arg, !!read_only, read_only_mode,
+                                   &err);
         if (err &&
             error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
             error_free(err);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c3177f4..9a19a50 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2425,6 +2425,8 @@
 #
 # @device:          block device name
 #
+# @id:              the name or QOM path of the guest device (since: 2.8)
+#
 # @filename:        filename of the new image to be loaded
 #
 # @format:          #optional, format to open the new image with (defaults to
@@ -2436,7 +2438,8 @@
 # Since: 2.5
 ##
 { 'command': 'blockdev-change-medium',
-  'data': { 'device': 'str',
+  'data': { '*device': 'str',
+            '*id': 'str',
             'filename': 'str',
             '*format': 'str',
             '*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8822fd5..b11e4e4 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4573,7 +4573,7 @@ EQMP
 
     {
         .name       = "blockdev-change-medium",
-        .args_type  = "device:B,filename:F,format:s?,read-only-mode:s?",
+        .args_type  = "device:B?,id:s?,filename:F,format:s?,read-only-mode:s?",
         .mhandler.cmd_new = qmp_marshal_blockdev_change_medium,
     },
 
@@ -4586,7 +4586,9 @@ and loading a new image file which is inserted as the new medium.
 
 Arguments:
 
-- "device": device name (json-string)
+- "device": block device name (deprecated, use @id instead)
+            (json-string, optional)
+- "id": the name or QOM path of the guest device (json-string, optional)
 - "filename": filename of the new image (json-string)
 - "format": format of the new image (json-string, optional)
 - "read-only-mode": new read-only mode (json-string, optional)
diff --git a/qmp.c b/qmp.c
index b6d531e..f8be3e4 100644
--- a/qmp.c
+++ b/qmp.c
@@ -446,8 +446,8 @@ void qmp_change(const char *device, const char *target,
     if (strcmp(device, "vnc") == 0) {
         qmp_change_vnc(target, has_arg, arg, errp);
     } else {
-        qmp_blockdev_change_medium(device, target, has_arg, arg, false, 0,
-                                   errp);
+        qmp_blockdev_change_medium(true, device, false, NULL, target,
+                                   has_arg, arg, false, 0, errp);
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 09/10] block: Accept device model name for block_set_io_throttle

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow qdev device names in all device related
commands.

This converts block_set_io_throttle to accept a qdev device name.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 12 +++++++-----
 qapi/block-core.json |  6 ++++--
 qmp-commands.hx      |  6 ++++--
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6f81b07..6865c2d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2643,10 +2643,10 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
     BlockBackend *blk;
     AioContext *aio_context;
 
-    blk = blk_by_name(arg->device);
+    blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
+                      arg->has_id ? arg->id : NULL,
+                      errp);
     if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", arg->device);
         return;
     }
 
@@ -2655,7 +2655,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
 
     bs = blk_bs(blk);
     if (!bs) {
-        error_setg(errp, "Device '%s' has no medium", arg->device);
+        error_setg(errp, "Device has no medium");
         goto out;
     }
 
@@ -2719,7 +2719,9 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
          * just update the throttling group. */
         if (!blk_get_public(blk)->throttle_state) {
             blk_io_limits_enable(blk,
-                                 arg->has_group ? arg->group : arg->device);
+                                 arg->has_group ? arg->group :
+                                 arg->has_device ? arg->device :
+                                 arg->id);
         } else if (arg->has_group) {
             blk_io_limits_update_group(blk, arg->group);
         }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9a19a50..b4a5bdb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1378,6 +1378,8 @@
 #
 # @device: The name of the device
 #
+# @id: the name or QOM path of the guest device (since: 2.8)
+#
 # @bps: total throughput limit in bytes per second
 #
 # @bps_rd: read throughput limit in bytes per second
@@ -1445,8 +1447,8 @@
 # Since: 1.1
 ##
 { 'struct': 'BlockIOThrottle',
-  'data': { 'device': 'str', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
-            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
+  'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
+            'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
             '*bps_max': 'int', '*bps_rd_max': 'int',
             '*bps_wr_max': 'int', '*iops_max': 'int',
             '*iops_rd_max': 'int', '*iops_wr_max': 'int',
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b11e4e4..b58d9d9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2084,7 +2084,7 @@ EQMP
 
     {
         .name       = "block_set_io_throttle",
-        .args_type  = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?,bps_max_length:l?,bps_rd_max_length:l?,bps_wr_max_length:l?,iops_max_length:l?,iops_rd_max_length:l?,iops_wr_max_length:l?,iops_size:l?,group:s?",
+        .args_type  = "device:B?,id:s?,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?,bps_max_length:l?,bps_rd_max_length:l?,bps_wr_max_length:l?,iops_max_length:l?,iops_rd_max_length:l?,iops_wr_max_length:l?,iops_size:l?,group:s?",
         .mhandler.cmd_new = qmp_marshal_block_set_io_throttle,
     },
 
@@ -2096,7 +2096,9 @@ Change I/O throttle limits for a block drive.
 
 Arguments:
 
-- "device": device name (json-string)
+- "device": block device name (deprecated, use @id instead)
+            (json-string, optional)
+- "id": the name or QOM path of the guest device (json-string, optional)
 - "bps": total throughput limit in bytes per second (json-int)
 - "bps_rd": read throughput limit in bytes per second (json-int)
 - "bps_wr": write throughput limit in bytes per second (json-int)
-- 
1.8.3.1

[Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name

[Kevin Wolf]

We just added the option to use qdev device names in all device related
block QMP commands. This patch converts some of the test cases in 118 to
use qdev device names instead of BlockBackend names to cover the new
way. It converts cases for each of the media change commands, but only
for CD-ROM and not everywhere, so that the old way is still tested, too.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/118        | 85 ++++++++++++++++++++++++++++++++++---------
 tests/qemu-iotests/iotests.py |  5 +++
 2 files changed, 73 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index 9e5951f..0380069 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -62,6 +62,9 @@ class ChangeBaseClass(iotests.QMPTestCase):
             self.fail('Timeout while waiting for the tray to close')
 
 class GeneralChangeTestsBaseClass(ChangeBaseClass):
+
+    device_name = None
+
     def test_change(self):
         result = self.vm.qmp('change', device='drive0', target=new_img,
                                        arg=iotests.imgfmt)
@@ -76,9 +79,15 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
     def test_blockdev_change_medium(self):
-        result = self.vm.qmp('blockdev-change-medium', device='drive0',
-                                                       filename=new_img,
-                                                       format=iotests.imgfmt)
+        if self.device_name is not None:
+            result = self.vm.qmp('blockdev-change-medium',
+                                 id=self.device_name, filename=new_img,
+                                 format=iotests.imgfmt)
+        else:
+            result = self.vm.qmp('blockdev-change-medium',
+                                 device='drive0', filename=new_img,
+                                 format=iotests.imgfmt)
+
         self.assert_qmp(result, 'return', {})
 
         self.wait_for_open()
@@ -90,7 +99,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
     def test_eject(self):
-        result = self.vm.qmp('eject', device='drive0', force=True)
+        if self.device_name is not None:
+            result = self.vm.qmp('eject', id=self.device_name, force=True)
+        else:
+            result = self.vm.qmp('eject', device='drive0', force=True)
         self.assert_qmp(result, 'return', {})
 
         self.wait_for_open()
@@ -101,7 +113,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.assert_qmp_absent(result, 'return[0]/inserted')
 
     def test_tray_eject_change(self):
-        result = self.vm.qmp('eject', device='drive0', force=True)
+        if self.device_name is not None:
+            result = self.vm.qmp('eject', id=self.device_name, force=True)
+        else:
+            result = self.vm.qmp('eject', device='drive0', force=True)
         self.assert_qmp(result, 'return', {})
 
         self.wait_for_open()
@@ -111,9 +126,12 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
             self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp_absent(result, 'return[0]/inserted')
 
-        result = self.vm.qmp('blockdev-change-medium', device='drive0',
-                                                       filename=new_img,
-                                                       format=iotests.imgfmt)
+        if self.device_name is not None:
+            result = self.vm.qmp('blockdev-change-medium', id=self.device_name,
+                                 filename=new_img, format=iotests.imgfmt)
+        else:
+            result = self.vm.qmp('blockdev-change-medium', device='drive0',
+                                 filename=new_img, format=iotests.imgfmt)
         self.assert_qmp(result, 'return', {})
 
         self.wait_for_close()
@@ -124,7 +142,12 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
     def test_tray_open_close(self):
-        result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True)
+        if self.device_name is not None:
+            result = self.vm.qmp('blockdev-open-tray',
+                                 id=self.device_name, force=True)
+        else:
+            result = self.vm.qmp('blockdev-open-tray',
+                                 device='drive0', force=True)
         self.assert_qmp(result, 'return', {})
 
         self.wait_for_open()
@@ -137,7 +160,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         else:
             self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
-        result = self.vm.qmp('blockdev-close-tray', device='drive0')
+        if self.device_name is not None:
+            result = self.vm.qmp('blockdev-close-tray', id=self.device_name)
+        else:
+            result = self.vm.qmp('blockdev-close-tray', device='drive0')
         self.assert_qmp(result, 'return', {})
 
         if self.has_real_tray or not self.was_empty:
@@ -162,7 +188,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
             self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp_absent(result, 'return[0]/inserted')
 
-        result = self.vm.qmp('blockdev-close-tray', device='drive0')
+        if self.device_name is not None:
+            result = self.vm.qmp('blockdev-close-tray', id=self.device_name)
+        else:
+            result = self.vm.qmp('blockdev-close-tray', device='drive0')
         self.assert_qmp(result, 'return', {})
 
         self.wait_for_close()
@@ -206,7 +235,12 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
                                                'driver': 'file'}})
         self.assert_qmp(result, 'return', {})
 
-        result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True)
+        if self.device_name is not None:
+            result = self.vm.qmp('blockdev-open-tray',
+                                 id=self.device_name, force=True)
+        else:
+            result = self.vm.qmp('blockdev-open-tray',
+                                 device='drive0', force=True)
         self.assert_qmp(result, 'return', {})
 
         self.wait_for_open()
@@ -219,7 +253,11 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         else:
             self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
-        result = self.vm.qmp('x-blockdev-remove-medium', device='drive0')
+        if self.device_name is not None:
+            result = self.vm.qmp('x-blockdev-remove-medium',
+                                 id=self.device_name)
+        else:
+            result = self.vm.qmp('x-blockdev-remove-medium', device='drive0')
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block')
@@ -227,8 +265,12 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
             self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp_absent(result, 'return[0]/inserted')
 
-        result = self.vm.qmp('x-blockdev-insert-medium', device='drive0',
-                                                       node_name='new')
+        if self.device_name is not None:
+            result = self.vm.qmp('x-blockdev-insert-medium',
+                                 id=self.device_name, node_name='new')
+        else:
+            result = self.vm.qmp('x-blockdev-insert-medium',
+                                 device='drive0', node_name='new')
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block')
@@ -236,7 +278,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
             self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
-        result = self.vm.qmp('blockdev-close-tray', device='drive0')
+        if self.device_name is not None:
+            result = self.vm.qmp('blockdev-close-tray', id=self.device_name)
+        else:
+            result = self.vm.qmp('blockdev-close-tray', device='drive0')
         self.assert_qmp(result, 'return', {})
 
         self.wait_for_close()
@@ -280,7 +325,13 @@ class TestInitiallyFilled(GeneralChangeTestsBaseClass):
     def setUp(self, media, interface):
         qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k')
         qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
-        self.vm = iotests.VM().add_drive(old_img, 'media=%s' % media, interface)
+        self.vm = iotests.VM()
+        if interface == 'ide':
+            self.device_name = 'qdev0'
+            self.vm.add_drive(old_img, 'media=%s' % media, 'none')
+            self.vm.add_device('ide-cd,drive=drive0,id=%s' % self.device_name)
+        else:
+            self.vm.add_drive(old_img, 'media=%s' % media, interface)
         self.vm.launch()
 
     def tearDown(self):
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 69aa41e..03bccdd 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -136,6 +136,11 @@ class VM(qtest.QEMUQtestMachine):
                                  socket_scm_helper=socket_scm_helper)
         self._num_drives = 0
 
+    def add_device(self, opts):
+        self._args.append('-device')
+        self._args.append(opts)
+        return self
+
     def add_drive_raw(self, opts):
         self._args.append('-drive')
         self._args.append(opts)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 00/10] block: Accept qdev IDs in device level QMP commands

[Kevin Wolf]

Am 19.08.2016 um 18:50 hat Kevin Wolf geschrieben:
> In order to remove the necessity to use BlockBackend names in the external API,
> we already converted all block layer QMP commands on the node level to accept
> node names instead of BlockBackend names. This series converts the second part,
> device level commands, to allow qdev device names instead of BlockBackend
> names.

ping?

Re: [Qemu-devel] [Qemu-block] [PATCH 00/10] block: Accept qdev IDs in device level QMP commands

[Kevin Wolf]

Am 05.09.2016 um 17:55 hat Kevin Wolf geschrieben:
> Am 19.08.2016 um 18:50 hat Kevin Wolf geschrieben:
> > In order to remove the necessity to use BlockBackend names in the external API,
> > we already converted all block layer QMP commands on the node level to accept
> > node names instead of BlockBackend names. This series converts the second part,
> > device level commands, to allow qdev device names instead of BlockBackend
> > names.
> 
> ping?

ping v2

(Maybe I should just set a timeout and simply merge the patches if
nobody has commented by then?)

Re: [Qemu-devel] [PATCH 04/10] block: Accept device model name for blockdev-open/close-tray

[Eric Blake]

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

On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow qdev device names in all device related
> commands.
> 
> This converts blockdev-open/close-tray to accept a qdev device name.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c           | 60 +++++++++++++++++++++++++++++++++++++++-------------
>  qapi/block-core.json | 14 ++++++++----
>  qmp-commands.hx      | 12 +++++++----
>  3 files changed, 63 insertions(+), 23 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -2316,7 +2316,9 @@
>  #   to it
>  # - if the guest device does not have an actual tray
>  #
> -# @device: block device name
> +# @device:  block device name (deprecated, use @id instead)
> +#
> +# @id:      the name or QOM path of the guest device (since: 2.8)

Wish there were an easier way to write mutually-exclusive pairs in JSON,
but without that, your approach is fine.

> +++ b/qmp-commands.hx
> @@ -4277,7 +4277,7 @@ EQMP
>  
>      {
>          .name       = "blockdev-open-tray",
> -        .args_type  = "device:s,force:b?",
> +        .args_type  = "device:s?,id:s?,force:b?",
>          .mhandler.cmd_new = qmp_marshal_blockdev_open_tray,
>      },

Will conflict with Marc-Andre's work to remove qmp-commands.hx; but we
can figure it out based on what merges first.

>  
> @@ -4302,7 +4302,9 @@ which no such event will be generated, these include:
>  
>  Arguments:
>  
> -- "device": block device name (json-string)
> +- "device": block device name (deprecated, use @id instead)
> +            (json-string, optional)
> +- "id": the name or QOM path of the guest device (json-string, optional)
>  - "force": if false (the default), an eject request will be sent to the guest if
>             it has locked the tray (and the tray will not be opened immediately);
>             if true, the tray will be opened regardless of whether it is locked

Are there any example code snippets that should be updated alongside
this? If not, should we be thinking of adding an example?

But I can live with this patch as an incremental improvement, even if we
decide we want more as a followup based on my question above, so:

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 05/10] block: Accept device model name for x-blockdev-insert-medium

[Eric Blake]

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

On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow qdev device names in all device related
> commands.
> 
> This converts x-blockdev-insert-medium to accept a qdev device name.
> 

Since this command is experimental...

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +++ b/qapi/block-core.json
> @@ -2380,14 +2380,17 @@
>  # This command is still a work in progress and is considered experimental.
>  # Stay away from it unless you want to help with its development.
>  #
> -# @device:    block device name
> +# @device:    block device name (deprecated, use @id instead)
> +#
> +# @id:        the name or QOM path of the guest device (since: 2.8)

...why even bother to deprecate 'device'?  Can't we just do a whole-sale
switch to a required 'id' only?  Or should such a wholesale switch be
reserved for the day that we remove the x- prefix when promoting the
command to stable?

-- 
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 06/10] block: Accept device model name for x-blockdev-remove-medium

[Eric Blake]

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

On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow qdev device names in all device related
> commands.
> 
> This converts x-blockdev-remove-medium to accept a qdev device name.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c           | 23 +++++++++++++----------
>  qapi/block-core.json |  7 +++++--
>  qmp-commands.hx      |  6 ++++--
>  3 files changed, 22 insertions(+), 14 deletions(-)

Same question as in 5/10.

-- 
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 07/10] block: Accept device model name for eject

[Eric Blake]

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

On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow qdev device names in all device related
> commands.
> 
> This converts eject to accept a qdev device name.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c      | 10 +++++++---
>  hmp.c           |  2 +-
>  qapi/block.json |  7 ++++++-
>  qmp-commands.hx |  8 +++++---
>  4 files changed, 19 insertions(+), 8 deletions(-)
> 

> +++ b/blockdev.c

> @@ -2272,14 +2274,16 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
>          force = false;
>      }
>  
> -    rc = do_open_tray(device, NULL, force, &local_err);
> +    rc = do_open_tray(has_device ? device : NULL,
> +                      has_id ? id : NULL,
> +                      force, &local_err);
>      if (rc && rc != -ENOSYS) {
>          error_propagate(errp, local_err);
>          return;
>      }
>      error_free(local_err);
>  
> -    qmp_x_blockdev_remove_medium(true, device, false, NULL, errp);
> +    qmp_x_blockdev_remove_medium(has_device, device, has_id, id, errp);

Hmm. We have to call into the underlying x- command, without breaking
'eject' which must indeed keep the deprecated 'device' parameter.  So
maybe that answers my question on 5 and 6.

Or can we teach qmp_eject() to do the lookup now, so that it can call
into qmp_x_blockdev_remove_medium() with just id, even if the user
called in with device?  Do we even have that information readily
accessible (given a device, resolve it to an id that would work as if we
were passing an id in the first place)?

> +++ b/qapi/block.json
> @@ -127,6 +127,8 @@
>  #
>  # @device:  The name of the device
>  #
> +# @id:      The name or QOM path of the guest device (since: 2.8)
> +#

Missing mention of '#optional' in both parameters, as well as the
deprecation warning you had in 4/10.

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH 00/10] block: Accept qdev IDs in device level QMP commands

[John Snow]

On 09/14/2016 09:03 AM, Kevin Wolf wrote:
> Am 05.09.2016 um 17:55 hat Kevin Wolf geschrieben:
>> Am 19.08.2016 um 18:50 hat Kevin Wolf geschrieben:
>>> In order to remove the necessity to use BlockBackend names in the external API,
>>> we already converted all block layer QMP commands on the node level to accept
>>> node names instead of BlockBackend names. This series converts the second part,
>>> device level commands, to allow qdev device names instead of BlockBackend
>>> names.
>>
>> ping?
>
> ping v2
>
> (Maybe I should just set a timeout and simply merge the patches if
> nobody has commented by then?)
>

Give me about a week.

--js

Re: [Qemu-devel] [PATCH 08/10] block: Accept device model name for blockdev-change-medium

[Eric Blake]

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

On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow qdev device names in all device related
> commands.
> 
> This converts blockdev-change-medium to accept a qdev device name.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> @@ -2608,7 +2612,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
>      error_free(err);
>      err = NULL;
>  
> -    qmp_x_blockdev_remove_medium(true, device, false, NULL, errp);
> +    qmp_x_blockdev_remove_medium(has_device, device, has_id, id, errp);

More possible churn based on the decisions on 5-6/10.

> +++ b/qapi/block-core.json
> @@ -2425,6 +2425,8 @@
>  #
>  # @device:          block device name
>  #
> +# @id:              the name or QOM path of the guest device (since: 2.8)
> +#

Missing #optional markers, and deprecation notice.

> +++ b/qmp.c
> @@ -446,8 +446,8 @@ void qmp_change(const char *device, const char *target,
>      if (strcmp(device, "vnc") == 0) {
>          qmp_change_vnc(target, has_arg, arg, errp);
>      } else {
> -        qmp_blockdev_change_medium(device, target, has_arg, arg, false, 0,
> -                                   errp);
> +        qmp_blockdev_change_medium(true, device, false, NULL, target,
> +                                   has_arg, arg, false, 0, errp);

Side note - it would be nice to get defaults into QAPI already, so we
can reduce the number of these has_FOO parameters.  But not your series'
problem.

-- 
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 09/10] block: Accept device model name for block_set_io_throttle

[Eric Blake]

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

On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow qdev device names in all device related
> commands.
> 
> This converts block_set_io_throttle to accept a qdev device name.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c           | 12 +++++++-----
>  qapi/block-core.json |  6 ++++--
>  qmp-commands.hx      |  6 ++++--
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -1378,6 +1378,8 @@
>  #
>  # @device: The name of the device
>  #
> +# @id: the name or QOM path of the guest device (since: 2.8)
> +#

Missing #optional markers, and whether device is deprecated and/or
mutually-exclusive with id.

-- 
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 10/10] qemu-iotests/118: Test media change with qdev name

[Eric Blake]

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

On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> We just added the option to use qdev device names in all device related
> block QMP commands. This patch converts some of the test cases in 118 to
> use qdev device names instead of BlockBackend names to cover the new
> way. It converts cases for each of the media change commands, but only
> for CD-ROM and not everywhere, so that the old way is still tested, too.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/118        | 85 ++++++++++++++++++++++++++++++++++---------
>  tests/qemu-iotests/iotests.py |  5 +++
>  2 files changed, 73 insertions(+), 17 deletions(-)
> 

> @@ -76,9 +79,15 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
>          self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
>  
>      def test_blockdev_change_medium(self):
> -        result = self.vm.qmp('blockdev-change-medium', device='drive0',
> -                                                       filename=new_img,
> -                                                       format=iotests.imgfmt)
> +        if self.device_name is not None:
> +            result = self.vm.qmp('blockdev-change-medium',
> +                                 id=self.device_name, filename=new_img,
> +                                 format=iotests.imgfmt)
> +        else:
> +            result = self.vm.qmp('blockdev-change-medium',
> +                                 device='drive0', filename=new_img,
> +                                 format=iotests.imgfmt)

I'm not enough of a python guru to know if there is any way to compress
this to a shorter format (I do know, however, that the lack of an
obvious ?: operator in python can indeed result in verbose if/else
clauses compared to other languages).

At any rate, the ultimate test is whether the change still passes; and
looks like you have good coverage of using exactly one or the other
argument.  Do you also want to add tests (either here, or in 11/10) that
validate that providing neither 'device' nor 'id' gives a sane error,
likewise that providing both has sane behavior?  (For now, our behavior
is that we fail, although it could also be argued that sane behavior
would validate that 'id' happens to be currently in use by 'device' and
only fail if they are not pointing to the same backend).

-- 
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 04/10] block: Accept device model name for blockdev-open/close-tray

[Kevin Wolf]

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

Am 14.09.2016 um 22:49 hat Eric Blake geschrieben:
> On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> > In order to remove the necessity to use BlockBackend names in the
> > external API, we want to allow qdev device names in all device related
> > commands.
> > 
> > This converts blockdev-open/close-tray to accept a qdev device name.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  blockdev.c           | 60 +++++++++++++++++++++++++++++++++++++++-------------
> >  qapi/block-core.json | 14 ++++++++----
> >  qmp-commands.hx      | 12 +++++++----
> >  3 files changed, 63 insertions(+), 23 deletions(-)
> > 
> 
> > +++ b/qapi/block-core.json
> > @@ -2316,7 +2316,9 @@
> >  #   to it
> >  # - if the guest device does not have an actual tray
> >  #
> > -# @device: block device name
> > +# @device:  block device name (deprecated, use @id instead)
> > +#
> > +# @id:      the name or QOM path of the guest device (since: 2.8)
> 
> Wish there were an easier way to write mutually-exclusive pairs in JSON,
> but without that, your approach is fine.
> 
> > +++ b/qmp-commands.hx
> > @@ -4277,7 +4277,7 @@ EQMP
> >  
> >      {
> >          .name       = "blockdev-open-tray",
> > -        .args_type  = "device:s,force:b?",
> > +        .args_type  = "device:s?,id:s?,force:b?",
> >          .mhandler.cmd_new = qmp_marshal_blockdev_open_tray,
> >      },
> 
> Will conflict with Marc-Andre's work to remove qmp-commands.hx; but we
> can figure it out based on what merges first.
> 
> >  
> > @@ -4302,7 +4302,9 @@ which no such event will be generated, these include:
> >  
> >  Arguments:
> >  
> > -- "device": block device name (json-string)
> > +- "device": block device name (deprecated, use @id instead)
> > +            (json-string, optional)
> > +- "id": the name or QOM path of the guest device (json-string, optional)
> >  - "force": if false (the default), an eject request will be sent to the guest if
> >             it has locked the tray (and the tray will not be opened immediately);
> >             if true, the tray will be opened regardless of whether it is locked
> 
> Are there any example code snippets that should be updated alongside
> this? If not, should we be thinking of adding an example?

We could probably update the examples to avoid deprecated fields in
them. Though the old examples still work, so is it worth changing all
examples if we're goig to remove qmp-commands.hx anyway? Or will the
examples be moved to somewhere else?

Kevin

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

Re: [Qemu-devel] [PATCH 05/10] block: Accept device model name for x-blockdev-insert-medium

[Kevin Wolf]

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

Am 14.09.2016 um 22:57 hat Eric Blake geschrieben:
> On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> > In order to remove the necessity to use BlockBackend names in the
> > external API, we want to allow qdev device names in all device related
> > commands.
> > 
> > This converts x-blockdev-insert-medium to accept a qdev device name.
> 
> Since this command is experimental...
> 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> 
> > +++ b/qapi/block-core.json
> > @@ -2380,14 +2380,17 @@
> >  # This command is still a work in progress and is considered experimental.
> >  # Stay away from it unless you want to help with its development.
> >  #
> > -# @device:    block device name
> > +# @device:    block device name (deprecated, use @id instead)
> > +#
> > +# @id:        the name or QOM path of the guest device (since: 2.8)
> 
> ...why even bother to deprecate 'device'?  Can't we just do a whole-sale
> switch to a required 'id' only?  Or should such a wholesale switch be
> reserved for the day that we remove the x- prefix when promoting the
> command to stable?

I tried this for the experimental commands, but I think it required some
more test case rewrites, so I decided to leave it for later. Not
necessarily "when promoting to stable" later, but a separate series
anyway.

Kevin

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

Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name

[Kevin Wolf]

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

Am 15.09.2016 um 00:13 hat Eric Blake geschrieben:
> On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> > We just added the option to use qdev device names in all device related
> > block QMP commands. This patch converts some of the test cases in 118 to
> > use qdev device names instead of BlockBackend names to cover the new
> > way. It converts cases for each of the media change commands, but only
> > for CD-ROM and not everywhere, so that the old way is still tested, too.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/118        | 85 ++++++++++++++++++++++++++++++++++---------
> >  tests/qemu-iotests/iotests.py |  5 +++
> >  2 files changed, 73 insertions(+), 17 deletions(-)
> > 
> 
> > @@ -76,9 +79,15 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
> >          self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
> >  
> >      def test_blockdev_change_medium(self):
> > -        result = self.vm.qmp('blockdev-change-medium', device='drive0',
> > -                                                       filename=new_img,
> > -                                                       format=iotests.imgfmt)
> > +        if self.device_name is not None:
> > +            result = self.vm.qmp('blockdev-change-medium',
> > +                                 id=self.device_name, filename=new_img,
> > +                                 format=iotests.imgfmt)
> > +        else:
> > +            result = self.vm.qmp('blockdev-change-medium',
> > +                                 device='drive0', filename=new_img,
> > +                                 format=iotests.imgfmt)
> 
> I'm not enough of a python guru to know if there is any way to compress
> this to a shorter format (I do know, however, that the lack of an
> obvious ?: operator in python can indeed result in verbose if/else
> clauses compared to other languages).

Not a Python guru either, but it does have an equivalent for the ?:
operator, just with a weird syntax:

    <value1> if <condition> else <value2>

However, I'm not sure if that would be applicable here. It depends on
whether not passing an option at all and passing None does the same
thing, or if None sends something like an empty object or JSON null.

> At any rate, the ultimate test is whether the change still passes; and
> looks like you have good coverage of using exactly one or the other
> argument.  Do you also want to add tests (either here, or in 11/10) that
> validate that providing neither 'device' nor 'id' gives a sane error,
> likewise that providing both has sane behavior?  (For now, our behavior
> is that we fail, although it could also be argued that sane behavior
> would validate that 'id' happens to be currently in use by 'device' and
> only fail if they are not pointing to the same backend).

I think failure is right. If you're using the new interface, you should
be using the new interface only.

Anyway, adding tests for this probably makes sense, I'll have a look.

Kevin

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

Re: [Qemu-devel] [PATCH 04/10] block: Accept device model name for blockdev-open/close-tray

[Eric Blake]

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

On 09/15/2016 03:35 AM, Kevin Wolf wrote:

>>>  
>>> -- "device": block device name (json-string)
>>> +- "device": block device name (deprecated, use @id instead)
>>> +            (json-string, optional)
>>> +- "id": the name or QOM path of the guest device (json-string, optional)
>>>  - "force": if false (the default), an eject request will be sent to the guest if
>>>             it has locked the tray (and the tray will not be opened immediately);
>>>             if true, the tray will be opened regardless of whether it is locked
>>
>> Are there any example code snippets that should be updated alongside
>> this? If not, should we be thinking of adding an example?
> 
> We could probably update the examples to avoid deprecated fields in
> them. Though the old examples still work, so is it worth changing all
> examples if we're goig to remove qmp-commands.hx anyway? Or will the
> examples be moved to somewhere else?

Marc-Andre is moving the examples into qapi-schema.json (and friends),
over the course of multiple commits.  It's going to be a lot of churn
and potential merge conflicts, based on what changes go into
qmp-commands.hx after his work starts, but hopefully we can avoid things
slipping through the cracks, without too much rebase pain on either
Marc-Andre or other developers.

-- 
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 05/10] block: Accept device model name for x-blockdev-insert-medium

[Eric Blake]

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

On 09/15/2016 03:37 AM, Kevin Wolf wrote:
> Am 14.09.2016 um 22:57 hat Eric Blake geschrieben:
>> On 08/19/2016 11:50 AM, Kevin Wolf wrote:
>>> In order to remove the necessity to use BlockBackend names in the
>>> external API, we want to allow qdev device names in all device related
>>> commands.
>>>
>>> This converts x-blockdev-insert-medium to accept a qdev device name.
>>
>> Since this command is experimental...
>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>
>>> +++ b/qapi/block-core.json
>>> @@ -2380,14 +2380,17 @@
>>>  # This command is still a work in progress and is considered experimental.
>>>  # Stay away from it unless you want to help with its development.
>>>  #
>>> -# @device:    block device name
>>> +# @device:    block device name (deprecated, use @id instead)
>>> +#
>>> +# @id:        the name or QOM path of the guest device (since: 2.8)
>>
>> ...why even bother to deprecate 'device'?  Can't we just do a whole-sale
>> switch to a required 'id' only?  Or should such a wholesale switch be
>> reserved for the day that we remove the x- prefix when promoting the
>> command to stable?
> 
> I tried this for the experimental commands, but I think it required some
> more test case rewrites, so I decided to leave it for later. Not
> necessarily "when promoting to stable" later, but a separate series
> anyway.

Fair enough.  Maybe mention it in the commit message, or even in a
comment that part of promoting to stable may include removing the
'driver' parameter.  That way, we won't forget the discussion.  But in
the meantime, the conversion looks sane, so I'm okay with adding:
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 10/10] qemu-iotests/118: Test media change with qdev name

[Sascha Silbe]

Dear Eric,

(replying only to the Python coding part, haven't looked at the patch
itself)

Eric Blake <eblake@redhat.com> writes:

> On 08/19/2016 11:50 AM, Kevin Wolf wrote:
>> @@ -76,9 +79,15 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
>>          self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
>>  
>>      def test_blockdev_change_medium(self):
>> -        result = self.vm.qmp('blockdev-change-medium', device='drive0',
>> -                                                       filename=new_img,
>> -                                                       format=iotests.imgfmt)
>> +        if self.device_name is not None:
>> +            result = self.vm.qmp('blockdev-change-medium',
>> +                                 id=self.device_name, filename=new_img,
>> +                                 format=iotests.imgfmt)
>> +        else:
>> +            result = self.vm.qmp('blockdev-change-medium',
>> +                                 device='drive0', filename=new_img,
>> +                                 format=iotests.imgfmt)
>
> I'm not enough of a python guru to know if there is any way to compress
> this to a shorter format (I do know, however, that the lack of an
> obvious ?: operator in python can indeed result in verbose if/else
> clauses compared to other languages).

Actually there is a direct equivalent of "?:" (which unfortunately isn't
in C11) in Python: "or". So you can use:

        result = self.vm.qmp('blockdev-change-medium',
                             id=self.device_name or 'drive0', filename=new_img,
                             format=iotests.imgfmt)

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name

[Eric Blake]

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

On 09/15/2016 11:16 AM, Sascha Silbe wrote:

>>> +        if self.device_name is not None:
>>> +            result = self.vm.qmp('blockdev-change-medium',
>>> +                                 id=self.device_name, filename=new_img,
>>> +                                 format=iotests.imgfmt)
>>> +        else:
>>> +            result = self.vm.qmp('blockdev-change-medium',
>>> +                                 device='drive0', filename=new_img,
>>> +                                 format=iotests.imgfmt)
>>
>> I'm not enough of a python guru to know if there is any way to compress
>> this to a shorter format (I do know, however, that the lack of an
>> obvious ?: operator in python can indeed result in verbose if/else
>> clauses compared to other languages).
> 
> Actually there is a direct equivalent of "?:" (which unfortunately isn't
> in C11) in Python: "or". So you can use:
> 
>         result = self.vm.qmp('blockdev-change-medium',
>                              id=self.device_name or 'drive0', filename=new_img,

Not what we want.  We want to use a different parameter, based on the
overall condition (so it is NOT as simple as a conditional assignment to
the 'id' parameter, but an actual swap between whether we have the 'id'
or the 'device' parameter).  It's not obvious whether:

result = foo(id=self.device_name,
             device='drive0' if not self.device_name else None)

would have the same semantics as:

if self.device_name:
    result = foo(id=self.device_name)
else
    result = foo(device='drive0')

and even if it does have the same semantics, it's not obvious if there's
a shorter way to write it.  So we end up with verbose duplication of all
the remaining parameters.

-- 
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 10/10] qemu-iotests/118: Test media change with qdev name

[Sascha Silbe]

Dear Eric,

Sascha Silbe <silbe@linux.vnet.ibm.com> writes:

>         result = self.vm.qmp('blockdev-change-medium',
>                              id=self.device_name or 'drive0', filename=new_img,
>                              format=iotests.imgfmt)

Sorry, my eyes deceived me. I thought the original code was setting the
"id" parameter in both cases. QEMUMachine.qmp() and
QEMUMonitorProtocol.cmd() together translate the keyword arguments
directly into QMP arguments, so passing None is different from not
passing anything at all. The if/else is probably the best approach in
that case; everything else I can think of would just be more
complicated.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name

[Eric Blake]

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

On 09/15/2016 11:28 AM, Sascha Silbe wrote:
> Dear Eric,
> 
> Sascha Silbe <silbe@linux.vnet.ibm.com> writes:
> 
>>         result = self.vm.qmp('blockdev-change-medium',
>>                              id=self.device_name or 'drive0', filename=new_img,
>>                              format=iotests.imgfmt)
> 
> Sorry, my eyes deceived me. I thought the original code was setting the
> "id" parameter in both cases. QEMUMachine.qmp() and
> QEMUMonitorProtocol.cmd() together translate the keyword arguments
> directly into QMP arguments, so passing None is different from not
> passing anything at all. The if/else is probably the best approach in
> that case; everything else I can think of would just be more
> complicated.

If there were some way to do:

arglist = (all other parameters)
if self.device_name:
    arglist.append(id=self.device_name)
else:
    arglist.append(device='drive0')
invoke(self.vm.qmp, arglist)

that might be a little cleaner (no repetition on all the other
parameters); but I don't know enough python to know if there is some
sort of invoke() mechanism for pairing a method object and an arbitrary
list as though it were a normal method call (I suspect there is, and I
just don't know 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 10/10] qemu-iotests/118: Test media change with qdev name

[Sascha Silbe]

Dear Eric,

Eric Blake <eblake@redhat.com> writes:

> arglist = (all other parameters)
> if self.device_name:
>     arglist.append(id=self.device_name)
> else:
>     arglist.append(device='drive0')
> invoke(self.vm.qmp, arglist)

That would be:

        qmp_args = {'cmd': 'blockdev-change-medium', 'filename': new_img,
                    'format': iotests.imgfmt}
        if self.device_name is not None:
            qmp_args['id'] = self.device_name
        else:
            qmp_args['device'] = 'drive0'
        result = self.vm.qmp(**qmp_args)

Or:

        qmp_args = {'filename': new_img, 'format': iotests.imgfmt}
        if self.device_name is not None:
            qmp_args['id'] = self.device_name
        else:
            qmp_args['device'] = 'drive0'
        result = self.vm.qmp('blockdev-change-medium', **qmp_args)


FWIW these days I tend to use Python magics (like *args / **kwargs) only
if there's a _significant_ improvement in either functionality or
readability; especially for code bases that are being worked on by
developers who don't speak Python "natively". Using magics sparingly
also makes it easier for static analysis tools like pylint.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [PATCH 04/10] block: Accept device model name for blockdev-open/close-tray

[Kevin Wolf]

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

Am 15.09.2016 um 17:51 hat Eric Blake geschrieben:
> On 09/15/2016 03:35 AM, Kevin Wolf wrote:
> 
> >>>  
> >>> -- "device": block device name (json-string)
> >>> +- "device": block device name (deprecated, use @id instead)
> >>> +            (json-string, optional)
> >>> +- "id": the name or QOM path of the guest device (json-string, optional)
> >>>  - "force": if false (the default), an eject request will be sent to the guest if
> >>>             it has locked the tray (and the tray will not be opened immediately);
> >>>             if true, the tray will be opened regardless of whether it is locked
> >>
> >> Are there any example code snippets that should be updated alongside
> >> this? If not, should we be thinking of adding an example?
> > 
> > We could probably update the examples to avoid deprecated fields in
> > them. Though the old examples still work, so is it worth changing all
> > examples if we're goig to remove qmp-commands.hx anyway? Or will the
> > examples be moved to somewhere else?
> 
> Marc-Andre is moving the examples into qapi-schema.json (and friends),
> over the course of multiple commits.  It's going to be a lot of churn
> and potential merge conflicts, based on what changes go into
> qmp-commands.hx after his work starts, but hopefully we can avoid things
> slipping through the cracks, without too much rebase pain on either
> Marc-Andre or other developers.

Ok, I'm changing the examples from 'device' to 'id' now. And actually
the first one immediately made me notice that the DEVICE_TRAY_MOVED
event needs to be changed, too (because its 'device' field can be empty
now). Matter for another series, but I put it on my list.

Kevin

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