From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MLhxq-0002nP-Sx for qemu-devel@nongnu.org; Tue, 30 Jun 2009 14:21:54 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MLhxm-0002iV-7S for qemu-devel@nongnu.org; Tue, 30 Jun 2009 14:21:54 -0400 Received: from [199.232.76.173] (port=49202 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MLhxm-0002iI-25 for qemu-devel@nongnu.org; Tue, 30 Jun 2009 14:21:50 -0400 Received: from mail-ew0-f211.google.com ([209.85.219.211]:52040) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MLhxl-0004GV-If for qemu-devel@nongnu.org; Tue, 30 Jun 2009 14:21:49 -0400 Received: by ewy7 with SMTP id 7so404310ewy.34 for ; Tue, 30 Jun 2009 11:21:48 -0700 (PDT) Message-ID: <4A4A57B6.1090702@codemonkey.ws> Date: Tue, 30 Jun 2009 13:21:42 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 01/11] QMP: Introduce specification file References: <4A412339.5000109@redhat.com> <4A42200C.6060600@codemonkey.ws> <5b31733c0906240857g546316e0pd92fee9afe6115fa@mail.gmail.com> <4A4252DD.70300@redhat.com> <20090624190539.GR14121@shareable.org> <5b31733c0906241224j50baa7e6lc80b8c79c5d6baa7@mail.gmail.com> <20090624211358.GA14121@shareable.org> <4A43768A.2090604@eu.citrix.com> <4A438FDD.5060206@redhat.com> <4A43935D.6000506@codemonkey.ws> <4A4395B8.4010401@redhat.com> <4A43BD5D.80307@codemonkey.ws> <4A43C264.6060803@redhat.com> <4A43D600.8060605@codemonkey.ws> <4A449113.8070907@redhat.com> <4A44CB74.1070808@codemonkey.ws> <4A44E2F3.8050804@codemonkey.ws> <4A476C60.1080609@redhat.com> <4A47A70B.7070806@codemonkey.ws> <4A47A9B4.4050600@redhat.com> <4A480E0F.6030000@codemonkey.ws> <4A485971.1010000@redhat.com> <4A4922AC.4030707@codemonkey.ws> <4A49A482.30908@redhat.com> <4A4A1390.2050602@codemonkey.ws> <4A4A18B4.2070004@redhat.com> <4A4A198A.3050505@codemonkey.ws> <20090630110332.39436ed7@doriath> <4A4A37E6.9090705@redhat.com> In-Reply-To: <4A4A37E6.9090705@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: "ehabkost@redhat.com" , Stefano Stabellini , "jan.kiszka@siemens.com" , "dlaor@redhat.com" , "qemu-devel@nongnu.org" , Luiz Capitulino , Filip Navara , Vincent Hanquez Avi Kivity wrote: > On 06/30/2009 05:03 PM, Luiz Capitulino wrote: >>> Great. So Luiz, do you understand and agree with the proposed changes >>> to your series? >>> >> >> Yes, only the lists and dictionaries types are not very clear >> to me. >> >> Should command handlers return data in those types to the >> monitor and thus get printed by it? >> >> Detailed is welcome, anyway. >> > > I would recommend proceeding as follows: > > 1. Add QValue, QArray, QDictionary > > QValue is a polymorphic object that can be a number, a string, an > array, or a dictionary. It needs a bunch of accessors like > qvalue_to_qarray() to check if a value is an array, and things like > qarray_get() or qdict_put() to manipulate them. For the simpler data > types, qvalue_to_int64() and qvalue_from_int64() should suffice. And add a return value to the monitor functions. I think this is a good first step. Passing a QArray to each monitor function is one way to go. Another way to go is to use something like libffi to do proper dynamic dispatch. The later approach has the benefit of requiring less code churn and reusing the static type checking. > 2. Split the human monitor command implementations into protocol > adapters and implementation. > > The protocol adapter reads the command, parses it according to > qemu-monitor.hx, and constructs a QArray of the parameter list and > passes it down to the command implementation. The command > implementation returns a QValue, which the protocol adapter formats to > the human monitor response format. One place to start would be to convert something like: monitor_printf(mon, "file: f=%s,b=%s,c=%d", ...) to: value = qlist_create(qstring_create("file"), qdict_create("f", qstring_create(..), "b", qstring_create(..), "c", qstring_create(..), NULL)); But since this is a very regular pattern, you could also do: value = qmonlist_create("file", "f", QVALUE_STRING, ..., "b", QVALUE_STRING, ..., "c", QVALUE_INT, ..., NULL); You could potentially introduce some serious ninja action by doing something like: value = qvalue_create("[%s, {'f': %s, 'b': %s', 'c': %d}]", ...); What's cool about this is that you can mark qvalue_create() as having printf formatting so GCC will help you with type checking. Regards, Anthony Liguori