Re: [Qemu-devel] [PATCH v2] watchdog: Allow setting action on the fly
Michal Privoznik writes: > On 09/05/2017 10:36 PM, Eric Blake wrote: >> On 09/05/2017 08:22 AM, Michal Privoznik wrote: >>> Currently, the only time that users can set watchdog action is at >>> the start as all we expose is this -watchdog-action command line >>> argument. This is suboptimal when users want to plug the device >>> later via monitor. Alternatively, they might want to change the >>> action for already existing device on the fly. >>> >>> At the same time, drop local redefinition of the actions eum in >> >> s/eum/enum/ >> >>> watchdog.h in favour of the ones defined in schema. >>> >>> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169 >>> >>> Signed-off-by: Michal Privoznik [...] >>> +++ b/qapi-schema.json >>> @@ -6539,3 +6539,12 @@ >>> # Since 2.9 >>> ## >>> { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } >>> + >>> +## >>> +# @watchdog-set-action: >>> +# >>> +# Set watchdog action >>> +# >>> +# Since 2.11 >>> +## >>> +{ 'command': 'watchdog-set-action', 'data' : {'action': >>> 'WatchdogExpirationAction'} } >> >> Can a machine have more than one watchdog device, in which case you'd >> want a device name to select which watchdog gets which action? But the >> command-line version that you are replacing seems to be global, so I >> guess you're okay with this interface. >> > > Yeah, I don't think that a guest can have more than one watchdog: > > /qemu.git $ ./x86_64-softmmu/qemu-system-x86_64 \ > -watchdog ib700 -watchdog i6300esb > qemu-system-x86_64: -watchdog i6300esb: only one watchdog option may be > given -watchdog is a thin wrapper around -device, and all it adds is this error. It's quite redundant. I recommend using -device instead. "-device i6300esb -device i6300esb" works. It's just a PCI device after all. > Also, would it make sense? I mean, what would be the benefit of having > two or more watchdogs? None. All it would accomplish is complicating things.
Re: [Qemu-devel] [PATCH v2] watchdog: Allow setting action on the fly
Michal Privoznik writes: > On 09/06/2017 07:59 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> On 09/05/2017 08:22 AM, Michal Privoznik wrote: Currently, the only time that users can set watchdog action is at the start as all we expose is this -watchdog-action command line argument. This is suboptimal when users want to plug the device later via monitor. Alternatively, they might want to change the action for already existing device on the fly. At the same time, drop local redefinition of the actions eum in >>> >>> s/eum/enum/ >>> watchdog.h in favour of the ones defined in schema. Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169 Signed-off-by: Michal Privoznik --- >>> @@ -77,27 +80,17 @@ int select_watchdog(const char *p) int select_watchdog_action(const char *p) { >>> +int action; +action = qapi_enum_parse(WatchdogExpirationAction_lookup, p, + WATCHDOG_EXPIRATION_ACTION__MAX, -1, NULL); >>> >>> Semantic conflict; you need to rebase now that the qapi enum lookup code >>> has landed (see commit ebf677c8 and parents) >>> + +case WATCHDOG_EXPIRATION_ACTION__MAX: +/* keep gcc happy */ +break; >>> >>> Could use g_assert_not_reached() here instead. >> >> Catches one out of approximately 2^64 invalid values. To catches all, >> and in fewer words: >> >> default: >> assert(0); > > I'm not a fan of this, but I'm not a qemu developer so I don't know what > your coding style is (if any for this case). However, since this switch > is over an enum, compiler will scream if a new value is introduced to > the enum and not handled in the switch. IMO it's useful because when I'm > adding new value I can immediately see what other places I need to consider. Two separate purposes: catch invalid state, catch forgotten code update. I can't see how to serve both. Your patch picks the latter. > Also, yeah, I am going to rename the enum in v3. Thanks!
Re: [Qemu-devel] [PATCH v2] watchdog: Allow setting action on the fly
On 09/06/2017 07:59 AM, Markus Armbruster wrote: > Eric Blake writes: > >> On 09/05/2017 08:22 AM, Michal Privoznik wrote: >>> Currently, the only time that users can set watchdog action is at >>> the start as all we expose is this -watchdog-action command line >>> argument. This is suboptimal when users want to plug the device >>> later via monitor. Alternatively, they might want to change the >>> action for already existing device on the fly. >>> >>> At the same time, drop local redefinition of the actions eum in >> >> s/eum/enum/ >> >>> watchdog.h in favour of the ones defined in schema. >>> >>> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169 >>> >>> Signed-off-by: Michal Privoznik >>> --- >> >>> @@ -77,27 +80,17 @@ int select_watchdog(const char *p) >>> >>> int select_watchdog_action(const char *p) >>> { >> >>> +int action; >>> >>> +action = qapi_enum_parse(WatchdogExpirationAction_lookup, p, >>> + WATCHDOG_EXPIRATION_ACTION__MAX, -1, NULL); >> >> Semantic conflict; you need to rebase now that the qapi enum lookup code >> has landed (see commit ebf677c8 and parents) >> >>> + >>> +case WATCHDOG_EXPIRATION_ACTION__MAX: >>> +/* keep gcc happy */ >>> +break; >> >> Could use g_assert_not_reached() here instead. > > Catches one out of approximately 2^64 invalid values. To catches all, > and in fewer words: > > default: > assert(0); I'm not a fan of this, but I'm not a qemu developer so I don't know what your coding style is (if any for this case). However, since this switch is over an enum, compiler will scream if a new value is introduced to the enum and not handled in the switch. IMO it's useful because when I'm adding new value I can immediately see what other places I need to consider. Also, yeah, I am going to rename the enum in v3. Michal
Re: [Qemu-devel] [PATCH v2] watchdog: Allow setting action on the fly
Michal Privoznik writes: > Currently, the only time that users can set watchdog action is at > the start as all we expose is this -watchdog-action command line > argument. This is suboptimal when users want to plug the device > later via monitor. Alternatively, they might want to change the > action for already existing device on the fly. > > At the same time, drop local redefinition of the actions eum in > watchdog.h in favour of the ones defined in schema. > > Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169 > > Signed-off-by: Michal Privoznik > --- > > diff to v1: > - instead of allowing 'action' to be arbitrary string, use enum for it and > thus > drop local redefinition of the enum. > > hw/watchdog/watchdog.c| 52 > --- > hw/watchdog/wdt_diag288.c | 6 +++--- > include/sysemu/watchdog.h | 12 ++- > qapi-schema.json | 9 > 4 files changed, 41 insertions(+), 38 deletions(-) > > diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c > index 0c5c9cde1c..a3e9d7b59b 100644 > --- a/hw/watchdog/watchdog.c > +++ b/hw/watchdog/watchdog.c > @@ -29,8 +29,11 @@ > #include "qapi-event.h" > #include "hw/nmi.h" > #include "qemu/help_option.h" > +#include "qmp-commands.h" > +#include "qapi/qmp/qerror.h" > +#include "qapi/util.h" > > -static int watchdog_action = WDT_RESET; > +static WatchdogExpirationAction watchdog_action = > WATCHDOG_EXPIRATION_ACTION_RESET; > static QLIST_HEAD(watchdog_list, WatchdogTimerModel) watchdog_list; > > void watchdog_add_model(WatchdogTimerModel *model) > @@ -77,27 +80,17 @@ int select_watchdog(const char *p) > > int select_watchdog_action(const char *p) > { > -if (strcasecmp(p, "reset") == 0) > -watchdog_action = WDT_RESET; > -else if (strcasecmp(p, "shutdown") == 0) > -watchdog_action = WDT_SHUTDOWN; > -else if (strcasecmp(p, "poweroff") == 0) > -watchdog_action = WDT_POWEROFF; > -else if (strcasecmp(p, "pause") == 0) > -watchdog_action = WDT_PAUSE; > -else if (strcasecmp(p, "debug") == 0) > -watchdog_action = WDT_DEBUG; > -else if (strcasecmp(p, "none") == 0) > -watchdog_action = WDT_NONE; > -else if (strcasecmp(p, "inject-nmi") == 0) > -watchdog_action = WDT_NMI; > -else > -return -1; > +int action; > > +action = qapi_enum_parse(WatchdogExpirationAction_lookup, p, > + WATCHDOG_EXPIRATION_ACTION__MAX, -1, NULL); > +if (action < 0) > +return -1; > +qmp_watchdog_set_action(action, NULL); &error_abort > return 0; > } > > -int get_watchdog_action(void) > +WatchdogExpirationAction get_watchdog_action(void) > { > return watchdog_action; > } > @@ -108,21 +101,21 @@ int get_watchdog_action(void) > void watchdog_perform_action(void) > { > switch (watchdog_action) { > -case WDT_RESET: /* same as 'system_reset' in monitor */ > +case WATCHDOG_EXPIRATION_ACTION_RESET: /* same as 'system_reset' in > monitor */ > qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_RESET, > &error_abort); > qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); > break; > > -case WDT_SHUTDOWN: /* same as 'system_powerdown' in monitor */ > +case WATCHDOG_EXPIRATION_ACTION_SHUTDOWN: /* same as > 'system_powerdown' in monitor */ > qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_SHUTDOWN, > &error_abort); > qemu_system_powerdown_request(); > break; > > -case WDT_POWEROFF: /* same as 'quit' command in monitor */ > +case WATCHDOG_EXPIRATION_ACTION_POWEROFF: /* same as 'quit' command in > monitor */ > qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_POWEROFF, > &error_abort); > exit(0); > > -case WDT_PAUSE: /* same as 'stop' command in monitor */ > +case WATCHDOG_EXPIRATION_ACTION_PAUSE: /* same as 'stop' command in > monitor */ > /* In a timer callback, when vm_stop calls qemu_clock_enable > * you would get a deadlock. Bypass the problem. > */ > @@ -131,19 +124,28 @@ void watchdog_perform_action(void) > qemu_system_vmstop_request(RUN_STATE_WATCHDOG); > break; > > -case WDT_DEBUG: > +case WATCHDOG_EXPIRATION_ACTION_DEBUG: > qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_DEBUG, > &error_abort); > fprintf(stderr, "watchdog: timer fired\n"); > break; > > -case WDT_NONE: > +case WATCHDOG_EXPIRATION_ACTION_NONE: > qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_NONE, > &error_abort); > break; > > -case WDT_NMI: > +case WATCHDOG_EXPIRATION_ACTION_INJECT_NMI: > qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_INJECT_NMI, > &error_abort); > nmi_monitor_handle(0, NULL); > br
Re: [Qemu-devel] [PATCH v2] watchdog: Allow setting action on the fly
On 09/05/2017 10:36 PM, Eric Blake wrote: > On 09/05/2017 08:22 AM, Michal Privoznik wrote: >> Currently, the only time that users can set watchdog action is at >> the start as all we expose is this -watchdog-action command line >> argument. This is suboptimal when users want to plug the device >> later via monitor. Alternatively, they might want to change the >> action for already existing device on the fly. >> >> At the same time, drop local redefinition of the actions eum in > > s/eum/enum/ > >> watchdog.h in favour of the ones defined in schema. >> >> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169 >> >> Signed-off-by: Michal Privoznik >> --- > >> @@ -77,27 +80,17 @@ int select_watchdog(const char *p) >> >> int select_watchdog_action(const char *p) >> { > >> +int action; >> >> +action = qapi_enum_parse(WatchdogExpirationAction_lookup, p, >> + WATCHDOG_EXPIRATION_ACTION__MAX, -1, NULL); > > Semantic conflict; you need to rebase now that the qapi enum lookup code > has landed (see commit ebf677c8 and parents) D'oh. Okay. > >> + >> +case WATCHDOG_EXPIRATION_ACTION__MAX: >> +/* keep gcc happy */ >> +break; > > Could use g_assert_not_reached() here instead. > >> +++ b/qapi-schema.json >> @@ -6539,3 +6539,12 @@ >> # Since 2.9 >> ## >> { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } >> + >> +## >> +# @watchdog-set-action: >> +# >> +# Set watchdog action >> +# >> +# Since 2.11 >> +## >> +{ 'command': 'watchdog-set-action', 'data' : {'action': >> 'WatchdogExpirationAction'} } > > Can a machine have more than one watchdog device, in which case you'd > want a device name to select which watchdog gets which action? But the > command-line version that you are replacing seems to be global, so I > guess you're okay with this interface. > Yeah, I don't think that a guest can have more than one watchdog: /qemu.git $ ./x86_64-softmmu/qemu-system-x86_64 \ -watchdog ib700 -watchdog i6300esb qemu-system-x86_64: -watchdog i6300esb: only one watchdog option may be given Also, would it make sense? I mean, what would be the benefit of having two or more watchdogs? Michal
Re: [Qemu-devel] [PATCH v2] watchdog: Allow setting action on the fly
Eric Blake writes: > On 09/05/2017 08:22 AM, Michal Privoznik wrote: >> Currently, the only time that users can set watchdog action is at >> the start as all we expose is this -watchdog-action command line >> argument. This is suboptimal when users want to plug the device >> later via monitor. Alternatively, they might want to change the >> action for already existing device on the fly. >> >> At the same time, drop local redefinition of the actions eum in > > s/eum/enum/ > >> watchdog.h in favour of the ones defined in schema. >> >> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169 >> >> Signed-off-by: Michal Privoznik >> --- > >> @@ -77,27 +80,17 @@ int select_watchdog(const char *p) >> >> int select_watchdog_action(const char *p) >> { > >> +int action; >> >> +action = qapi_enum_parse(WatchdogExpirationAction_lookup, p, >> + WATCHDOG_EXPIRATION_ACTION__MAX, -1, NULL); > > Semantic conflict; you need to rebase now that the qapi enum lookup code > has landed (see commit ebf677c8 and parents) > >> + >> +case WATCHDOG_EXPIRATION_ACTION__MAX: >> +/* keep gcc happy */ >> +break; > > Could use g_assert_not_reached() here instead. Catches one out of approximately 2^64 invalid values. To catches all, and in fewer words: default: assert(0); >> +++ b/qapi-schema.json >> @@ -6539,3 +6539,12 @@ >> # Since 2.9 >> ## >> { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } >> + >> +## >> +# @watchdog-set-action: >> +# >> +# Set watchdog action >> +# >> +# Since 2.11 >> +## >> +{ 'command': 'watchdog-set-action', 'data' : {'action': >> 'WatchdogExpirationAction'} } > > Can a machine have more than one watchdog device, in which case you'd > want a device name to select which watchdog gets which action? But the > command-line version that you are replacing seems to be global, so I > guess you're okay with this interface. For better or worse, all watchdogs share the action, see hw/watchdog/watchdog.c. Watchdogs predate qdev. Commit 9dd986c lets you configure up to one, with two command line options: --watchdog and --watchdog-action. In a way, --watchdog configures the frontend, and --watchdog-action the backend (having watchdog.c in hw/watchdog/ contradicts this view, though). Since the frontends got qdevified (commit 09aaa16), you can configure any number of watchdog frontends, but still only one backend shared by all. Unsharing the backend wasn't worth the trouble, because having multiple watchdogs would be weird, and multiple watchdogs with different actions would be weirder. Trouble would include maintaining command line backward compatibility, as always. Of course, if we *want* to unshare the backend, we should do it *before* we add QMP backward compatibility to the trouble.
Re: [Qemu-devel] [PATCH v2] watchdog: Allow setting action on the fly
On 09/05/2017 08:22 AM, Michal Privoznik wrote: > Currently, the only time that users can set watchdog action is at > the start as all we expose is this -watchdog-action command line > argument. This is suboptimal when users want to plug the device > later via monitor. Alternatively, they might want to change the > action for already existing device on the fly. > > At the same time, drop local redefinition of the actions eum in s/eum/enum/ > watchdog.h in favour of the ones defined in schema. > > Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169 > > Signed-off-by: Michal Privoznik > --- > @@ -77,27 +80,17 @@ int select_watchdog(const char *p) > > int select_watchdog_action(const char *p) > { > +int action; > > +action = qapi_enum_parse(WatchdogExpirationAction_lookup, p, > + WATCHDOG_EXPIRATION_ACTION__MAX, -1, NULL); Semantic conflict; you need to rebase now that the qapi enum lookup code has landed (see commit ebf677c8 and parents) > + > +case WATCHDOG_EXPIRATION_ACTION__MAX: > +/* keep gcc happy */ > +break; Could use g_assert_not_reached() here instead. > +++ b/qapi-schema.json > @@ -6539,3 +6539,12 @@ > # Since 2.9 > ## > { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } > + > +## > +# @watchdog-set-action: > +# > +# Set watchdog action > +# > +# Since 2.11 > +## > +{ 'command': 'watchdog-set-action', 'data' : {'action': > 'WatchdogExpirationAction'} } Can a machine have more than one watchdog device, in which case you'd want a device name to select which watchdog gets which action? But the command-line version that you are replacing seems to be global, so I guess you're okay with this interface. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2] watchdog: Allow setting action on the fly
Currently, the only time that users can set watchdog action is at the start as all we expose is this -watchdog-action command line argument. This is suboptimal when users want to plug the device later via monitor. Alternatively, they might want to change the action for already existing device on the fly. At the same time, drop local redefinition of the actions eum in watchdog.h in favour of the ones defined in schema. Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169 Signed-off-by: Michal Privoznik --- diff to v1: - instead of allowing 'action' to be arbitrary string, use enum for it and thus drop local redefinition of the enum. hw/watchdog/watchdog.c| 52 --- hw/watchdog/wdt_diag288.c | 6 +++--- include/sysemu/watchdog.h | 12 ++- qapi-schema.json | 9 4 files changed, 41 insertions(+), 38 deletions(-) diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c index 0c5c9cde1c..a3e9d7b59b 100644 --- a/hw/watchdog/watchdog.c +++ b/hw/watchdog/watchdog.c @@ -29,8 +29,11 @@ #include "qapi-event.h" #include "hw/nmi.h" #include "qemu/help_option.h" +#include "qmp-commands.h" +#include "qapi/qmp/qerror.h" +#include "qapi/util.h" -static int watchdog_action = WDT_RESET; +static WatchdogExpirationAction watchdog_action = WATCHDOG_EXPIRATION_ACTION_RESET; static QLIST_HEAD(watchdog_list, WatchdogTimerModel) watchdog_list; void watchdog_add_model(WatchdogTimerModel *model) @@ -77,27 +80,17 @@ int select_watchdog(const char *p) int select_watchdog_action(const char *p) { -if (strcasecmp(p, "reset") == 0) -watchdog_action = WDT_RESET; -else if (strcasecmp(p, "shutdown") == 0) -watchdog_action = WDT_SHUTDOWN; -else if (strcasecmp(p, "poweroff") == 0) -watchdog_action = WDT_POWEROFF; -else if (strcasecmp(p, "pause") == 0) -watchdog_action = WDT_PAUSE; -else if (strcasecmp(p, "debug") == 0) -watchdog_action = WDT_DEBUG; -else if (strcasecmp(p, "none") == 0) -watchdog_action = WDT_NONE; -else if (strcasecmp(p, "inject-nmi") == 0) -watchdog_action = WDT_NMI; -else -return -1; +int action; +action = qapi_enum_parse(WatchdogExpirationAction_lookup, p, + WATCHDOG_EXPIRATION_ACTION__MAX, -1, NULL); +if (action < 0) +return -1; +qmp_watchdog_set_action(action, NULL); return 0; } -int get_watchdog_action(void) +WatchdogExpirationAction get_watchdog_action(void) { return watchdog_action; } @@ -108,21 +101,21 @@ int get_watchdog_action(void) void watchdog_perform_action(void) { switch (watchdog_action) { -case WDT_RESET: /* same as 'system_reset' in monitor */ +case WATCHDOG_EXPIRATION_ACTION_RESET: /* same as 'system_reset' in monitor */ qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_RESET, &error_abort); qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); break; -case WDT_SHUTDOWN: /* same as 'system_powerdown' in monitor */ +case WATCHDOG_EXPIRATION_ACTION_SHUTDOWN: /* same as 'system_powerdown' in monitor */ qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_SHUTDOWN, &error_abort); qemu_system_powerdown_request(); break; -case WDT_POWEROFF: /* same as 'quit' command in monitor */ +case WATCHDOG_EXPIRATION_ACTION_POWEROFF: /* same as 'quit' command in monitor */ qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_POWEROFF, &error_abort); exit(0); -case WDT_PAUSE: /* same as 'stop' command in monitor */ +case WATCHDOG_EXPIRATION_ACTION_PAUSE: /* same as 'stop' command in monitor */ /* In a timer callback, when vm_stop calls qemu_clock_enable * you would get a deadlock. Bypass the problem. */ @@ -131,19 +124,28 @@ void watchdog_perform_action(void) qemu_system_vmstop_request(RUN_STATE_WATCHDOG); break; -case WDT_DEBUG: +case WATCHDOG_EXPIRATION_ACTION_DEBUG: qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_DEBUG, &error_abort); fprintf(stderr, "watchdog: timer fired\n"); break; -case WDT_NONE: +case WATCHDOG_EXPIRATION_ACTION_NONE: qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_NONE, &error_abort); break; -case WDT_NMI: +case WATCHDOG_EXPIRATION_ACTION_INJECT_NMI: qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_INJECT_NMI, &error_abort); nmi_monitor_handle(0, NULL); break; + +case WATCHDOG_EXPIRATION_ACTION__MAX: +/* keep gcc happy */ +break; } } + +void qmp_watchdog_set_action(WatchdogExpirationAction action, Error **errp) +{ +watchdog_action = action; +} diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c index 47f289216a..4