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

[Gerd Hoffmann <kraxel@redhat.com>]

   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).

>> +/*
>> + * 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.

>> +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