Paolo Bonzini <[email protected]> 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.
