[Qemu-devel] Re: [RfC PATCH] spice: qmp windup: connection events & info command.

[Luiz Capitulino]

On Thu, 25 Nov 2010 12:42:52 +0100
Gerd Hoffmann <address@hidden> wrote:

>    Hi,
> 
> > The first thing we have to check with Daniel is whether or not we're
> > providing the info they would need/expect,
> 
> Daniel?
> 
> > apart from that I have the
> > following general comments:
> >
> >   1. It's missing documentation in QMP/qmp-events.txt and qmp-commands.hx
> >      (yeah, docs are far from code, hope to fix soon)
> 
> Ok.
> 
> >   2. Can you please split this in two patches? One adding the events and
> >      the other adding the query command
> 
> Doesn't make that much sense IMHO as the both provide quite simliar 
> informations (i.e. "info spice" gives you a list of connections with 
> pretty much the same info provided by the events).

Still, they're different interfaces, also it's going to be a little bit harder
to review now that you're going to introduce the docs in the same patch.

> >> +/*
> >> + * generic print handler for hmp 'info $what'
> >> + * simply pretty-print the josn representation
> >> + */
> >> +#if defined(CONFIG_SPICE) /* because 'info spice' is the only user */
> >> +static void do_info_generic_print(Monitor *mon, const QObject *data)
> >> +{
> >> +    QString *json = qobject_to_json_pretty(data);
> >> +    monitor_printf(mon, "%s\n", qstring_get_str(json));
> >> +    QDECREF(json);
> >> +}
> >> +#endif
> >
> > We definitely need a generic print handler, but I don't that stringifying
> > JSON makes a minimal good user interface.
> 
> Well, this is the pretty json version which prints stuff multi-line and 
> with intention.  Certainly not perfect but reasonable readable with 
> minimum effort.  We can replace it with something else when it shows up.

We have refused introducing json as part of our interface with the user
in the past, and it was a good decision: json doesn't make a good user
interface.

We have two options here:

 1. Write a default (user friendly) printer function. Doesn't have to be
    fancy, it could just print lists as CSV sequences and dicts like:

       key1: value
       key2: value

    Of course it has to handle nested objects.

 2. Write a regular, specific print function, as all info commands available
    under QMP do

> 
> >> +static QList *channel_list_get(void)
> >> +{
> >> +    ChannelList *item;
> >> +    QList *list;
> >> +    QDict *dict;
> >> +
> >> +    list = qlist_new();
> >> +    QTAILQ_FOREACH(item,&channel_list, link) {
> >> +        dict = qdict_new();
> >> +        add_addr_info(dict,&item->info->paddr, item->info->plen);
> >> +        add_channel_info(dict, item->info);
> >> +        qlist_append_obj(list, QOBJECT(dict));
> >
> > You can use qlist_append() and drop the QOBJECT() usage.
> >
> 
> Ok.
> 
> cheers,
>    Gerd
>