On 10/05/2017 04:07 AM, Michal Privoznik wrote:
On 10/04/2017 11:20 PM, John Ferlan wrote:
On 09/27/2017 08:12 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1447169
Since domain can have at most one watchdog it simplifies things a bit. However, since we must be able to set the watchdog action as well, new monitor command needs to be used.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_alias.c | 13 +++- src/qemu/qemu_alias.h | 2 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 4 +- src/qemu/qemu_driver.c | 10 ++- src/qemu/qemu_hotplug.c | 72 ++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 3 + src/qemu/qemu_monitor.c | 12 ++++ src/qemu/qemu_monitor.h | 2 + src/qemu/qemu_monitor_json.c | 28 +++++++++ src/qemu/qemu_monitor_json.h | 3 + tests/qemuhotplugtest.c | 9 ++- .../qemuhotplug-watchdog.xml | 1 + .../qemuhotplug-base-live+watchdog.xml | 56 +++++++++++++++++ 14 files changed, 212 insertions(+), 5 deletions(-) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+watchdog.xml
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 914b2b94d..72df1083f 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -405,6 +405,17 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def, }
+int +qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog) +{ + /* Currently, there's just one watchdog per domain */ + + if (VIR_STRDUP(watchdog->info.alias, "watchdog0") < 0) + return -1; + return 0; +} + +
Not trying to increase the patch count for review purposes, but this easily could have been a separate patch ;-)
[...]
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4913e18e6..11afa7ec6 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2836,6 +2836,78 @@ qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, }
+int +qemuDomainAttachWatchdog(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainWatchdogDefPtr watchdog) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_WATCHDOG, { .watchdog = watchdog } }; + virDomainWatchdogAction actualAction = watchdog->action; + const char *actionStr = NULL; + char *watchdogstr = NULL; + bool releaseAddress = false; + int rv; + + if (vm->def->watchdog) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain already has a watchdog")); + return -1; + } + + if (qemuAssignDeviceWatchdogAlias(watchdog) < 0) + return -1; + + if (!(watchdogstr = qemuBuildWatchdogDevStr(vm->def, watchdog, priv->qemuCaps))) + return -1; + + if (watchdog->model == VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB) { + if (qemuDomainEnsurePCIAddress(vm, &dev, driver) < 0) + goto cleanup; + releaseAddress = true; + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("hotplug of watchdog of model %s is not supported"), + virDomainWatchdogModelTypeToString(watchdog->model)); + goto cleanup; + } + + /* QEMU doesn't have a 'dump' action; we tell qemu to 'pause', then + libvirt listens for the watchdog event, and we perform the dump + ourselves. so convert 'dump' to 'pause' for the qemu cli */ + if (actualAction == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) + actualAction = VIR_DOMAIN_WATCHDOG_ACTION_PAUSE; + + actionStr = virDomainWatchdogActionTypeToString(actualAction); + + qemuDomainObjEnterMonitor(driver, vm); + + rv = qemuMonitorSetWatchdogAction(priv->mon, actionStr);
Something that didn't dawn on me for previous review... Where's the qemuCaps for "watchdog-set-action" that (ahem) you introduced into qemu 2.11?
Do we do that? IMO, if the command is not there, we just error out. There are plenty of examples, for instance: qemuMonitorJSONGetMemoryDeviceInfo(). If the command is not there, -2 is returned. Not that the caller would care.
Well - guess I just assumed... Too many commands to know about I guess in order to know whether we had/used ones in that manner. I suppose the "usual" manner of ensuring that a command option exists before using and supplying a (nicer) message about that "This QEMU doesn't support ..." as opposed to what I assume will be "unable to execute QEMU command...".
I mean, we might have caps for commands, but I guess that's for different reason. For instance, we have QEMU_CAPS_WAKEUP. But this capability exist so that we know upfront if the command is available and don't learn that in the middle after we've suspended the domain and have no way of waking it up. So that's slightly different use case, isn't it? I view qemuCaps as helper for cmd line generation mostly.
No sense in even trying if the command is not there. Not personally trying to increase the patches to review, but seems there could be some extra separation possible (alias, caps, monitor/json, hotplug, unplug).
Additionally, is there a minimum version that allows hot-plug of a watchdog device that we need to concern ourselves with handling?
I don't think so. If qemu is old enough that it lacks watchdog-set-action we don't even get to the point of trying to hotplug the watchdog. and if it is new enough that it as the command it supports watchdog hotplug already.
Duh, the question came as a result of hot unplug where I started thinking too much, but without a hot unplug, then you've got no hot plug. Same 'concept' applies - failure will come from QEMU though (one would think/hope).
I'm fine with the rest of the overall design/concepts, I just think you need to split up a wee bit more and of course add the caps check....
Well, I can split it if you want me to, but:
a) in the end the code will look the same, b) it doesn't make sense for somebody to backport just a part of it. They'll backport either all of them or none. They might as well just backport this one. Or not.
Michal
Hey - I used those arguments in my head many times ;-) - perhaps even the dog has heard them a few times. I suppose since there's no reason to go back and rework in order to add a capability for the command, then no need to deal with splitting up any more, so... Reviewed-by: John Ferlan <jferlan@redhat.com> John