[Qemu-devel] Re: [PATCH v2 12/15] monitor: Add basic device state visualization

[Avi Kivity <avi@redhat.com>]

On 05/23/2010 10:57 AM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 05/22/2010 11:18 AM, Jan Kiszka wrote:
>>      
>>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>>
>>> This introduces device_show, a monitor command that saves the vmstate of
>>> a qdev device and visualizes it. QMP is also supported. Buffers are cut
>>> after 16 byte by default, but the full content can be requested via
>>> '-f'. To pretty-print sub-arrays, vmstate is extended to store the start
>>> index name. A new qerror is introduced to signal a missing vmstate. And
>>> it comes with documentation.
>>>
>>> +
>>> +Dump a snapshot of the device state. Buffers are cut after 16 bytes
>>> unless
>>> +a full dump is requested.
>>> +
>>> +Arguments:
>>> +
>>> +- "path": the device's qtree path or unique ID (json-string)
>>>
>>>        
>> This may be ambiguous.
>>      
> Can your elaborate what precisely is ambiguous?
>    

Can't the user choose the unique ID so that it aliases an unrelated 
qtree path?

I prefer having mutually exclusive 'path' and 'ref' arguments.

>>> +- "full": report full state (json-bool, optional)
>>>
>>>        
>> Is this needed for QMP?  The client can always truncate it to any length.
>>      
> The effect may not be needed for QMP, but I do need this channel from
> the command line to the monitor pretty-printer. I could just stick
> "full": json-bool back into the return dict, but that would look somehow
> strange IMO.
>    

So we could disallow it as a QMP input, but allow it as an HMP input.

>>> +
>>> +Schema of returned object:
>>> +
>>> +{ "device": json-string, "id": json-string, "fields" : [
>>> field-objects ] }
>>> +
>>> +The field object array may be empty, otherwise it consists of
>>> +
>>> +{ "name": json-string, "size": json-int, "elems": [ element-objects ] }
>>> +
>>> +"size" describes the real number of bytes required for a binary
>>> representation
>>> +of a single field element in the array. The actually transfered
>>> amount may be
>>> +smaller unless a full dump was requested.
>>>
>>>        
>> This converts the entire qdev tree into an undocumented stable protocol
>> (the qdev paths were already in this state I believe).  This really
>> worries me.
>>      
> Being primarily a debugging tool, device_show exports the entire
> (qdev'ified) vmstates via QMP. Unlike the migration protocol, it does
> not provide something like backward compatibility.

Should be explicitly documented.  All QMP commands should be backwards 
and forwards compatible unless noted.

> This would be
> overkill for the intended purpose (though someone may find a different
> use case one day).
>    

Even for simply showing things, a GUI may depend on the presence of 
certain fields.  If we document that the fields may change, a correctly 
written GUI can fall back to a simpler display.

> I think we have the following options:
>   - disable device_show via QMP, limit it to the monitor console
>   - declare its output inherently unstable, maybe at least adding the
>     vmstate version to each device so that potential QMP consumers notice
>     that they may have to update their tools or switch to a different
>     processing function
>
> Given that vmstate annotations will most probably require some work on
> the output structure (and I don't have a QMP use case ATM anyway), I
> would be fine with the first option for now. Still, I don't think we
> will ever get beyond the second option because this service is tight to
> some internals of QEMU we don't want to freeze.
>    

I agree.  This feature is very useful as a debugging aid, and as I don't 
think we'll have debugging GUIs any time soon, it's better to defer the 
problem until we really need to solve it.

-- 
error compiling committee.c: too many arguments to function