We already have enum that enumerates all the action that a
watchdog can take when hitting its timeout: WatchdogAction.
Use that instead of inventing our own.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
hw/watchdog/watchdog.c | 42 +++++++++++++++++-------------------------
hw/watchdog/wdt_diag288.c | 6 +++---
include/sysemu/watchdog.h | 12 ++----------
3 files changed, 22 insertions(+), 38 deletions(-)
diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 358d79804d..547a49a1e4 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -30,7 +30,7 @@
#include "hw/nmi.h"
#include "qemu/help_option.h"
-static int watchdog_action = WDT_RESET;
+static WatchdogAction watchdog_action = WATCHDOG_ACTION_RESET;
static QLIST_HEAD(watchdog_list, WatchdogTimerModel) watchdog_list;
void watchdog_add_model(WatchdogTimerModel *model)
@@ -77,27 +77,16 @@ 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(&WatchdogAction_lookup, p, -1, NULL);
+ if (action < 0)
+ return -1;
+ watchdog_action = action;
return 0;
}
-int get_watchdog_action(void)
+WatchdogAction get_watchdog_action(void)
{
return watchdog_action;
}
@@ -108,21 +97,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_ACTION_RESET: /* same as 'system_reset' in monitor */
qapi_event_send_watchdog(WATCHDOG_ACTION_RESET, &error_abort);
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
break;
- case WDT_SHUTDOWN: /* same as 'system_powerdown' in monitor */
+ case WATCHDOG_ACTION_SHUTDOWN: /* same as 'system_powerdown' in monitor */
qapi_event_send_watchdog(WATCHDOG_ACTION_SHUTDOWN, &error_abort);
qemu_system_powerdown_request();
break;
- case WDT_POWEROFF: /* same as 'quit' command in monitor */
+ case WATCHDOG_ACTION_POWEROFF: /* same as 'quit' command in monitor */
qapi_event_send_watchdog(WATCHDOG_ACTION_POWEROFF, &error_abort);
exit(0);
- case WDT_PAUSE: /* same as 'stop' command in monitor */
+ case WATCHDOG_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 +120,22 @@ void watchdog_perform_action(void)
qemu_system_vmstop_request(RUN_STATE_WATCHDOG);
break;
- case WDT_DEBUG:
+ case WATCHDOG_ACTION_DEBUG:
qapi_event_send_watchdog(WATCHDOG_ACTION_DEBUG, &error_abort);
fprintf(stderr, "watchdog: timer fired\n");
break;
- case WDT_NONE:
+ case WATCHDOG_ACTION_NONE:
qapi_event_send_watchdog(WATCHDOG_ACTION_NONE, &error_abort);
break;
- case WDT_NMI:
+ case WATCHDOG_ACTION_INJECT_NMI:
qapi_event_send_watchdog(WATCHDOG_ACTION_INJECT_NMI,
&error_abort);
nmi_monitor_handle(0, NULL);
break;
+
+ default:
+ assert(0);
}
}
diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
index 47f289216a..1475743527 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_ACTION_DEBUG:
+ case WATCHDOG_ACTION_NONE:
+ case WATCHDOG_ACTION_PAUSE:
break;
default:
wdt_diag288_reset(dev);
diff --git a/include/sysemu/watchdog.h b/include/sysemu/watchdog.h
index 72a4da07a6..677ace3945 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);
+WatchdogAction get_watchdog_action(void);
void watchdog_add_model(WatchdogTimerModel *model);
void watchdog_perform_action(void);
--
2.13.5
On 09/06/2017 06:24 AM, Michal Privoznik wrote:
> We already have enum that enumerates all the action that a
s/action/actions/
> watchdog can take when hitting its timeout: WatchdogAction.
> Use that instead of inventing our own.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> @@ -77,27 +77,16 @@ int select_watchdog(const char *p)
>
> int select_watchdog_action(const char *p)
> {
> - if (strcasecmp(p, "reset") == 0)
> - watchdog_action = WDT_RESET;
The old code was case-insensitive,
> + action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);
the new code is not. Do we care? (I don't, but we could be breaking
someone's control flow). Should qapi_enum_parse be taught to be
case-insensitive? Or perhaps we answer related questions first: Do we
have any QAPI enums that have values differing only in case? Do we
prevent such QAPI definitions, to give us the potential of making the
parsing insensitive?
--
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/06/2017 06:24 AM, Michal Privoznik wrote:
>> We already have enum that enumerates all the action that a
>
> s/action/actions/
>
>> watchdog can take when hitting its timeout: WatchdogAction.
>> Use that instead of inventing our own.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>
>> @@ -77,27 +77,16 @@ int select_watchdog(const char *p)
>>
>> int select_watchdog_action(const char *p)
>> {
>> - if (strcasecmp(p, "reset") == 0)
>> - watchdog_action = WDT_RESET;
>
> The old code was case-insensitive,
>
>> + action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);
>
> the new code is not. Do we care? (I don't, but we could be breaking
> someone's control flow). Should qapi_enum_parse be taught to be
> case-insensitive? Or perhaps we answer related questions first: Do we
> have any QAPI enums that have values differing only in case? Do we
> prevent such QAPI definitions, to give us the potential of making the
> parsing insensitive?
Case-sensitive everywhere is fine. Case-insensitive everywhere also
fine, just not my personal preference. What's not fine is "guess
whether this part of the interface is case-sensitive or not".
QMP is case-sensitive. Let's keep it that way.
The -watchdog-action option has a case-insensitive argument. The
obvious way to remain misfeature-^Wbackwards compatible is converting
the argument to lower case before handing it off to qapi_enum_parse. I
doubt it matters, but just doing it is less work than debating how far
exactly we want to bend over backwards.
g_ascii_strdown() should do. It only converts ASCII characters, but
anything else is going to fail in qapi_enum_parse() anyway.
On 09/06/2017 07:25 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 09/06/2017 06:24 AM, Michal Privoznik wrote:
>>> We already have enum that enumerates all the action that a
>>
>> s/action/actions/
>>
>>> watchdog can take when hitting its timeout: WatchdogAction.
>>> Use that instead of inventing our own.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>
>>> @@ -77,27 +77,16 @@ int select_watchdog(const char *p)
>>>
>>> int select_watchdog_action(const char *p)
>>> {
>>> - if (strcasecmp(p, "reset") == 0)
>>> - watchdog_action = WDT_RESET;
>>
>> The old code was case-insensitive,
>>
>>> + action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);
>>
>> the new code is not. Do we care? (I don't, but we could be breaking
>> someone's control flow). Should qapi_enum_parse be taught to be
>> case-insensitive? Or perhaps we answer related questions first: Do we
>> have any QAPI enums that have values differing only in case? Do we
>> prevent such QAPI definitions, to give us the potential of making the
>> parsing insensitive?
>
> Case-sensitive everywhere is fine. Case-insensitive everywhere also
> fine, just not my personal preference. What's not fine is "guess
> whether this part of the interface is case-sensitive or not".
>
> QMP is case-sensitive. Let's keep it that way.
>
> The -watchdog-action option has a case-insensitive argument. The
> obvious way to remain misfeature-^Wbackwards compatible is converting
> the argument to lower case before handing it off to qapi_enum_parse. I
> doubt it matters, but just doing it is less work than debating how far
> exactly we want to bend over backwards.
>
> g_ascii_strdown() should do. It only converts ASCII characters, but
> anything else is going to fail in qapi_enum_parse() anyway.
>
On the other hand, the documentation enumerates the accepted values in
lowercase. So one can argue that upper- or mixed-case is just a misuse
of a bug in the code. But getting the code in is more important to me so
I'll do the strdown() conversion and sent yet another version.
Michal
Michal Privoznik <mprivozn@redhat.com> writes:
> On 09/06/2017 07:25 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 09/06/2017 06:24 AM, Michal Privoznik wrote:
>>>> We already have enum that enumerates all the action that a
>>>
>>> s/action/actions/
>>>
>>>> watchdog can take when hitting its timeout: WatchdogAction.
>>>> Use that instead of inventing our own.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>
>>>> @@ -77,27 +77,16 @@ int select_watchdog(const char *p)
>>>>
>>>> int select_watchdog_action(const char *p)
>>>> {
>>>> - if (strcasecmp(p, "reset") == 0)
>>>> - watchdog_action = WDT_RESET;
>>>
>>> The old code was case-insensitive,
>>>
>>>> + action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);
>>>
>>> the new code is not. Do we care? (I don't, but we could be breaking
>>> someone's control flow). Should qapi_enum_parse be taught to be
>>> case-insensitive? Or perhaps we answer related questions first: Do we
>>> have any QAPI enums that have values differing only in case? Do we
>>> prevent such QAPI definitions, to give us the potential of making the
>>> parsing insensitive?
>>
>> Case-sensitive everywhere is fine. Case-insensitive everywhere also
>> fine, just not my personal preference. What's not fine is "guess
>> whether this part of the interface is case-sensitive or not".
>>
>> QMP is case-sensitive. Let's keep it that way.
>>
>> The -watchdog-action option has a case-insensitive argument. The
>> obvious way to remain misfeature-^Wbackwards compatible is converting
>> the argument to lower case before handing it off to qapi_enum_parse. I
>> doubt it matters, but just doing it is less work than debating how far
>> exactly we want to bend over backwards.
>>
>> g_ascii_strdown() should do. It only converts ASCII characters, but
>> anything else is going to fail in qapi_enum_parse() anyway.
>>
>
> On the other hand, the documentation enumerates the accepted values in
> lowercase. So one can argue that upper- or mixed-case is just a misuse
> of a bug in the code.
I quite agree, but...
> But getting the code in is more important to me so
> I'll do the strdown() conversion and sent yet another version.
... like you, I have to pick my battles. A respin adding the stupid
case conversion seems safer for both of us than risking a debate on how
far we need to go for backward compatibility.
© 2016 - 2026 Red Hat, Inc.