[05/10] block: Accept device model name for x-blockdev-insert-medium
diff mbox

Message ID 1471625435-6190-6-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 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(-)

Comments

Eric Blake Sept. 14, 2016, 8:57 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 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?
Kevin Wolf Sept. 15, 2016, 8:37 a.m. UTC | #2
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
Eric Blake Sept. 15, 2016, 3:53 p.m. UTC | #3
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>

Patch
diff mbox

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: