| Message ID | 1401970944-18735-18-git-send-email-wenchaoqemu@gmail.com |
|---|---|
| State | New |
| Headers | show |
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)
On 06/13/2014 03:47 PM, Eric Blake wrote: > 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(-) >> > >> + '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. s/error.json/qapi-event.json/ > >> +++ b/qapi-schema.json > >> +## >> +# @WatchdogExpirationAction > > I think you will be better off to ensure that error.json is and again qapi-event.json (not sure why I typed error.json). > 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). Oh weird! I've managed to run all four of scripts/qapi-{visit,types,commands,event}.py directly on qapi-event.json, and didn't get any complaints from the generator. BUT, the generated code is definitely different: -void qapi_event_send_watchdog(WatchdogExpirationAction action, +void qapi_event_send_watchdog(WatchdogExpirationAction * action, Error **errp) That is, when the enum type is known (because the parse was done on qapi-schema.json), the WatchdogExpirationAction argument is treated as an integer enum value; but when the enum type is unknown (because the parse was done directly on an incomplete qapi-event.json), the generator tries to treat it as a pointer to an otherwise unknown structure. (Never mind the odd formatting of the space after the '*' - I believe this pending patch fixes it: https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg02385.html). This little exercise raises red flags for me - we probably ought to enhance the code generators to error out instead of blindly act as if unknown types are pointers. But save it for another day - no need to stall this series just to wait for a robustness improvement to the generators. Meanwhile, my suggestion of making qapi-event.json to be self-sufficient is going to be a bit harder to test, but is still probably worth trying (just moving common types into a common shared include).
δΊ 2014/6/14 6:05, Eric Blake ει: > On 06/13/2014 03:47 PM, Eric Blake wrote: >> 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(-) >>> > >> >>> + '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. > > s/error.json/qapi-event.json/ > >> >>> +++ b/qapi-schema.json >> >>> +## >>> +# @WatchdogExpirationAction >> >> I think you will be better off to ensure that error.json is > > and again qapi-event.json (not sure why I typed error.json). > >> 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). > > Oh weird! I've managed to run all four of > scripts/qapi-{visit,types,commands,event}.py directly on > qapi-event.json, and didn't get any complaints from the generator. BUT, > the generated code is definitely different: > > -void qapi_event_send_watchdog(WatchdogExpirationAction action, > +void qapi_event_send_watchdog(WatchdogExpirationAction * action, > Error **errp) > > That is, when the enum type is known (because the parse was done on > qapi-schema.json), the WatchdogExpirationAction argument is treated as > an integer enum value; but when the enum type is unknown (because the > parse was done directly on an incomplete qapi-event.json), the generator > tries to treat it as a pointer to an otherwise unknown structure. > (Never mind the odd formatting of the space after the '*' - I believe > this pending patch fixes it: > https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg02385.html). > > This little exercise raises red flags for me - we probably ought to > enhance the code generators to error out instead of blindly act as if > unknown types are pointers. But save it for another day - no need to > stall this series just to wait for a robustness improvement to the > generators. > > Meanwhile, my suggestion of making qapi-event.json to be self-sufficient > is going to be a bit harder to test, but is still probably worth trying > (just moving common types into a common shared include). > I think it is a issue about how to orgnize the .json files. There are some common type defines needed for different .json files, in my series they are needed both in qapi-schema.json and qapi-event.json. So to make qapi-event.json self-sufficient, qapi-schema.json will be insufficient. I considered this before, and thought it is better to reorgnize .json files as: qapi-types.json | | qapi-cmd.json qapi-event.json It is an adjusting work for existing code, So I didn't do that in my series.
Il 13/06/2014 23:47, Eric Blake ha scritto: >> +## >> > +# @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). > Can be done later though, I think? Paolo
On 06/17/2014 03:23 AM, Paolo Bonzini wrote: > Il 13/06/2014 23:47, Eric Blake ha scritto: >>> +## >>> > +# @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). >> > > Can be done later though, I think? Yes; as I mentioned in my other mail: https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03208.html This little exercise raises red flags for me - we probably ought to enhance the code generators to error out instead of blindly act as if unknown types are pointers. But save it for another day - no need to stall this series just to wait for a robustness improvement to the generators.
diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt index 8cad3e7..df15dc8 100644 --- a/docs/qmp/qmp-events.txt +++ b/docs/qmp/qmp-events.txt @@ -415,22 +415,3 @@ Example: "client": { "family": "ipv4", "service": "46089", "host": "127.0.0.1", "sasl_username": "luiz" } }, "timestamp": { "seconds": 1263475302, "microseconds": 150772 } } - -WATCHDOG --------- - -Emitted when the watchdog device's timer is expired. - -Data: - -- "action": Action that has been taken, it's one of the following (json-string): - "reset", "shutdown", "poweroff", "pause", "debug", or "none" - -Example: - -{ "event": "WATCHDOG", - "data": { "action": "reset" }, - "timestamp": { "seconds": 1267061043, "microseconds": 959568 } } - -Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is -followed respectively by the RESET, SHUTDOWN, or STOP events. diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c index f28161b..9284d3f 100644 --- a/hw/watchdog/watchdog.c +++ b/hw/watchdog/watchdog.c @@ -24,9 +24,9 @@ #include "qemu/config-file.h" #include "qemu/queue.h" #include "qapi/qmp/types.h" -#include "monitor/monitor.h" #include "sysemu/sysemu.h" #include "sysemu/watchdog.h" +#include "qapi-event.h" /* Possible values for action parameter. */ #define WDT_RESET 1 /* Hard reset. */ @@ -101,15 +101,6 @@ int select_watchdog_action(const char *p) return 0; } -static void watchdog_mon_event(const char *action) -{ - QObject *data; - - data = qobject_from_jsonf("{ 'action': %s }", action); - monitor_protocol_event(QEVENT_WATCHDOG, data); - qobject_decref(data); -} - /* This actually performs the "action" once a watchdog has expired, * ie. reboot, shutdown, exit, etc. */ @@ -117,31 +108,31 @@ void watchdog_perform_action(void) { switch(watchdog_action) { case WDT_RESET: /* same as 'system_reset' in monitor */ - watchdog_mon_event("reset"); + qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_RESET, NULL); qemu_system_reset_request(); break; case WDT_SHUTDOWN: /* same as 'system_powerdown' in monitor */ - watchdog_mon_event("shutdown"); + qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_SHUTDOWN, NULL); qemu_system_powerdown_request(); break; case WDT_POWEROFF: /* same as 'quit' command in monitor */ - watchdog_mon_event("poweroff"); + qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_POWEROFF, NULL); exit(0); case WDT_PAUSE: /* same as 'stop' command in monitor */ - watchdog_mon_event("pause"); + qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_PAUSE, NULL); vm_stop(RUN_STATE_WATCHDOG); break; case WDT_DEBUG: - watchdog_mon_event("debug"); + qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_DEBUG, NULL); fprintf(stderr, "watchdog: timer fired\n"); break; case WDT_NONE: - watchdog_mon_event("none"); + qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_NONE, NULL); break; } } diff --git a/monitor.c b/monitor.c index e6d32c2..cae60ab 100644 --- a/monitor.c +++ b/monitor.c @@ -614,6 +614,7 @@ static void monitor_qapi_event_init(void) { /* Limit RTC & BALLOON events to 1 per second */ monitor_qapi_event_throttle(QAPI_EVENT_RTC_CHANGE, 1000); + monitor_qapi_event_throttle(QAPI_EVENT_WATCHDOG, 1000); qmp_event_set_func_emit(monitor_qapi_event_queue); } @@ -741,7 +742,6 @@ static void monitor_protocol_event_init(void) { /* Limit RTC & BALLOON events to 1 per second */ monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000); - monitor_protocol_event_throttle(QEVENT_WATCHDOG, 1000); /* limit the rate of quorum events to avoid hammering the management */ monitor_protocol_event_throttle(QEVENT_QUORUM_REPORT_BAD, 1000); monitor_protocol_event_throttle(QEVENT_QUORUM_FAILURE, 1000); diff --git a/qapi-event.json b/qapi-event.json index dc20bb4..640f841 100644 --- a/qapi-event.json +++ b/qapi-event.json @@ -91,3 +91,18 @@ ## { 'event': 'RTC_CHANGE', 'data': { 'offset': 'int' } } + +## +# @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 +## +{ 'event': 'WATCHDOG', + 'data': { 'action': 'WatchdogExpirationAction' } } diff --git a/qapi-schema.json b/qapi-schema.json index d04514a..a14504d 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -4799,4 +4799,28 @@ { 'enum': 'BlockErrorAction', 'data': [ 'ignore', 'report', 'stop' ] } +## +# @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 +# +# @debug: system enters debug state +# +# @none: nothing is done +# +# Since: 2.1 +## +{ 'enum': 'WatchdogExpirationAction', + 'data': [ 'reset', 'shutdown', 'poweroff', 'pause', 'debug', 'none' ] } + { 'include': 'qapi-event.json' }
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(-)