[Qemu-devel] Re: [RFC] New API for asynchronous monitor commands

[Luiz Capitulino]

On Mon, 25 Jan 2010 08:15:53 -0600
Anthony Liguori <address@hidden> wrote:

> On 01/25/2010 07:08 AM, Luiz Capitulino wrote:
> > On Fri, 22 Jan 2010 13:03:51 -0600
> > Adam Litke<address@hidden>  wrote:
> >
> >   I like the patch, but I don't think it's a good idea to use this in
> > synchronous commands as they will have to call QMPCompletion (not to
> > mention unneeded suspend/resume calls).
> >    
> 
> I think the value of having a single mechanism is that it gives us a 
> common code path that's exercised for every command.  That helps avoid 
> bugs in the long term.

 I do agree with this, I just think that the calling of the completion
callback should be done in common code, instead of duplicating its call
among all synchronous clients. Also, we should not suspend/resume if
there's no need for that.

 At least for now async handlers are the exception, making their handling
the rule doesn't seem right to me.

 However, I believe we can check if the handler is synchronous and
do all this job for it, can't we?

> I'd like to see the monitor move from a static table of commands to 
> dynamic command registration.  IOW, we'd have something like:
> 
> monitor_register_cmd(name, args_type, help, params, my_qmp_command, 
> my_opaque);
> 
> Given this API, it's pretty easy to write a wrapper that takes a simple 
> synchronous callback without making such a concept core to the monitor 
> infrastructure.

 Why do would we need this for the short-term?

 Before getting more complex, I'd like to see the Monitor code really
refectored and this can't be done today w/o breaking mine (and probably
Markus) queues, as we're still working on getting QMP ready for clients.

> Notice that there's no user_print here.  I also would like to see us 
> decouple QMP from the human monitor such that the human monitor was 
> implemented purely in terms of QMP commands.  The core monitor 
> infrastructure should really only deal with QMP and the human monitor 
> should be an independent client.
> 
> Not all QMP concepts make sense for the human monitor and vice versa.  
> The reason to start QMP as a mirror of the human monitor is to ensure 
> management apps can have an easy transition.  However, once we're there, 
> we should not continue duplicating QMP commands in the human monitor.

 Sure thing! We are moving in this direction already and IMHO it would be
desirable to see that happen first, so that we don't add more code today
to be refectored later.