Re: [Qemu-devel] [PATCH V5 05/28] qapi: define events in qapi schema

[Wenchao Xia <wenchaoqemu@gmail.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 <wenchaoqemu@gmail.com>
>> ---
>
> 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?