Patchwork [08/10] monitor: Convert bdrv_info() to QObject

login
register
mail settings
Submitter Luiz Capitulino
Date Oct. 8, 2009, 9:35 p.m.
Message ID <1255037747-3340-9-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/35538/
State Under Review
Headers show

Comments

Luiz Capitulino - Oct. 8, 2009, 9:35 p.m.
Each block device information is stored in a QDict and the
returned QObject is a QList of all devices.

The QDict contains the following:

- "device": device name
- "type": device type
- "removable": 1 if the device is removable 0 otherwise
- "locked": 1 if the device is locked 0 otherwise
- "backing_file": backing file name if one is used
- "inserted": only present if the device is inserted, it is a QDict
  containing the following:
        - "file": device file name
        - "ro": 1 if read-only 0 otherwise
        - "drv": driver format name
        - "encrypted":  1 if encrypted 0 otherwise

The current implemention uses integers as booleans, to make
things simple those integers are stored in the QDict. Ideally,
we would have a QBool type and this is probably going to be
a requirement for the protocol.

But the integers will do the job for now.

This commit should not change user output, the following is an
example of the returned QList:

[ { "device": "ide0-hd0", "type": "hd", "removable": 0,
    "file": "/tmp/foobar", "ro": 0, "drv": "qcow2", "encrypted": 0 }
  { "device": "floppy0", "type": "floppy", "removable": 1,
  "locked": 0 } ]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 Makefile  |    1 +
 block.c   |  132 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
 block.h   |    4 +-
 monitor.c |    3 +-
 4 files changed, 118 insertions(+), 22 deletions(-)
Markus Armbruster - Oct. 10, 2009, 12:18 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> Each block device information is stored in a QDict and the
> returned QObject is a QList of all devices.
>
> The QDict contains the following:
>
> - "device": device name
> - "type": device type
> - "removable": 1 if the device is removable 0 otherwise
> - "locked": 1 if the device is locked 0 otherwise
> - "backing_file": backing file name if one is used

Isn't this in "interted" rather than here?

If yes, the comment in the code needs fixing, too.

> - "inserted": only present if the device is inserted, it is a QDict

I'd call this "media".

>   containing the following:
>         - "file": device file name
>         - "ro": 1 if read-only 0 otherwise
>         - "drv": driver format name
>         - "encrypted":  1 if encrypted 0 otherwise
>
> The current implemention uses integers as booleans, to make
> things simple those integers are stored in the QDict. Ideally,
> we would have a QBool type and this is probably going to be
> a requirement for the protocol.
>
> But the integers will do the job for now.
>
> This commit should not change user output, the following is an
> example of the returned QList:
>
> [ { "device": "ide0-hd0", "type": "hd", "removable": 0,
>     "file": "/tmp/foobar", "ro": 0, "drv": "qcow2", "encrypted": 0 }
>   { "device": "floppy0", "type": "floppy", "removable": 1,
>   "locked": 0 } ]
[...]
Luiz Capitulino - Oct. 14, 2009, 1:23 p.m.
On Sat, 10 Oct 2009 14:18:03 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Each block device information is stored in a QDict and the
> > returned QObject is a QList of all devices.
> >
> > The QDict contains the following:
> >
> > - "device": device name
> > - "type": device type
> > - "removable": 1 if the device is removable 0 otherwise
> > - "locked": 1 if the device is locked 0 otherwise
> > - "backing_file": backing file name if one is used
> 
> Isn't this in "interted" rather than here?
> 
> If yes, the comment in the code needs fixing, too.

 Yes, although I don't think I should resend just because of that.
Markus Armbruster - Oct. 14, 2009, 2:11 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Sat, 10 Oct 2009 14:18:03 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > Each block device information is stored in a QDict and the
>> > returned QObject is a QList of all devices.
>> >
>> > The QDict contains the following:
>> >
>> > - "device": device name
>> > - "type": device type
>> > - "removable": 1 if the device is removable 0 otherwise
>> > - "locked": 1 if the device is locked 0 otherwise
>> > - "backing_file": backing file name if one is used
>> 
>> Isn't this in "interted" rather than here?
>> 
>> If yes, the comment in the code needs fixing, too.
>
>  Yes, although I don't think I should resend just because of that.

Any other way to reliably avoid commit of a misleading comment with a
misleading commit message?

If it goes in that way, we need to follow up with a commit fixing the
comment.
Anthony Liguori - Oct. 15, 2009, 2:13 p.m.
Luiz Capitulino wrote:
> Each block device information is stored in a QDict and the
> returned QObject is a QList of all devices.
>
> The QDict contains the following:
>
> - "device": device name
> - "type": device type
> - "removable": 1 if the device is removable 0 otherwise
> - "locked": 1 if the device is locked 0 otherwise
> - "backing_file": backing file name if one is used
> - "inserted": only present if the device is inserted, it is a QDict
>   containing the following:
>         - "file": device file name
>         - "ro": 1 if read-only 0 otherwise
>         - "drv": driver format name
>         - "encrypted":  1 if encrypted 0 otherwise
>
> The current implemention uses integers as booleans, to make
> things simple those integers are stored in the QDict. Ideally,
> we would have a QBool type and this is probably going to be
> a requirement for the protocol.
>
> But the integers will do the job for now.
>
> This commit should not change user output, the following is an
> example of the returned QList:
>
> [ { "device": "ide0-hd0", "type": "hd", "removable": 0,
>     "file": "/tmp/foobar", "ro": 0, "drv": "qcow2", "encrypted": 0 }
>   { "device": "floppy0", "type": "floppy", "removable": 1,
>   "locked": 0 } ]
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  Makefile  |    1 +
>  block.c   |  132 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  block.h   |    4 +-
>  monitor.c |    3 +-
>  4 files changed, 118 insertions(+), 22 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 182f176..d29d871 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -62,6 +62,7 @@ recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES)
>  # block-obj-y is code used by both qemu system emulation and qemu-img
>  
>  block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
> +block-obj-y += qint.o qstring.o qlist.o qdict.o qmisc.o
>  block-obj-y += nbd.o block.o aio.o aes.o osdep.o
>  block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> diff --git a/block.c b/block.c
> index 33f3d65..31a58e4 100644
> --- a/block.c
> +++ b/block.c
> @@ -26,6 +26,10 @@
>  #include "monitor.h"
>  #include "block_int.h"
>  #include "module.h"
> +#include "qlist.h"
> +#include "qdict.h"
> +#include "qstring.h"
> +#include "qmisc.h"
>  
>  #ifdef CONFIG_BSD
>  #include <sys/types.h>
> @@ -1075,43 +1079,131 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>      return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
>  }
>  
> -void bdrv_info(Monitor *mon)
> +static void bdrv_print_dict(QObject *obj, void *opaque)
>  {
> +    QDict *bs_dict;
> +    Monitor *mon = opaque;
> +
> +    assert(qobject_type(obj) == QTYPE_QDICT);
> +    bs_dict = qobject_to_qdict(obj);
> +
> +    monitor_printf(mon, "%s: type=%s removable=%d",
> +                        qdict_get_str(bs_dict, "device"),
> +                        qdict_get_str(bs_dict, "type"),
> +                        (int) qdict_get_int(bs_dict, "removable"));
>   

This is a very common format.  So much so, it probably makes sense to have:

monitor_print_dict(mon, "device", bs_dict);

> +
> +    if (qdict_get_int(bs_dict, "removable")) {
> +        monitor_printf(mon, " locked=%d",(int)qdict_get_int(bs_dict, "locked"));
> +    }
>   

Which suggests that the dict entry should be locked, not removable.

> +    if (qdict_haskey(bs_dict, "inserted")) {
>   

I guess you could flatten nested dicts.

> +        QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
> +        monitor_printf(mon, " file=%s", qdict_get_str(qdict, "file"));
> +        if (qdict_haskey(qdict, "backing_file")) {
> +            monitor_printf(mon, " backing_file=%s",
> +                           qdict_get_str(qdict, "backing_file"));
> +        }
> +        monitor_printf(mon, " ro=%d drv=%s encrypted=%d",
> +                            (int) qdict_get_int(qdict, "ro"),
> +                            qdict_get_str(qdict, "drv"),
> +                            (int) qdict_get_int(qdict, "encrypted"));
> +    } else {
> +        monitor_printf(mon, " [not inserted]");
>   

This bit you would probably have to handle manually.

Alternatively, you could build a compat dict from the new dict format 
and then print that with the monitor_print_dict function.
   
Regards,

Anthony Liguori

Patch

diff --git a/Makefile b/Makefile
index 182f176..d29d871 100644
--- a/Makefile
+++ b/Makefile
@@ -62,6 +62,7 @@  recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES)
 # block-obj-y is code used by both qemu system emulation and qemu-img
 
 block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
+block-obj-y += qint.o qstring.o qlist.o qdict.o qmisc.o
 block-obj-y += nbd.o block.o aio.o aes.o osdep.o
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
diff --git a/block.c b/block.c
index 33f3d65..31a58e4 100644
--- a/block.c
+++ b/block.c
@@ -26,6 +26,10 @@ 
 #include "monitor.h"
 #include "block_int.h"
 #include "module.h"
+#include "qlist.h"
+#include "qdict.h"
+#include "qstring.h"
+#include "qmisc.h"
 
 #ifdef CONFIG_BSD
 #include <sys/types.h>
@@ -1075,43 +1079,131 @@  int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
 }
 
-void bdrv_info(Monitor *mon)
+static void bdrv_print_dict(QObject *obj, void *opaque)
 {
+    QDict *bs_dict;
+    Monitor *mon = opaque;
+
+    assert(qobject_type(obj) == QTYPE_QDICT);
+    bs_dict = qobject_to_qdict(obj);
+
+    monitor_printf(mon, "%s: type=%s removable=%d",
+                        qdict_get_str(bs_dict, "device"),
+                        qdict_get_str(bs_dict, "type"),
+                        (int) qdict_get_int(bs_dict, "removable"));
+
+    if (qdict_get_int(bs_dict, "removable")) {
+        monitor_printf(mon, " locked=%d",(int)qdict_get_int(bs_dict, "locked"));
+    }
+
+    if (qdict_haskey(bs_dict, "inserted")) {
+        QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
+        monitor_printf(mon, " file=%s", qdict_get_str(qdict, "file"));
+        if (qdict_haskey(qdict, "backing_file")) {
+            monitor_printf(mon, " backing_file=%s",
+                           qdict_get_str(qdict, "backing_file"));
+        }
+        monitor_printf(mon, " ro=%d drv=%s encrypted=%d",
+                            (int) qdict_get_int(qdict, "ro"),
+                            qdict_get_str(qdict, "drv"),
+                            (int) qdict_get_int(qdict, "encrypted"));
+    } else {
+        monitor_printf(mon, " [not inserted]");
+    }
+
+    monitor_printf(mon, "\n");
+}
+
+void bdrv_user_print(Monitor *mon, const QObject *data)
+{
+    QList *bs_list;
+
+    assert(qobject_type(data) == QTYPE_QLIST);
+    bs_list = qobject_to_qlist(data);
+
+    qlist_iter(bs_list, bdrv_print_dict, mon);
+}
+
+/**
+ * bdrv_info(): Show block devices
+ *
+ * Each block device information is stored in a QDict and the
+ * returned QObject is a QList of all devices.
+ *
+ * The QDict contains the following:
+ *
+ * - "device": device name
+ * - "type": device type
+ * - "removable": 1 if the device is removable 0 otherwise
+ * - "locked": 1 if the device is locked 0 otherwise
+ * - "backing_file": backing file name if one is used
+ * - "inserted": only present if the device is inserted, it is a QDict
+ *    containing the following:
+ *          - "file": device file name
+ *          - "ro": 1 if read-only 0 otherwise
+ *          - "drv": driver format name
+ *          - "encrypted":  1 if encrypted 0 otherwise
+ *
+ * Example:
+ *
+ * [ { "device": "ide0-hd0", "type": "hd", "removable": 0,
+ *     "file": "/tmp/foobar", "ro": 0, "drv": "qcow2", "encrypted": 0 }
+ *   { "device": "floppy0", "type": "floppy", "removable": 1,
+ *     "locked": 0 } ]
+ */
+void bdrv_info(Monitor *mon, QObject **ret_data)
+{
+    QList *bs_list;
     BlockDriverState *bs;
 
+    bs_list = qlist_new();
+
     for (bs = bdrv_first; bs != NULL; bs = bs->next) {
-        monitor_printf(mon, "%s:", bs->device_name);
-        monitor_printf(mon, " type=");
+        QObject *bs_obj;
+        const char *type = "unknown";
+
         switch(bs->type) {
         case BDRV_TYPE_HD:
-            monitor_printf(mon, "hd");
+            type = "hd";
             break;
         case BDRV_TYPE_CDROM:
-            monitor_printf(mon, "cdrom");
+            type = "cdrom";
             break;
         case BDRV_TYPE_FLOPPY:
-            monitor_printf(mon, "floppy");
+            type = "floppy";
             break;
         }
-        monitor_printf(mon, " removable=%d", bs->removable);
-        if (bs->removable) {
-            monitor_printf(mon, " locked=%d", bs->locked);
-        }
+
+        bs_obj = qobject_from_fmt("{ s: s, s: s, s: i, s: i }",
+                                  "device", bs->device_name,
+                                  "type", type,
+                                  "removable", bs->removable,
+                                  "locked", bs->locked);
+        assert(bs_obj != NULL);
+
         if (bs->drv) {
-            monitor_printf(mon, " file=");
-            monitor_print_filename(mon, bs->filename);
+            QObject *obj;
+            QDict *bs_dict = qobject_to_qdict(bs_obj);
+
+            obj = qobject_from_fmt("{ s: s, s: i, s: s, s: i }",
+                                   "file", bs->filename,
+                                   "ro", bs->read_only,
+                                   "drv", bs->drv->format_name,
+                                   "encrypted", bdrv_is_encrypted(bs));
+            assert(obj != NULL);
+
             if (bs->backing_file[0] != '\0') {
-                monitor_printf(mon, " backing_file=");
-                monitor_print_filename(mon, bs->backing_file);
+                QDict *qdict = qobject_to_qdict(obj);
+                qdict_put(qdict, "backing_file",
+                          qstring_from_str(bs->backing_file));
             }
-            monitor_printf(mon, " ro=%d", bs->read_only);
-            monitor_printf(mon, " drv=%s", bs->drv->format_name);
-            monitor_printf(mon, " encrypted=%d", bdrv_is_encrypted(bs));
-        } else {
-            monitor_printf(mon, " [not inserted]");
+            qdict_put_obj(bs_dict, "inserted", obj);
         }
-        monitor_printf(mon, "\n");
+
+        qlist_append_obj(bs_list, bs_obj);
     }
+
+    *ret_data = QOBJECT(bs_list);
 }
 
 /* The "info blockstats" command. */
diff --git a/block.h b/block.h
index a966afb..99dc360 100644
--- a/block.h
+++ b/block.h
@@ -4,6 +4,7 @@ 
 #include "qemu-aio.h"
 #include "qemu-common.h"
 #include "qemu-option.h"
+#include "qobject.h"
 
 /* block.c */
 typedef struct BlockDriver BlockDriver;
@@ -41,7 +42,8 @@  typedef struct QEMUSnapshotInfo {
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB)
 
-void bdrv_info(Monitor *mon);
+void bdrv_user_print(Monitor *mon, const QObject *data);
+void bdrv_info(Monitor *mon, QObject **ret_data);
 void bdrv_info_stats(Monitor *mon);
 
 void bdrv_init(void);
diff --git a/monitor.c b/monitor.c
index 7e5c07d..39d3201 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1979,7 +1979,8 @@  static const mon_cmd_t info_cmds[] = {
         .args_type  = "",
         .params     = "",
         .help       = "show the block devices",
-        .mhandler.info = bdrv_info,
+        .user_print = bdrv_user_print,
+        .mhandler.info_new = bdrv_info,
     },
     {
         .name       = "blockstats",