Re: [Qemu-devel] RFC: Monitor high-level design

[Anthony Liguori <anthony@codemonkey.ws>]

On 09/22/2010 10:03 AM, Luiz Capitulino wrote:
> On Wed, 22 Sep 2010 08:18:10 -0500
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 09/22/2010 07:32 AM, Markus Armbruster wrote:
>>      
> [...]
>
>    
>>> I can't see why we must have a dogmatic "thou shalt not use anything but
>>> QMP to implement HMP commands, ever" rule.
>>>        
> I based most of my ideas on what Anthony has described at:
>
>     http://wiki.qemu.org/Features/QMP_0.14
>
> But that's more like a start point, of course that we shouldn't be dogmatic,
> specially when we have better ideas.
>
>    
>> FWIW, print/sum can be easily implemented in terms of QMP commands.
>>      
> Yes, but I think the point is when/if we should do it.
>
>    
>> It's not really necessary for every possibly HMP command to be
>> implemented in terms of QMP.  However, unless you can do everything in
>> QMP that you can do in HMP, then people will be inclined to use HMP as a
>> management interface.
>>      
> s/use HMP/use QMP
>
>    
>> So, the only real rule we should try to adopt is that QMP is at least as
>> powerful as HMP.  That should be our goal.
>>      
> Agreed.
>
>    
>>>>          - HMP can't be moved outside of QEMU, if we want that we'd have to
>>>>            write a new Monitor client (potentially in a different language,
>>>>            which is actually good)
>>>>
>>>>          
>>> I'd rather not worry about that now.
>>>
>>>
>>>        
>>>>          - Not sure if HMP features like command completion will perfectly fit
>>>>
>>>>          
>>> Maybe I'm naive, but I figure that as long as the human monitor controls
>>> its character device and knows enough about its own commands, it can do
>>> completion pretty much the same way it does now.
>>>        
> Yes, that's right.
>
>    
>>> Let's have a closer look at the human monitor:
>>>
>>>       +---------------+   reads/writes
>>>       | human monitor |<--------------->    character device
>>>       +---------------+                          ^
>>>               |                                  |
>>>               | calls                            |
>>>               |                                  |
>>>               v                                  |
>>>       +---------------+        writes            |
>>>       | hmon handlers | -------------------------+
>>>       +---------------+
>>>
>>> The "human monitor" box actually has identifiable sub-boxes:
>>>
>>> * Command table: human monitor command description
>>>
>>> * Several argument types: methods to parse, check, complete, ...
>>>
>>> * Reader: read a line of input from character device
>>>
>>>     - Readline, uses table for completion
>>>
>>> * Parser: parse a line of input, uses table to parse arguments
>>>
>>> * Executor: call the appropriate handler, uses table
>>>
>>> The machine monitor (short: qmon) is pretty similar, actually.
>>>        
> That's the 'is pretty similar' part I was referring to when I said (by IRC
> I guess) that qmon and hmon can share the device handling code.
>
>    
>>> Differences:
>>>
>>> * We read and parse JSON objects, not lines.
>>>
>>> * We don't bother to set up completion.
>>>
>>> * The qmon handlers don't write to the character device.  They return
>>>     objects instead, to be written by the qmon executor.
>>>
>>> Remember that we want qmon handlers to use proper internal interfaces to
>>> do the real work.  In other words, qmon handlers should be fairly
>>> trivial.
>>>
>>> I can think of three ways to write a human monitor handler:
>>>
>>> 1. Call a qmon handler, print the object it returns, suitably formatted.
>>>
>>> 2. Call the internal interfaces, just like qmon handlers do.  Print the
>>>      results.
>>>
>>>      This cut out the qmon handler middle-man.  Since the qmon handler is
>>>      supposed to be trivial, this isn't a big deal.
>>>
>>> 3. Just do it.  I think that's fine for some commands that make sense
>>>      only human monitor, like print.  Also fine for legacy commands
>>>      scheduled for removal; we got more useful things to do than
>>>      beautifying those.  The ones we want to keep we should convert to
>>>      1. or 2.  Doing that early may be advisable to get the internal
>>>      interfaces used, but it's not like "need to convert everything before
>>>      anything works".
>>>
>>> You see hmon doesn't use all of qmon, only qmon handlers, and even that
>>> only with 1.  Can't see a layering issue there.
>>>        
> Yes and note that we have 1 and 3 today already.
>
>    
>> Yeah, I think what you describe makes perfect sense.  As long as QMP is
>> a thin wrapper around an API, and HMP also calls the API, then it's
>> pretty clear that QMP is at least as powerful as HMP.
>>      
> Ok, let me get into two details:
>
>   1. The BufferCharDriverState is only going to be used for human monitor
>      passthrough, right?
>    

Well, I think the pointer Markus raised is that there are two ways to 
implement HMP in terms of QMP.  One of them is to do:

static void do_commit(Monitor *mon, const char *device)
{
      QObject *res;

      res = qmp_command("commit", "{'device': %s}", device);
      if (qobject_is_qerror(res)) {
           monitor_printf(mon, "commit: %s\n", 
qerror_to_str(qobject_to_qerror(res)));
           return;
      }
}

And the second way is:

static void do_commit(Monitor *mon, const char *device)
{
     Error *err = NULL;

     qpi_commit(device, &err);
     if (err) {
          monitor_printf(mon, "commit: %s\n", error_to_str(err);
          error_free(err);
     }
}

Which takes advantage of the notion that all QMP commands are expressed 
as proper C interfaces internally.  Under the covers, qpi_commit() could 
be a direct API call or it could even be a QMP client RPC interface.  As 
long as qpi_commit(...) maps directly to qmp_command("commit", ...) it's 
all equivalent.

There's definitely an elegance to the second approach.

Regards,

Anthony Liguori

>   2. As Markus described above, QMP and HMP perform similar internal steps,
>      so what about having them behind a common interface, like this:
>
>      struct monitor_ops {
>           const char *name;
>           int (*init)(void *opaque);
>           void (*exit)(void *opaque);
>           int (*can_read)(void *opaque);
>           void (*read)(void *opaque, const uint8_t *buf, int size);
>           void (*event)(void *opaque, int event);
>           void (*async_message)(void *opaque, MonitorEvent, QObject *data);
>      };
>
>      Then monitor.c code would register itself with the chardev layer, calling
>      each monitor's monitor_ops.
>
>      Note that we already have a feature request to be able to create new monitors
>      on demand. I believe such design will help.
>
>      Is this overkill? Does anyone have better ideas on how both modules (QMP/HMP)
>      should be designed/separated?
>
>      Sketches are welcome.
>