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

[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/

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

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

signature.asc
Description: OpenPGP digital signature