Re: [Qemu-devel] [PATCH V6 17/29] qapi event: convert WATCHDOG

[Eric Blake <eblake@redhat.com>]

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

On 06/05/2014 06:22 AM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
> ---
>  docs/qmp/qmp-events.txt |   19 -------------------
>  hw/watchdog/watchdog.c  |   23 +++++++----------------
>  monitor.c               |    2 +-
>  qapi-event.json         |   15 +++++++++++++++
>  qapi-schema.json        |   24 ++++++++++++++++++++++++
>  5 files changed, 47 insertions(+), 36 deletions(-)
> 

> @@ -117,31 +108,31 @@ void watchdog_perform_action(void)
>  {
>      switch(watchdog_action) {

Worth fixing the missing space after switch while touching this area of
code?

>      case WDT_RESET:             /* same as 'system_reset' in monitor */
> -        watchdog_mon_event("reset");
> +        qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_RESET, NULL);

More instances of ignoring the errp argument, where eliminating it might
be nicer.

> +##
> +# @WATCHDOG
> +#
> +# Emitted when the watchdog device's timer is expired
> +#
> +# @action: action that has been taken
> +#
> +# Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
> +# followed respectively by the RESET, SHUTDOWN, or STOP events
> +#
> +# Since: 2.1

0.14.0

> +  'data': { 'action': 'WatchdogExpirationAction' } }

Hmm.  You've managed to create error.json in such a manner that it is
not self-sufficient.  If some other file includes error.json, it must
also define WatchdogExpirationAction or it will fail the generators.

> +++ b/qapi-schema.json

> +##
> +# @WatchdogExpirationAction

I think you will be better off to ensure that error.json is
self-sufficient, perhaps by sticking any data type it references
directly into common.json rather than qapi-schema.json, and having
error.json include common.json.  (This is the first instance of
referencing an external type, but other events later in the series have
the same issue).

> +# Since: 2.1
> +##
> +{ 'enum': 'WatchdogExpirationAction',
> +  'data': [ 'reset', 'shutdown', 'poweroff', 'pause', 'debug', 'none' ] }

Nice conversion of open-coded string to an enum.  While I've been asking
for the earliest QMP version for Since fields on event objects proper,
here, I think you're okay keeping the 'since 2.1' indication.  Why?
Because we already have other examples in the code base of converting
open-coded strings to an enum, where the QMP wire format is the same,
but where the version claimed on those enums was the qemu version that
did the conversion rather than the age of the command being converted.
(Maybe we could audit all of those conversions and retroactively update
their Since field, or even come up with a notation for wire-stability
release vs. QAPI introspection release - but that sounds like more pain
than necessary with no obvious benefit)

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