[libvirt] [PATCH 4/4] qemu: hot-unplug of watchdog
John Ferlan
jferlan at redhat.com
Tue Sep 12 14:29:49 UTC 2017
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 at 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...
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?
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.
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("device not present in domain configuration"));
s/device/watchdog device/
> + return -1;
> + }
> +
> + watchdog = vm->def->watchdog;
Now unnecessary...
> +
> + qemuDomainMarkDeviceForRemoval(vm, &watchdog->info);
> + qemuDomainObjEnterMonitor(driver, vm);
> +
> + ret = qemuMonitorDelDevice(priv->mon, watchdog->info.alias);
So if this succeeds and the following fails, then we have no watchdog in
the domain, but then again we probably have no domain if the following
fails... Of course this is similar to what the shmem code is doing, so
> +
> + 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..28827b58a
> --- /dev/null
> +++ b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
> @@ -0,0 +1,3 @@
> +<watchdog model='ib700' action='poweroff'>
> + <alias name='watchdog0'/>
> +</watchdog>
>
Like I noted above, I don't think supplying the alias is correct or
necessary.
John
More information about the libvir-list
mailing list