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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4329 bytes --]

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)

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


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

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]