8:54 a.m.
Patches speak for themselves... Convert to RWObjectLockable and then
merge the various ForEach callback functions into one API and structure.
John Ferlan (3):
nodedev: Convert virNodeDeviceObjList to use RWObjectLockable
nodedev: Convert virNodeDeviceObjHasCap to bool
nodedev: Introduce virNodeDeviceObjListForEachCb
src/conf/virnodedeviceobj.c | 237 ++++++++++++++++++--------------------------
1 file changed, 98 insertions(+), 139 deletions(-)
--
2.13.6
8:54 a.m.
New subject: [libvirt] [PATCH 1/3] nodedev: Convert virNodeDeviceObjList to use RWObjectLockable
Let's use the RWObjectLockable for the various list lock mgmt.
Only time need Write lock will be for Add/Remove logic.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virnodedeviceobj.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index b0dcee16f1..49f4f436b8 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -40,7 +40,7 @@ struct _virNodeDeviceObj {
};
struct _virNodeDeviceObjList {
- virObjectLockable parent;
+ virObjectRWLockable parent;
/* name string -> virNodeDeviceObj mapping
* for O(1), lockless lookup-by-name */
@@ -63,7 +63,7 @@ virNodeDeviceObjOnceInit(void)
virNodeDeviceObjDispose)))
return -1;
- if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(),
+ if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectRWLockable(),
"virNodeDeviceObjList",
sizeof(virNodeDeviceObjList),
virNodeDeviceObjListDispose)))
@@ -231,10 +231,10 @@ virNodeDeviceObjListSearch(virNodeDeviceObjListPtr devs,
{
virNodeDeviceObjPtr obj;
- virObjectLock(devs);
+ virObjectRWLockRead(devs);
obj = virHashSearch(devs->objs, callback, data, NULL);
virObjectRef(obj);
- virObjectUnlock(devs);
+ virObjectRWUnlock(devs);
if (obj)
virObjectLock(obj);
@@ -284,9 +284,9 @@ virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs,
{
virNodeDeviceObjPtr obj;
- virObjectLock(devs);
+ virObjectRWLockRead(devs);
obj = virNodeDeviceObjListFindByNameLocked(devs, name);
- virObjectUnlock(devs);
+ virObjectRWUnlock(devs);
if (obj)
virObjectLock(obj);
@@ -462,7 +462,7 @@ virNodeDeviceObjListNew(void)
if (virNodeDeviceObjInitialize() < 0)
return NULL;
- if (!(devs = virObjectLockableNew(virNodeDeviceObjListClass)))
+ if (!(devs = virObjectRWLockableNew(virNodeDeviceObjListClass)))
return NULL;
if (!(devs->objs = virHashCreate(50, virObjectFreeHashData))) {
@@ -487,7 +487,7 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs,
{
virNodeDeviceObjPtr obj;
- virObjectLock(devs);
+ virObjectRWLockWrite(devs);
if ((obj = virNodeDeviceObjListFindByNameLocked(devs, def->name))) {
virObjectLock(obj);
@@ -507,7 +507,7 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs,
}
cleanup:
- virObjectUnlock(devs);
+ virObjectRWUnlock(devs);
return obj;
}
@@ -524,12 +524,12 @@ virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs,
virObjectRef(obj);
virObjectUnlock(obj);
- virObjectLock(devs);
+ virObjectRWLockWrite(devs);
virObjectLock(obj);
virHashRemoveEntry(devs->objs, def->name);
virObjectUnlock(obj);
virObjectUnref(obj);
- virObjectUnlock(devs);
+ virObjectRWUnlock(devs);
}
@@ -767,9 +767,9 @@ virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs,
struct virNodeDeviceCountData data = {
.conn = conn, .filter = filter, .matchstr = cap, .count = 0 };
- virObjectLock(devs);
+ virObjectRWLockRead(devs);
virHashForEach(devs->objs, virNodeDeviceObjListNumOfDevicesCallback, &data);
- virObjectUnlock(devs);
+ virObjectRWUnlock(devs);
return data.count;
}
@@ -828,9 +828,9 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs,
.conn = conn, .filter = filter, .matchstr = cap, .names = names,
.nnames = 0, .maxnames = maxnames, .error = false };
- virObjectLock(devs);
+ virObjectRWLockRead(devs);
virHashForEach(devs->objs, virNodeDeviceObjListGetNamesCallback, &data);
- virObjectUnlock(devs);
+ virObjectRWUnlock(devs);
if (data.error)
goto error;
@@ -932,15 +932,15 @@ virNodeDeviceObjListExport(virConnectPtr conn,
.conn = conn, .filter = filter, .flags = flags,
.devices = NULL, .ndevices = 0, .error = false };
- virObjectLock(devs);
+ virObjectRWLockRead(devs);
if (devices &&
VIR_ALLOC_N(data.devices, virHashSize(devs->objs) + 1) < 0) {
- virObjectUnlock(devs);
+ virObjectRWUnlock(devs);
return -1;
}
virHashForEach(devs->objs, virNodeDeviceObjListExportCallback, &data);
- virObjectUnlock(devs);
+ virObjectRWUnlock(devs);
if (data.error)
goto cleanup;
--
2.13.6
8:54 a.m.
New subject: [libvirt] [PATCH 2/3] nodedev: Convert virNodeDeviceObjHasCap to bool
It only returns 0 or 1 anyway, let's be realistic
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virnodedeviceobj.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 49f4f436b8..7db8fccbc3 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -120,7 +120,7 @@ virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj)
}
-static int
+static bool
virNodeDeviceObjHasCap(const virNodeDeviceObj *obj,
const char *cap)
{
@@ -134,13 +134,13 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj,
while (caps) {
if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) {
- return 1;
+ return true;
} 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;
+ return true;
break;
case VIR_NODE_DEV_CAP_SCSI_HOST:
@@ -148,7 +148,7 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj,
(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;
+ return true;
break;
case VIR_NODE_DEV_CAP_SYSTEM:
@@ -172,7 +172,7 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj,
caps = caps->next;
}
- return 0;
+ return false;
}
--
2.13.6
8:01 a.m.
New subject: [libvirt] [PATCH 2/3] nodedev: Convert virNodeDeviceObjHasCap to bool
On Tue, Oct 10, 2017 at 09:54:27AM -0400, John Ferlan wrote:
It only returns 0 or 1 anyway, let's be realistic
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
For patches 1-2:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
8:54 a.m.
New subject: [libvirt] [PATCH 3/3] nodedev: Introduce virNodeDeviceObjListForEachCb
Rather than separate functions for NumOfDevices, GetNames, and
ListExport - let's converge the code into one handler.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virnodedeviceobj.c | 191 +++++++++++++++++---------------------------
1 file changed, 75 insertions(+), 116 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 7db8fccbc3..c4be03d67d 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -730,29 +730,68 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj,
}
-struct virNodeDeviceCountData {
+typedef bool (*virNodeDeviceObjListHasCap)(const virNodeDeviceObj *obj,
+ const char *cap);
+typedef bool (*virNodeDeviceObjListMatch)(virNodeDeviceObjPtr obj,
+ unsigned int flags);
+struct _virNodeDeviceObjForEachData {
virConnectPtr conn;
virNodeDeviceObjListFilter filter;
- const char *matchstr;
- int count;
+ virNodeDeviceObjListHasCap hascap;
+ const char *cap;
+ virNodeDeviceObjListMatch match;
+ unsigned int flags;
+ bool error;
+ int nElems;
+ int maxElems;
+ char **const elems;
+ virNodeDevicePtr *devices;
};
static int
-virNodeDeviceObjListNumOfDevicesCallback(void *payload,
- const void *name ATTRIBUTE_UNUSED,
- void *opaque)
+virNodeDeviceObjListForEachCb(void *payload,
+ const void *name ATTRIBUTE_UNUSED,
+ void *opaque)
{
virNodeDeviceObjPtr obj = payload;
- virNodeDeviceDefPtr def;
- struct virNodeDeviceCountData *data = opaque;
- virNodeDeviceObjListFilter filter = data->filter;
+ struct _virNodeDeviceObjForEachData *data = opaque;
+ virNodeDevicePtr device = NULL;
+
+ if (data->error)
+ return 0;
+
+ if (data->maxElems >= 0 && data->nElems == data->maxElems)
+ return 0;
virObjectLock(obj);
- def = obj->def;
- if ((!filter || filter(data->conn, def)) &&
- (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr)))
- data->count++;
+ if (data->filter && !data->filter(data->conn, obj->def))
+ goto cleanup;
+
+ if (data->hascap && data->hascap(obj, data->cap))
+ goto cleanup;
+
+ if (data->match && !data->match(obj, data->flags))
+ goto cleanup;
+
+ if (data->elems) {
+ if (VIR_STRDUP(data->elems[data->nElems], obj->def->name) < 0) {
+ data->error = true;
+ goto cleanup;
+ }
+ } else if (data->devices) {
+ if (!(device = virGetNodeDevice(data->conn, obj->def->name)) ||
+ VIR_STRDUP(device->parent, obj->def->parent) < 0) {
+ virObjectUnref(device);
+ data->error = true;
+ goto cleanup;
+ }
+ data->devices[data->nElems] = device;
+ }
+
+ data->nElems++;
+
+ cleanup:
virObjectUnlock(obj);
return 0;
}
@@ -764,55 +803,16 @@ virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs,
const char *cap,
virNodeDeviceObjListFilter filter)
{
- struct virNodeDeviceCountData data = {
- .conn = conn, .filter = filter, .matchstr = cap, .count = 0 };
+ struct _virNodeDeviceObjForEachData data = { .conn = conn,
+ .filter = filter, .hascap = virNodeDeviceObjHasCap, .cap = cap,
+ .match = NULL, .flags = 0, .error = false, .nElems = 0, .maxElems = -1,
+ .elems = NULL, .devices = NULL };
virObjectRWLockRead(devs);
- virHashForEach(devs->objs, virNodeDeviceObjListNumOfDevicesCallback, &data);
+ virHashForEach(devs->objs, virNodeDeviceObjListForEachCb, &data);
virObjectRWUnlock(devs);
- return data.count;
-}
-
-
-struct virNodeDeviceGetNamesData {
- virConnectPtr conn;
- virNodeDeviceObjListFilter filter;
- const char *matchstr;
- int nnames;
- char **names;
- int maxnames;
- bool error;
-};
-
-static int
-virNodeDeviceObjListGetNamesCallback(void *payload,
- const void *name ATTRIBUTE_UNUSED,
- void *opaque)
-{
- virNodeDeviceObjPtr obj = payload;
- virNodeDeviceDefPtr def;
- struct virNodeDeviceGetNamesData *data = opaque;
- virNodeDeviceObjListFilter filter = data->filter;
-
- if (data->error)
- return 0;
-
- virObjectLock(obj);
- def = obj->def;
-
- if ((!filter || filter(data->conn, def)) &&
- (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) {
- if (VIR_STRDUP(data->names[data->nnames], def->name) < 0) {
- data->error = true;
- goto cleanup;
- }
- data->nnames++;
- }
-
- cleanup:
- virObjectUnlock(obj);
- return 0;
+ return data.nElems;
}
@@ -824,22 +824,23 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs,
char **const names,
int maxnames)
{
- struct virNodeDeviceGetNamesData data = {
- .conn = conn, .filter = filter, .matchstr = cap, .names = names,
- .nnames = 0, .maxnames = maxnames, .error = false };
+ struct _virNodeDeviceObjForEachData data = { .conn = conn,
+ .filter = filter, .hascap = virNodeDeviceObjHasCap, .cap = cap,
+ .match = NULL, .flags = 0, .error = false, .nElems = 0,
+ .maxElems = maxnames, .elems = names, .devices = NULL };
virObjectRWLockRead(devs);
- virHashForEach(devs->objs, virNodeDeviceObjListGetNamesCallback, &data);
+ virHashForEach(devs->objs, virNodeDeviceObjListForEachCb, &data);
virObjectRWUnlock(devs);
if (data.error)
goto error;
- return data.nnames;
+ return data.nElems;
error:
- while (--data.nnames)
- VIR_FREE(data.names[data.nnames]);
+ while (--data.nElems)
+ VIR_FREE(data.elems[data.nElems]);
return -1;
}
@@ -876,51 +877,6 @@ virNodeDeviceMatch(virNodeDeviceObjPtr obj,
#undef MATCH
-struct virNodeDeviceObjListExportData {
- virConnectPtr conn;
- virNodeDeviceObjListFilter filter;
- unsigned int flags;
- virNodeDevicePtr *devices;
- int ndevices;
- bool error;
-};
-
-static int
-virNodeDeviceObjListExportCallback(void *payload,
- const void *name ATTRIBUTE_UNUSED,
- void *opaque)
-{
- virNodeDeviceObjPtr obj = payload;
- virNodeDeviceDefPtr def;
- struct virNodeDeviceObjListExportData *data = opaque;
- virNodeDevicePtr device = NULL;
-
- if (data->error)
- return 0;
-
- virObjectLock(obj);
- def = obj->def;
-
- if ((!data->filter || data->filter(data->conn, def)) &&
- virNodeDeviceMatch(obj, data->flags)) {
- if (data->devices) {
- if (!(device = virGetNodeDevice(data->conn, def->name)) ||
- VIR_STRDUP(device->parent, def->parent) < 0) {
- virObjectUnref(device);
- data->error = true;
- goto cleanup;
- }
- data->devices[data->ndevices] = device;
- }
- data->ndevices++;
- }
-
- cleanup:
- virObjectUnlock(obj);
- return 0;
-}
-
-
int
virNodeDeviceObjListExport(virConnectPtr conn,
virNodeDeviceObjListPtr devs,
@@ -928,9 +884,10 @@ virNodeDeviceObjListExport(virConnectPtr conn,
virNodeDeviceObjListFilter filter,
unsigned int flags)
{
- struct virNodeDeviceObjListExportData data = {
- .conn = conn, .filter = filter, .flags = flags,
- .devices = NULL, .ndevices = 0, .error = false };
+ struct _virNodeDeviceObjForEachData data = { .conn = conn,
+ .filter = filter, .hascap = NULL, .cap = NULL,
+ .match = virNodeDeviceMatch, .flags = flags, .error = false,
+ .nElems = 0, .maxElems = -1, .elems = NULL, .devices = NULL };
virObjectRWLockRead(devs);
if (devices &&
@@ -938,19 +895,21 @@ virNodeDeviceObjListExport(virConnectPtr conn,
virObjectRWUnlock(devs);
return -1;
}
+ if (data.devices)
+ data.maxElems = virHashSize(devs->objs) + 1;
- virHashForEach(devs->objs, virNodeDeviceObjListExportCallback, &data);
+ virHashForEach(devs->objs, virNodeDeviceObjListForEachCb, &data);
virObjectRWUnlock(devs);
if (data.error)
goto cleanup;
if (data.devices) {
- ignore_value(VIR_REALLOC_N(data.devices, data.ndevices + 1));
+ ignore_value(VIR_REALLOC_N(data.devices, data.nElems + 1));
*devices = data.devices;
}
- return data.ndevices;
+ return data.nElems;
cleanup:
virObjectListFree(data.devices);
--
2.13.6
8:46 a.m.
New subject: [libvirt] [PATCH 3/3] nodedev: Introduce virNodeDeviceObjListForEachCb
On Tue, Oct 10, 2017 at 09:54:28AM -0400, John Ferlan wrote:
Rather than separate functions for NumOfDevices, GetNames, and
ListExport - let's converge the code into one handler.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
NACK, for the very same reasons stated in [1], IOW we really should not make
this a busybox. But unlike [1], I'm not entirely sure what a nice cleanup would
look like (and it sure needs some).
Erik
8:52 a.m.
New subject: [libvirt] [PATCH 3/3] nodedev: Introduce virNodeDeviceObjListForEachCb
On Thu, Oct 12, 2017 at 03:46:25PM +0200, Erik Skultety wrote:
On Tue, Oct 10, 2017 at 09:54:28AM -0400, John Ferlan wrote:
> Rather than separate functions for NumOfDevices, GetNames, and
> ListExport - let's converge the code into one handler.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
NACK, for the very same reasons stated in [1], IOW we really should not make
this a busybox. But unlike [1], I'm not entirely sure what a nice cleanup would
look like (and it sure needs some).
A link would be actually nice too.
[1] https://www.redhat.com/archives/libvir-list/2017-October/msg00464.html
Erik
2598
days inactive
2600
days old
6 comments
2 participants
participants (2)
-
Erik Skultety -
John Ferlan