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

2016-09-20 Thread Eric Blake 
On 09/20/2016 06:38 AM, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the

What you have works, but still feels a bit awkward to this native
speaker.  You could get away with the shorter:

In order to remove the need for BlockBackend names...

Up to you if you want to tweak the series (many commits share this pattern).

> 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 
> ---
>  blockdev.c| 61 
> ++-
>  docs/qmp-commands.txt | 12 ++
>  qapi/block-core.json  | 14 
>  3 files changed, 64 insertions(+), 23 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

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

2016-09-20 Thread 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 
---
 blockdev.c| 61 ++-
 docs/qmp-commands.txt | 12 ++
 qapi/block-core.json  | 14 
 3 files changed, 64 insertions(+), 23 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index fb207cd..046f9c6 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",
@@ -1198,6 +1199,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");
@@ -2250,7 +2274,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;
@@ -2295,15 +2319,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;
 }
 
@@ -2339,7 +2363,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;
@@ -2348,7 +2374,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;
@@ -2356,19 +2384,22 @@ 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);
+device = has_device ? device : NULL;
+id = has_id ? id : NULL;
+
+blk = qmp_get_blk(device, id, 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;
 }
 
@@ -2564,7 +2595,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;
@@ -2586,7 +2617,7 @@ void qmp_blockdev_change_medium(const