[libvirt] [PATCH v4 00/14] Make virNodeDeviceObj and virNodeDeviceObjList private
v3 here: https://www.redhat.com/archives/libvir-list/2017-June/msg00155.html Most patches were ACK'd in v3 (some with conditions). Changes this time around are: -> Patch1: Make the call to virNodeDeviceObjFree in dev_refresh() prior to dev_create Move the virNodeDeviceObjFree in device_removed() to after the Unlock as it avoids the need for { } else { } (besides the *ObjFree checks for !dev anyway). -> Patch2: Change as requested to remove the unnecessary obj = NULL in testDestroyVport Change as requested to goto cleanup in testStoragePoolDestroy -> Patch3: Fixed the comments to have the right variable name -> Patch4: Use "if ((obj = ...))) {" type syntax as suggested. -> Patch5-9: No change -> Patch10: Fix commit description -> Patch11: No change -> Patch 12: Fix the testNodeDeviceDestroy logic -> Patch 13-14 New patches that take the next two logical steps - objectifying the virNodeDeviceObjList and using a HashTable for access (along with all those bells and whistles). Finally remove the need for driver level locks when accessing the @devs ObjList. John Ferlan (14): nodedev: Alter virNodeDeviceObjRemove test: Adjust cleanup/error paths for nodedev test APIs nodedev: Use common naming for virnodedeviceobj nodedev: Use consistent names for driver variables nodedev: Introduce virNodeDeviceObjNew nodedev: Introduce virNodeDeviceObjListNew nodedev: Alter node device obj list function names nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs nodedev: Introduce virNodeDeviceGetSCSIHostCaps nodedev: Introduce virNodeDeviceObjListFindSCSIHostByWWNs nodedev: Privatize _virNodeDeviceObj and _virNodeDeviceObjList nodedev: Convert virNodeDeviceObj to use virObjectLockable nodedev: Convert virNodeDeviceObjListPtr to use hash tables nodedev: Remove driver locks around object list mgmt code src/conf/node_device_conf.c | 82 +++ src/conf/node_device_conf.h | 20 +- src/conf/virnodedeviceobj.c | 846 ++++++++++++++++++++---------- src/conf/virnodedeviceobj.h | 67 +-- src/libvirt_private.syms | 20 +- src/node_device/node_device_driver.c | 201 +++---- src/node_device/node_device_hal.c | 51 +- src/node_device/node_device_linux_sysfs.c | 77 +-- src/node_device/node_device_udev.c | 63 +-- src/test/test_driver.c | 133 +++-- 10 files changed, 895 insertions(+), 665 deletions(-) -- 2.9.4
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@redhat.com> --- 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
- In testDestroyVport rather than use a cleanup label, just return -1 immediately since nothing else is needed. - In testStoragePoolDestroy, if !privpool, then just return -1 since nothing else will happen anyway. - Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName an @obj, just return directly. This then allows the cleanup: label code to not have to check "if (obj)" before calling virNodeDeviceObjUnlock. This also simplifies some exit logic... - In testNodeDeviceObjFindByName use an error: label to handle the failure and don't do the ncaps++ within the VIR_STRDUP() source target index. Only increment ncaps after success. Easier on eyes at error label too. - In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 78 +++++++++++++++++++------------------------------- 1 file changed, 30 insertions(+), 48 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 04887e0..2889ffa 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4509,7 +4509,6 @@ testDestroyVport(testDriverPtr privconn, const char *wwnn ATTRIBUTE_UNUSED, const char *wwpn ATTRIBUTE_UNUSED) { - int ret = -1; virNodeDeviceObjPtr obj = NULL; virObjectEventPtr event = NULL; @@ -4523,7 +4522,7 @@ testDestroyVport(testDriverPtr privconn, if (!(obj = virNodeDeviceObjFindByName(&privconn->devs, "scsi_host12"))) { virReportError(VIR_ERR_NO_NODE_DEVICE, "%s", _("no node device with matching name 'scsi_host12'")); - goto cleanup; + return -1; } event = virNodeDeviceEventLifecycleNew("scsi_host12", @@ -4532,15 +4531,9 @@ testDestroyVport(testDriverPtr privconn, virNodeDeviceObjRemove(&privconn->devs, obj); virNodeDeviceObjFree(obj); - obj = NULL; - ret = 0; - - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); testObjectEventQueue(privconn, event); - return ret; + return 0; } @@ -4553,7 +4546,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool) virObjectEventPtr event = NULL; if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name))) - goto cleanup; + return -1; if (!virStoragePoolObjIsActive(privpool)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -5328,7 +5321,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name) virNodeDevicePtr ret = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, name))) - goto cleanup; + return NULL; def = virNodeDeviceObjGetDef(obj); if ((ret = virGetNodeDevice(conn, name))) { @@ -5338,9 +5331,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name) } } - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5355,13 +5346,11 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev, virCheckFlags(0, NULL); if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return NULL; ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj)); - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5374,7 +5363,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) char *ret = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return NULL; def = virNodeDeviceObjGetDef(obj); if (def->parent) { @@ -5384,9 +5373,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) "%s", _("no parent for this device")); } - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5399,20 +5386,16 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev) virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; def = virNodeDeviceObjGetDef(obj); for (caps = def->caps; caps; caps = caps->next) ++ncaps; - ret = ncaps; - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); - return ret; + virNodeDeviceObjUnlock(obj); + return ncaps; } @@ -5424,27 +5407,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; def = virNodeDeviceObjGetDef(obj); for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) { - if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0) - goto cleanup; + if (VIR_STRDUP(names[ncaps], + virNodeDevCapTypeToString(caps->data.type)) < 0) + goto error; + ncaps++; } - ret = ncaps; - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); - if (ret == -1) { - --ncaps; - while (--ncaps >= 0) - VIR_FREE(names[ncaps]); - } - return ret; + virNodeDeviceObjUnlock(obj); + return ncaps; + + error: + while (--ncaps >= 0) + VIR_FREE(names[ncaps]); + virNodeDeviceObjUnlock(obj); + return -1; } @@ -5598,14 +5580,14 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virObjectEventPtr event = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto out; + return -1; def = virNodeDeviceObjGetDef(obj); if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) - goto out; + goto cleanup; if (VIR_STRDUP(parent_name, def->parent) < 0) - goto out; + goto cleanup; /* virNodeDeviceGetParentHost will cause the device object's lock to be * taken, so we have to dup the parent's name and drop the lock @@ -5618,7 +5600,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) if (virNodeDeviceObjGetParentHost(&driver->devs, def, EXISTING_DEVICE) < 0) { obj = NULL; - goto out; + goto cleanup; } event = virNodeDeviceEventLifecycleNew(dev->name, @@ -5630,7 +5612,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virNodeDeviceObjFree(obj); obj = NULL; - out: + cleanup: if (obj) virNodeDeviceObjUnlock(obj); testObjectEventQueue(driver, event); -- 2.9.4
A virNodeDeviceObjPtr is an @obj A virNodeDeviceObjListPtr is an @devs Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 130 ++++++++++++++++++++++---------------------- 1 file changed, 65 insertions(+), 65 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index fa73de1..619c32d 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -41,10 +41,10 @@ virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj) static int -virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, +virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, const char *cap) { - virNodeDevCapsDefPtr caps = dev->def->caps; + virNodeDevCapsDefPtr caps = obj->def->caps; const char *fc_host_cap = virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_FC_HOST); const char *vports_cap = @@ -97,7 +97,7 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, /* virNodeDeviceFindFCCapDef: - * @dev: Pointer to current device + * @obj: Pointer to current device * * Search the device object 'caps' array for fc_host capability. * @@ -105,9 +105,9 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, * Pointer to the caps or NULL if not found */ static virNodeDevCapsDefPtr -virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev) +virNodeDeviceFindFCCapDef(const virNodeDeviceObj *obj) { - virNodeDevCapsDefPtr caps = dev->def->caps; + virNodeDevCapsDefPtr caps = obj->def->caps; while (caps) { if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST && @@ -121,7 +121,7 @@ virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev) /* virNodeDeviceFindVPORTCapDef: - * @dev: Pointer to current device + * @obj: Pointer to current device * * Search the device object 'caps' array for vport_ops capability. * @@ -129,9 +129,9 @@ virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev) * Pointer to the caps or NULL if not found */ static virNodeDevCapsDefPtr -virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *dev) +virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj) { - virNodeDevCapsDefPtr caps = dev->def->caps; + virNodeDevCapsDefPtr caps = obj->def->caps; while (caps) { if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST && @@ -240,16 +240,16 @@ virNodeDeviceFindByCap(virNodeDeviceObjListPtr devs, void -virNodeDeviceObjFree(virNodeDeviceObjPtr dev) +virNodeDeviceObjFree(virNodeDeviceObjPtr obj) { - if (!dev) + if (!obj) return; - virNodeDeviceDefFree(dev->def); + virNodeDeviceDefFree(obj->def); - virMutexDestroy(&dev->lock); + virMutexDestroy(&obj->lock); - VIR_FREE(dev); + VIR_FREE(obj); } @@ -268,48 +268,48 @@ virNodeDeviceObjPtr virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def) { - virNodeDeviceObjPtr device; + virNodeDeviceObjPtr obj; - if ((device = virNodeDeviceObjFindByName(devs, def->name))) { - virNodeDeviceDefFree(device->def); - device->def = def; - return device; + if ((obj = virNodeDeviceObjFindByName(devs, def->name))) { + virNodeDeviceDefFree(obj->def); + obj->def = def; + return obj; } - if (VIR_ALLOC(device) < 0) + if (VIR_ALLOC(obj) < 0) return NULL; - if (virMutexInit(&device->lock) < 0) { + if (virMutexInit(&obj->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); - VIR_FREE(device); + VIR_FREE(obj); return NULL; } - virNodeDeviceObjLock(device); + virNodeDeviceObjLock(obj); - if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, device) < 0) { - virNodeDeviceObjUnlock(device); - virNodeDeviceObjFree(device); + if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) { + virNodeDeviceObjUnlock(obj); + virNodeDeviceObjFree(obj); return NULL; } - device->def = def; + obj->def = def; - return device; + return obj; } void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr dev) + virNodeDeviceObjPtr obj) { size_t i; - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); for (i = 0; i < devs->count; i++) { virNodeDeviceObjLock(devs->objs[i]); - if (devs->objs[i] == dev) { + if (devs->objs[i] == obj) { virNodeDeviceObjUnlock(devs->objs[i]); VIR_DELETE_ELEMENT(devs->objs, i, devs->count); @@ -324,7 +324,7 @@ virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, * Return the NPIV dev's parent device name */ /* virNodeDeviceFindFCParentHost: - * @parent: Pointer to node device object + * @obj: Pointer to node device object * * Search the capabilities for the device to find the FC capabilities * in order to set the parent_host value. @@ -333,15 +333,15 @@ virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, * parent_host value on success (>= 0), -1 otherwise. */ static int -virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr parent) +virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr obj) { - virNodeDevCapsDefPtr cap = virNodeDeviceFindVPORTCapDef(parent); + virNodeDevCapsDefPtr cap = virNodeDeviceFindVPORTCapDef(obj); if (!cap) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Parent device %s is not capable " "of vport operations"), - parent->def->name); + obj->def->name); return -1; } @@ -354,19 +354,19 @@ virNodeDeviceGetParentHostByParent(virNodeDeviceObjListPtr devs, const char *dev_name, const char *parent_name) { - virNodeDeviceObjPtr parent = NULL; + virNodeDeviceObjPtr obj = NULL; int ret; - if (!(parent = virNodeDeviceObjFindByName(devs, parent_name))) { + if (!(obj = virNodeDeviceObjFindByName(devs, parent_name))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); return -1; } - ret = virNodeDeviceFindFCParentHost(parent); + ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(parent); + virNodeDeviceObjUnlock(obj); return ret; } @@ -378,19 +378,19 @@ virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs, const char *parent_wwnn, const char *parent_wwpn) { - virNodeDeviceObjPtr parent = NULL; + virNodeDeviceObjPtr obj = NULL; int ret; - if (!(parent = virNodeDeviceFindByWWNs(devs, parent_wwnn, parent_wwpn))) { + if (!(obj = virNodeDeviceFindByWWNs(devs, parent_wwnn, parent_wwpn))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); return -1; } - ret = virNodeDeviceFindFCParentHost(parent); + ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(parent); + virNodeDeviceObjUnlock(obj); return ret; } @@ -401,19 +401,19 @@ virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, const char *dev_name, const char *parent_fabric_wwn) { - virNodeDeviceObjPtr parent = NULL; + virNodeDeviceObjPtr obj = NULL; int ret; - if (!(parent = virNodeDeviceFindByFabricWWN(devs, parent_fabric_wwn))) { + if (!(obj = virNodeDeviceFindByFabricWWN(devs, parent_fabric_wwn))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); return -1; } - ret = virNodeDeviceFindFCParentHost(parent); + ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(parent); + virNodeDeviceObjUnlock(obj); return ret; } @@ -422,19 +422,19 @@ virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, static int virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs) { - virNodeDeviceObjPtr parent = NULL; + virNodeDeviceObjPtr obj = NULL; const char *cap = virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_VPORTS); int ret; - if (!(parent = virNodeDeviceFindByCap(devs, cap))) { + if (!(obj = virNodeDeviceFindByCap(devs, cap))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find any vport capable device")); return -1; } - ret = virNodeDeviceFindFCParentHost(parent); + ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(parent); + virNodeDeviceObjUnlock(obj); return ret; } @@ -482,12 +482,12 @@ virNodeDeviceObjUnlock(virNodeDeviceObjPtr obj) static bool -virNodeDeviceCapMatch(virNodeDeviceObjPtr devobj, +virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, int type) { virNodeDevCapsDefPtr cap = NULL; - for (cap = devobj->def->caps; cap; cap = cap->next) { + for (cap = obj->def->caps; cap; cap = cap->next) { if (type == cap->data.type) return true; @@ -588,9 +588,9 @@ virNodeDeviceObjGetNames(virNodeDeviceObjListPtr devs, #define MATCH(FLAG) ((flags & (VIR_CONNECT_LIST_NODE_DEVICES_CAP_ ## FLAG)) && \ - virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_ ## FLAG)) + virNodeDeviceCapMatch(obj, VIR_NODE_DEV_CAP_ ## FLAG)) static bool -virNodeDeviceMatch(virNodeDeviceObjPtr devobj, +virNodeDeviceMatch(virNodeDeviceObjPtr obj, unsigned int flags) { /* filter by cap type */ @@ -621,7 +621,7 @@ virNodeDeviceMatch(virNodeDeviceObjPtr devobj, int virNodeDeviceObjListExport(virConnectPtr conn, - virNodeDeviceObjListPtr devobjs, + virNodeDeviceObjListPtr devs, virNodeDevicePtr **devices, virNodeDeviceObjListFilter filter, unsigned int flags) @@ -632,26 +632,26 @@ virNodeDeviceObjListExport(virConnectPtr conn, int ret = -1; size_t i; - if (devices && VIR_ALLOC_N(tmp_devices, devobjs->count + 1) < 0) + if (devices && VIR_ALLOC_N(tmp_devices, devs->count + 1) < 0) goto cleanup; - for (i = 0; i < devobjs->count; i++) { - virNodeDeviceObjPtr devobj = devobjs->objs[i]; - virNodeDeviceObjLock(devobj); - if ((!filter || filter(conn, devobj->def)) && - virNodeDeviceMatch(devobj, flags)) { + for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDeviceObjLock(obj); + if ((!filter || filter(conn, obj->def)) && + virNodeDeviceMatch(obj, flags)) { if (devices) { - if (!(device = virGetNodeDevice(conn, devobj->def->name)) || - VIR_STRDUP(device->parent, devobj->def->parent) < 0) { + if (!(device = virGetNodeDevice(conn, obj->def->name)) || + VIR_STRDUP(device->parent, obj->def->parent) < 0) { virObjectUnref(device); - virNodeDeviceObjUnlock(devobj); + virNodeDeviceObjUnlock(obj); goto cleanup; } tmp_devices[ndevices] = device; } ndevices++; } - virNodeDeviceObjUnlock(devobj); + virNodeDeviceObjUnlock(obj); } if (tmp_devices) { -- 2.9.4
A virNodeDeviceObjPtr is an @obj A virNodeDeviceObjListPtr is a @devs A virNodeDevicePtr is a @device Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 74 ++++++++++++++++++------------------ src/node_device/node_device_hal.c | 39 +++++++++---------- src/node_device/node_device_udev.c | 46 ++++++++++------------ 3 files changed, 76 insertions(+), 83 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 5bf202e..45eb3f2 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -260,7 +260,7 @@ nodeDeviceLookupByName(virConnectPtr conn, { virNodeDeviceObjPtr obj; virNodeDeviceDefPtr def; - virNodeDevicePtr ret = NULL; + virNodeDevicePtr device = NULL; if (!(obj = nodeDeviceObjFindByName(name))) return NULL; @@ -269,16 +269,16 @@ nodeDeviceLookupByName(virConnectPtr conn, if (virNodeDeviceLookupByNameEnsureACL(conn, def) < 0) goto cleanup; - if ((ret = virGetNodeDevice(conn, name))) { - if (VIR_STRDUP(ret->parent, def->parent) < 0) { - virObjectUnref(ret); - ret = NULL; + if ((device = virGetNodeDevice(conn, name))) { + if (VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + device = NULL; } } cleanup: virNodeDeviceObjUnlock(obj); - return ret; + return device; } @@ -293,7 +293,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, virNodeDevCapsDefPtr cap = NULL; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; - virNodeDevicePtr dev = NULL; + virNodeDevicePtr device = NULL; virCheckFlags(0, NULL); @@ -316,10 +316,10 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) goto error; - if ((dev = virGetNodeDevice(conn, def->name))) { - if (VIR_STRDUP(dev->parent, def->parent) < 0) { - virObjectUnref(dev); - dev = NULL; + if ((device = virGetNodeDevice(conn, def->name))) { + if (VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + device = NULL; } } virNodeDeviceObjUnlock(obj); @@ -335,7 +335,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, out: nodeDeviceUnlock(); - return dev; + return device; error: virNodeDeviceObjUnlock(obj); @@ -344,7 +344,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, char * -nodeDeviceGetXMLDesc(virNodeDevicePtr dev, +nodeDeviceGetXMLDesc(virNodeDevicePtr device, unsigned int flags) { virNodeDeviceObjPtr obj; @@ -353,11 +353,11 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, virCheckFlags(0, NULL); - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return NULL; def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, def) < 0) + if (virNodeDeviceGetXMLDescEnsureACL(device->conn, def) < 0) goto cleanup; if (nodeDeviceUpdateDriverName(def) < 0) @@ -375,17 +375,17 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, char * -nodeDeviceGetParent(virNodeDevicePtr dev) +nodeDeviceGetParent(virNodeDevicePtr device) { virNodeDeviceObjPtr obj; virNodeDeviceDefPtr def; char *ret = NULL; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return NULL; def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceGetParentEnsureACL(dev->conn, def) < 0) + if (virNodeDeviceGetParentEnsureACL(device->conn, def) < 0) goto cleanup; if (def->parent) { @@ -403,7 +403,7 @@ nodeDeviceGetParent(virNodeDevicePtr dev) int -nodeDeviceNumOfCaps(virNodeDevicePtr dev) +nodeDeviceNumOfCaps(virNodeDevicePtr device) { virNodeDeviceObjPtr obj; virNodeDeviceDefPtr def; @@ -411,11 +411,11 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev) int ncaps = 0; int ret = -1; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceNumOfCapsEnsureACL(dev->conn, def) < 0) + if (virNodeDeviceNumOfCapsEnsureACL(device->conn, def) < 0) goto cleanup; for (caps = def->caps; caps; caps = caps->next) { @@ -442,7 +442,7 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev) int -nodeDeviceListCaps(virNodeDevicePtr dev, +nodeDeviceListCaps(virNodeDevicePtr device, char **const names, int maxnames) { @@ -452,11 +452,11 @@ nodeDeviceListCaps(virNodeDevicePtr dev, int ncaps = 0; int ret = -1; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceListCapsEnsureACL(dev->conn, def) < 0) + if (virNodeDeviceListCapsEnsureACL(device->conn, def) < 0) goto cleanup; for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) { @@ -530,7 +530,7 @@ nodeDeviceFindNewDevice(virConnectPtr conn, const char *wwnn, const char *wwpn) { - virNodeDevicePtr dev = NULL; + virNodeDevicePtr device = NULL; time_t start = 0, now = 0; /* The thread that creates the device takes the driver lock, so we @@ -546,9 +546,9 @@ nodeDeviceFindNewDevice(virConnectPtr conn, virWaitForDevices(); - dev = nodeDeviceLookupSCSIHostByWWN(conn, wwnn, wwpn, 0); + device = nodeDeviceLookupSCSIHostByWWN(conn, wwnn, wwpn, 0); - if (dev != NULL) + if (device != NULL) break; sleep(5); @@ -558,7 +558,7 @@ nodeDeviceFindNewDevice(virConnectPtr conn, nodeDeviceLock(); - return dev; + return device; } @@ -570,7 +570,7 @@ nodeDeviceCreateXML(virConnectPtr conn, virNodeDeviceDefPtr def = NULL; char *wwnn = NULL, *wwpn = NULL; int parent_host = -1; - virNodeDevicePtr dev = NULL; + virNodeDevicePtr device = NULL; const char *virt_type = NULL; virCheckFlags(0, NULL); @@ -594,11 +594,11 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_CREATE) < 0) goto cleanup; - dev = nodeDeviceFindNewDevice(conn, wwnn, wwpn); + device = nodeDeviceFindNewDevice(conn, wwnn, wwpn); /* We don't check the return value, because one way or another, * we're returning what we get... */ - if (dev == NULL) + if (device == NULL) virReportError(VIR_ERR_NO_NODE_DEVICE, _("no node device for '%s' with matching " "wwnn '%s' and wwpn '%s'"), @@ -608,12 +608,12 @@ nodeDeviceCreateXML(virConnectPtr conn, virNodeDeviceDefFree(def); VIR_FREE(wwnn); VIR_FREE(wwpn); - return dev; + return device; } int -nodeDeviceDestroy(virNodeDevicePtr dev) +nodeDeviceDestroy(virNodeDevicePtr device) { int ret = -1; virNodeDeviceObjPtr obj = NULL; @@ -621,13 +621,13 @@ nodeDeviceDestroy(virNodeDevicePtr dev) char *wwnn = NULL, *wwpn = NULL; int parent_host = -1; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; def = virNodeDeviceObjGetDef(obj); nodeDeviceLock(); - if (virNodeDeviceDestroyEnsureACL(dev->conn, def) < 0) + if (virNodeDeviceDestroyEnsureACL(device->conn, def) < 0) goto cleanup; if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) @@ -660,7 +660,7 @@ nodeDeviceDestroy(virNodeDevicePtr dev) int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, - virNodeDevicePtr dev, + virNodeDevicePtr device, int eventID, virConnectNodeDeviceEventGenericCallback callback, void *opaque, @@ -672,7 +672,7 @@ nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, goto cleanup; if (virNodeDeviceEventStateRegisterID(conn, driver->nodeDeviceEventState, - dev, eventID, callback, + device, eventID, callback, opaque, freecb, &callbackID) < 0) callbackID = -1; cleanup: diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index dc9202b..6441a3d 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -449,7 +449,7 @@ dev_create(const char *udi) { LibHalContext *ctx; char *parent_key = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def = NULL; virNodeDeviceDefPtr objdef; const char *name = hal_name(udi); @@ -482,15 +482,15 @@ dev_create(const char *udi) /* Some devices don't have a path in sysfs, so ignore failure */ (void)get_str_prop(ctx, udi, "linux.sysfs_path", &devicePath); - if (!(dev = virNodeDeviceObjAssignDef(&driver->devs, def))) { + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) { VIR_FREE(devicePath); goto failure; } - objdef = virNodeDeviceObjGetDef(dev); + objdef = virNodeDeviceObjGetDef(obj); objdef->sysfs_path = devicePath; - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); nodeDeviceUnlock(); return; @@ -506,22 +506,21 @@ static void dev_refresh(const char *udi) { const char *name = hal_name(udi); - virNodeDeviceObjPtr dev; + virNodeDeviceObjPtr obj; nodeDeviceLock(); - dev = virNodeDeviceObjFindByName(&driver->devs, name); - if (dev) { + if ((obj = virNodeDeviceObjFindByName(&driver->devs, name))) { /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, dev); + virNodeDeviceObjRemove(&driver->devs, obj); } else { VIR_DEBUG("no device named %s", name); } nodeDeviceUnlock(); - if (dev) { - virNodeDeviceObjFree(dev); + if (obj) { + virNodeDeviceObjFree(obj); dev_create(udi); } } @@ -540,17 +539,17 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, const char *udi) { const char *name = hal_name(udi); - virNodeDeviceObjPtr dev; + virNodeDeviceObjPtr obj; nodeDeviceLock(); - dev = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(&driver->devs, name); VIR_DEBUG("%s", name); - if (dev) - virNodeDeviceObjRemove(&driver->devs, dev); + if (obj) + virNodeDeviceObjRemove(&driver->devs, obj); else VIR_DEBUG("no device named %s", name); nodeDeviceUnlock(); - virNodeDeviceObjFree(dev); + virNodeDeviceObjFree(obj); } @@ -559,17 +558,17 @@ device_cap_added(LibHalContext *ctx, const char *udi, const char *cap) { const char *name = hal_name(udi); - virNodeDeviceObjPtr dev; + virNodeDeviceObjPtr obj; virNodeDeviceDefPtr def; nodeDeviceLock(); - dev = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(&driver->devs, name); nodeDeviceUnlock(); VIR_DEBUG("%s %s", cap, name); - if (dev) { - def = virNodeDeviceObjGetDef(dev); + if (obj) { + def = virNodeDeviceObjGetDef(obj); (void)gather_capability(ctx, udi, cap, &def->caps); - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); } else { VIR_DEBUG("no device named %s", name); } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 819e4e7..2672784 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1312,20 +1312,18 @@ udevGetDeviceDetails(struct udev_device *device, static int udevRemoveOneDevice(struct udev_device *device) { - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; virObjectEventPtr event = NULL; const char *name = NULL; name = udev_device_get_syspath(device); - dev = virNodeDeviceObjFindBySysfsPath(&driver->devs, name); - - if (!dev) { + if (!(obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, name))) { VIR_DEBUG("Failed to find device to remove that has udev name '%s'", name); return -1; } - def = virNodeDeviceObjGetDef(dev); + def = virNodeDeviceObjGetDef(obj); event = virNodeDeviceEventLifecycleNew(def->name, VIR_NODE_DEVICE_EVENT_DELETED, @@ -1333,8 +1331,8 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); - virNodeDeviceObjRemove(&driver->devs, dev); - virNodeDeviceObjFree(dev); + virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjFree(obj); if (event) virObjectEventStateQueue(driver->nodeDeviceEventState, event); @@ -1348,7 +1346,7 @@ udevSetParent(struct udev_device *device, { struct udev_device *parent_device = NULL; const char *parent_sysfs_path = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr objdef; int ret = -1; @@ -1367,15 +1365,14 @@ udevSetParent(struct udev_device *device, goto cleanup; } - dev = virNodeDeviceObjFindBySysfsPath(&driver->devs, - parent_sysfs_path); - if (dev != NULL) { - objdef = virNodeDeviceObjGetDef(dev); + if ((obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, + parent_sysfs_path))) { + objdef = virNodeDeviceObjGetDef(obj); if (VIR_STRDUP(def->parent, objdef->name) < 0) { - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); goto cleanup; } - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); if (VIR_STRDUP(def->parent_sysfs_path, parent_sysfs_path) < 0) goto cleanup; @@ -1397,7 +1394,7 @@ static int udevAddOneDevice(struct udev_device *device) { virNodeDeviceDefPtr def = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr objdef; virObjectEventPtr event = NULL; bool new_device = true; @@ -1427,18 +1424,16 @@ udevAddOneDevice(struct udev_device *device) if (udevSetParent(device, def) != 0) goto cleanup; - dev = virNodeDeviceObjFindByName(&driver->devs, def->name); - if (dev) { - virNodeDeviceObjUnlock(dev); + if ((obj = virNodeDeviceObjFindByName(&driver->devs, def->name))) { + virNodeDeviceObjUnlock(obj); new_device = false; } /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - dev = virNodeDeviceObjAssignDef(&driver->devs, def); - if (dev == NULL) + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) goto cleanup; - objdef = virNodeDeviceObjGetDef(dev); + objdef = virNodeDeviceObjGetDef(obj); if (new_device) event = virNodeDeviceEventLifecycleNew(objdef->name, @@ -1447,7 +1442,7 @@ udevAddOneDevice(struct udev_device *device) else event = virNodeDeviceEventUpdateNew(objdef->name); - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); ret = 0; @@ -1710,7 +1705,7 @@ static int udevSetupSystemDev(void) { virNodeDeviceDefPtr def = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; int ret = -1; if (VIR_ALLOC(def) < 0) @@ -1726,11 +1721,10 @@ udevSetupSystemDev(void) udevGetDMIData(&def->caps->data.system); #endif - dev = virNodeDeviceObjAssignDef(&driver->devs, def); - if (dev == NULL) + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) goto cleanup; - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); ret = 0; -- 2.9.4
Create an allocator for the virNodeDeviceObjPtr - include setting up the mutex, saving the virNodeDeviceDefPtr, and locking the return object. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 619c32d..9a2b5de 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -33,6 +33,27 @@ VIR_LOG_INIT("conf.virnodedeviceobj"); +static virNodeDeviceObjPtr +virNodeDeviceObjNew(virNodeDeviceDefPtr def) +{ + virNodeDeviceObjPtr obj; + + if (VIR_ALLOC(obj) < 0) + return NULL; + + if (virMutexInit(&obj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot initialize mutex")); + VIR_FREE(obj); + return NULL; + } + virNodeDeviceObjLock(obj); + obj->def = def; + + return obj; +} + + virNodeDeviceDefPtr virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj) { @@ -276,26 +297,17 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, return obj; } - if (VIR_ALLOC(obj) < 0) + if (!(obj = virNodeDeviceObjNew(def))) return NULL; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); - return NULL; - } - virNodeDeviceObjLock(obj); - if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) { + obj->def = NULL; virNodeDeviceObjUnlock(obj); virNodeDeviceObjFree(obj); return NULL; } - obj->def = def; return obj; - } -- 2.9.4
In preparation to make things private, make the ->devs be pointers to a virNodeDeviceObjList and then manage everything inside virnodedeviceobj Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 13 ++++++++++++- src/conf/virnodedeviceobj.h | 5 ++++- src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 14 +++++++------- src/node_device/node_device_hal.c | 19 +++++++++++-------- src/node_device/node_device_udev.c | 18 +++++++++++------- src/test/test_driver.c | 29 +++++++++++++++-------------- 7 files changed, 61 insertions(+), 38 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 9a2b5de..0c89790 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -274,6 +274,17 @@ virNodeDeviceObjFree(virNodeDeviceObjPtr obj) } +virNodeDeviceObjListPtr +virNodeDeviceObjListNew(void) +{ + virNodeDeviceObjListPtr devs; + + if (VIR_ALLOC(devs) < 0) + return NULL; + return devs; +} + + void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) { @@ -281,7 +292,7 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) for (i = 0; i < devs->count; i++) virNodeDeviceObjFree(devs->objs[i]); VIR_FREE(devs->objs); - devs->count = 0; + VIR_FREE(devs); } diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 9bc02ee..77250a0 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -32,7 +32,7 @@ typedef virNodeDeviceDriverState *virNodeDeviceDriverStatePtr; struct _virNodeDeviceDriverState { virMutex lock; - virNodeDeviceObjList devs; /* currently-known devices */ + virNodeDeviceObjListPtr devs; /* currently-known devices */ void *privateData; /* driver-specific private data */ /* Immutable pointer, self-locking APIs */ @@ -68,6 +68,9 @@ virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs, void virNodeDeviceObjFree(virNodeDeviceObjPtr dev); +virNodeDeviceObjListPtr +virNodeDeviceObjListNew(void); + void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e6ed981..c7140c8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -971,6 +971,7 @@ virNodeDeviceObjGetNames; virNodeDeviceObjGetParentHost; virNodeDeviceObjListExport; virNodeDeviceObjListFree; +virNodeDeviceObjListNew; virNodeDeviceObjLock; virNodeDeviceObjNumOfDevices; virNodeDeviceObjRemove; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 45eb3f2..1ad2557 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -182,7 +182,7 @@ nodeNumOfDevices(virConnectPtr conn, virCheckFlags(0, -1); nodeDeviceLock(); - ndevs = virNodeDeviceObjNumOfDevices(&driver->devs, conn, cap, + ndevs = virNodeDeviceObjNumOfDevices(driver->devs, conn, cap, virNodeNumOfDevicesCheckACL); nodeDeviceUnlock(); @@ -205,7 +205,7 @@ nodeListDevices(virConnectPtr conn, virCheckFlags(0, -1); nodeDeviceLock(); - nnames = virNodeDeviceObjGetNames(&driver->devs, conn, + nnames = virNodeDeviceObjGetNames(driver->devs, conn, virNodeListDevicesCheckACL, cap, names, maxnames); nodeDeviceUnlock(); @@ -227,7 +227,7 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, return -1; nodeDeviceLock(); - ret = virNodeDeviceObjListExport(conn, &driver->devs, devices, + ret = virNodeDeviceObjListExport(conn, driver->devs, devices, virConnectListAllNodeDevicesCheckACL, flags); nodeDeviceUnlock(); @@ -241,7 +241,7 @@ nodeDeviceObjFindByName(const char *name) virNodeDeviceObjPtr obj; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(driver->devs, name); nodeDeviceUnlock(); if (!obj) { @@ -289,7 +289,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, unsigned int flags) { size_t i; - virNodeDeviceObjListPtr devs = &driver->devs; + virNodeDeviceObjListPtr devs = driver->devs; virNodeDevCapsDefPtr cap = NULL; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; @@ -587,7 +587,7 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup; - if ((parent_host = virNodeDeviceObjGetParentHost(&driver->devs, def, + if ((parent_host = virNodeDeviceObjGetParentHost(driver->devs, def, CREATE_DEVICE)) < 0) goto cleanup; @@ -639,7 +639,7 @@ nodeDeviceDestroy(virNodeDevicePtr device) * or parent_fabric_wwn) and drop the object lock. */ virNodeDeviceObjUnlock(obj); obj = NULL; - if ((parent_host = virNodeDeviceObjGetParentHost(&driver->devs, def, + if ((parent_host = virNodeDeviceObjGetParentHost(driver->devs, def, EXISTING_DEVICE)) < 0) goto cleanup; diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 6441a3d..fcffaab 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -482,7 +482,7 @@ dev_create(const char *udi) /* Some devices don't have a path in sysfs, so ignore failure */ (void)get_str_prop(ctx, udi, "linux.sysfs_path", &devicePath); - if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) { + if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) { VIR_FREE(devicePath); goto failure; } @@ -509,11 +509,11 @@ dev_refresh(const char *udi) virNodeDeviceObjPtr obj; nodeDeviceLock(); - if ((obj = virNodeDeviceObjFindByName(&driver->devs, name))) { + if ((obj = virNodeDeviceObjFindByName(driver->devs, name))) { /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjRemove(driver->devs, obj); } else { VIR_DEBUG("no device named %s", name); } @@ -542,10 +542,10 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, virNodeDeviceObjPtr obj; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(driver->devs, name); VIR_DEBUG("%s", name); if (obj) - virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjRemove(driver->devs, obj); else VIR_DEBUG("no device named %s", name); nodeDeviceUnlock(); @@ -562,7 +562,7 @@ device_cap_added(LibHalContext *ctx, virNodeDeviceDefPtr def; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(driver->devs, name); nodeDeviceUnlock(); VIR_DEBUG("%s %s", cap, name); if (obj) { @@ -627,6 +627,9 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, } nodeDeviceLock(); + if (!(driver->devs = virNodeDeviceObjListNew())) + goto failure; + dbus_error_init(&err); if (!(sysbus = virDBusGetSystemBus())) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -701,7 +704,7 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, _("%s: %s"), err.name, err.message); dbus_error_free(&err); } - virNodeDeviceObjListFree(&driver->devs); + virNodeDeviceObjListFree(driver->devs); if (hal_ctx) (void)libhal_ctx_free(hal_ctx); nodeDeviceUnlock(); @@ -717,7 +720,7 @@ nodeStateCleanup(void) if (driver) { nodeDeviceLock(); LibHalContext *hal_ctx = DRV_STATE_HAL_CTX(driver); - virNodeDeviceObjListFree(&driver->devs); + virNodeDeviceObjListFree(driver->devs); (void)libhal_ctx_shutdown(hal_ctx, NULL); (void)libhal_ctx_free(hal_ctx); nodeDeviceUnlock(); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 2672784..b113d93 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1318,7 +1318,7 @@ udevRemoveOneDevice(struct udev_device *device) const char *name = NULL; name = udev_device_get_syspath(device); - if (!(obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, name))) { + if (!(obj = virNodeDeviceObjFindBySysfsPath(driver->devs, name))) { VIR_DEBUG("Failed to find device to remove that has udev name '%s'", name); return -1; @@ -1331,7 +1331,7 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); - virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjRemove(driver->devs, obj); virNodeDeviceObjFree(obj); if (event) @@ -1365,7 +1365,7 @@ udevSetParent(struct udev_device *device, goto cleanup; } - if ((obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, + if ((obj = virNodeDeviceObjFindBySysfsPath(driver->devs, parent_sysfs_path))) { objdef = virNodeDeviceObjGetDef(obj); if (VIR_STRDUP(def->parent, objdef->name) < 0) { @@ -1424,14 +1424,14 @@ udevAddOneDevice(struct udev_device *device) if (udevSetParent(device, def) != 0) goto cleanup; - if ((obj = virNodeDeviceObjFindByName(&driver->devs, def->name))) { + if ((obj = virNodeDeviceObjFindByName(driver->devs, def->name))) { virNodeDeviceObjUnlock(obj); new_device = false; } /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) + if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) goto cleanup; objdef = virNodeDeviceObjGetDef(obj); @@ -1585,7 +1585,7 @@ nodeStateCleanup(void) if (udev != NULL) udev_unref(udev); - virNodeDeviceObjListFree(&driver->devs); + virNodeDeviceObjListFree(driver->devs); nodeDeviceUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -1721,7 +1721,7 @@ udevSetupSystemDev(void) udevGetDMIData(&def->caps->data.system); #endif - if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) + if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) goto cleanup; virNodeDeviceObjUnlock(obj); @@ -1790,6 +1790,10 @@ nodeStateInitialize(bool privileged, driver->privateData = priv; nodeDeviceLock(); + + if (!(driver->devs = virNodeDeviceObjListNew())) + goto cleanup; + driver->nodeDeviceEventState = virObjectEventStateNew(); if (udevPCITranslateInit(privileged) < 0) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2889ffa..48ef92f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -101,7 +101,7 @@ struct _testDriver { bool transaction_running; virInterfaceObjListPtr backupIfaces; virStoragePoolObjList pools; - virNodeDeviceObjList devs; + virNodeDeviceObjListPtr devs; int numCells; testCell cells[MAX_CELLS]; size_t numAuths; @@ -152,7 +152,7 @@ testDriverFree(testDriverPtr driver) virObjectUnref(driver->caps); virObjectUnref(driver->xmlopt); virObjectUnref(driver->domains); - virNodeDeviceObjListFree(&driver->devs); + virNodeDeviceObjListFree(driver->devs); virObjectUnref(driver->networks); virInterfaceObjListFree(driver->ifaces); virStoragePoolObjListFree(&driver->pools); @@ -418,7 +418,8 @@ testDriverNew(void) !(ret->eventState = virObjectEventStateNew()) || !(ret->ifaces = virInterfaceObjListNew()) || !(ret->domains = virDomainObjListNew()) || - !(ret->networks = virNetworkObjListNew())) + !(ret->networks = virNetworkObjListNew()) || + !(ret->devs = virNodeDeviceObjListNew())) goto error; virAtomicIntSet(&ret->nextDomID, 1); @@ -1169,7 +1170,7 @@ testParseNodedevs(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virNodeDeviceObjAssignDef(&privconn->devs, def))) { + if (!(obj = virNodeDeviceObjAssignDef(privconn->devs, def))) { virNodeDeviceDefFree(def); goto error; } @@ -4519,7 +4520,7 @@ testDestroyVport(testDriverPtr privconn, * * Reaching across the boundaries of space and time into the * Node Device in order to remove */ - if (!(obj = virNodeDeviceObjFindByName(&privconn->devs, "scsi_host12"))) { + if (!(obj = virNodeDeviceObjFindByName(privconn->devs, "scsi_host12"))) { virReportError(VIR_ERR_NO_NODE_DEVICE, "%s", _("no node device with matching name 'scsi_host12'")); return -1; @@ -4529,7 +4530,7 @@ testDestroyVport(testDriverPtr privconn, VIR_NODE_DEVICE_EVENT_DELETED, 0); - virNodeDeviceObjRemove(&privconn->devs, obj); + virNodeDeviceObjRemove(privconn->devs, obj); virNodeDeviceObjFree(obj); testObjectEventQueue(privconn, event); @@ -5261,7 +5262,7 @@ testNodeDeviceObjFindByName(testDriverPtr driver, virNodeDeviceObjPtr obj; testDriverLock(driver); - obj = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(driver->devs, name); testDriverUnlock(driver); if (!obj) @@ -5284,7 +5285,7 @@ testNodeNumOfDevices(virConnectPtr conn, virCheckFlags(0, -1); testDriverLock(driver); - ndevs = virNodeDeviceObjNumOfDevices(&driver->devs, conn, cap, NULL); + ndevs = virNodeDeviceObjNumOfDevices(driver->devs, conn, cap, NULL); testDriverUnlock(driver); return ndevs; @@ -5304,7 +5305,7 @@ testNodeListDevices(virConnectPtr conn, virCheckFlags(0, -1); testDriverLock(driver); - nnames = virNodeDeviceObjGetNames(&driver->devs, conn, NULL, + nnames = virNodeDeviceObjGetNames(driver->devs, conn, NULL, cap, names, maxnames); testDriverUnlock(driver); @@ -5451,7 +5452,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, * using the scsi_host11 definition, changing the name and the * scsi_host capability fields before calling virNodeDeviceAssignDef * to add the def to the node device objects list. */ - if (!(objcopy = virNodeDeviceObjFindByName(&driver->devs, "scsi_host11"))) + if (!(objcopy = virNodeDeviceObjFindByName(driver->devs, "scsi_host11"))) goto cleanup; xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy)); @@ -5491,7 +5492,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, caps = caps->next; } - if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) + if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) goto cleanup; def = NULL; objdef = virNodeDeviceObjGetDef(obj); @@ -5536,7 +5537,7 @@ testNodeDeviceCreateXML(virConnectPtr conn, /* Unlike the "real" code we don't need the parent_host in order to * call virVHBAManageVport, but still let's make sure the code finds * something valid and no one messed up the mock environment. */ - if (virNodeDeviceObjGetParentHost(&driver->devs, def, CREATE_DEVICE) < 0) + if (virNodeDeviceObjGetParentHost(driver->devs, def, CREATE_DEVICE) < 0) goto cleanup; /* In the real code, we'd call virVHBAManageVport followed by @@ -5597,7 +5598,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) /* We do this just for basic validation, but also avoid finding a * vport capable HBA if for some reason our vHBA doesn't exist */ - if (virNodeDeviceObjGetParentHost(&driver->devs, def, + if (virNodeDeviceObjGetParentHost(driver->devs, def, EXISTING_DEVICE) < 0) { obj = NULL; goto cleanup; @@ -5608,7 +5609,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) 0); virNodeDeviceObjLock(obj); - virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjRemove(driver->devs, obj); virNodeDeviceObjFree(obj); obj = NULL; -- 2.9.4
Ensure that any function that walks the node device object list is prefixed by virNodeDeviceObjList. Also, modify the @filter param name for virNodeDeviceObjListExport to be @aclfilter. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 109 ++++++++++++++++++----------------- src/conf/virnodedeviceobj.h | 42 +++++++------- src/libvirt_private.syms | 14 ++--- src/node_device/node_device_driver.c | 20 +++---- src/node_device/node_device_hal.c | 12 ++-- src/node_device/node_device_udev.c | 14 ++--- src/test/test_driver.c | 28 ++++----- 7 files changed, 121 insertions(+), 118 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 0c89790..f91f4a0 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -166,8 +166,8 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj) virNodeDeviceObjPtr -virNodeDeviceObjFindBySysfsPath(virNodeDeviceObjListPtr devs, - const char *sysfs_path) +virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, + const char *sysfs_path) { size_t i; @@ -185,8 +185,8 @@ virNodeDeviceObjFindBySysfsPath(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr -virNodeDeviceObjFindByName(virNodeDeviceObjListPtr devs, - const char *name) +virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, + const char *name) { size_t i; @@ -202,9 +202,9 @@ virNodeDeviceObjFindByName(virNodeDeviceObjListPtr devs, static virNodeDeviceObjPtr -virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs, - const char *parent_wwnn, - const char *parent_wwpn) +virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, + const char *parent_wwnn, + const char *parent_wwpn) { size_t i; @@ -224,8 +224,8 @@ virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs, static virNodeDeviceObjPtr -virNodeDeviceFindByFabricWWN(virNodeDeviceObjListPtr devs, - const char *parent_fabric_wwn) +virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs, + const char *parent_fabric_wwn) { size_t i; @@ -244,8 +244,8 @@ virNodeDeviceFindByFabricWWN(virNodeDeviceObjListPtr devs, static virNodeDeviceObjPtr -virNodeDeviceFindByCap(virNodeDeviceObjListPtr devs, - const char *cap) +virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, + const char *cap) { size_t i; @@ -297,12 +297,12 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) virNodeDeviceObjPtr -virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def) +virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, + virNodeDeviceDefPtr def) { virNodeDeviceObjPtr obj; - if ((obj = virNodeDeviceObjFindByName(devs, def->name))) { + if ((obj = virNodeDeviceObjListFindByName(devs, def->name))) { virNodeDeviceDefFree(obj->def); obj->def = def; return obj; @@ -323,8 +323,8 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, void -virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr obj) +virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, + virNodeDeviceObjPtr obj) { size_t i; @@ -373,14 +373,14 @@ virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr obj) static int -virNodeDeviceGetParentHostByParent(virNodeDeviceObjListPtr devs, - const char *dev_name, - const char *parent_name) +virNodeDeviceObjListGetParentHostByParent(virNodeDeviceObjListPtr devs, + const char *dev_name, + const char *parent_name) { virNodeDeviceObjPtr obj = NULL; int ret; - if (!(obj = virNodeDeviceObjFindByName(devs, parent_name))) { + if (!(obj = virNodeDeviceObjListFindByName(devs, parent_name))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); @@ -396,15 +396,16 @@ virNodeDeviceGetParentHostByParent(virNodeDeviceObjListPtr devs, static int -virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs, - const char *dev_name, - const char *parent_wwnn, - const char *parent_wwpn) +virNodeDeviceObjListGetParentHostByWWNs(virNodeDeviceObjListPtr devs, + const char *dev_name, + const char *parent_wwnn, + const char *parent_wwpn) { virNodeDeviceObjPtr obj = NULL; int ret; - if (!(obj = virNodeDeviceFindByWWNs(devs, parent_wwnn, parent_wwpn))) { + if (!(obj = virNodeDeviceObjListFindByWWNs(devs, parent_wwnn, + parent_wwpn))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); @@ -420,14 +421,14 @@ virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs, static int -virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, - const char *dev_name, - const char *parent_fabric_wwn) +virNodeDeviceObjListGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, + const char *dev_name, + const char *parent_fabric_wwn) { virNodeDeviceObjPtr obj = NULL; int ret; - if (!(obj = virNodeDeviceFindByFabricWWN(devs, parent_fabric_wwn))) { + if (!(obj = virNodeDeviceObjListFindByFabricWWN(devs, parent_fabric_wwn))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not find parent device for '%s'"), dev_name); @@ -443,13 +444,13 @@ virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, static int -virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs) +virNodeDeviceObjListFindVportParentHost(virNodeDeviceObjListPtr devs) { virNodeDeviceObjPtr obj = NULL; const char *cap = virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_VPORTS); int ret; - if (!(obj = virNodeDeviceFindByCap(devs, cap))) { + if (!(obj = virNodeDeviceObjListFindByCap(devs, cap))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find any vport capable device")); return -1; @@ -464,26 +465,26 @@ virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs) int -virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def, - int create) +virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, + virNodeDeviceDefPtr def, + int create) { int parent_host = -1; if (def->parent) { - parent_host = virNodeDeviceGetParentHostByParent(devs, def->name, - def->parent); + parent_host = virNodeDeviceObjListGetParentHostByParent(devs, def->name, + def->parent); } else if (def->parent_wwnn && def->parent_wwpn) { - parent_host = virNodeDeviceGetParentHostByWWNs(devs, def->name, - def->parent_wwnn, - def->parent_wwpn); + parent_host = virNodeDeviceObjListGetParentHostByWWNs(devs, def->name, + def->parent_wwnn, + def->parent_wwpn); } else if (def->parent_fabric_wwn) { parent_host = - virNodeDeviceGetParentHostByFabricWWN(devs, def->name, - def->parent_fabric_wwn); + virNodeDeviceObjListGetParentHostByFabricWWN(devs, def->name, + def->parent_fabric_wwn); } else if (create == CREATE_DEVICE) { /* Try to find a vport capable scsi_host when no parent supplied */ - parent_host = virNodeDeviceFindVportParentHost(devs); + parent_host = virNodeDeviceObjListFindVportParentHost(devs); } return parent_host; @@ -555,10 +556,10 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, int -virNodeDeviceObjNumOfDevices(virNodeDeviceObjListPtr devs, - virConnectPtr conn, - const char *cap, - virNodeDeviceObjListFilter aclfilter) +virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, + virConnectPtr conn, + const char *cap, + virNodeDeviceObjListFilter aclfilter) { size_t i; int ndevs = 0; @@ -577,12 +578,12 @@ virNodeDeviceObjNumOfDevices(virNodeDeviceObjListPtr devs, int -virNodeDeviceObjGetNames(virNodeDeviceObjListPtr devs, - virConnectPtr conn, - virNodeDeviceObjListFilter aclfilter, - const char *cap, - char **const names, - int maxnames) +virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, + virConnectPtr conn, + virNodeDeviceObjListFilter aclfilter, + const char *cap, + char **const names, + int maxnames) { int nnames = 0; size_t i; @@ -646,7 +647,7 @@ int virNodeDeviceObjListExport(virConnectPtr conn, virNodeDeviceObjListPtr devs, virNodeDevicePtr **devices, - virNodeDeviceObjListFilter filter, + virNodeDeviceObjListFilter aclfilter, unsigned int flags) { virNodeDevicePtr *tmp_devices = NULL; @@ -661,7 +662,7 @@ virNodeDeviceObjListExport(virConnectPtr conn, for (i = 0; i < devs->count; i++) { virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDeviceObjLock(obj); - if ((!filter || filter(conn, obj->def)) && + if ((!aclfilter || aclfilter(conn, obj->def)) && virNodeDeviceMatch(obj, flags)) { if (devices) { if (!(device = virGetNodeDevice(conn, obj->def->name)) || diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 77250a0..6194c6c 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -44,25 +44,25 @@ virNodeDeviceDefPtr virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj); virNodeDeviceObjPtr -virNodeDeviceObjFindByName(virNodeDeviceObjListPtr devs, - const char *name); +virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, + const char *name); virNodeDeviceObjPtr -virNodeDeviceObjFindBySysfsPath(virNodeDeviceObjListPtr devs, - const char *sysfs_path) +virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, + const char *sysfs_path) ATTRIBUTE_NONNULL(2); virNodeDeviceObjPtr -virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def); +virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, + virNodeDeviceDefPtr def); void -virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr dev); +virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, + virNodeDeviceObjPtr dev); int -virNodeDeviceObjGetParentHost(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def, +virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, + virNodeDeviceDefPtr def, int create); void @@ -85,24 +85,24 @@ typedef bool virNodeDeviceDefPtr def); int -virNodeDeviceObjNumOfDevices(virNodeDeviceObjListPtr devs, - virConnectPtr conn, - const char *cap, - virNodeDeviceObjListFilter aclfilter); +virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, + virConnectPtr conn, + const char *cap, + virNodeDeviceObjListFilter aclfilter); int -virNodeDeviceObjGetNames(virNodeDeviceObjListPtr devs, - virConnectPtr conn, - virNodeDeviceObjListFilter aclfilter, - const char *cap, - char **const names, - int maxnames); +virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, + virConnectPtr conn, + virNodeDeviceObjListFilter aclfilter, + const char *cap, + char **const names, + int maxnames); int virNodeDeviceObjListExport(virConnectPtr conn, virNodeDeviceObjListPtr devobjs, virNodeDevicePtr **devices, - virNodeDeviceObjListFilter filter, + virNodeDeviceObjListFilter aclfilter, unsigned int flags); #endif /* __VIRNODEDEVICEOBJ_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c7140c8..f81a035 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -962,19 +962,19 @@ virNetworkObjUpdateAssignDef; # conf/virnodedeviceobj.h -virNodeDeviceObjAssignDef; -virNodeDeviceObjFindByName; -virNodeDeviceObjFindBySysfsPath; virNodeDeviceObjFree; virNodeDeviceObjGetDef; -virNodeDeviceObjGetNames; -virNodeDeviceObjGetParentHost; +virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; +virNodeDeviceObjListFindByName; +virNodeDeviceObjListFindBySysfsPath; virNodeDeviceObjListFree; +virNodeDeviceObjListGetNames; +virNodeDeviceObjListGetParentHost; virNodeDeviceObjListNew; +virNodeDeviceObjListNumOfDevices; +virNodeDeviceObjListRemove; virNodeDeviceObjLock; -virNodeDeviceObjNumOfDevices; -virNodeDeviceObjRemove; virNodeDeviceObjUnlock; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 1ad2557..930f9b6 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -182,8 +182,8 @@ nodeNumOfDevices(virConnectPtr conn, virCheckFlags(0, -1); nodeDeviceLock(); - ndevs = virNodeDeviceObjNumOfDevices(driver->devs, conn, cap, - virNodeNumOfDevicesCheckACL); + ndevs = virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap, + virNodeNumOfDevicesCheckACL); nodeDeviceUnlock(); return ndevs; @@ -205,9 +205,9 @@ nodeListDevices(virConnectPtr conn, virCheckFlags(0, -1); nodeDeviceLock(); - nnames = virNodeDeviceObjGetNames(driver->devs, conn, - virNodeListDevicesCheckACL, - cap, names, maxnames); + nnames = virNodeDeviceObjListGetNames(driver->devs, conn, + virNodeListDevicesCheckACL, + cap, names, maxnames); nodeDeviceUnlock(); return nnames; @@ -241,7 +241,7 @@ nodeDeviceObjFindByName(const char *name) virNodeDeviceObjPtr obj; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(driver->devs, name); + obj = virNodeDeviceObjListFindByName(driver->devs, name); nodeDeviceUnlock(); if (!obj) { @@ -587,8 +587,8 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup; - if ((parent_host = virNodeDeviceObjGetParentHost(driver->devs, def, - CREATE_DEVICE)) < 0) + if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, + CREATE_DEVICE)) < 0) goto cleanup; if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_CREATE) < 0) @@ -639,8 +639,8 @@ nodeDeviceDestroy(virNodeDevicePtr device) * or parent_fabric_wwn) and drop the object lock. */ virNodeDeviceObjUnlock(obj); obj = NULL; - if ((parent_host = virNodeDeviceObjGetParentHost(driver->devs, def, - EXISTING_DEVICE)) < 0) + if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, + EXISTING_DEVICE)) < 0) goto cleanup; if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_DELETE) < 0) diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index fcffaab..5d99e79 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -482,7 +482,7 @@ dev_create(const char *udi) /* Some devices don't have a path in sysfs, so ignore failure */ (void)get_str_prop(ctx, udi, "linux.sysfs_path", &devicePath); - if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) { + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) { VIR_FREE(devicePath); goto failure; } @@ -509,11 +509,11 @@ dev_refresh(const char *udi) virNodeDeviceObjPtr obj; nodeDeviceLock(); - if ((obj = virNodeDeviceObjFindByName(driver->devs, name))) { + if ((obj = virNodeDeviceObjListFindByName(driver->devs, name))) { /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver->devs, obj); } else { VIR_DEBUG("no device named %s", name); } @@ -542,10 +542,10 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, virNodeDeviceObjPtr obj; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(driver->devs, name); + obj = virNodeDeviceObjListFindByName(driver->devs, name); VIR_DEBUG("%s", name); if (obj) - virNodeDeviceObjRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver->devs, obj); else VIR_DEBUG("no device named %s", name); nodeDeviceUnlock(); @@ -562,7 +562,7 @@ device_cap_added(LibHalContext *ctx, virNodeDeviceDefPtr def; nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(driver->devs, name); + obj = virNodeDeviceObjListFindByName(driver->devs, name); nodeDeviceUnlock(); VIR_DEBUG("%s %s", cap, name); if (obj) { diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b113d93..e1b9a5c 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1318,7 +1318,7 @@ udevRemoveOneDevice(struct udev_device *device) const char *name = NULL; name = udev_device_get_syspath(device); - if (!(obj = virNodeDeviceObjFindBySysfsPath(driver->devs, name))) { + if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, name))) { VIR_DEBUG("Failed to find device to remove that has udev name '%s'", name); return -1; @@ -1331,7 +1331,7 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); - virNodeDeviceObjRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver->devs, obj); virNodeDeviceObjFree(obj); if (event) @@ -1365,8 +1365,8 @@ udevSetParent(struct udev_device *device, goto cleanup; } - if ((obj = virNodeDeviceObjFindBySysfsPath(driver->devs, - parent_sysfs_path))) { + if ((obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, + parent_sysfs_path))) { objdef = virNodeDeviceObjGetDef(obj); if (VIR_STRDUP(def->parent, objdef->name) < 0) { virNodeDeviceObjUnlock(obj); @@ -1424,14 +1424,14 @@ udevAddOneDevice(struct udev_device *device) if (udevSetParent(device, def) != 0) goto cleanup; - if ((obj = virNodeDeviceObjFindByName(driver->devs, def->name))) { + if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { virNodeDeviceObjUnlock(obj); new_device = false; } /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; objdef = virNodeDeviceObjGetDef(obj); @@ -1721,7 +1721,7 @@ udevSetupSystemDev(void) udevGetDMIData(&def->caps->data.system); #endif - if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; virNodeDeviceObjUnlock(obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 48ef92f..6dfabfa 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1170,7 +1170,7 @@ testParseNodedevs(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virNodeDeviceObjAssignDef(privconn->devs, def))) { + if (!(obj = virNodeDeviceObjListAssignDef(privconn->devs, def))) { virNodeDeviceDefFree(def); goto error; } @@ -4520,7 +4520,8 @@ testDestroyVport(testDriverPtr privconn, * * Reaching across the boundaries of space and time into the * Node Device in order to remove */ - if (!(obj = virNodeDeviceObjFindByName(privconn->devs, "scsi_host12"))) { + if (!(obj = virNodeDeviceObjListFindByName(privconn->devs, + "scsi_host12"))) { virReportError(VIR_ERR_NO_NODE_DEVICE, "%s", _("no node device with matching name 'scsi_host12'")); return -1; @@ -4530,7 +4531,7 @@ testDestroyVport(testDriverPtr privconn, VIR_NODE_DEVICE_EVENT_DELETED, 0); - virNodeDeviceObjRemove(privconn->devs, obj); + virNodeDeviceObjListRemove(privconn->devs, obj); virNodeDeviceObjFree(obj); testObjectEventQueue(privconn, event); @@ -5262,7 +5263,7 @@ testNodeDeviceObjFindByName(testDriverPtr driver, virNodeDeviceObjPtr obj; testDriverLock(driver); - obj = virNodeDeviceObjFindByName(driver->devs, name); + obj = virNodeDeviceObjListFindByName(driver->devs, name); testDriverUnlock(driver); if (!obj) @@ -5285,7 +5286,7 @@ testNodeNumOfDevices(virConnectPtr conn, virCheckFlags(0, -1); testDriverLock(driver); - ndevs = virNodeDeviceObjNumOfDevices(driver->devs, conn, cap, NULL); + ndevs = virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap, NULL); testDriverUnlock(driver); return ndevs; @@ -5305,8 +5306,8 @@ testNodeListDevices(virConnectPtr conn, virCheckFlags(0, -1); testDriverLock(driver); - nnames = virNodeDeviceObjGetNames(driver->devs, conn, NULL, - cap, names, maxnames); + nnames = virNodeDeviceObjListGetNames(driver->devs, conn, NULL, + cap, names, maxnames); testDriverUnlock(driver); return nnames; @@ -5452,7 +5453,8 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, * using the scsi_host11 definition, changing the name and the * scsi_host capability fields before calling virNodeDeviceAssignDef * to add the def to the node device objects list. */ - if (!(objcopy = virNodeDeviceObjFindByName(driver->devs, "scsi_host11"))) + if (!(objcopy = virNodeDeviceObjListFindByName(driver->devs, + "scsi_host11"))) goto cleanup; xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy)); @@ -5492,7 +5494,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, caps = caps->next; } - if (!(obj = virNodeDeviceObjAssignDef(driver->devs, def))) + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; def = NULL; objdef = virNodeDeviceObjGetDef(obj); @@ -5537,7 +5539,7 @@ testNodeDeviceCreateXML(virConnectPtr conn, /* Unlike the "real" code we don't need the parent_host in order to * call virVHBAManageVport, but still let's make sure the code finds * something valid and no one messed up the mock environment. */ - if (virNodeDeviceObjGetParentHost(driver->devs, def, CREATE_DEVICE) < 0) + if (virNodeDeviceObjListGetParentHost(driver->devs, def, CREATE_DEVICE) < 0) goto cleanup; /* In the real code, we'd call virVHBAManageVport followed by @@ -5598,8 +5600,8 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) /* We do this just for basic validation, but also avoid finding a * vport capable HBA if for some reason our vHBA doesn't exist */ - if (virNodeDeviceObjGetParentHost(driver->devs, def, - EXISTING_DEVICE) < 0) { + if (virNodeDeviceObjListGetParentHost(driver->devs, def, + EXISTING_DEVICE) < 0) { obj = NULL; goto cleanup; } @@ -5609,7 +5611,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) 0); virNodeDeviceObjLock(obj); - virNodeDeviceObjRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver->devs, obj); virNodeDeviceObjFree(obj); obj = NULL; -- 2.9.4
Create local @obj and @def for the API's rather than referencing the devs->objs[i][->def->]. It'll make future patches easier to read. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 60 ++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index f91f4a0..1dbaf83 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -172,12 +172,16 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, size_t i; for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); - if ((devs->objs[i]->def->sysfs_path != NULL) && - (STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) { - return devs->objs[i]; + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDeviceDefPtr def; + + virNodeDeviceObjLock(obj); + def = obj->def; + if ((def->sysfs_path != NULL) && + (STREQ(def->sysfs_path, sysfs_path))) { + return obj; } - virNodeDeviceObjUnlock(devs->objs[i]); + virNodeDeviceObjUnlock(obj); } return NULL; @@ -191,10 +195,14 @@ virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, size_t i; for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); - if (STREQ(devs->objs[i]->def->name, name)) - return devs->objs[i]; - virNodeDeviceObjUnlock(devs->objs[i]); + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDeviceDefPtr def; + + virNodeDeviceObjLock(obj); + def = obj->def; + if (STREQ(def->name, name)) + return obj; + virNodeDeviceObjUnlock(obj); } return NULL; @@ -209,14 +217,16 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, size_t i; for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDevCapsDefPtr cap; - virNodeDeviceObjLock(devs->objs[i]); - if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) && + + virNodeDeviceObjLock(obj); + if ((cap = virNodeDeviceFindFCCapDef(obj)) && STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) && STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) && - virNodeDeviceFindVPORTCapDef(devs->objs[i])) - return devs->objs[i]; - virNodeDeviceObjUnlock(devs->objs[i]); + virNodeDeviceFindVPORTCapDef(obj)) + return obj; + virNodeDeviceObjUnlock(obj); } return NULL; @@ -230,13 +240,15 @@ virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs, size_t i; for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDevCapsDefPtr cap; - virNodeDeviceObjLock(devs->objs[i]); - if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) && + + virNodeDeviceObjLock(obj); + if ((cap = virNodeDeviceFindFCCapDef(obj)) && STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) && - virNodeDeviceFindVPORTCapDef(devs->objs[i])) - return devs->objs[i]; - virNodeDeviceObjUnlock(devs->objs[i]); + virNodeDeviceFindVPORTCapDef(obj)) + return obj; + virNodeDeviceObjUnlock(obj); } return NULL; @@ -250,10 +262,12 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, size_t i; for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); - if (virNodeDeviceObjHasCap(devs->objs[i], cap)) - return devs->objs[i]; - virNodeDeviceObjUnlock(devs->objs[i]); + virNodeDeviceObjPtr obj = devs->objs[i]; + + virNodeDeviceObjLock(obj); + if (virNodeDeviceObjHasCap(obj, cap)) + return obj; + virNodeDeviceObjUnlock(obj); } return NULL; -- 2.9.4
We're about to move the call to nodeDeviceSysfsGetSCSIHostCaps from node_device_driver into virnodedeviceobj, so move the guts of the code from the driver specific node_device_linux_sysfs into its own API since virnodedeviceobj cannot callback into the driver. Nothing in the code deals with sysfs anyway, as that's hidden by the various virSCSIHost* and virVHBA* utility function calls. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 82 +++++++++++++++++++++++++++++++ src/conf/node_device_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/node_device/node_device_linux_sysfs.c | 77 +---------------------------- 4 files changed, 87 insertions(+), 76 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index e5947e6..503b129 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2483,3 +2483,85 @@ virNodeDeviceDeleteVport(virConnectPtr conn, VIR_FREE(scsi_host_name); return ret; } + + +int +virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host) +{ + char *tmp = NULL; + int ret = -1; + + if ((scsi_host->unique_id = + virSCSIHostGetUniqueId(NULL, scsi_host->host)) < 0) { + VIR_DEBUG("Failed to read unique_id for host%d", scsi_host->host); + scsi_host->unique_id = -1; + } + + VIR_DEBUG("Checking if host%d is an FC HBA", scsi_host->host); + + if (virVHBAPathExists(NULL, scsi_host->host)) { + scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST; + + if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, "port_name"))) { + VIR_WARN("Failed to read WWPN for host%d", scsi_host->host); + goto cleanup; + } + VIR_FREE(scsi_host->wwpn); + VIR_STEAL_PTR(scsi_host->wwpn, tmp); + + if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, "node_name"))) { + VIR_WARN("Failed to read WWNN for host%d", scsi_host->host); + goto cleanup; + } + VIR_FREE(scsi_host->wwnn); + VIR_STEAL_PTR(scsi_host->wwnn, tmp); + + if ((tmp = virVHBAGetConfig(NULL, scsi_host->host, "fabric_name"))) { + VIR_FREE(scsi_host->fabric_wwn); + VIR_STEAL_PTR(scsi_host->fabric_wwn, tmp); + } + } + + if (virVHBAIsVportCapable(NULL, scsi_host->host)) { + scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS; + + if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, + "max_npiv_vports"))) { + VIR_WARN("Failed to read max_npiv_vports for host%d", + scsi_host->host); + goto cleanup; + } + + if (virStrToLong_i(tmp, NULL, 10, &scsi_host->max_vports) < 0) { + VIR_WARN("Failed to parse value of max_npiv_vports '%s'", tmp); + goto cleanup; + } + + VIR_FREE(tmp); + if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, + "npiv_vports_inuse"))) { + VIR_WARN("Failed to read npiv_vports_inuse for host%d", + scsi_host->host); + goto cleanup; + } + + if (virStrToLong_i(tmp, NULL, 10, &scsi_host->vports) < 0) { + VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'", tmp); + goto cleanup; + } + } + + ret = 0; + cleanup: + if (ret < 0) { + /* Clear the two flags in case of producing confusing XML output */ + scsi_host->flags &= ~(VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST | + VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS); + + VIR_FREE(scsi_host->wwnn); + VIR_FREE(scsi_host->wwpn); + VIR_FREE(scsi_host->fabric_wwn); + } + VIR_FREE(tmp); + return ret; +} diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 0a5e731..90c7e1f 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -408,4 +408,7 @@ int virNodeDeviceDeleteVport(virConnectPtr conn, virStorageAdapterFCHostPtr fchost); +int +virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host); + #endif /* __VIR_NODE_DEVICE_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f81a035..acd123f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -687,6 +687,7 @@ virNodeDeviceDefParseNode; virNodeDeviceDefParseString; virNodeDeviceDeleteVport; virNodeDeviceGetParentName; +virNodeDeviceGetSCSIHostCaps; virNodeDeviceGetWWNs; diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index e02c384..6f438e5 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -48,82 +48,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs"); int nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host) { - char *tmp = NULL; - int ret = -1; - - if ((scsi_host->unique_id = - virSCSIHostGetUniqueId(NULL, scsi_host->host)) < 0) { - VIR_DEBUG("Failed to read unique_id for host%d", scsi_host->host); - scsi_host->unique_id = -1; - } - - VIR_DEBUG("Checking if host%d is an FC HBA", scsi_host->host); - - if (virVHBAPathExists(NULL, scsi_host->host)) { - scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST; - - if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, "port_name"))) { - VIR_WARN("Failed to read WWPN for host%d", scsi_host->host); - goto cleanup; - } - VIR_FREE(scsi_host->wwpn); - VIR_STEAL_PTR(scsi_host->wwpn, tmp); - - if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, "node_name"))) { - VIR_WARN("Failed to read WWNN for host%d", scsi_host->host); - goto cleanup; - } - VIR_FREE(scsi_host->wwnn); - VIR_STEAL_PTR(scsi_host->wwnn, tmp); - - if ((tmp = virVHBAGetConfig(NULL, scsi_host->host, "fabric_name"))) { - VIR_FREE(scsi_host->fabric_wwn); - VIR_STEAL_PTR(scsi_host->fabric_wwn, tmp); - } - } - - if (virVHBAIsVportCapable(NULL, scsi_host->host)) { - scsi_host->flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS; - - if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, - "max_npiv_vports"))) { - VIR_WARN("Failed to read max_npiv_vports for host%d", - scsi_host->host); - goto cleanup; - } - - if (virStrToLong_i(tmp, NULL, 10, &scsi_host->max_vports) < 0) { - VIR_WARN("Failed to parse value of max_npiv_vports '%s'", tmp); - goto cleanup; - } - - VIR_FREE(tmp); - if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host, - "npiv_vports_inuse"))) { - VIR_WARN("Failed to read npiv_vports_inuse for host%d", - scsi_host->host); - goto cleanup; - } - - if (virStrToLong_i(tmp, NULL, 10, &scsi_host->vports) < 0) { - VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'", tmp); - goto cleanup; - } - } - - ret = 0; - cleanup: - if (ret < 0) { - /* Clear the two flags in case of producing confusing XML output */ - scsi_host->flags &= ~(VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST | - VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS); - - VIR_FREE(scsi_host->wwnn); - VIR_FREE(scsi_host->wwpn); - VIR_FREE(scsi_host->fabric_wwn); - } - VIR_FREE(tmp); - return ret; + return virNodeDeviceGetSCSIHostCaps(scsi_host); } -- 2.9.4
In an overall effort to privatize access to virNodeDeviceObj and virNodeDeviceObjList into the virnodedeviceobj module, move the object list parsing from node_device_driver and replace with a call to a virnodedeviceobj helper. This follows other similar APIs/helpers which peruse the object list looking for some specific data in order to get/return an @device (virNodeDevice) object to the caller. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 33 +++++++++++++++++++++ src/conf/virnodedeviceobj.h | 5 ++++ src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 56 +++++++++++------------------------- 4 files changed, 55 insertions(+), 40 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 1dbaf83..7ebd4e8 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -274,6 +274,39 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, } +virNodeDeviceObjPtr +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, + const char *wwnn, + const char *wwpn) +{ + size_t i; + + for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDevCapsDefPtr cap; + + virNodeDeviceObjLock(obj); + cap = obj->def->caps; + + while (cap) { + if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { + virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host); + if (cap->data.scsi_host.flags & + VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { + if (STREQ(cap->data.scsi_host.wwnn, wwnn) && + STREQ(cap->data.scsi_host.wwpn, wwpn)) + return obj; + } + } + cap = cap->next; + } + virNodeDeviceObjUnlock(obj); + } + + return NULL; +} + + void virNodeDeviceObjFree(virNodeDeviceObjPtr obj) { diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 6194c6c..6ec5ee7 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -53,6 +53,11 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, ATTRIBUTE_NONNULL(2); virNodeDeviceObjPtr +virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, + const char *wwnn, + const char *wwpn); + +virNodeDeviceObjPtr virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index acd123f..589e587 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -969,6 +969,7 @@ virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; virNodeDeviceObjListFindByName; virNodeDeviceObjListFindBySysfsPath; +virNodeDeviceObjListFindSCSIHostByWWNs; virNodeDeviceObjListFree; virNodeDeviceObjListGetNames; virNodeDeviceObjListGetParentHost; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 930f9b6..85a7c88 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -288,9 +288,6 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, const char *wwpn, unsigned int flags) { - size_t i; - virNodeDeviceObjListPtr devs = driver->devs; - virNodeDevCapsDefPtr cap = NULL; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; virNodeDevicePtr device = NULL; @@ -298,48 +295,27 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, virCheckFlags(0, NULL); nodeDeviceLock(); + obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn); + nodeDeviceUnlock(); - for (i = 0; i < devs->count; i++) { - obj = devs->objs[i]; - virNodeDeviceObjLock(obj); - def = virNodeDeviceObjGetDef(obj); - cap = def->caps; - - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - nodeDeviceSysfsGetSCSIHostCaps(&cap->data.scsi_host); - if (cap->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { - if (STREQ(cap->data.scsi_host.wwnn, wwnn) && - STREQ(cap->data.scsi_host.wwpn, wwpn)) { - - if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) - goto error; - - if ((device = virGetNodeDevice(conn, def->name))) { - if (VIR_STRDUP(device->parent, def->parent) < 0) { - virObjectUnref(device); - device = NULL; - } - } - virNodeDeviceObjUnlock(obj); - goto out; - } - } - } - cap = cap->next; - } + if (!obj) + return NULL; - virNodeDeviceObjUnlock(obj); - } + def = virNodeDeviceObjGetDef(obj); - out: - nodeDeviceUnlock(); - return device; + if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) + goto cleanup; - error: + if ((device = virGetNodeDevice(conn, def->name))) { + if (VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + device = NULL; + } + } + + cleanup: virNodeDeviceObjUnlock(obj); - goto out; + return device; } -- 2.9.4
Move the structures to withing virnodedeviceobj.c Move the typedefs from node_device_conf to virnodedeviceobj.h Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.h | 17 ----------------- src/conf/virnodedeviceobj.c | 11 +++++++++++ src/conf/virnodedeviceobj.h | 6 ++++++ 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 90c7e1f..d10683d 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -328,23 +328,6 @@ struct _virNodeDeviceDef { virNodeDevCapsDefPtr caps; /* optional device capabilities */ }; - -typedef struct _virNodeDeviceObj virNodeDeviceObj; -typedef virNodeDeviceObj *virNodeDeviceObjPtr; -struct _virNodeDeviceObj { - virMutex lock; - - virNodeDeviceDefPtr def; /* device definition */ - -}; - -typedef struct _virNodeDeviceObjList virNodeDeviceObjList; -typedef virNodeDeviceObjList *virNodeDeviceObjListPtr; -struct _virNodeDeviceObjList { - size_t count; - virNodeDeviceObjPtr *objs; -}; - char * virNodeDeviceDefFormat(const virNodeDeviceDef *def); diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 7ebd4e8..04dba70 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -32,6 +32,17 @@ VIR_LOG_INIT("conf.virnodedeviceobj"); +struct _virNodeDeviceObj { + virMutex lock; + + virNodeDeviceDefPtr def; /* device definition */ +}; + +struct _virNodeDeviceObjList { + size_t count; + virNodeDeviceObjPtr *objs; +}; + static virNodeDeviceObjPtr virNodeDeviceObjNew(virNodeDeviceDefPtr def) diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 6ec5ee7..1122b67 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -27,6 +27,12 @@ # include "object_event.h" +typedef struct _virNodeDeviceObj virNodeDeviceObj; +typedef virNodeDeviceObj *virNodeDeviceObjPtr; + +typedef struct _virNodeDeviceObjList virNodeDeviceObjList; +typedef virNodeDeviceObjList *virNodeDeviceObjListPtr; + typedef struct _virNodeDeviceDriverState virNodeDeviceDriverState; typedef virNodeDeviceDriverState *virNodeDeviceDriverStatePtr; struct _virNodeDeviceDriverState { -- 2.9.4
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock. This also involves creating a virNodeDeviceEndAPI in order to handle the object cleaup for API's that use the Add or Find API's in order to get a locked/reffed object. The EndAPI will unlock and unref the object returning NULL to indicate to the caller to not use the obj. For now this also involves a forward reference to the Unlock, but that'll be removed shortly when the object is convert to use the virObjectLockable shortly. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 156 ++++++++++++++++++----------------- src/conf/virnodedeviceobj.h | 11 +-- src/libvirt_private.syms | 4 +- src/node_device/node_device_driver.c | 18 ++-- src/node_device/node_device_hal.c | 8 +- src/node_device/node_device_udev.c | 12 +-- src/test/test_driver.c | 34 ++++---- 7 files changed, 119 insertions(+), 124 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 04dba70..3ce54fc 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -33,7 +33,7 @@ VIR_LOG_INIT("conf.virnodedeviceobj"); struct _virNodeDeviceObj { - virMutex lock; + virObjectLockable parent; virNodeDeviceDefPtr def; /* device definition */ }; @@ -44,27 +44,63 @@ struct _virNodeDeviceObjList { }; +static virClassPtr virNodeDeviceObjClass; +static void virNodeDeviceObjDispose(void *opaque); + +static int +virNodeDeviceObjOnceInit(void) +{ + if (!(virNodeDeviceObjClass = virClassNew(virClassForObjectLockable(), + "virNodeDeviceObj", + sizeof(virNodeDeviceObj), + virNodeDeviceObjDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNodeDeviceObj) + + +static void +virNodeDeviceObjDispose(void *opaque) +{ + virNodeDeviceObjPtr obj = opaque; + + virNodeDeviceDefFree(obj->def); +} + + static virNodeDeviceObjPtr virNodeDeviceObjNew(virNodeDeviceDefPtr def) { virNodeDeviceObjPtr obj; - if (VIR_ALLOC(obj) < 0) + if (virNodeDeviceObjInitialize() < 0) return NULL; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virObjectLockableNew(virNodeDeviceObjClass))) return NULL; - } - virNodeDeviceObjLock(obj); + + virObjectLock(obj); obj->def = def; return obj; } +void +virNodeDeviceObjEndAPI(virNodeDeviceObjPtr *obj) +{ + if (!*obj) + return; + + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; +} + + virNodeDeviceDefPtr virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj) { @@ -186,13 +222,13 @@ virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDeviceDefPtr def; - virNodeDeviceObjLock(obj); + virObjectLock(obj); def = obj->def; if ((def->sysfs_path != NULL) && (STREQ(def->sysfs_path, sysfs_path))) { - return obj; + return virObjectRef(obj); } - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); } return NULL; @@ -209,11 +245,11 @@ virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDeviceDefPtr def; - virNodeDeviceObjLock(obj); + virObjectLock(obj); def = obj->def; if (STREQ(def->name, name)) - return obj; - virNodeDeviceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -231,13 +267,13 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDevCapsDefPtr cap; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if ((cap = virNodeDeviceFindFCCapDef(obj)) && STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) && STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) && virNodeDeviceFindVPORTCapDef(obj)) - return obj; - virNodeDeviceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -254,12 +290,12 @@ virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDevCapsDefPtr cap; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if ((cap = virNodeDeviceFindFCCapDef(obj)) && STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) && virNodeDeviceFindVPORTCapDef(obj)) - return obj; - virNodeDeviceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -275,10 +311,10 @@ virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, for (i = 0; i < devs->count; i++) { virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if (virNodeDeviceObjHasCap(obj, cap)) - return obj; - virNodeDeviceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -296,7 +332,7 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj = devs->objs[i]; virNodeDevCapsDefPtr cap; - virNodeDeviceObjLock(obj); + virObjectLock(obj); cap = obj->def->caps; while (cap) { @@ -306,32 +342,18 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { if (STREQ(cap->data.scsi_host.wwnn, wwnn) && STREQ(cap->data.scsi_host.wwpn, wwpn)) - return obj; + return virObjectRef(obj); } } cap = cap->next; } - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); } return NULL; } -void -virNodeDeviceObjFree(virNodeDeviceObjPtr obj) -{ - if (!obj) - return; - - virNodeDeviceDefFree(obj->def); - - virMutexDestroy(&obj->lock); - - VIR_FREE(obj); -} - - virNodeDeviceObjListPtr virNodeDeviceObjListNew(void) { @@ -348,7 +370,7 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) { size_t i; for (i = 0; i < devs->count; i++) - virNodeDeviceObjFree(devs->objs[i]); + virObjectUnref(devs->objs[i]); VIR_FREE(devs->objs); VIR_FREE(devs); } @@ -371,12 +393,11 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) { obj->def = NULL; - virNodeDeviceObjUnlock(obj); - virNodeDeviceObjFree(obj); + virNodeDeviceObjEndAPI(&obj); return NULL; } - return obj; + return virObjectRef(obj); } @@ -386,17 +407,18 @@ virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, { size_t i; - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(devs->objs[i]); + virObjectLock(devs->objs[i]); if (devs->objs[i] == obj) { - virNodeDeviceObjUnlock(devs->objs[i]); + virObjectUnlock(devs->objs[i]); + virObjectUnref(devs->objs[i]); VIR_DELETE_ELEMENT(devs->objs, i, devs->count); break; } - virNodeDeviceObjUnlock(devs->objs[i]); + virObjectUnlock(devs->objs[i]); } } @@ -447,7 +469,7 @@ virNodeDeviceObjListGetParentHostByParent(virNodeDeviceObjListPtr devs, ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -472,7 +494,7 @@ virNodeDeviceObjListGetParentHostByWWNs(virNodeDeviceObjListPtr devs, ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -495,7 +517,7 @@ virNodeDeviceObjListGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -516,7 +538,7 @@ virNodeDeviceObjListFindVportParentHost(virNodeDeviceObjListPtr devs) ret = virNodeDeviceFindFCParentHost(obj); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -549,20 +571,6 @@ virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, } -void -virNodeDeviceObjLock(virNodeDeviceObjPtr obj) -{ - virMutexLock(&obj->lock); -} - - -void -virNodeDeviceObjUnlock(virNodeDeviceObjPtr obj) -{ - virMutexUnlock(&obj->lock); -} - - static bool virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, int type) @@ -624,11 +632,11 @@ virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, for (i = 0; i < devs->count; i++) { virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if ((!aclfilter || aclfilter(conn, obj->def)) && (!cap || virNodeDeviceObjHasCap(obj, cap))) ++ndevs; - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); } return ndevs; @@ -648,16 +656,16 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, for (i = 0; i < devs->count && nnames < maxnames; i++) { virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if ((!aclfilter || aclfilter(conn, obj->def)) && (!cap || virNodeDeviceObjHasCap(obj, cap))) { if (VIR_STRDUP(names[nnames], obj->def->name) < 0) { - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); goto failure; } nnames++; } - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); } return nnames; @@ -719,21 +727,21 @@ virNodeDeviceObjListExport(virConnectPtr conn, for (i = 0; i < devs->count; i++) { virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceObjLock(obj); + virObjectLock(obj); if ((!aclfilter || aclfilter(conn, obj->def)) && virNodeDeviceMatch(obj, flags)) { if (devices) { if (!(device = virGetNodeDevice(conn, obj->def->name)) || VIR_STRDUP(device->parent, obj->def->parent) < 0) { virObjectUnref(device); - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); goto cleanup; } tmp_devices[ndevices] = device; } ndevices++; } - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); } if (tmp_devices) { diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 1122b67..788fb66 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -45,6 +45,8 @@ struct _virNodeDeviceDriverState { virObjectEventStatePtr nodeDeviceEventState; }; +void +virNodeDeviceObjEndAPI(virNodeDeviceObjPtr *obj); virNodeDeviceDefPtr virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj); @@ -76,21 +78,12 @@ virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def, int create); -void -virNodeDeviceObjFree(virNodeDeviceObjPtr dev); - virNodeDeviceObjListPtr virNodeDeviceObjListNew(void); void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs); -void -virNodeDeviceObjLock(virNodeDeviceObjPtr obj); - -void -virNodeDeviceObjUnlock(virNodeDeviceObjPtr obj); - typedef bool (*virNodeDeviceObjListFilter)(virConnectPtr conn, virNodeDeviceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 589e587..bb977a4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -963,7 +963,7 @@ virNetworkObjUpdateAssignDef; # conf/virnodedeviceobj.h -virNodeDeviceObjFree; +virNodeDeviceObjEndAPI; virNodeDeviceObjGetDef; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; @@ -976,8 +976,6 @@ virNodeDeviceObjListGetParentHost; virNodeDeviceObjListNew; virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; -virNodeDeviceObjLock; -virNodeDeviceObjUnlock; # conf/virnwfilterobj.h diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 85a7c88..0a19908 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -277,7 +277,7 @@ nodeDeviceLookupByName(virConnectPtr conn, } cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return device; } @@ -314,7 +314,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, } cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return device; } @@ -345,7 +345,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device, ret = virNodeDeviceDefFormat(def); cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -373,7 +373,7 @@ nodeDeviceGetParent(virNodeDevicePtr device) } cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -411,7 +411,7 @@ nodeDeviceNumOfCaps(virNodeDevicePtr device) ret = ncaps; cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -460,7 +460,7 @@ nodeDeviceListCaps(virNodeDevicePtr device, ret = ncaps; cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); if (ret == -1) { --ncaps; while (--ncaps >= 0) @@ -613,8 +613,7 @@ nodeDeviceDestroy(virNodeDevicePtr device) * to be taken, so grab the object def which will have the various * fields used to search (name, parent, parent_wwnn, parent_wwpn, * or parent_fabric_wwn) and drop the object lock. */ - virNodeDeviceObjUnlock(obj); - obj = NULL; + virNodeDeviceObjEndAPI(&obj); if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, EXISTING_DEVICE)) < 0) goto cleanup; @@ -626,8 +625,7 @@ nodeDeviceDestroy(virNodeDevicePtr device) cleanup: nodeDeviceUnlock(); - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); VIR_FREE(wwnn); VIR_FREE(wwpn); return ret; diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 5d99e79..b220798 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -490,7 +490,7 @@ dev_create(const char *udi) objdef->sysfs_path = devicePath; - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); nodeDeviceUnlock(); return; @@ -520,7 +520,7 @@ dev_refresh(const char *udi) nodeDeviceUnlock(); if (obj) { - virNodeDeviceObjFree(obj); + virObjectUnref(obj); dev_create(udi); } } @@ -549,7 +549,7 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, else VIR_DEBUG("no device named %s", name); nodeDeviceUnlock(); - virNodeDeviceObjFree(obj); + virObjectUnref(obj); } @@ -568,7 +568,7 @@ device_cap_added(LibHalContext *ctx, if (obj) { def = virNodeDeviceObjGetDef(obj); (void)gather_capability(ctx, udi, cap, &def->caps); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); } else { VIR_DEBUG("no device named %s", name); } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e1b9a5c..1b10c16 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1332,7 +1332,7 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); virNodeDeviceObjListRemove(driver->devs, obj); - virNodeDeviceObjFree(obj); + virObjectUnref(obj); if (event) virObjectEventStateQueue(driver->nodeDeviceEventState, event); @@ -1369,10 +1369,10 @@ udevSetParent(struct udev_device *device, parent_sysfs_path))) { objdef = virNodeDeviceObjGetDef(obj); if (VIR_STRDUP(def->parent, objdef->name) < 0) { - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); goto cleanup; } - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); if (VIR_STRDUP(def->parent_sysfs_path, parent_sysfs_path) < 0) goto cleanup; @@ -1425,7 +1425,7 @@ udevAddOneDevice(struct udev_device *device) goto cleanup; if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); new_device = false; } @@ -1442,7 +1442,7 @@ udevAddOneDevice(struct udev_device *device) else event = virNodeDeviceEventUpdateNew(objdef->name); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); ret = 0; @@ -1724,7 +1724,7 @@ udevSetupSystemDev(void) if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); ret = 0; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6dfabfa..c41f443 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1175,7 +1175,7 @@ testParseNodedevs(testDriverPtr privconn, goto error; } - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); } ret = 0; @@ -4320,7 +4320,7 @@ testCreateVport(testDriverPtr driver, * create the vHBA. In the long run the result is the same. */ if (!(obj = testNodeDeviceMockCreateVport(driver, wwnn, wwpn))) return -1; - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return 0; } @@ -4532,7 +4532,7 @@ testDestroyVport(testDriverPtr privconn, 0); virNodeDeviceObjListRemove(privconn->devs, obj); - virNodeDeviceObjFree(obj); + virObjectUnref(obj); testObjectEventQueue(privconn, event); return 0; @@ -5333,7 +5333,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name) } } - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -5352,7 +5352,7 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev, ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj)); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -5375,7 +5375,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) "%s", _("no parent for this device")); } - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ret; } @@ -5396,7 +5396,7 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev) for (caps = def->caps; caps; caps = caps->next) ++ncaps; - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ncaps; } @@ -5421,13 +5421,13 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) ncaps++; } - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return ncaps; error: while (--ncaps >= 0) VIR_FREE(names[ncaps]); - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); return -1; } @@ -5458,7 +5458,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, goto cleanup; xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy)); - virNodeDeviceObjUnlock(objcopy); + virNodeDeviceObjEndAPI(&objcopy); if (!xml) goto cleanup; @@ -5562,8 +5562,7 @@ testNodeDeviceCreateXML(virConnectPtr conn, dev = NULL; cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testDriverUnlock(driver); virNodeDeviceDefFree(def); virObjectUnref(dev); @@ -5596,13 +5595,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) * taken, so we have to dup the parent's name and drop the lock * before calling it. We don't need the reference to the object * any more once we have the parent's name. */ - virNodeDeviceObjUnlock(obj); + virObjectUnlock(obj); /* We do this just for basic validation, but also avoid finding a * vport capable HBA if for some reason our vHBA doesn't exist */ if (virNodeDeviceObjListGetParentHost(driver->devs, def, EXISTING_DEVICE) < 0) { - obj = NULL; + virObjectLock(obj); goto cleanup; } @@ -5610,14 +5609,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) VIR_NODE_DEVICE_EVENT_DELETED, 0); - virNodeDeviceObjLock(obj); + virObjectLock(obj); virNodeDeviceObjListRemove(driver->devs, obj); - virNodeDeviceObjFree(obj); + virObjectUnref(obj); obj = NULL; cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testObjectEventQueue(driver, event); VIR_FREE(parent_name); VIR_FREE(wwnn); -- 2.9.4
On Mon, Jul 03, 2017 at 05:25:28PM -0400, John Ferlan wrote:
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock.
This also involves creating a virNodeDeviceEndAPI in order to handle the object cleaup for API's that use the Add or Find API's in order to get a locked/reffed object. The EndAPI will unlock and unref the object returning NULL to indicate to the caller to not use the obj.
For now this also involves a forward reference to the Unlock, but that'll be removed shortly when the object is convert to use the virObjectLockable shortly.
Which hunk is ^this paragraph still relevant for? Erik
On 07/10/2017 11:43 AM, Erik Skultety wrote:
On Mon, Jul 03, 2017 at 05:25:28PM -0400, John Ferlan wrote:
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock.
This also involves creating a virNodeDeviceEndAPI in order to handle the object cleaup for API's that use the Add or Find API's in order to get a locked/reffed object. The EndAPI will unlock and unref the object returning NULL to indicate to the caller to not use the obj.
For now this also involves a forward reference to the Unlock, but that'll be removed shortly when the object is convert to use the virObjectLockable shortly.
Which hunk is ^this paragraph still relevant for?
Erik
Changes that are no longer relevant... I will remove it. John
On Mon, Jul 03, 2017 at 05:25:28PM -0400, John Ferlan wrote:
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock.
This also involves creating a virNodeDeviceEndAPI in order to handle the object cleaup for API's that use the Add or Find API's in order
s/cleaup/cleanup [...]
cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); if (ret == -1) { --ncaps; while (--ncaps >= 0) @@ -613,8 +613,7 @@ nodeDeviceDestroy(virNodeDevicePtr device) * to be taken, so grab the object def which will have the various * fields used to search (name, parent, parent_wwnn, parent_wwpn, * or parent_fabric_wwn) and drop the object lock. */
^This commentary got me thinking. There's a deadlock because of how the locks are acquired earlier in this function. Patch 14 solves the deadlock in exchange for a race, so see my comment in patch 14. [...]
cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testDriverUnlock(driver); virNodeDeviceDefFree(def); virObjectUnref(dev); @@ -5596,13 +5595,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) * taken, so we have to dup the parent's name and drop the lock * before calling it. We don't need the reference to the object * any more once we have the parent's name. */
Not that this patch would be touching it directly, but ^this commentary is pretty much useless now, since @parent_name is useless in this function, nodeDeviceDestroy doesn't work that way anymore. Erik
On 07/11/2017 10:38 AM, Erik Skultety wrote:
On Mon, Jul 03, 2017 at 05:25:28PM -0400, John Ferlan wrote:
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock.
This also involves creating a virNodeDeviceEndAPI in order to handle the object cleaup for API's that use the Add or Find API's in order
s/cleaup/cleanup
eagle eyes
[...]
cleanup: - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); if (ret == -1) { --ncaps; while (--ncaps >= 0) @@ -613,8 +613,7 @@ nodeDeviceDestroy(virNodeDevicePtr device) * to be taken, so grab the object def which will have the various * fields used to search (name, parent, parent_wwnn, parent_wwpn, * or parent_fabric_wwn) and drop the object lock. */
^This commentary got me thinking. There's a deadlock because of how the locks are acquired earlier in this function. Patch 14 solves the deadlock in exchange for a race, so see my comment in patch 14.
[...]
I'll respond there. Not sure where the deadlock would be here, but maybe the answer there will clear things up...
cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testDriverUnlock(driver); virNodeDeviceDefFree(def); virObjectUnref(dev); @@ -5596,13 +5595,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) * taken, so we have to dup the parent's name and drop the lock * before calling it. We don't need the reference to the object * any more once we have the parent's name. */
Not that this patch would be touching it directly, but ^this commentary is pretty much useless now, since @parent_name is useless in this function, nodeDeviceDestroy doesn't work that way anymore.
Erik
Hmmm, I see said the blind man. Looks like commit id '7ad479d0b' should have removed @parent_name as well. That should be a separate patch - I can create and add it to the end. John
cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testDriverUnlock(driver); virNodeDeviceDefFree(def); virObjectUnref(dev); @@ -5596,13 +5595,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) * taken, so we have to dup the parent's name and drop the lock * before calling it. We don't need the reference to the object * any more once we have the parent's name. */
Not that this patch would be touching it directly, but ^this commentary is pretty much useless now, since @parent_name is useless in this function, nodeDeviceDestroy doesn't work that way anymore.
Erik
Hmmm, I see said the blind man. Looks like commit id '7ad479d0b' should have removed @parent_name as well.
That should be a separate patch - I can create and add it to the end.
Sure, that was the expected outcome :), judging by a quick look at the hunk only, I don't see a problem having such patch at the beginning of the series, do you hit any merge conflicts? Erik
On 07/14/2017 05:28 AM, Erik Skultety wrote:
cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testDriverUnlock(driver); virNodeDeviceDefFree(def); virObjectUnref(dev); @@ -5596,13 +5595,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) * taken, so we have to dup the parent's name and drop the lock * before calling it. We don't need the reference to the object * any more once we have the parent's name. */
Not that this patch would be touching it directly, but ^this commentary is pretty much useless now, since @parent_name is useless in this function, nodeDeviceDestroy doesn't work that way anymore.
Erik
Hmmm, I see said the blind man. Looks like commit id '7ad479d0b' should have removed @parent_name as well.
That should be a separate patch - I can create and add it to the end.
Sure, that was the expected outcome :), judging by a quick look at the hunk only, I don't see a problem having such patch at the beginning of the series, do you hit any merge conflicts?
Erik
Yes multiple... Mainly because I changed the comment. I could add it immediately after this patch without conflicts. John
On Fri, Jul 14, 2017 at 10:12:12AM -0400, John Ferlan wrote:
On 07/14/2017 05:28 AM, Erik Skultety wrote:
cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjEndAPI(&obj); testDriverUnlock(driver); virNodeDeviceDefFree(def); virObjectUnref(dev); @@ -5596,13 +5595,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) * taken, so we have to dup the parent's name and drop the lock * before calling it. We don't need the reference to the object * any more once we have the parent's name. */
Not that this patch would be touching it directly, but ^this commentary is pretty much useless now, since @parent_name is useless in this function, nodeDeviceDestroy doesn't work that way anymore.
Erik
Hmmm, I see said the blind man. Looks like commit id '7ad479d0b' should have removed @parent_name as well.
That should be a separate patch - I can create and add it to the end.
Sure, that was the expected outcome :), judging by a quick look at the hunk only, I don't see a problem having such patch at the beginning of the series, do you hit any merge conflicts?
Erik
Yes multiple... Mainly because I changed the comment. I could add it immediately after this patch without conflicts.
Disregard my comment then and proceed with your proposal. Erik
Rather than use a forward linked list of elements, it'll be much more efficient to use a hash table to reference the elements by unique name and to perform hash searches. This patch does all the heavy lifting of converting the list object to use a self locking list that contains the hash table. Each of the FindBy functions that do not involve finding the object by it's key (name) is converted to use virHashSearch in order to find the specific object. When searching for the key (name), it's possible to use virHashLookup. For any of the list perusal functions that are required to evaluate each object, the virHashForEach function is used. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 580 ++++++++++++++++++++++++++++++-------------- 1 file changed, 404 insertions(+), 176 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 3ce54fc..9def83f 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -25,6 +25,7 @@ #include "viralloc.h" #include "virnodedeviceobj.h" #include "virerror.h" +#include "virhash.h" #include "virlog.h" #include "virstring.h" @@ -39,13 +40,19 @@ struct _virNodeDeviceObj { }; struct _virNodeDeviceObjList { - size_t count; - virNodeDeviceObjPtr *objs; + virObjectLockable parent; + + /* name string -> virNodeDeviceObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; + }; static virClassPtr virNodeDeviceObjClass; +static virClassPtr virNodeDeviceObjListClass; static void virNodeDeviceObjDispose(void *opaque); +static void virNodeDeviceObjListDispose(void *opaque); static int virNodeDeviceObjOnceInit(void) @@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void) virNodeDeviceObjDispose))) return -1; + if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(), + "virNodeDeviceObjList", + sizeof(virNodeDeviceObjList), + virNodeDeviceObjListDispose))) + return -1; + return 0; } @@ -212,26 +225,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj) } +static int +virNodeDeviceObjListFindBySysfsPathCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + virNodeDeviceDefPtr def; + const char *sysfs_path = opaque; + int want = 0; + + virObjectLock(obj); + def = obj->def; + if (STREQ_NULLABLE(def->sysfs_path, sysfs_path)) + want = 1; + virObjectUnlock(obj); + return want; +} + + virNodeDeviceObjPtr virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, const char *sysfs_path) { - size_t i; + virNodeDeviceObjPtr obj; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceDefPtr def; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback, + (void *)sysfs_path); + virObjectRef(obj); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - def = obj->def; - if ((def->sysfs_path != NULL) && - (STREQ(def->sysfs_path, sysfs_path))) { - return virObjectRef(obj); - } - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +static virNodeDeviceObjPtr +virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs, + const char *name) +{ + return virObjectRef(virHashLookup(devs->objs, name)); } @@ -239,20 +275,42 @@ virNodeDeviceObjPtr virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, const char *name) { - size_t i; - - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceDefPtr def; + virNodeDeviceObjPtr obj; + virObjectLock(devs); + obj = virNodeDeviceObjListFindByNameLocked(devs, name); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - def = obj->def; - if (STREQ(def->name, name)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +struct virNodeDeviceObjListFindByWWNsData { + const char *parent_wwnn; + const char *parent_wwpn; +}; + +static int +virNodeDeviceObjListFindByWWNsCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + struct virNodeDeviceObjListFindByWWNsData *data = + (struct virNodeDeviceObjListFindByWWNsData *) opaque; + virNodeDevCapsDefPtr cap; + int want = 0; + + virObjectLock(obj); + if ((cap = virNodeDeviceFindFCCapDef(obj)) && + STREQ_NULLABLE(cap->data.scsi_host.wwnn, data->parent_wwnn) && + STREQ_NULLABLE(cap->data.scsi_host.wwpn, data->parent_wwpn) && + virNodeDeviceFindVPORTCapDef(obj)) + want = 1; + virObjectUnlock(obj); + return want; } @@ -261,22 +319,40 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, const char *parent_wwnn, const char *parent_wwpn) { - size_t i; + virNodeDeviceObjPtr obj; + struct virNodeDeviceObjListFindByWWNsData data = { + .parent_wwnn = parent_wwnn, .parent_wwpn = parent_wwpn }; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDevCapsDefPtr cap; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByWWNsCallback, + &data); + virObjectRef(obj); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - if ((cap = virNodeDeviceFindFCCapDef(obj)) && - STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) && - STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) && - virNodeDeviceFindVPORTCapDef(obj)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +static int +virNodeDeviceObjListFindByFabricWWNCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + const char *matchstr = opaque; + virNodeDevCapsDefPtr cap; + int want = 0; + + virObjectLock(obj); + if ((cap = virNodeDeviceFindFCCapDef(obj)) && + STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, matchstr) && + virNodeDeviceFindVPORTCapDef(obj)) + want = 1; + virObjectUnlock(obj); + return want; } @@ -284,21 +360,35 @@ static virNodeDeviceObjPtr virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs, const char *parent_fabric_wwn) { - size_t i; + virNodeDeviceObjPtr obj; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDevCapsDefPtr cap; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByFabricWWNCallback, + (void *)parent_fabric_wwn); + virObjectRef(obj); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - if ((cap = virNodeDeviceFindFCCapDef(obj)) && - STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) && - virNodeDeviceFindVPORTCapDef(obj)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +static int +virNodeDeviceObjListFindByCapCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + const char *matchstr = opaque; + int want = 0; + + virObjectLock(obj); + if (virNodeDeviceObjHasCap(obj, matchstr)) + want = 1; + virObjectUnlock(obj); + return want; } @@ -306,18 +396,59 @@ static virNodeDeviceObjPtr virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, const char *cap) { - size_t i; + virNodeDeviceObjPtr obj; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByCapCallback, + (void *)cap); + virObjectRef(obj); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - if (virNodeDeviceObjHasCap(obj, cap)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +struct virNodeDeviceObjListFindSCSIHostByWWNsData { + const char *wwnn; + const char *wwpn; +}; + +static int +virNodeDeviceObjListFindSCSIHostByWWNsCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + virNodeDeviceDefPtr def; + struct virNodeDeviceObjListFindSCSIHostByWWNsData *data = + (struct virNodeDeviceObjListFindSCSIHostByWWNsData *) opaque; + virNodeDevCapsDefPtr cap; + int want = 0; + + virObjectLock(obj); + def = obj->def; + cap = def->caps; + + while (cap) { + if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { + virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host); + if (cap->data.scsi_host.flags & + VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { + if (STREQ(cap->data.scsi_host.wwnn, data->wwnn) && + STREQ(cap->data.scsi_host.wwpn, data->wwpn)) { + want = 1; + break; + } + } + } + cap = cap->next; + } + + virObjectUnlock(obj); + return want; } @@ -326,31 +457,30 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, const char *wwnn, const char *wwpn) { - size_t i; + virNodeDeviceObjPtr obj; + struct virNodeDeviceObjListFindSCSIHostByWWNsData data = { + .wwnn = wwnn, .wwpn = wwpn }; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDevCapsDefPtr cap; + virObjectLock(devs); + obj = virHashSearch(devs->objs, + virNodeDeviceObjListFindSCSIHostByWWNsCallback, + &data); + virObjectRef(obj); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - cap = obj->def->caps; - - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host); - if (cap->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { - if (STREQ(cap->data.scsi_host.wwnn, wwnn) && - STREQ(cap->data.scsi_host.wwpn, wwpn)) - return virObjectRef(obj); - } - } - cap = cap->next; - } - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +static void +virNodeDeviceObjListDispose(void *obj) +{ + virNodeDeviceObjListPtr devs = obj; + + virHashFree(devs->objs); } @@ -359,8 +489,17 @@ virNodeDeviceObjListNew(void) { virNodeDeviceObjListPtr devs; - if (VIR_ALLOC(devs) < 0) + if (virNodeDeviceObjInitialize() < 0) return NULL; + + if (!(devs = virObjectLockableNew(virNodeDeviceObjListClass))) + return NULL; + + if (!(devs->objs = virHashCreate(50, virObjectFreeHashData))) { + virObjectUnref(devs); + return NULL; + } + return devs; } @@ -368,11 +507,7 @@ virNodeDeviceObjListNew(void) void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) { - size_t i; - for (i = 0; i < devs->count; i++) - virObjectUnref(devs->objs[i]); - VIR_FREE(devs->objs); - VIR_FREE(devs); + virObjectUnref(devs); } @@ -381,23 +516,32 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def) { virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr objdef; - if ((obj = virNodeDeviceObjListFindByName(devs, def->name))) { + virObjectLock(devs); + + if ((obj = virNodeDeviceObjListFindByNameLocked(devs, def->name))) { + virObjectLock(obj); virNodeDeviceDefFree(obj->def); obj->def = def; - return obj; - } - - if (!(obj = virNodeDeviceObjNew(def))) - return NULL; + goto cleanup; + } else { + if (!(obj = virNodeDeviceObjNew(def))) + goto cleanup; + objdef = obj->def; + + if (virHashAddEntry(devs->objs, objdef->name, obj) < 0) { + obj->def = NULL; + virNodeDeviceObjEndAPI(&obj); + goto cleanup; + } - if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) { - obj->def = NULL; - virNodeDeviceObjEndAPI(&obj); - return NULL; + virObjectRef(obj); } - return virObjectRef(obj); + cleanup: + virObjectUnlock(devs); + return obj; } @@ -405,21 +549,20 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj) { - size_t i; - - virObjectUnlock(obj); + virNodeDeviceDefPtr def; - for (i = 0; i < devs->count; i++) { - virObjectLock(devs->objs[i]); - if (devs->objs[i] == obj) { - virObjectUnlock(devs->objs[i]); - virObjectUnref(devs->objs[i]); + if (!obj) + return; + def = obj->def; - VIR_DELETE_ELEMENT(devs->objs, i, devs->count); - break; - } - virObjectUnlock(devs->objs[i]); - } + virObjectRef(obj); + virObjectUnlock(obj); + virObjectLock(devs); + virObjectLock(obj); + virHashRemoveEntry(devs->objs, def->name); + virObjectUnlock(obj); + virObjectUnref(obj); + virObjectUnlock(devs); } @@ -621,25 +764,89 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, } +struct virNodeDeviceCountData { + virConnectPtr conn; + virNodeDeviceObjListFilter aclfilter; + const char *matchstr; + int count; +}; + +static int +virNodeDeviceObjListNumOfDevicesCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNodeDeviceObjPtr obj = payload; + virNodeDeviceDefPtr def; + struct virNodeDeviceCountData *data = opaque; + virNodeDeviceObjListFilter aclfilter = data->aclfilter; + + virObjectLock(obj); + def = obj->def; + if ((!aclfilter || aclfilter(data->conn, def)) && + (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) + data->count++; + + virObjectUnlock(obj); + return 0; +} + + int virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, virConnectPtr conn, const char *cap, virNodeDeviceObjListFilter aclfilter) { - size_t i; - int ndevs = 0; + struct virNodeDeviceCountData data = { + .conn = conn, .aclfilter = aclfilter, .matchstr = cap, .count = 0 }; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virObjectLock(obj); - if ((!aclfilter || aclfilter(conn, obj->def)) && - (!cap || virNodeDeviceObjHasCap(obj, cap))) - ++ndevs; - virObjectUnlock(obj); - } + virObjectLock(devs); + virHashForEach(devs->objs, virNodeDeviceObjListNumOfDevicesCallback, &data); + virObjectUnlock(devs); + + return data.count; +} + + +struct virNodeDeviceGetNamesData { + virConnectPtr conn; + virNodeDeviceObjListFilter aclfilter; + const char *matchstr; + int nnames; + char **names; + int maxnames; + bool error; +}; + +static int +virNodeDeviceObjListGetNamesCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNodeDeviceObjPtr obj = payload; + virNodeDeviceDefPtr def; + struct virNodeDeviceGetNamesData *data = opaque; + virNodeDeviceObjListFilter aclfilter = data->aclfilter; + + if (data->error) + return 0; - return ndevs; + virObjectLock(obj); + def = obj->def; + + if ((!aclfilter || aclfilter(data->conn, def)) && + (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) { + if (VIR_STRDUP(data->names[data->nnames], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nnames++; + } + + cleanup: + virObjectUnlock(obj); + return 0; } @@ -651,28 +858,22 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, char **const names, int maxnames) { - int nnames = 0; - size_t i; + struct virNodeDeviceGetNamesData data = { + .conn = conn, .aclfilter = aclfilter, .matchstr = cap, .names = names, + .nnames = 0, .maxnames = maxnames, .error = false }; - for (i = 0; i < devs->count && nnames < maxnames; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virObjectLock(obj); - if ((!aclfilter || aclfilter(conn, obj->def)) && - (!cap || virNodeDeviceObjHasCap(obj, cap))) { - if (VIR_STRDUP(names[nnames], obj->def->name) < 0) { - virObjectUnlock(obj); - goto failure; - } - nnames++; - } - virObjectUnlock(obj); - } + virObjectLock(devs); + virHashForEach(devs->objs, virNodeDeviceObjListGetNamesCallback, &data); + virObjectUnlock(devs); + + if (data.error) + goto error; - return nnames; + return data.nnames; - failure: - while (--nnames >= 0) - VIR_FREE(names[nnames]); + error: + while (--data.nnames) + VIR_FREE(data.names[data.nnames]); return -1; } @@ -709,6 +910,51 @@ virNodeDeviceMatch(virNodeDeviceObjPtr obj, #undef MATCH +struct virNodeDeviceObjListExportData { + virConnectPtr conn; + virNodeDeviceObjListFilter aclfilter; + unsigned int flags; + virNodeDevicePtr *devices; + int ndevices; + bool error; +}; + +static int +virNodeDeviceObjListExportCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNodeDeviceObjPtr obj = payload; + virNodeDeviceDefPtr def; + struct virNodeDeviceObjListExportData *data = opaque; + virNodeDevicePtr device = NULL; + + if (data->error) + return 0; + + virObjectLock(obj); + def = obj->def; + + if ((!data->aclfilter || data->aclfilter(data->conn, def)) && + virNodeDeviceMatch(obj, data->flags)) { + if (data->devices) { + if (!(device = virGetNodeDevice(data->conn, def->name)) || + VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + data->error = true; + goto cleanup; + } + data->devices[data->ndevices] = device; + } + data->ndevices++; + } + + cleanup: + virObjectUnlock(obj); + return 0; +} + + int virNodeDeviceObjListExport(virConnectPtr conn, virNodeDeviceObjListPtr devs, @@ -716,49 +962,31 @@ virNodeDeviceObjListExport(virConnectPtr conn, virNodeDeviceObjListFilter aclfilter, unsigned int flags) { - virNodeDevicePtr *tmp_devices = NULL; - virNodeDevicePtr device = NULL; - int ndevices = 0; - int ret = -1; - size_t i; + struct virNodeDeviceObjListExportData data = { + .conn = conn, .aclfilter = aclfilter, .flags = flags, + .devices = NULL, .ndevices = 0, .error = false }; + + virObjectLock(devs); + if (devices && + VIR_ALLOC_N(data.devices, virHashSize(devs->objs) + 1) < 0) { + virObjectUnlock(devs); + return -1; + } - if (devices && VIR_ALLOC_N(tmp_devices, devs->count + 1) < 0) - goto cleanup; + virHashForEach(devs->objs, virNodeDeviceObjListExportCallback, &data); + virObjectUnlock(devs); - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virObjectLock(obj); - if ((!aclfilter || aclfilter(conn, obj->def)) && - virNodeDeviceMatch(obj, flags)) { - if (devices) { - if (!(device = virGetNodeDevice(conn, obj->def->name)) || - VIR_STRDUP(device->parent, obj->def->parent) < 0) { - virObjectUnref(device); - virObjectUnlock(obj); - goto cleanup; - } - tmp_devices[ndevices] = device; - } - ndevices++; - } - virObjectUnlock(obj); - } + if (data.error) + goto cleanup; - if (tmp_devices) { - /* trim the array to the final size */ - ignore_value(VIR_REALLOC_N(tmp_devices, ndevices + 1)); - *devices = tmp_devices; - tmp_devices = NULL; - } + if (data.devices) { + ignore_value(VIR_REALLOC_N(data.devices, data.ndevices + 1)); + *devices = data.devices; + } - ret = ndevices; + return data.ndevices; cleanup: - if (tmp_devices) { - for (i = 0; i < ndevices; i++) - virObjectUnref(tmp_devices[i]); - } - - VIR_FREE(tmp_devices); - return ret; + virObjectListFree(data.devices); + return -1; } -- 2.9.4
Since virnodedeviceobj now has a self-lockable hash table, there's no need to lock the table from the driver for processing. Thus remove the locks from the driver for NodeDeviceObjList mgmt. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 61 +++++++----------------------------- src/node_device/node_device_hal.c | 16 ++-------- src/node_device/node_device_udev.c | 12 +++---- 3 files changed, 18 insertions(+), 71 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0a19908..f3a6cfc 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -174,19 +174,13 @@ nodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) { - int ndevs = 0; - if (virNodeNumOfDevicesEnsureACL(conn) < 0) return -1; virCheckFlags(0, -1); - nodeDeviceLock(); - ndevs = virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap, - virNodeNumOfDevicesCheckACL); - nodeDeviceUnlock(); - - return ndevs; + return virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap, + virNodeNumOfDevicesCheckACL); } @@ -197,20 +191,14 @@ nodeListDevices(virConnectPtr conn, int maxnames, unsigned int flags) { - int nnames; - if (virNodeListDevicesEnsureACL(conn) < 0) return -1; virCheckFlags(0, -1); - nodeDeviceLock(); - nnames = virNodeDeviceObjListGetNames(driver->devs, conn, - virNodeListDevicesCheckACL, - cap, names, maxnames); - nodeDeviceUnlock(); - - return nnames; + return virNodeDeviceObjListGetNames(driver->devs, conn, + virNodeListDevicesCheckACL, + cap, names, maxnames); } @@ -219,19 +207,14 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, virNodeDevicePtr **devices, unsigned int flags) { - int ret = -1; - virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP, -1); if (virConnectListAllNodeDevicesEnsureACL(conn) < 0) return -1; - nodeDeviceLock(); - ret = virNodeDeviceObjListExport(conn, driver->devs, devices, - virConnectListAllNodeDevicesCheckACL, - flags); - nodeDeviceUnlock(); - return ret; + return virNodeDeviceObjListExport(conn, driver->devs, devices, + virConnectListAllNodeDevicesCheckACL, + flags); } @@ -240,11 +223,7 @@ nodeDeviceObjFindByName(const char *name) { virNodeDeviceObjPtr obj; - nodeDeviceLock(); - obj = virNodeDeviceObjListFindByName(driver->devs, name); - nodeDeviceUnlock(); - - if (!obj) { + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, name))) { virReportError(VIR_ERR_NO_NODE_DEVICE, _("no node device with matching name '%s'"), name); @@ -294,11 +273,8 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, virCheckFlags(0, NULL); - nodeDeviceLock(); - obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn); - nodeDeviceUnlock(); - - if (!obj) + if (!(obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, + wwnn, wwpn))) return NULL; def = virNodeDeviceObjGetDef(obj); @@ -509,13 +485,6 @@ nodeDeviceFindNewDevice(virConnectPtr conn, virNodeDevicePtr device = NULL; time_t start = 0, now = 0; - /* The thread that creates the device takes the driver lock, so we - * must release it in order to allow the device to be created. - * We're not doing anything with the driver pointer at this point, - * so it's safe to release it, assuming that the pointer itself - * doesn't become invalid. */ - nodeDeviceUnlock(); - nodeDeviceGetTime(&start); while ((now - start) < LINUX_NEW_DEVICE_WAIT_TIME) { @@ -532,8 +501,6 @@ nodeDeviceFindNewDevice(virConnectPtr conn, break; } - nodeDeviceLock(); - return device; } @@ -552,8 +519,6 @@ nodeDeviceCreateXML(virConnectPtr conn, virCheckFlags(0, NULL); virt_type = virConnectGetType(conn); - nodeDeviceLock(); - if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type))) goto cleanup; @@ -580,7 +545,6 @@ nodeDeviceCreateXML(virConnectPtr conn, "wwnn '%s' and wwpn '%s'"), def->name, wwnn, wwpn); cleanup: - nodeDeviceUnlock(); virNodeDeviceDefFree(def); VIR_FREE(wwnn); VIR_FREE(wwpn); @@ -601,8 +565,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) return -1; def = virNodeDeviceObjGetDef(obj); - nodeDeviceLock(); - if (virNodeDeviceDestroyEnsureACL(device->conn, def) < 0) goto cleanup; @@ -624,7 +586,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) ret = 0; cleanup: - nodeDeviceUnlock(); virNodeDeviceObjEndAPI(&obj); VIR_FREE(wwnn); VIR_FREE(wwpn); diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index b220798..8731e3b 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -508,20 +508,15 @@ dev_refresh(const char *udi) const char *name = hal_name(udi); virNodeDeviceObjPtr obj; - nodeDeviceLock(); if ((obj = virNodeDeviceObjListFindByName(driver->devs, name))) { /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ virNodeDeviceObjListRemove(driver->devs, obj); - } else { - VIR_DEBUG("no device named %s", name); - } - nodeDeviceUnlock(); - - if (obj) { virObjectUnref(obj); dev_create(udi); + } else { + VIR_DEBUG("no device named %s", name); } } @@ -541,14 +536,12 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, const char *name = hal_name(udi); virNodeDeviceObjPtr obj; - nodeDeviceLock(); obj = virNodeDeviceObjListFindByName(driver->devs, name); VIR_DEBUG("%s", name); if (obj) virNodeDeviceObjListRemove(driver->devs, obj); else VIR_DEBUG("no device named %s", name); - nodeDeviceUnlock(); virObjectUnref(obj); } @@ -561,11 +554,8 @@ device_cap_added(LibHalContext *ctx, virNodeDeviceObjPtr obj; virNodeDeviceDefPtr def; - nodeDeviceLock(); - obj = virNodeDeviceObjListFindByName(driver->devs, name); - nodeDeviceUnlock(); VIR_DEBUG("%s %s", cap, name); - if (obj) { + if ((obj = virNodeDeviceObjListFindByName(driver->devs, name))) { def = virNodeDeviceObjGetDef(obj); (void)gather_capability(ctx, udi, cap, &def->caps); virNodeDeviceObjEndAPI(&obj); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1b10c16..f56a647 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1607,7 +1607,6 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, const char *action = NULL; int udev_fd = -1; - nodeDeviceLock(); udev_fd = udev_monitor_get_fd(udev_monitor); if (fd != udev_fd) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1639,7 +1638,6 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, cleanup: udev_device_unref(device); - nodeDeviceUnlock(); return; } @@ -1767,7 +1765,6 @@ nodeStateInitialize(bool privileged, { udevPrivate *priv = NULL; struct udev *udev = NULL; - int ret = -1; if (VIR_ALLOC(priv) < 0) return -1; @@ -1848,17 +1845,16 @@ nodeStateInitialize(bool privileged, /* Populate with known devices */ + nodeDeviceUnlock(); if (udevEnumerateDevices(udev) != 0) goto cleanup; - ret = 0; + return 0; cleanup: nodeDeviceUnlock(); - - if (ret == -1) - nodeStateCleanup(); - return ret; + nodeStateCleanup(); + return -1; } -- 2.9.4
On Mon, Jul 03, 2017 at 05:25:30PM -0400, John Ferlan wrote:
Since virnodedeviceobj now has a self-lockable hash table, there's no need to lock the table from the driver for processing. Thus remove the locks from the driver for NodeDeviceObjList mgmt.
Signed-off-by: John Ferlan <jferlan@redhat.com>
[..]
@@ -601,8 +565,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) return -1; def = virNodeDeviceObjGetDef(obj);
- nodeDeviceLock(); -
Consider the following scenario handling the same device: Thread 1 (udev event handler callback) | Thread 2 (nodeDeviceDestroy) =======================================|============================= | # attempt to destroy a device | obj = nodeDeviceObjFindByName() | | # @obj is locked | def = virNodeDeviceObjGetDef(obj) | | virNodeDeviceObjEndAPI(&obj) | # @obj is unlocked <------ # change event | udevAddOneDevice() | | obj = virNodeDeviceObjListFindByName | # @obj locked | new_device = false | # @obj unlocked | | obj = virNodeDeviceObjListAssignDef | # @obj locked | virNodeDeviceObjEndAPI(&obj) | # @obj unlocked and @def changed | ------> | virNodeDeviceObjListGetParentHost() | if (def->parent) ===> SIGSEGV In patch 12/14 this would have been a deadlock because you first locked the @obj, then nodedev driver while udevAddOneDevice did in the reverse order. I don't know about NPIV so I'm not sure whether there is another way of removing a device and updating the parent device tree, might require an updated model, but for now, you need to make a deep copy of @def. I can see that the chance of getting an 'change' event from udev on a vHBA device is low, but I'm trying to look at it from a higher perspective, as we want to be able to remove mdevs this way, possibly other devices in the future. The rest of the patch looks okay, but I want to try it first. Erik
On 07/11/2017 10:38 AM, Erik Skultety wrote:
On Mon, Jul 03, 2017 at 05:25:30PM -0400, John Ferlan wrote:
Since virnodedeviceobj now has a self-lockable hash table, there's no need to lock the table from the driver for processing. Thus remove the locks from the driver for NodeDeviceObjList mgmt.
Signed-off-by: John Ferlan <jferlan@redhat.com>
[..]
@@ -601,8 +565,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) return -1; def = virNodeDeviceObjGetDef(obj);
- nodeDeviceLock(); -
Consider the following scenario handling the same device:
What if I told you that's impossible? You cannot "have" a scsi_hostN, delete a scsi_hostN, and then have a new one created with the same name. The scsi_hostN's are an ever increasing value of N. Once created and deleted, the N will not be reused.
Thread 1 (udev event handler callback) | Thread 2 (nodeDeviceDestroy) =======================================|============================= | # attempt to destroy a device | obj = nodeDeviceObjFindByName() | | # @obj is locked | def = virNodeDeviceObjGetDef(obj) | | virNodeDeviceObjEndAPI(&obj) | # @obj is unlocked <------ # change event | udevAddOneDevice() | | obj = virNodeDeviceObjListFindByName | # @obj locked | new_device = false | # @obj unlocked | | obj = virNodeDeviceObjListAssignDef | # @obj locked | virNodeDeviceObjEndAPI(&obj) | # @obj unlocked and @def changed | ------> | virNodeDeviceObjListGetParentHost() | if (def->parent) ===> SIGSEGV
In patch 12/14 this would have been a deadlock because you first locked the @obj, then nodedev driver while udevAddOneDevice did in the reverse order. I don't know about NPIV so I'm not sure whether there is another way of removing a device and updating the parent device tree, might require an updated model, but for now, you need to make a deep copy of @def. I can see that the chance of getting an 'change' event from udev on a vHBA device is low, but I'm trying to look at it from a higher perspective, as we want to be able to remove mdevs this way, possibly other devices in the future.
I think what happens is code from virNodeDeviceGetWWNs through virVHBAManageVport gets placed into a function that handles vHBA's on deletion. Similarly for CreateXML, vHBA specific code ends up in a helper function. Those helpers would be called based on the type of object/device we're talking about (vHBA/mdev). BTW: I recall doing a review suggesting perhaps creating an mdev pool driver of sorts. Daniel essentially responded that using the node device driver and augmenting it to fit the needs would be OK. At the time, I wasn't specifically thinking about this case, but it certainly qualifies as something of concern where the existing node device code can make some assumptions about names that the underlying udev code provides. You may need to add some extra layer of protection if names can be reused especially because of this event mgmt "problem"... You may also need to modify that "if (dev)" code to check if the dev is an mdev and if so, do something different. Looks like the code was added by commit id '546fa3ef'. Perhaps as a way to "follow" how other drivers did things. "Fortunately" the only consumer of CreateXML/Destroy ends up being vHBA and we know there's rules around the naming. IIRC - you were letting MDEV also set the name, right? That is, a 'name' on input is essentially and happily ignored. So what creates that name? And can you be assured it's going to be unique for the life/run time of the host? If so, there's no way a CreateXML could reuse a name that AddOneDevice would be using, right? Maybe I need to think some more - it's been a long day already John
The rest of the patch looks okay, but I want to try it first.
Erik
On Thu, Jul 13, 2017 at 07:23:58PM -0400, John Ferlan wrote:
On 07/11/2017 10:38 AM, Erik Skultety wrote:
On Mon, Jul 03, 2017 at 05:25:30PM -0400, John Ferlan wrote:
Since virnodedeviceobj now has a self-lockable hash table, there's no need to lock the table from the driver for processing. Thus remove the locks from the driver for NodeDeviceObjList mgmt.
Signed-off-by: John Ferlan <jferlan@redhat.com>
[..]
@@ -601,8 +565,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) return -1; def = virNodeDeviceObjGetDef(obj);
- nodeDeviceLock(); -
Consider the following scenario handling the same device:
What if I told you that's impossible? You cannot "have" a scsi_hostN, delete a scsi_hostN, and then have a new one created with the same name.
Except I wasn't considering creation, rather than plain change. Although I didn't manage to find out under what circumstances would kernel actually emit the 'change' event (I tried to write to various writable attributes - but since it either lacks documentation completely or it's just well hidden from me - I wasn't able to trigger it), other than explicitly triggering it by writing to sysfs' uevent - so in that aspect, as I wrote below in my previous response, it's highly unlikely but not impossible to hit the race.
The scsi_hostN's are an ever increasing value of N. Once created and deleted, the N will not be reused.
Thread 1 (udev event handler callback) | Thread 2 (nodeDeviceDestroy) =======================================|============================= | # attempt to destroy a device | obj = nodeDeviceObjFindByName() | | # @obj is locked | def = virNodeDeviceObjGetDef(obj) | | virNodeDeviceObjEndAPI(&obj) | # @obj is unlocked <------ # change event | udevAddOneDevice() | | obj = virNodeDeviceObjListFindByName | # @obj locked | new_device = false | # @obj unlocked | | obj = virNodeDeviceObjListAssignDef | # @obj locked | virNodeDeviceObjEndAPI(&obj) | # @obj unlocked and @def changed | ------> | virNodeDeviceObjListGetParentHost() | if (def->parent) ===> SIGSEGV
In patch 12/14 this would have been a deadlock because you first locked the @obj, then nodedev driver while udevAddOneDevice did in the reverse order. I don't know about NPIV so I'm not sure whether there is another way of removing a device and updating the parent device tree, might require an updated model, but for now, you need to make a deep copy of @def. I can see that the chance of getting an 'change' event from udev on a vHBA device is low, but I'm trying to look at it from a higher perspective, as we want to be able to remove mdevs this way, possibly other devices in the future.
I think what happens is code from virNodeDeviceGetWWNs through virVHBAManageVport gets placed into a function that handles vHBA's on deletion. Similarly for CreateXML, vHBA specific code ends up in a helper function. Those helpers would be called based on the type of object/device we're talking about (vHBA/mdev).
Despite the likelihood of the case I'm describing, the main point I'm trying to make is that the lock protects a mutable resource (@def) and by releasing it followed by querying it without actually holding the lock violates thread safety.
BTW: I recall doing a review suggesting perhaps creating an mdev pool driver of sorts. Daniel essentially responded that using the node device driver and augmenting it to fit the needs would be OK. At the time, I wasn't specifically thinking about this case, but it certainly qualifies as something of concern where the existing node device code can make some assumptions about names that the underlying udev code provides.
You may need to add some extra layer of protection if names can be reused especially because of this event mgmt "problem"... You may also need to modify that "if (dev)" code to check if the dev is an mdev and if so, do something different.
Looks like the code was added by commit id '546fa3ef'. Perhaps as a way to "follow" how other drivers did things.
"Fortunately" the only consumer of CreateXML/Destroy ends up being vHBA and we know there's rules around the naming. IIRC - you were letting MDEV also set the name, right? That is, a 'name' on input is essentially
No, mdev's name is a readonly attribute that is optional and exposed by the vendor, the only thing libvirt can currently write to the mdev's sysfs interface is UUID. Erik
and happily ignored. So what creates that name? And can you be assured it's going to be unique for the life/run time of the host? If so, there's no way a CreateXML could reuse a name that AddOneDevice would be using, right?
Maybe I need to think some more - it's been a long day already
John
The rest of the patch looks okay, but I want to try it first.
Erik
On 07/14/2017 05:23 AM, Erik Skultety wrote:
On Thu, Jul 13, 2017 at 07:23:58PM -0400, John Ferlan wrote:
On 07/11/2017 10:38 AM, Erik Skultety wrote:
On Mon, Jul 03, 2017 at 05:25:30PM -0400, John Ferlan wrote:
Since virnodedeviceobj now has a self-lockable hash table, there's no need to lock the table from the driver for processing. Thus remove the locks from the driver for NodeDeviceObjList mgmt.
Signed-off-by: John Ferlan <jferlan@redhat.com>
[..]
@@ -601,8 +565,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) return -1; def = virNodeDeviceObjGetDef(obj);
- nodeDeviceLock(); -
Consider the following scenario handling the same device:
What if I told you that's impossible? You cannot "have" a scsi_hostN, delete a scsi_hostN, and then have a new one created with the same name.
Except I wasn't considering creation, rather than plain change. Although I didn't manage to find out under what circumstances would kernel actually emit the 'change' event (I tried to write to various writable attributes - but since it either lacks documentation completely or it's just well hidden from me - I wasn't able to trigger it), other than explicitly triggering it by writing to sysfs' uevent - so in that aspect, as I wrote below in my previous response, it's highly unlikely but not impossible to hit the race.
Not quite sure what would trigger a change event. A vHBA has a wwnn/wwpn and a parent wwnn/wwpn and WWN. If any of those change, the vHBA would need to be deleted and recreated. I do have a faint recollection of considering the ramifications of dropping the obj lock in that path and the race drawback, but I dismissed it mainly because of "how" vHBA's are created and what could constitute a change event for a vHBA essentially redefines it.
The scsi_hostN's are an ever increasing value of N. Once created and deleted, the N will not be reused.
Thread 1 (udev event handler callback) | Thread 2 (nodeDeviceDestroy) =======================================|============================= | # attempt to destroy a device | obj = nodeDeviceObjFindByName() | | # @obj is locked | def = virNodeDeviceObjGetDef(obj) | | virNodeDeviceObjEndAPI(&obj) | # @obj is unlocked <------ # change event | udevAddOneDevice() | | obj = virNodeDeviceObjListFindByName | # @obj locked | new_device = false | # @obj unlocked | | obj = virNodeDeviceObjListAssignDef | # @obj locked | virNodeDeviceObjEndAPI(&obj) | # @obj unlocked and @def changed | ------> | virNodeDeviceObjListGetParentHost() | if (def->parent) ===> SIGSEGV
In patch 12/14 this would have been a deadlock because you first locked the @obj, then nodedev driver while udevAddOneDevice did in the reverse order. I don't know about NPIV so I'm not sure whether there is another way of removing a device and updating the parent device tree, might require an updated model, but for now, you need to make a deep copy of @def. I can see that the chance of getting an 'change' event from udev on a vHBA device is low, but I'm trying to look at it from a higher perspective, as we want to be able to remove mdevs this way, possibly other devices in the future.
I think what happens is code from virNodeDeviceGetWWNs through virVHBAManageVport gets placed into a function that handles vHBA's on deletion. Similarly for CreateXML, vHBA specific code ends up in a helper function. Those helpers would be called based on the type of object/device we're talking about (vHBA/mdev).
Despite the likelihood of the case I'm describing, the main point I'm trying to make is that the lock protects a mutable resource (@def) and by releasing it followed by querying it without actually holding the lock violates thread safety.
I understand the position and while the likelihood is essentially next to zero that something like that could happen it's also possible to remove @def and pass copies of the name, parent, parent_wwnn, parent_wwpn, and parent_fabric_wwn. So before I do that - can we close on patches 1-12? I can then post a v5 that alters the virNodeDeviceObjListGetParentHost to take parameters instead of @def. That should allay this concern and make patches 13/14 be 2 and 3 of the next series. John
BTW: I recall doing a review suggesting perhaps creating an mdev pool driver of sorts. Daniel essentially responded that using the node device driver and augmenting it to fit the needs would be OK. At the time, I wasn't specifically thinking about this case, but it certainly qualifies as something of concern where the existing node device code can make some assumptions about names that the underlying udev code provides.
You may need to add some extra layer of protection if names can be reused especially because of this event mgmt "problem"... You may also need to modify that "if (dev)" code to check if the dev is an mdev and if so, do something different.
Looks like the code was added by commit id '546fa3ef'. Perhaps as a way to "follow" how other drivers did things.
"Fortunately" the only consumer of CreateXML/Destroy ends up being vHBA and we know there's rules around the naming. IIRC - you were letting MDEV also set the name, right? That is, a 'name' on input is essentially
No, mdev's name is a readonly attribute that is optional and exposed by the vendor, the only thing libvirt can currently write to the mdev's sysfs interface is UUID.
Erik
and happily ignored. So what creates that name? And can you be assured it's going to be unique for the life/run time of the host? If so, there's no way a CreateXML could reuse a name that AddOneDevice would be using, right?
Maybe I need to think some more - it's been a long day already
John
The rest of the patch looks okay, but I want to try it first.
Erik
On 07/14/2017 11:37 AM, John Ferlan wrote:
On 07/14/2017 05:23 AM, Erik Skultety wrote:
On Thu, Jul 13, 2017 at 07:23:58PM -0400, John Ferlan wrote:
On 07/11/2017 10:38 AM, Erik Skultety wrote:
On Mon, Jul 03, 2017 at 05:25:30PM -0400, John Ferlan wrote:
Since virnodedeviceobj now has a self-lockable hash table, there's no need to lock the table from the driver for processing. Thus remove the locks from the driver for NodeDeviceObjList mgmt.
Signed-off-by: John Ferlan <jferlan@redhat.com>
[..]
@@ -601,8 +565,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) return -1; def = virNodeDeviceObjGetDef(obj);
- nodeDeviceLock(); -
Consider the following scenario handling the same device:
What if I told you that's impossible? You cannot "have" a scsi_hostN, delete a scsi_hostN, and then have a new one created with the same name.
Except I wasn't considering creation, rather than plain change. Although I didn't manage to find out under what circumstances would kernel actually emit the 'change' event (I tried to write to various writable attributes - but since it either lacks documentation completely or it's just well hidden from me - I wasn't able to trigger it), other than explicitly triggering it by writing to sysfs' uevent - so in that aspect, as I wrote below in my previous response, it's highly unlikely but not impossible to hit the race.
Not quite sure what would trigger a change event. A vHBA has a wwnn/wwpn and a parent wwnn/wwpn and WWN. If any of those change, the vHBA would need to be deleted and recreated.
I do have a faint recollection of considering the ramifications of dropping the obj lock in that path and the race drawback, but I dismissed it mainly because of "how" vHBA's are created and what could constitute a change event for a vHBA essentially redefines it.
The scsi_hostN's are an ever increasing value of N. Once created and deleted, the N will not be reused.
Thread 1 (udev event handler callback) | Thread 2 (nodeDeviceDestroy) =======================================|============================= | # attempt to destroy a device | obj = nodeDeviceObjFindByName() | | # @obj is locked | def = virNodeDeviceObjGetDef(obj) | | virNodeDeviceObjEndAPI(&obj) | # @obj is unlocked <------ # change event | udevAddOneDevice() | | obj = virNodeDeviceObjListFindByName | # @obj locked | new_device = false | # @obj unlocked | | obj = virNodeDeviceObjListAssignDef | # @obj locked | virNodeDeviceObjEndAPI(&obj) | # @obj unlocked and @def changed | ------> | virNodeDeviceObjListGetParentHost() | if (def->parent) ===> SIGSEGV
In patch 12/14 this would have been a deadlock because you first locked the @obj, then nodedev driver while udevAddOneDevice did in the reverse order. I don't know about NPIV so I'm not sure whether there is another way of removing a device and updating the parent device tree, might require an updated model, but for now, you need to make a deep copy of @def. I can see that the chance of getting an 'change' event from udev on a vHBA device is low, but I'm trying to look at it from a higher perspective, as we want to be able to remove mdevs this way, possibly other devices in the future.
I think what happens is code from virNodeDeviceGetWWNs through virVHBAManageVport gets placed into a function that handles vHBA's on deletion. Similarly for CreateXML, vHBA specific code ends up in a helper function. Those helpers would be called based on the type of object/device we're talking about (vHBA/mdev).
Despite the likelihood of the case I'm describing, the main point I'm trying to make is that the lock protects a mutable resource (@def) and by releasing it followed by querying it without actually holding the lock violates thread safety.
I understand the position and while the likelihood is essentially next to zero that something like that could happen it's also possible to remove @def and pass copies of the name, parent, parent_wwnn, parent_wwpn, and parent_fabric_wwn.
So before I do that - can we close on patches 1-12?
Let me amend this statement to include - modulo passing @def to virNodeDeviceObjNew in patch 5 as Michal has requested in his review of Secrets and NWFilter that @def is not passed to the vir*ObjNew. I will also post a reversal for Interfaces to keep everything the same... John
I can then post a v5 that alters the virNodeDeviceObjListGetParentHost to take parameters instead of @def. That should allay this concern and make patches 13/14 be 2 and 3 of the next series.
John
BTW: I recall doing a review suggesting perhaps creating an mdev pool driver of sorts. Daniel essentially responded that using the node device driver and augmenting it to fit the needs would be OK. At the time, I wasn't specifically thinking about this case, but it certainly qualifies as something of concern where the existing node device code can make some assumptions about names that the underlying udev code provides.
You may need to add some extra layer of protection if names can be reused especially because of this event mgmt "problem"... You may also need to modify that "if (dev)" code to check if the dev is an mdev and if so, do something different.
Looks like the code was added by commit id '546fa3ef'. Perhaps as a way to "follow" how other drivers did things.
"Fortunately" the only consumer of CreateXML/Destroy ends up being vHBA and we know there's rules around the naming. IIRC - you were letting MDEV also set the name, right? That is, a 'name' on input is essentially
No, mdev's name is a readonly attribute that is optional and exposed by the vendor, the only thing libvirt can currently write to the mdev's sysfs interface is UUID.
Erik
and happily ignored. So what creates that name? And can you be assured it's going to be unique for the life/run time of the host? If so, there's no way a CreateXML could reuse a name that AddOneDevice would be using, right?
Maybe I need to think some more - it's been a long day already
John
The rest of the patch looks okay, but I want to try it first.
Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jul 14, 2017 at 11:37:36AM -0400, John Ferlan wrote:
On 07/14/2017 05:23 AM, Erik Skultety wrote:
On Thu, Jul 13, 2017 at 07:23:58PM -0400, John Ferlan wrote:
On 07/11/2017 10:38 AM, Erik Skultety wrote:
On Mon, Jul 03, 2017 at 05:25:30PM -0400, John Ferlan wrote:
Since virnodedeviceobj now has a self-lockable hash table, there's no need to lock the table from the driver for processing. Thus remove the locks from the driver for NodeDeviceObjList mgmt.
Signed-off-by: John Ferlan <jferlan@redhat.com>
[..]
@@ -601,8 +565,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) return -1; def = virNodeDeviceObjGetDef(obj);
- nodeDeviceLock(); -
Consider the following scenario handling the same device:
What if I told you that's impossible? You cannot "have" a scsi_hostN, delete a scsi_hostN, and then have a new one created with the same name.
Except I wasn't considering creation, rather than plain change. Although I didn't manage to find out under what circumstances would kernel actually emit the 'change' event (I tried to write to various writable attributes - but since it either lacks documentation completely or it's just well hidden from me - I wasn't able to trigger it), other than explicitly triggering it by writing to sysfs' uevent - so in that aspect, as I wrote below in my previous response, it's highly unlikely but not impossible to hit the race.
Not quite sure what would trigger a change event. A vHBA has a wwnn/wwpn and a parent wwnn/wwpn and WWN. If any of those change, the vHBA would need to be deleted and recreated.
I do have a faint recollection of considering the ramifications of dropping the obj lock in that path and the race drawback, but I dismissed it mainly because of "how" vHBA's are created and what could constitute a change event for a vHBA essentially redefines it.
The scsi_hostN's are an ever increasing value of N. Once created and deleted, the N will not be reused.
Thread 1 (udev event handler callback) | Thread 2 (nodeDeviceDestroy) =======================================|============================= | # attempt to destroy a device | obj = nodeDeviceObjFindByName() | | # @obj is locked | def = virNodeDeviceObjGetDef(obj) | | virNodeDeviceObjEndAPI(&obj) | # @obj is unlocked <------ # change event | udevAddOneDevice() | | obj = virNodeDeviceObjListFindByName | # @obj locked | new_device = false | # @obj unlocked | | obj = virNodeDeviceObjListAssignDef | # @obj locked | virNodeDeviceObjEndAPI(&obj) | # @obj unlocked and @def changed | ------> | virNodeDeviceObjListGetParentHost() | if (def->parent) ===> SIGSEGV
In patch 12/14 this would have been a deadlock because you first locked the @obj, then nodedev driver while udevAddOneDevice did in the reverse order. I don't know about NPIV so I'm not sure whether there is another way of removing a device and updating the parent device tree, might require an updated model, but for now, you need to make a deep copy of @def. I can see that the chance of getting an 'change' event from udev on a vHBA device is low, but I'm trying to look at it from a higher perspective, as we want to be able to remove mdevs this way, possibly other devices in the future.
I think what happens is code from virNodeDeviceGetWWNs through virVHBAManageVport gets placed into a function that handles vHBA's on deletion. Similarly for CreateXML, vHBA specific code ends up in a helper function. Those helpers would be called based on the type of object/device we're talking about (vHBA/mdev).
Despite the likelihood of the case I'm describing, the main point I'm trying to make is that the lock protects a mutable resource (@def) and by releasing it followed by querying it without actually holding the lock violates thread safety.
I understand the position and while the likelihood is essentially next to zero that something like that could happen it's also possible to remove @def and pass copies of the name, parent, parent_wwnn, parent_wwpn, and parent_fabric_wwn.
So before I do that - can we close on patches 1-12?
Yeah, I wasn't explicit with the ACK for 1-12 because I first wanted to have a small discussion about the changes, but we can now focus solely on the rest. So, ACK to 1-12 with the proposed change to virNode*ObjNew not consuming @def (I don't have a strong preference here that I could back with some reasonable pros/cons, to me, having it behave like a constructor for OOP, which we're essentially trying to copy in libvirt, naturally makes sense, but I also see Michal's point). Erik
/* Populate with known devices */
This commentary should stay with the function it describes (below), so the context doesn't get lost. Erik
+ nodeDeviceUnlock();
if (udevEnumerateDevices(udev) != 0) goto cleanup;
- ret = 0; + return 0;
cleanup: nodeDeviceUnlock(); - - if (ret == -1) - nodeStateCleanup(); - return ret; + nodeStateCleanup(); + return -1; }
-- 2.9.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Erik Skultety -
John Ferlan