[libvirt] [PATCH v3 0/3] qemu: Watchdog plug/unplug
v3 of: https://www.redhat.com/archives/libvir-list/2017-September/msg00974.html diff to v2: - Peter's review worked int - Dropped audit patch - More check on watchdog device Michal Privoznik (3): qemuDomainDeviceDefValidate: Validate watchdog qemu: hot-plug of watchdog qemu: hot-unplug of watchdog 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_domain.c | 51 ++++++++ src/qemu/qemu_driver.c | 14 ++- src/qemu/qemu_hotplug.c | 139 +++++++++++++++++++++ src/qemu/qemu_hotplug.h | 6 + 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 | 14 ++- .../qemuhotplug-watchdog-full.xml | 4 + .../qemuhotplug-watchdog.xml | 1 + .../qemuhotplug-base-live+watchdog.xml | 56 +++++++++ 16 files changed, 345 insertions(+), 6 deletions(-) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+watchdog.xml -- 2.13.5
Currently we don't do it. Therefore we accept senseless combinations of models and buses they are attached to. Moreover, diag288 watchdog is exclusive to s390(x). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0b094a15e..71926bcd8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3340,6 +3340,54 @@ qemuDomainRedirdevDefValidate(const virDomainRedirdevDef *def) } +static int +qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev, + const virDomainDef *def) +{ + switch ((virDomainWatchdogModel) dev->model) { + case VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB: + if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s model of watchog can go only on PCI bus"), + virDomainWatchdogModelTypeToString(dev->model)); + return -1; + } + break; + + case VIR_DOMAIN_WATCHDOG_MODEL_IB700: + if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s model of watchog can go only on ISA bus"), + virDomainWatchdogModelTypeToString(dev->model)); + return -1; + } + break; + + case VIR_DOMAIN_WATCHDOG_MODEL_DIAG288: + if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s model of watchog is virtual and cannot go on any bus."), + virDomainWatchdogModelTypeToString(dev->model)); + return -1; + } + if (!(ARCH_IS_S390(def->os.arch))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s model of watchdog is allowed for s390 and s390x only"), + virDomainWatchdogModelTypeToString(dev->model)); + return -1; + } + break; + + case VIR_DOMAIN_WATCHDOG_MODEL_LAST: + break; + } + + return 0; +} + + static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, @@ -3448,6 +3496,9 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, } else if (dev->type == VIR_DOMAIN_DEVICE_REDIRDEV) { if (qemuDomainRedirdevDefValidate(dev->data.redirdev) < 0) goto cleanup; + } else if (dev->type == VIR_DOMAIN_DEVICE_WATCHDOG) { + if (qemuDomainWatchdogDefValidate(dev->data.watchdog, def) < 0) + goto cleanup; } /* forbid capabilities mode hostdev in this kind of hypervisor */ -- 2.13.5
On 09/27/2017 08:12 AM, Michal Privoznik wrote:
Currently we don't do it. Therefore we accept senseless combinations of models and buses they are attached to. Moreover, diag288 watchdog is exclusive to s390(x).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
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; +} + + int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { @@ -482,7 +493,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 abeb24846..5ded0ae79 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3964,7 +3964,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..94e4592cc 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -205,6 +205,8 @@ char *qemuBuildShmemDevStr(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); - +char *qemuBuildWatchdogDevStr(const virDomainDef *def, + virDomainWatchdogDefPtr dev, + virQEMUCapsPtr qemuCaps); #endif /* __QEMU_COMMAND_H__*/ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4855c9047..db8ad0b04 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7648,12 +7648,20 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, } break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + ret = qemuDomainAttachWatchdog(driver, vm, + dev->data.watchdog); + if (!ret) { + 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 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); + + if (rv >= 0) + rv = qemuMonitorAddDevice(priv->mon, watchdogstr); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + releaseAddress = false; + goto cleanup; + } + + if (rv < 0) + goto cleanup; + + 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 363ad76cf..7a2678587 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4339,3 +4339,15 @@ qemuMonitorEventPanicInfoFree(qemuMonitorEventPanicInfoPtr info) VIR_FREE(info); } + + +int +qemuMonitorSetWatchdogAction(qemuMonitorPtr mon, + const char *action) +{ + VIR_DEBUG("watchdogAction=%s", action); + + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONSetWatchdogAction(mon, action); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 6414d2483..d9c27acae 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1129,4 +1129,6 @@ int qemuMonitorSetBlockThreshold(qemuMonitorPtr mon, virJSONValuePtr qemuMonitorQueryNamedBlockNodes(qemuMonitorPtr mon); +int qemuMonitorSetWatchdogAction(qemuMonitorPtr mon, + const char *action); #endif /* QEMU_MONITOR_H */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c63d250d3..a9070fe63 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7695,3 +7695,31 @@ qemuMonitorJSONQueryNamedBlockNodes(qemuMonitorPtr mon) return ret; } + + +int +qemuMonitorJSONSetWatchdogAction(qemuMonitorPtr mon, + const char *action) +{ + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + int ret = -1; + + 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 0cdfc5ead..f418c7426 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -521,4 +521,7 @@ int qemuMonitorJSONSetBlockThreshold(qemuMonitorPtr mon, virJSONValuePtr qemuMonitorJSONQueryNamedBlockNodes(qemuMonitorPtr mon) ATTRIBUTE_NONNULL(1); +int qemuMonitorJSONSetWatchdogAction(qemuMonitorPtr mon, + const char *action) + ATTRIBUTE_NONNULL(1); #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, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); + DO_TEST_ATTACH("base-live", "watchdog", false, false, + "watchdog-set-action", QMP_OK, + "device_add", QMP_OK); + #define DO_TEST_CPU_GROUP(prefix, vcpus, modernhp, expectfail) \ do { \ cpudata.test = prefix; \ diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-watchdog.xml b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog.xml new file mode 100644 index 000000000..2980c0f9c --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog.xml @@ -0,0 +1 @@ +<watchdog model='i6300esb' action='poweroff'/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+watchdog.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+watchdog.xml new file mode 100644 index 000000000..9f8f983e5 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+watchdog.xml @@ -0,0 +1,56 @@ +<domain type='kvm' id='7'> + <name>hotplug</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <alias name='usb'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <alias name='ide'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <alias name='scsi0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <alias name='pci'/> + </controller> + <controller type='virtio-serial' index='0'> + <alias name='virtio-serial0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'> + <alias name='input0'/> + </input> + <input type='keyboard' bus='ps2'> + <alias name='input1'/> + </input> + <watchdog model='i6300esb' action='poweroff'> + <alias name='watchdog0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </watchdog> + <memballoon model='none'> + <alias name='balloon0'/> + </memballoon> + </devices> + <seclabel type='none' model='none'/> +</domain> -- 2.13.5
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? 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'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.... John
+ + if (rv >= 0) + rv = qemuMonitorAddDevice(priv->mon, watchdogstr); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + releaseAddress = false; + goto cleanup; + } + + if (rv < 0) + goto cleanup; + + releaseAddress = false; + vm->def->watchdog = watchdog; + ret = 0; + + cleanup: + if (releaseAddress) + qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL); + VIR_FREE(watchdogstr); + return ret; +} + +
[...]
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. 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.
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
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
On 10/05/2017 01:48 PM, John Ferlan wrote:
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
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>
Pushed thanks :-) I'll post news.xml patch shortly. Should learn myself to include it in the series. Michal
https://bugzilla.redhat.com/show_bug.cgi?id=1447169 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 67 ++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 3 + tests/qemuhotplugtest.c | 7 ++- .../qemuhotplug-watchdog-full.xml | 4 ++ 5 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index db8ad0b04..4c8e9297a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7747,12 +7747,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SHMEM: ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem); break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog); + break; 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 11afa7ec6..1d95b88c4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4337,6 +4337,25 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, } +static int +qemuDomainRemoveWatchdog(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainWatchdogDefPtr watchdog) +{ + virObjectEventPtr event = NULL; + + VIR_DEBUG("Removing watchdog %s from domain %p %s", + watchdog->info.alias, vm, vm->def->name); + + event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias); + qemuDomainEventQueue(driver, event); + qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL); + virDomainWatchdogDefFree(vm->def->watchdog); + vm->def->watchdog = NULL; + return 0; +} + + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -5053,6 +5072,54 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, } +int +qemuDomainDetachWatchdog(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainWatchdogDefPtr dev) +{ + int ret = -1; + virDomainWatchdogDefPtr watchdog = vm->def->watchdog; + qemuDomainObjPrivatePtr priv = vm->privateData; + + /* While domains can have up to one watchdog, the one supplied by the user + * doesn't necessarily match the one domain has. Refuse to detach in such + * case. */ + if (!(watchdog && + watchdog->model == dev->model && + watchdog->action == dev->action && + virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info))) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("watchdog device not present in domain configuration")); + return -1; + } + + if (watchdog->model != VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("hotunplug of watchdog of model %s is not supported"), + virDomainWatchdogModelTypeToString(watchdog->model)); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &watchdog->info); + qemuDomainObjEnterMonitor(driver, vm); + + ret = qemuMonitorDelDevice(priv->mon, watchdog->info.alias); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + if (ret == 0) { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { + qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL); + ret = qemuDomainRemoveWatchdog(driver, vm, watchdog); + } + } + qemuDomainResetDeviceRemoval(vm); + + return ret; +} + + int qemuDomainDetachNetDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index a9dfd8f7a..3455832d6 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -122,6 +122,9 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainShmemDefPtr dev); +int qemuDomainDetachWatchdog(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainWatchdogDefPtr watchdog); int qemuDomainAttachLease(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainLeaseDefPtr lease); diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index b02ae8034..df28a7fbd 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -155,6 +155,9 @@ testQemuHotplugDetach(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SHMEM: ret = qemuDomainDetachShmemDevice(&driver, vm, dev->data.shmem); break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + ret = qemuDomainDetachWatchdog(&driver, vm, dev->data.watchdog); + break; default: VIR_TEST_VERBOSE("device type '%s' cannot be detached\n", virDomainDeviceTypeToString(dev->type)); @@ -818,9 +821,11 @@ mymain(void) "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); - DO_TEST_ATTACH("base-live", "watchdog", false, false, + DO_TEST_ATTACH("base-live", "watchdog", false, true, "watchdog-set-action", QMP_OK, "device_add", QMP_OK); + DO_TEST_DETACH("base-live", "watchdog-full", false, false, + "device_del", QMP_OK); #define DO_TEST_CPU_GROUP(prefix, vcpus, modernhp, expectfail) \ do { \ diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml new file mode 100644 index 000000000..68bbfa87c --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml @@ -0,0 +1,4 @@ +<watchdog model='i6300esb' action='poweroff'> + <alias name='watchdog0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> +</watchdog> -- 2.13.5
On 09/27/2017 08:12 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1447169
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 67 ++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 3 + tests/qemuhotplugtest.c | 7 ++- .../qemuhotplug-watchdog-full.xml | 4 ++ 5 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
[...]
+int +qemuDomainDetachWatchdog(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainWatchdogDefPtr dev) +{ + int ret = -1; + virDomainWatchdogDefPtr watchdog = vm->def->watchdog; + qemuDomainObjPrivatePtr priv = vm->privateData; +
Similar to the hot-plug side - does hot unplug work for older qemu's?
+ /* While domains can have up to one watchdog, the one supplied by the user + * doesn't necessarily match the one domain has. Refuse to detach in such + * case. */ + if (!(watchdog && + watchdog->model == dev->model && + watchdog->action == dev->action && + virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info))) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("watchdog device not present in domain configuration")); + return -1; + } + + if (watchdog->model != VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("hotunplug of watchdog of model %s is not supported"),
"hot unplug"
+ virDomainWatchdogModelTypeToString(watchdog->model)); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &watchdog->info); + qemuDomainObjEnterMonitor(driver, vm); +
Not a problem per se, just a question - is there a need to do any sort of "watchdog-set-action" to say WATCHDOG_ACTION_NONE or RESET (since RESET is the default if not provided). I guess I'm just thinking outside the box of how someone could add a watchdog device (not w/ libvirt) without an action if they knew how and the old action would then conceivably take place. I can think of one such team that would try something like that ;-) In general - based on of course the qemuCaps question answer... Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ ret = qemuMonitorDelDevice(priv->mon, watchdog->info.alias); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + if (ret == 0) { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { + qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL); + ret = qemuDomainRemoveWatchdog(driver, vm, watchdog); + } + } + qemuDomainResetDeviceRemoval(vm); + + return ret; +} + +
[..]
participants (2)
-
John Ferlan -
Michal Privoznik