[libvirt] [PATCH 00/13] Make the virNodeDeviceObjPtr private
All part of the effort I have to have a common object model. This series is node device test, driver, and virnodedevobj related. There's also a couple of bug fixes at the beginning of the series from things I have found during this effort. There's still a few more patches in local branches to make the virNodeDeviceObjListPtr private as well, but those have some merge needs with other patches currently on list elsewhere, so I'll hold onto them for now. John Ferlan (13): test: Adjust cleanup/error paths for nodedev test APIs nodedev: Fix locking in virNodeDeviceObjRemove nodedev: Need to check for vport capable scsi_host for vHBA searches nodedev: Move node_device_linux_sysfs from node_driver to conf nodedev: Use common naming for virnodedeviceobj nodedev: Cleanup driver code and prototypes nodedev: Alter param to nodeDeviceUpdateDriverName nodedev: Alter param to nodeDeviceUpdateCaps nodedev: Create helper for finding by name in driver nodedev: Use consistent names for driver variables nodedev: Introduce virNodeDeviceObjGetDef nodedev: Remove privateData from virNodeDeviceObj nodedev: Introduce virNodeDeviceObjNew src/Makefile.am | 8 +- src/conf/node_device_conf.h | 2 - .../node_device_linux_sysfs.c | 24 +- .../node_device_linux_sysfs.h | 9 +- src/conf/virnodedeviceobj.c | 170 ++++++----- src/conf/virnodedeviceobj.h | 2 + src/libvirt_private.syms | 6 + src/node_device/node_device_driver.c | 235 +++++++-------- src/node_device/node_device_driver.h | 93 ++++-- src/node_device/node_device_hal.c | 60 ++-- src/node_device/node_device_udev.c | 323 ++++++++++++--------- src/test/test_driver.c | 113 ++++--- 12 files changed, 576 insertions(+), 469 deletions(-) rename src/{node_device => conf}/node_device_linux_sysfs.c (89%) rename src/{node_device => conf}/node_device_linux_sysfs.h (82%) -- 2.9.3
- 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 | 75 +++++++++++++++++++------------------------------- 1 file changed, 29 insertions(+), 46 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2db3f7d..3389edd 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4512,7 +4512,6 @@ testDestroyVport(testDriverPtr privconn, const char *wwnn ATTRIBUTE_UNUSED, const char *wwpn ATTRIBUTE_UNUSED) { - int ret = -1; virNodeDeviceObjPtr obj = NULL; virObjectEventPtr event = NULL; @@ -4526,7 +4525,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", @@ -4535,13 +4534,8 @@ testDestroyVport(testDriverPtr privconn, virNodeDeviceObjRemove(&privconn->devs, &obj); - ret = 0; - - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); testObjectEventQueue(privconn, event); - return ret; + return 0; } @@ -5328,16 +5322,14 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name) virNodeDevicePtr ret = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, name))) - goto cleanup; + return NULL; if ((ret = virGetNodeDevice(conn, name))) { if (VIR_STRDUP(ret->parent, obj->def->parent) < 0) virObjectUnref(ret); } - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5352,13 +5344,11 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev, virCheckFlags(0, NULL); if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return NULL; ret = virNodeDeviceDefFormat(obj->def); - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5370,7 +5360,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) char *ret = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return NULL; if (obj->def->parent) { ignore_value(VIR_STRDUP(ret, obj->def->parent)); @@ -5379,9 +5369,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) "%s", _("no parent for this device")); } - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5393,19 +5381,15 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev) virNodeDeviceObjPtr obj; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; for (caps = obj->def->caps; caps; caps = caps->next) ++ncaps; - ret = ncaps; - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); - return ret; + virNodeDeviceObjUnlock(obj); + return ncaps; } @@ -5416,26 +5400,25 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) virNodeDeviceObjPtr obj; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; for (caps = obj->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; } @@ -5584,13 +5567,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virObjectEventPtr event = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto out; + return -1; if (virNodeDeviceGetWWNs(obj->def, &wwnn, &wwpn) == -1) - goto out; + goto cleanup; if (VIR_STRDUP(parent_name, obj->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 @@ -5603,7 +5586,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) if (virNodeDeviceObjGetParentHost(&driver->devs, obj->def, EXISTING_DEVICE) < 0) { obj = NULL; - goto out; + goto cleanup; } event = virNodeDeviceEventLifecycleNew(dev->name, @@ -5613,7 +5596,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virNodeDeviceObjLock(obj); virNodeDeviceObjRemove(&driver->devs, &obj); - out: + cleanup: if (obj) virNodeDeviceObjUnlock(obj); testObjectEventQueue(driver, event); -- 2.9.3
The current mechanism doesn't lock each element in devs->objs as it's looking at it, rather it keeps locking/unlocking the passed element for which the removal is being attempted. Fix things to lock each element as we're looking at them. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 02ac544..1352720 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -281,16 +281,16 @@ virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjUnlock(*dev); for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(*dev); + virNodeDeviceObjLock(devs->objs[i]); if (devs->objs[i] == *dev) { - virNodeDeviceObjUnlock(*dev); + virNodeDeviceObjUnlock(devs->objs[i]); virNodeDeviceObjFree(devs->objs[i]); *dev = NULL; VIR_DELETE_ELEMENT(devs->objs, i, devs->count); break; } - virNodeDeviceObjUnlock(*dev); + virNodeDeviceObjUnlock(devs->objs[i]); } } -- 2.9.3
When searching for an NPIV capable fc_host, not only does there need to be an "fc_host" capability with the specified wwnn/wwpn or fabric_wwn, but that scsi_host must be vport capable; otherwise, one could end up picking an exising vHBA/NPIV which wouldn't be good. Currently not a problem since scsi_hosts are in an as found forward linked list and the vport capable scsi_hosts will always appear before a vHBA by definition. However, in the near term future a hash table will be used to lookup the devices and that could cause problems for these algorithms. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 1352720..bbb6eeb 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -165,7 +165,8 @@ virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs, virNodeDeviceObjLock(devs->objs[i]); if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) && STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) && - STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn)) + STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) && + virNodeDeviceFindVPORTCapDef(devs->objs[i])) return devs->objs[i]; virNodeDeviceObjUnlock(devs->objs[i]); } @@ -184,7 +185,8 @@ virNodeDeviceFindByFabricWWN(virNodeDeviceObjListPtr devs, virNodeDevCapsDefPtr cap; virNodeDeviceObjLock(devs->objs[i]); if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) && - STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn)) + STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) && + virNodeDeviceFindVPORTCapDef(devs->objs[i])) return devs->objs[i]; virNodeDeviceObjUnlock(devs->objs[i]); } -- 2.9.3
Move the whole file from src/node_device into src/conf and rename the API's to have the "virNodeDevice" prefix. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/Makefile.am | 8 ++++---- .../node_device_linux_sysfs.c | 24 ++++++++++------------ .../node_device_linux_sysfs.h | 9 +++++--- src/libvirt_private.syms | 5 +++++ src/node_device/node_device_driver.c | 8 ++++---- src/node_device/node_device_hal.c | 4 ++-- src/node_device/node_device_udev.c | 4 ++-- 7 files changed, 34 insertions(+), 28 deletions(-) rename src/{node_device => conf}/node_device_linux_sysfs.c (89%) rename src/{node_device => conf}/node_device_linux_sysfs.h (82%) diff --git a/src/Makefile.am b/src/Makefile.am index f95f30e..23c35eb 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -462,7 +462,9 @@ SECRET_CONF_SOURCES = \ # Network driver generic impl APIs NODE_DEVICE_CONF_SOURCES = \ conf/node_device_conf.c conf/node_device_conf.h \ - conf/virnodedeviceobj.c conf/virnodedeviceobj.h + conf/virnodedeviceobj.c conf/virnodedeviceobj.h \ + conf/node_device_linux_sysfs.c \ + conf/node_device_linux_sysfs.h CPU_CONF_SOURCES = \ conf/cpu_conf.c conf/cpu_conf.h @@ -1162,9 +1164,7 @@ ACCESS_DRIVER_POLKIT_POLICY = \ NODE_DEVICE_DRIVER_SOURCES = \ node_device/node_device_driver.c \ - node_device/node_device_driver.h \ - node_device/node_device_linux_sysfs.c \ - node_device/node_device_linux_sysfs.h + node_device/node_device_driver.h NODE_DEVICE_DRIVER_HAL_SOURCES = \ node_device/node_device_hal.c \ diff --git a/src/node_device/node_device_linux_sysfs.c b/src/conf/node_device_linux_sysfs.c similarity index 89% rename from src/node_device/node_device_linux_sysfs.c rename to src/conf/node_device_linux_sysfs.c index 1b7aa94..a905cd9 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/conf/node_device_linux_sysfs.c @@ -26,8 +26,6 @@ #include <sys/stat.h> #include <stdlib.h> -#include "node_device_driver.h" -#include "node_device_hal.h" #include "node_device_linux_sysfs.h" #include "virerror.h" #include "viralloc.h" @@ -44,7 +42,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs"); int -nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host) +virNodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host) { char *tmp = NULL; int ret = -1; @@ -125,8 +123,8 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host) static int -nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath, - virNodeDevCapPCIDevPtr pci_dev) +virNodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath, + virNodeDevCapPCIDevPtr pci_dev) { size_t i; int ret; @@ -164,7 +162,7 @@ nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath, static int -nodeDeviceSysfsGetPCIIOMMUGroupCaps(virNodeDevCapPCIDevPtr pci_dev) +virNodeDeviceSysfsGetPCIIOMMUGroupCaps(virNodeDevCapPCIDevPtr pci_dev) { size_t i; int tmpGroup, ret = -1; @@ -204,19 +202,19 @@ nodeDeviceSysfsGetPCIIOMMUGroupCaps(virNodeDevCapPCIDevPtr pci_dev) } -/* nodeDeviceSysfsGetPCIRelatedCaps() get info that is stored in sysfs +/* virNodeDeviceSysfsGetPCIRelatedCaps() get info that is stored in sysfs * about devices related to this device, i.e. things that can change * without this device itself changing. These must be refreshed * anytime full XML of the device is requested, because they can * change with no corresponding notification from the kernel/udev. */ int -nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath, - virNodeDevCapPCIDevPtr pci_dev) +virNodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath, + virNodeDevCapPCIDevPtr pci_dev) { - if (nodeDeviceSysfsGetPCISRIOVCaps(sysfsPath, pci_dev) < 0) + if (virNodeDeviceSysfsGetPCISRIOVCaps(sysfsPath, pci_dev) < 0) return -1; - if (nodeDeviceSysfsGetPCIIOMMUGroupCaps(pci_dev) < 0) + if (virNodeDeviceSysfsGetPCIIOMMUGroupCaps(pci_dev) < 0) return -1; return 0; } @@ -225,13 +223,13 @@ nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath, #else int -nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host ATTRIBUTE_UNUSED) +virNodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host ATTRIBUTE_UNUSED) { return -1; } int -nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath ATTRIBUTE_UNUSED, +virNodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath ATTRIBUTE_UNUSED, virNodeDevCapPCIDevPtr pci_dev ATTRIBUTE_UNUSED) { return -1; diff --git a/src/node_device/node_device_linux_sysfs.h b/src/conf/node_device_linux_sysfs.h similarity index 82% rename from src/node_device/node_device_linux_sysfs.h rename to src/conf/node_device_linux_sysfs.h index 8deea66..4e0bf65 100644 --- a/src/node_device/node_device_linux_sysfs.h +++ b/src/conf/node_device_linux_sysfs.h @@ -25,8 +25,11 @@ # include "node_device_conf.h" -int nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host); -int nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath, - virNodeDevCapPCIDevPtr pci_dev); +int +virNodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host); + +int +virNodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath, + virNodeDevCapPCIDevPtr pci_dev); #endif /* __VIR_NODE_DEVICE_LINUX_SYSFS_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d361454..d8602e6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -689,6 +689,11 @@ virNodeDeviceEventStateRegisterID; virNodeDeviceEventUpdateNew; +# conf/node_device_linux_sysfs.h +virNodeDeviceSysfsGetPCIRelatedDevCaps; +virNodeDeviceSysfsGetSCSIHostCaps; + + # conf/numa_conf.h virDomainNumaCheckABIStability; virDomainNumaEquals; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index ba3da62..b4c0020 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -54,7 +54,7 @@ static int update_caps(virNodeDeviceObjPtr dev) while (cap) { switch (cap->data.type) { case VIR_NODE_DEV_CAP_SCSI_HOST: - nodeDeviceSysfsGetSCSIHostCaps(&dev->def->caps->data.scsi_host); + virNodeDeviceSysfsGetSCSIHostCaps(&dev->def->caps->data.scsi_host); break; case VIR_NODE_DEV_CAP_NET: if (virNetDevGetLinkInfo(cap->data.net.ifname, &cap->data.net.lnk) < 0) @@ -64,8 +64,8 @@ static int update_caps(virNodeDeviceObjPtr dev) return -1; break; case VIR_NODE_DEV_CAP_PCI_DEV: - if (nodeDeviceSysfsGetPCIRelatedDevCaps(dev->def->sysfs_path, - &dev->def->caps->data.pci_dev) < 0) + if (virNodeDeviceSysfsGetPCIRelatedDevCaps(dev->def->sysfs_path, + &dev->def->caps->data.pci_dev) < 0) return -1; break; @@ -275,7 +275,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, while (cap) { if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - nodeDeviceSysfsGetSCSIHostCaps(&cap->data.scsi_host); + virNodeDeviceSysfsGetSCSIHostCaps(&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) && diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 81e5ecc..e76201b 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -153,7 +153,7 @@ gather_pci_cap(LibHalContext *ctx, const char *udi, ignore_value(virStrToLong_ui(p+1, &p, 16, &d->pci_dev.function)); } - if (nodeDeviceSysfsGetPCIRelatedDevCaps(sysfs_path, &d->pci_dev) < 0) { + if (virNodeDeviceSysfsGetPCIRelatedDevCaps(sysfs_path, &d->pci_dev) < 0) { VIR_FREE(sysfs_path); return -1; } @@ -239,7 +239,7 @@ gather_scsi_host_cap(LibHalContext *ctx, const char *udi, (void)get_int_prop(ctx, udi, "scsi_host.host", (int *)&d->scsi_host.host); - retval = nodeDeviceSysfsGetSCSIHostCaps(&d->scsi_host); + retval = virNodeDeviceSysfsGetSCSIHostCaps(&d->scsi_host); if (retval == -1) goto out; diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4ecb0b1..c88a234 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -474,7 +474,7 @@ static int udevProcessPCI(struct udev_device *device, &pci_dev->numa_node, 10) < 0) goto cleanup; - if (nodeDeviceSysfsGetPCIRelatedDevCaps(def->sysfs_path, pci_dev) < 0) + if (virNodeDeviceSysfsGetPCIRelatedDevCaps(def->sysfs_path, pci_dev) < 0) goto cleanup; if (!(pciDev = virPCIDeviceNew(pci_dev->domain, @@ -688,7 +688,7 @@ static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, return -1; } - nodeDeviceSysfsGetSCSIHostCaps(&def->caps->data.scsi_host); + virNodeDeviceSysfsGetSCSIHostCaps(&def->caps->data.scsi_host); if (udevGenerateDeviceName(device, def, NULL) != 0) return -1; -- 2.9.3
John Ferlan <jferlan@redhat.com> [2017-05-19, 09:08AM -0400]:
Move the whole file from src/node_device into src/conf and rename the API's to have the "virNodeDevice" prefix.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/Makefile.am | 8 ++++---- .../node_device_linux_sysfs.c | 24 ++++++++++------------ .../node_device_linux_sysfs.h | 9 +++++--- src/libvirt_private.syms | 5 +++++ src/node_device/node_device_driver.c | 8 ++++---- src/node_device/node_device_hal.c | 4 ++-- src/node_device/node_device_udev.c | 4 ++-- 7 files changed, 34 insertions(+), 28 deletions(-) rename src/{node_device => conf}/node_device_linux_sysfs.c (89%) rename src/{node_device => conf}/node_device_linux_sysfs.h (82%)
What's the reasoning for this move? It somehow doesn't feel right and it will make backports harder.
On 05/23/2017 04:13 AM, Bjoern Walk wrote:
John Ferlan <jferlan@redhat.com> [2017-05-19, 09:08AM -0400]:
Move the whole file from src/node_device into src/conf and rename the API's to have the "virNodeDevice" prefix.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/Makefile.am | 8 ++++---- .../node_device_linux_sysfs.c | 24 ++++++++++------------ .../node_device_linux_sysfs.h | 9 +++++--- src/libvirt_private.syms | 5 +++++ src/node_device/node_device_driver.c | 8 ++++---- src/node_device/node_device_hal.c | 4 ++-- src/node_device/node_device_udev.c | 4 ++-- 7 files changed, 34 insertions(+), 28 deletions(-) rename src/{node_device => conf}/node_device_linux_sysfs.c (89%) rename src/{node_device => conf}/node_device_linux_sysfs.h (82%)
What's the reasoning for this move? It somehow doesn't feel right and it will make backports harder.
"Eventually" code that is currently in node_device_driver to peruse the node device object list is going to move to virnodedevobj.c and there was some "boundary" thing about calling back into the driver that I don't quite recall the exact compile/link error which caused me to move the code. Although the commit date is relatively recent - it's more or less a copy of work done in Dec 2016. One could also make the argument that there's nothing "driver specific" in the code, so why should it have ever been put in the src/node_device directory? My other option would be to essentially move the guts of the code into virnodedevobj and call it from src/node_device/... IDC which way this happens, but I just found this easier. Using gitk I've never had issues in the past following history of the code when a module moves from one directory to another. FWIW: My first instinct was move the code to src/util like other code that has linux (or arch specific) pieces, but I couldn't do that because node_device_linux_sysfs.h references "node_device_conf.h". Inclusion of conf/*.h has been disallowed from src/util. So to answer the other point... I do not believe that making backports harder should be a reason to limit changes, but I also understand that's a touchy subject to many. Still when I first started working on libvirt there was quite a bit of code refactoring and movement - it was at times difficult to follow, but it subsided fairly quickly and I learned how to use gitk to follow changes. As an aside, for any of this common object model work if I have come across something that I consider a "bug" I've tried to "front load" that to the beginning of the patch series so that it is easier to cherry-pick to some backport. In the long run any of these changes is only painful when it affects the code you care most about. I would submit that that nodedev code has long been "overlooked" for optimizations and change to bring it more in line with how libvirt is currently structured. I've even wondered if HAL is even relevant any more, but I don't want to go down that path! Couple of other "datapoints" to consider. 1. The impetus for all the changes I've posted in various drivers and conf/vir*obj.c module is to use a common object model to handle the functions that every driver has essentially cut-n-paste'd (create, add, remove, FindBySomeKey, NumOf, return ListOfSomeKey, and return ListOfExternalReference). Over time each has varied slightly or fixed bugs. Late last year there was a new driver proposed from Virtuozzo which essentially copied everything from the storage pool into it's own model. That got me to looking at the code and thinking there's no way the model should be to essentially copy everything and just change the names. In the long run all the drivers have their own XML/data definition, but there's no way they shouldn't be able to share object handling. It's taking a while to get there, but I think when it's done it'll be easier to maintain. 2. The current object handling model is scattered. Some of the drivers are more active (domain, network, storage) and each could have some fix applied to one that isn't applied to another that is probably a bug in the other. If you have common code handling the objects for each driver, then fixing one fixes all. Reading the code and trying to compare how some other driver did things to fit into another driver to fix a similar issue seen can be painful. Been there while working through making the secret driver use hash tables (my model varied slightly between domain and network depending on which I thought did something better). 3. Most of the drivers use forward linked lists to manage their objects (nodedev, interface, nwfilter, storagepool, storagevol), while others use hash tables (domain, domainsnapshot, network, secret). Those forward linked list drivers roll their own code w/r/t object mutexes which have long been provided by virObjectLockable. These should have been converted ages ago, but no one's taken the time to do so. 3a. In particular, nodedev uses a forward linked list. Ironically, for nodedev "performance" this usually means the bulk of object lookups have to traverse potentially long lists to find what's being searched most frequently. Compare that to a hash table lookup. On my work laptop there are 140 nodedev objs. Of those I usually want to reference the ones added last - e.g iSCSI luns for "pseudo" block devices. Those block_*, scsi_host*, and scsi_generic* are always going to be at the end just by the nature of the udev discovery algorithm. I would assume this is much more so true for those "larger" environments as well, but I don't have empirical evidence. If you consider that a volume for a storage pool could use some nodedev list in order to essentially "find" the storage it wants. In order to find the pool, follow a linked list of pools to find your specific pool. Then follow a linked list to find the specific volume within the pool. Then for some volumes this also means following the nodedev list in order to find system representation for it. If you start with a pool w/ 0 volumes, then as you add 10, 20, 100, 200, 1000, etc. volumes I would hope you could see the scaling problem that a forward linked list for each of these drivers would present. Now change all those to use a hash table lookup and consider your performance. I don't disagree this is painful, but it could also be short lived pain depending on reviews. For a reference to the object and even more history see: https://www.redhat.com/archives/libvir-list/2017-April/msg00321.html In my branches the order and quantity of those patches has changed. I plan to post an update shortly, but wanted to get through the bulk of the changes I originally posted in February before reposting (plus I needed to add a few more comments). John
On Wed, May 24, 2017 at 09:45:09AM -0400, John Ferlan wrote:
On 05/23/2017 04:13 AM, Bjoern Walk wrote:
John Ferlan <jferlan@redhat.com> [2017-05-19, 09:08AM -0400]:
Move the whole file from src/node_device into src/conf and rename the API's to have the "virNodeDevice" prefix.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/Makefile.am | 8 ++++---- .../node_device_linux_sysfs.c | 24 ++++++++++------------ .../node_device_linux_sysfs.h | 9 +++++--- src/libvirt_private.syms | 5 +++++ src/node_device/node_device_driver.c | 8 ++++---- src/node_device/node_device_hal.c | 4 ++-- src/node_device/node_device_udev.c | 4 ++-- 7 files changed, 34 insertions(+), 28 deletions(-) rename src/{node_device => conf}/node_device_linux_sysfs.c (89%) rename src/{node_device => conf}/node_device_linux_sysfs.h (82%)
What's the reasoning for this move? It somehow doesn't feel right and it will make backports harder.
"Eventually" code that is currently in node_device_driver to peruse the node device object list is going to move to virnodedevobj.c and there was some "boundary" thing about calling back into the driver that I don't quite recall the exact compile/link error which caused me to move the code. Although the commit date is relatively recent - it's more or less a copy of work done in Dec 2016.
This still doesn't make sense really. The src/conf directory should only contain code that's related to the XML parsing/formatting, and generic object management. It should never contain any functional driver code, and certainly none that pokes in sysfs.
One could also make the argument that there's nothing "driver specific" in the code, so why should it have ever been put in the src/node_device directory? My other option would be to essentially move the guts of the code into virnodedevobj and call it from src/node_device/... IDC which way this happens, but I just found this easier. Using gitk I've never had issues in the past following history of the code when a module moves from one directory to another.
FWIW: My first instinct was move the code to src/util like other code that has linux (or arch specific) pieces, but I couldn't do that because node_device_linux_sysfs.h references "node_device_conf.h". Inclusion of conf/*.h has been disallowed from src/util.
To get around that simply change the APIs so that instead of taking the virNodeDevXXX structs as a single parameter, you just pass in an exploded list of parameters, populated from the individual struct fields that are needed for the methods in question. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 05/24/2017 10:35 AM, Daniel P. Berrange wrote:
On Wed, May 24, 2017 at 09:45:09AM -0400, John Ferlan wrote:
On 05/23/2017 04:13 AM, Bjoern Walk wrote:
John Ferlan <jferlan@redhat.com> [2017-05-19, 09:08AM -0400]:
Move the whole file from src/node_device into src/conf and rename the API's to have the "virNodeDevice" prefix.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/Makefile.am | 8 ++++---- .../node_device_linux_sysfs.c | 24 ++++++++++------------ .../node_device_linux_sysfs.h | 9 +++++--- src/libvirt_private.syms | 5 +++++ src/node_device/node_device_driver.c | 8 ++++---- src/node_device/node_device_hal.c | 4 ++-- src/node_device/node_device_udev.c | 4 ++-- 7 files changed, 34 insertions(+), 28 deletions(-) rename src/{node_device => conf}/node_device_linux_sysfs.c (89%) rename src/{node_device => conf}/node_device_linux_sysfs.h (82%)
What's the reasoning for this move? It somehow doesn't feel right and it will make backports harder.
"Eventually" code that is currently in node_device_driver to peruse the node device object list is going to move to virnodedevobj.c and there was some "boundary" thing about calling back into the driver that I don't quite recall the exact compile/link error which caused me to move the code. Although the commit date is relatively recent - it's more or less a copy of work done in Dec 2016.
This still doesn't make sense really. The src/conf directory should only contain code that's related to the XML parsing/formatting, and generic object management. It should never contain any functional driver code, and certainly none that pokes in sysfs.
Other than the module function names having Sysfs in their names, there's no sysfs poking in the code. There are calls to src/util functions which do the sysfs poking. So I could change the function names or just move the guts of the code into node_device_conf.c and then call those APIs from the unmoved node_device_linux_sysfs.c. It was just simpler to take this approach. I can drop this and rework things. It was created "backwards" from a future need...
One could also make the argument that there's nothing "driver specific" in the code, so why should it have ever been put in the src/node_device directory? My other option would be to essentially move the guts of the code into virnodedevobj and call it from src/node_device/... IDC which way this happens, but I just found this easier. Using gitk I've never had issues in the past following history of the code when a module moves from one directory to another.
FWIW: My first instinct was move the code to src/util like other code that has linux (or arch specific) pieces, but I couldn't do that because node_device_linux_sysfs.h references "node_device_conf.h". Inclusion of conf/*.h has been disallowed from src/util.
To get around that simply change the APIs so that instead of taking the virNodeDevXXX structs as a single parameter, you just pass in an exploded list of parameters, populated from the individual struct fields that are needed for the methods in question.
IMO that's worse especially since _virNodeDevCapSCSIHost and _virNodeDevCapPCIDev fields are getting filled in. John
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 bbb6eeb..3a833e1 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -34,10 +34,10 @@ VIR_LOG_INIT("conf.virnodedeviceobj"); 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 = @@ -78,9 +78,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 && @@ -102,9 +102,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 && @@ -213,18 +213,18 @@ virNodeDeviceFindByCap(virNodeDeviceObjListPtr devs, void -virNodeDeviceObjFree(virNodeDeviceObjPtr dev) +virNodeDeviceObjFree(virNodeDeviceObjPtr obj) { - if (!dev) + if (!obj) return; - virNodeDeviceDefFree(dev->def); - if (dev->privateFree) - (*dev->privateFree)(dev->privateData); + virNodeDeviceDefFree(obj->def); + if (obj->privateFree) + (*obj->privateFree)(obj->privateData); - virMutexDestroy(&dev->lock); + virMutexDestroy(&obj->lock); - VIR_FREE(dev); + VIR_FREE(obj); } @@ -243,51 +243,51 @@ 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]); virNodeDeviceObjFree(devs->objs[i]); - *dev = NULL; + *obj = NULL; VIR_DELETE_ELEMENT(devs->objs, i, devs->count); break; @@ -310,15 +310,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; } @@ -331,19 +331,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; } @@ -355,19 +355,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; } @@ -378,19 +378,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; } @@ -399,19 +399,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; } @@ -459,12 +459,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; @@ -549,9 +549,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 */ @@ -581,7 +581,7 @@ virNodeDeviceMatch(virNodeDeviceObjPtr devobj, int virNodeDeviceObjListExport(virConnectPtr conn, - virNodeDeviceObjListPtr devobjs, + virNodeDeviceObjListPtr devs, virNodeDevicePtr **devices, virNodeDeviceObjListFilter filter, unsigned int flags) @@ -592,26 +592,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.3
John Ferlan <jferlan@redhat.com> [2017-05-19, 09:08AM -0400]:
A virNodeDeviceObjPtr is an @obj
A virNodeDeviceObjListPtr is an @devs
More intuitive for the virNodeDeviceObjListPtr would be @objs, but I guess the naming scheme is already well defined here.
On 05/23/2017 04:28 AM, Bjoern Walk wrote:
John Ferlan <jferlan@redhat.com> [2017-05-19, 09:08AM -0400]:
A virNodeDeviceObjPtr is an @obj
A virNodeDeviceObjListPtr is an @devs
More intuitive for the virNodeDeviceObjListPtr would be @objs, but I guess the naming scheme is already well defined here.
True.. but I've followed the naming for each driver so far... I probably would confuse myself between @obj and @objs though ;-) John
Alter the node_device_driver source and prototypes to follow more recent code style guidelines w/r/t spacing between functions, format of the function, and the prototype definitions. While the new names for nodeDeviceUpdateCaps, nodeDeviceUpdateDriverName, and nodeDeviceGetTime don't follow exactly w/r/t a "vir" prefix, they do follow other driver nomenclature style. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 41 ++++-- src/node_device/node_device_driver.h | 93 +++++++++---- src/node_device/node_device_udev.c | 256 +++++++++++++++++++++-------------- 3 files changed, 252 insertions(+), 138 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index b4c0020..c5a50f8 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -47,7 +47,9 @@ virNodeDeviceDriverStatePtr driver; -static int update_caps(virNodeDeviceObjPtr dev) + +static int +nodeDeviceUpdateCaps(virNodeDeviceObjPtr dev) { virNodeDevCapsDefPtr cap = dev->def->caps; @@ -103,7 +105,8 @@ static int update_caps(virNodeDeviceObjPtr dev) * the driver name for a device each time its entry is used, both for * udev *and* HAL backends. */ -static int update_driver_name(virNodeDeviceObjPtr dev) +static int +nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev) { char *driver_link = NULL; char *devpath = NULL; @@ -140,7 +143,8 @@ static int update_driver_name(virNodeDeviceObjPtr dev) } #else /* XXX: Implement me for non-linux */ -static int update_driver_name(virNodeDeviceObjPtr dev ATTRIBUTE_UNUSED) +static int +nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev ATTRIBUTE_UNUSED) { return 0; } @@ -151,11 +155,14 @@ void nodeDeviceLock(void) { virMutexLock(&driver->lock); } + + void nodeDeviceUnlock(void) { virMutexUnlock(&driver->lock); } + int nodeNumOfDevices(virConnectPtr conn, const char *cap, @@ -200,6 +207,7 @@ nodeListDevices(virConnectPtr conn, return nnames; } + int nodeConnectListAllNodeDevices(virConnectPtr conn, virNodeDevicePtr **devices, @@ -220,8 +228,10 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, return ret; } + virNodeDevicePtr -nodeDeviceLookupByName(virConnectPtr conn, const char *name) +nodeDeviceLookupByName(virConnectPtr conn, + const char *name) { virNodeDeviceObjPtr obj; virNodeDevicePtr ret = NULL; @@ -328,8 +338,8 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, obj->def) < 0) goto cleanup; - update_driver_name(obj); - if (update_caps(obj) < 0) + nodeDeviceUpdateDriverName(obj); + if (nodeDeviceUpdateCaps(obj) < 0) goto cleanup; ret = virNodeDeviceDefFormat(obj->def); @@ -421,8 +431,11 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev) } + int -nodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) +nodeDeviceListCaps(virNodeDevicePtr dev, + char **const names, + int maxnames) { virNodeDeviceObjPtr obj; virNodeDevCapsDefPtr caps; @@ -478,8 +491,9 @@ nodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) return ret; } + static int -get_time(time_t *t) +nodeDeviceGetTime(time_t *t) { int ret = 0; @@ -522,7 +536,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const char *wwpn) * doesn't become invalid. */ nodeDeviceUnlock(); - get_time(&start); + nodeDeviceGetTime(&start); while ((now - start) < LINUX_NEW_DEVICE_WAIT_TIME) { @@ -534,7 +548,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const char *wwpn) break; sleep(5); - if (get_time(&now) == -1) + if (nodeDeviceGetTime(&now) == -1) break; } @@ -543,6 +557,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const char *wwpn) return dev; } + virNodeDevicePtr nodeDeviceCreateXML(virConnectPtr conn, const char *xmlDesc, @@ -641,6 +656,7 @@ nodeDeviceDestroy(virNodeDevicePtr dev) return ret; } + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr dev, @@ -662,6 +678,7 @@ nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, return callbackID; } + int nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, int callbackID) @@ -682,7 +699,9 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, return ret; } -int nodedevRegister(void) + +int +nodedevRegister(void) { #ifdef WITH_UDEV return udevNodeRegister(); diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index bc8af8a..b46f001 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -31,37 +31,75 @@ # define LINUX_NEW_DEVICE_WAIT_TIME 60 # ifdef WITH_HAL -int halNodeRegister(void); +int +halNodeRegister(void); # endif + # ifdef WITH_UDEV -int udevNodeRegister(void); +int +udevNodeRegister(void); # endif -void nodeDeviceLock(void); -void nodeDeviceUnlock(void); - -extern virNodeDeviceDriverStatePtr driver; - -int nodedevRegister(void); - -int nodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags); -int nodeListDevices(virConnectPtr conn, const char *cap, char **const names, - int maxnames, unsigned int flags); -int nodeConnectListAllNodeDevices(virConnectPtr conn, - virNodeDevicePtr **devices, - unsigned int flags); -virNodeDevicePtr nodeDeviceLookupByName(virConnectPtr conn, const char *name); -virNodeDevicePtr nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, - const char *wwnn, - const char *wwpn, - unsigned int flags); -char *nodeDeviceGetXMLDesc(virNodeDevicePtr dev, unsigned int flags); -char *nodeDeviceGetParent(virNodeDevicePtr dev); -int nodeDeviceNumOfCaps(virNodeDevicePtr dev); -int nodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames); -virNodeDevicePtr nodeDeviceCreateXML(virConnectPtr conn, - const char *xmlDesc, unsigned int flags); -int nodeDeviceDestroy(virNodeDevicePtr dev); +void +nodeDeviceLock(void); + +void +nodeDeviceUnlock(void); + +extern +virNodeDeviceDriverStatePtr driver; + +int +nodedevRegister(void); + +int +nodeNumOfDevices(virConnectPtr conn, + const char *cap, + unsigned int flags); + +int nodeListDevices(virConnectPtr conn, + const char *cap, + char **const names, + int maxnames, + unsigned int flags); + +int +nodeConnectListAllNodeDevices(virConnectPtr conn, + virNodeDevicePtr **devices, + unsigned int flags); + +virNodeDevicePtr +nodeDeviceLookupByName(virConnectPtr conn, + const char *name); + +virNodeDevicePtr +nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, + const char *wwnn, + const char *wwpn, + unsigned int flags); + +char * +nodeDeviceGetXMLDesc(virNodeDevicePtr dev, + unsigned int flags); + +char * +nodeDeviceGetParent(virNodeDevicePtr dev); + +int +nodeDeviceNumOfCaps(virNodeDevicePtr dev); + +int +nodeDeviceListCaps(virNodeDevicePtr dev, + char **const names, + int maxnames); + +virNodeDevicePtr +nodeDeviceCreateXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + +int +nodeDeviceDestroy(virNodeDevicePtr dev); int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, @@ -73,4 +111,5 @@ nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, int nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, int callbackID); + #endif /* __VIR_NODE_DEVICE_H__ */ diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index c88a234..9ff8691 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -71,8 +71,9 @@ udevHasDeviceProperty(struct udev_device *dev, } -static const char *udevGetDeviceProperty(struct udev_device *udev_device, - const char *property_key) +static const char * +udevGetDeviceProperty(struct udev_device *udev_device, + const char *property_key) { const char *ret = NULL; @@ -85,9 +86,10 @@ static const char *udevGetDeviceProperty(struct udev_device *udev_device, } -static int udevGetStringProperty(struct udev_device *udev_device, - const char *property_key, - char **value) +static int +udevGetStringProperty(struct udev_device *udev_device, + const char *property_key, + char **value) { if (VIR_STRDUP(*value, udevGetDeviceProperty(udev_device, property_key)) < 0) @@ -97,10 +99,11 @@ static int udevGetStringProperty(struct udev_device *udev_device, } -static int udevGetIntProperty(struct udev_device *udev_device, - const char *property_key, - int *value, - int base) +static int +udevGetIntProperty(struct udev_device *udev_device, + const char *property_key, + int *value, + int base) { const char *str = NULL; @@ -115,10 +118,11 @@ static int udevGetIntProperty(struct udev_device *udev_device, } -static int udevGetUintProperty(struct udev_device *udev_device, - const char *property_key, - unsigned int *value, - int base) +static int +udevGetUintProperty(struct udev_device *udev_device, + const char *property_key, + unsigned int *value, + int base) { const char *str = NULL; @@ -133,8 +137,9 @@ static int udevGetUintProperty(struct udev_device *udev_device, } -static const char *udevGetDeviceSysfsAttr(struct udev_device *udev_device, - const char *attr_name) +static const char * +udevGetDeviceSysfsAttr(struct udev_device *udev_device, + const char *attr_name) { const char *ret = NULL; @@ -148,9 +153,10 @@ static const char *udevGetDeviceSysfsAttr(struct udev_device *udev_device, } -static int udevGetStringSysfsAttr(struct udev_device *udev_device, - const char *attr_name, - char **value) +static int +udevGetStringSysfsAttr(struct udev_device *udev_device, + const char *attr_name, + char **value) { if (VIR_STRDUP(*value, udevGetDeviceSysfsAttr(udev_device, attr_name)) < 0) return -1; @@ -164,10 +170,11 @@ static int udevGetStringSysfsAttr(struct udev_device *udev_device, } -static int udevGetIntSysfsAttr(struct udev_device *udev_device, - const char *attr_name, - int *value, - int base) +static int +udevGetIntSysfsAttr(struct udev_device *udev_device, + const char *attr_name, + int *value, + int base) { const char *str = NULL; @@ -183,10 +190,11 @@ static int udevGetIntSysfsAttr(struct udev_device *udev_device, } -static int udevGetUintSysfsAttr(struct udev_device *udev_device, - const char *attr_name, - unsigned int *value, - int base) +static int +udevGetUintSysfsAttr(struct udev_device *udev_device, + const char *attr_name, + unsigned int *value, + int base) { const char *str = NULL; @@ -202,9 +210,10 @@ static int udevGetUintSysfsAttr(struct udev_device *udev_device, } -static int udevGetUint64SysfsAttr(struct udev_device *udev_device, - const char *attr_name, - unsigned long long *value) +static int +udevGetUint64SysfsAttr(struct udev_device *udev_device, + const char *attr_name, + unsigned long long *value) { const char *str = NULL; @@ -220,9 +229,10 @@ static int udevGetUint64SysfsAttr(struct udev_device *udev_device, } -static int udevGenerateDeviceName(struct udev_device *device, - virNodeDeviceDefPtr def, - const char *s) +static int +udevGenerateDeviceName(struct udev_device *device, + virNodeDeviceDefPtr def, + const char *s) { size_t i; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -247,14 +257,16 @@ static int udevGenerateDeviceName(struct udev_device *device, return 0; } + #if HAVE_UDEV_LOGGING -typedef void (*udevLogFunctionPtr)(struct udev *udev, - int priority, - const char *file, - int line, - const char *fn, - const char *format, - va_list args); +typedef void +(*udevLogFunctionPtr)(struct udev *udev, + int priority, + const char *file, + int line, + const char *fn, + const char *format, + va_list args); static void ATTRIBUTE_FMT_PRINTF(6, 0) @@ -283,10 +295,11 @@ udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED, #endif -static int udevTranslatePCIIds(unsigned int vendor, - unsigned int product, - char **vendor_string, - char **product_string) +static int +udevTranslatePCIIds(unsigned int vendor, + unsigned int product, + char **vendor_string, + char **product_string) { struct pci_id_match m; const char *vendor_name = NULL, *device_name = NULL; @@ -427,8 +440,9 @@ udevPCIGetMdevTypesCap(struct udev_device *device, } -static int udevProcessPCI(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessPCI(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapPCIDevPtr pci_dev = &def->caps->data.pci_dev; virPCIEDeviceInfoPtr pci_express = NULL; @@ -527,7 +541,9 @@ static int udevProcessPCI(struct udev_device *device, return ret; } -static int drmGetMinorType(int minor) + +static int +drmGetMinorType(int minor) { int type = minor >> 6; @@ -544,8 +560,10 @@ static int drmGetMinorType(int minor) } } -static int udevProcessDRMDevice(struct udev_device *device, - virNodeDeviceDefPtr def) + +static int +udevProcessDRMDevice(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapDRMPtr drm = &def->caps->data.drm; int minor; @@ -564,8 +582,10 @@ static int udevProcessDRMDevice(struct udev_device *device, return 0; } -static int udevProcessUSBDevice(struct udev_device *device, - virNodeDeviceDefPtr def) + +static int +udevProcessUSBDevice(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapUSBDevPtr usb_dev = &def->caps->data.usb_dev; @@ -606,8 +626,9 @@ static int udevProcessUSBDevice(struct udev_device *device, } -static int udevProcessUSBInterface(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessUSBInterface(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapUSBIfPtr usb_if = &def->caps->data.usb_if; @@ -634,8 +655,9 @@ static int udevProcessUSBInterface(struct udev_device *device, } -static int udevProcessNetworkInterface(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessNetworkInterface(struct udev_device *device, + virNodeDeviceDefPtr def) { const char *devtype = udev_device_get_devtype(device); virNodeDevCapNetPtr net = &def->caps->data.net; @@ -671,8 +693,9 @@ static int udevProcessNetworkInterface(struct udev_device *device, } -static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, - virNodeDeviceDefPtr def) +static int +udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, + virNodeDeviceDefPtr def) { virNodeDevCapSCSIHostPtr scsi_host = &def->caps->data.scsi_host; char *filename = NULL; @@ -697,8 +720,9 @@ static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, } -static int udevProcessSCSITarget(struct udev_device *device ATTRIBUTE_UNUSED, - virNodeDeviceDefPtr def) +static int +udevProcessSCSITarget(struct udev_device *device ATTRIBUTE_UNUSED, + virNodeDeviceDefPtr def) { const char *sysname = NULL; virNodeDevCapSCSITargetPtr scsi_target = &def->caps->data.scsi_target; @@ -715,8 +739,10 @@ static int udevProcessSCSITarget(struct udev_device *device ATTRIBUTE_UNUSED, } -static int udevGetSCSIType(virNodeDeviceDefPtr def ATTRIBUTE_UNUSED, - unsigned int type, char **typestring) +static int +udevGetSCSIType(virNodeDeviceDefPtr def ATTRIBUTE_UNUSED, + unsigned int type, + char **typestring) { int ret = 0; int foundtype = 1; @@ -773,8 +799,9 @@ static int udevGetSCSIType(virNodeDeviceDefPtr def ATTRIBUTE_UNUSED, } -static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, - virNodeDeviceDefPtr def) +static int +udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, + virNodeDeviceDefPtr def) { int ret = -1; unsigned int tmp = 0; @@ -816,8 +843,9 @@ static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, } -static int udevProcessDisk(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessDisk(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapStoragePtr storage = &def->caps->data.storage; @@ -834,9 +862,10 @@ static int udevProcessDisk(struct udev_device *device, } -static int udevProcessRemoveableMedia(struct udev_device *device, - virNodeDeviceDefPtr def, - int has_media) +static int +udevProcessRemoveableMedia(struct udev_device *device, + virNodeDeviceDefPtr def, + int has_media) { virNodeDevCapStoragePtr storage = &def->caps->data.storage; int is_removable = 0; @@ -875,8 +904,10 @@ static int udevProcessRemoveableMedia(struct udev_device *device, return 0; } -static int udevProcessCDROM(struct udev_device *device, - virNodeDeviceDefPtr def) + +static int +udevProcessCDROM(struct udev_device *device, + virNodeDeviceDefPtr def) { int has_media = 0; @@ -895,8 +926,10 @@ static int udevProcessCDROM(struct udev_device *device, return udevProcessRemoveableMedia(device, def, has_media); } -static int udevProcessFloppy(struct udev_device *device, - virNodeDeviceDefPtr def) + +static int +udevProcessFloppy(struct udev_device *device, + virNodeDeviceDefPtr def) { int has_media = 0; @@ -913,8 +946,9 @@ static int udevProcessFloppy(struct udev_device *device, } -static int udevProcessSD(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessSD(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapStoragePtr storage = &def->caps->data.storage; @@ -932,12 +966,12 @@ static int udevProcessSD(struct udev_device *device, } - /* This function exists to deal with the case in which a driver does * not provide a device type in the usual place, but udev told us it's * a storage device, and we can make a good guess at what kind of * storage device it is from other information that is provided. */ -static int udevKludgeStorageType(virNodeDeviceDefPtr def) +static int +udevKludgeStorageType(virNodeDeviceDefPtr def) { VIR_DEBUG("Could not find definitive storage type for device " "with sysfs path '%s', trying to guess it", @@ -958,8 +992,9 @@ static int udevKludgeStorageType(virNodeDeviceDefPtr def) } -static int udevProcessStorage(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessStorage(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapStoragePtr storage = &def->caps->data.storage; int ret = -1; @@ -1054,6 +1089,7 @@ static int udevProcessStorage(struct udev_device *device, return ret; } + static int udevProcessSCSIGeneric(struct udev_device *dev, virNodeDeviceDefPtr def) @@ -1068,6 +1104,7 @@ udevProcessSCSIGeneric(struct udev_device *dev, return 0; } + static int udevProcessMediatedDevice(struct udev_device *dev, virNodeDeviceDefPtr def) @@ -1132,6 +1169,7 @@ udevGetDeviceNodes(struct udev_device *device, return 0; } + static int udevGetDeviceType(struct udev_device *device, virNodeDevCapType *type) @@ -1196,8 +1234,9 @@ udevGetDeviceType(struct udev_device *device, } -static int udevGetDeviceDetails(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevGetDeviceDetails(struct udev_device *device, + virNodeDeviceDefPtr def) { switch (def->caps->data.type) { case VIR_NODE_DEV_CAP_PCI_DEV: @@ -1234,7 +1273,8 @@ static int udevGetDeviceDetails(struct udev_device *device, } -static int udevRemoveOneDevice(struct udev_device *device) +static int +udevRemoveOneDevice(struct udev_device *device) { virNodeDeviceObjPtr dev = NULL; virObjectEventPtr event = NULL; @@ -1266,8 +1306,9 @@ static int udevRemoveOneDevice(struct udev_device *device) } -static int udevSetParent(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevSetParent(struct udev_device *device, + virNodeDeviceDefPtr def) { struct udev_device *parent_device = NULL; const char *parent_sysfs_path = NULL; @@ -1314,7 +1355,8 @@ static int udevSetParent(struct udev_device *device, } -static int udevAddOneDevice(struct udev_device *device) +static int +udevAddOneDevice(struct udev_device *device) { virNodeDeviceDefPtr def = NULL; virNodeDeviceObjPtr dev = NULL; @@ -1383,8 +1425,9 @@ static int udevAddOneDevice(struct udev_device *device) } -static int udevProcessDeviceListEntry(struct udev *udev, - struct udev_list_entry *list_entry) +static int +udevProcessDeviceListEntry(struct udev *udev, + struct udev_list_entry *list_entry) { struct udev_device *device; const char *name = NULL; @@ -1416,7 +1459,8 @@ const char *subsystem_blacklist[] = { "acpi", "tty", "vc", "i2c", }; -static int udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) +static int +udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) { size_t i; @@ -1431,7 +1475,8 @@ static int udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) } -static int udevEnumerateDevices(struct udev *udev) +static int +udevEnumerateDevices(struct udev *udev) { struct udev_enumerate *udev_enumerate = NULL; struct udev_list_entry *list_entry = NULL; @@ -1461,7 +1506,8 @@ static int udevEnumerateDevices(struct udev *udev) } -static void udevPCITranslateDeinit(void) +static void +udevPCITranslateDeinit(void) { #if defined __s390__ || defined __s390x_ /* Nothing was initialized, nothing needs to be cleaned up */ @@ -1473,7 +1519,8 @@ static void udevPCITranslateDeinit(void) } -static int nodeStateCleanup(void) +static int +nodeStateCleanup(void) { udevPrivate *priv = NULL; struct udev_monitor *udev_monitor = NULL; @@ -1514,10 +1561,11 @@ static int nodeStateCleanup(void) } -static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, - int fd, - int events ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED) +static void +udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, + int fd, + int events ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED) { struct udev_device *device = NULL; struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver); @@ -1618,7 +1666,8 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap) #endif -static int udevSetupSystemDev(void) +static int +udevSetupSystemDev(void) { virNodeDeviceDefPtr def = NULL; virNodeDeviceObjPtr dev = NULL; @@ -1652,7 +1701,9 @@ static int udevSetupSystemDev(void) return ret; } -static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) + +static int +udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) { #if defined __s390__ || defined __s390x_ /* On s390(x) system there is no PCI bus. @@ -1674,9 +1725,11 @@ static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) return 0; } -static int nodeStateInitialize(bool privileged, - virStateInhibitCallback callback ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + +static int +nodeStateInitialize(bool privileged, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) { udevPrivate *priv = NULL; struct udev *udev = NULL; @@ -1762,7 +1815,8 @@ static int nodeStateInitialize(bool privileged, } -static int nodeStateReload(void) +static int +nodeStateReload(void) { return 0; } @@ -1792,7 +1846,9 @@ static virStateDriver udevStateDriver = { .stateReload = nodeStateReload, /* 0.7.3 */ }; -int udevNodeRegister(void) + +int +udevNodeRegister(void) { VIR_DEBUG("Registering udev node device backend"); -- 2.9.3
Rather than taking an virNodeDeviceObjPtr and dereffing the obj->def, just pass the def. Also check for an error in the function to have the calling function goto cleanup on error. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index c5a50f8..26ed0f1 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -106,16 +106,16 @@ nodeDeviceUpdateCaps(virNodeDeviceObjPtr dev) * udev *and* HAL backends. */ static int -nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev) +nodeDeviceUpdateDriverName(virNodeDeviceDefPtr def) { char *driver_link = NULL; char *devpath = NULL; char *p; int ret = -1; - VIR_FREE(dev->def->driver); + VIR_FREE(def->driver); - if (virAsprintf(&driver_link, "%s/driver", dev->def->sysfs_path) < 0) + if (virAsprintf(&driver_link, "%s/driver", def->sysfs_path) < 0) goto cleanup; /* Some devices don't have an explicit driver, so just return @@ -132,7 +132,7 @@ nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev) } p = strrchr(devpath, '/'); - if (p && VIR_STRDUP(dev->def->driver, p + 1) < 0) + if (p && VIR_STRDUP(def->driver, p + 1) < 0) goto cleanup; ret = 0; @@ -144,7 +144,7 @@ nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev) #else /* XXX: Implement me for non-linux */ static int -nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev ATTRIBUTE_UNUSED) +nodeDeviceUpdateDriverName(virNodeDeviceDefPtr def ATTRIBUTE_UNUSED) { return 0; } @@ -338,7 +338,9 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, obj->def) < 0) goto cleanup; - nodeDeviceUpdateDriverName(obj); + if (nodeDeviceUpdateDriverName(obj->def) < 0) + goto cleanup; + if (nodeDeviceUpdateCaps(obj) < 0) goto cleanup; -- 2.9.3
Rather than taking an virNodeDeviceObjPtr and dereffing the obj->def, just pass the def. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 26ed0f1..d4e9f6b 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -49,14 +49,14 @@ virNodeDeviceDriverStatePtr driver; static int -nodeDeviceUpdateCaps(virNodeDeviceObjPtr dev) +nodeDeviceUpdateCaps(virNodeDeviceDefPtr def) { - virNodeDevCapsDefPtr cap = dev->def->caps; + virNodeDevCapsDefPtr cap = def->caps; while (cap) { switch (cap->data.type) { case VIR_NODE_DEV_CAP_SCSI_HOST: - virNodeDeviceSysfsGetSCSIHostCaps(&dev->def->caps->data.scsi_host); + virNodeDeviceSysfsGetSCSIHostCaps(&def->caps->data.scsi_host); break; case VIR_NODE_DEV_CAP_NET: if (virNetDevGetLinkInfo(cap->data.net.ifname, &cap->data.net.lnk) < 0) @@ -66,8 +66,8 @@ nodeDeviceUpdateCaps(virNodeDeviceObjPtr dev) return -1; break; case VIR_NODE_DEV_CAP_PCI_DEV: - if (virNodeDeviceSysfsGetPCIRelatedDevCaps(dev->def->sysfs_path, - &dev->def->caps->data.pci_dev) < 0) + if (virNodeDeviceSysfsGetPCIRelatedDevCaps(def->sysfs_path, + &def->caps->data.pci_dev) < 0) return -1; break; @@ -341,7 +341,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, if (nodeDeviceUpdateDriverName(obj->def) < 0) goto cleanup; - if (nodeDeviceUpdateCaps(obj) < 0) + if (nodeDeviceUpdateCaps(obj->def) < 0) goto cleanup; ret = virNodeDeviceDefFormat(obj->def); -- 2.9.3
Create nodeDeviceObjFindByName which will perform the corresponding virNodeDeviceObjFindByName call for various node_device_driver callers rather than having the same repetitive code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 96 +++++++++++++----------------------- 1 file changed, 33 insertions(+), 63 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index d4e9f6b..279c133 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -229,12 +229,10 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, } -virNodeDevicePtr -nodeDeviceLookupByName(virConnectPtr conn, - const char *name) +static virNodeDeviceObjPtr +nodeDeviceObjFindByName(const char *name) { virNodeDeviceObjPtr obj; - virNodeDevicePtr ret = NULL; nodeDeviceLock(); obj = virNodeDeviceObjFindByName(&driver->devs, name); @@ -244,9 +242,22 @@ nodeDeviceLookupByName(virConnectPtr conn, virReportError(VIR_ERR_NO_NODE_DEVICE, _("no node device with matching name '%s'"), name); - goto cleanup; } + return obj; +} + + +virNodeDevicePtr +nodeDeviceLookupByName(virConnectPtr conn, + const char *name) +{ + virNodeDeviceObjPtr obj; + virNodeDevicePtr ret = NULL; + + if (!(obj = nodeDeviceObjFindByName(name))) + return NULL; + if (virNodeDeviceLookupByNameEnsureACL(conn, obj->def) < 0) goto cleanup; @@ -256,8 +267,7 @@ nodeDeviceLookupByName(virConnectPtr conn, } cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -324,16 +334,8 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, virCheckFlags(0, NULL); - nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, dev->name); - nodeDeviceUnlock(); - - if (!obj) { - virReportError(VIR_ERR_NO_NODE_DEVICE, - _("no node device with matching name '%s'"), - dev->name); - goto cleanup; - } + if (!(obj = nodeDeviceObjFindByName(dev->name))) + return NULL; if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, obj->def) < 0) goto cleanup; @@ -347,8 +349,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, ret = virNodeDeviceDefFormat(obj->def); cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -359,16 +360,8 @@ nodeDeviceGetParent(virNodeDevicePtr dev) virNodeDeviceObjPtr obj; char *ret = NULL; - nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, dev->name); - nodeDeviceUnlock(); - - if (!obj) { - virReportError(VIR_ERR_NO_NODE_DEVICE, - _("no node device with matching name '%s'"), - dev->name); - goto cleanup; - } + if (!(obj = nodeDeviceObjFindByName(dev->name))) + return NULL; if (virNodeDeviceGetParentEnsureACL(dev->conn, obj->def) < 0) goto cleanup; @@ -382,8 +375,7 @@ nodeDeviceGetParent(virNodeDevicePtr dev) } cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -396,16 +388,8 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev) int ncaps = 0; int ret = -1; - nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, dev->name); - nodeDeviceUnlock(); - - if (!obj) { - virReportError(VIR_ERR_NO_NODE_DEVICE, - _("no node device with matching name '%s'"), - dev->name); - goto cleanup; - } + if (!(obj = nodeDeviceObjFindByName(dev->name))) + return -1; if (virNodeDeviceNumOfCapsEnsureACL(dev->conn, obj->def) < 0) goto cleanup; @@ -427,8 +411,7 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev) ret = ncaps; cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -444,16 +427,8 @@ nodeDeviceListCaps(virNodeDevicePtr dev, int ncaps = 0; int ret = -1; - nodeDeviceLock(); - obj = virNodeDeviceObjFindByName(&driver->devs, dev->name); - nodeDeviceUnlock(); - - if (!obj) { - virReportError(VIR_ERR_NO_NODE_DEVICE, - _("no node device with matching name '%s'"), - dev->name); - goto cleanup; - } + if (!(obj = nodeDeviceObjFindByName(dev->name))) + return -1; if (virNodeDeviceListCapsEnsureACL(dev->conn, obj->def) < 0) goto cleanup; @@ -483,8 +458,7 @@ nodeDeviceListCaps(virNodeDevicePtr dev, ret = ncaps; cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); if (ret == -1) { --ncaps; while (--ncaps >= 0) @@ -619,13 +593,10 @@ nodeDeviceDestroy(virNodeDevicePtr dev) char *wwnn = NULL, *wwpn = NULL; int parent_host = -1; + if (!(obj = nodeDeviceObjFindByName(dev->name))) + return -1; + nodeDeviceLock(); - if (!(obj = virNodeDeviceObjFindByName(&driver->devs, dev->name))) { - virReportError(VIR_ERR_NO_NODE_DEVICE, - _("no node device with matching name '%s'"), - dev->name); - goto cleanup; - } if (virNodeDeviceDestroyEnsureACL(dev->conn, obj->def) < 0) goto cleanup; @@ -651,8 +622,7 @@ nodeDeviceDestroy(virNodeDevicePtr dev) cleanup: nodeDeviceUnlock(); - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); VIR_FREE(wwnn); VIR_FREE(wwpn); return ret; -- 2.9.3
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 | 70 ++++++++++++++++++------------------ src/node_device/node_device_hal.c | 42 +++++++++++----------- src/node_device/node_device_udev.c | 50 +++++++++++++------------- 3 files changed, 81 insertions(+), 81 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 279c133..fe2cb3c 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -253,7 +253,7 @@ nodeDeviceLookupByName(virConnectPtr conn, const char *name) { virNodeDeviceObjPtr obj; - virNodeDevicePtr ret = NULL; + virNodeDevicePtr device = NULL; if (!(obj = nodeDeviceObjFindByName(name))) return NULL; @@ -261,14 +261,14 @@ nodeDeviceLookupByName(virConnectPtr conn, if (virNodeDeviceLookupByNameEnsureACL(conn, obj->def) < 0) goto cleanup; - if ((ret = virGetNodeDevice(conn, name))) { - if (VIR_STRDUP(ret->parent, obj->def->parent) < 0) - virObjectUnref(ret); + if ((device = virGetNodeDevice(conn, name))) { + if (VIR_STRDUP(device->parent, obj->def->parent) < 0) + virObjectUnref(device); } cleanup: virNodeDeviceObjUnlock(obj); - return ret; + return device; } @@ -282,7 +282,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, virNodeDeviceObjListPtr devs = &driver->devs; virNodeDevCapsDefPtr cap = NULL; virNodeDeviceObjPtr obj = NULL; - virNodeDevicePtr dev = NULL; + virNodeDevicePtr device = NULL; virCheckFlags(0, NULL); @@ -304,9 +304,9 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, obj->def) < 0) goto out; - if ((dev = virGetNodeDevice(conn, obj->def->name))) { - if (VIR_STRDUP(dev->parent, obj->def->parent) < 0) - virObjectUnref(dev); + if ((device = virGetNodeDevice(conn, obj->def->name))) { + if (VIR_STRDUP(device->parent, obj->def->parent) < 0) + virObjectUnref(device); } virNodeDeviceObjUnlock(obj); goto out; @@ -321,12 +321,12 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, out: nodeDeviceUnlock(); - return dev; + return device; } char * -nodeDeviceGetXMLDesc(virNodeDevicePtr dev, +nodeDeviceGetXMLDesc(virNodeDevicePtr device, unsigned int flags) { virNodeDeviceObjPtr obj; @@ -334,10 +334,10 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, virCheckFlags(0, NULL); - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return NULL; - if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, obj->def) < 0) + if (virNodeDeviceGetXMLDescEnsureACL(device->conn, obj->def) < 0) goto cleanup; if (nodeDeviceUpdateDriverName(obj->def) < 0) @@ -355,15 +355,15 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, char * -nodeDeviceGetParent(virNodeDevicePtr dev) +nodeDeviceGetParent(virNodeDevicePtr device) { virNodeDeviceObjPtr obj; char *ret = NULL; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return NULL; - if (virNodeDeviceGetParentEnsureACL(dev->conn, obj->def) < 0) + if (virNodeDeviceGetParentEnsureACL(device->conn, obj->def) < 0) goto cleanup; if (obj->def->parent) { @@ -381,17 +381,17 @@ nodeDeviceGetParent(virNodeDevicePtr dev) int -nodeDeviceNumOfCaps(virNodeDevicePtr dev) +nodeDeviceNumOfCaps(virNodeDevicePtr device) { virNodeDeviceObjPtr obj; virNodeDevCapsDefPtr caps; int ncaps = 0; int ret = -1; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; - if (virNodeDeviceNumOfCapsEnsureACL(dev->conn, obj->def) < 0) + if (virNodeDeviceNumOfCapsEnsureACL(device->conn, obj->def) < 0) goto cleanup; for (caps = obj->def->caps; caps; caps = caps->next) { @@ -418,7 +418,7 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev) int -nodeDeviceListCaps(virNodeDevicePtr dev, +nodeDeviceListCaps(virNodeDevicePtr device, char **const names, int maxnames) { @@ -427,10 +427,10 @@ nodeDeviceListCaps(virNodeDevicePtr dev, int ncaps = 0; int ret = -1; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; - if (virNodeDeviceListCapsEnsureACL(dev->conn, obj->def) < 0) + if (virNodeDeviceListCapsEnsureACL(device->conn, obj->def) < 0) goto cleanup; for (caps = obj->def->caps; caps && ncaps < maxnames; caps = caps->next) { @@ -502,7 +502,7 @@ nodeDeviceGetTime(time_t *t) static virNodeDevicePtr find_new_device(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 @@ -518,9 +518,9 @@ find_new_device(virConnectPtr conn, const char *wwnn, const char *wwpn) virWaitForDevices(); - dev = nodeDeviceLookupSCSIHostByWWN(conn, wwnn, wwpn, 0); + device = nodeDeviceLookupSCSIHostByWWN(conn, wwnn, wwpn, 0); - if (dev != NULL) + if (device != NULL) break; sleep(5); @@ -530,7 +530,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const char *wwpn) nodeDeviceLock(); - return dev; + return device; } @@ -542,7 +542,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); @@ -566,11 +566,11 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_CREATE) < 0) goto cleanup; - dev = find_new_device(conn, wwnn, wwpn); + device = find_new_device(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'"), @@ -580,12 +580,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; @@ -593,12 +593,12 @@ nodeDeviceDestroy(virNodeDevicePtr dev) char *wwnn = NULL, *wwpn = NULL; int parent_host = -1; - if (!(obj = nodeDeviceObjFindByName(dev->name))) + if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; nodeDeviceLock(); - if (virNodeDeviceDestroyEnsureACL(dev->conn, obj->def) < 0) + if (virNodeDeviceDestroyEnsureACL(device->conn, obj->def) < 0) goto cleanup; if (virNodeDeviceGetWWNs(obj->def, &wwnn, &wwpn) < 0) @@ -631,7 +631,7 @@ nodeDeviceDestroy(virNodeDevicePtr dev) int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, - virNodeDevicePtr dev, + virNodeDevicePtr device, int eventID, virConnectNodeDeviceEventGenericCallback callback, void *opaque, @@ -643,7 +643,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 e76201b..11b9aab 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -457,7 +457,7 @@ dev_create(const char *udi) { LibHalContext *ctx; char *parent_key = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def = NULL; const char *name = hal_name(udi); int rv; @@ -493,17 +493,17 @@ 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); - dev = virNodeDeviceObjAssignDef(&driver->devs, def); - if (!dev) { + obj = virNodeDeviceObjAssignDef(&driver->devs, def); + if (!obj) { VIR_FREE(devicePath); goto failure; } - dev->privateData = privData; - dev->privateFree = free_udi; - dev->def->sysfs_path = devicePath; + obj->privateData = privData; + obj->privateFree = free_udi; + obj->def->sysfs_path = devicePath; - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); nodeDeviceUnlock(); return; @@ -520,21 +520,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) { + obj = virNodeDeviceObjFindByName(&driver->devs, name); + if (obj) { /* 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) + if (obj) dev_create(udi); } @@ -552,13 +552,13 @@ 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(); @@ -570,15 +570,15 @@ device_cap_added(LibHalContext *ctx, const char *udi, const char *cap) { const char *name = hal_name(udi); - virNodeDeviceObjPtr dev; + virNodeDeviceObjPtr obj; nodeDeviceLock(); - dev = virNodeDeviceObjFindByName(&driver->devs, name); + obj = virNodeDeviceObjFindByName(&driver->devs, name); nodeDeviceUnlock(); VIR_DEBUG("%s %s", cap, name); - if (dev) { - (void)gather_capability(ctx, udi, cap, &dev->def->caps); - virNodeDeviceObjUnlock(dev); + if (obj) { + (void)gather_capability(ctx, udi, cap, &obj->def->caps); + 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 9ff8691..6fd8896 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1276,27 +1276,27 @@ udevGetDeviceDetails(struct udev_device *device, static int udevRemoveOneDevice(struct udev_device *device) { - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virObjectEventPtr event = NULL; const char *name = NULL; int ret = -1; name = udev_device_get_syspath(device); - dev = virNodeDeviceObjFindBySysfsPath(&driver->devs, name); + obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, name); - if (!dev) { + if (!obj) { VIR_DEBUG("Failed to find device to remove that has udev name '%s'", name); goto cleanup; } - event = virNodeDeviceEventLifecycleNew(dev->def->name, + event = virNodeDeviceEventLifecycleNew(obj->def->name, VIR_NODE_DEVICE_EVENT_DELETED, 0); VIR_DEBUG("Removing device '%s' with sysfs path '%s'", - dev->def->name, name); - virNodeDeviceObjRemove(&driver->devs, &dev); + obj->def->name, name); + virNodeDeviceObjRemove(&driver->devs, &obj); ret = 0; cleanup: @@ -1312,7 +1312,7 @@ udevSetParent(struct udev_device *device, { struct udev_device *parent_device = NULL; const char *parent_sysfs_path = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; int ret = -1; parent_device = device; @@ -1330,14 +1330,14 @@ udevSetParent(struct udev_device *device, goto cleanup; } - dev = virNodeDeviceObjFindBySysfsPath(&driver->devs, + obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, parent_sysfs_path); - if (dev != NULL) { - if (VIR_STRDUP(def->parent, dev->def->name) < 0) { - virNodeDeviceObjUnlock(dev); + if (obj != NULL) { + if (VIR_STRDUP(def->parent, obj->def->name) < 0) { + virNodeDeviceObjUnlock(obj); goto cleanup; } - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); if (VIR_STRDUP(def->parent_sysfs_path, parent_sysfs_path) < 0) goto cleanup; @@ -1359,7 +1359,7 @@ static int udevAddOneDevice(struct udev_device *device) { virNodeDeviceDefPtr def = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; virObjectEventPtr event = NULL; bool new_device = true; int ret = -1; @@ -1388,26 +1388,26 @@ udevAddOneDevice(struct udev_device *device) if (udevSetParent(device, def) != 0) goto cleanup; - dev = virNodeDeviceObjFindByName(&driver->devs, def->name); - if (dev) { - virNodeDeviceObjUnlock(dev); + obj = virNodeDeviceObjFindByName(&driver->devs, def->name); + if (obj) { + 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) + obj = virNodeDeviceObjAssignDef(&driver->devs, def); + if (obj == NULL) goto cleanup; if (new_device) - event = virNodeDeviceEventLifecycleNew(dev->def->name, + event = virNodeDeviceEventLifecycleNew(obj->def->name, VIR_NODE_DEVICE_EVENT_CREATED, 0); else - event = virNodeDeviceEventUpdateNew(dev->def->name); + event = virNodeDeviceEventUpdateNew(obj->def->name); - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); ret = 0; @@ -1670,7 +1670,7 @@ static int udevSetupSystemDev(void) { virNodeDeviceDefPtr def = NULL; - virNodeDeviceObjPtr dev = NULL; + virNodeDeviceObjPtr obj = NULL; int ret = -1; if (VIR_ALLOC(def) < 0) @@ -1686,11 +1686,11 @@ udevSetupSystemDev(void) udevGetDMIData(&def->caps->data.system); #endif - dev = virNodeDeviceObjAssignDef(&driver->devs, def); - if (dev == NULL) + obj = virNodeDeviceObjAssignDef(&driver->devs, def); + if (obj == NULL) goto cleanup; - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(obj); ret = 0; -- 2.9.3
In preparation for privatizing the virNodeDeviceObj - create an accessor for the @def field and then use it for various callers. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 7 +++++ src/conf/virnodedeviceobj.h | 2 ++ src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 52 ++++++++++++++++++++++-------------- src/node_device/node_device_hal.c | 11 +++++--- src/node_device/node_device_udev.c | 35 ++++++++++++------------ src/test/test_driver.c | 38 +++++++++++++++++--------- 7 files changed, 92 insertions(+), 54 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 3a833e1..dcf871c 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -33,6 +33,13 @@ VIR_LOG_INIT("conf.virnodedeviceobj"); +virNodeDeviceDefPtr +virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj) +{ + return obj->def; +} + + static int virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, const char *cap) diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index b8b534b..135a424 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -39,6 +39,8 @@ struct _virNodeDeviceDriverState { virObjectEventStatePtr nodeDeviceEventState; }; +virNodeDeviceDefPtr +virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj); virNodeDeviceObjPtr virNodeDeviceObjFindByName(virNodeDeviceObjListPtr devs, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d8602e6..2d12b2e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -960,6 +960,7 @@ virNetworkObjUpdateAssignDef; virNodeDeviceObjAssignDef; virNodeDeviceObjFindByName; virNodeDeviceObjFindBySysfsPath; +virNodeDeviceObjGetDef; virNodeDeviceObjGetNames; virNodeDeviceObjGetParentHost; virNodeDeviceObjListExport; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index fe2cb3c..2c79044 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -253,16 +253,18 @@ nodeDeviceLookupByName(virConnectPtr conn, const char *name) { virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; virNodeDevicePtr device = NULL; if (!(obj = nodeDeviceObjFindByName(name))) return NULL; + def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceLookupByNameEnsureACL(conn, obj->def) < 0) + if (virNodeDeviceLookupByNameEnsureACL(conn, def) < 0) goto cleanup; if ((device = virGetNodeDevice(conn, name))) { - if (VIR_STRDUP(device->parent, obj->def->parent) < 0) + if (VIR_STRDUP(device->parent, def->parent) < 0) virObjectUnref(device); } @@ -282,6 +284,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, virNodeDeviceObjListPtr devs = &driver->devs; virNodeDevCapsDefPtr cap = NULL; virNodeDeviceObjPtr obj = NULL; + virNodeDeviceDefPtr def; virNodeDevicePtr device = NULL; virCheckFlags(0, NULL); @@ -291,7 +294,8 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, for (i = 0; i < devs->count; i++) { obj = devs->objs[i]; virNodeDeviceObjLock(obj); - cap = obj->def->caps; + def = virNodeDeviceObjGetDef(obj); + cap = def->caps; while (cap) { if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { @@ -301,11 +305,11 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, if (STREQ(cap->data.scsi_host.wwnn, wwnn) && STREQ(cap->data.scsi_host.wwpn, wwpn)) { - if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, obj->def) < 0) + if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, def) < 0) goto out; - if ((device = virGetNodeDevice(conn, obj->def->name))) { - if (VIR_STRDUP(device->parent, obj->def->parent) < 0) + if ((device = virGetNodeDevice(conn, def->name))) { + if (VIR_STRDUP(device->parent, def->parent) < 0) virObjectUnref(device); } virNodeDeviceObjUnlock(obj); @@ -330,23 +334,25 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device, unsigned int flags) { virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; char *ret = NULL; virCheckFlags(0, NULL); if (!(obj = nodeDeviceObjFindByName(device->name))) return NULL; + def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceGetXMLDescEnsureACL(device->conn, obj->def) < 0) + if (virNodeDeviceGetXMLDescEnsureACL(device->conn, def) < 0) goto cleanup; - if (nodeDeviceUpdateDriverName(obj->def) < 0) + if (nodeDeviceUpdateDriverName(def) < 0) goto cleanup; - if (nodeDeviceUpdateCaps(obj->def) < 0) + if (nodeDeviceUpdateCaps(def) < 0) goto cleanup; - ret = virNodeDeviceDefFormat(obj->def); + ret = virNodeDeviceDefFormat(def); cleanup: virNodeDeviceObjUnlock(obj); @@ -358,16 +364,18 @@ char * nodeDeviceGetParent(virNodeDevicePtr device) { virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; char *ret = NULL; if (!(obj = nodeDeviceObjFindByName(device->name))) return NULL; + def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceGetParentEnsureACL(device->conn, obj->def) < 0) + if (virNodeDeviceGetParentEnsureACL(device->conn, def) < 0) goto cleanup; - if (obj->def->parent) { - if (VIR_STRDUP(ret, obj->def->parent) < 0) + if (def->parent) { + if (VIR_STRDUP(ret, def->parent) < 0) goto cleanup; } else { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -384,17 +392,19 @@ int nodeDeviceNumOfCaps(virNodeDevicePtr device) { virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; int ret = -1; if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; + def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceNumOfCapsEnsureACL(device->conn, obj->def) < 0) + if (virNodeDeviceNumOfCapsEnsureACL(device->conn, def) < 0) goto cleanup; - for (caps = obj->def->caps; caps; caps = caps->next) { + for (caps = def->caps; caps; caps = caps->next) { ++ncaps; if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { @@ -423,17 +433,19 @@ nodeDeviceListCaps(virNodeDevicePtr device, int maxnames) { virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; int ret = -1; if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; + def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceListCapsEnsureACL(device->conn, obj->def) < 0) + if (virNodeDeviceListCapsEnsureACL(device->conn, def) < 0) goto cleanup; - for (caps = obj->def->caps; caps && ncaps < maxnames; caps = caps->next) { + for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) { if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0) goto cleanup; @@ -595,20 +607,20 @@ nodeDeviceDestroy(virNodeDevicePtr device) if (!(obj = nodeDeviceObjFindByName(device->name))) return -1; + def = virNodeDeviceObjGetDef(obj); nodeDeviceLock(); - if (virNodeDeviceDestroyEnsureACL(device->conn, obj->def) < 0) + if (virNodeDeviceDestroyEnsureACL(device->conn, def) < 0) goto cleanup; - if (virNodeDeviceGetWWNs(obj->def, &wwnn, &wwpn) < 0) + if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) goto cleanup; /* virNodeDeviceGetParentHost will cause the device object's lock * 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. */ - def = obj->def; virNodeDeviceObjUnlock(obj); obj = NULL; if ((parent_host = virNodeDeviceObjGetParentHost(&driver->devs, def, diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 11b9aab..17cd130 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -459,6 +459,7 @@ dev_create(const char *udi) char *parent_key = NULL; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def = NULL; + virNodeDeviceDefPtr objdef; const char *name = hal_name(udi); int rv; char *privData; @@ -493,15 +494,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); - obj = virNodeDeviceObjAssignDef(&driver->devs, def); - if (!obj) { + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) { VIR_FREE(devicePath); goto failure; } + objdef = virNodeDeviceObjGetDef(obj); obj->privateData = privData; obj->privateFree = free_udi; - obj->def->sysfs_path = devicePath; + objdef->sysfs_path = devicePath; virNodeDeviceObjUnlock(obj); @@ -571,13 +572,15 @@ device_cap_added(LibHalContext *ctx, { const char *name = hal_name(udi); virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; nodeDeviceLock(); obj = virNodeDeviceObjFindByName(&driver->devs, name); nodeDeviceUnlock(); VIR_DEBUG("%s %s", cap, name); if (obj) { - (void)gather_capability(ctx, udi, cap, &obj->def->caps); + def = virNodeDeviceObjGetDef(obj); + (void)gather_capability(ctx, udi, cap, &def->caps); 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 6fd8896..65ad10c 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1277,32 +1277,29 @@ static int udevRemoveOneDevice(struct udev_device *device) { virNodeDeviceObjPtr obj = NULL; + virNodeDeviceDefPtr def; virObjectEventPtr event = NULL; const char *name = NULL; - int ret = -1; name = udev_device_get_syspath(device); - obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, name); - - if (!obj) { + if (!(obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, name))) { VIR_DEBUG("Failed to find device to remove that has udev name '%s'", name); - goto cleanup; + return -1; } + def = virNodeDeviceObjGetDef(obj); - event = virNodeDeviceEventLifecycleNew(obj->def->name, + event = virNodeDeviceEventLifecycleNew(def->name, VIR_NODE_DEVICE_EVENT_DELETED, 0); VIR_DEBUG("Removing device '%s' with sysfs path '%s'", - obj->def->name, name); + def->name, name); virNodeDeviceObjRemove(&driver->devs, &obj); - ret = 0; - cleanup: if (event) virObjectEventStateQueue(driver->nodeDeviceEventState, event); - return ret; + return 0; } @@ -1313,6 +1310,7 @@ udevSetParent(struct udev_device *device, struct udev_device *parent_device = NULL; const char *parent_sysfs_path = NULL; virNodeDeviceObjPtr obj = NULL; + virNodeDeviceDefPtr objdef; int ret = -1; parent_device = device; @@ -1330,10 +1328,10 @@ udevSetParent(struct udev_device *device, goto cleanup; } - obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, - parent_sysfs_path); - if (obj != NULL) { - if (VIR_STRDUP(def->parent, obj->def->name) < 0) { + if ((obj = virNodeDeviceObjFindBySysfsPath(&driver->devs, + parent_sysfs_path))) { + objdef = virNodeDeviceObjGetDef(obj); + if (VIR_STRDUP(objdef->parent, def->name) < 0) { virNodeDeviceObjUnlock(obj); goto cleanup; } @@ -1360,6 +1358,7 @@ udevAddOneDevice(struct udev_device *device) { virNodeDeviceDefPtr def = NULL; virNodeDeviceObjPtr obj = NULL; + virNodeDeviceDefPtr objdef; virObjectEventPtr event = NULL; bool new_device = true; int ret = -1; @@ -1396,16 +1395,16 @@ udevAddOneDevice(struct udev_device *device) /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - obj = virNodeDeviceObjAssignDef(&driver->devs, def); - if (obj == NULL) + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) goto cleanup; + objdef = virNodeDeviceObjGetDef(obj); if (new_device) - event = virNodeDeviceEventLifecycleNew(obj->def->name, + event = virNodeDeviceEventLifecycleNew(objdef->name, VIR_NODE_DEVICE_EVENT_CREATED, 0); else - event = virNodeDeviceEventUpdateNew(obj->def->name); + event = virNodeDeviceEventUpdateNew(objdef->name); virNodeDeviceObjUnlock(obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 3389edd..206fdf9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5319,13 +5319,15 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name) { testDriverPtr driver = conn->privateData; virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; virNodeDevicePtr ret = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, name))) return NULL; + def = virNodeDeviceObjGetDef(obj); if ((ret = virGetNodeDevice(conn, name))) { - if (VIR_STRDUP(ret->parent, obj->def->parent) < 0) + if (VIR_STRDUP(ret->parent, def->parent) < 0) virObjectUnref(ret); } @@ -5346,7 +5348,7 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev, if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) return NULL; - ret = virNodeDeviceDefFormat(obj->def); + ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj)); virNodeDeviceObjUnlock(obj); return ret; @@ -5357,13 +5359,15 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) { testDriverPtr driver = dev->conn->privateData; virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; char *ret = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) return NULL; + def = virNodeDeviceObjGetDef(obj); - if (obj->def->parent) { - ignore_value(VIR_STRDUP(ret, obj->def->parent)); + if (def->parent) { + ignore_value(VIR_STRDUP(ret, def->parent)); } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no parent for this device")); @@ -5379,13 +5383,15 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev) { testDriverPtr driver = dev->conn->privateData; virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) return -1; + def = virNodeDeviceObjGetDef(obj); - for (caps = obj->def->caps; caps; caps = caps->next) + for (caps = def->caps; caps; caps = caps->next) ++ncaps; virNodeDeviceObjUnlock(obj); @@ -5398,13 +5404,15 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) { testDriverPtr driver = dev->conn->privateData; virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) return -1; + def = virNodeDeviceObjGetDef(obj); - for (caps = obj->def->caps; caps && ncaps < maxnames; caps = caps->next) { + for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) { if (VIR_STRDUP(names[ncaps], virNodeDevCapTypeToString(caps->data.type)) < 0) goto error; @@ -5431,6 +5439,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, virNodeDeviceDefPtr def = NULL; virNodeDevCapsDefPtr caps; virNodeDeviceObjPtr obj = NULL, objcopy = NULL; + virNodeDeviceDefPtr objdef; virObjectEventPtr event = NULL; /* In the real code, we'd call virVHBAManageVport which would take the @@ -5445,7 +5454,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, if (!(objcopy = virNodeDeviceObjFindByName(&driver->devs, "scsi_host11"))) goto cleanup; - xml = virNodeDeviceDefFormat(objcopy->def); + xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy)); virNodeDeviceObjUnlock(objcopy); if (!xml) goto cleanup; @@ -5485,8 +5494,9 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) goto cleanup; def = NULL; + objdef = virNodeDeviceObjGetDef(obj); - event = virNodeDeviceEventLifecycleNew(obj->def->name, + event = virNodeDeviceEventLifecycleNew(objdef->name, VIR_NODE_DEVICE_EVENT_CREATED, 0); testObjectEventQueue(driver, event); @@ -5508,6 +5518,7 @@ testNodeDeviceCreateXML(virConnectPtr conn, char *wwnn = NULL, *wwpn = NULL; virNodeDevicePtr dev = NULL, ret = NULL; virNodeDeviceObjPtr obj = NULL; + virNodeDeviceDefPtr objdef; virCheckFlags(0, NULL); @@ -5535,8 +5546,9 @@ testNodeDeviceCreateXML(virConnectPtr conn, * work properly, we need to drop our lock */ if (!(obj = testNodeDeviceMockCreateVport(driver, wwnn, wwpn))) goto cleanup; + objdef = virNodeDeviceObjGetDef(obj); - if (!(dev = virGetNodeDevice(conn, obj->def->name))) + if (!(dev = virGetNodeDevice(conn, objdef->name))) goto cleanup; VIR_FREE(dev->parent); @@ -5563,16 +5575,18 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) int ret = 0; testDriverPtr driver = dev->conn->privateData; virNodeDeviceObjPtr obj = NULL; + virNodeDeviceDefPtr def; char *parent_name = NULL, *wwnn = NULL, *wwpn = NULL; virObjectEventPtr event = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) return -1; + def = virNodeDeviceObjGetDef(obj); - if (virNodeDeviceGetWWNs(obj->def, &wwnn, &wwpn) == -1) + if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup; - if (VIR_STRDUP(parent_name, obj->def->parent) < 0) + if (VIR_STRDUP(parent_name, def->parent) < 0) goto cleanup; /* virNodeDeviceGetParentHost will cause the device object's lock to be @@ -5583,7 +5597,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, obj->def, + if (virNodeDeviceObjGetParentHost(&driver->devs, def, EXISTING_DEVICE) < 0) { obj = NULL; goto cleanup; -- 2.9.3
It was only ever used in node_device_hal.c which really never used it anyway since the NODE_DEV_UDI was never referenced. Remove free_udi() and @privData as well as the references to obj->privateData & obj->privateFree. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.h | 2 -- src/conf/virnodedeviceobj.c | 2 -- src/node_device/node_device_hal.c | 15 --------------- 3 files changed, 19 deletions(-) diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 5743f9d..0ab2b96 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -318,8 +318,6 @@ struct _virNodeDeviceObj { virMutex lock; virNodeDeviceDefPtr def; /* device definition */ - void *privateData; /* driver-specific private data */ - void (*privateFree)(void *data); /* destructor for private data */ }; diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index dcf871c..86a552c 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -226,8 +226,6 @@ virNodeDeviceObjFree(virNodeDeviceObjPtr obj) return; virNodeDeviceDefFree(obj->def); - if (obj->privateFree) - (*obj->privateFree)(obj->privateData); virMutexDestroy(&obj->lock); diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 17cd130..e519b12 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -52,8 +52,6 @@ VIR_LOG_INIT("node_device.node_device_hal"); #define DRV_STATE_HAL_CTX(ds) ((LibHalContext *)((ds)->privateData)) -#define NODE_DEV_UDI(obj) ((const char *)((obj)->privateData) - static const char * hal_name(const char *udi) @@ -447,12 +445,6 @@ gather_capabilities(LibHalContext *ctx, const char *udi, } static void -free_udi(void *udi) -{ - VIR_FREE(udi); -} - -static void dev_create(const char *udi) { LibHalContext *ctx; @@ -462,12 +454,8 @@ dev_create(const char *udi) virNodeDeviceDefPtr objdef; const char *name = hal_name(udi); int rv; - char *privData; char *devicePath = NULL; - if (VIR_STRDUP(privData, udi) < 0) - return; - nodeDeviceLock(); ctx = DRV_STATE_HAL_CTX(driver); @@ -500,8 +488,6 @@ dev_create(const char *udi) } objdef = virNodeDeviceObjGetDef(obj); - obj->privateData = privData; - obj->privateFree = free_udi; objdef->sysfs_path = devicePath; virNodeDeviceObjUnlock(obj); @@ -512,7 +498,6 @@ dev_create(const char *udi) failure: VIR_DEBUG("FAILED TO ADD dev %s", name); cleanup: - VIR_FREE(privData); virNodeDeviceDefFree(def); nodeDeviceUnlock(); } -- 2.9.3
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 | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 86a552c..55d66f1 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) { @@ -256,26 +277,16 @@ 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) { virNodeDeviceObjUnlock(obj); virNodeDeviceObjFree(obj); return NULL; } - obj->def = def; return obj; - } -- 2.9.3
John Ferlan <jferlan@redhat.com> [2017-05-19, 09:08AM -0400]:
All part of the effort I have to have a common object model. This series is node device test, driver, and virnodedevobj related.
There's also a couple of bug fixes at the beginning of the series from things I have found during this effort.
There's still a few more patches in local branches to make the virNodeDeviceObjListPtr private as well, but those have some merge needs with other patches currently on list elsewhere, so I'll hold onto them for now.
Am I missing something or where does the privatization actually happen? In general, a lot of the cleanups you are performing are beneficial for the readability. My biggest concern is that this will generate a lot of work when backporting patches.
On 05/23/2017 04:26 AM, Bjoern Walk wrote:
John Ferlan <jferlan@redhat.com> [2017-05-19, 09:08AM -0400]:
All part of the effort I have to have a common object model. This series is node device test, driver, and virnodedevobj related.
There's also a couple of bug fixes at the beginning of the series from things I have found during this effort.
There's still a few more patches in local branches to make the virNodeDeviceObjListPtr private as well, but those have some merge needs with other patches currently on list elsewhere, so I'll hold onto them for now.
Am I missing something or where does the privatization actually happen?
Yeah about 5 or so more patches - bad subject for the cover letter then. The "issue" is that the "next" patch in the series deals with virNodeDeviceObjList and that required a test_driver change. Because the patches in my local branch build upon others and changes in other patches have conflicts with a test_driver.c change to _testDriver...
In general, a lot of the cleanups you are performing are beneficial for the readability. My biggest concern is that this will generate a lot of work when backporting patches.
Think I already addressed this in patch 4 response. John
participants (3)
-
Bjoern Walk -
Daniel P. Berrange -
John Ferlan