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

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> On 03/06/2015 15:14, Markus Armbruster wrote:
>>>                                    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.
>
> Right.  That however doesn't work if the watchdog is a device but isn't
> qdevified, or the watchdog isn't a device and doesn't have a dummy
> device wrapper around it, aka the KVM_EXIT_WATCHDOG case.
>
> Also, it's very hard to discover (e.g. how is one supposed to find a
> watchdog_action property under ICH9_LPC---not yet upstream, but should
> be in 2.4).

I completely agree with you that we don't want to turn it into a device
property.  The original design is just fine.

>>    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.
>
> True, but consistent with "-drive if=virtio" which doesn't produce a
> [device] stanza.

-drive is level 5 magic, and the need for backward compatibility makes
reducing its magic hard.

The watchdog options could be level 1.  Not doing "pure sugar" adds a
level, and merge_lists adds another.  If level 3 magic is what it takes
to get the user interface and the backward compatibility we want, then
so be it.

>>    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.
>
> True, but consistent with the handling of other merge_lists options: the
> last wins.

Still a user interface change for the worse.  That it's no worse now
than other places have always been is a rather weak excuse :)

>> 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.
>
> Because I'm not sure that we won't have other watchdog options in the
> future.

I believe what we got here is really just the usual split into frontend
(a.k.a. device model) and backend, except the backend is trivial and
shared by all watchdog frontends.  Device model options should be added
to the device model.  What we need is a place to hold the shared
backend's options, for easy-to-extend backend configuration.

Converting -watchdog to QemuOpts creates such a place.

So does converting -watchdog-action, except it's not mixed up with the
existing *frontend* configuration sugar.

>          Also,
>
>       [watchdog]
>           action = "reset"
>
> is marginally nicer than any of
>
>       [watchdog-action]
>           action = "reset"
>
> and
>
>       [machine]
>           watchdog-action = "reset"

I agree that "watchdog-action" is an ugly name for backend
configuration.  "watchdog" would be fine, but we foolishly burned that
on silly command line sugar for the frontend.

"watchdog-config"?  Precedent: "semihosting-config".

Then make -watchdog T pure sugar for -device T, and -watchdog-action A
pure sugar for -watchdog-config action=A.

Not demands, just ideas.