Re: [PATCH 21/21] qemu_hotplug: delay sending DEVICE_REMOVED event until after *all* teardown

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/22/19 8:51 AM, Peter Krempa wrote:
On Thu, Mar 21, 2019 at 18:29:01 -0400, Laine Stump wrote:
The VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event is sent after qemu has
responded to a device_del command with a DEVICE_DELETED event. Before
queuing the event, *some* of the final teardown of the device's
trappings in libvirt is done, but not *all* of it. As a result, an
application may receive and process the DEVICE_REMOVED event before
libvirt has really finished with it.

Usually this doesn't cause a problem, but it can - in the case of the
bug report referenced below, vdsm is assigning a PCI device to a guest
with managed='no', using livirt's virNodeDeviceDetachFlags() and
virNodeDeviceReAttach() APIs. Immediately after receiving a
DEVICE_REMOVED event from libvirt signalling that the device had been
successfully unplugged, vdsm would cal virNodeDeviceReAttach() to
unbind the device from vfio-pci and rebind it to the host driverm but
because the event was received before libvirt had completely finished
processing the removal, that device was still on the "activeDevs"
list, and so virNodeDeviceReAttach() failed.
So between the lines this reads as if qemuDomainRemoveHostDevice is
broken. I agree though that fixing everything systematically is better.

Experimentation with additional debug logs proved that libvirt would
always end up dispatching the DEVICE_REMOVED event before it had
removed the device from activeDevs (with a *much* greater difference
with managed='yes', since in that case the re-binding of the device
occurred after queuing the device).

Although the case of hostdev devices is the most extreme (since there
is so much involved in tearing down the device), *all* device types
suffer from the same problem - the DEVICE_REMOVED event is queued very
early in the qemuDomainRemove*Device() function for all of them,
resulting in a possibility of any application receiving the event
before libvirt has really finished with the device.

The solution is to save the device's alias (which is the only piece of
info from the device object that is needed for the event) at the
beginning of processing the device removal, and then queue the event
as a final act before returning. Since all of the
qemuDomainRemove*Device() functions (except
qemuDomainRemoveChrDevice()) are now called exclusively from
qemuDomainRemoveDevice() (which selects which of the subordinates to
call in a switch statement based on the type of device), the shortest
route to a solution is to doing the saving of alias, and later
queueing of the event, in the higher level qemuDomainRemoveDevice(),
and just completely remove the event-related code from all the
subordinate functions.

The single exception to this, as mentioned before, is
qemuDomainRemoveChrDevice(), which is still called from somewhere
other than qemuDomainRemoveDevice() (and has a separate arg used to
trigger different behavior when the chr device has targetType ==
GUESTFWD), so it must keep its original behavior intact, and must be
treated differently by qemuDomainRemoveDevice() (similar to the way
that qemuDomainDetachDeviceLive() treats chr and lease devices
differently from all the others).

Resolves: https://bugzilla.redhat.com/1658198

Signed-off-by: Laine Stump <laine@xxxxxxxxx>
---
  src/qemu/qemu_hotplug.c | 154 ++++++++++++++++++++--------------------
  1 file changed, 78 insertions(+), 76 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index de7a7a2c95..43cc3a314d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4501,7 +4501,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
  {
      qemuHotplugDiskSourceDataPtr diskbackend = NULL;
      virDomainDeviceDef dev;
-    virObjectEventPtr event;
      size_t i;
      qemuDomainObjPrivatePtr priv = vm->privateData;
      int ret = -1;
@@ -4529,9 +4528,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
virDomainAuditDisk(vm, disk->src, NULL, "detach", true); - event = virDomainEventDeviceRemovedNewFromObj(vm, disk->info.alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
-
      qemuDomainReleaseDeviceAddress(vm, &disk->info, virDomainDiskGetSource(disk));
/* tear down disk security access */
@@ -4555,19 +4551,14 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
static int
-qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver,
-                                 virDomainObjPtr vm,
+qemuDomainRemoveControllerDevice(virDomainObjPtr vm,
                                   virDomainControllerDefPtr controller)
  {
-    virObjectEventPtr event;
      size_t i;
VIR_DEBUG("Removing controller %s from domain %p %s",
                controller->info.alias, vm, vm->def->name);
- event = virDomainEventDeviceRemovedNewFromObj(vm, controller->info.alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
-
      for (i = 0; i < vm->def->ncontrollers; i++) {
          if (vm->def->controllers[i] == controller) {
              virDomainControllerRemove(vm->def, i);
@@ -4589,7 +4580,6 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
      qemuDomainObjPrivatePtr priv = vm->privateData;
      unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def);
      unsigned long long newmem = oldmem - mem->size;
-    virObjectEventPtr event;
      char *backendAlias = NULL;
      int rc;
      int idx;
@@ -4611,9 +4601,6 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
      if (rc < 0)
          return -1;
- event = virDomainEventDeviceRemovedNewFromObj(vm, mem->info.alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
-
      if ((idx = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
          virDomainMemoryRemove(vm->def, idx);
@@ -4693,7 +4680,6 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
  {
      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
      virDomainNetDefPtr net = NULL;
-    virObjectEventPtr event;
      size_t i;
      int ret = -1;
      qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -4737,9 +4723,6 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
              goto cleanup;
      }
- event = virDomainEventDeviceRemovedNewFromObj(vm, hostdev->info->alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
-
      if (hostdev->parent.type == VIR_DOMAIN_DEVICE_NET) {
          net = hostdev->parent.data.net;
@@ -4818,7 +4801,6 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
  {
      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
      qemuDomainObjPrivatePtr priv = vm->privateData;
-    virObjectEventPtr event;
      char *hostnet_name = NULL;
      char *charDevAlias = NULL;
      size_t i;
@@ -4863,9 +4845,6 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
virDomainAuditNet(vm, net, NULL, "detach", true); - event = virDomainEventDeviceRemovedNewFromObj(vm, net->info.alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
-
      for (i = 0; i < vm->def->nnets; i++) {
          if (vm->def->nets[i] == net) {
              virDomainNetRemove(vm->def, i);
@@ -4948,11 +4927,20 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
      if (qemuDomainNamespaceTeardownChardev(vm, chr) < 0)
          VIR_WARN("Unable to remove chr device from /dev");
+ qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL);
+    qemuDomainChrRemove(vm->def, chr);
+
+    /* NB: all other qemuDomainRemove*Device() functions omit the
+     * sending of the DEVICE_REMOVED event, because they are *only*
+     * called from qemuDomainRemoveDevice(), and that function sends
+     * the DEVICE_REMOVED event for them, this function is different -
+     * it is called from other places than just
+     * qemuDomainRemoveDevice(), so it must send the DEVICE_REMOVED
+     * event itself.
+     */
      event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias);
      virObjectEventStateQueue(driver->domainEventState, event);
- qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL);
-    qemuDomainChrRemove(vm->def, chr);
      virDomainChrDefFree(chr);
      ret = 0;
@@ -4967,7 +4955,6 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
                            virDomainObjPtr vm,
                            virDomainRNGDefPtr rng)
  {
-    virObjectEventPtr event;
      char *charAlias = NULL;
      char *objAlias = NULL;
      qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -5016,9 +5003,6 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
      if (qemuDomainNamespaceTeardownRNG(vm, rng) < 0)
          VIR_WARN("Unable to remove RNG device from /dev");
- event = virDomainEventDeviceRemovedNewFromObj(vm, rng->info.alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
-
      if ((idx = virDomainRNGFind(vm->def, rng)) >= 0)
          virDomainRNGRemove(vm->def, idx);
      qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL);
@@ -5043,7 +5027,6 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
      char *charAlias = NULL;
      char *memAlias = NULL;
      qemuDomainObjPrivatePtr priv = vm->privateData;
-    virObjectEventPtr event = NULL;
VIR_DEBUG("Removing shmem device %s from domain %p %s",
                shmem->info.alias, vm, vm->def->name);
@@ -5071,9 +5054,6 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
      if (rc < 0)
          goto cleanup;
- event = virDomainEventDeviceRemovedNewFromObj(vm, shmem->info.alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
-
      if ((idx = virDomainShmemDefFind(vm->def, shmem)) >= 0)
          virDomainShmemDefRemove(vm->def, idx);
      qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL);
@@ -5089,17 +5069,12 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
static int
-qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
-                         virDomainObjPtr vm,
+qemuDomainRemoveWatchdog(virDomainObjPtr vm,
                           virDomainWatchdogDefPtr watchdog)
  {
-    virObjectEventPtr event = NULL;
-
      VIR_DEBUG("Removing watchdog %s from domain %p %s",
                watchdog->info.alias, vm, vm->def->name);
- event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
      qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
      virDomainWatchdogDefFree(vm->def->watchdog);
      vm->def->watchdog = NULL;
@@ -5111,16 +5086,11 @@ static int
  qemuDomainRemoveInputDevice(virDomainObjPtr vm,
                              virDomainInputDefPtr dev)
  {
-    qemuDomainObjPrivatePtr priv = vm->privateData;
-    virQEMUDriverPtr driver = priv->driver;
-    virObjectEventPtr event = NULL;
      size_t i;
VIR_DEBUG("Removing input device %s from domain %p %s",
                dev->info.alias, vm, vm->def->name);
- event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
      for (i = 0; i < vm->def->ninputs; i++) {
          if (vm->def->inputs[i] == dev)
              break;
@@ -5145,15 +5115,9 @@ static int
  qemuDomainRemoveVsockDevice(virDomainObjPtr vm,
                              virDomainVsockDefPtr dev)
  {
-    qemuDomainObjPrivatePtr priv = vm->privateData;
-    virQEMUDriverPtr driver = priv->driver;
-    virObjectEventPtr event = NULL;
-
      VIR_DEBUG("Removing vsock device %s from domain %p %s",
                dev->info.alias, vm, vm->def->name);
- event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
      qemuDomainReleaseDeviceAddress(vm, &dev->info, NULL);
      virDomainVsockDefFree(vm->def->vsock);
      vm->def->vsock = NULL;
@@ -5167,7 +5131,6 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
                                 virDomainRedirdevDefPtr dev)
  {
      qemuDomainObjPrivatePtr priv = vm->privateData;
-    virObjectEventPtr event;
      char *charAlias = NULL;
      ssize_t idx;
      int ret = -1;
@@ -5192,9 +5155,6 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
virDomainAuditRedirdev(vm, dev, "detach", true); - event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias);
-    virObjectEventStateQueue(driver->domainEventState, event);
-
      if ((idx = virDomainRedirdevDefFind(vm->def, dev)) >= 0)
          virDomainRedirdevDefRemove(vm->def, idx);
      qemuDomainReleaseDeviceAddress(vm, &dev->info, NULL);
@@ -5285,50 +5245,88 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
                         virDomainObjPtr vm,
                         virDomainDeviceDefPtr dev)
  {
-    int ret = -1;
+    virDomainDeviceInfoPtr info;
+    virObjectEventPtr event;
+    VIR_AUTOFREE(char *)alias = NULL;
+
+    if (!(info = virDomainDeviceGetInfo(dev))) {
+        /*
+         * This should never happen, since all of the device types in
+         * the switch cases that end with a "break" instead of a
+         * return have a virDeviceInfo in them.
Well, but it's called prior to doing a return down below. The deduction
from the sentence is that those that use 'return' don't necessarily have
it and thus wouldn't ever be reached.


Yeah, I'm not sure what I was thinking when I wrote that comment :-/



+         */
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("device of type '%s' has no device info"),
+                       virDomainDeviceTypeToString(dev->type));
+        return -1;
I think you can change this to a best-effort scenario. Copy the alias
only if info is non-null and emit the event in the same case.


Okay, that sounds reasonable.



+    }
+
+    /*
+     * save the alias to use when sending a DEVICE_REMOVED event after
+     * all other teardown is complete
+     */
+    if (VIR_STRDUP(alias, info->alias) < 0)
+        return -1;
Also at this point 'info' should be set to NULL as any call to the
Remove function frees the definition thus info would be dangling.


Although it's not *currently* used after this point anyway, I agree that is the prudent thing to do to prevent some future maintainer from assuming they can use it.



+
      switch ((virDomainDeviceType)dev->type) {
+    case VIR_DOMAIN_DEVICE_CHR:
+        /* unlike other qemuDomainRemove*Device() functions, this one
+         * can't take advantage of any common code at the end of
+         * qemuDomainRemoveDevice(). This is because it is called
+         * directly from other places (with an extra arg that would be
+         * clumsy to add into qemuDomainRemoveDevice())
The last sentence is not necessary.


Yeah, sometimes my comments are useful when making the change, but pointless once it is made. I'll remove it.



+         */
+        return qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true);
+
+        /*
+         * all of the following qemuDomainRemove*Device() functions
+         * are (and must be) only called from this function, so any
+         * code that is common to them all can be pulled out and put
+         * at the end of this function.
Or before if it's the same prologue code.


Good point. I'll change "at the end of" to "in".



+         */
      case VIR_DOMAIN_DEVICE_DISK:
-        ret = qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk);
+        if (qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk) < 0)
+            return -1;
          break;
      case VIR_DOMAIN_DEVICE_CONTROLLER:
-        ret = qemuDomainRemoveControllerDevice(driver, vm, dev->data.controller);
+        if (qemuDomainRemoveControllerDevice(vm, dev->data.controller) < 0)
+            return -1;
          break;
      case VIR_DOMAIN_DEVICE_NET:
-        ret = qemuDomainRemoveNetDevice(driver, vm, dev->data.net);
+        if (qemuDomainRemoveNetDevice(driver, vm, dev->data.net) < 0)
+            return -1;
          break;
      case VIR_DOMAIN_DEVICE_HOSTDEV:
-        ret = qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev);
-        break;
-
-    case VIR_DOMAIN_DEVICE_CHR:
-        ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true);
+        if (qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev) < 0)
+            return -1;
          break;
      case VIR_DOMAIN_DEVICE_RNG:
-        ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng);
+        if (qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng) < 0)
+            return -1;
          break;
-
      case VIR_DOMAIN_DEVICE_MEMORY:
-        ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory);
+        if (qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory) < 0)
+            return -1;
          break;
-
      case VIR_DOMAIN_DEVICE_SHMEM:
-        ret = qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem);
+        if (qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem) < 0)
+            return -1;
          break;
-
      case VIR_DOMAIN_DEVICE_INPUT:
-        ret = qemuDomainRemoveInputDevice(vm, dev->data.input);
+        if (qemuDomainRemoveInputDevice(vm, dev->data.input) < 0)
+            return -1;
          break;
-
      case VIR_DOMAIN_DEVICE_REDIRDEV:
-        ret = qemuDomainRemoveRedirdevDevice(driver, vm, dev->data.redirdev);
+        if (qemuDomainRemoveRedirdevDevice(driver, vm, dev->data.redirdev) < 0)
+            return -1;
          break;
-
      case VIR_DOMAIN_DEVICE_WATCHDOG:
-        ret = qemuDomainRemoveWatchdog(driver, vm, dev->data.watchdog);
+        if (qemuDomainRemoveWatchdog(vm, dev->data.watchdog) < 0)
+            return -1;
          break;
-
      case VIR_DOMAIN_DEVICE_VSOCK:
-        ret = qemuDomainRemoveVsockDevice(vm, dev->data.vsock);
+        if (qemuDomainRemoveVsockDevice(vm, dev->data.vsock) < 0)
+            return -1;
          break;
case VIR_DOMAIN_DEVICE_NONE:
@@ -5350,7 +5348,11 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
                         virDomainDeviceTypeToString(dev->type));
          break;
      }
-    return ret;
+
+    event = virDomainEventDeviceRemovedNewFromObj(vm, alias);
+    virObjectEventStateQueue(driver->domainEventState, event);
Btw, this is probably a regression since we fixed locking of the
'driver' object. Until then the event would stay queued until the end of
the API.


It took me a minute to parse that. So what you mean is that the "event is dispatched to the application too early" bug was a regression resulting from a fix made to driver locking, right? That makes a lot of sense.



ACK when the alias is copied best-effort rather than reporting error and
info is set to NULL appropriately.


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux