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@xxxxxxxxxx> >>> --- >>> 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@xxxxxxxxxx> John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
