From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=34652 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1OFtrB-0004rQ-KL
	for qemu-devel@nongnu.org; Sat, 22 May 2010 14:55:34 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <avi@redhat.com>) id 1OFtr9-0008RC-JM
	for qemu-devel@nongnu.org; Sat, 22 May 2010 14:55:33 -0400
Received: from mx1.redhat.com ([209.132.183.28]:8670)
	by eggs.gnu.org with esmtp (Exim 4.69)
	(envelope-from <avi@redhat.com>) id 1OFtr9-0008R2-C4
	for qemu-devel@nongnu.org; Sat, 22 May 2010 14:55:31 -0400
Message-ID: <4BF82895.6000706@redhat.com>
Date: Sat, 22 May 2010 21:55:17 +0300
From: Avi Kivity <avi@redhat.com>
MIME-Version: 1.0
References: <cover.1274516288.git.jan.kiszka@web.de>
	<1f557b9feb1965a61e64f7166bcf4918bed8d0ec.1274516288.git.jan.kiszka@web.de>
In-Reply-To: <1f557b9feb1965a61e64f7166bcf4918bed8d0ec.1274516288.git.jan.kiszka@web.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Subject: [Qemu-devel] Re: [PATCH v2 12/15] monitor: Add basic device state
	visualization
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Jan Kiszka <jan.kiszka@web.de>
Cc: Anthony Liguori <aliguori@us.ibm.com>, Juan Quintela <quintela@redhat.com>, Jan Kiszka <jan.kiszka@siemens.com>, Markus Armbruster <armbru@redhat.com>, qemu-devel@nongnu.org, Luiz Capitulino <lcapitulino@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.