[PATCH v4 01/14] nodedev: Alter virNodeDeviceObjRemove

[John Ferlan <jferlan@xxxxxxxxxx>]

Rather than passing the object to be removed by reference, pass by value
and then let the caller decide whether or not the object should be free'd
and how to handle the logic afterwards. This includes free'ing the object
and/or setting the local variable to NULL to prevent subsequent unexpected
usage (via something like virNodeDeviceObjRemove in testNodeDeviceDestroy).

For now this function will just handle the remove of the object from the
list for which it was placed during virNodeDeviceObjAssignDef.

This essentially reverts logic from commit id '61148074' that free'd the
device entry on list, set *dev = NULL and returned. Thus fixing a bug in
node_device_hal.c/dev_refresh() which would never call dev_create(udi)
since @dev would have been set to NULL.

Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
 src/conf/virnodedeviceobj.c        | 14 ++++++--------
 src/conf/virnodedeviceobj.h        |  2 +-
 src/libvirt_private.syms           |  1 +
 src/node_device/node_device_hal.c  |  9 ++++++---
 src/node_device/node_device_udev.c |  3 ++-
 src/test/test_driver.c             |  8 ++++++--
 6 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index e78f451..fa73de1 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -301,23 +301,21 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
 
 void
 virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
-                       virNodeDeviceObjPtr *dev)
+                       virNodeDeviceObjPtr dev)
 {
     size_t i;
 
-    virNodeDeviceObjUnlock(*dev);
+    virNodeDeviceObjUnlock(dev);
 
     for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjLock(*dev);
-        if (devs->objs[i] == *dev) {
-            virNodeDeviceObjUnlock(*dev);
-            virNodeDeviceObjFree(devs->objs[i]);
-            *dev = NULL;
+        virNodeDeviceObjLock(devs->objs[i]);
+        if (devs->objs[i] == dev) {
+            virNodeDeviceObjUnlock(devs->objs[i]);
 
             VIR_DELETE_ELEMENT(devs->objs, i, devs->count);
             break;
         }
-        virNodeDeviceObjUnlock(*dev);
+        virNodeDeviceObjUnlock(devs->objs[i]);
     }
 }
 
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index 05a9d11..9bc02ee 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -58,7 +58,7 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
 
 void
 virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
-                       virNodeDeviceObjPtr *dev);
+                       virNodeDeviceObjPtr dev);
 
 int
 virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 888412a..e6ed981 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -965,6 +965,7 @@ virNetworkObjUpdateAssignDef;
 virNodeDeviceObjAssignDef;
 virNodeDeviceObjFindByName;
 virNodeDeviceObjFindBySysfsPath;
+virNodeDeviceObjFree;
 virNodeDeviceObjGetDef;
 virNodeDeviceObjGetNames;
 virNodeDeviceObjGetParentHost;
diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
index f468e42..dc9202b 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -514,14 +514,16 @@ dev_refresh(const char *udi)
         /* Simply "rediscover" device -- incrementally handling changes
          * to sub-capabilities (like net.80203) is nasty ... so avoid it.
          */
-        virNodeDeviceObjRemove(&driver->devs, &dev);
+        virNodeDeviceObjRemove(&driver->devs, dev);
     } else {
         VIR_DEBUG("no device named %s", name);
     }
     nodeDeviceUnlock();
 
-    if (dev)
+    if (dev) {
+        virNodeDeviceObjFree(dev);
         dev_create(udi);
+    }
 }
 
 static void
@@ -544,10 +546,11 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED,
     dev = virNodeDeviceObjFindByName(&driver->devs, name);
     VIR_DEBUG("%s", name);
     if (dev)
-        virNodeDeviceObjRemove(&driver->devs, &dev);
+        virNodeDeviceObjRemove(&driver->devs, dev);
     else
         VIR_DEBUG("no device named %s", name);
     nodeDeviceUnlock();
+    virNodeDeviceObjFree(dev);
 }
 
 
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 174124a..819e4e7 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1333,7 +1333,8 @@ udevRemoveOneDevice(struct udev_device *device)
 
     VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
               def->name, name);
-    virNodeDeviceObjRemove(&driver->devs, &dev);
+    virNodeDeviceObjRemove(&driver->devs, dev);
+    virNodeDeviceObjFree(dev);
 
     if (event)
         virObjectEventStateQueue(driver->nodeDeviceEventState, event);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 54e252c..04887e0 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4530,7 +4530,9 @@ testDestroyVport(testDriverPtr privconn,
                                            VIR_NODE_DEVICE_EVENT_DELETED,
                                            0);
 
-    virNodeDeviceObjRemove(&privconn->devs, &obj);
+    virNodeDeviceObjRemove(&privconn->devs, obj);
+    virNodeDeviceObjFree(obj);
+    obj = NULL;
 
     ret = 0;
 
@@ -5624,7 +5626,9 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
                                            0);
 
     virNodeDeviceObjLock(obj);
-    virNodeDeviceObjRemove(&driver->devs, &obj);
+    virNodeDeviceObjRemove(&driver->devs, obj);
+    virNodeDeviceObjFree(obj);
+    obj = NULL;
 
  out:
     if (obj)
-- 
2.9.4

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