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(a)redhat.com&gt; --- 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

A virNodeDeviceObjPtr is an @obj A virNodeDeviceObjListPtr is an @devs Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

Move the structures to withing virnodedeviceobj.c Move the typedefs from node_device_conf to virnodedeviceobj.h Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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

On Mon, Jul 03, 2017 at 05:25:28PM -0400, John Ferlan wrote: s/cleaup/cleanup [...] ^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. [...] 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

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 Fri, Jul 14, 2017 at 10:12:12AM -0400, John Ferlan wrote: Disregard my comment then and proceed with your proposal. Erik

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(a)redhat.com&gt; --- 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 07/11/2017 10:38 AM, Erik Skultety wrote: 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. 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

On 07/14/2017 05:23 AM, Erik Skultety wrote: 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. 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

On Fri, Jul 14, 2017 at 11:37:36AM -0400, John Ferlan wrote: 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