10:53 p.m.
This series depends on "qemu: Process DEVICE_DELETED event in a separate
thread" series I sent yesterday.
Jiri Denemark (4):
qemu: Unref cfg when detaching hostdev interface
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
src/qemu/qemu_hotplug.c | 122 ++++++++++++++++++++++++------------------------
1 file changed, 60 insertions(+), 62 deletions(-)
--
1.9.3
10:53 p.m.
New subject: [libvirt] [PATCH 1/4] qemu: Unref cfg when detaching hostdev interface
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index fdede5d..b209b04 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2643,7 +2643,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
/* this function handles all hostdev and netdev cleanup */
qemuDomainRemoveHostDevice(driver, vm, virDomainNetGetActualHostdev(net));
- return;
+ goto cleanup;
}
VIR_DEBUG("Removing network interface %s from domain %p %s",
@@ -2689,6 +2689,8 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
networkReleaseActualDevice(vm->def, net);
virDomainNetDefFree(net);
+
+ cleanup:
virObjectUnref(cfg);
}
--
1.9.3
10:49 p.m.
New subject: [libvirt] [PATCH 1/4] qemu: Unref cfg when detaching hostdev interface
On Mon, Jun 02, 2014 at 14:00:57 +0200, Peter Krempa wrote:
On 05/27/14 16:53, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
> ---
> src/qemu/qemu_hotplug.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
ACK
Pushed, thanks.
Jirka
10:53 p.m.
New subject: [libvirt] [PATCH 2/4] qemu: Remove interface backend only after frontend is gone
[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(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 56 +++++++++++++++++++++++++------------------------
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b209b04..fd1f002 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2636,8 +2636,10 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
virDomainNetDefPtr net)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ qemuDomainObjPrivatePtr priv = vm->privateData;
virNetDevVPortProfilePtr vport;
virObjectEventPtr event;
+ char *hostnet_name = NULL;
size_t i;
if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
@@ -2649,6 +2651,32 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
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);
@@ -2692,6 +2720,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
cleanup:
virObjectUnref(cfg);
+ VIR_FREE(hostnet_name);
}
@@ -3379,8 +3408,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 +3445,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,21 +3462,6 @@ 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);
@@ -3469,7 +3472,6 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
cleanup:
qemuDomainResetDeviceRemoval(vm);
- VIR_FREE(hostnet_name);
return ret;
}
--
1.9.3
8:30 p.m.
New subject: [libvirt] [PATCH 2/4] qemu: Remove interface backend only after frontend is gone
On 05/27/14 16:53, 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(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 56 +++++++++++++++++++++++++------------------------
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b209b04..fd1f002 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2636,8 +2636,10 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
virDomainNetDefPtr net)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ qemuDomainObjPrivatePtr priv = vm->privateData;
virNetDevVPortProfilePtr vport;
virObjectEventPtr event;
+ char *hostnet_name = NULL;
size_t i;
if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
@@ -2649,6 +2651,32 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
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"));
This function is void, so this error won't get propagated as it was in
the original place. As this function is called from the place the above
code originally was you should probably convert it to return int and
propagate the error.
+ }
+ 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);
Peter
10:53 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: Remove disk backend only after frontend is gone
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 frontends 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(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 37 +++++++++++++------------------------
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index fd1f002..43c52bf 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -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;
+
+ 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);
@@ -2872,7 +2885,6 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
{
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
- char *drivestr = NULL;
int rc;
if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) {
@@ -2899,12 +2911,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);
@@ -2924,10 +2930,6 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
goto cleanup;
}
}
-
- /* disconnect guest from host device */
- qemuMonitorDriveDel(priv->mon, drivestr);
-
qemuDomainObjExitMonitor(driver, vm);
rc = qemuDomainWaitForDeviceRemoval(vm);
@@ -2937,7 +2939,6 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
cleanup:
qemuDomainResetDeviceRemoval(vm);
- VIR_FREE(drivestr);
return ret;
}
@@ -2948,7 +2949,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
{
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
- char *drivestr = NULL;
int rc;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
@@ -2965,12 +2965,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);
@@ -2980,10 +2974,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
NULL, "detach", false);
goto cleanup;
}
-
- /* disconnect guest from host device */
- qemuMonitorDriveDel(priv->mon, drivestr);
-
qemuDomainObjExitMonitor(driver, vm);
rc = qemuDomainWaitForDeviceRemoval(vm);
@@ -2993,7 +2983,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
cleanup:
qemuDomainResetDeviceRemoval(vm);
- VIR_FREE(drivestr);
return ret;
}
--
1.9.3
8:33 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: Remove disk backend only after frontend is gone
On 05/27/14 16:53, 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 frontends but it's still better to make our code
s/frontends/backends/ ?
consistent. And it may start making difference in case we switch to
attaching disks using -blockdev.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 37 +++++++++++++------------------------
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index fd1f002..43c52bf 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -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;
Should this be treated the same way for propagating errors as in the
previous patch? I'm not going to enforce this here as the allocaion is
unlikely to fail.
+
+ 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);
ACK if you go with this patch as-is, v2 if you upgrade the error reporting.
Peter
10:53 p.m.
New subject: [libvirt] [PATCH 4/4] qemu: Remove character device backend only after frontend is gone
In general, we should only remove a backend after seeing DEVICE_DELETED
event for a corresponding frontend.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 43c52bf..4c2f6e3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2743,16 +2743,31 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
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;
+
+ 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);
}
@@ -3589,7 +3604,6 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDefPtr vmdef = vm->def;
virDomainChrDefPtr tmpChr;
- char *charAlias = NULL;
char *devstr = NULL;
int rc;
@@ -3602,9 +3616,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);
@@ -3612,11 +3623,6 @@ 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);
@@ -3627,6 +3633,5 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
cleanup:
qemuDomainResetDeviceRemoval(vm);
VIR_FREE(devstr);
- VIR_FREE(charAlias);
return ret;
}
--
1.9.3
8:34 p.m.
New subject: [libvirt] [PATCH 4/4] qemu: Remove character device backend only after frontend is gone
On 05/27/14 16:53, 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(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 43c52bf..4c2f6e3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2743,16 +2743,31 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
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;
+
+ qemuDomainObjEnterMonitor(driver, vm);
+ if (qemuMonitorDetachCharDev(priv->mon, charAlias) < 0) {
+ qemuDomainObjExitMonitor(driver, vm);
+ goto cleanup;
+ }
+ qemuDomainObjExitMonitor(driver, vm);
+
Same conditions as in 3/4.
Peter
3750
days inactive
3756
days old
9 comments
2 participants
participants (2)
-
Jiri Denemark -
Peter Krempa