Patchwork [v4,4/6] block QMP: Deprecate query-block's "type", drop info block's "type="

login
register
mail settings
Submitter Markus Armbruster
Date May 16, 2011, 1:04 p.m.
Message ID <1305551097-11387-5-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/95744/
State New
Headers show

Comments

Markus Armbruster - May 16, 2011, 1:04 p.m.
query-block's specification documents response member "type" with
values "hd", "cdrom", "floppy", "unknown".

Its value is unreliable: a block device used as floppy has type
"floppy" if created with if=floppy, but type "hd" if created with
if=none.

That's because with if=none, the type is at best a declaration of
intent: the drive can be connected to any guest device.  Its type is
really the guest device's business.  Reporting it here is wrong.

No known user of QMP uses "type".  It's unlikely that any unknown
users exist, because its value is useless unless you know how the
block device was created.  But then you also know the true value.

Fixing the broken value risks breaking (hypothetical!) clients that
somehow rely on the current behavior.  Not fixing the value risks
breaking (hypothetical!) clients that rely on the value to be
accurate.  Can't entirely avoid hypothetical lossage.  Change the
value to be always "unknown".

This makes "info block" always report "type=unknown".  Pointless.
Change it to not report the type.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c         |   20 +++-----------------
 qmp-commands.hx |   11 ++++++-----
 2 files changed, 9 insertions(+), 22 deletions(-)
Kevin Wolf - May 18, 2011, 12:44 p.m.
Am 16.05.2011 15:04, schrieb Markus Armbruster:
> query-block's specification documents response member "type" with
> values "hd", "cdrom", "floppy", "unknown".
> 
> Its value is unreliable: a block device used as floppy has type
> "floppy" if created with if=floppy, but type "hd" if created with
> if=none.
> 
> That's because with if=none, the type is at best a declaration of
> intent: the drive can be connected to any guest device.  Its type is
> really the guest device's business.  Reporting it here is wrong.
> 
> No known user of QMP uses "type".  It's unlikely that any unknown
> users exist, because its value is useless unless you know how the
> block device was created.  But then you also know the true value.
> 
> Fixing the broken value risks breaking (hypothetical!) clients that
> somehow rely on the current behavior.  Not fixing the value risks
> breaking (hypothetical!) clients that rely on the value to be
> accurate.  Can't entirely avoid hypothetical lossage.  Change the
> value to be always "unknown".
> 
> This makes "info block" always report "type=unknown".  Pointless.
> Change it to not report the type.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Luiz/Anthony, I'm not sure if Markus asked you, but I'm waiting for an
Ack from one of you here before merging this series. Just in case
someone wonders why nothing has happened yet.

Kevin
Luiz Capitulino - May 18, 2011, 1:08 p.m.
On Wed, 18 May 2011 14:44:11 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 16.05.2011 15:04, schrieb Markus Armbruster:
> > query-block's specification documents response member "type" with
> > values "hd", "cdrom", "floppy", "unknown".
> > 
> > Its value is unreliable: a block device used as floppy has type
> > "floppy" if created with if=floppy, but type "hd" if created with
> > if=none.
> > 
> > That's because with if=none, the type is at best a declaration of
> > intent: the drive can be connected to any guest device.  Its type is
> > really the guest device's business.  Reporting it here is wrong.
> > 
> > No known user of QMP uses "type".  It's unlikely that any unknown
> > users exist, because its value is useless unless you know how the
> > block device was created.  But then you also know the true value.
> > 
> > Fixing the broken value risks breaking (hypothetical!) clients that
> > somehow rely on the current behavior.  Not fixing the value risks
> > breaking (hypothetical!) clients that rely on the value to be
> > accurate.  Can't entirely avoid hypothetical lossage.  Change the
> > value to be always "unknown".
> > 
> > This makes "info block" always report "type=unknown".  Pointless.
> > Change it to not report the type.
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Luiz/Anthony, I'm not sure if Markus asked you, but I'm waiting for an
> Ack from one of you here before merging this series. Just in case
> someone wonders why nothing has happened yet.

Sorry for the delay:

ACK

Honestly, I'm a bit reluctant because we don't provide an alternative.
Yes, I understand the value is unreliable, but as far as I understand it
has reliable usages and it's the only way a client can currently learn
about the VM's block devices. However, we don't want new clients to rely
on this field, we're not sure it's used and we're not breaking the
protocol, anyway.

Patch

diff --git a/block.c b/block.c
index f403718..9de7450 100644
--- a/block.c
+++ b/block.c
@@ -1704,9 +1704,8 @@  static void bdrv_print_dict(QObject *obj, void *opaque)
 
     bs_dict = qobject_to_qdict(obj);
 
-    monitor_printf(mon, "%s: type=%s removable=%d",
+    monitor_printf(mon, "%s: removable=%d",
                         qdict_get_str(bs_dict, "device"),
-                        qdict_get_str(bs_dict, "type"),
                         qdict_get_bool(bs_dict, "removable"));
 
     if (qdict_get_bool(bs_dict, "removable")) {
@@ -1747,23 +1746,10 @@  void bdrv_info(Monitor *mon, QObject **ret_data)
 
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
         QObject *bs_obj;
-        const char *type = "unknown";
-
-        switch(bs->type) {
-        case BDRV_TYPE_HD:
-            type = "hd";
-            break;
-        case BDRV_TYPE_CDROM:
-            type = "cdrom";
-            break;
-        case BDRV_TYPE_FLOPPY:
-            type = "floppy";
-            break;
-        }
 
-        bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': %s, "
+        bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
                                     "'removable': %i, 'locked': %i }",
-                                    bs->device_name, type, bs->removable,
+                                    bs->device_name, bs->removable,
                                     bs->locked);
 
         if (bs->drv) {
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fbd98ee..a9f109a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1039,7 +1039,8 @@  Each json-object contain the following:
 
 - "device": device name (json-string)
 - "type": device type (json-string)
-         - Possible values: "hd", "cdrom", "floppy", "unknown"
+         - deprecated, retained for backward compatibility
+         - Possible values: "unknown"
 - "removable": true if the device is removable, false otherwise (json-bool)
 - "locked": true if the device is locked, false otherwise (json-bool)
 - "inserted": only present if the device is inserted, it is a json-object
@@ -1070,25 +1071,25 @@  Example:
                "encrypted":false,
                "file":"disks/test.img"
             },
-            "type":"hd"
+            "type":"unknown"
          },
          {
             "device":"ide1-cd0",
             "locked":false,
             "removable":true,
-            "type":"cdrom"
+            "type":"unknown"
          },
          {
             "device":"floppy0",
             "locked":false,
             "removable":true,
-            "type": "floppy"
+            "type":"unknown"
          },
          {
             "device":"sd0",
             "locked":false,
             "removable":true,
-            "type":"floppy"
+            "type":"unknown"
          }
       ]
    }