Re: [Qemu-devel] [PATCH v2] watchdog: convert to QemuOpts

[Richard W.M. Jones]

On Wed, May 27, 2015 at 03:09:01PM +0200, Paolo Bonzini wrote:
> This makes it possible to specify a watchdog action in a configuration file.
> The previous behavior of "-watchdog" is moved to the (implied) "-watchdog
> model" suboption.  This is already more or less obsolete, since it is possible
> to achieve the same effect with "-device", but "-watchdog-action" does not 
> have
> an equivalent.
> 
> One alternative implementation is possible, namely to add an "action"
> property to each watchdog device.  However, boards often have embedded
> watchdog devices; even if they currently don't, these should call
> watchdog_perform_action() so that they are affected by -watchdog-action.
> (This is listed in our BiteSizedTasks wiki page).
> 
> For these boards, "-watchdog action=foo" has two advantages:
> 
> 1) it is much easier to use than a "-global" option, and can be
> configured globally for all boards.
> 
> 2) it is harder to add a property to a device than it is to just
> s/qemu_system_reset_request/watchdog_perform_action/; in some cases,
> the devices are not even qdev-ified at all.  The chance of the conversion
> happening then would basically be zero if one had to add a property as
> a prerequisite.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>       tweak brackets in help messages [Eric]
>       fix missing error message for -watchdog action [Richard]
> ---
>  docs/qdev-device-use.txt |  4 ++++
>  qemu-options.hx          | 37 +++++++++++++++--------------
>  vl.c                     | 61 
> +++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 69 insertions(+), 33 deletions(-)
> 
> diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
> index 136d271..1906b7d 100644
> --- a/docs/qdev-device-use.txt
> +++ b/docs/qdev-device-use.txt
> @@ -365,6 +365,10 @@ The old way to define a guest watchdog device is 
> -watchdog DEVNAME.
>  The new way is -device DEVNAME.  For PCI devices, you can add
>  bus=PCI-BUS,addr=DEVFN to control the PCI device address, as usual.
>  
> +No matter if you use -watchdog or -device to define the guest watchdog
> +device, you can specify the watchdog action with "-watchdog-action ACTION"
> +or "-watchdog action=ACTION".
> +
>  === Host Device Assignment ===
>  
>  QEMU supports assigning host PCI devices (qemu-kvm only at this time)
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ec356f6..628f282 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3115,12 +3115,20 @@ when the shift value is high (how high depends on the 
> host machine).
>  ETEXI
>  
>  DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
> -    "-watchdog i6300esb|ib700\n" \
> -    "                enable virtual hardware watchdog [default=none]\n",
> +    "-watchdog [[model=]i6300esb|ib700]\n" \
> +    "          [,action=reset|shutdown|poweroff|pause|debug|none]\n" \
> +    "                enable virtual hardware watchdog (default: 
> model=none,action=reset)\n",
> +    QEMU_ARCH_ALL)
> +DEF("watchdog-action", HAS_ARG, QEMU_OPTION_watchdog_action, \
> +    "-watchdog-action reset|shutdown|poweroff|pause|debug|none\n" \
> +    "                action when watchdog fires (default: reset)\n",
>      QEMU_ARCH_ALL)
>  STEXI
> address@hidden -watchdog @var{model}
> address@hidden -watchdog address@hidden,address@hidden
>  @findex -watchdog
> address@hidden -watchdog-action @var{action}
> address@hidden -watchdog-action
> +
>  Create a virtual hardware watchdog device.  Once enabled (by a guest
>  action), the watchdog must be periodically polled by an agent inside
>  the guest or else the guest will be restarted.
> @@ -3129,22 +3137,16 @@ The @var{model} is the model of hardware watchdog to 
> emulate.  Choices
>  for model are: @code{ib700} (iBASE 700) which is a very simple ISA
>  watchdog with a single timer, or @code{i6300esb} (Intel 6300ESB I/O
>  controller hub) which is a much more featureful PCI-based dual-timer
> -watchdog.  Choose a model for which your guest has drivers.
> -
> -Use @code{-watchdog help} to list available hardware models.  Only one
> +watchdog.  Choose a model for which your guest has drivers;
> +use @code{-watchdog help} to list available hardware models.  Only one
>  watchdog can be enabled for a guest.
> -ETEXI
> -
> -DEF("watchdog-action", HAS_ARG, QEMU_OPTION_watchdog_action, \
> -    "-watchdog-action reset|shutdown|poweroff|pause|debug|none\n" \
> -    "                action when watchdog fires [default=reset]\n",
> -    QEMU_ARCH_ALL)
> -STEXI
> address@hidden -watchdog-action @var{action}
> address@hidden -watchdog-action
>  
>  The @var{action} controls what QEMU will do when the watchdog timer
> -expires.
> +expires.  It can be specified directly in the @option{-watchdog} option,
> +or as the argument of a separate @option{-watchdog-action} parameter.
> +The effect is the same.  The watchdog action can be specified even if
> +the watchdog is enabled through the @option{-device} option.
> +
>  The default is
>  @code{reset} (forcefully reset the guest).
>  Other possible actions are:
> @@ -3162,8 +3164,9 @@ situations where the watchdog would have expired, and 
> thus
>  Examples:
>  
>  @table @code
> address@hidden -watchdog i6300esb -watchdog-action pause
> address@hidden -watchdog i6300esb,action=pause
>  @item -watchdog ib700
> address@hidden -device ib700 -watchdog action=poweroff
>  @end table
>  ETEXI
>  
> diff --git a/vl.c b/vl.c
> index 9a04ad4..55363fe 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -168,7 +168,6 @@ static int no_reboot;
>  int no_shutdown = 0;
>  int cursor_hide = 1;
>  int graphic_rotate = 0;
> -const char *watchdog;
>  QEMUOptionRom option_rom[MAX_OPTION_ROMS];
>  int nb_option_roms;
>  int semihosting_enabled = 0;
> @@ -475,6 +474,23 @@ static QemuOptsList qemu_icount_opts = {
>      },
>  };
>  
> +static QemuOptsList qemu_watchdog_opts = {
> +    .name = "watchdog",
> +    .implied_opt_name = "model",
> +    .merge_lists = true,
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_watchdog_opts.head),
> +    .desc = {
> +        {
> +            .name = "model",
> +            .type = QEMU_OPT_STRING,
> +        }, {
> +            .name = "action",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  static QemuOptsList qemu_semihosting_config_opts = {
>      .name = "semihosting-config",
>      .implied_opt_name = "enable",
> @@ -1224,6 +1240,26 @@ static void configure_msg(QemuOpts *opts)
>      enable_timestamp_msg = qemu_opt_get_bool(opts, "timestamp", true);
>  }
>  
> +static void configure_watchdog(QemuOpts *opts)
> +{
> +    const char *s;
> +
> +    s = qemu_opt_get(opts, "model");
> +    if (s) {
> +        int i = select_watchdog(s);
> +        if (i != 0) {
> +            exit (i <= 1 ? 1 : 0);
> +        }
> +    }
> +    s = qemu_opt_get(opts, "action");
> +    if (s) {
> +        int i = select_watchdog_action(s);
> +        if (i != 0) {
> +            exit (i <= 1 ? 1 : 0);
> +        }
> +    }
> +}
> +
>  /***********************************************************/
>  /* USB devices */
>  
> @@ -2739,7 +2775,6 @@ static void set_memory_options(uint64_t *ram_slots, 
> ram_addr_t *maxram_size)
>  
>  int main(int argc, char **argv, char **envp)
>  {
> -    int i;
>      int snapshot, linux_boot;
>      const char *initrd_filename;
>      const char *kernel_filename, *kernel_cmdline;
> @@ -2815,6 +2850,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_name_opts);
>      qemu_add_opts(&qemu_numa_opts);
>      qemu_add_opts(&qemu_icount_opts);
> +    qemu_add_opts(&qemu_watchdog_opts);
>      qemu_add_opts(&qemu_semihosting_config_opts);
>  
>      runstate_init();
> @@ -3343,18 +3379,15 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_watchdog:
> -                if (watchdog) {
> -                    fprintf(stderr,
> -                            "qemu: only one watchdog option may be given\n");
> -                    return 1;
> +                olist = qemu_find_opts("watchdog");
> +                opts = qemu_opts_parse(olist, optarg, 1);
> +                if (!opts) {
> +                    exit(1);
>                  }
> -                watchdog = optarg;
>                  break;
>              case QEMU_OPTION_watchdog_action:
> -                if (select_watchdog_action(optarg) == -1) {
> -                    fprintf(stderr, "Unknown -watchdog-action parameter\n");
> -                    exit(1);
> -                }
> +                qemu_opts_set(qemu_find_opts("watchdog"), 0, "action", 
> optarg,
> +                              &error_abort);
>                  break;
>              case QEMU_OPTION_virtiocon:
>                  add_device_config(DEV_VIRTCON, optarg);
> @@ -4227,11 +4260,7 @@ int main(int argc, char **argv, char **envp)
>          select_vgahw(vga_model);
>      }
>  
> -    if (watchdog) {
> -        i = select_watchdog(watchdog);
> -        if (i > 0)
> -            exit (i == 1 ? 1 : 0);
> -    }
> +    configure_watchdog(qemu_find_opts_singleton("watchdog"));
>  
>      if (machine_class->compat_props) {
>          qdev_prop_register_global_list(machine_class->compat_props);
> -- 

ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org