From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52672) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wi1HH-0000Q7-E5 for qemu-devel@nongnu.org; Wed, 07 May 2014 08:49:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wi1H8-0008Dk-5n for qemu-devel@nongnu.org; Wed, 07 May 2014 08:48:51 -0400 Received: from mail-pa0-x22e.google.com ([2607:f8b0:400e:c03::22e]:35759) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wi1H7-0008DS-QX for qemu-devel@nongnu.org; Wed, 07 May 2014 08:48:42 -0400 Received: by mail-pa0-f46.google.com with SMTP id kx10so1131945pab.19 for ; Wed, 07 May 2014 05:48:40 -0700 (PDT) Message-ID: <536A2B98.90702@gmail.com> Date: Wed, 07 May 2014 20:48:24 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1398918422-3019-1-git-send-email-wenchaoqemu@gmail.com> <1398918422-3019-6-git-send-email-wenchaoqemu@gmail.com> <53626181.2000505@redhat.com> In-Reply-To: <53626181.2000505@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V5 05/28] qapi: define events in qapi schema List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: mdroth@linux.vnet.ibm.com, armbru@redhat.com, lcapitulino@redhat.com 于 2014/5/1 23:00, Eric Blake 写道: > On 04/30/2014 10:26 PM, Wenchao Xia wrote: >> Some old type defines for spice and vnc are changed to let new >> event defines use them instead of redefine. Note that define of >> BlockErrorAction is moved from block.h to qapi schema, and it is >> not merged with BlockdevOnError. In schema NIC_RX_FILTER_CHANGED's >> param 'name' is changed as optional one, since in caller it is >> optional. >> >> Signed-off-by: Wenchao Xia >> --- > > This is a big patch. See my comments on 7/28; do you have to do ALL of > the qapi conversion here, or can you split it so that you are adding > qapi one event at a time, in the same patch as that event also uses the > generated code? > > Is the code motion of BlockErrorAction something that can be split into > its own patch, to make the review focus easier? (Code motion and > renaming fallout being separated from new additions is always easier > than having both in one commit) > OK, adjust it in next version. >> +++ b/docs/qmp/qmp-events.txt >> @@ -1,39 +1,14 @@ >> - QEMU Machine Protocol Events >> - ============================ >> + QEMU Machine Protocol Events Examples >> + ===================================== >> >> BALLOON_CHANGE >> -------------- >> - >> -Emitted when the guest changes the actual BALLOON level. This >> -value is equivalent to the 'actual' field return by the >> -'query-balloon' command >> - >> -Data: >> - >> -- "actual": actual level of the guest memory balloon in bytes (json-number) >> - >> -Example: >> - >> { "event": "BALLOON_CHANGE", >> "data": { "actual": 944766976 }, >> "timestamp": { "seconds": 1267020223, "microseconds": 435656 } } > > I'm wondering if we still need this file, or if (by the end of the > conversion to qapi) we can just drop it. Showing only an example usage, > when the qapi already documents everything, isn't adding much value from > my perspective. On the other hand, if you are able to rebase this patch > to do one event at a time, then keep this file around until the end of > the series. Then, for each event converted, you remove one chunk of > this file, add one chunk to the schema.json file, and update all places > to generate that new event, all in a single commit, where it becomes > much easier to track that the conversion for that event was correct > (here, there are so many events converted from .txt to .json at once > that it is harder to correlate that the conversion of each event was > correct). > > >> +# @VncBasicInfo >> +# >> +# The basic information for vnc network connection >> # >> -# @host: The host name of the client. QEMU tries to resolve this to a DNS name >> -# when possible. >> +# @host: IP address >> # >> -# @family: 'ipv6' if the client is connected via IPv6 and TCP >> -# 'ipv4' if the client is connected via IPv4 and TCP >> -# 'unix' if the client is connected via a unix domain socket >> -# 'unknown' otherwise >> +# @service: port number >> # >> -# @service: The service name of the client's port. This may depends on the >> -# host system's service database so symbolic names should not be >> -# relied on. > > Why are you reducing the information about @service? At least you got > rid of the typo (s/depends/depend/). > It is VncBasicInfo so it may indicate the server's port. Maybe: # @service: The service name of vnc port. This may depends on the # host system's service database so symbolic names should not be # relied on. > >> +## >> +# @VncClientInfo: >> +# >> +# Information about a connected VNC client. >> # >> # @x509_dname: #optional If x509 authentication is in use, the Distinguished >> # Name of the client. >> @@ -1180,8 +1217,8 @@ >> # Since: 0.14.0 >> ## >> { 'type': 'VncClientInfo', >> - 'data': {'host': 'str', 'family': 'str', 'service': 'str', >> - '*x509_dname': 'str', '*sasl_username': 'str'} } >> + 'base': 'VncBasicInfo', >> + 'data': { '*x509_dname' : 'str', '*sasl_username': 'str' } } > > All this work on refactoring the existing vnc QMP types should be its > own patch, prior to any patch that introduces an event that also uses > the shared types created by your refactoring. > refactor it later. >> + >> +## >> +# Event defines >> +## > > If Lluís' series goes in first, it might make sense to have all of the > event descriptions in their own file which gets included from the main > qemu-schema.json. > How about define them in qemu-events.json?