[libvirt] [PATCH 3/4] qemu: hot-plug of watchdog
John Ferlan
jferlan at redhat.com
Tue Sep 12 14:23:23 UTC 2017
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 at 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>
>
More information about the libvir-list
mailing list