qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH V5 05/28] qapi: define events in qapi schema
Date: Wed, 07 May 2014 20:48:24 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0

于 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 <address@hidden>
---

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?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]