On 09/05/2017 07:45 AM, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1447169

With this patch users can cold plug a watchdog. Things are pretty simple because a domain can have at most one watchdog device.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b7824512c..583908972 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7842,6 +7842,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainFSDefPtr fs; virDomainRedirdevDefPtr redirdev; virDomainShmemDefPtr shmem; + virDomainWatchdogDefPtr watchdog;

switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -7978,10 +7979,20 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, dev->data.shmem = NULL; break;

+ case VIR_DOMAIN_DEVICE_WATCHDOG: + watchdog = dev->data.watchdog; + if (vmdef->watchdog) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain already has a watchdog")); + return -1; + } + vmdef->watchdog = watchdog; + dev->data.watchdog = NULL;

Couldn't this just simply be: VIR_STEAL_PTR(vmdef->watchdog, dev->data.watchdog); the local @watchdog wouldn't be necessary then either... Reviewed-by: John Ferlan <jferlan@redhat.com> John

+ break; + 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:

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@redhat.com> --- 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.

+ 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. 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. 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.

+ + if (rv >= 0)

This only ever returns -1 or 0, right?

+ rv = qemuMonitorAddDevice(priv->mon, watchdogstr); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + releaseAddress = false; + goto cleanup; + } + + if (rv < 0) + goto cleanup;

Why no virDomainAuditWatchdog type code?

+ + 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. A few things to clean up before an R-B... John

"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..7e0e0a863 --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog.xml @@ -0,0 +1 @@ +<watchdog model='ib700' 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..f884eec50 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+watchdog.xml @@ -0,0 +1,55 @@ +<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='ib700' action='poweroff'> + <alias name='watchdog0'/> + </watchdog> + <memballoon model='none'> + <alias name='balloon0'/> + </memballoon> + </devices> + <seclabel type='none' model='none'/> +</domain>

Michal, the watchdog model diag288 does not support hot-plug and hot-unplug. On 09/05/2017 01:45 PM, Michal Privoznik wrote:

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@redhat.com> --- 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) + 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);

+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 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) { + 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; + + qemuDomainObjEnterMonitor(driver, vm); + + rv = qemuMonitorSetWatchdogAction(priv->mon, actualAction); + + 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 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); #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; + + 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); #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..7e0e0a863 --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog.xml @@ -0,0 +1 @@ +<watchdog model='ib700' 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..f884eec50 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+watchdog.xml @@ -0,0 +1,55 @@ +<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='ib700' action='poweroff'> + <alias name='watchdog0'/> + </watchdog> + <memballoon model='none'> + <alias name='balloon0'/> + </memballoon> + </devices> + <seclabel type='none' model='none'/> +</domain>

-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 09/12/2017 04:29 PM, John Ferlan wrote:

On 09/05/2017 07:45 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 | 61 ++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 3 ++ tests/qemuhotplugtest.c | 7 ++- .../qemuhotplug-watchdog-full.xml | 3 ++ 5 files changed, 76 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 626148dba..4c636b956 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7692,12 +7692,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 989146eb9..44472ce9f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4360,6 +4360,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",

Is "Removing watchdog watchdog0 from ..." a bit superfluous?

Perhaps just "Removing '%s' from ..."

...

+ watchdog->info.alias, vm, vm->def->name); +

This would/could be the place for the virDomainAuditWatchdog for "detach" too.

...

+ 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, @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, }

+int +qemuDomainDetachWatchdog(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainWatchdogDefPtr dev) +{ + int ret = -1; + virDomainWatchdogDefPtr watchdog;

Why not watchdog = vm->def->watchdog; here?

...

+ qemuDomainObjPrivatePtr priv = vm->privateData; + + /* While domains can have up to one watchdog, the one supplied by user

s/by/by the/

...

+ * doesn't necessarily match the one domain has. Refuse to detach in such + * case. */ + if (!(vm->def->watchdog && + STREQ_NULLABLE(dev->info.alias, + vm->def->watchdog->info.alias))) {

if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias))

Trying to think why NULLABLE would be necessary... So it seems it's required that that input XML has the alias - something not quite right with that...

Is that so? We match devices based on their alias in a lot of places. Users are expected to pass the full device XML anyway. So it's up to us how we pick the right device to be detached. For instance, when detaching a vNIC we match MAC addresses and ignore the rest, since MAC identifies the device uniquely. So for instance, if the detach XML provided by user has <bandwidth/> set but the one in domain doesn't have it, we detach the device anyway. One can argue this is a buggy behaviour. But I just don't think we should care. There's a line we have to draw between protecting users from themselves and too complex and mostly useless code. So we've documented that users are supposed to pass the device XML as is in the domain XML.

I would think for unplug there'd be something that would compare the input "model" and "action" values to whatever watchdog is currently present and if they don't match, then fail. Not sure why comparing the alias is "right". It's not something we require of other unplugs, is it?

Sort of. For instance when detaching disks we only care about the target. And for a lot of other devices we require alias too.

We just need to make sure the same "something" is being removed. Since the alias is essentially static for the single available watchdog device, then as long as the model and action are the same, we can remove; otherwise, someone has to fix the input XML to have the matching model and action.

Okay, I'll add these two checks. Michal

On 09/26/2017 12:30 PM, Daniel P. Berrange wrote:

On Tue, Sep 26, 2017 at 11:38:26AM +0200, Michal Privoznik wrote:

...

On 09/12/2017 04:29 PM, John Ferlan wrote:

...

On 09/05/2017 07:45 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 | 61 ++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 3 ++ tests/qemuhotplugtest.c | 7 ++- .../qemuhotplug-watchdog-full.xml | 3 ++ 5 files changed, 76 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 626148dba..4c636b956 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7692,12 +7692,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 989146eb9..44472ce9f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4360,6 +4360,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",

Is "Removing watchdog watchdog0 from ..." a bit superfluous?

Perhaps just "Removing '%s' from ..."

...

+ watchdog->info.alias, vm, vm->def->name); +

This would/could be the place for the virDomainAuditWatchdog for "detach" too.

...

+ 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, @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, }

+int +qemuDomainDetachWatchdog(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainWatchdogDefPtr dev) +{ + int ret = -1; + virDomainWatchdogDefPtr watchdog;

Why not watchdog = vm->def->watchdog; here?

...

+ qemuDomainObjPrivatePtr priv = vm->privateData; + + /* While domains can have up to one watchdog, the one supplied by user

s/by/by the/

...

+ * doesn't necessarily match the one domain has. Refuse to detach in such + * case. */ + if (!(vm->def->watchdog && + STREQ_NULLABLE(dev->info.alias, + vm->def->watchdog->info.alias))) {

if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias))

Trying to think why NULLABLE would be necessary... So it seems it's required that that input XML has the alias - something not quite right with that...

Is that so? We match devices based on their alias in a lot of places. Users are expected to pass the full device XML anyway. So it's up to us how we pick the right device to be detached. For instance, when detaching a vNIC we match MAC addresses and ignore the rest, since MAC identifies the device uniquely. So for instance, if the detach XML provided by user has <bandwidth/> set but the one in domain doesn't have it, we detach the device anyway. One can argue this is a buggy behaviour. But I just don't think we should care. There's a line we have to draw between protecting users from themselves and too complex and mostly useless code. So we've documented that users are supposed to pass the device XML as is in the domain XML.

We can protect ourselves from the danger of user passing incomplete device XML by not using the passed in device XML directly.

IOW, once we parse the XML to create our virDomainDevicePtr instance, lets say we want the dev->disk virDomainDiskDefPtr to unplug a disk. Don't use the the dev->disk pointer in the unplug code. Instead use the dev->disk to lookup the def->disk[N] we want from virDomaniDefPtr and then use that virDomainDiskDefPtr instead. That way our code doesn't have to be paranoid about incompletely specified device configs. This is safer because we'll inevitably forget to be paranoid enough in places which leaves us vulnerable to crashes / bugs.

Yup. That's exactly what we do. And while in fact using our own definition of the device, the one provided by user is not taken fully into consideration. Which is good. I'm just saying that to explain why I think that alias is just enough for watchdog detach. Look at my code, I'm using alias just to lookup the config stored in domain def. Michal

On 09/26/2017 01:34 PM, John Ferlan wrote:

On 09/26/2017 05:38 AM, Michal Privoznik wrote:

...

On 09/12/2017 04:29 PM, John Ferlan wrote:

...

On 09/05/2017 07:45 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 | 61 ++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 3 ++ tests/qemuhotplugtest.c | 7 ++- .../qemuhotplug-watchdog-full.xml | 3 ++ 5 files changed, 76 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 626148dba..4c636b956 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7692,12 +7692,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 989146eb9..44472ce9f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4360,6 +4360,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",

Is "Removing watchdog watchdog0 from ..." a bit superfluous?

Perhaps just "Removing '%s' from ..."

...

+ watchdog->info.alias, vm, vm->def->name); +

This would/could be the place for the virDomainAuditWatchdog for "detach" too.

...

+ 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, @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, }

+int +qemuDomainDetachWatchdog(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainWatchdogDefPtr dev) +{ + int ret = -1; + virDomainWatchdogDefPtr watchdog;

Why not watchdog = vm->def->watchdog; here?

...

+ qemuDomainObjPrivatePtr priv = vm->privateData; + + /* While domains can have up to one watchdog, the one supplied by user

s/by/by the/

...

+ * doesn't necessarily match the one domain has. Refuse to detach in such + * case. */ + if (!(vm->def->watchdog && + STREQ_NULLABLE(dev->info.alias, + vm->def->watchdog->info.alias))) {

if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias))

Trying to think why NULLABLE would be necessary... So it seems it's required that that input XML has the alias - something not quite right with that...

Is that so? We match devices based on their alias in a lot of places. Users are expected to pass the full device XML anyway. So it's up to us how we pick the right device to be detached. For instance, when detaching a vNIC we match MAC addresses and ignore the rest, since MAC identifies the device uniquely. So for instance, if the detach XML provided by user has <bandwidth/> set but the one in domain doesn't have it, we detach the device anyway. One can argue this is a buggy behaviour. But I just don't think we should care. There's a line we have to draw between protecting users from themselves and too complex and mostly useless code. So we've documented that users are supposed to pass the device XML as is in the domain XML.

Can the alias be user supplied and not be overwritten on attach? IOW: If someone supplies watchdog5 on hot plug, would supplying the same XML work on hot unplug?

No, it wouldn't work. Because there's no watchdog5. However, what you're doing is flawed according to the documentation. You're misusing the APIs, because you know how they work internally and that's okay for the testing we do when developing new feature. But in production, users are required to: 1) write the device XML 2) hotplug it 3) dump the domain XML 4) find the device they want to detach 5) provide full device XML Note, that after step 2, there are going to be differences between user supplied XML and the device XML in domain XML. For instance, user are not required to provide PCI address they want to attach the device at. Libvirt calculates its own.

Typically I pass/use the same XML for my disk/hostdev hot plug/unplug. Some of those aliases could be tricky unless of course someone's looked at the process arguments.

Since there can be one watchdog per domain at most, passing "watchdog5" as device alias would be unwise. But as I say, you're kind of misusing the attach/detach APIs. I'm doing the same thing, because we both know the internals. BTW: This approach does not work with all the devices - just try to unplug a dimm module with such 'generic' XML. Dimm's alias is checked explicitly. Anyway, to raise chances of me getting this in, I've already added checks for model & action.

And of course I now see the NULLABLE is being used because even the input XML doesn't require it, you're checking that it is provided and using that as the sole determination of matching the input XML against what's been loaded into QEMU either via cold or hot plug.

Yes.

...

...

I would think for unplug there'd be something that would compare the input "model" and "action" values to whatever watchdog is currently present and if they don't match, then fail. Not sure why comparing the alias is "right". It's not something we require of other unplugs, is it?

Sort of. For instance when detaching disks we only care about the target. And for a lot of other devices we require alias too.

But we also seem to make use of whatever alias exists for the device in many places... I looked at a few and I couldn't find a case where the alias was required on input for the detach.

/** * virDomainDetachDeviceFlags: * @domain: pointer to domain object * @xml: pointer to XML description of one device * @flags: bitwise-OR of virDomainDeviceModifyFlags * * .. * * The supplied XML description of the device should be as specific * as its definition in the domain XML. The set of attributes used * to match the device are internal to the drivers. Using a partial definition, * or attempting to detach a device that is not present in the domain XML, * but shares some specific attributes with one that is present, * may lead to unexpected results. */ I'm open for discussion though. Michal