Re: [Qemu-devel] [PATCH 01/11] QMP: Introduce specification file

[Anthony Liguori]

Avi Kivity wrote:
> On 06/30/2009 05:03 PM, Luiz Capitulino wrote:
> > > Great.  So Luiz, do you understand and agree with the proposed changes
> > > to your series?
> > >      
> >
> >   Yes, only the lists and dictionaries types are not very clear
> > to me.
> >
> >   Should command handlers return data in those types to the
> > monitor and thus get printed by it?
> >
> >   Detailed is welcome, anyway.
> >    
>
> I would recommend proceeding as follows:
>
> 1. Add QValue, QArray, QDictionary
>
> QValue is a polymorphic object that can be a number, a string, an 
> array, or a dictionary.  It needs a bunch of accessors like 
> qvalue_to_qarray() to check if a value is an array, and things like 
> qarray_get() or qdict_put() to manipulate them.  For the simpler data 
> types, qvalue_to_int64() and qvalue_from_int64() should suffice.

And add a return value to the monitor functions.  I think this is a good 
first step.

Passing a QArray to each monitor function is one way to go.  Another way 
to go is to use something like libffi to do proper dynamic dispatch.  
The later approach has the benefit of requiring less code churn and 
reusing the static type checking.

> 2. Split the human monitor command implementations into protocol 
> adapters and implementation.
>
> The protocol adapter reads the command, parses it according to 
> qemu-monitor.hx, and constructs a QArray of the parameter list and 
> passes it down to the command implementation.  The command 
> implementation returns a QValue, which the protocol adapter formats to 
> the human monitor response format.

One place to start would be to convert something like:

monitor_printf(mon, "file: f=%s,b=%s,c=%d", ...)

to:

value = qlist_create(qstring_create("file"), qdict_create("f", 
qstring_create(..), "b", qstring_create(..), "c", qstring_create(..), 
NULL));

But since this is a very regular pattern, you could also do:

value = qmonlist_create("file",
                                     "f", QVALUE_STRING, ...,
                                     "b", QVALUE_STRING, ...,
                                     "c", QVALUE_INT, ...,
                                     NULL);

You could potentially introduce some serious ninja action by doing 
something like:

value = qvalue_create("[%s, {'f': %s, 'b': %s', 'c': %d}]", ...);

What's cool about this is that you can mark qvalue_create() as having 
printf formatting so GCC will help you with type checking.

Regards,

Anthony Liguori