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(-)
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 <mprivozn@redhat.com>
---
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..451644eb89 100644
--- a/hw/watchdog/wdt_diag288.c
+++ b/hw/watchdog/wdt_diag288.c
@@ -57,9 +57,9 @@ static void diag288_timer_expired(void *dev)
* the BQL; reset before triggering the action to avoid races with
* diag288 instructions. */
switch (get_watchdog_action()) {
- case WDT_DEBUG:
- case WDT_NONE:
- case WDT_PAUSE:
+ case WATCHDOG_EXPIRATION_ACTION_DEBUG:
+ case WATCHDOG_EXPIRATION_ACTION_NONE:
+ case WATCHDOG_EXPIRATION_ACTION_PAUSE:
break;
default:
wdt_diag288_reset(dev);
diff --git a/include/sysemu/watchdog.h b/include/sysemu/watchdog.h
index 72a4da07a6..1b830df152 100644
--- a/include/sysemu/watchdog.h
+++ b/include/sysemu/watchdog.h
@@ -23,15 +23,7 @@
#define QEMU_WATCHDOG_H
#include "qemu/queue.h"
-
-/* Possible values for action parameter. */
-#define WDT_RESET 1 /* Hard reset. */
-#define WDT_SHUTDOWN 2 /* Shutdown. */
-#define WDT_POWEROFF 3 /* Quit. */
-#define WDT_PAUSE 4 /* Pause. */
-#define WDT_DEBUG 5 /* Prints a message and continues running. */
-#define WDT_NONE 6 /* Do nothing. */
-#define WDT_NMI 7 /* Inject nmi into the guest. */
+#include "qapi-types.h"
struct WatchdogTimerModel {
QLIST_ENTRY(WatchdogTimerModel) entry;
@@ -46,7 +38,7 @@ typedef struct WatchdogTimerModel WatchdogTimerModel;
/* in hw/watchdog.c */
int select_watchdog(const char *p);
int select_watchdog_action(const char *action);
-int get_watchdog_action(void);
+WatchdogExpirationAction get_watchdog_action(void);
void watchdog_add_model(WatchdogTimerModel *model);
void watchdog_perform_action(void);
diff --git a/qapi-schema.json b/qapi-schema.json
index 802ea53d00..40605be36f 100644
--- a/qapi-schema.json
+++ 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'} }
--
2.13.5
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 <mprivozn@redhat.com>
> ---
> @@ -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
Eric Blake <eblake@redhat.com> 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 <mprivozn@redhat.com>
>> ---
>
>> @@ -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.
On 09/06/2017 07:59 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <mprivozn@redhat.com>
>>> ---
>>
>>> @@ -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
Michal Privoznik <mprivozn@redhat.com> writes:
> On 09/06/2017 07:59 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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 <mprivozn@redhat.com>
>>>> ---
>>>
>>>> @@ -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!
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 <mprivozn@redhat.com>
>> ---
>
>> @@ -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
Michal Privoznik <mprivozn@redhat.com> 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 <mprivozn@redhat.com>
[...]
>>> +++ 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.
Michal Privoznik <mprivozn@redhat.com> 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 <mprivozn@redhat.com>
> ---
>
> 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);
> break;
> +
> + case WATCHDOG_EXPIRATION_ACTION__MAX:
> + /* keep gcc happy */
> + break;
default:
assert(0);
> }
> }
> +
> +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..451644eb89 100644
> --- a/hw/watchdog/wdt_diag288.c
> +++ b/hw/watchdog/wdt_diag288.c
> @@ -57,9 +57,9 @@ static void diag288_timer_expired(void *dev)
> * the BQL; reset before triggering the action to avoid races with
> * diag288 instructions. */
> switch (get_watchdog_action()) {
> - case WDT_DEBUG:
> - case WDT_NONE:
> - case WDT_PAUSE:
> + case WATCHDOG_EXPIRATION_ACTION_DEBUG:
> + case WATCHDOG_EXPIRATION_ACTION_NONE:
> + case WATCHDOG_EXPIRATION_ACTION_PAUSE:
> break;
> default:
> wdt_diag288_reset(dev);
> diff --git a/include/sysemu/watchdog.h b/include/sysemu/watchdog.h
> index 72a4da07a6..1b830df152 100644
> --- a/include/sysemu/watchdog.h
> +++ b/include/sysemu/watchdog.h
> @@ -23,15 +23,7 @@
> #define QEMU_WATCHDOG_H
>
> #include "qemu/queue.h"
> -
> -/* Possible values for action parameter. */
> -#define WDT_RESET 1 /* Hard reset. */
> -#define WDT_SHUTDOWN 2 /* Shutdown. */
> -#define WDT_POWEROFF 3 /* Quit. */
> -#define WDT_PAUSE 4 /* Pause. */
> -#define WDT_DEBUG 5 /* Prints a message and continues running. */
> -#define WDT_NONE 6 /* Do nothing. */
> -#define WDT_NMI 7 /* Inject nmi into the guest. */
> +#include "qapi-types.h"
>
> struct WatchdogTimerModel {
> QLIST_ENTRY(WatchdogTimerModel) entry;
> @@ -46,7 +38,7 @@ typedef struct WatchdogTimerModel WatchdogTimerModel;
> /* in hw/watchdog.c */
> int select_watchdog(const char *p);
> int select_watchdog_action(const char *action);
> -int get_watchdog_action(void);
> +WatchdogExpirationAction get_watchdog_action(void);
> void watchdog_add_model(WatchdogTimerModel *model);
> void watchdog_perform_action(void);
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 802ea53d00..40605be36f 100644
> --- a/qapi-schema.json
> +++ 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'} }
I was wondering whether you forgot to include the enum definition, until
I discovered you smartly reused the existing one.
If it was a new one instead, I would suggest naming it WatchdogAction,
because a watchdog's one and only action is the expiration action, and
therefore the shorter name serves just fine. Hmm, it's used in just two
places... okay, I'm suggesting you rename it, in a separate preparatory
patch.
This cleans up code duplication. Worth mentioning in the commit
message.
Looks pretty good overall; a quick respin should get us there.
© 2016 - 2024 Red Hat, Inc.