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

[Avi Kivity <avi@redhat.com>]

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.

> +- "full": report full state (json-bool, optional)
>    

Is this needed for QMP?  The client can always truncate it to any length.

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

> +
> +The element object array may be empty, otherwise it can contain
> +
> +- json-int objects
> +- QMP buffer objects
> +- field objects
> +- arrays of json-ints, QMP buffers, or field objects
> +
> +Example:
> +
> +->  { "execute": "device_show", "arguments": { "path": "isa.0/i8042" } }
> +<- { "return": { "device": "i8042", "id": "", "fields":
> +                 [ { "name": "kbd", "size": 4, "elems":
> +                     [ { "name": "write_cmd", "size": 1, "elems": [0] },
> +                       { "name": "status", "size": 1, "elems": [25] },
> +                       { "name": "mode", "size": 1, "elems": [3] },
> +                       { "name": "pending", "size": 1, "elems": [1] }
> +                     ] }
> +                 ]
> +               }
> +   }
> +
> +EQMP
>    

Looks good.  I am only worried about long term stability and documentation.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.