[04/10] block: Accept device model name for blockdev-open/close-tray
diff mbox

Message ID 1471625435-6190-5-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf Aug. 19, 2016, 4:50 p.m. UTC
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(-)

Comments

Eric Blake Sept. 14, 2016, 8:49 p.m. UTC | #1
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>
Kevin Wolf Sept. 15, 2016, 8:35 a.m. UTC | #2
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
Eric Blake Sept. 15, 2016, 3:51 p.m. UTC | #3
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.
Kevin Wolf Sept. 19, 2016, 3:10 p.m. UTC | #4
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

Patch
diff mbox

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: