diff mbox

[6/6] qapi: add doc for QEvent

Message ID 1382321765-29052-7-git-send-email-xiawenc@linux.vnet.ibm.com
State New
Headers show

Commit Message

Wayne Xia Oct. 21, 2013, 2:16 a.m. UTC
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qapi-schema.json |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

Comments

Eric Blake Oct. 21, 2013, 9 p.m. UTC | #1
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
Wayne Xia Oct. 22, 2013, 3:19 a.m. UTC | #2
于 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.
Eric Blake Oct. 22, 2013, 3:46 a.m. UTC | #3
On 10/22/2013 04:19 AM, Wenchao Xia wrote:

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

That's a bug in the existing docs, then :)

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

Based on patch 3/6, the SHUTDOWN event is triggered if
qemu_shutdown_requested() is called, and the POWERDOWN event is issued
if qemu_shutdown_requested() is issued.  I'm fuzzy myself on the
conditions behind those two requests, maybe someone else can chime in.

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

Yes.

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

Implementation wise, we may issue some events before the action.  But
events are asynchronous by nature, we cannot guarantee that management
reads the event from QMP in any particular order, and in MOST cases,
management won't know about the event until AFTER we have done the
action, even if we queued the event for delivery on the front end of the
action.  It's better to just document ALL events as generically being
past tense.

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

Indeed, which is why separating the docs from the refactoring made sense
in your series, so that we could hammer out good docs.
Wayne Xia Oct. 22, 2013, 6:55 a.m. UTC | #4
于 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.
>
   I just found QEvent is a int(enum) so this change can't be done.
Is there a way to tell use QEvent's correspond string? Add a new
way to specify enum's correspond string automatically? for example:

{ 'type': 'EventInfo', 'data': {'name': '&QEvent'}
Wayne Xia Oct. 22, 2013, 7:33 a.m. UTC | #5
于 2013/10/22 14:55, Wenchao Xia 写道:
> 于 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.
>>
>   I just found QEvent is a int(enum) so this change can't be done.
> Is there a way to tell use QEvent's correspond string? Add a new
> way to specify enum's correspond string automatically? for example:
>
> { 'type': 'EventInfo', 'data': {'name': '&QEvent'}
>
   I just found visitor will do it for me automatically, so it is not
a problem, sorry for the disturbing messsage.
Eric Blake Oct. 22, 2013, 8:58 a.m. UTC | #6
On 10/22/2013 07:55 AM, Wenchao Xia wrote:
> 于 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.
>>
>   I just found QEvent is a int(enum) so this change can't be done.

Huh?  That's the whole point of qapi enums - they are stored as int
constants in C code, but sent as strings over QMP.

> Is there a way to tell use QEvent's correspond string? Add a new
> way to specify enum's correspond string automatically? for example:
> 
> { 'type': 'EventInfo', 'data': {'name': '&QEvent'}

No, QAPI does it for you already, no change in syntax needed.
Wayne Xia Oct. 23, 2013, 12:37 a.m. UTC | #7
>>    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.
>>
> Indeed, which is why separating the docs from the refactoring made sense
> in your series, so that we could hammer out good docs.
>
>
Hi, here is my draft for qapi-schema.json, please have a look.
Note:
1 it requires directly support of 'base', so I will sent additonal patch
support it by key word '_base' in 'data' contents.
2 some define not labeled with "since 1.8', are code move.
3 reordered.


##
# @QEvent
#
# QEMU event types
#
# Since: 1.8
##
{ 'enum': 'QEvent',
   'data': [
# system related
             'SHUTDOWN',
             'POWERDOWN',
             'RESET',
             'STOP',
             'RESUME',
             'SUSPEND',
             'SUSPEND_DISK',
             'WAKEUP',

# system virtual device related
             'RTC_CHANGE',
             'WATCHDOG',
             'DEVICE_DELETED',
             'DEVICE_TRAY_MOVED',

# block related
             'BLOCK_IO_ERROR',
             'BLOCK_IMAGE_CORRUPTED',

# block job related
             'BLOCK_JOB_COMPLETED',
             'BLOCK_JOB_CANCELLED',
             'BLOCK_JOB_ERROR',
             'BLOCK_JOB_READY',

# network related
             'NIC_RX_FILTER_CHANGED',

# vnc display related
             'VNC_CONNECTED',
             'VNC_INITIALIZED',
             'VNC_DISCONNECTED',

# spice display related
             'SPICE_CONNECTED',
             'SPICE_INITIALIZED',
             'SPICE_DISCONNECTED',
             'SPICE_MIGRATE_COMPLETED',

# guest related
             'BALLOON_CHANGE',
             'GUEST_PANICKED' ] }

##
# @EventTimestamp
#
# Reflect the time when event happens
#
# @seconds: the seconds have passed on host system
#
# @microsecnds: the microseconds have passed on host system
#
# Since: 1.8
##
{ 'type': 'EventTimestamp',
   'data': { 'seconds': 'int', 'microsecnds': 'int' } }

##
# @EventBase
#
# Base type containing properties that are available for all kind of events
#
# @event: type of the event
#
# @timestamp: the time stamp of the event, which is got from host system
#
# Since: 1.8
##
{ 'type': 'EventBase',
   'data': { 'event': 'QEvent', 'timestamp': 'EventTimestamp' } }


##
# @EventShutdown
#
# Emitted when the virtual machine shutdown, qemu terminates the 
emulation and
# exit. If the command-line option "-no-shutdown" has been specified, 
qemu will
# not exit, and a STOP event will eventually follow the SHUTDOWN event
#
# Since: 1.8
##
{ 'type': 'EventShutdown',
   'data': { } }

##
# @EventPowerdown
#
# Emitted when the virtual machine is powered down, qemu tries to set the
# system power control system, such as ACPI related virtual chips
#
# Since: 1.8
##
{ 'type': 'EventPowerdown',
   'data': { } }

##
# @EventReset
#
# Emitted when the virtual machine is reseted
#
# Since: 1.8
##
{ 'type': 'EventReset',
   'data': { } }

##
# @EventStop
#
# Emitted when the virtual machine is stopped
#
# Since: 1.8
##
{ 'type': 'EventStop',
   'data': { } }

##
# @EventResume
#
# Emitted when the virtual machine resumes execution
#
# Since: 1.8
##
{ 'type': 'EventResume',
   'data': { } }


##
# @EventSuspend
#
# Emitted when guest enters S3 state
#
# Since: 1.8
##
{ 'type': 'EventSuspend',
   'data': { } }

##
# @EventSuspendDisk
#
# Emitted when the guest makes a request to enter S4 state. Note QEMU shuts
# down (similar to @shutdown) when entering S4 state
#
# Since: 1.8
##
{ 'type': 'EventSuspendDisk',
   'data': { } }

##
# @EventWakeup
#
# Emitted when the guest has woken up from S3 and is running
#
# Since: 1.8
##
{ 'type': 'EventWakeup',
   'data': { } }


##
# @EventRtcChange
#
# Emitted when the guest changes the RTC time
#
# @offset: Offset between base RTC clock (as specified by -rtc base), and
#          new RTC clock value
#
# Since: 1.8
##
{ 'type': 'EventRtcChange',
   'data': {
       'data': {
           'offset': 'int'
       } } }

##
# @ActionTypeOnWatchdogExpired
#
# An enumeration of the actions taken when the watchdog device's timer is
# expired
#
# @reset: system resets
#
# @shutdown: system shutdown, note that it is similar to @powerdown, which
#            tries to set to system status and notify guest
#
# @poweroff: system poweroff, the emulator program exits
#
# @pause: system pauses, similar to @stop
#
# @pause: system enters debug state
#
# @none: nothing is done
#
# Since: 1.8
##
{ 'enum': 'ActionTypeOnWatchdogExpired',
   'data': [ 'reset', 'shutdown', 'poweroff', 'pause', 'debug', 'none' ] }

##
# @EventWatchdog
#
# Emitted when the watchdog device's timer is expired
#
# @action: Action that has been taken
#
# Since: 1.8
##
{ 'type': 'Watchdog',
   'data': {
       'data': {
           'action': 'ActionTypeOnWatchdogExpired'
       } } }

##
# @EventDeviceDeleted
#
# Emitted whenever the device removal completion is acknowledged by the 
guest.
# At this point, it's safe to reuse the specified device ID. Device 
removal can
# be initiated by the guest or by HMP/QMP commands.
#
# @device: #optional, device name
#
# @path: device path
#
# Since: 1.8
##
{ 'type': 'EventDeviceDeleted',
   'data': {
       'data': {
           '*device': 'str',
           'path'   : 'str'
       } } }

##
# @EventTrayMoved
#
# It's emitted whenever the tray of a removable device is moved by the guest
# or by HMP/QMP commands
#
# @device: device name
#
# @tray-open: true if the tray has been opened or false if it has been 
closed
#
# Since: 1.8
##
{ 'type': 'EventTrayMoved',
   'data': {
       'data': {
           'device'   : 'str',
           'tray-open': 'bool'
       } } }


##
# @IoOperationType
#
# An enumeration of the I/O operation types
#
# @read: read operation
#
# @write: write operation
#
# Since: 1.8
##
{ 'enum': 'IoOperationType',
   'data': [ 'read', 'write' ] }

##
# @BlockdevOnError:
#
# An enumeration of possible behaviors for errors on I/O operations.
# The exact meaning depends on whether the I/O was initiated by a guest
# or by a block job
#
# @report: for guest operations, report the error to the guest;
#          for jobs, cancel the job
#
# @ignore: ignore the error, only report a QMP event (BLOCK_IO_ERROR
#          or BLOCK_JOB_ERROR)
#
# @enospc: same as @stop on ENOSPC, same as @report otherwise.
#
# @stop: for guest operations, stop the virtual machine;
#        for jobs, pause the job
#
# Since: 1.3
##
{ 'enum': 'BlockdevOnError',
   'data': ['report', 'ignore', 'enospc', 'stop'] }

##
# @BlockErrorInfo
#
# The error info for a block error
#
# @device: device name
#
# @operation: I/O operation
#
# @action: action that has been taken
#
# Since: 1.8
##
{ 'type': 'BlockErrorInfo',
   'data': { 'device': 'str', 'operation': 'IoOperationType',
             'action': 'BlockdevOnError' } }

##
# @EventBlockIoError
#
# Emitted when a disk I/O error occurs
#
# Since: 1.8
##
{ 'type': 'EventBlockIoError',
   'data': {
       'data': {
           '_base': 'BlockErrorInfo'
       } } }

##
# @EventBlockImageCorrupted
#
# Emitted when a disk image is being marked corrupt
#
# @device: Device name
#
# @msg: Informative message, for example, reason for the corruption
#
# @offset: If the corruption resulted from an image access, this is the 
access
#          offset into the image
# @size: If the corruption resulted from an image access, this is the access
#        size
#
# Since: 1.8
##
{ 'type': 'EventBlockImageCorrupted',
   'data': {
       'data': {
           'device': 'str',
           'msg'   : 'str',
           'offset': 'int',
           'size'  : 'int'
       } } }


##
# @BlockJobType:
#
# Type of a block job.
#
# @commit: block commit job type, see "block-commit"
#
# @stream: block stream job type, see "block-stream"
#
# @mirror: drive mirror job type, see "drive-mirror"
#
# @backup: drive backup job type, see "drive-backup"
#
# Since: 1.7
##
{ 'enum': 'BlockJobType',
   'data': ['commit', 'stream', 'mirror', 'backup'] }

##
# @BlockJobInfoBase
#
# Basic info of a block job
#
# @type: Job type
#
# @device: Device name
#
# @len: Maximum progress value
#
# @offset: Current progress value. On success this is equal to len.
#          On failure this is less than len
#
# @speed: Rate limit, bytes per second
#
# Since: 1.8
##
{ 'type': 'BlockJobInfoBase',
   'data': { 'type': 'BlockJobType', 'device': 'str', 'len': 'int',
             'offset': 'int', 'speed': 'int' } }

##
# @EventBlockJobCompleted
#
# Emitted when a block job has completed
#
# @error: #optional, error message. Only present on failure. This field
#         contains a human-readable error message. There are no semantics
#         other than that streaming has failed and clients should not try to
#         interpret the error string
#
# Since: 1.8
##
{ 'type': 'EventBlockJobCompleted',
   'data': {
       'data': {
           '_base' : 'BlockJobInfoBase',
           '*error': 'str'
       } } }

##
# @EventBlockJobCancelled
#
# Emitted when a block job has been cancelled
#
# @error: #optional, error message. Only present on failure. This field
#         contains a human-readable error message. There are no semantics
#         other than that streaming has failed and clients should not try to
#         interpret the error string
#
# Since: 1.8
##
{ 'type': 'EventBlockJobCancelled',
   'data': {
       'data': {
           '_base': 'BlockJobInfoBase'
       } } }

##
# @EventBlockJobError
#
# Emitted when a block job encounters an error
#
# Since: 1.8
##
{ 'type': 'EventBlockJobError',
   'data': {
       'data': {
           '_base': 'BlockErrorInfo'
       } } }

##
# @EventBlockJobReady
#
# Emitted when a block job is ready to complete
#
# @device: device name
#
# Since: 1.8
##
{ 'type': 'EventBlockJobReady',
   'data': {
       'data': {
           'device': 'str'
       } } }

##
# @EventNicRxFilterChanged
#
# The event is emitted once until the query command is executed, the first
# event will always be emitted
#
# @name: net client name
#
# @path: device path
#
# Since: 1.8
##
{ 'type': 'EventNicRxFilterChanged',
   'data': {
       'data': {
           'name': 'str',
           'path': 'str'
       } } }


##
# @NetworkConnectionInfo
#
# The information for network connection
#
# @host: IP address
#
# @service: port number
#
# @family: address family, "ipv4" or "ipv6"
#
# Since: 1.8
##
{ 'type': 'NetworkConnectionInfo',
   'data': { 'host': 'str', 'service': 'str', 'family': 'str' } }

##
# @EventVncConnected
#
# Emitted when a VNC client establishes a connection
#
# @server: Server information
#
#   @auth: #optional, authentication method
#
# @client: Client information
#
# Since: 1.8
##
{ 'type': 'EventVncConnected',
   'data': {
       'data': {
           'server': {
               '_base': 'NetworkConnectionInfo',
               '*auth': 'str' },
           'client': 'NetworkConnectionInfo'
       } } }

##
# @EventVncInitialized
#
# Emitted after authentication takes place (if any) and the VNC session is
# made active
#
# @server: Server information
#
#   @auth: #optional, authentication method
#
# @client: Client information
#
#   @x509_dname: #optional, TLS dname
#
#   @sasl_username: #optional, SASL username
#
# Since: 1.8
##
{ 'type': 'EventVncInitialized',
   'data': {
       'data': {
           'server': {
               '_base': 'NetworkConnectionInfo',
               '*auth': 'str' },
           'client': {
               '_base'         : 'NetworkConnectionInfo',
               '*x509_dname'   : 'str',
               '*sasl_username': 'str' }
       } } }

##
# @EventVncDisconnected
#
# Emitted when the connection is closed
#
# @server: Server information
#
#   @auth: #optional, authentication method
#
# @client: Client information
#
#   @x509_dname: #optional, TLS dname
#
#   @sasl_username: #optional, SASL username
#
# Since: 1.8
##
{ 'type': 'EventVncDisconnected',
   'data': {
       'data': {
           'server': {
               '_base': 'NetworkConnectionInfo',
               '*auth': 'str' },
           'client': {
               '_base'         : 'NetworkConnectionInfo',
               '*x509_dname'   : 'str',
               '*sasl_username': 'str' }
       } } }


##
# @EventSpiceConnected
#
# Emitted when a Spice client establishes a connection
#
# @server: Server information
#
# @client: Client information
#
# Since: 1.8
##
{ 'type': 'EventSpiceConnected',
   'data': {
       'data': {
           'server': 'NetworkConnectionInfo',
           'client': 'NetworkConnectionInfo'
       } } }

##
# @EventSpiceInitialized
#
# Emitted after initial handshake and authentication takes place (if any)
# and the SPICE channel is up'n'running
#
# @server: Server information
#
#   @auth: #optional, authentication method
#
# @client: Client information
#
#   @connection-id: spice connection id. All channels with the same id
#                   belong to the same spice session
#
#   @channel-type: channel type. "1" is the main control channel, filter for
#                  this one if you want track spice sessions only
#
#   @channel-id: channel id. Usually "0", might be different needed when
#                multiple channels of the same type exist, such as multiple
#                display channels in a multihead setup
#
#   @tls: whevener the channel is encrypted
#
# Since: 1.8
##
{ 'type': 'EventSpiceInitialized',
   'data': {
       'data': {
           'server': {
               '_base': 'NetworkConnectionInfo',
               '*auth': 'str' },
           'client': {
               '_base'        : 'NetworkConnectionInfo',
               'connection-id': 'int',
               'channel-type' : 'int',
               'channel-id'   : 'int',
               'tls'          : 'bool' }
       } } }

##
# @EventSpiceDisconnected
#
# Emitted when the spice connection is closed
#
# @server: Server information
#
# @client: Client information
#
# Since: 1.8
##
{ 'type': 'EventSpiceDisconnected',
   'data': {
       'data': {
           'server': 'NetworkConnectionInfo',
           'client': 'NetworkConnectionInfo'
       } } }

##
# @EventSpiceMigrateCompleted
#
# Emitted when spice migration has completed
#
# Since: 1.8
##
{ 'type': 'EventSpiceMigrateCompleted',
   'data': { } }


##
# @EventBalloonChange
#
# Emitted when the guest changes the actual BALLOON level. This value is
# equivalent to the @actual field return by the 'query-balloon' command
#
# @actual: actual level of the guest memory balloon in bytes
#
# Since: 1.8
##
{ 'type': 'EventBalloonChange',
   'data': {
       'data': {
           'actual': 'int'
       } } }

##
# @EventGuestPanicked
#
# Emitted when guest OS panic is detected
#
# @action: Action that has been taken, currently always "pause"
#
# Since: 1.8
##
{ 'type': 'EventGuestPanicked',
   'data': {
       'data': {
           'action': 'str'
       } } }


##
# @Event
#
# Emitted when an event happens
#
# Since: 1.8
##
{ 'Union': 'Event',
   'base': 'EventBase',
   'discriminator': 'event',
   'data': {
       'SHUTDOWN'               : 'EventShutdown',
       'POWERDOWN'              : 'EventPowerdown',
       'RESET'                  : 'EventReset',
       'STOP'                   : 'EventStop',
       'RESUME'                 : 'EventResume',
       'SUSPEND'                : 'EventSuspend',
       'SUSPEND_DISK'           : 'EventSuspendDisk',
       'WAKEUP'                 : 'EventWakeup',

       'RTC_CHANGE'             : 'EventRtcChange',
       'WATCHDOG'               : 'EventWatchdog',
       'DEVICE_DELETED'         : 'EventDeviceDeleted',
       'DEVICE_TRAY_MOVED'      : 'EventDeviceTrayMoved',

       'BLOCK_IO_ERROR'         : 'EventBlockIoError',
       'BLOCK_IMAGE_CORRUPTED'  : 'EventBlockImageCorrupted',

       'BLOCK_JOB_COMPLETED'    : 'EventBlockJobCompleted',
       'BLOCK_JOB_CANCELLED'    : 'EventBlockJobCancelled',
       'BLOCK_JOB_ERROR'        : 'EventBlockJobError',
       'BLOCK_JOB_READY'        : 'EventBlockJobReady',

       'NIC_RX_FILTER_CHANGED'  : 'EventNicRxFilterChanged',

       'VNC_CONNECTED'          : 'EventVncConnected',
       'VNC_INITIALIZED'        : 'EventVncInitialized',
       'VNC_DISCONNECTED'       : 'EventVncDisconnected',

       'SPICE_CONNECTED'        : 'EventSpiceConnected',
       'SPICE_INITIALIZED'      : 'EventSpiceInitialized',
       'SPICE_DISCONNECTED'     : 'EventSpiceDisconnected',
       'SPICE_MIGRATE_COMPLETED': 'EventSpiceMigrateCompleted',

       'BALLOON_CHANGE'         : 'EventBalloonChange',
       'GUEST_PANICKED'         : 'EventGuestPanicked'
   } }
Eric Blake Oct. 29, 2013, 11:02 p.m. UTC | #8
On 10/22/2013 06:37 PM, Wenchao Xia wrote:
> Hi, here is my draft for qapi-schema.json, please have a look.
> Note:
> 1 it requires directly support of 'base', so I will sent additonal patch
> support it by key word '_base' in 'data' contents.
> 2 some define not labeled with "since 1.8', are code move.
> 3 reordered.

You may find it easier to break this into multiple patches (in
particular, code motion patches are almost always easier to review when
done separate from the patch that changes contents).

> ##
> { 'enum': 'QEvent',
>   'data': [
> # system related
>             'SHUTDOWN',
>             'POWERDOWN',
>             'RESET',
>             'STOP',
>             'RESUME',
>             'SUSPEND',
>             'SUSPEND_DISK',
>             'WAKEUP',
> 
> # system virtual device related
>             'RTC_CHANGE',

I like how you grouped events.

> 
> ##
> # @EventTimestamp
> #
> # Reflect the time when event happens
> #
> # @seconds: the seconds have passed on host system
> #
> # @microsecnds: the microseconds have passed on host system

s/microsecnds/microseconds/

> #
> # Since: 1.8
> ##
> { 'type': 'EventTimestamp',
>   'data': { 'seconds': 'int', 'microsecnds': 'int' } }

s/microsecnds/microseconds/

> 
> ##
> # @EventBase
> #
> # Base type containing properties that are available for all kind of events
> #
> # @event: type of the event
> #
> # @timestamp: the time stamp of the event, which is got from host system

Grammar is awkward, and you are missing a reference point; maybe:

@timestamp: time stamp when the event was issued by the host system, in
seconds since the Epoch

> 
> ##
> # @EventShutdown
> #
> # Emitted when the virtual machine shutdown, qemu terminates the
> emulation and

Line wrapping looks awkward here and elsewhere in your patch; be careful
that you fit in a reasonable column width.

s/terminates/terminated/

> # exit. If the command-line option "-no-shutdown" has been specified,

s/exit/is about to exit/

> qemu will
> # not exit, and a STOP event will eventually follow the SHUTDOWN event
> #
> # Since: 1.8
> ##
> { 'type': 'EventShutdown',
>   'data': { } }
> 
> ##
> # @EventPowerdown
> #
> # Emitted when the virtual machine is powered down, qemu tries to set the
> # system power control system, such as ACPI related virtual chips

Hmm, this one is undocumented in qmp-events.txt.  Maybe:

Emitted when the virtual machine is powered down through the system
power control system, such as via ACPI.

Do we have a better idea of how EventShutdown and EventPowerdown differ?

> #
> # Since: 1.8
> ##
> { 'type': 'EventPowerdown',
>   'data': { } }
> 
> ##
> # @EventReset
> #
> # Emitted when the virtual machine is reseted

s/reseted/reset/

> ##
> # @EventSuspend
> #
> # Emitted when guest enters S3 state

Is S3 an x86-specific term, or does it apply to other architectures?  I
know this is what qmp-events.txt says, but maybe it would be better as
"Emitted when guest enters a hardware suspension state (such as S3)".

> #
> # Since: 1.8
> ##
> { 'type': 'EventSuspend',
>   'data': { } }
> 
> ##
> # @EventSuspendDisk
> #
> # Emitted when the guest makes a request to enter S4 state. Note QEMU shuts
> # down (similar to @shutdown) when entering S4 state

Again, is S4 an x86-specific term?  The reference to '@shutdown' looks
awkward; should it just call out 'EventShutdown' instead?

> #
> # Since: 1.8
> ##
> { 'type': 'EventSuspendDisk',
>   'data': { } }
> 
> ##
> # @EventWakeup
> #
> # Emitted when the guest has woken up from S3 and is running

Again a question on S3 wording.

> 
> ##
> # @ActionTypeOnWatchdogExpired

That's a mouthful.  How about:

WatchdogExpirationAction

> #
> # An enumeration of the actions taken when the watchdog device's timer is
> # expired
> #
> # @reset: system resets
> #
> # @shutdown: system shutdown, note that it is similar to @powerdown, which
> #            tries to set to system status and notify guest
> #
> # @poweroff: system poweroff, the emulator program exits
> #
> # @pause: system pauses, similar to @stop
> #
> # @pause: system enters debug state

s/pause/debug/

> ##
> # @EventTrayMoved
> #
> # It's emitted whenever the tray of a removable device is moved by the

s/It's emitted/Emitted/

> 
> ##
> # @BlockdevOnError:

Again, separate code motion from new content.  qapi-schema.json does not
have to be in topological order (ie. we already allow for recursive
types, so there's no need to worry whether all intermediate types are
declared prior to the outer type that uses them).

> ##
> { 'type': 'BlockErrorInfo',
>   'data': { 'device': 'str', 'operation': 'IoOperationType',
>             'action': 'BlockdevOnError' } }
> 
> ##
> # @EventBlockIoError
> #
> # Emitted when a disk I/O error occurs
> #
> # Since: 1.8
> ##
> { 'type': 'EventBlockIoError',
>   'data': {
>       'data': {
>           '_base': 'BlockErrorInfo'

Huh?  I think you mean:

{ 'type': 'EventBlockIoError',
  'data': {
    'data': 'BlockErrorInfo'
  } }

> ##
> # @EventBlockImageCorrupted
> #
> # Emitted when a disk image is being marked corrupt
> #
> # @device: Device name
> #
> # @msg: Informative message, for example, reason for the corruption
> #
> # @offset: If the corruption resulted from an image access, this is the
> access
> #          offset into the image
> # @size: If the corruption resulted from an image access, this is the
> access
> #        size
> #
> # Since: 1.8
> ##
> { 'type': 'EventBlockImageCorrupted',
>   'data': {
>       'data': {
>           'device': 'str',
>           'msg'   : 'str',
>           'offset': 'int',
>           'size'  : 'int'

Double-check if 'offset' and 'size' are always present, or if they
should be written '*offset' and '*size'.

> ##
> # @BlockJobInfoBase
> #
> # Basic info of a block job
> #
> # @type: Job type
> #
> # @device: Device name
> #
> # @len: Maximum progress value
> #
> # @offset: Current progress value. On success this is equal to len.
> #          On failure this is less than len
> #
> # @speed: Rate limit, bytes per second
> #
> # Since: 1.8
> ##
> { 'type': 'BlockJobInfoBase',
>   'data': { 'type': 'BlockJobType', 'device': 'str', 'len': 'int',
>             'offset': 'int', 'speed': 'int' } }

I would fold '*error':'str' into this type, rename BlockJobInfoBase to
BlockJobEventInfo (so it is not confused with the existing BlockJobInfo,
and because it is not a base type), then...

> 
> ##
> # @EventBlockJobCompleted
> #
> # Emitted when a block job has completed
> #
> # @error: #optional, error message. Only present on failure. This field
> #         contains a human-readable error message. There are no semantics
> #         other than that streaming has failed and clients should not
> try to
> #         interpret the error string
> #
> # Since: 1.8
> ##
> { 'type': 'EventBlockJobCompleted',
>   'data': {
>       'data': {
>           '_base' : 'BlockJobInfoBase',
>           '*error': 'str'
>       } } }

...fix this to avoid a '_base' by just using:

{ 'type': 'EventBlockJobCompleted',
  'data': {
    'data': 'BlockJobEventInfo'
  } }
...

> 
> ##
> # @EventBlockJobCancelled
> #
> # Emitted when a block job has been cancelled
> #
> # @error: #optional, error message. Only present on failure. This field
> #         contains a human-readable error message. There are no semantics
> #         other than that streaming has failed and clients should not
> try to
> #         interpret the error string
> #
> # Since: 1.8
> ##
> { 'type': 'EventBlockJobCancelled',
>   'data': {
>       'data': {
>           '_base': 'BlockJobInfoBase'
>       } } }

...and do the same here (maybe with a doc that the optional '*error'
will never be populated):

{ 'type': 'EventBlockJobCancelled',
  'data': {
    'data': 'BlockJobEventInfo'
  } }

> 
> ##
> # @EventBlockJobError
> #
> # Emitted when a block job encounters an error
> #
> # Since: 1.8
> ##
> { 'type': 'EventBlockJobError',
>   'data': {
>       'data': {
>           '_base': 'BlockErrorInfo'
>       } } }

Again, don't use '_base', but just properly inline the
'data':'BlockErrorInfo'.

> 
> ##
> # @EventNicRxFilterChanged
> #
> # The event is emitted once until the query command is executed, the first

s/query/'query-rx-filter'/

> # event will always be emitted
> #

> ##
> # @NetworkConnectionInfo
> #
> # The information for network connection
> #
> # @host: IP address
> #
> # @service: port number
> #
> # @family: address family, "ipv4" or "ipv6"

Seems like a candidate for an enum rather than an open-coded string.

> ##
> { 'type': 'EventVncConnected',
>   'data': {
>       'data': {
>           'server': {
>               '_base': 'NetworkConnectionInfo',
>               '*auth': 'str' },

Doesn't work like that.  I think you want:

{ 'type': 'NetworkConnectionInfoServer',
  'base': 'NetworkConnectionInfo',
  'data': { '*auth': 'str'} }

followed by:

{ 'type': 'EventVncConnected',
  'data': {
    'data': {
      'server': 'NetworkConnectionInfoServer',
      'client': 'NetworkConnectionInfo'
    } } }

> ##
> # @EventVncInitialized
> #
> # Emitted after authentication takes place (if any) and the VNC session is
> # made active
> #
> # @server: Server information
> #
> #   @auth: #optional, authentication method
> #
> # @client: Client information
> #
> #   @x509_dname: #optional, TLS dname
> #
> #   @sasl_username: #optional, SASL username
> #
> # Since: 1.8
> ##
> { 'type': 'EventVncInitialized',
>   'data': {
>       'data': {
>           'server': {
>               '_base': 'NetworkConnectionInfo',
>               '*auth': 'str' },
>           'client': {
>               '_base'         : 'NetworkConnectionInfo',
>               '*x509_dname'   : 'str',
>               '*sasl_username': 'str' }

Again, _base doesn't work.  You'll need to create a struct containing
the optional members (but can be a child class of
NetworkConnectionInfo), then use direct reference to that struct, as in
'client':'NetworkConnectionInfoXYZ'.

> ##
> # @EventSpiceInitialized
> #
> # Emitted after initial handshake and authentication takes place (if any)
> # and the SPICE channel is up'n'running

s/up'n'running/up and running/

> ##
> { 'type': 'EventSpiceInitialized',
>   'data': {
>       'data': {
>           'server': {
>               '_base': 'NetworkConnectionInfo',
>               '*auth': 'str' },
>           'client': {
>               '_base'        : 'NetworkConnectionInfo',
>               'connection-id': 'int',
>               'channel-type' : 'int',
>               'channel-id'   : 'int',
>               'tls'          : 'bool' }

More uses of '_base' that instead need to be actual child structs
deriving from the common base.

> 
> ##
> # @EventGuestPanicked
> #
> # Emitted when guest OS panic is detected
> #
> # @action: Action that has been taken, currently always "pause"

Sounds like it should be an enum rather than a 'str'

> ##
> # @Event
> #
> # Emitted when an event happens
> #
> # Since: 1.8
> ##
> { 'Union': 'Event',

s/Union/union/

>   'base': 'EventBase',
>   'discriminator': 'event',
>   'data': {
>       'SHUTDOWN'               : 'EventShutdown',
>       'POWERDOWN'              : 'EventPowerdown',
...
>       'BALLOON_CHANGE'         : 'EventBalloonChange',
>       'GUEST_PANICKED'         : 'EventGuestPanicked'
>   } }

Looks like you're headed in the right direction.
Wayne Xia Oct. 30, 2013, 7:51 a.m. UTC | #9
于 2013/10/30 7:02, Eric Blake 写道:
> On 10/22/2013 06:37 PM, Wenchao Xia wrote:
>> Hi, here is my draft for qapi-schema.json, please have a look.
>> Note:
>> 1 it requires directly support of 'base', so I will sent additonal patch
>> support it by key word '_base' in 'data' contents.
>> 2 some define not labeled with "since 1.8', are code move.
>> 3 reordered.
> You may find it easier to break this into multiple patches (in
> particular, code motion patches are almost always easier to review when
> done separate from the patch that changes contents).
>
>> ##
>> { 'enum': 'QEvent',
>>    'data': [
>> # system related
>>              'SHUTDOWN',
>>              'POWERDOWN',
>>              'RESET',
>>              'STOP',
>>              'RESUME',
>>              'SUSPEND',
>>              'SUSPEND_DISK',
>>              'WAKEUP',
>>
>> # system virtual device related
>>              'RTC_CHANGE',
> I like how you grouped events.
>
>> ##
>> # @EventTimestamp
>> #
>> # Reflect the time when event happens
>> #
>> # @seconds: the seconds have passed on host system
>> #
>> # @microsecnds: the microseconds have passed on host system
> s/microsecnds/microseconds/
   Will fix.

>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventTimestamp',
>>    'data': { 'seconds': 'int', 'microsecnds': 'int' } }
> s/microsecnds/microseconds/
>
>> ##
>> # @EventBase
>> #
>> # Base type containing properties that are available for all kind of events
>> #
>> # @event: type of the event
>> #
>> # @timestamp: the time stamp of the event, which is got from host system
> Grammar is awkward, and you are missing a reference point; maybe:
>
> @timestamp: time stamp when the event was issued by the host system, in
> seconds since the Epoch
   Will doc as above.

>> ##
>> # @EventShutdown
>> #
>> # Emitted when the virtual machine shutdown, qemu terminates the
>> emulation and
> Line wrapping looks awkward here and elsewhere in your patch; be careful
> that you fit in a reasonable column width.
>
> s/terminates/terminated/
   OK.

>> # exit. If the command-line option "-no-shutdown" has been specified,
> s/exit/is about to exit/
   OK.

>> qemu will
>> # not exit, and a STOP event will eventually follow the SHUTDOWN event
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventShutdown',
>>    'data': { } }
>>
>> ##
>> # @EventPowerdown
>> #
>> # Emitted when the virtual machine is powered down, qemu tries to set the
>> # system power control system, such as ACPI related virtual chips
> Hmm, this one is undocumented in qmp-events.txt.  Maybe:
>
> Emitted when the virtual machine is powered down through the system
> power control system, such as via ACPI.
   Will use above.

> Do we have a better idea of how EventShutdown and EventPowerdown differ?
>
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventPowerdown',
>>    'data': { } }
>>
>> ##
>> # @EventReset
>> #
>> # Emitted when the virtual machine is reseted
> s/reseted/reset/
   OK.

>> ##
>> # @EventSuspend
>> #
>> # Emitted when guest enters S3 state
> Is S3 an x86-specific term, or does it apply to other architectures?  I
> know this is what qmp-events.txt says, but maybe it would be better as
> "Emitted when guest enters a hardware suspension state (such as S3)".
   I am not sure if other arch have S3 term, will use as:

"Emitted when guest enters a hardware suspension state (such as S3 on x86)"


>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventSuspend',
>>    'data': { } }
>>
>> ##
>> # @EventSuspendDisk
>> #
>> # Emitted when the guest makes a request to enter S4 state. Note QEMU shuts
>> # down (similar to @shutdown) when entering S4 state
> Again, is S4 an x86-specific term?  The reference to '@shutdown' looks
> awkward; should it just call out 'EventShutdown' instead?
   yes, it should be 'EventShutdown'

>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventSuspendDisk',
>>    'data': { } }
>>
>> ##
>> # @EventWakeup
>> #
>> # Emitted when the guest has woken up from S3 and is running
> Again a question on S3 wording.
   will reword:

'Emitted when the guest has woken up from suspension state'



>> ##
>> # @ActionTypeOnWatchdogExpired
> That's a mouthful.  How about:
>
> WatchdogExpirationAction
   OK.

>> #
>> # An enumeration of the actions taken when the watchdog device's timer is
>> # expired
>> #
>> # @reset: system resets
>> #
>> # @shutdown: system shutdown, note that it is similar to @powerdown, which
>> #            tries to set to system status and notify guest
>> #
>> # @poweroff: system poweroff, the emulator program exits
>> #
>> # @pause: system pauses, similar to @stop
>> #
>> # @pause: system enters debug state
> s/pause/debug/
>
>> ##
>> # @EventTrayMoved
>> #
>> # It's emitted whenever the tray of a removable device is moved by the
> s/It's emitted/Emitted/
   Will fix.

>> ##
>> # @BlockdevOnError:
> Again, separate code motion from new content.  qapi-schema.json does not
> have to be in topological order (ie. we already allow for recursive
> types, so there's no need to worry whether all intermediate types are
> declared prior to the outer type that uses them).
   Nice to hear no topological limit present, but put the define ahead
my make it easier to review? will use seprate patch to do it.

>> ##
>> { 'type': 'BlockErrorInfo',
>>    'data': { 'device': 'str', 'operation': 'IoOperationType',
>>              'action': 'BlockdevOnError' } }
>>
>> ##
>> # @EventBlockIoError
>> #
>> # Emitted when a disk I/O error occurs
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventBlockIoError',
>>    'data': {
>>        'data': {
>>            '_base': 'BlockErrorInfo'
> Huh?  I think you mean:
>
> { 'type': 'EventBlockIoError',
>    'data': {
>      'data': 'BlockErrorInfo'
>    } }
   Yes, I misse it.

>> ##
>> # @EventBlockImageCorrupted
>> #
>> # Emitted when a disk image is being marked corrupt
>> #
>> # @device: Device name
>> #
>> # @msg: Informative message, for example, reason for the corruption
>> #
>> # @offset: If the corruption resulted from an image access, this is the
>> access
>> #          offset into the image
>> # @size: If the corruption resulted from an image access, this is the
>> access
>> #        size
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventBlockImageCorrupted',
>>    'data': {
>>        'data': {
>>            'device': 'str',
>>            'msg'   : 'str',
>>            'offset': 'int',
>>            'size'  : 'int'
> Double-check if 'offset' and 'size' are always present, or if they
> should be written '*offset' and '*size'.
   The original doc says always present, But I think it is resonable to 
change
it as optional, do you agree?

>> ##
>> # @BlockJobInfoBase
>> #
>> # Basic info of a block job
>> #
>> # @type: Job type
>> #
>> # @device: Device name
>> #
>> # @len: Maximum progress value
>> #
>> # @offset: Current progress value. On success this is equal to len.
>> #          On failure this is less than len
>> #
>> # @speed: Rate limit, bytes per second
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'BlockJobInfoBase',
>>    'data': { 'type': 'BlockJobType', 'device': 'str', 'len': 'int',
>>              'offset': 'int', 'speed': 'int' } }
> I would fold '*error':'str' into this type, rename BlockJobInfoBase to
> BlockJobEventInfo (so it is not confused with the existing BlockJobInfo,
> and because it is not a base type), then...
>> ##
>> # @EventBlockJobCompleted
>> #
>> # Emitted when a block job has completed
>> #
>> # @error: #optional, error message. Only present on failure. This field
>> #         contains a human-readable error message. There are no semantics
>> #         other than that streaming has failed and clients should not
>> try to
>> #         interpret the error string
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventBlockJobCompleted',
>>    'data': {
>>        'data': {
>>            '_base' : 'BlockJobInfoBase',
>>            '*error': 'str'
>>        } } }
> ...fix this to avoid a '_base' by just using:
>
> { 'type': 'EventBlockJobCompleted',
>    'data': {
>      'data': 'BlockJobEventInfo'
>    } }
> ...
   OK.

>> ##
>> # @EventBlockJobCancelled
>> #
>> # Emitted when a block job has been cancelled
>> #
>> # @error: #optional, error message. Only present on failure. This field
>> #         contains a human-readable error message. There are no semantics
>> #         other than that streaming has failed and clients should not
>> try to
>> #         interpret the error string
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventBlockJobCancelled',
>>    'data': {
>>        'data': {
>>            '_base': 'BlockJobInfoBase'
>>        } } }
> ...and do the same here (maybe with a doc that the optional '*error'
> will never be populated):
>
> { 'type': 'EventBlockJobCancelled',
>    'data': {
>      'data': 'BlockJobEventInfo'
>    } }
   OK.

>> ##
>> # @EventBlockJobError
>> #
>> # Emitted when a block job encounters an error
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventBlockJobError',
>>    'data': {
>>        'data': {
>>            '_base': 'BlockErrorInfo'
>>        } } }
> Again, don't use '_base', but just properly inline the
> 'data':'BlockErrorInfo'.
   OK.

>> ##
>> # @EventNicRxFilterChanged
>> #
>> # The event is emitted once until the query command is executed, the first
> s/query/'query-rx-filter'/
   will fix.

>> # event will always be emitted
>> #
>> ##
>> # @NetworkConnectionInfo
>> #
>> # The information for network connection
>> #
>> # @host: IP address
>> #
>> # @service: port number
>> #
>> # @family: address family, "ipv4" or "ipv6"
> Seems like a candidate for an enum rather than an open-coded string.
   OK, will use enum.

>> ##
>> { 'type': 'EventVncConnected',
>>    'data': {
>>        'data': {
>>            'server': {
>>                '_base': 'NetworkConnectionInfo',
>>                '*auth': 'str' },
> Doesn't work like that.  I think you want:
>
> { 'type': 'NetworkConnectionInfoServer',
>    'base': 'NetworkConnectionInfo',
>    'data': { '*auth': 'str'} }
>
> followed by:
>
> { 'type': 'EventVncConnected',
>    'data': {
>      'data': {
>        'server': 'NetworkConnectionInfoServer',
>        'client': 'NetworkConnectionInfo'
>      } } }
>
>> ##
>> # @EventVncInitialized
>> #
>> # Emitted after authentication takes place (if any) and the VNC session is
>> # made active
>> #
>> # @server: Server information
>> #
>> #   @auth: #optional, authentication method
>> #
>> # @client: Client information
>> #
>> #   @x509_dname: #optional, TLS dname
>> #
>> #   @sasl_username: #optional, SASL username
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventVncInitialized',
>>    'data': {
>>        'data': {
>>            'server': {
>>                '_base': 'NetworkConnectionInfo',
>>                '*auth': 'str' },
>>            'client': {
>>                '_base'         : 'NetworkConnectionInfo',
>>                '*x509_dname'   : 'str',
>>                '*sasl_username': 'str' }
> Again, _base doesn't work.  You'll need to create a struct containing
> the optional members (but can be a child class of
> NetworkConnectionInfo), then use direct reference to that struct, as in
> 'client':'NetworkConnectionInfoXYZ'.
   We will have a lot of struct defines, not sure if it is good:

NetworkConnectionInfoVNCConnectedServer
NetworkConnectionInfoVNCInitializedServer
NetworkConnectionInfoVNCInitializedClient
NetworkConnectionInfoSPICEConnectedServer
......

   Instead of above, I modified qapi script a bit to recognize keyword 
"_base",
which directly inherit data field, just as "base".

>> ##
>> # @EventSpiceInitialized
>> #
>> # Emitted after initial handshake and authentication takes place (if any)
>> # and the SPICE channel is up'n'running
> s/up'n'running/up and running/

   OK.

>> ##
>> { 'type': 'EventSpiceInitialized',
>>    'data': {
>>        'data': {
>>            'server': {
>>                '_base': 'NetworkConnectionInfo',
>>                '*auth': 'str' },
>>            'client': {
>>                '_base'        : 'NetworkConnectionInfo',
>>                'connection-id': 'int',
>>                'channel-type' : 'int',
>>                'channel-id'   : 'int',
>>                'tls'          : 'bool' }
> More uses of '_base' that instead need to be actual child structs
> deriving from the common base.
>
>> ##
>> # @EventGuestPanicked
>> #
>> # Emitted when guest OS panic is detected
>> #
>> # @action: Action that has been taken, currently always "pause"
> Sounds like it should be an enum rather than a 'str'
   OK.

>> ##
>> # @Event
>> #
>> # Emitted when an event happens
>> #
>> # Since: 1.8
>> ##
>> { 'Union': 'Event',
> s/Union/union/
   typo mistake.

>>    'base': 'EventBase',
>>    'discriminator': 'event',
>>    'data': {
>>        'SHUTDOWN'               : 'EventShutdown',
>>        'POWERDOWN'              : 'EventPowerdown',
> ...
>>        'BALLOON_CHANGE'         : 'EventBalloonChange',
>>        'GUEST_PANICKED'         : 'EventGuestPanicked'
>>    } }
> Looks like you're headed in the right direction.
>
diff mbox

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index fbc1fab..0f966ab 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -33,6 +33,62 @@ 
 #
 # QEMU event types
 #
+# @SHUTDOWN: system shutdown
+#
+# @RESET: system resets
+#
+# @POWERDOWN: system power down, if it is suppoted
+#
+# @STOP: stops the emulation
+#
+# @RESUME: resumes the emulation, typically after system stop
+#
+# @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
+#
+# @RTC_CHANGE: rtc changes
+#
+# @WATCHDOG: watch dog performs a action
+#
+# @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
+#
+# @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
+#
+# @SUSPEND_DISK: system suspends to disk, typically request comes from guest
+#
+# @WAKEUP: system wakes up from suspend
+#
+# @BALLOON_CHANGE: system resource balloon status changes
+#
+# @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',