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

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

[Paolo Bonzini]

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 the 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 <pbonzini@redhat.com>
---
 docs/qdev-device-use.txt |  4 ++++
 qemu-options.hx          | 36 ++++++++++++++--------------
 vl.c                     | 61 +++++++++++++++++++++++++++++++++++-------------
 3 files changed, 68 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..719253c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3115,12 +3115,19 @@ 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,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
-@item -watchdog @var{model}
+@item -watchdog [model=]@var{model},action=@var{action}
 @findex -watchdog
+@itemx -watchdog-action @var{action}
+@findex -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 +3136,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
-@item -watchdog-action @var{action}
-@findex -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 +3163,9 @@ situations where the watchdog would have expired, and thus
 Examples:
 
 @table @code
-@item -watchdog i6300esb -watchdog-action pause
+@item -watchdog i6300esb,action=pause
 @item -watchdog ib700
+@item -device ib700 -watchdog action=poweroff
 @end table
 ETEXI
 
diff --git a/vl.c b/vl.c
index 9a04ad4..b76919e 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);
-- 
1.8.3.1

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

[Richard W.M. Jones]

On Wed, May 27, 2015 at 02:35:26PM +0200, Paolo Bonzini wrote:
>              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);
>                  }

Is there an error message missing (before the call to exit)?

Patch seems OK otherwise.

libvirt still uses

  -watchdog <model> -watchdog-action <action>

Should it be changed to use -device?  I'm guessing for backwards
compatibility with old and current qemu it probably shouldn't be
changed for a few years.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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

[Paolo Bonzini]

On 27/05/2015 14:41, Richard W.M. Jones wrote:
> On Wed, May 27, 2015 at 02:35:26PM +0200, Paolo Bonzini wrote:
>>              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);
>>                  }
> 
> Is there an error message missing (before the call to exit)?
> 
> Patch seems OK otherwise.
> 
> libvirt still uses
> 
>   -watchdog <model> -watchdog-action <action>
> 
> Should it be changed to use -device?  I'm guessing for backwards
> compatibility with old and current qemu it probably shouldn't be
> changed for a few years.

-device has been working forever for watchdogs.  Changing libvirt to
-device would be possible, but cosmetic.  Changing to "-watchdog
action=foo" is also entirely cosmetic, and would not work on older
QEMUs, so again there's no need.

The advantage of this patch would be for -readconfig users, which
couldn't specify the watchdog action.  Or you could put '[watchdog]
action="pause"' in /etc/qemu/qemu.conf if you're into obscure
configuration files.

Paolo

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

[Paolo Bonzini]

On 27/05/2015 14:41, Richard W.M. Jones wrote:
>> > -                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);
>> >                  }
> Is there an error message missing (before the call to exit)?

Oh, I forgot to answer this.  Any error message is printed by
qemu_opts_parse, for example:

$ x86_64-softmmu/qemu-system-x86_64 -watchdog foo=bar
qemu-system-x86_64: -watchdog foo=bar: Invalid parameter 'foo'

Paolo

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

[Richard W.M. Jones]

On Wed, May 27, 2015 at 02:45:29PM +0200, Paolo Bonzini wrote:
> 
> 
> On 27/05/2015 14:41, Richard W.M. Jones wrote:
> >> > -                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);
> >> >                  }
> > Is there an error message missing (before the call to exit)?
> 
> Oh, I forgot to answer this.  Any error message is printed by
> qemu_opts_parse, for example:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -watchdog foo=bar
> qemu-system-x86_64: -watchdog foo=bar: Invalid parameter 'foo'

Patch seems fine then, 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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2671 bytes --]

On 05/27/2015 06:35 AM, 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 the 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 <pbonzini@redhat.com>
> ---
>  docs/qdev-device-use.txt |  4 ++++
>  qemu-options.hx          | 36 ++++++++++++++--------------
>  vl.c                     | 61 +++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 68 insertions(+), 33 deletions(-)
> 

> +++ b/qemu-options.hx
> @@ -3115,12 +3115,19 @@ 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,action=reset|shutdown|poweroff|pause|debug|none\n" \

Should this be

-watchdog [model=]i6300esb|ib700[,action=...none]

to make it obvious that ,action= is optional for back-compat with old usage?

> +    "                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
> -@item -watchdog @var{model}
> +@item -watchdog [model=]@var{model},action=@var{action}

same here.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 27/05/2015 15:00, Eric Blake wrote:
> Should this be
> 
> -watchdog [model=]i6300esb|ib700[,action=...none]
> 
> to make it obvious that ,action= is optional for back-compat with
> old usage?

Sure, but then it should be

   -watchdog [[model=]i6300esb|ib700][,action=...none]

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJVZcB5AAoJEL/70l94x66DyZMIAJN5YCp+NwIxYMieIEssXDsG
sZCyu3pv7RG8tx66VnExBXSUxMd4U4JXvDbME9g6jVPie3CNGXbkgE0f7BWAfBbi
1oA07px8Zx0xOMZIMWDJc803viSixHn/UJxFcN00baS+lx9l6a0yRt013Z0sFbNl
0TBSVpYUOM0JNo42QXuSiMClbxLE/DaSS4NO+2uPV9NV1USz3LDM+/bd7BKHmRsb
JZhVPXYmmq/ZxSrb+UJprwJOqQ7yHqZeLU0IL76TrWGthRWfMZP7Dg6y5QZgR3cd
cM00qERu4iIYH18TwA6jBdoPjgsHMDWvHuwLhrpqOcnl5KKu/F4rKjdXLOvfxHw=
=XXtJ
-----END PGP SIGNATURE-----