7:22 a.m.
v2 of:
https://www.redhat.com/archives/libvir-list/2019-March/msg00753.html
diff to v1:
- qemuDomainDeleteDevice now can do enter/exit monitor
- qemuMonitorDelDevice doesn't report error on DeviceNotFound
- On DeviceNotFound the caller is made wait for event, if none came
Michal Prívozník (5):
qemu_hotplug: Introduce and use qemuDomainDeleteDevice
DO NOT APPLY: Simple reproducer
qemuMonitorJSONDelDevice: Return -2 on DeviceNotFound error
qemu_hotplug: Fix a rare race condition when detaching a device twice
DO NOT APPLY: Revert simple reproducer
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_hotplug.c | 326 ++++++++++++++++-------------------
src/qemu/qemu_monitor.c | 10 ++
src/qemu/qemu_monitor_json.c | 5 +
4 files changed, 167 insertions(+), 175 deletions(-)
--
2.19.2
7:22 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemu_hotplug: Introduce and use qemuDomainDeleteDevice
The aim of this function will be to fix return value of
qemuMonitorDelDevice() in one specific case. But that is yet to
come. Right now this is nothing but a plain substitution.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 278 +++++++++++++++-------------------------
1 file changed, 103 insertions(+), 175 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f43f80668c..baa4713cf4 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -67,6 +67,44 @@ VIR_LOG_INIT("qemu.qemu_hotplug");
unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
+/**
+ * qemuDomainDeleteDevice:
+ * @vm: domain object
+ * @alias: device to remove
+ * @enterMonitor: whether do EnterMonitor/ExitMonitor too
+ *
+ * This is a wrapper over qemuMonitorDelDevice() plus
+ * (optionally) enter/exit monitor calls. This function MUST be
+ * used instead of plain qemuMonitorDelDevice().
+ *
+ * If @enterMonitor is true then the function expects @vm to be
+ * locked upon entry.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise.
+ */
+static int
+qemuDomainDeleteDevice(virDomainObjPtr vm,
+ const char *alias,
+ bool enterMonitor)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ virQEMUDriverPtr driver = priv->driver;
+ int rc;
+
+ if (enterMonitor)
+ qemuDomainObjEnterMonitor(driver, vm);
+
+ rc = qemuMonitorDelDevice(priv->mon, alias);
+
+ if (enterMonitor &&
+ qemuDomainObjExitMonitor(driver, vm) < 0)
+ rc = -1;
+
+ return rc;
+}
+
+
/**
* qemuHotplugPrepareDiskSourceAccess:
* @driver: qemu driver struct
@@ -158,7 +196,7 @@ qemuDomainAttachZPCIDevice(qemuMonitorPtr mon,
static int
-qemuDomainDetachZPCIDevice(qemuMonitorPtr mon,
+qemuDomainDetachZPCIDevice(virDomainObjPtr vm,
virDomainDeviceInfoPtr info)
{
char *zpciAlias = NULL;
@@ -167,7 +205,7 @@ qemuDomainDetachZPCIDevice(qemuMonitorPtr mon,
if (virAsprintf(&zpciAlias, "zpci%d", info->addr.pci.zpci.uid) <
0)
goto cleanup;
- if (qemuMonitorDelDevice(mon, zpciAlias) < 0)
+ if (qemuDomainDeleteDevice(vm, zpciAlias, false) < 0)
goto cleanup;
ret = 0;
@@ -195,7 +233,7 @@ qemuDomainAttachExtensionDevice(qemuMonitorPtr mon,
static int
-qemuDomainDetachExtensionDevice(qemuMonitorPtr mon,
+qemuDomainDetachExtensionDevice(virDomainObjPtr vm,
virDomainDeviceInfoPtr info)
{
if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI ||
@@ -204,7 +242,7 @@ qemuDomainDetachExtensionDevice(qemuMonitorPtr mon,
}
if (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
- return qemuDomainDetachZPCIDevice(mon, info);
+ return qemuDomainDetachZPCIDevice(vm, info);
return 0;
}
@@ -904,7 +942,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
goto exit_monitor;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
- ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info));
+ ignore_value(qemuDomainDetachExtensionDevice(vm, &disk->info));
goto exit_monitor;
}
@@ -1021,7 +1059,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
}
if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0)
- ignore_value(qemuDomainDetachExtensionDevice(priv->mon,
&controller->info));
+ ignore_value(qemuDomainDetachExtensionDevice(vm, &controller->info));
exit_monitor:
if (qemuDomainObjExitMonitor(driver, vm) < 0) {
@@ -1564,7 +1602,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
}
if (qemuMonitorAddDevice(priv->mon, nicstr) < 0) {
- ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &net->info));
+ ignore_value(qemuDomainDetachExtensionDevice(vm, &net->info));
ignore_value(qemuDomainObjExitMonitor(driver, vm));
virDomainAuditNet(vm, NULL, net, "attach", false);
goto try_remove;
@@ -1787,7 +1825,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
configfd, configfd_name)) < 0) {
- ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
+ ignore_value(qemuDomainDetachExtensionDevice(vm, hostdev->info));
}
exit_monitor:
@@ -2460,7 +2498,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
goto exit_monitor;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
- ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &rng->info));
+ ignore_value(qemuDomainDetachExtensionDevice(vm, &rng->info));
goto exit_monitor;
}
@@ -2948,7 +2986,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver,
if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, vhostfd,
vhostfdName)) < 0) {
- ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
+ ignore_value(qemuDomainDetachExtensionDevice(vm, hostdev->info));
goto exit_monitor;
}
@@ -3201,7 +3239,7 @@ qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
goto exit_monitor;
if (qemuMonitorAddDevice(priv->mon, shmstr) < 0) {
- ignore_value(qemuDomainDetachExtensionDevice(priv->mon,
&shmem->info));
+ ignore_value(qemuDomainDetachExtensionDevice(vm, &shmem->info));
goto exit_monitor;
}
@@ -3381,7 +3419,7 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
goto exit_monitor;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
- ignore_value(qemuDomainDetachExtensionDevice(priv->mon,
&input->info));
+ ignore_value(qemuDomainDetachExtensionDevice(vm, &input->info));
goto exit_monitor;
}
@@ -3466,7 +3504,7 @@ qemuDomainAttachVsockDevice(virQEMUDriverPtr driver,
goto exit_monitor;
if (qemuMonitorAddDeviceWithFd(priv->mon, devstr, vsockPriv->vhostfd, fdname)
< 0) {
- ignore_value(qemuDomainDetachExtensionDevice(priv->mon,
&vsock->info));
+ ignore_value(qemuDomainDetachExtensionDevice(vm, &vsock->info));
goto exit_monitor;
}
@@ -4819,7 +4857,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuDomainDetachExtensionDevice(priv->mon, &rng->info) < 0)
+ if (qemuDomainDetachExtensionDevice(vm, &rng->info) < 0)
rc = -1;
if (rc == 0 &&
@@ -5216,7 +5254,6 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
bool async)
{
int ret = -1;
- qemuDomainObjPrivatePtr priv = vm->privateData;
if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) {
virReportError(VIR_ERR_OPERATION_FAILED,
@@ -5228,15 +5265,11 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &detach->info);
- qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto cleanup;
- virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
+ if (qemuDomainDeleteDevice(vm, detach->info.alias, true) < 0) {
+ if (virDomainObjIsActive(vm))
+ virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
goto cleanup;
}
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto cleanup;
if (async) {
ret = 0;
@@ -5258,7 +5291,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
bool async)
{
int ret = -1;
- qemuDomainObjPrivatePtr priv = vm->privateData;
if (qemuDomainDiskBlockJobIsActive(detach))
goto cleanup;
@@ -5266,15 +5298,11 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &detach->info);
- qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto cleanup;
- virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
+ if (qemuDomainDeleteDevice(vm, detach->info.alias, true) < 0) {
+ if (virDomainObjIsActive(vm))
+ virDomainAuditDisk(vm, detach->src, NULL, "detach", false);
goto cleanup;
}
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto cleanup;
if (async) {
ret = 0;
@@ -5409,7 +5437,6 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
{
int idx, ret = -1;
virDomainControllerDefPtr detach = NULL;
- qemuDomainObjPrivatePtr priv = vm->privateData;
if ((idx = virDomainControllerFind(vm->def,
dev->data.controller->type,
@@ -5455,19 +5482,18 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &detach->info);
- qemuDomainObjEnterMonitor(driver, vm);
- if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
- qemuDomainDetachExtensionDevice(priv->mon, &detach->info) < 0) {
- goto exit_monitor;
- }
+ if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
+ int rc;
+ qemuDomainObjEnterMonitor(driver, vm);
+ rc = qemuDomainDetachExtensionDevice(vm, &detach->info);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ rc = -1;
- if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
- goto cleanup;
+ if (rc < 0)
+ goto cleanup;
}
- exit_monitor:
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ if (qemuDomainDeleteDevice(vm, detach->info.alias, true) < 0)
goto cleanup;
if (async) {
@@ -5484,14 +5510,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
}
static int
-qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachHostPCIDevice(virDomainObjPtr vm,
virDomainHostdevDefPtr detach,
bool async)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainHostdevSubsysPCIPtr pcisrc = &detach->source.subsys.u.pci;
- int ret;
if (qemuIsMultiFunctionDevice(vm->def, detach->info)) {
virReportError(VIR_ERR_OPERATION_FAILED,
@@ -5504,23 +5527,14 @@ qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, detach->info);
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- ret = -1;
-
- return ret;
+ return qemuDomainDeleteDevice(vm, detach->info->alias, true);
}
static int
-qemuDomainDetachHostUSBDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachHostUSBDevice(virDomainObjPtr vm,
virDomainHostdevDefPtr detach,
bool async)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
- int ret;
-
if (!detach->info->alias) {
virReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("device cannot be detached without a device
alias"));
@@ -5530,23 +5544,14 @@ qemuDomainDetachHostUSBDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, detach->info);
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- ret = -1;
-
- return ret;
+ return qemuDomainDeleteDevice(vm, detach->info->alias, true);
}
static int
-qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachHostSCSIDevice(virDomainObjPtr vm,
virDomainHostdevDefPtr detach,
bool async)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
- int ret = -1;
-
if (!detach->info->alias) {
virReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("device cannot be detached without a device
alias"));
@@ -5556,24 +5561,14 @@ qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, detach->info);
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
-
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- return -1;
-
- return ret;
+ return qemuDomainDeleteDevice(vm, detach->info->alias, true);
}
static int
-qemuDomainDetachSCSIVHostDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachSCSIVHostDevice(virDomainObjPtr vm,
virDomainHostdevDefPtr detach,
bool async)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
- int ret = -1;
-
if (!detach->info->alias) {
virReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("device cannot be detached without a device
alias"));
@@ -5583,25 +5578,15 @@ qemuDomainDetachSCSIVHostDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, detach->info);
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
-
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- return -1;
-
- return ret;
+ return qemuDomainDeleteDevice(vm, detach->info->alias, true);
}
static int
-qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachMediatedDevice(virDomainObjPtr vm,
virDomainHostdevDefPtr detach,
bool async)
{
- int ret = -1;
- qemuDomainObjPrivatePtr priv = vm->privateData;
-
if (!detach->info->alias) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("device cannot be detached without a device alias"));
@@ -5611,12 +5596,7 @@ qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, detach->info);
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- ret = -1;
-
- return ret;
+ return qemuDomainDeleteDevice(vm, detach->info->alias, true);
}
@@ -5633,19 +5613,19 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver,
switch (detach->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- ret = qemuDomainDetachHostPCIDevice(driver, vm, detach, async);
+ ret = qemuDomainDetachHostPCIDevice(vm, detach, async);
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
- ret = qemuDomainDetachHostUSBDevice(driver, vm, detach, async);
+ ret = qemuDomainDetachHostUSBDevice(vm, detach, async);
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
- ret = qemuDomainDetachHostSCSIDevice(driver, vm, detach, async);
+ ret = qemuDomainDetachHostSCSIDevice(vm, detach, async);
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
- ret = qemuDomainDetachSCSIVHostDevice(driver, vm, detach, async);
+ ret = qemuDomainDetachSCSIVHostDevice(vm, detach, async);
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
- ret = qemuDomainDetachMediatedDevice(driver, vm, detach, async);
+ ret = qemuDomainDetachMediatedDevice(vm, detach, async);
break;
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -5762,7 +5742,6 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
int ret = -1;
ssize_t idx = -1;
virDomainShmemDefPtr shmem = NULL;
- qemuDomainObjPrivatePtr priv = vm->privateData;
if ((idx = virDomainShmemDefFind(vm->def, dev)) < 0) {
virReportError(VIR_ERR_DEVICE_MISSING,
@@ -5791,12 +5770,7 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &shmem->info);
- qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorDelDevice(priv->mon, shmem->info.alias) < 0) {
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
- goto cleanup;
- }
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ if (qemuDomainDeleteDevice(vm, shmem->info.alias, true) < 0)
goto cleanup;
if (async) {
@@ -5821,7 +5795,6 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
{
int ret = -1;
virDomainWatchdogDefPtr watchdog = vm->def->watchdog;
- qemuDomainObjPrivatePtr priv = vm->privateData;
if (!watchdog) {
virReportError(VIR_ERR_DEVICE_MISSING, "%s",
@@ -5852,12 +5825,7 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &watchdog->info);
- qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorDelDevice(priv->mon, watchdog->info.alias) < 0) {
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
- goto cleanup;
- }
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ if (qemuDomainDeleteDevice(vm, watchdog->info.alias, true) < 0)
goto cleanup;
if (async) {
@@ -5881,7 +5849,6 @@ qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
bool async)
{
int ret = -1;
- qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainRedirdevDefPtr tmpRedirdevDef;
ssize_t idx;
@@ -5902,12 +5869,7 @@ qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &tmpRedirdevDef->info);
- qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorDelDevice(priv->mon, tmpRedirdevDef->info.alias) < 0) {
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
- goto cleanup;
- }
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ if (qemuDomainDeleteDevice(vm, tmpRedirdevDef->info.alias, true) < 0)
goto cleanup;
if (async) {
@@ -5932,7 +5894,6 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
{
int detachidx, ret = -1;
virDomainNetDefPtr detach = NULL;
- qemuDomainObjPrivatePtr priv = vm->privateData;
if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0)
goto cleanup;
@@ -5973,15 +5934,11 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &detach->info);
- qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto cleanup;
- virDomainAuditNet(vm, detach, NULL, "detach", false);
+ if (qemuDomainDeleteDevice(vm, detach->info.alias, true) < 0) {
+ if (virDomainObjIsActive(vm))
+ virDomainAuditNet(vm, detach, NULL, "detach", false);
goto cleanup;
}
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto cleanup;
if (async) {
ret = 0;
@@ -6144,20 +6101,19 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
if (!async && !guestfwd)
qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info);
- qemuDomainObjEnterMonitor(driver, vm);
if (guestfwd) {
- if (qemuMonitorRemoveNetdev(priv->mon, tmpChr->info.alias) < 0) {
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ int rc;
+ qemuDomainObjEnterMonitor(driver, vm);
+ rc = qemuMonitorRemoveNetdev(priv->mon, tmpChr->info.alias);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ rc = -1;
+
+ if (rc < 0)
goto cleanup;
- }
} else {
- if (qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) {
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ if (qemuDomainDeleteDevice(vm, tmpChr->info.alias, true) < 0)
goto cleanup;
- }
}
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto cleanup;
if (guestfwd) {
ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr, false);
@@ -6181,10 +6137,8 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
virDomainRNGDefPtr rng,
bool async)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
ssize_t idx;
virDomainRNGDefPtr tmpRNG;
- int rc;
int ret = -1;
if ((idx = virDomainRNGFind(vm->def, rng)) < 0) {
@@ -6206,9 +6160,7 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &tmpRNG->info);
- qemuDomainObjEnterMonitor(driver, vm);
- rc = qemuMonitorDelDevice(priv->mon, tmpRNG->info.alias);
- if (qemuDomainObjExitMonitor(driver, vm) || rc < 0)
+ if (qemuDomainDeleteDevice(vm, tmpRNG->info.alias, true) < 0)
goto cleanup;
if (async) {
@@ -6231,10 +6183,8 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
virDomainMemoryDefPtr memdef,
bool async)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainMemoryDefPtr mem;
int idx;
- int rc;
int ret = -1;
qemuDomainMemoryDeviceAlignSize(vm->def, memdef);
@@ -6258,9 +6208,7 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &mem->info);
- qemuDomainObjEnterMonitor(driver, vm);
- rc = qemuMonitorDelDevice(priv->mon, mem->info.alias);
- if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
+ if (qemuDomainDeleteDevice(vm, mem->info.alias, true) < 0)
goto cleanup;
if (async) {
@@ -6365,15 +6313,9 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
qemuDomainMarkDeviceAliasForRemoval(vm, vcpupriv->alias);
- qemuDomainObjEnterMonitor(driver, vm);
-
- rc = qemuMonitorDelDevice(qemuDomainGetMonitor(vm), vcpupriv->alias);
-
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto cleanup;
-
- if (rc < 0) {
- virDomainAuditVcpu(vm, oldvcpus, oldvcpus - nvcpus, "update", false);
+ if (qemuDomainDeleteDevice(vm, vcpupriv->alias, true) < 0) {
+ if (virDomainObjIsActive(vm))
+ virDomainAuditVcpu(vm, oldvcpus, oldvcpus - nvcpus, "update",
false);
goto cleanup;
}
@@ -6943,8 +6885,6 @@ qemuDomainDetachInputDevice(virDomainObjPtr vm,
virDomainInputDefPtr def,
bool async)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
- virQEMUDriverPtr driver = priv->driver;
virDomainInputDefPtr input;
int ret = -1;
int idx;
@@ -6974,12 +6914,7 @@ qemuDomainDetachInputDevice(virDomainObjPtr vm,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &input->info);
- qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorDelDevice(priv->mon, input->info.alias) < 0) {
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
- goto cleanup;
- }
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ if (qemuDomainDeleteDevice(vm, input->info.alias, true) < 0)
goto cleanup;
if (async) {
@@ -7001,8 +6936,6 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm,
virDomainVsockDefPtr dev,
bool async)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
- virQEMUDriverPtr driver = priv->driver;
virDomainVsockDefPtr vsock = vm->def->vsock;
int ret = -1;
@@ -7017,12 +6950,7 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm,
if (!async)
qemuDomainMarkDeviceForRemoval(vm, &vsock->info);
- qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorDelDevice(priv->mon, vsock->info.alias) < 0) {
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
- goto cleanup;
- }
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ if (qemuDomainDeleteDevice(vm, vsock->info.alias, true) < 0)
goto cleanup;
if (async) {
--
2.19.2
8:04 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemu_hotplug: Introduce and use qemuDomainDeleteDevice
On Thu, Mar 14, 2019 at 13:22:35 +0100, Michal Privoznik wrote:
The aim of this function will be to fix return value of
qemuMonitorDelDevice() in one specific case. But that is yet to
come. Right now this is nothing but a plain substitution.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 278 +++++++++++++++-------------------------
1 file changed, 103 insertions(+), 175 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f43f80668c..baa4713cf4 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -67,6 +67,44 @@ VIR_LOG_INIT("qemu.qemu_hotplug");
unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
+/**
+ * qemuDomainDeleteDevice:
+ * @vm: domain object
+ * @alias: device to remove
+ * @enterMonitor: whether do EnterMonitor/ExitMonitor too
I'm not persuaded about usefullnes of this argument.
The ZPCI device which is the only code path which ever sets this to
false is never detached synchronously so vm->privateData->unplug.alias
will never contain it's alias.
This means that the event will never include the ZPCI device and thus we
don't need to actually modify the returned value for it.
In this regard we can also safely ignore code paths where
qemuDomainDetachExtensionDevice is called when hotplug of a device fails
as in that case vm->privateData->unplug.alias is never set and also the
errors from that function are ignored.
That leaves two calls which are not ignored:
The call in qemuDomainRemoveRNGDevice happens in the 'Remove' handler
which is executed after the device which would set the unplug.alias was
already removed so it's not relevant
The interresting one is in qemuDomainDetachControllerDevice:
Ee call qemuDomainMarkDeviceForRemoval(vm, &detach->info)
and then qemuDomainDetachExtensionDevice(vm, &detach->info)
Since qemuDomainDetachZPCIDevice prefixes the device alias with 'zpci'
it's a completely different device and thus the waiting thing does not
make sense here as it should not modify the return value for
that as it's a different device.
Note that this still hints to a probable bug in the ZPCI extension
device code. The ZPCI extension device deletion error if the device does
not exist needs to be ignored. Your second patch will help in that, but
it will require more work. It's most probable that nobody will ever
encounter that bug though given the popularity of that platform.
This means that for the ZPCI extension device does not need this
handling as basically any errors should be ignore. Since the
'enterMonitor' flag results into an utter locking mess in the upcomming
commit which is basically useless you should drop
it, make the ZPCI function ignore error if the device is missing and use
qemuMonitorDelDevice rather than qemuDomainDeleteDevice in it.
8:56 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemu_hotplug: Introduce and use qemuDomainDeleteDevice
On 3/14/19 2:04 PM, Peter Krempa wrote:
On Thu, Mar 14, 2019 at 13:22:35 +0100, Michal Privoznik wrote:
> The aim of this function will be to fix return value of
> qemuMonitorDelDevice() in one specific case. But that is yet to
> come. Right now this is nothing but a plain substitution.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_hotplug.c | 278 +++++++++++++++-------------------------
> 1 file changed, 103 insertions(+), 175 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index f43f80668c..baa4713cf4 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -67,6 +67,44 @@ VIR_LOG_INIT("qemu.qemu_hotplug");
> unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
>
>
> +/**
> + * qemuDomainDeleteDevice:
> + * @vm: domain object
> + * @alias: device to remove
> + * @enterMonitor: whether do EnterMonitor/ExitMonitor too
I'm not persuaded about usefullnes of this argument.
<snip/>
This means that for the ZPCI extension device does not need this
handling as basically any errors should be ignore. Since the
'enterMonitor' flag results into an utter locking mess in the upcomming
commit which is basically useless you should drop
it, make the ZPCI function ignore error if the device is missing and use
qemuMonitorDelDevice rather than qemuDomainDeleteDevice in it.
Fair enough.
Michal
7:22 a.m.
New subject: [libvirt] [PATCH v2 2/5] DO NOT APPLY: Simple reproducer
https://bugzilla.redhat.com/show_bug.cgi?id=1623389
Steps to reproduce:
1) cat shmem.xml
<shmem name='my_shmem0'>
<model type='ivshmem-plain'/>
<size unit='M'>4</size>
<alias name='ua-123'/>
</shmem>
2) virsh attach-device vm1 shmem.xml
3) virsh detach-device-alias vm1 ua-123; virsh detach-device vm1 shmem.xml
4) observe that the device is still in the domain:
virsh dumpxml vm1
<shmem name='my_shmem0'>
<model type='ivshmem-plain'/>
<size unit='M'>4</size>
<alias name='ua-123'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x0a' function='0x0'/>
</shmem>
5) But qemu has the device no more:
virsh detach-device-alias vm1 ua-123
error: Failed to detach device with alias ua-123
error: internal error: unable to execute QEMU command 'device_del': Device
'ua-123' not found
This reproducer is to make sure that DELETE_DEVICE event arrives
while monitor is unlocked. It is very hard to time qemu and
libvirt so that the event comes exactly at the time when
detach-device from 3) is doing the monitor call. Simulate this by
unlocking monitor, waiting a few seconds and locking the monitor
again.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index baa4713cf4..59a5d21a5b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -96,6 +96,11 @@ qemuDomainDeleteDevice(virDomainObjPtr vm,
qemuDomainObjEnterMonitor(driver, vm);
rc = qemuMonitorDelDevice(priv->mon, alias);
+ if (rc < 0) {
+ virObjectUnlock(priv->mon);
+ sleep(10);
+ virObjectLock(priv->mon);
+ }
if (enterMonitor &&
qemuDomainObjExitMonitor(driver, vm) < 0)
--
2.19.2
7:22 a.m.
New subject: [libvirt] [PATCH v2 3/5] qemuMonitorJSONDelDevice: Return -2 on DeviceNotFound error
A caller might be interested in differentiating the cause for
error, especially if DeviceNotFound error occurred.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_monitor.c | 10 ++++++++++
src/qemu/qemu_monitor_json.c | 5 +++++
2 files changed, 15 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 8bd4d4d761..5602a20ee4 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3008,6 +3008,16 @@ qemuMonitorDriveDel(qemuMonitorPtr mon,
}
+/**
+ * @mon: monitor object
+ * @devalias: alias of the device to detach
+ *
+ * Sends device detach request to qemu.
+ *
+ * Returns: 0 on success,
+ * -2 if DeviceNotFound error encountered
+ * -1 otherwise
+ */
int
qemuMonitorDelDevice(qemuMonitorPtr mon,
const char *devalias)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 0236323a51..5c16e1f3a1 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -4191,6 +4191,11 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon,
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
goto cleanup;
+ if (qemuMonitorJSONHasError(reply, "DeviceNotFound")) {
+ ret = -2;
+ goto cleanup;
+ }
+
if (qemuMonitorJSONCheckError(cmd, reply) < 0)
goto cleanup;
--
2.19.2
8:06 a.m.
New subject: [libvirt] [PATCH v2 3/5] qemuMonitorJSONDelDevice: Return -2 on DeviceNotFound error
On Thu, Mar 14, 2019 at 13:22:37 +0100, Michal Privoznik wrote:
A caller might be interested in differentiating the cause for
error, especially if DeviceNotFound error occurred.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_monitor.c | 10 ++++++++++
src/qemu/qemu_monitor_json.c | 5 +++++
2 files changed, 15 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 8bd4d4d761..5602a20ee4 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3008,6 +3008,16 @@ qemuMonitorDriveDel(qemuMonitorPtr mon,
}
+/**
+ * @mon: monitor object
+ * @devalias: alias of the device to detach
+ *
+ * Sends device detach request to qemu.
+ *
+ * Returns: 0 on success,
+ * -2 if DeviceNotFound error encountered
+ * -1 otherwise
+ */
int
qemuMonitorDelDevice(qemuMonitorPtr mon,
const char *devalias)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 0236323a51..5c16e1f3a1 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -4191,6 +4191,11 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon,
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
goto cleanup;
+ if (qemuMonitorJSONHasError(reply, "DeviceNotFound")) {
+ ret = -2;
+ goto cleanup;
With this patch you'll stop reporting the error. Given that callers
currently jump to an error label this would make us return error
without setting the error object.
Callers need to be fixed that in case when they choose not to ignore the
error they report some error.
8:56 a.m.
New subject: [libvirt] [PATCH v2 3/5] qemuMonitorJSONDelDevice: Return -2 on DeviceNotFound error
On 3/14/19 2:06 PM, Peter Krempa wrote:
On Thu, Mar 14, 2019 at 13:22:37 +0100, Michal Privoznik wrote:
> A caller might be interested in differentiating the cause for
> error, especially if DeviceNotFound error occurred.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_monitor.c | 10 ++++++++++
> src/qemu/qemu_monitor_json.c | 5 +++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 8bd4d4d761..5602a20ee4 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3008,6 +3008,16 @@ qemuMonitorDriveDel(qemuMonitorPtr mon,
> }
>
>
> +/**
> + * @mon: monitor object
> + * @devalias: alias of the device to detach
> + *
> + * Sends device detach request to qemu.
> + *
> + * Returns: 0 on success,
> + * -2 if DeviceNotFound error encountered
> + * -1 otherwise
> + */
> int
> qemuMonitorDelDevice(qemuMonitorPtr mon,
> const char *devalias)
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 0236323a51..5c16e1f3a1 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4191,6 +4191,11 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon,
> if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> goto cleanup;
>
> + if (qemuMonitorJSONHasError(reply, "DeviceNotFound")) {
> + ret = -2;
> + goto cleanup;
With this patch you'll stop reporting the error. Given that callers
currently jump to an error label this would make us return error
without setting the error object.
Callers need to be fixed that in case when they choose not to ignore the
error they report some error.
You mean callers after I do what you suggest in 1/5 (where zpci will
call qemuMonitorDelDevice() directly)? Because after 4/5 there is only
one caller which doesn't want the error to be reported. And I guess you
did not mean to add some code here only to remove it in 4/5, did you?
Michal
9:16 a.m.
New subject: [libvirt] [PATCH v2 3/5] qemuMonitorJSONDelDevice: Return -2 on DeviceNotFound error
On Thu, Mar 14, 2019 at 14:56:49 +0100, Michal Privoznik wrote:
On 3/14/19 2:06 PM, Peter Krempa wrote:
> On Thu, Mar 14, 2019 at 13:22:37 +0100, Michal Privoznik wrote:
[...]
> Callers need to be fixed that in case when they choose not to
ignore the
> error they report some error.
>
You mean callers after I do what you suggest in 1/5 (where zpci will call
qemuMonitorDelDevice() directly)? Because after 4/5 there is only one caller
which doesn't want the error to be reported. And I guess you did not mean to
add some code here only to remove it in 4/5, did you?
I still consider the case when 'device_del' fails but the event is not
delivered to be a reporting-worthy problem due to a possible race when
deleting backends of the removed device.
9:54 a.m.
New subject: [libvirt] [PATCH v2 3/5] qemuMonitorJSONDelDevice: Return -2 on DeviceNotFound error
On Thu, Mar 14, 2019 at 15:16:56 +0100, Peter Krempa wrote:
On Thu, Mar 14, 2019 at 14:56:49 +0100, Michal Privoznik wrote:
> On 3/14/19 2:06 PM, Peter Krempa wrote:
> > On Thu, Mar 14, 2019 at 13:22:37 +0100, Michal Privoznik wrote:
[...]
> > Callers need to be fixed that in case when they choose not to ignore the
> > error they report some error.
> >
>
> You mean callers after I do what you suggest in 1/5 (where zpci will call
> qemuMonitorDelDevice() directly)? Because after 4/5 there is only one caller
> which doesn't want the error to be reported. And I guess you did not mean to
> add some code here only to remove it in 4/5, did you?
I still consider the case when 'device_del' fails but the event is not
delivered to be a reporting-worthy problem due to a possible race when
deleting backends of the removed device.
Okay, this can't happen as I've figured out, so you are right in the
regard that you'd need to add code which would be deleted later.
ACK as I don't have a better option as the ZPCI stuff will also require
this functionality.
7:22 a.m.
New subject: [libvirt] [PATCH v2 4/5] qemu_hotplug: Fix a rare race condition when detaching a device twice
https://bugzilla.redhat.com/show_bug.cgi?id=1623389
If a device is detached twice from the same domain the following
race condition may happen:
1) The first DetachDevice() call will issue "device_del" on qemu
monitor, but since the DEVICE_DELETED event did not arrive in
time, the API ends claiming "Device detach request sent
successfully".
2) The second DetachDevice() therefore still find the device in
the domain and thus proceeds to detaching it again. It calls
EnterMonitor() and qemuMonitorSend() trying to issue "device_del"
command again. This gets both domain lock and monitor lock
released.
3) At this point, qemu sends us the DEVICE_DELETED event which is
going to be handled by the event loop which ends up calling
qemuDomainSignalDeviceRemoval() to determine who is going to
remove the device from domain definition. Whether it is the
caller that marked the device for removal or whether it is going
to be the event processing thread.
4) Because the device was marked for removal,
qemuDomainSignalDeviceRemoval() returns true, which means the
event is to be processed by the thread that has marked the device
for removal (and is currently still trying to issue "device_del"
command)
5) The thread finally issues the "device_del" command, which
fails (obviously) and therefore it calls
qemuDomainResetDeviceRemoval() to reset the device marking and
quits immediately after, NOT removing any device from the domain
definition.
At this point, the device is still present in the domain
definition but doesn't exist in qemu anymore. Worse, there is no
way to remove it from the domain definition.
Solution is to note down that we've seen the event and if the
second "device_del" fails, not take it as a failure but carry on
with the usual execution.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_hotplug.c | 52 +++++++++++++++++++++++++++++++++++++++--
2 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 9f468e5661..fb361515ba 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -218,6 +218,7 @@ typedef qemuDomainUnpluggingDevice *qemuDomainUnpluggingDevicePtr;
struct _qemuDomainUnpluggingDevice {
const char *alias;
qemuDomainUnpluggingDeviceStatus status;
+ bool eventSeen; /* True if DEVICE_DELETED event arrived. */
};
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 59a5d21a5b..6b5b37bda5 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -67,6 +67,9 @@ VIR_LOG_INIT("qemu.qemu_hotplug");
unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
+static void
+qemuDomainResetDeviceRemoval(virDomainObjPtr vm);
+
/**
* qemuDomainDeleteDevice:
* @vm: domain object
@@ -103,8 +106,51 @@ qemuDomainDeleteDevice(virDomainObjPtr vm,
}
if (enterMonitor &&
- qemuDomainObjExitMonitor(driver, vm) < 0)
- rc = -1;
+ qemuDomainObjExitMonitor(driver, vm) < 0) {
+ /* Domain is no longer running. No cleanup needed. */
+ return -1;
+ }
+
+ if (rc < 0) {
+ bool eventSeen;
+
+ /* Deleting device failed. Let's check if DEVICE_DELETED
+ * even arrived. If it did, we need to claim success to
+ * make the caller remove device from domain XML. */
+ if (enterMonitor) {
+ /* Here @vm is locked again. It's safe to access
+ * private data directly. */
+ } else {
+ /* Here @vm is not locked. Do some locking magic to
+ * be able to access private data. It is safe to lock
+ * and unlock both @mon and @vm here because:
+ * a) qemuDomainObjEnterMonitor() ensures @mon is ref()'d
+ * b) The API that is calling us ensures that @vm is ref()'d
+ */
+ virObjectUnlock(priv->mon);
+ virObjectLock(vm);
+ virObjectLock(priv->mon);
+ }
+
+ eventSeen = priv->unplug.eventSeen;
+ if (eventSeen) {
+ /* The event arrived. Return success. */
+ VIR_DEBUG("Detaching of device %s failed, but event arrived",
alias);
+ qemuDomainResetDeviceRemoval(vm);
+ rc = 0;
+ } else if (rc == -2) {
+ /* The device does not exist in qemu, but it still
+ * exists in libvirt. Claim success to make caller
+ * qemuDomainWaitForDeviceRemoval(). Otherwise if
+ * domain XML is queried right after detach API the
+ * device would still be there. */
+ VIR_DEBUG("Detaching of device %s failed and no event arrived",
alias);
+ rc = 0;
+ }
+
+ if (!enterMonitor)
+ virObjectUnlock(vm);
+ }
return rc;
}
@@ -5186,6 +5232,7 @@ qemuDomainResetDeviceRemoval(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
priv->unplug.alias = NULL;
+ priv->unplug.eventSeen = false;
}
/* Returns:
@@ -5245,6 +5292,7 @@ qemuDomainSignalDeviceRemoval(virDomainObjPtr vm,
VIR_DEBUG("Removal of device '%s' continues in waiting thread",
devAlias);
qemuDomainResetDeviceRemoval(vm);
priv->unplug.status = status;
+ priv->unplug.eventSeen = true;
virDomainObjBroadcast(vm);
return true;
}
--
2.19.2
8:18 a.m.
New subject: [libvirt] [PATCH v2 4/5] qemu_hotplug: Fix a rare race condition when detaching a device twice
On Thu, Mar 14, 2019 at 13:22:38 +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1623389
If a device is detached twice from the same domain the following
race condition may happen:
1) The first DetachDevice() call will issue "device_del" on qemu
monitor, but since the DEVICE_DELETED event did not arrive in
time, the API ends claiming "Device detach request sent
successfully".
2) The second DetachDevice() therefore still find the device in
the domain and thus proceeds to detaching it again. It calls
EnterMonitor() and qemuMonitorSend() trying to issue "device_del"
command again. This gets both domain lock and monitor lock
released.
3) At this point, qemu sends us the DEVICE_DELETED event which is
going to be handled by the event loop which ends up calling
qemuDomainSignalDeviceRemoval() to determine who is going to
remove the device from domain definition. Whether it is the
caller that marked the device for removal or whether it is going
to be the event processing thread.
4) Because the device was marked for removal,
qemuDomainSignalDeviceRemoval() returns true, which means the
event is to be processed by the thread that has marked the device
for removal (and is currently still trying to issue "device_del"
command)
5) The thread finally issues the "device_del" command, which
fails (obviously) and therefore it calls
qemuDomainResetDeviceRemoval() to reset the device marking and
quits immediately after, NOT removing any device from the domain
definition.
At this point, the device is still present in the domain
definition but doesn't exist in qemu anymore. Worse, there is no
way to remove it from the domain definition.
Solution is to note down that we've seen the event and if the
second "device_del" fails, not take it as a failure but carry on
with the usual execution.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_hotplug.c | 52 +++++++++++++++++++++++++++++++++++++++--
2 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 9f468e5661..fb361515ba 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -218,6 +218,7 @@ typedef qemuDomainUnpluggingDevice *qemuDomainUnpluggingDevicePtr;
struct _qemuDomainUnpluggingDevice {
const char *alias;
qemuDomainUnpluggingDeviceStatus status;
+ bool eventSeen; /* True if DEVICE_DELETED event arrived. */
};
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 59a5d21a5b..6b5b37bda5 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -67,6 +67,9 @@ VIR_LOG_INIT("qemu.qemu_hotplug");
unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
+static void
+qemuDomainResetDeviceRemoval(virDomainObjPtr vm);
+
/**
* qemuDomainDeleteDevice:
* @vm: domain object
@@ -103,8 +106,51 @@ qemuDomainDeleteDevice(virDomainObjPtr vm,
}
if (enterMonitor &&
- qemuDomainObjExitMonitor(driver, vm) < 0)
- rc = -1;
+ qemuDomainObjExitMonitor(driver, vm) < 0) {
+ /* Domain is no longer running. No cleanup needed. */
+ return -1;
+ }
+
+ if (rc < 0) {
+ bool eventSeen;
+
+ /* Deleting device failed. Let's check if DEVICE_DELETED
+ * even arrived. If it did, we need to claim success to
+ * make the caller remove device from domain XML. */
+ if (enterMonitor) {
+ /* Here @vm is locked again. It's safe to access
+ * private data directly. */
+ } else {
+ /* Here @vm is not locked. Do some locking magic to
+ * be able to access private data. It is safe to lock
+ * and unlock both @mon and @vm here because:
+ * a) qemuDomainObjEnterMonitor() ensures @mon is ref()'d
+ * b) The API that is calling us ensures that @vm is ref()'d
+ */
As I've said in review for patch 1. This spiel is not necessary and only
relevant in the ZPCI extension device case which will not be the device
which the event is meant for.
+ virObjectUnlock(priv->mon);
+ virObjectLock(vm);
+ virObjectLock(priv->mon);
+ }
+
+ eventSeen = priv->unplug.eventSeen;
Saving this into a temporary variable used only here does not make much
sense.
+ if (eventSeen) {
+ /* The event arrived. Return success. */
+ VIR_DEBUG("Detaching of device %s failed, but event arrived",
alias);
+ qemuDomainResetDeviceRemoval(vm);
+ rc = 0;
+ } else if (rc == -2) {
+ /* The device does not exist in qemu, but it still
+ * exists in libvirt. Claim success to make caller
+ * qemuDomainWaitForDeviceRemoval(). Otherwise if
+ * domain XML is queried right after detach API the
+ * device would still be there. */
+ VIR_DEBUG("Detaching of device %s failed and no event arrived",
alias);
+ rc = 0;
How can this be considered success? Also this introduces a possible
regression. The DEVICE_DELETED event should be fired only after the
device was entirely unplugged. Claiming success before seeing the event
can lead to another race when qemu deleted the device from the internal
list so that 'device_del' does not see it any more but did not finish
cleanup fully.
We need to start the '*Remove' handler only after the DEVICE_DELETED
event was received.
+ }
+
+ if (!enterMonitor)
+ virObjectUnlock(vm);
+ }
return rc;
}
@@ -5186,6 +5232,7 @@ qemuDomainResetDeviceRemoval(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
priv->unplug.alias = NULL;
+ priv->unplug.eventSeen = false;
}
/* Returns:
@@ -5245,6 +5292,7 @@ qemuDomainSignalDeviceRemoval(virDomainObjPtr vm,
VIR_DEBUG("Removal of device '%s' continues in waiting
thread", devAlias);
qemuDomainResetDeviceRemoval(vm);
priv->unplug.status = status;
+ priv->unplug.eventSeen = true;
virDomainObjBroadcast(vm);
return true;
}
--
2.19.2
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:56 a.m.
New subject: [libvirt] [PATCH v2 4/5] qemu_hotplug: Fix a rare race condition when detaching a device twice
On 3/14/19 2:18 PM, Peter Krempa wrote:
On Thu, Mar 14, 2019 at 13:22:38 +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1623389
>
> If a device is detached twice from the same domain the following
> race condition may happen:
>
> 1) The first DetachDevice() call will issue "device_del" on qemu
> monitor, but since the DEVICE_DELETED event did not arrive in
> time, the API ends claiming "Device detach request sent
> successfully".
>
> 2) The second DetachDevice() therefore still find the device in
> the domain and thus proceeds to detaching it again. It calls
> EnterMonitor() and qemuMonitorSend() trying to issue "device_del"
> command again. This gets both domain lock and monitor lock
> released.
>
> 3) At this point, qemu sends us the DEVICE_DELETED event which is
> going to be handled by the event loop which ends up calling
> qemuDomainSignalDeviceRemoval() to determine who is going to
> remove the device from domain definition. Whether it is the
> caller that marked the device for removal or whether it is going
> to be the event processing thread.
>
> 4) Because the device was marked for removal,
> qemuDomainSignalDeviceRemoval() returns true, which means the
> event is to be processed by the thread that has marked the device
> for removal (and is currently still trying to issue "device_del"
> command)
>
> 5) The thread finally issues the "device_del" command, which
> fails (obviously) and therefore it calls
> qemuDomainResetDeviceRemoval() to reset the device marking and
> quits immediately after, NOT removing any device from the domain
> definition.
>
> At this point, the device is still present in the domain
> definition but doesn't exist in qemu anymore. Worse, there is no
> way to remove it from the domain definition.
>
> Solution is to note down that we've seen the event and if the
> second "device_del" fails, not take it as a failure but carry on
> with the usual execution.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_domain.h | 1 +
> src/qemu/qemu_hotplug.c | 52 +++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 9f468e5661..fb361515ba 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -218,6 +218,7 @@ typedef qemuDomainUnpluggingDevice
*qemuDomainUnpluggingDevicePtr;
> struct _qemuDomainUnpluggingDevice {
> const char *alias;
> qemuDomainUnpluggingDeviceStatus status;
> + bool eventSeen; /* True if DEVICE_DELETED event arrived. */
> };
>
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 59a5d21a5b..6b5b37bda5 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -67,6 +67,9 @@ VIR_LOG_INIT("qemu.qemu_hotplug");
> unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
>
>
> +static void
> +qemuDomainResetDeviceRemoval(virDomainObjPtr vm);
> +
> /**
> * qemuDomainDeleteDevice:
> * @vm: domain object
> @@ -103,8 +106,51 @@ qemuDomainDeleteDevice(virDomainObjPtr vm,
> }
>
> if (enterMonitor &&
> - qemuDomainObjExitMonitor(driver, vm) < 0)
> - rc = -1;
> + qemuDomainObjExitMonitor(driver, vm) < 0) {
> + /* Domain is no longer running. No cleanup needed. */
> + return -1;
> + }
> +
> + if (rc < 0) {
> + bool eventSeen;
> +
> + /* Deleting device failed. Let's check if DEVICE_DELETED
> + * even arrived. If it did, we need to claim success to
> + * make the caller remove device from domain XML. */
> + if (enterMonitor) {
> + /* Here @vm is locked again. It's safe to access
> + * private data directly. */
> + } else {
> + /* Here @vm is not locked. Do some locking magic to
> + * be able to access private data. It is safe to lock
> + * and unlock both @mon and @vm here because:
> + * a) qemuDomainObjEnterMonitor() ensures @mon is ref()'d
> + * b) The API that is calling us ensures that @vm is ref()'d
> + */
As I've said in review for patch 1. This spiel is not necessary and only
relevant in the ZPCI extension device case which will not be the device
which the event is meant for.
Okay.
> + virObjectUnlock(priv->mon);
> + virObjectLock(vm);
> + virObjectLock(priv->mon);
> + }
> +
> + eventSeen = priv->unplug.eventSeen;
Saving this into a temporary variable used only here does not make much
sense.
> + if (eventSeen) {
> + /* The event arrived. Return success. */
> + VIR_DEBUG("Detaching of device %s failed, but event arrived",
alias);
> + qemuDomainResetDeviceRemoval(vm);
> + rc = 0;
> + } else if (rc == -2) {
> + /* The device does not exist in qemu, but it still
> + * exists in libvirt. Claim success to make caller
> + * qemuDomainWaitForDeviceRemoval(). Otherwise if
> + * domain XML is queried right after detach API the
> + * device would still be there. */
> + VIR_DEBUG("Detaching of device %s failed and no event
arrived", alias);
> + rc = 0;
How can this be considered success? Also this introduces a possible
regression. The DEVICE_DELETED event should be fired only after the
device was entirely unplugged. Claiming success before seeing the event
can lead to another race when qemu deleted the device from the internal
list so that 'device_del' does not see it any more but did not finish
cleanup fully.
We need to start the '*Remove' handler only after the DEVICE_DELETED
event was received.
I beg to differ. If we were to report error here users would see the API
failing with error "Device not found". So they'd run 'virsh dumpxml'
only to find the device there. I don't find such behaviour sane. If one
API tells me a devie is not there then another one shall not tell otherwise.
Michal
9:14 a.m.
New subject: [libvirt] [PATCH v2 4/5] qemu_hotplug: Fix a rare race condition when detaching a device twice
On Thu, Mar 14, 2019 at 14:56:48 +0100, Michal Privoznik wrote:
On 3/14/19 2:18 PM, Peter Krempa wrote:
> On Thu, Mar 14, 2019 at 13:22:38 +0100, Michal Privoznik wrote:
[...]
>
> How can this be considered success? Also this introduces a possible
> regression. The DEVICE_DELETED event should be fired only after the
> device was entirely unplugged. Claiming success before seeing the event
> can lead to another race when qemu deleted the device from the internal
> list so that 'device_del' does not see it any more but did not finish
> cleanup fully.
>
> We need to start the '*Remove' handler only after the DEVICE_DELETED
> event was received.
I beg to differ. If we were to report error here users would see the API
failing with error "Device not found". So they'd run 'virsh
dumpxml' only to
find the device there. I don't find such behaviour sane. If one API tells me
a devie is not there then another one shall not tell otherwise.
Well. The user semantics can be confusing here. What we can't allow
though is that some of the steps done in the qemuDomainRemove*Device
will fail because qemu will still have some internal reference to some
backend object. What I'd find more of a problem is that I'd try to
attach a similar device only to be told that it already exists.
9:31 a.m.
New subject: [libvirt] [PATCH v2 4/5] qemu_hotplug: Fix a rare race condition when detaching a device twice
On 3/14/19 3:14 PM, Peter Krempa wrote:
On Thu, Mar 14, 2019 at 14:56:48 +0100, Michal Privoznik wrote:
> On 3/14/19 2:18 PM, Peter Krempa wrote:
>> On Thu, Mar 14, 2019 at 13:22:38 +0100, Michal Privoznik wrote:
[...]
>>
>> How can this be considered success? Also this introduces a possible
>> regression. The DEVICE_DELETED event should be fired only after the
>> device was entirely unplugged. Claiming success before seeing the event
>> can lead to another race when qemu deleted the device from the internal
>> list so that 'device_del' does not see it any more but did not finish
>> cleanup fully.
>>
>> We need to start the '*Remove' handler only after the DEVICE_DELETED
>> event was received.
>
> I beg to differ. If we were to report error here users would see the API
> failing with error "Device not found". So they'd run 'virsh
dumpxml' only to
> find the device there. I don't find such behaviour sane. If one API tells me
> a devie is not there then another one shall not tell otherwise.
Well. The user semantics can be confusing here. What we can't allow
though is that some of the steps done in the qemuDomainRemove*Device
will fail because qemu will still have some internal reference to some
backend object.
I'm not quite sure I follow. qemuDomainRemove*Device will be run exactly
once. Not any more times. Running it more times is a problem, but I'm
failing to see how my patch allows that. Can you shed more light into
that please?
What I'd find more of a problem is that I'd try to
attach a similar device only to be told that it already exists.
I'm don't know what you mean here either. With my patches not only we
enter the wait for the event again (thus widening the window when the
event may arrive), but we are actually compliant with the detach
semantics. Let's think of an extreme case: qemu fails to deliver
DEVICE_DELETED event. With my patches you'll get:
1) virsh detach-device-alias $dom $alias
Device detach request sent successfully
2) virsh detach-device-alias $dom $alias
Device detach request sent successfully
3) virsh detach-device-alias $dom $alias
Device detach request sent successfully
...
If we were to fail, as you suggest:
1) virsh detach-device-alias $dom $alias
Device detach request sent successfully
2) virsh detach-device-alias $dom $alias
monitor error: DeviceNotFound
3) virsh detach-device-alias $dom $alias
monitor error: DeviceNotFound
Now if you run 'virsh dumpxl $dom' as 4th step (for both scenarios) the
device is still there. So how can it be in the domain XML and not found
at the same time? And if you try to attach it, everything will work:
libvirt generates a different address to plug the device to, since it
still sees the old one.
Michal
9:50 a.m.
New subject: [libvirt] [PATCH v2 4/5] qemu_hotplug: Fix a rare race condition when detaching a device twice
On Thu, Mar 14, 2019 at 15:31:48 +0100, Michal Privoznik wrote:
On 3/14/19 3:14 PM, Peter Krempa wrote:
> On Thu, Mar 14, 2019 at 14:56:48 +0100, Michal Privoznik wrote:
> > On 3/14/19 2:18 PM, Peter Krempa wrote:
> > > On Thu, Mar 14, 2019 at 13:22:38 +0100, Michal Privoznik wrote:
>
> [...]
>
> > >
> > > How can this be considered success? Also this introduces a possible
> > > regression. The DEVICE_DELETED event should be fired only after the
> > > device was entirely unplugged. Claiming success before seeing the event
> > > can lead to another race when qemu deleted the device from the internal
> > > list so that 'device_del' does not see it any more but did not
finish
> > > cleanup fully.
> > >
> > > We need to start the '*Remove' handler only after the
DEVICE_DELETED
> > > event was received.
> >
> > I beg to differ. If we were to report error here users would see the API
> > failing with error "Device not found". So they'd run 'virsh
dumpxml' only to
> > find the device there. I don't find such behaviour sane. If one API tells
me
> > a devie is not there then another one shall not tell otherwise.
>
> Well. The user semantics can be confusing here. What we can't allow
> though is that some of the steps done in the qemuDomainRemove*Device
> will fail because qemu will still have some internal reference to some
> backend object.
I'm not quite sure I follow. qemuDomainRemove*Device will be run exactly
once. Not any more times. Running it more times is a problem, but I'm
failing to see how my patch allows that. Can you shed more light into that
please?
What I've meant is that qemuDomainRemove*Device shall never be called
earlier than DEVICE_DELETED is received.
What I've forgot to take into account is that we will still call
qemuDomainWaitForDeviceRemoval (as your comment mentions).
This means that the scenario I was outlining will not happen as success
from this API does not automatically mean we'll try to delete the
backends of the device (which I didn't notice at first).
I'd probably just delete the mention of dumping XML. I think the
statement that qemuDomainWaitForDeviceRemoval should be clear enough.
7:22 a.m.
New subject: [libvirt] [PATCH v2 5/5] DO NOT APPLY: Revert simple reproducer
This reverts previous commit.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6b5b37bda5..576bf1db26 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -99,11 +99,6 @@ qemuDomainDeleteDevice(virDomainObjPtr vm,
qemuDomainObjEnterMonitor(driver, vm);
rc = qemuMonitorDelDevice(priv->mon, alias);
- if (rc < 0) {
- virObjectUnlock(priv->mon);
- sleep(10);
- virObjectLock(priv->mon);
- }
if (enterMonitor &&
qemuDomainObjExitMonitor(driver, vm) < 0) {
--
2.19.2
2080
days inactive
2080
days old
16 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa