2:56 p.m.
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
2:56 p.m.
New subject: [libvirt] [PATCH v2 01/14] test: Adjust cleanup/error paths for nodedev test APIs
- 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(a)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
2:05 a.m.
New subject: [libvirt] [PATCH v2 01/14] test: Adjust cleanup/error paths for nodedev test APIs
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(a)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
6:39 a.m.
New subject: [libvirt] [PATCH v2 01/14] test: Adjust cleanup/error paths for nodedev test APIs
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(a)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
2:56 p.m.
New subject: [libvirt] [PATCH v2 02/14] nodedev: Fix locking in virNodeDeviceObjRemove
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(a)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
1:57 a.m.
New subject: [libvirt] [PATCH v2 02/14] nodedev: Fix locking in virNodeDeviceObjRemove
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?
6:52 a.m.
New subject: [libvirt] [PATCH v2 02/14] nodedev: Fix locking in virNodeDeviceObjRemove
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
2:57 p.m.
New subject: [libvirt] [PATCH v2 03/14] nodedev: Need to check for vport capable scsi_host for vHBA searches
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(a)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
2:11 a.m.
New subject: [libvirt] [PATCH v2 03/14] nodedev: Need to check for vport capable scsi_host for vHBA searches
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(a)redhat.com>
---
src/conf/virnodedeviceobj.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
ACK
2:57 p.m.
New subject: [libvirt] [PATCH v2 04/14] nodedev: Use switch for virNodeDeviceObjHasCap and virNodeDeviceCapMatch
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(a)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
2:14 a.m.
New subject: [libvirt] [PATCH v2 04/14] nodedev: Use switch for virNodeDeviceObjHasCap and virNodeDeviceCapMatch
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(a)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.
7:22 a.m.
New subject: [libvirt] [PATCH v2 04/14] nodedev: Use switch for virNodeDeviceObjHasCap and virNodeDeviceCapMatch
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(a)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.
7:36 a.m.
New subject: [libvirt] [PATCH v2 04/14] nodedev: Use switch for virNodeDeviceObjHasCap and virNodeDeviceCapMatch
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(a)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.
8:50 a.m.
New subject: [libvirt] [PATCH v2 04/14] nodedev: Use switch for virNodeDeviceObjHasCap and virNodeDeviceCapMatch
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(a)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.
2:57 p.m.
New subject: [libvirt] [PATCH v2 05/14] nodedev: Use common naming for virnodedeviceobj
A virNodeDeviceObjPtr is an @obj
A virNodeDeviceObjListPtr is an @devs
Signed-off-by: John Ferlan <jferlan(a)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
2:15 a.m.
New subject: [libvirt] [PATCH v2 05/14] nodedev: Use common naming for virnodedeviceobj
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(a)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.
7:46 a.m.
New subject: [libvirt] [PATCH v2 05/14] nodedev: Use common naming for virnodedeviceobj
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(a)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
2:57 p.m.
New subject: [libvirt] [PATCH v2 06/14] nodedev: Cleanup driver code and prototypes
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(a)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
3:13 a.m.
New subject: [libvirt] [PATCH v2 06/14] nodedev: Cleanup driver code and prototypes
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(a)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.
8:04 a.m.
New subject: [libvirt] [PATCH v2 06/14] nodedev: Cleanup driver code and prototypes
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(a)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
2:57 p.m.
New subject: [libvirt] [PATCH v2 07/14] nodedev: Alter param to nodeDeviceUpdateDriverName
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(a)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
3:23 a.m.
New subject: [libvirt] [PATCH v2 07/14] nodedev: Alter param to nodeDeviceUpdateDriverName
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(a)redhat.com>
---
src/node_device/node_device_driver.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
ACK
2:57 p.m.
New subject: [libvirt] [PATCH v2 08/14] nodedev: Alter param to nodeDeviceUpdateCaps
Rather than taking an virNodeDeviceObjPtr and dereffing the obj->def,
just pass the def.
Signed-off-by: John Ferlan <jferlan(a)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
3:24 a.m.
New subject: [libvirt] [PATCH v2 08/14] nodedev: Alter param to nodeDeviceUpdateCaps
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(a)redhat.com>
---
src/node_device/node_device_driver.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
ACK
2:57 p.m.
New subject: [libvirt] [PATCH v2 09/14] nodedev: Create helper for finding by name in driver
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(a)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
6:46 a.m.
New subject: [libvirt] [PATCH v2 09/14] nodedev: Create helper for finding by name in driver
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(a)redhat.com>
---
src/node_device/node_device_driver.c | 96 +++++++++++++-----------------------
1 file changed, 33 insertions(+), 63 deletions(-)
ACK
2:57 p.m.
New subject: [libvirt] [PATCH v2 10/14] nodedev: Use consistent names for driver variables
A virNodeDeviceObjPtr is an @obj
A virNodeDeviceObjListPtr is a @devs
A virNodeDevicePtr is a @device
Signed-off-by: John Ferlan <jferlan(a)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
2:57 p.m.
New subject: [libvirt] [PATCH v2 11/14] nodedev: Introduce virNodeDeviceObjGetDef
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(a)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
7:02 a.m.
New subject: [libvirt] [PATCH v2 11/14] nodedev: Introduce virNodeDeviceObjGetDef
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(a)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
2:57 p.m.
New subject: [libvirt] [PATCH v2 12/14] nodedev: Remove privateData from virNodeDeviceObj
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(a)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
7:11 a.m.
New subject: [libvirt] [PATCH v2 12/14] nodedev: Remove privateData from virNodeDeviceObj
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(a)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
2:57 p.m.
New subject: [libvirt] [PATCH v2 13/14] nodedev: Pass @def by reference to create/assign object
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(a)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
7:12 a.m.
New subject: [libvirt] [PATCH v2 13/14] nodedev: Pass @def by reference to create/assign object
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(a)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.
8:19 a.m.
New subject: [libvirt] [PATCH v2 13/14] nodedev: Pass @def by reference to create/assign object
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(a)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
2:57 p.m.
New subject: [libvirt] [PATCH v2 14/14] nodedev: Introduce virNodeDeviceObjNew
Create an allocator for the virNodeDeviceObjPtr - include setting up
the mutex, saving the virNodeDeviceDefPtr, and locking the return object.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
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
7:13 a.m.
New subject: [libvirt] [PATCH v2 14/14] nodedev: Introduce virNodeDeviceObjNew
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(a)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.
7:59 a.m.
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
2728
days inactive
2737
days old
36 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa