[libvirt] [PATCH v2 0/4] qemu: Some more device unplug fixes
Jiri Denemark (4): qemu: Remove interface backend only after frontend is gone qemu: Remove disk backend only after frontend is gone qemu: Remove character device backend only after frontend is gone qemu: Return in from qemuDomainRemove*Device src/qemu/qemu_hotplug.c | 162 +++++++++++++++++++++++++----------------------- 1 file changed, 84 insertions(+), 78 deletions(-) -- 2.0.0
[1] reported that we are removing network's backend too early. I didn't really get the reproducer but libvirt behaves strangely when a guest does not confirm the removal, e.g., it does not support PCI hotplug. In such case, detaching a network device leaves its frontend in place but removes the backend, which makes the device unusable for the guest. Moreover attaching the same device again succeeds and both the guest and libvirt will see two network interfaces attached but only one of them is actually working. I checked with Paolo Bonzini and he confirmed we should only remove a backend after seeing DEVICE_DELETED event for a corresponding frontend. [1] https://www.redhat.com/archives/libvir-list/2014-March/msg01740.html Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 2: - return int and propagate errors src/qemu/qemu_hotplug.c | 68 +++++++++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0acf18a..4d8d3cd 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2630,25 +2630,55 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, } -static void +static int qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainNetDefPtr net) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv = vm->privateData; virNetDevVPortProfilePtr vport; virObjectEventPtr event; + char *hostnet_name = NULL; size_t i; + int ret = -1; if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { /* this function handles all hostdev and netdev cleanup */ qemuDomainRemoveHostDevice(driver, vm, virDomainNetGetActualHostdev(net)); + ret = 0; goto cleanup; } VIR_DEBUG("Removing network interface %s from domain %p %s", net->info.alias, vm, vm->def->name); + if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { + qemuDomainObjExitMonitor(driver, vm); + virDomainAuditNet(vm, net, NULL, "detach", false); + goto cleanup; + } + } else { + int vlan; + if ((vlan = qemuDomainNetVLAN(net)) < 0 || + qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) { + if (vlan < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("unable to determine original VLAN")); + } + qemuDomainObjExitMonitor(driver, vm); + virDomainAuditNet(vm, net, NULL, "detach", false); + goto cleanup; + } + } + qemuDomainObjExitMonitor(driver, vm); + virDomainAuditNet(vm, net, NULL, "detach", true); event = virDomainEventDeviceRemovedNewFromObj(vm, net->info.alias); @@ -2689,9 +2719,12 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, networkReleaseActualDevice(vm->def, net); virDomainNetDefFree(net); + ret = 0; cleanup: virObjectUnref(cfg); + VIR_FREE(hostnet_name); + return ret; } @@ -3379,8 +3412,6 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, int detachidx, ret = -1; virDomainNetDefPtr detach = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; - int vlan; - char *hostnet_name = NULL; int rc; if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0) @@ -3418,15 +3449,6 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, } } - if ((vlan = qemuDomainNetVLAN(detach)) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("unable to determine original VLAN")); - goto cleanup; - } - - if (virAsprintf(&hostnet_name, "host%s", detach->info.alias) < 0) - goto cleanup; - qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); @@ -3444,32 +3466,16 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, goto cleanup; } } - - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { - qemuDomainObjExitMonitor(driver, vm); - virDomainAuditNet(vm, detach, NULL, "detach", false); - goto cleanup; - } - } else { - if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) { - qemuDomainObjExitMonitor(driver, vm); - virDomainAuditNet(vm, detach, NULL, "detach", false); - goto cleanup; - } - } qemuDomainObjExitMonitor(driver, vm); rc = qemuDomainWaitForDeviceRemoval(vm); if (rc == 0 || rc == 1) - qemuDomainRemoveNetDevice(driver, vm, detach); - - ret = 0; + ret = qemuDomainRemoveNetDevice(driver, vm, detach); + else + ret = 0; cleanup: qemuDomainResetDeviceRemoval(vm); - VIR_FREE(hostnet_name); return ret; } -- 2.0.0
On 06/03/14 10:22, Jiri Denemark wrote:
[1] reported that we are removing network's backend too early. I didn't really get the reproducer but libvirt behaves strangely when a guest does not confirm the removal, e.g., it does not support PCI hotplug. In such case, detaching a network device leaves its frontend in place but removes the backend, which makes the device unusable for the guest. Moreover attaching the same device again succeeds and both the guest and libvirt will see two network interfaces attached but only one of them is actually working.
I checked with Paolo Bonzini and he confirmed we should only remove a backend after seeing DEVICE_DELETED event for a corresponding frontend.
[1] https://www.redhat.com/archives/libvir-list/2014-March/msg01740.html
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 2: - return int and propagate errors
src/qemu/qemu_hotplug.c | 68 +++++++++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 31 deletions(-)
ACK Peter
In general, we should only remove a backend after seeing DEVICE_DELETED event for a corresponding frontend. This doesn't make any difference for disks attached using -drive or drive_add since QEMU automatically removes their backends but it's still better to make our code consistent. And it may start making difference in case we switch to attaching disks using -blockdev. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 2: - s/frontends/backends/ in the commit message - return int and propagate errors src/qemu/qemu_hotplug.c | 45 ++++++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4d8d3cd..35099e4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2463,7 +2463,7 @@ static bool qemuIsMultiFunctionDevice(virDomainDefPtr def, } -static void +static int qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) @@ -2472,10 +2472,23 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virObjectEventPtr event; size_t i; const char *src = virDomainDiskGetSource(disk); + qemuDomainObjPrivatePtr priv = vm->privateData; + char *drivestr; VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); + /* build the actual drive id string as the disk->info.alias doesn't + * contain the QEMU_DRIVE_HOST_PREFIX that is passed to qemu */ + if (virAsprintf(&drivestr, "%s%s", + QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + qemuMonitorDriveDel(priv->mon, drivestr); + qemuDomainObjExitMonitor(driver, vm); + VIR_FREE(drivestr); + virDomainAuditDisk(vm, src, NULL, "detach", true); event = virDomainEventDeviceRemovedNewFromObj(vm, disk->info.alias); @@ -2506,6 +2519,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, ignore_value(qemuRemoveSharedDevice(driver, &dev, vm->def->name)); virDomainDiskDefFree(disk); + return 0; } @@ -2876,7 +2890,6 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - char *drivestr = NULL; int rc; if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { @@ -2903,12 +2916,6 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, } } - /* build the actual drive id string as the disk->info.alias doesn't - * contain the QEMU_DRIVE_HOST_PREFIX that is passed to qemu */ - if (virAsprintf(&drivestr, "%s%s", - QEMU_DRIVE_HOST_PREFIX, detach->info.alias) < 0) - goto cleanup; - qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); @@ -2928,20 +2935,16 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, goto cleanup; } } - - /* disconnect guest from host device */ - qemuMonitorDriveDel(priv->mon, drivestr); - qemuDomainObjExitMonitor(driver, vm); rc = qemuDomainWaitForDeviceRemoval(vm); if (rc == 0 || rc == 1) - qemuDomainRemoveDiskDevice(driver, vm, detach); - ret = 0; + ret = qemuDomainRemoveDiskDevice(driver, vm, detach); + else + ret = 0; cleanup: qemuDomainResetDeviceRemoval(vm); - VIR_FREE(drivestr); return ret; } @@ -2952,7 +2955,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - char *drivestr = NULL; int rc; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { @@ -2969,12 +2971,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, goto cleanup; } - /* build the actual drive id string as the disk->info.alias doesn't - * contain the QEMU_DRIVE_HOST_PREFIX that is passed to qemu */ - if (virAsprintf(&drivestr, "%s%s", - QEMU_DRIVE_HOST_PREFIX, detach->info.alias) < 0) - goto cleanup; - qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); @@ -2984,10 +2980,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, NULL, "detach", false); goto cleanup; } - - /* disconnect guest from host device */ - qemuMonitorDriveDel(priv->mon, drivestr); - qemuDomainObjExitMonitor(driver, vm); rc = qemuDomainWaitForDeviceRemoval(vm); @@ -2997,7 +2989,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, cleanup: qemuDomainResetDeviceRemoval(vm); - VIR_FREE(drivestr); return ret; } -- 2.0.0
On 06/03/14 10:22, Jiri Denemark wrote:
In general, we should only remove a backend after seeing DEVICE_DELETED event for a corresponding frontend. This doesn't make any difference for disks attached using -drive or drive_add since QEMU automatically removes their backends but it's still better to make our code consistent. And it may start making difference in case we switch to attaching disks using -blockdev.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 2: - s/frontends/backends/ in the commit message - return int and propagate errors
src/qemu/qemu_hotplug.c | 45 ++++++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4d8d3cd..35099e4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c
...
@@ -2984,10 +2980,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, NULL, "detach", false); goto cleanup; } - - /* disconnect guest from host device */ - qemuMonitorDriveDel(priv->mon, drivestr); - qemuDomainObjExitMonitor(driver, vm);
rc = qemuDomainWaitForDeviceRemoval(vm);
A few lines below you forgot to propagate the returned value as you've done in qemuDomainDetachVirtioDiskDevice.
@@ -2997,7 +2989,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
cleanup: qemuDomainResetDeviceRemoval(vm); - VIR_FREE(drivestr); return ret; }
ACK with the nit resolved. Peter
In general, we should only remove a backend after seeing DEVICE_DELETED event for a corresponding frontend. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 2: - return int and propagate errors src/qemu/qemu_hotplug.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 35099e4..fde46ad 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2742,22 +2742,38 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, } -static void +static int qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr) { virObjectEventPtr event; + char *charAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; VIR_DEBUG("Removing character device %s from domain %p %s", chr->info.alias, vm, vm->def->name); + if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorDetachCharDev(priv->mon, charAlias) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm); + event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias); if (event) qemuDomainEventQueue(driver, event); qemuDomainChrRemove(vm->def, chr); virDomainChrDefFree(chr); + + cleanup: + VIR_FREE(charAlias); + return 0; } @@ -3595,7 +3611,6 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; - char *charAlias = NULL; char *devstr = NULL; int rc; @@ -3608,9 +3623,6 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) return ret; - if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) - goto cleanup; - qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info); qemuDomainObjEnterMonitor(driver, vm); @@ -3618,21 +3630,16 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, qemuDomainObjExitMonitor(driver, vm); goto cleanup; } - - if (qemuMonitorDetachCharDev(priv->mon, charAlias) < 0) { - qemuDomainObjExitMonitor(driver, vm); - goto cleanup; - } qemuDomainObjExitMonitor(driver, vm); rc = qemuDomainWaitForDeviceRemoval(vm); if (rc == 0 || rc == 1) - qemuDomainRemoveChrDevice(driver, vm, tmpChr); - ret = 0; + ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); + else + ret = 0; cleanup: qemuDomainResetDeviceRemoval(vm); VIR_FREE(devstr); - VIR_FREE(charAlias); return ret; } -- 2.0.0
On 06/03/14 10:22, Jiri Denemark wrote:
In general, we should only remove a backend after seeing DEVICE_DELETED event for a corresponding frontend.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 2: - return int and propagate errors
src/qemu/qemu_hotplug.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 35099e4..fde46ad 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2742,22 +2742,38 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, }
-static void +static int qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr) { virObjectEventPtr event; + char *charAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData;
VIR_DEBUG("Removing character device %s from domain %p %s", chr->info.alias, vm, vm->def->name);
+ if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorDetachCharDev(priv->mon, charAlias) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup;
You will return 0 even if qemuMonitorDetachCharDev fails which wouldn't happen before as qemuDomainDetachChrDevice initializes ret to -1. I think you need to add the "ret" variable to this function too.
+ } + qemuDomainObjExitMonitor(driver, vm); + event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias); if (event) qemuDomainEventQueue(driver, event);
qemuDomainChrRemove(vm->def, chr); virDomainChrDefFree(chr); + + cleanup: + VIR_FREE(charAlias); + return 0; }
ACK if you correctly propagate errors from qemuMonitorDetachCharDev. Peter
Some of the APIs already return int since they can produce errors that need to be propagated. For consistency reasons, this patch changes the rest of the APIs to also return int even though they do not fail or report any errors. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 2: - new patch src/qemu/qemu_hotplug.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index fde46ad..8c409ff 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2523,7 +2523,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } -static void +static int qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainControllerDefPtr controller) @@ -2547,6 +2547,7 @@ qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver, qemuDomainReleaseDeviceAddress(vm, &controller->info, NULL); virDomainControllerDefFree(controller); + return 0; } @@ -2575,7 +2576,7 @@ qemuDomainRemoveSCSIHostDevice(virQEMUDriverPtr driver, qemuDomainReAttachHostSCSIDevices(driver, vm->def->name, &hostdev, 1); } -static void +static int qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -2641,6 +2642,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, virDomainNetDefFree(net); } virObjectUnref(cfg); + return 0; } @@ -2659,8 +2661,8 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { /* this function handles all hostdev and netdev cleanup */ - qemuDomainRemoveHostDevice(driver, vm, virDomainNetGetActualHostdev(net)); - ret = 0; + ret = qemuDomainRemoveHostDevice(driver, vm, + virDomainNetGetActualHostdev(net)); goto cleanup; } @@ -3186,9 +3188,9 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, rc = qemuDomainWaitForDeviceRemoval(vm); if (rc == 0 || rc == 1) - qemuDomainRemoveControllerDevice(driver, vm, detach); - - ret = 0; + ret = qemuDomainRemoveControllerDevice(driver, vm, detach); + else + ret = 0; cleanup: qemuDomainResetDeviceRemoval(vm); @@ -3342,7 +3344,7 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, } else { int rc = qemuDomainWaitForDeviceRemoval(vm); if (rc == 0 || rc == 1) - qemuDomainRemoveHostDevice(driver, vm, detach); + ret = qemuDomainRemoveHostDevice(driver, vm, detach); } qemuDomainResetDeviceRemoval(vm); -- 2.0.0
On 06/03/14 10:22, Jiri Denemark wrote:
Some of the APIs already return int since they can produce errors that need to be propagated. For consistency reasons, this patch changes the rest of the APIs to also return int even though they do not fail or report any errors.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 2: - new patch
src/qemu/qemu_hotplug.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
ACK Peter
On Tue, Jun 03, 2014 at 10:22:32 +0200, Jiri Denemark wrote:
Jiri Denemark (4): qemu: Remove interface backend only after frontend is gone qemu: Remove disk backend only after frontend is gone qemu: Remove character device backend only after frontend is gone qemu: Return in from qemuDomainRemove*Device
src/qemu/qemu_hotplug.c | 162 +++++++++++++++++++++++++----------------------- 1 file changed, 84 insertions(+), 78 deletions(-)
I fixed the remaining issues and pushed this series, thanks. Jirka
participants (2)
-
Jiri Denemark -
Peter Krempa