Re: [Qemu-devel] [PATCH 11/17] block: Convert bdrv_info() to QObject

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 20 Nov 2009 15:06:26 +0100
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.
> >
> > This commit should not change user output.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  Makefile  |    2 +-
> >  block.c   |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
> >  block.h   |    4 +-
> >  monitor.c |    3 +-
> >  4 files changed, 109 insertions(+), 23 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 6be75a1..3424cdb 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -80,7 +80,7 @@ qobject-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
> >  # 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 += nbd.o block.o aio.o aes.o osdep.o
> > +block-obj-y += nbd.o block.o aio.o aes.o osdep.o $(qobject-obj-y)
> >  block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
> >  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> >  
> 
> Both $(block-obj-y) and $(qobject-obj-y) go into obj-y, which thus list
> all the $(qobject-obj-y) objects twice.  Sure that's okay?

 $(qobject-obj-y) is a dependency of qemu- block tools, it can be moved
there then.

> > diff --git a/block.c b/block.c
> > index 6fdabff..fc4e2f2 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -26,6 +26,7 @@
> >  #include "monitor.h"
> >  #include "block_int.h"
> >  #include "module.h"
> > +#include "qemu-objects.h"
> >  
> >  #ifdef CONFIG_BSD
> >  #include <sys/types.h>
> > @@ -1133,43 +1134,125 @@ 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;
> > +
> > +    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"),
> > +                        qdict_get_bool(bs_dict, "removable"));
> > +
> > +    if (qdict_get_bool(bs_dict, "removable")) {
> > +        monitor_printf(mon, " locked=%d", (int)qdict_get_bool(bs_dict, "locked"));
> 
> qdict_get_bool() returns int, no need to cast.

 Right.

> > +    }
> > +
> > +    if (qdict_haskey(bs_dict, "inserted")) {
> > +        QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
> > +
> > +        monitor_printf(mon, " file=");
> > +        monitor_print_filename(mon, qdict_get_str(qdict, "file"));
> > +        if (qdict_haskey(qdict, "backing_file")) {
> > +            monitor_printf(mon, " backing_file=");
> > +            monitor_print_filename(mon, qdict_get_str(qdict, "backing_file"));
> > +        }
> > +        monitor_printf(mon, " ro=%d drv=%s encrypted=%d",
> > +                            qdict_get_bool(qdict, "ro"),
> > +                            qdict_get_str(qdict, "drv"),
> > +                            qdict_get_bool(qdict, "encrypted"));
> > +    } else {
> > +        monitor_printf(mon, " [not inserted]");
> > +    }
> > +
> > +    monitor_printf(mon, "\n");
> > +}
> > +
> > +void bdrv_info_print(Monitor *mon, const QObject *data)
> > +{
> > +    qlist_iter(qobject_to_qlist(data), bdrv_print_dict, mon);
> > +}
> > +
> > +/**
> > + * bdrv_info(): Block devices information
> > + *
> > + * 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
> 
> Use above and example below suggest that "locked" is only present if
> device is removable.  Document?

 Yes.

> > + * - "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
> > + *          - "backing_file": backing file name if one is used
> > + *          - "encrypted":  1 if encrypted 0 otherwise
> > + *
> > + * Example:
> > + *
> > + * [ { "device": "ide0-hd0", "type": "hd", "removable": 0,
> > + *     "file": "/tmp/foobar", "ro": 0, "drv": "qcow2", "encrypted": 0 }
> 
> Shouldn't "file" & friends ne in nested dictionary "inserted"?

 Yes, will fix.

> > -        monitor_printf(mon, " removable=%d", bs->removable);
> > -        if (bs->removable) {
> > -            monitor_printf(mon, " locked=%d", bs->locked);
> > -        }
> > +
> > +        bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': %s, "
> > +                                    "'removable': %i, 'locked': %i }",
> > +                                    bs->device_name, type, bs->removable,
> > +                                    bs->locked);
> > +        assert(bs_obj != NULL);
> 
> Failure modes of qobject_from_jsonf()?  I'm asking because depending on
> the answer assert() may not be appropriate here.

 As far as I know it will fail on wrong syntax.