On 09/12/2017 04:23 PM, John Ferlan wrote: > > > On 09/05/2017 07:45 AM, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1447169 >> >> Once again, 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 | 15 ++++- >> src/qemu/qemu_alias.h | 2 + >> src/qemu/qemu_command.c | 2 +- >> src/qemu/qemu_command.h | 3 + >> src/qemu/qemu_driver.c | 10 +++- >> src/qemu/qemu_hotplug.c | 64 ++++++++++++++++++++++ >> 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 | 55 +++++++++++++++++++ >> 14 files changed, 205 insertions(+), 4 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..63f69e80f 100644 >> --- a/src/qemu/qemu_alias.c >> +++ b/src/qemu/qemu_alias.c >> @@ -405,6 +405,19 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def, >> } >> >> >> +int >> +qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog) >> +{ >> + const int idx = 0; >> + >> + /* Currently, there's just one watchdog per domain */ >> + >> + if (virAsprintf(&watchdog->info.alias, "watchdog%d", idx) < 0) > > There's really no need for @idx... s/idx/0/ works fine. > > Although if multiple watchdogs were ever to be implemented, then using > the watchdog->action could perhaps be a mechanism to have a watchdog for > each type of action. Of course that breaks down for an existing domain > on an old version where a new libvirtd gets added and hot unplug is > attempted (since the running domain would always have watchdog0). Oh > well.... It was a thought. > >> + return -1; >> + return 0; >> +} >> + >> + >> int >> qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) >> { >> @@ -482,7 +495,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) >> return -1; >> } >> if (def->watchdog) { >> - if (virAsprintf(&def->watchdog->info.alias, "watchdog%d", 0) < 0) >> + if (qemuAssignDeviceWatchdogAlias(def->watchdog) < 0) >> return -1; >> } >> if (def->memballoon) { >> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h >> index 300fd4de5..652ffea0c 100644 >> --- a/src/qemu/qemu_alias.h >> +++ b/src/qemu/qemu_alias.h >> @@ -65,6 +65,8 @@ int qemuAssignDeviceShmemAlias(virDomainDefPtr def, >> virDomainShmemDefPtr shmem, >> int idx); >> >> +int qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog); >> + >> int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps); >> >> int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info, >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index 9a27987d4..eb9df044b 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -3921,7 +3921,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, >> } >> >> >> -static char * >> +char * >> qemuBuildWatchdogDevStr(const virDomainDef *def, >> virDomainWatchdogDefPtr dev, >> virQEMUCapsPtr qemuCaps) >> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h >> index 6fbfb3e5f..4b504f685 100644 >> --- a/src/qemu/qemu_command.h >> +++ b/src/qemu/qemu_command.h >> @@ -206,5 +206,8 @@ char *qemuBuildShmemDevStr(virDomainDefPtr def, >> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >> > > Only one space between... Although, I see commit id '06524fd5' a few > extra blank spaces. We can use this opportunity to clean them up > though... Just be sure there's an extra blank line before the #endif > though (helps w/ readability at least for me). > >> >> +char * qemuBuildWatchdogDevStr(const virDomainDef *def, >> + virDomainWatchdogDefPtr dev, >> + virQEMUCapsPtr qemuCaps); > > You have an extra space between * and qemu... Of course affects spacing > of subsequent lines too. Could always go with the same format as .c: > > char * > qemuBuildWatchdogDevStr(const virDomainDef *def, > virDomainWatchdogDefPtr dev, > virQEMUCapsPtr qemuCaps); > > > > My only other note is I really wish qemu_command.h was ordered the same > way as qemu_command.c, but that's a different problem. IOW: Why add to > the end, but that's one of those type-A type problems. > >> >> #endif /* __QEMU_COMMAND_H__*/ >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 85b4be360..626148dba 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -7593,12 +7593,20 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, >> } >> break; >> >> + case VIR_DOMAIN_DEVICE_WATCHDOG: >> + ret = qemuDomainAttachWatchdog(driver, vm, >> + dev->data.watchdog); >> + if (!ret) { > > I wish all the callers used "if (ret == 0)", but I won't complain ;-) > >> + alias = dev->data.watchdog->info.alias; >> + dev->data.watchdog = NULL; >> + } >> + break; >> + >> case VIR_DOMAIN_DEVICE_NONE: >> case VIR_DOMAIN_DEVICE_FS: >> case VIR_DOMAIN_DEVICE_INPUT: >> case VIR_DOMAIN_DEVICE_SOUND: >> case VIR_DOMAIN_DEVICE_VIDEO: >> - case VIR_DOMAIN_DEVICE_WATCHDOG: >> case VIR_DOMAIN_DEVICE_GRAPHICS: >> case VIR_DOMAIN_DEVICE_HUB: >> case VIR_DOMAIN_DEVICE_SMARTCARD: >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index b365078ec..989146eb9 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -2866,6 +2866,70 @@ 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; >> + 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->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { >> + if (qemuDomainEnsurePCIAddress(vm, &dev, driver) < 0) >> + goto cleanup; >> + releaseAddress = true; >> + } >> + >> + /* 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; >> + > > I see the command line building has similar code, but also has a check > that actualAction actually *TypeToString properly. That is really useless. How can we successfully parse action but fail to format it back? > >> + qemuDomainObjEnterMonitor(driver, vm); >> + >> + rv = qemuMonitorSetWatchdogAction(priv->mon, actualAction); > > Any reason to not pass a const char *action here instead? Allowing this > code to check validity rather than failing lower in the stack. We can fail lower in the stack but not because of bad conversion from enum to string. But okay, I can change this to const char *action. We use both approaches, so one can argue either way. > > I also think perhaps there could be a "common" API that both > qemu_command and qemu_hotplug could call in order to return the action > string so that you remove the duplicated code. Well the only duplicated part is the check where action is overwritten. I don't think it's that bad. > > My only other comment is that it seems a bit strange to adjust the > action of something that isn't yet added to the domain. That is the > ordering of altering the action, then adding the watchdog doesn't seem > right. The command line has the "-device" first, then the "action" > second, that is I see from the *.args file: > > ... > -device ib700,id=watchdog0 \ > -watchdog-action pause \ > ... > > which if qemu is reading the order would seem to imply the device is > added, then the action is applied. That's just coincidence IMO. The true reason was that I don't have to try unplug the watchdog if setting action fails. > >> + >> + if (rv >= 0) > > This only ever returns -1 or 0, right? Yep. But in general, 0 or a positive value are considered as success. > >> + rv = qemuMonitorAddDevice(priv->mon, watchdogstr); >> + >> + if (qemuDomainObjExitMonitor(driver, vm) < 0) { >> + releaseAddress = false; >> + goto cleanup; >> + } >> + >> + if (rv < 0) >> + goto cleanup; > > Why no virDomainAuditWatchdog type code? Ah, good point. There's no virDomainAuditWatchdog(), but there should be! I'll post another version where I'll introduce the audit function as the first patch. > >> + >> + releaseAddress = false; >> + vm->def->watchdog = watchdog; >> + ret = 0; >> + >> + cleanup: >> + if (releaseAddress) >> + qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL); >> + VIR_FREE(watchdogstr); >> + return ret; >> +} >> + >> + >> static int >> qemuDomainChangeNetBridge(virDomainObjPtr vm, >> virDomainNetDefPtr olddev, >> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h >> index 985c6733f..a9dfd8f7a 100644 >> --- a/src/qemu/qemu_hotplug.h >> +++ b/src/qemu/qemu_hotplug.h >> @@ -80,6 +80,9 @@ int qemuDomainAttachHostDevice(virConnectPtr conn, >> int qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, >> virDomainObjPtr vm, >> virDomainShmemDefPtr shmem); >> +int qemuDomainAttachWatchdog(virQEMUDriverPtr driver, >> + virDomainObjPtr vm, >> + virDomainWatchdogDefPtr watchdog); >> int qemuDomainFindGraphicsIndex(virDomainDefPtr def, >> virDomainGraphicsDefPtr dev); >> int qemuDomainAttachMemory(virQEMUDriverPtr driver, >> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c >> index 19082d8bf..5d3f9df47 100644 >> --- a/src/qemu/qemu_monitor.c >> +++ b/src/qemu/qemu_monitor.c >> @@ -4343,3 +4343,15 @@ qemuMonitorEventPanicInfoFree(qemuMonitorEventPanicInfoPtr info) >> >> VIR_FREE(info); >> } >> + >> + >> +int >> +qemuMonitorSetWatchdogAction(qemuMonitorPtr mon, >> + virDomainWatchdogAction watchdogAction) >> +{ >> + VIR_DEBUG("watchdogAction=%d", watchdogAction); >> + >> + QEMU_CHECK_MONITOR_JSON(mon); >> + >> + return qemuMonitorJSONSetWatchdogAction(mon, watchdogAction); >> +} >> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h >> index 9805a3390..b8c9409e5 100644 >> --- a/src/qemu/qemu_monitor.h >> +++ b/src/qemu/qemu_monitor.h >> @@ -1132,4 +1132,6 @@ int qemuMonitorSetBlockThreshold(qemuMonitorPtr mon, >> >> virJSONValuePtr qemuMonitorQueryNamedBlockNodes(qemuMonitorPtr mon); >> >> +int qemuMonitorSetWatchdogAction(qemuMonitorPtr mon, >> + virDomainWatchdogAction watchdogAction); > > Prefer to see that extra blank line here... > >> #endif /* QEMU_MONITOR_H */ >> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c >> index df5fb7c8f..02ba701e6 100644 >> --- a/src/qemu/qemu_monitor_json.c >> +++ b/src/qemu/qemu_monitor_json.c >> @@ -7679,3 +7679,31 @@ qemuMonitorJSONQueryNamedBlockNodes(qemuMonitorPtr mon) >> >> return ret; >> } >> + >> + >> +int >> +qemuMonitorJSONSetWatchdogAction(qemuMonitorPtr mon, >> + virDomainWatchdogAction watchdogAction) >> +{ >> + virJSONValuePtr cmd; >> + virJSONValuePtr reply = NULL; >> + const char *action = virDomainWatchdogActionTypeToString(watchdogAction); >> + int ret = -1; > > Command line building checks !action.... > > I would think this function should take a "const char *action" and the > caller handle the error though. > >> + >> + if (!(cmd = qemuMonitorJSONMakeCommand("watchdog-set-action", >> + "s:action", action, >> + NULL))) >> + return -1; >> + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) >> + goto cleanup; >> + >> + if (qemuMonitorJSONCheckError(cmd, reply) < 0) >> + goto cleanup; >> + >> + ret = 0; >> + >> + cleanup: >> + virJSONValueFree(cmd); >> + virJSONValueFree(reply); >> + return ret; >> +} >> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h >> index 7462967b5..a7a43d7b8 100644 >> --- a/src/qemu/qemu_monitor_json.h >> +++ b/src/qemu/qemu_monitor_json.h >> @@ -524,4 +524,7 @@ int qemuMonitorJSONSetBlockThreshold(qemuMonitorPtr mon, >> virJSONValuePtr qemuMonitorJSONQueryNamedBlockNodes(qemuMonitorPtr mon) >> ATTRIBUTE_NONNULL(1); >> >> +int qemuMonitorJSONSetWatchdogAction(qemuMonitorPtr mon, >> + virDomainWatchdogAction watchdogAction) >> + ATTRIBUTE_NONNULL(1); > > Likewise with the extra blank line. > >> #endif /* QEMU_MONITOR_JSON_H */ >> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c >> index 23a498617..b02ae8034 100644 >> --- a/tests/qemuhotplugtest.c >> +++ b/tests/qemuhotplugtest.c >> @@ -127,6 +127,9 @@ testQemuHotplugAttach(virDomainObjPtr vm, >> case VIR_DOMAIN_DEVICE_SHMEM: >> ret = qemuDomainAttachShmemDevice(&driver, vm, dev->data.shmem); >> break; >> + case VIR_DOMAIN_DEVICE_WATCHDOG: >> + ret = qemuDomainAttachWatchdog(&driver, vm, dev->data.watchdog); >> + break; >> default: >> VIR_TEST_VERBOSE("device type '%s' cannot be attached\n", >> virDomainDeviceTypeToString(dev->type)); >> @@ -811,10 +814,14 @@ mymain(void) >> "device_del", QMP_OK, >> "object-del", QMP_OK); >> DO_TEST_ATTACH("base-live+disk-scsi-wwn", >> - "disk-scsi-duplicate-wwn", false, true, >> + "disk-scsi-duplicate-wwn", false, false, > > This would seem to be unrelated... and should be separate. Not really. We don't want to keep the device (=disk) in the domain because then the test case I'm adding would need to look different (with that disk in, which would be insane). Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
