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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013/10/22 5:00, Eric Blake 写道:
> 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.
>
  Will add this part, thanks for tipping it.


>>   #
>> +# @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.
   Ok, I will skip that phrase. The point here is that, many people are 
confused
about shutdown and powerdown, and it seems POWERDOWN item is not present
in doc/qmp/qapi-events.txt? I want to add doc tips the difference:
How about: It will set the system power control unit to notify guest, 
such as
ACPI chips.(This is where I am not sure, the qemu online doc says, 
shutdown is
gracefully....).

>> +#
>> +# @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/
   Do you mean all tense in the doc should use past tense?
I hesitated before about tense usage, it seems not all event
is emitted after it happens, for example, powerdown emitted before
it call notifier to set the states.
   Take another think, I think I may use past tense through the doc,
but with more carefully meaning, such as:
the system has enter powerdown state.
   If you agree with the tense, I'd like sent the reformed doc
in the following, before respin.

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

>> +#
>> +# @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
   I'll take a look to find a good way inline those content.