[libvirt] [PATCH v2 00/14] Work towards making virNodeDeviceObjPtr private
v1: https://www.redhat.com/archives/libvir-list/2017-May/msg00718.html Changes since v1: * Adjusted the title of cover letter to more appropriately match what's being done. * Added new patch 3 to cover issues I've noted in recent code reviews where additions to virNodeDevCapType may not be properly 'covered' in the virNodeDeviceObjHasCap and virNodeDeviceCapMatch helpers. With the switch, it'll be forced. * Removed former patch 4 - I'll deal with it later. * Added patch 13 * Patch 14 is the old patch 13o There's another 8 or so patches waiting to go, but the "next" one in the series depends on other things currently on list waiting for review. John Ferlan (14): 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: Use switch for virNodeDeviceObjHasCap and virNodeDeviceCapMatch 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: Pass @def by reference to create/assign object nodedev: Introduce virNodeDeviceObjNew src/conf/node_device_conf.h | 2 - src/conf/virnodedeviceobj.c | 252 ++++++++++++++++----------- src/conf/virnodedeviceobj.h | 4 +- src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 233 ++++++++++++------------- src/node_device/node_device_driver.h | 93 +++++++--- src/node_device/node_device_hal.c | 56 +++--- src/node_device/node_device_udev.c | 321 ++++++++++++++++++++--------------- src/test/test_driver.c | 118 +++++++------ 9 files changed, 609 insertions(+), 471 deletions(-) -- 2.9.4
- 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.4
On Thu, May 25, 2017 at 15:56:58 -0400, John Ferlan wrote:
- 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);
A static analyzer may argue that 'obj' may be non-null in some cases at this point ...
- ret = 0; - - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj);
So this would not unlock it.
testObjectEventQueue(privconn, event); - return ret; + return 0; }
ACK to the rest
On 05/26/2017 03:05 AM, Peter Krempa wrote:
On Thu, May 25, 2017 at 15:56:58 -0400, John Ferlan wrote:
- 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);
A static analyzer may argue that 'obj' may be non-null in some cases at this point ...
Not the one I've used... It seems it was smart enough to realize that virNodeDeviceObjRemove will unlock *obj and then lock/unlock *obj each time through it's loop and then set it to NULL if it matches something on list. So either we return with obj=NULL or an unlocked obj
- ret = 0; - - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj);
So this would not unlock it.
Or did I miss something more subtle? When originally wrote the code I have to assume now I was paying less attention to what got called. Tks - John
testObjectEventQueue(privconn, event); - return ret; + return 0; }
ACK to the rest
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.4
On Thu, May 25, 2017 at 15:56:59 -0400, John Ferlan wrote:
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.
Since you are comparing only the pointer of the item locking it is really not necessary. Any other justification for this change?
On 05/26/2017 02:57 AM, Peter Krempa wrote:
On Thu, May 25, 2017 at 15:56:59 -0400, John Ferlan wrote:
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.
Since you are comparing only the pointer of the item locking it is really not necessary.
Any other justification for this change?
Beyond making this like other vir.*ObjRemove APIs, no and fixing what would be an "obvious" coding error. I suppose the argument is that the driver/test lock wouldn't allow change to the list so true technically it doesn't matter. I can drop this if that's what you'd prefer. Eventually it goes away completely, but that's code I have in my branches. John
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.4
On Thu, May 25, 2017 at 15:57:00 -0400, John Ferlan wrote:
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(-)
ACK
In order to ensure that whenever something is added to virNodeDevCapType that both functions are considered for processing of a new capability, change the if-then-else construct into a switch statement. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 80 +++++++++++++++++++++++++++++++++------------ 1 file changed, 60 insertions(+), 20 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index bbb6eeb..913cdda 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -48,19 +48,41 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, while (caps) { if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) { return 1; - } else if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - if ((STREQ(cap, fc_host_cap) && - (caps->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || - (STREQ(cap, vports_cap) && - (caps->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))) - return 1; - } else if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - if ((STREQ(cap, mdev_types)) && - (caps->data.pci_dev.flags & - VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)) - return 1; + } else { + switch (caps->data.type) { + case VIR_NODE_DEV_CAP_PCI_DEV: + if ((STREQ(cap, mdev_types)) && + (caps->data.pci_dev.flags & + VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)) + return 1; + break; + + case VIR_NODE_DEV_CAP_SCSI_HOST: + if ((STREQ(cap, fc_host_cap) && + (caps->data.scsi_host.flags & + VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || + (STREQ(cap, vports_cap) && + (caps->data.scsi_host.flags & + VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))) + return 1; + break; + + case VIR_NODE_DEV_CAP_SYSTEM: + case VIR_NODE_DEV_CAP_USB_DEV: + case VIR_NODE_DEV_CAP_USB_INTERFACE: + case VIR_NODE_DEV_CAP_NET: + case VIR_NODE_DEV_CAP_SCSI_TARGET: + case VIR_NODE_DEV_CAP_SCSI: + case VIR_NODE_DEV_CAP_STORAGE: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_DRM: + case VIR_NODE_DEV_CAP_MDEV_TYPES: + case VIR_NODE_DEV_CAP_MDEV: + case VIR_NODE_DEV_CAP_LAST: + break; + } } caps = caps->next; @@ -468,7 +490,15 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr devobj, if (type == cap->data.type) return true; - if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { + switch (cap->data.type) { + case VIR_NODE_DEV_CAP_PCI_DEV: + if (type == VIR_NODE_DEV_CAP_MDEV_TYPES && + (cap->data.pci_dev.flags & + VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)) + return true; + break; + + case VIR_NODE_DEV_CAP_SCSI_HOST: if (type == VIR_NODE_DEV_CAP_FC_HOST && (cap->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) @@ -478,13 +508,23 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr devobj, (cap->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS)) return true; - } + break; - if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - if (type == VIR_NODE_DEV_CAP_MDEV_TYPES && - (cap->data.pci_dev.flags & - VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)) - return true; + case VIR_NODE_DEV_CAP_SYSTEM: + case VIR_NODE_DEV_CAP_USB_DEV: + case VIR_NODE_DEV_CAP_USB_INTERFACE: + case VIR_NODE_DEV_CAP_NET: + case VIR_NODE_DEV_CAP_SCSI_TARGET: + case VIR_NODE_DEV_CAP_SCSI: + case VIR_NODE_DEV_CAP_STORAGE: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_DRM: + case VIR_NODE_DEV_CAP_MDEV_TYPES: + case VIR_NODE_DEV_CAP_MDEV: + case VIR_NODE_DEV_CAP_LAST: + break; } } -- 2.9.4
On Thu, May 25, 2017 at 15:57:01 -0400, John Ferlan wrote:
In order to ensure that whenever something is added to virNodeDevCapType that both functions are considered for processing of a new capability, change the if-then-else construct into a switch statement.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 80 +++++++++++++++++++++++++++++++++------------ 1 file changed, 60 insertions(+), 20 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index bbb6eeb..913cdda 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -48,19 +48,41 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, while (caps) { if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) { return 1; - } else if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - if ((STREQ(cap, fc_host_cap) && - (caps->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || - (STREQ(cap, vports_cap) && - (caps->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))) - return 1; - } else if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - if ((STREQ(cap, mdev_types)) && - (caps->data.pci_dev.flags & - VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)) - return 1; + } else { + switch (caps->data.type) { + case VIR_NODE_DEV_CAP_PCI_DEV: + if ((STREQ(cap, mdev_types)) && + (caps->data.pci_dev.flags & + VIR_NODE_DEV_CAP_FLAG_PCI_MDEV))
Since you are touching this, put this on a single line. It looks very ugly this way. It also fits into the 80 col boundary, so I don't see a reaosn for this. ACK with this adjustment applied to the whole patch.
On 05/26/2017 03:14 AM, Peter Krempa wrote:
On Thu, May 25, 2017 at 15:57:01 -0400, John Ferlan wrote:
In order to ensure that whenever something is added to virNodeDevCapType that both functions are considered for processing of a new capability, change the if-then-else construct into a switch statement.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 80 +++++++++++++++++++++++++++++++++------------ 1 file changed, 60 insertions(+), 20 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index bbb6eeb..913cdda 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -48,19 +48,41 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, while (caps) { if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) { return 1; - } else if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - if ((STREQ(cap, fc_host_cap) && - (caps->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || - (STREQ(cap, vports_cap) && - (caps->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))) - return 1; - } else if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - if ((STREQ(cap, mdev_types)) && - (caps->data.pci_dev.flags & - VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)) - return 1; + } else { + switch (caps->data.type) { + case VIR_NODE_DEV_CAP_PCI_DEV: + if ((STREQ(cap, mdev_types)) && + (caps->data.pci_dev.flags & + VIR_NODE_DEV_CAP_FLAG_PCI_MDEV))
Since you are touching this, put this on a single line. It looks very ugly this way. It also fits into the 80 col boundary, so I don't see a reaosn for this.
For MDEV - it can fit, for SCSI_HOST, not as clean, but it could be: if ((STREQ(cap, fc_host_cap) && (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || (STREQ(cap, vports_cap) && (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))) return 1; Although I'm not sure I like the way that looks. So, as an option I could also: ... virNodeDevCapSCSIHostPtr scsi_host; ... and case VIR_NODE_DEV_CAP_SCSI_HOST: scsi_host = &caps->data.scsi_host; if ((STREQ(cap, fc_host_cap) && (scsi_host->flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || (STREQ(cap, vports_cap) && (scsi_host->flags & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))) return 1; and case VIR_NODE_DEV_CAP_SCSI_HOST: scsi_host = &cap->data.scsi_host; if (type == VIR_NODE_DEV_CAP_FC_HOST && (scsi_host->flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) return true; if (type == VIR_NODE_DEV_CAP_VPORTS && (scsi_host->flags & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS)) return true; But does that "violate" the too many changes at once "guideline"? John
ACK with this adjustment applied to the whole patch.
On Fri, May 26, 2017 at 08:22:26 -0400, John Ferlan wrote:
On 05/26/2017 03:14 AM, Peter Krempa wrote:
On Thu, May 25, 2017 at 15:57:01 -0400, John Ferlan wrote:
In order to ensure that whenever something is added to virNodeDevCapType that both functions are considered for processing of a new capability, change the if-then-else construct into a switch statement.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 80 +++++++++++++++++++++++++++++++++------------ 1 file changed, 60 insertions(+), 20 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index bbb6eeb..913cdda 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -48,19 +48,41 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, while (caps) { if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) { return 1; - } else if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - if ((STREQ(cap, fc_host_cap) && - (caps->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || - (STREQ(cap, vports_cap) && - (caps->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))) - return 1; - } else if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - if ((STREQ(cap, mdev_types)) && - (caps->data.pci_dev.flags & - VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)) - return 1; + } else { + switch (caps->data.type) { + case VIR_NODE_DEV_CAP_PCI_DEV: + if ((STREQ(cap, mdev_types)) && + (caps->data.pci_dev.flags & + VIR_NODE_DEV_CAP_FLAG_PCI_MDEV))
Since you are touching this, put this on a single line. It looks very ugly this way. It also fits into the 80 col boundary, so I don't see a reaosn for this.
For MDEV - it can fit, for SCSI_HOST, not as clean, but it could be:
if ((STREQ(cap, fc_host_cap) && (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || (STREQ(cap, vports_cap) && (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS)))
That is WAY worse. The binary mask should be on a single line since it's semantically connected. Also the 80 col rule is not really strict. Especially if it hinders readability of the code. The above suggestion is a very good example where you'd completely destroy readability.
return 1;
Although I'm not sure I like the way that looks.
[...]
But does that "violate" the too many changes at once "guideline"?
If you feel so, leave it as-is.
On 05/26/2017 08:36 AM, Peter Krempa wrote:
On Fri, May 26, 2017 at 08:22:26 -0400, John Ferlan wrote:
On 05/26/2017 03:14 AM, Peter Krempa wrote:
On Thu, May 25, 2017 at 15:57:01 -0400, John Ferlan wrote:
In order to ensure that whenever something is added to virNodeDevCapType that both functions are considered for processing of a new capability, change the if-then-else construct into a switch statement.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 80 +++++++++++++++++++++++++++++++++------------ 1 file changed, 60 insertions(+), 20 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index bbb6eeb..913cdda 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -48,19 +48,41 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev, while (caps) { if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) { return 1; - } else if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - if ((STREQ(cap, fc_host_cap) && - (caps->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || - (STREQ(cap, vports_cap) && - (caps->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))) - return 1; - } else if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - if ((STREQ(cap, mdev_types)) && - (caps->data.pci_dev.flags & - VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)) - return 1; + } else { + switch (caps->data.type) { + case VIR_NODE_DEV_CAP_PCI_DEV: + if ((STREQ(cap, mdev_types)) && + (caps->data.pci_dev.flags & + VIR_NODE_DEV_CAP_FLAG_PCI_MDEV))
Since you are touching this, put this on a single line. It looks very ugly this way. It also fits into the 80 col boundary, so I don't see a reaosn for this.
For MDEV - it can fit, for SCSI_HOST, not as clean, but it could be:
if ((STREQ(cap, fc_host_cap) && (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || (STREQ(cap, vports_cap) && (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS)))
That is WAY worse. The binary mask should be on a single line since it's semantically connected.
I don't disagree! I wouldn't take that route, but it keeps everything inside 80 cols...
Also the 80 col rule is not really strict. Especially if it hinders readability of the code. The above suggestion is a very good example where you'd completely destroy readability.
I'll just go beyond the 80 cols. I personally don't like that, but that's just a personal preference thing and readability wise it's I think better than the multiline condition just because of 80 cols. Tks - John
return 1;
Although I'm not sure I like the way that looks.
[...]
But does that "violate" the too many changes at once "guideline"?
If you feel so, leave it as-is.
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 913cdda..a2d09ad 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 = @@ -100,9 +100,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 && @@ -124,9 +124,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 && @@ -235,18 +235,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); } @@ -265,51 +265,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; @@ -332,15 +332,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; } @@ -353,19 +353,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; } @@ -377,19 +377,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; } @@ -400,19 +400,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; } @@ -421,19 +421,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; } @@ -481,12 +481,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; @@ -589,9 +589,9 @@ virNodeDeviceObjGetNames(virNodeDeviceObjListPtr devs, #define MATCH(FLAG) ((flags & (VIR_CONNECT_LIST_NODE_DEVICES_CAP_ ## FLAG)) && \ - virNodeDeviceCapMatch(devobj, VIR_NODE_DEV_CAP_ ## FLAG)) + virNodeDeviceCapMatch(obj, VIR_NODE_DEV_CAP_ ## FLAG)) static bool -virNodeDeviceMatch(virNodeDeviceObjPtr devobj, +virNodeDeviceMatch(virNodeDeviceObjPtr obj, unsigned int flags) { /* filter by cap type */ @@ -621,7 +621,7 @@ virNodeDeviceMatch(virNodeDeviceObjPtr devobj, int virNodeDeviceObjListExport(virConnectPtr conn, - virNodeDeviceObjListPtr devobjs, + virNodeDeviceObjListPtr devs, virNodeDevicePtr **devices, virNodeDeviceObjListFilter filter, unsigned int flags) @@ -632,26 +632,26 @@ virNodeDeviceObjListExport(virConnectPtr conn, int ret = -1; size_t i; - if (devices && VIR_ALLOC_N(tmp_devices, devobjs->count + 1) < 0) + if (devices && VIR_ALLOC_N(tmp_devices, devs->count + 1) < 0) goto cleanup; - for (i = 0; i < devobjs->count; i++) { - virNodeDeviceObjPtr devobj = devobjs->objs[i]; - virNodeDeviceObjLock(devobj); - if ((!filter || filter(conn, devobj->def)) && - virNodeDeviceMatch(devobj, flags)) { + for (i = 0; i < devs->count; i++) { + virNodeDeviceObjPtr obj = devs->objs[i]; + virNodeDeviceObjLock(obj); + if ((!filter || filter(conn, obj->def)) && + virNodeDeviceMatch(obj, flags)) { if (devices) { - if (!(device = virGetNodeDevice(conn, devobj->def->name)) || - VIR_STRDUP(device->parent, devobj->def->parent) < 0) { + if (!(device = virGetNodeDevice(conn, obj->def->name)) || + VIR_STRDUP(device->parent, obj->def->parent) < 0) { virObjectUnref(device); - virNodeDeviceObjUnlock(devobj); + virNodeDeviceObjUnlock(obj); goto cleanup; } tmp_devices[ndevices] = device; } ndevices++; } - virNodeDeviceObjUnlock(devobj); + virNodeDeviceObjUnlock(obj); } if (tmp_devices) { -- 2.9.4
On Thu, May 25, 2017 at 15:57:02 -0400, John Ferlan wrote:
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 913cdda..a2d09ad 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,
I'm not a fan of this. Adds line churn and it's not really worth it.
On 05/26/2017 03:15 AM, Peter Krempa wrote:
On Thu, May 25, 2017 at 15:57:02 -0400, John Ferlan wrote:
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 913cdda..a2d09ad 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,
I'm not a fan of this. Adds line churn and it's not really worth it.
Undo-ing would alter so many patches going forward which causes such a ripple effect w/r/t the process I've been using for all these patches and a "common" look and feel. I get the churn and back port thing - really, but I was asked to undo the single monolithic patch and this is what happens - (too) many smaller patches each that can be debated and affect the final result. So, is it technically wrong to rename or is it just not being a fan? Just because I'm not a fan of something someone else has done, doesn't make it wrong. While one could focus on a module by module approach, for me it's been a bit more global. In some code the vir<driver>Ptr is referenced as an @obj while other times it's referenced as @<drv>. In the same module there's vir<driver>ObjPtr's that are sometimes referenced as @obj and other times as @<drv>. (where <drv> is some shorthand, dev, iface, net, filter, secret, pool, vol, etc.). In this module an @device is a virNodeDeviceObjPtr in virNodeDeviceObjAssignDef and a virNodeDevicePtr in virNodeDeviceObjListExport. So without going back to the argument definition how does one really know what it is. If one creates a common way to describe arguments, then one doesn't have to think about it. So that's been my approach.
From my perspective of trying to create this "common" look and feel to arguments across all these various modules and to help me keep some amount of sanity, I've tried to think of vir<driver>ObjPtr as @obj and vir<driver>Ptr as @drv.
Of course there's also the vir<driver>ObjListPtr's that as was pointed out could be changed into @objs (following the same logic); however, for me that was too close to @obj, so I left those as devs, nets, nwfilters, pools, ifaces, etc. (in general). 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 ba3da62..2a461fb 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 4ecb0b1..481358a 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.4
On Thu, May 25, 2017 at 15:57:03 -0400, John Ferlan wrote:
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.
Still, this patch is mixing function renames with whitespace changes.
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 ba3da62..2a461fb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c
@@ -151,11 +155,14 @@ void nodeDeviceLock(void) { virMutexLock(&driver->lock); } + + void nodeDeviceUnlock(void) { virMutexUnlock(&driver->lock); }
This one should be fixed too. [...]
@@ -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)
Why didn't you change name of this function, since you changed the one above? [...]
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 @@
[...]
+void +nodeDeviceLock(void); + +void +nodeDeviceUnlock(void); + +extern +virNodeDeviceDriverStatePtr driver;
This is ugly. It's also not a function and 'extern' keyword is not the return type. [...] ACK if you fix nodeDeviceUnlock too. Also please don't mix the header whitespace update with function renaming in future patches.
On 05/26/2017 04:13 AM, Peter Krempa wrote:
On Thu, May 25, 2017 at 15:57:03 -0400, John Ferlan wrote:
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.
Still, this patch is mixing function renames with whitespace changes.
So add "n" more patches - one to deal with whitespace... one to deal with function renames... one to deal with argument placement... I just tried to lump the formatting stuff into one - trying to avoid too much minutiae. The node_device* code is I would say the oldest and least touched so it gets the most adjustment - too much.
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 ba3da62..2a461fb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c
@@ -151,11 +155,14 @@ void nodeDeviceLock(void) { virMutexLock(&driver->lock); } + + void nodeDeviceUnlock(void) { virMutexUnlock(&driver->lock); }
This one should be fixed too.
[...]
Oh right...
@@ -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)
Why didn't you change name of this function, since you changed the one above?
Hmm... not sure why. Strange - oh I see it already had the two space between functions so I skipped right past it. it's changed to nodeDeviceFindNewDevice in my branch now.
[...]
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 @@
[...]
+void +nodeDeviceLock(void); + +void +nodeDeviceUnlock(void); + +extern +virNodeDeviceDriverStatePtr driver;
This is ugly. It's also not a function and 'extern' keyword is not the return type.
Oh right - this got to be such a rote exercise that I overdid it. I restored it back to one line.
[...]
ACK if you fix nodeDeviceUnlock too. Also please don't mix the header whitespace update with function renaming in future patches.
Well the horse has already left the barn on that front for some on list patches... But I can go back through those patches that haven't been posted yet and create singular change. Tks - John
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 2a461fb..87953f3 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.4
On Thu, May 25, 2017 at 15:57:04 -0400, John Ferlan wrote:
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.
Again two semanticaly different changes in single commit.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
ACK
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 87953f3..b599460 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: - nodeDeviceSysfsGetSCSIHostCaps(&dev->def->caps->data.scsi_host); + nodeDeviceSysfsGetSCSIHostCaps(&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 (nodeDeviceSysfsGetPCIRelatedDevCaps(dev->def->sysfs_path, - &dev->def->caps->data.pci_dev) < 0) + if (nodeDeviceSysfsGetPCIRelatedDevCaps(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.4
On Thu, May 25, 2017 at 15:57:05 -0400, John Ferlan wrote:
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(-)
ACK
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 b599460..4b8a66e 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.4
On Thu, May 25, 2017 at 15:57:06 -0400, John Ferlan wrote:
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(-)
ACK
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 4b8a66e..d34f399 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 81e5ecc..ff6200b 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 481358a..42d3e73 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.4
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 a2d09ad..1b9d032 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 d361454..222e3b2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -955,6 +955,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 d34f399..93aa2af 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 ff6200b..cde1c7b 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 42d3e73..83a8fcc 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.4
On Thu, May 25, 2017 at 15:57:08 -0400, John Ferlan wrote:
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(-)
ACK
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 1b9d032..a7e51ef 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -248,8 +248,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 cde1c7b..e9031ea 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.4
On Thu, May 25, 2017 at 15:57:09 -0400, John Ferlan wrote:
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(-)
ACK
Since the @def is consumed by the assignment function, let's pass by reference instead of value and really consume it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 8 ++++---- src/conf/virnodedeviceobj.h | 2 +- src/node_device/node_device_hal.c | 2 +- src/node_device/node_device_udev.c | 8 +++----- src/test/test_driver.c | 5 ++--- 5 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index a7e51ef..1648b33 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -268,13 +268,13 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) virNodeDeviceObjPtr virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def) + virNodeDeviceDefPtr *def) { virNodeDeviceObjPtr obj; - if ((obj = virNodeDeviceObjFindByName(devs, def->name))) { + if ((obj = virNodeDeviceObjFindByName(devs, (*def)->name))) { virNodeDeviceDefFree(obj->def); - obj->def = def; + VIR_STEAL_PTR(obj->def, *def); return obj; } @@ -294,7 +294,7 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceObjFree(obj); return NULL; } - obj->def = def; + VIR_STEAL_PTR(obj->def, *def); return obj; diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 135a424..49c28e7 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -53,7 +53,7 @@ virNodeDeviceObjFindBySysfsPath(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def); + virNodeDeviceDefPtr *def); void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index e9031ea..2d996a9 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -482,7 +482,7 @@ dev_create(const char *udi) /* Some devices don't have a path in sysfs, so ignore failure */ (void)get_str_prop(ctx, udi, "linux.sysfs_path", &devicePath); - if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) { + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, &def))) { VIR_FREE(devicePath); goto failure; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 83a8fcc..0250aab 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1395,7 +1395,7 @@ 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. */ - if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, &def))) goto cleanup; objdef = virNodeDeviceObjGetDef(obj); @@ -1685,8 +1685,7 @@ udevSetupSystemDev(void) udevGetDMIData(&def->caps->data.system); #endif - obj = virNodeDeviceObjAssignDef(&driver->devs, def); - if (obj == NULL) + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, &def))) goto cleanup; virNodeDeviceObjUnlock(obj); @@ -1694,8 +1693,7 @@ udevSetupSystemDev(void) ret = 0; cleanup: - if (ret == -1) - virNodeDeviceDefFree(def); + virNodeDeviceDefFree(def); return ret; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 206fdf9..84ff1de 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1168,7 +1168,7 @@ testParseNodedevs(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virNodeDeviceObjAssignDef(&privconn->devs, def))) { + if (!(obj = virNodeDeviceObjAssignDef(&privconn->devs, &def))) { virNodeDeviceDefFree(def); goto error; } @@ -5491,9 +5491,8 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, caps = caps->next; } - if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, def))) + if (!(obj = virNodeDeviceObjAssignDef(&driver->devs, &def))) goto cleanup; - def = NULL; objdef = virNodeDeviceObjGetDef(obj); event = virNodeDeviceEventLifecycleNew(objdef->name, -- 2.9.4
On Thu, May 25, 2017 at 15:57:10 -0400, John Ferlan wrote:
Since the @def is consumed by the assignment function, let's pass by reference instead of value and really consume it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 8 ++++---- src/conf/virnodedeviceobj.h | 2 +- src/node_device/node_device_hal.c | 2 +- src/node_device/node_device_udev.c | 8 +++----- src/test/test_driver.c | 5 ++--- 5 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index a7e51ef..1648b33 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -268,13 +268,13 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs)
virNodeDeviceObjPtr virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def) + virNodeDeviceDefPtr *def)
I don't really like this. You can documment that this function either consumes def always or only on success and the callers can free the pointer. Passing the pointer to a pointer just to clear it in some cases is overcomplicating stuff.
On 05/26/2017 08:12 AM, Peter Krempa wrote:
On Thu, May 25, 2017 at 15:57:10 -0400, John Ferlan wrote:
Since the @def is consumed by the assignment function, let's pass by reference instead of value and really consume it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 8 ++++---- src/conf/virnodedeviceobj.h | 2 +- src/node_device/node_device_hal.c | 2 +- src/node_device/node_device_udev.c | 8 +++----- src/test/test_driver.c | 5 ++--- 5 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index a7e51ef..1648b33 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -268,13 +268,13 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs)
virNodeDeviceObjPtr virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def) + virNodeDeviceDefPtr *def)
I don't really like this. You can documment that this function either consumes def always or only on success and the callers can free the pointer. Passing the pointer to a pointer just to clear it in some cases is overcomplicating stuff.
True, but I ran into in one case (I forget which one now) where the assumption was that the @def wasn't consumed on a failure, but in reality it had been. The obj->def got assigned, then the attempt was made to add the obj to the list which if it failed would call whatever cleanup routine was there which free'd @obj->def and returned to the caller with a failure which would attempt to free @def (again). My first instinct was to just set obj->def = NULL prior to the cleanup, which worked for a short time until obj->def was a "real" object and the cleanup code didn't own obj any more. So it's the long winded way of saying - I can drop this and the following patch either be "more careful" as necessary or just claim at some point in time that @def would be consumed on both success and failure, which I think follows how virJSONValueObjectCreate eats the "a:" arguments (and causes many false positives for Coverity). John
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 1648b33..a84266b 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); + VIR_STEAL_PTR(obj->def, *def); + + return obj; +} + + virNodeDeviceDefPtr virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj) { @@ -278,26 +299,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; } - VIR_STEAL_PTR(obj->def, *def); return obj; - } -- 2.9.4
On Thu, May 25, 2017 at 15:57:11 -0400, John Ferlan wrote:
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 1648b33..a84266b 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)
Same as in previous patch. PAssing pointer to a pointer is really overcomplicating stuff here.
On 05/25/2017 03:56 PM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2017-May/msg00718.html
Changes since v1:
* Adjusted the title of cover letter to more appropriately match what's being done.
* Added new patch 3 to cover issues I've noted in recent code reviews where additions to virNodeDevCapType may not be properly 'covered' in the virNodeDeviceObjHasCap and virNodeDeviceCapMatch helpers. With the switch, it'll be forced.
* Removed former patch 4 - I'll deal with it later.
* Added patch 13
* Patch 14 is the old patch 13o
There's another 8 or so patches waiting to go, but the "next" one in the series depends on other things currently on list waiting for review.
John Ferlan (14): 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: Use switch for virNodeDeviceObjHasCap and virNodeDeviceCapMatch 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: Pass @def by reference to create/assign object nodedev: Introduce virNodeDeviceObjNew
src/conf/node_device_conf.h | 2 - src/conf/virnodedeviceobj.c | 252 ++++++++++++++++----------- src/conf/virnodedeviceobj.h | 4 +- src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 233 ++++++++++++------------- src/node_device/node_device_driver.h | 93 +++++++--- src/node_device/node_device_hal.c | 56 +++--- src/node_device/node_device_udev.c | 321 ++++++++++++++++++++--------------- src/test/test_driver.c | 118 +++++++------ 9 files changed, 609 insertions(+), 471 deletions(-)
Now that 3.4.0 is out... I went back through this series, grabbed the 8 patches that were ACK'd and pushed them (3, 4, 6, 7, 8, 9, 11, & 12). I will post a v3 series shortly that will include "some" of the patches not ACK and will include the rest of the patches to privatize virNodeDeviceObj and virNodeDeviceObjList Tks - John
participants (2)
-
John Ferlan -
Peter Krempa