Re: [Qemu-devel] [PATCH v2] watchdog: Allow setting action on the fly

[Markus Armbruster]

Michal Privoznik <address@hidden> 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 <address@hidden>
> ---
>
> 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.