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

[Wenchao Xia]

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