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

[Markus Armbruster]

Note: depends on Gerd's "[PATCH] QemuOpts: increase number of
vm_config_groups".  Without it, I get "ran out of space in
vm_config_groups".

Paolo Bonzini <address@hidden> writes:

> 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.

Adding -watchdog was a mistake.

> One alternative implementation is possible, namely to add an "action"
> property to each watchdog device.

Yes, this is possible.  Current global watchdog_action gets replaced by
a watchdog device property, passed to watchdog_perform_action() as a
parameter.

What kvm_arch_handle_exit() should pass on KVM_EXIT_WATCHDOG isn't
immediately obvious :)

>                                    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).

You'd have to configure their watchdog action with -global, because
that's how we set onboard device properties.

> 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.

The advantages equally apply to existing -watchdog-action.

Since using multiple watchdogs is weird, and with different actions even
weirder, a single, shared watchdog action suffices.

Let's take a step back.

Stated objective: ability to configure watchdog in a configuration file.

    Have already: ability to configure watchdog device there.  Example:

        [device]
          driver = "ib700"

    Note: unlike -balloon virtio, -watchdog does *not* expand into the
    equivalent -device.  "-balloon virtio -watchdog ib700 -writeconfig
    /dev/stdout" produces

        [device]
          driver = "virtio-balloon"

    -balloon is pure syntactic sugar, -watchdog is alternative syntax.

    Missing part: ability to configure the watchdog action.

Proposed solution:

1. Convert -watchdog to QemuOpts, with implied_opt_name "model".

   This makes -watchdog available in configuration files, like this:

        [watchdog]
           model = "ib700"

   "-balloon virtio -watchdog ib700 -writeconfig /dev/stdout" now
   produces

        [device]
          driver = "virtio-balloon"

        [watchdog]
          model = "ib700"

   Digs us deeper into the "alternative syntax" hole.

   The new QemuOptsList has .merge_lists = true, i.e. multiple
   definitions get merged.  That one's always great fun.

   Before the patch, multiple -watchdog get rejected with "qemu: only
   one watchdog option may be given"[*].  Afterwards, all but one are
   silently ignored, and predicting which of multiple -watchdog is used
   is left as an exercise for the reader ;-P

   Of the three -watchdog behaviors "reject more than one watchdog",
   "just add them all whether it makes sense or not" and "add at most
   one, silently ignore the rest", this gives us the worst.

2. Add an "action" parameter to QemuOptsList "watchdog"

   This provides the missing part.

   -watchdog-action A becomes sugar for -watchdog action=A.  Should be
   spelled out in the commit message.

   Remember, all the "watchdog" options get merged.  So what does
   "--watchdog ib700,action=reset --watchdog i6300esb" do?  Spoiler:
   adds i6300 with action reset[**].

Welcome again to the QemuOpts swamp, hope you'll enjoy your stay.

If the stated objective is all you want, why not convert
-watchdog-action instead of -watchdog?  Should be simpler.  Just make
sure to preserve "last option wins" behavior.


[*] Sub-par error message, use of error_report() would fix.

[**] Found by trying it out; I'll be hanged if I can predict it without
running the code