Re: [Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Eric Blake <eblake@redhat.com>]

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

On 10/21/2013 03:16 AM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  qapi-schema.json |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)

Incomplete.  Now that you are actually using the enum (see the spot I
pointed out in 5/6), you ALSO need to change:

-{ 'type': 'EventInfo', 'data': {'name': 'str'} }
+{ 'type': 'EventInfo', 'data': {'name': 'QEvent'} }

and make use of the enum in the QAPI documentation.

>  #
> +# @SHUTDOWN: system shutdown
> +#
> +# @RESET: system resets

s/resets/has reset/

> +#
> +# @POWERDOWN: system power down, if it is suppoted

s/suppoted/supported/

Events aren't issued if they aren't supported, so that phrase is pointless.

> +#
> +# @STOP: stops the emulation
> +#

Your use of present tense makes it sounds like this is a causal command
("issuing STOP will stop the emulation"), but you really want it to
sound like a notification of an effect ("STOP is issued after emulation
is stopped).  That is:

s/stops the emulation/emulation stopped/

> +# @RESUME: resumes the emulation, typically after system stop

Again, tense matters; I suggest:

@RESUME: emulation resumed

> +#
> +# @VNC_CONNECTED: a vnc client has connected to system
> +#
> +# @VNC_INITIALIZED: system has initialized for a vnc client
> +#
> +# @VNC_DISCONNECTED: a vnc client has disconnected from system
> +#
> +# @BLOCK_IO_ERROR: block layer meets I/O error

s/meets/encountered an/

> +#
> +# @RTC_CHANGE: rtc changes

s/changes/changed/

> +#
> +# @WATCHDOG: watch dog performs a action

I suggest:

@WATCHDOG: watchdog expired

> +#
> +# @SPICE_CONNECTED: a spice client has connected to system
> +#
> +# @SPICE_INITIALIZED: system has initialized for a spice client
> +#
> +# @SPICE_DISCONNECTED: a spice client has disconnected from system
> +#
> +# @BLOCK_JOB_COMPLETED: a block job has been completed
> +#
> +# @BLOCK_JOB_CANCELLED: a block job has been cancelled
> +#
> +# @BLOCK_JOB_ERROR: a block job meets error

s/meets/encountered an/

> +#
> +# @BLOCK_JOB_READY: a block job is ready
> +#
> +# @DEVICE_DELETED: a device has been deleted
> +#
> +# @DEVICE_TRAY_MOVED: a device tray's status has changed
> +#
> +# @NIC_RX_FILTER_CHANGED: the filter for receiving on a nic has been changed
> +#
> +# @SUSPEND: system suspends, typically request comes from guest

No need to say the typical cause for an event here; I'd much rather see
us give that extra detail in the place where we further describe each
event (more on that later).  I suggest:

@SUSPEND: system has suspended to memory (S3 power state)

> +#
> +# @SUSPEND_DISK: system suspends to disk, typically request comes from guest

I suggest:

@SUSPEND_DISK: system has suspended to disk (S4 power state)

> +#
> +# @WAKEUP: system wakes up from suspend

s/wakes/woke/

> +#
> +# @BALLOON_CHANGE: system resource balloon status changes

s/changes/changed/

> +#
> +# @SPICE_MIGRATE_COMPLETED: spice migration has been completed
> +#
> +# @GUEST_PANICKED: guest has panicked
> +#
> +# @BLOCK_IMAGE_CORRUPTED: block image has been corrupted
> +#
>  # Since: 1.8
>  ##
>  { 'enum': 'QEvent',
> 

Good start, but this series needs more.  Ultimately, I'd like to get rid
of docs/qmp/qmp-events.txt, and inline that content into
qapi-schema.json.  We already documented how to do it:

https://lists.gnu.org/archive/html/qemu-devel/2013-09/msg02164.html

-- 
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: 621 bytes --]