[libvirt] [PATCH v3] interfaces: Convert virInterfaceObjList to virObjectRWLockable
Rather than a forward linked list, let's use the ObjectRWLockable object in order to manage the data. Requires numerous changes from List to Object management similar to many other drivers/vir*obj.c modules Signed-off-by: John Ferlan <jferlan@redhat.com> --- This is the real one - I need more coffee. src/conf/virinterfaceobj.c | 365 +++++++++++++++++++++++++++++++-------------- src/libvirt_private.syms | 1 - src/test/test_driver.c | 6 +- 3 files changed, 253 insertions(+), 119 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index e993b929d7..cf3626def4 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -25,6 +25,7 @@ #include "viralloc.h" #include "virerror.h" #include "virinterfaceobj.h" +#include "virhash.h" #include "virlog.h" #include "virstring.h" @@ -40,14 +41,19 @@ struct _virInterfaceObj { }; struct _virInterfaceObjList { - size_t count; - virInterfaceObjPtr *objs; + virObjectRWLockable parent; + + /* name string -> virInterfaceObj mapping + * for O(1), lockless lookup-by-name */ + virHashTable *objsName; }; /* virInterfaceObj manipulation */ static virClassPtr virInterfaceObjClass; +static virClassPtr virInterfaceObjListClass; static void virInterfaceObjDispose(void *obj); +static void virInterfaceObjListDispose(void *obj); static int virInterfaceObjOnceInit(void) @@ -58,6 +64,12 @@ virInterfaceObjOnceInit(void) virInterfaceObjDispose))) return -1; + if (!(virInterfaceObjListClass = virClassNew(virClassForObjectRWLockable(), + "virInterfaceObjList", + sizeof(virInterfaceObjList), + virInterfaceObjListDispose))) + return -1; + return 0; } @@ -130,119 +142,188 @@ virInterfaceObjListNew(void) { virInterfaceObjListPtr interfaces; - if (VIR_ALLOC(interfaces) < 0) + if (virInterfaceObjInitialize() < 0) + return NULL; + + if (!(interfaces = virObjectRWLockableNew(virInterfaceObjListClass))) + return NULL; + + if (!(interfaces->objsName = virHashCreate(10, virObjectFreeHashData))) { + virObjectUnref(interfaces); return NULL; + } + return interfaces; } +struct _virInterfaceObjFindMACData { + const char *matchStr; + bool error; + int nmacs; + int maxmacs; + char **const macs; +}; + +static int +virInterfaceObjListFindByMACStringCb(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virInterfaceObjPtr obj = payload; + struct _virInterfaceObjFindMACData *data = opaque; + + if (data->error) + return 0; + + if (data->nmacs == data->maxmacs) + return 0; + + virObjectLock(obj); + + if (STRCASEEQ(obj->def->mac, data->matchStr)) { + if (VIR_STRDUP(data->macs[data->nmacs], data->matchStr) < 0) { + data->error = true; + goto cleanup; + } + data->nmacs++; + } + + cleanup: + virObjectUnlock(obj); + return 0; +} + + int virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, const char *mac, char **const matches, int maxmatches) { - size_t i; - int matchct = 0; + struct _virInterfaceObjFindMACData data = { .matchStr = mac, + .error = false, + .nmacs = 0, + .maxmacs = maxmatches, + .macs = matches }; - for (i = 0; i < interfaces->count; i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceDefPtr def; + virObjectRWLockRead(interfaces); + virHashForEach(interfaces->objsName, virInterfaceObjListFindByMACStringCb, + &data); + virObjectRWUnlock(interfaces); - virObjectLock(obj); - def = obj->def; - if (STRCASEEQ(def->mac, mac)) { - if (matchct < maxmatches) { - if (VIR_STRDUP(matches[matchct], def->name) < 0) { - virObjectUnlock(obj); - goto error; - } - matchct++; - } - } - virObjectUnlock(obj); - } - return matchct; + if (data.error) + goto error; + + return data.nmacs; error: - while (--matchct >= 0) - VIR_FREE(matches[matchct]); + while (--data.nmacs >= 0) + VIR_FREE(data.macs[data.nmacs]); return -1; } +static virInterfaceObjPtr +virInterfaceObjListFindByNameLocked(virInterfaceObjListPtr interfaces, + const char *name) +{ + return virObjectRef(virHashLookup(interfaces->objsName, name)); +} + + virInterfaceObjPtr virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, const char *name) { - size_t i; - - for (i = 0; i < interfaces->count; i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceDefPtr def; - + virInterfaceObjPtr obj; + virObjectRWLockRead(interfaces); + obj = virInterfaceObjListFindByNameLocked(interfaces, name); + virObjectRWUnlock(interfaces); + if (obj) virObjectLock(obj); - def = obj->def; - if (STREQ(def->name, name)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - return NULL; + return obj; } void -virInterfaceObjListFree(virInterfaceObjListPtr interfaces) +virInterfaceObjListDispose(void *obj) { - size_t i; + virInterfaceObjListPtr interfaces = obj; + + virHashFree(interfaces->objsName); +} + + +struct _virInterfaceObjListCloneData { + bool error; + virInterfaceObjListPtr dest; +}; + +static int +virInterfaceObjListCloneCb(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virInterfaceObjPtr srcObj = payload; + struct _virInterfaceObjListCloneData *data = opaque; + char *xml = NULL; + virInterfaceDefPtr backup = NULL; + virInterfaceObjPtr obj; + + if (data->error) + return 0; + + virObjectLock(srcObj); + + if (!(xml = virInterfaceDefFormat(srcObj->def))) + goto error; + + if (!(backup = virInterfaceDefParseString(xml))) + goto error; + VIR_FREE(xml); - for (i = 0; i < interfaces->count; i++) - virObjectUnref(interfaces->objs[i]); - VIR_FREE(interfaces->objs); - VIR_FREE(interfaces); + if (!(obj = virInterfaceObjListAssignDef(data->dest, backup))) + goto error; + virInterfaceObjEndAPI(&obj); + + virObjectUnlock(srcObj); + return 0; + + error: + data->error = true; + VIR_FREE(xml); + virInterfaceDefFree(backup); + virObjectUnlock(srcObj); + return 0; } virInterfaceObjListPtr virInterfaceObjListClone(virInterfaceObjListPtr interfaces) { - size_t i; - unsigned int cnt; - virInterfaceObjListPtr dest; + struct _virInterfaceObjListCloneData data = { .error = false, + .dest = NULL }; if (!interfaces) return NULL; - if (!(dest = virInterfaceObjListNew())) + if (!(data.dest = virInterfaceObjListNew())) return NULL; - cnt = interfaces->count; - for (i = 0; i < cnt; i++) { - virInterfaceObjPtr srcobj = interfaces->objs[i]; - virInterfaceDefPtr backup; - virInterfaceObjPtr obj; - char *xml = virInterfaceDefFormat(srcobj->def); - - if (!xml) - goto error; + virObjectRWLockRead(interfaces); + virHashForEach(interfaces->objsName, virInterfaceObjListCloneCb, &data); + virObjectRWUnlock(interfaces); - if (!(backup = virInterfaceDefParseString(xml))) { - VIR_FREE(xml); - goto error; - } - - VIR_FREE(xml); - if (!(obj = virInterfaceObjListAssignDef(dest, backup))) - goto error; - virInterfaceObjEndAPI(&obj); - } + if (data.error) + goto error; - return dest; + return data.dest; error: - virInterfaceObjListFree(dest); + virObjectUnref(data.dest); return NULL; } @@ -253,9 +334,11 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, { virInterfaceObjPtr obj; - if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { + virObjectRWLockWrite(interfaces); + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { virInterfaceDefFree(obj->def); obj->def = def; + virObjectRWUnlock(interfaces); return obj; } @@ -263,13 +346,19 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, if (!(obj = virInterfaceObjNew())) return NULL; - if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, - interfaces->count, obj) < 0) { - virInterfaceObjEndAPI(&obj); - return NULL; - } + if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) + goto error; + virObjectRef(obj); + obj->def = def; - return virObjectRef(obj); + virObjectRWUnlock(interfaces); + + return obj; + + error: + virInterfaceObjEndAPI(&obj); + virObjectRWUnlock(interfaces); + return NULL; } @@ -277,20 +366,37 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj) { - size_t i; + virObjectRef(obj); + virObjectUnlock(obj); + virObjectRWLockWrite(interfaces); + virObjectLock(obj); + virHashRemoveEntry(interfaces->objsName, obj->def->name); + virObjectUnlock(obj); + virObjectUnref(obj); + virObjectRWUnlock(interfaces); +} + + +struct _virInterfaceObjNumOfInterfacesData { + bool wantActive; + int count; +}; + +static int +virInterfaceObjListNumOfInterfacesCb(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virInterfaceObjPtr obj = payload; + struct _virInterfaceObjNumOfInterfacesData *data = opaque; + + virObjectLock(obj); + + if (data->wantActive == virInterfaceObjIsActive(obj)) + data->count++; virObjectUnlock(obj); - for (i = 0; i < interfaces->count; i++) { - virObjectLock(interfaces->objs[i]); - if (interfaces->objs[i] == obj) { - virObjectUnlock(interfaces->objs[i]); - virObjectUnref(interfaces->objs[i]); - - VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count); - break; - } - virObjectUnlock(interfaces->objs[i]); - } + return 0; } @@ -298,18 +404,55 @@ int virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces, bool wantActive) { - size_t i; - int ninterfaces = 0; + struct _virInterfaceObjNumOfInterfacesData data = { + .wantActive = wantActive, .count = 0 }; - for (i = 0; (i < interfaces->count); i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virObjectLock(obj); - if (wantActive == virInterfaceObjIsActive(obj)) - ninterfaces++; - virObjectUnlock(obj); + virObjectRWLockRead(interfaces); + virHashForEach(interfaces->objsName, virInterfaceObjListNumOfInterfacesCb, + &data); + virObjectRWUnlock(interfaces); + + return data.count; +} + + +struct _virInterfaceObjGetNamesData { + bool wantActive; + bool error; + int nnames; + int maxnames; + char **const names; +}; + +static int +virInterfaceObjListGetNamesCb(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virInterfaceObjPtr obj = payload; + struct _virInterfaceObjGetNamesData *data = opaque; + + if (data->error) + return 0; + + if (data->maxnames >= 0 && data->nnames == data->maxnames) + return 0; + + virObjectLock(obj); + + if (data->wantActive != virInterfaceObjIsActive(obj)) + goto cleanup; + + if (VIR_STRDUP(data->names[data->nnames], obj->def->name) < 0) { + data->error = true; + goto cleanup; } - return ninterfaces; + data->nnames++; + + cleanup: + virObjectUnlock(obj); + return 0; } @@ -319,30 +462,22 @@ virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces, char **const names, int maxnames) { - int nnames = 0; - size_t i; + struct _virInterfaceObjGetNamesData data = { + .wantActive = wantActive, .error = false, .nnames = 0, + .maxnames = maxnames, .names = names }; - for (i = 0; i < interfaces->count && nnames < maxnames; i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceDefPtr def; + virObjectRWLockRead(interfaces); + virHashForEach(interfaces->objsName, virInterfaceObjListGetNamesCb, &data); + virObjectRWUnlock(interfaces); - virObjectLock(obj); - def = obj->def; - if (wantActive == virInterfaceObjIsActive(obj)) { - if (VIR_STRDUP(names[nnames], def->name) < 0) { - virObjectUnlock(obj); - goto failure; - } - nnames++; - } - virObjectUnlock(obj); - } + if (data.error) + goto error; - return nnames; + return data.nnames; - failure: - while (--nnames >= 0) - VIR_FREE(names[nnames]); + error: + while (--data.nnames >= 0) + VIR_FREE(data.names[data.nnames]); return -1; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 26c5ddb405..9133199895 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -932,7 +932,6 @@ virInterfaceObjListAssignDef; virInterfaceObjListClone; virInterfaceObjListFindByMACString; virInterfaceObjListFindByName; -virInterfaceObjListFree; virInterfaceObjListGetNames; virInterfaceObjListNew; virInterfaceObjListNumOfInterfaces; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1c48347994..14bba7494c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -154,7 +154,7 @@ testDriverFree(testDriverPtr driver) virObjectUnref(driver->domains); virNodeDeviceObjListFree(driver->devs); virObjectUnref(driver->networks); - virInterfaceObjListFree(driver->ifaces); + virObjectUnref(driver->ifaces); virStoragePoolObjListFree(&driver->pools); virObjectUnref(driver->eventState); virMutexUnlock(&driver->lock); @@ -3880,7 +3880,7 @@ testInterfaceChangeCommit(virConnectPtr conn, goto cleanup; } - virInterfaceObjListFree(privconn->backupIfaces); + virObjectUnref(privconn->backupIfaces); privconn->transaction_running = false; ret = 0; @@ -3910,7 +3910,7 @@ testInterfaceChangeRollback(virConnectPtr conn, goto cleanup; } - virInterfaceObjListFree(privconn->ifaces); + virObjectUnref(privconn->ifaces); privconn->ifaces = privconn->backupIfaces; privconn->backupIfaces = NULL; -- 2.13.6
On Fri, Oct 13, 2017 at 07:45:01AM -0400, John Ferlan wrote:
Rather than a forward linked list, let's use the ObjectRWLockable object in order to manage the data.
Requires numerous changes from List to Object management similar to many other drivers/vir*obj.c modules
This patch should be split in two. One that converts it to RWLockable, the other using a hash table instead of a linked list. [...]
@@ -253,9 +334,11 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, { virInterfaceObjPtr obj;
- if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { + virObjectRWLockWrite(interfaces); + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { virInterfaceDefFree(obj->def); obj->def = def; + virObjectRWUnlock(interfaces);
return obj; } @@ -263,13 +346,19 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, if (!(obj = virInterfaceObjNew())) return NULL;
- if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, - interfaces->count, obj) < 0) { - virInterfaceObjEndAPI(&obj); - return NULL; - } + if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) + goto error; + virObjectRef(obj); + obj->def = def; - return virObjectRef(obj); + virObjectRWUnlock(interfaces); + + return obj; + + error: + virInterfaceObjEndAPI(&obj); + virObjectRWUnlock(interfaces); + return NULL; }
^This could be tweaked even more: diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index cf3626def..941893fc5 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -337,19 +337,15 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, virObjectRWLockWrite(interfaces); if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { virInterfaceDefFree(obj->def); - obj->def = def; - virObjectRWUnlock(interfaces); + } else { + if (!(obj = virInterfaceObjNew())) + return NULL; - return obj; + if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) + goto error; + virObjectRef(obj); } - if (!(obj = virInterfaceObjNew())) - return NULL; - - if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) - goto error; - virObjectRef(obj); - obj->def = def; virObjectRWUnlock(interfaces);
@@ -277,20 +366,37 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj) { - size_t i;
if (!obj) return; I didn't bother to check whether it could happen (it's used in test driver only anyway), but just in case there's an early cleanup exit path like it was in my recent nodedev code which was missing this very check too which would have made it fail miserably in such scenario. With the patch split in 2 introducing 2 distinct changes + the NULL check: Reviewed-by: Erik Skultety <eskultet@redhat.com> PS: I'm also sad, that we have two backends here just like we have in nodedev with one of them being essentially useless (just like in nodedev) we have this 'conf' generic code (just like in nodedev), yet in this case it's only used in the test driver. I'd very much appreciate if those backends could be adjusted in a way where we could make use of these functions. I can also imagine a cooperation of the udev backend with the nodedev driver where we have an active connection to the monitor, thus reacting to all events realtime, instead of defining a bunch of filters and then letting udev re-enumerate the list of interfaces each and every time (and by saying that I'm also aware that udev is actually the useless backend here). Erik
On 10/19/2017 10:07 AM, Erik Skultety wrote:
On Fri, Oct 13, 2017 at 07:45:01AM -0400, John Ferlan wrote:
Rather than a forward linked list, let's use the ObjectRWLockable object in order to manage the data.
Requires numerous changes from List to Object management similar to many other drivers/vir*obj.c modules
This patch should be split in two. One that converts it to RWLockable, the other using a hash table instead of a linked list.
[...]
Personally, I don't recall during various conversions I've done to having: virObjectLockable parent; size_t count; virInterfaceObjPtr *objs; first, then converting to count/objs to (a) hash table(s). Although looking through history I see Michal had the networkobj code take a long winding path to get there, so it's not impossible - just IMO pointless. I think whenever I've converted from count/obs to a hash table with a 'real object', that the locking was done a the same time. As I recall, usually it's been converting from : size_t count; virInterfaceObjPtr *objs; to: virObjectLockable parent; /* name string -> virInterfaceObj mapping * for O(1), lockless lookup-by-name */ virHashTable *objsName; then more recently the conversion from virObjectLockable to virObjectRWLockable Since this patch is taking @objs and converting to a hash table - it's avoiding the Lockable and going straight to RWLockable.
@@ -253,9 +334,11 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, { virInterfaceObjPtr obj;
- if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { + virObjectRWLockWrite(interfaces); + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { virInterfaceDefFree(obj->def); obj->def = def; + virObjectRWUnlock(interfaces);
return obj; } @@ -263,13 +346,19 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, if (!(obj = virInterfaceObjNew())) return NULL;
- if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, - interfaces->count, obj) < 0) { - virInterfaceObjEndAPI(&obj); - return NULL; - } + if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) + goto error; + virObjectRef(obj); + obj->def = def; - return virObjectRef(obj); + virObjectRWUnlock(interfaces); + + return obj; + + error: + virInterfaceObjEndAPI(&obj); + virObjectRWUnlock(interfaces); + return NULL; }
^This could be tweaked even more:
Sure, but in doing so eagle eyes now spots a problem in his own code...
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index cf3626def..941893fc5 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -337,19 +337,15 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, virObjectRWLockWrite(interfaces); if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { virInterfaceDefFree(obj->def); - obj->def = def; - virObjectRWUnlock(interfaces); + } else { + if (!(obj = virInterfaceObjNew())) + return NULL;
Leaving with RWLock, <sigh>
- return obj; + if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) + goto error; + virObjectRef(obj); }
- if (!(obj = virInterfaceObjNew())) - return NULL; - - if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) - goto error; - virObjectRef(obj); - obj->def = def; virObjectRWUnlock(interfaces);
@@ -277,20 +366,37 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj) { - size_t i;
if (!obj) return;
OK - I think at some point in time I figured it wasn't possible, but adding it doesn't hurt. It's there in my branch.
I didn't bother to check whether it could happen (it's used in test driver only anyway), but just in case there's an early cleanup exit path like it was in my recent nodedev code which was missing this very check too which would have made it fail miserably in such scenario.
With the patch split in 2 introducing 2 distinct changes + the NULL check: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Hopefully you reconsider the desire for 2 patches...
PS: I'm also sad, that we have two backends here just like we have in nodedev with one of them being essentially useless (just like in nodedev) we have this 'conf' generic code (just like in nodedev), yet in this case it's only used in the test driver. I'd very much appreciate if those backends could be adjusted in a way where we could make use of these functions. I can also imagine a cooperation of the udev backend with the nodedev driver where we have an active connection to the monitor, thus reacting to all events realtime, instead of defining a bunch of filters and then letting udev re-enumerate the list of interfaces each and every time (and by saying that I'm also aware that udev is actually the useless backend here).
Erik
Yeah - the driver code here is quite different/strange and could possibly use a bit more convergence. I feel too battered and bruised over this convergence right now though ;-).... Besides the differences between the two really kind of are a bit scary w/r/t needing to call some sort of netcf* or udev* interface to get the answers. John
On Thu, Oct 19, 2017 at 10:48:56AM -0400, John Ferlan wrote:
On 10/19/2017 10:07 AM, Erik Skultety wrote:
On Fri, Oct 13, 2017 at 07:45:01AM -0400, John Ferlan wrote:
Rather than a forward linked list, let's use the ObjectRWLockable object in order to manage the data.
Requires numerous changes from List to Object management similar to many other drivers/vir*obj.c modules
This patch should be split in two. One that converts it to RWLockable, the other using a hash table instead of a linked list.
[...]
Personally, I don't recall during various conversions I've done to having:
virObjectLockable parent;
size_t count; virInterfaceObjPtr *objs;
first, then converting to count/objs to (a) hash table(s). Although looking through history I see Michal had the networkobj code take a long winding path to get there, so it's not impossible - just IMO pointless.
I think whenever I've converted from count/obs to a hash table with a 'real object', that the locking was done a the same time.
As I recall, usually it's been converting from :
size_t count; virInterfaceObjPtr *objs;
to:
virObjectLockable parent;
/* name string -> virInterfaceObj mapping * for O(1), lockless lookup-by-name */ virHashTable *objsName;
then more recently the conversion from virObjectLockable to virObjectRWLockable
Since this patch is taking @objs and converting to a hash table - it's avoiding the Lockable and going straight to RWLockable.
@@ -253,9 +334,11 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, { virInterfaceObjPtr obj;
- if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { + virObjectRWLockWrite(interfaces); + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { virInterfaceDefFree(obj->def); obj->def = def; + virObjectRWUnlock(interfaces);
return obj; } @@ -263,13 +346,19 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, if (!(obj = virInterfaceObjNew())) return NULL;
- if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, - interfaces->count, obj) < 0) { - virInterfaceObjEndAPI(&obj); - return NULL; - } + if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) + goto error; + virObjectRef(obj); + obj->def = def; - return virObjectRef(obj); + virObjectRWUnlock(interfaces); + + return obj; + + error: + virInterfaceObjEndAPI(&obj); + virObjectRWUnlock(interfaces); + return NULL; }
^This could be tweaked even more:
Sure, but in doing so eagle eyes now spots a problem in his own code...
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index cf3626def..941893fc5 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -337,19 +337,15 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, virObjectRWLockWrite(interfaces); if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { virInterfaceDefFree(obj->def); - obj->def = def; - virObjectRWUnlock(interfaces); + } else { + if (!(obj = virInterfaceObjNew())) + return NULL;
Leaving with RWLock, <sigh>
- return obj; + if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) + goto error; + virObjectRef(obj); }
- if (!(obj = virInterfaceObjNew())) - return NULL; - - if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) - goto error; - virObjectRef(obj); - obj->def = def; virObjectRWUnlock(interfaces);
@@ -277,20 +366,37 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj) { - size_t i;
if (!obj) return;
OK - I think at some point in time I figured it wasn't possible, but adding it doesn't hurt. It's there in my branch.
I didn't bother to check whether it could happen (it's used in test driver only anyway), but just in case there's an early cleanup exit path like it was in my recent nodedev code which was missing this very check too which would have made it fail miserably in such scenario.
With the patch split in 2 introducing 2 distinct changes + the NULL check: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Hopefully you reconsider the desire for 2 patches...
Well, since I was apparently fine with the change when reviewing the same changes to nodedev, I guess I should be fine with it now too, I don't know what made me change my opinion as time has passed, nevermind, it's not a deal breaker for me.
PS: I'm also sad, that we have two backends here just like we have in nodedev with one of them being essentially useless (just like in nodedev) we have this 'conf' generic code (just like in nodedev), yet in this case it's only used in the test driver. I'd very much appreciate if those backends could be adjusted in a way where we could make use of these functions. I can also imagine a cooperation of the udev backend with the nodedev driver where we have an active connection to the monitor, thus reacting to all events realtime, instead of defining a bunch of filters and then letting udev re-enumerate the list of interfaces each and every time (and by saying that I'm also aware that udev is actually the useless backend here).
Erik
Yeah - the driver code here is quite different/strange and could possibly use a bit more convergence. I feel too battered and bruised over this convergence right now though ;-).... Besides the differences
I hope it didn't sound like a request, it was meant to be more like a wish that we should do something about it (sometime). Erik
between the two really kind of are a bit scary w/r/t needing to call some sort of netcf* or udev* interface to get the answers.
John
[...]
With the patch split in 2 introducing 2 distinct changes + the NULL check: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Hopefully you reconsider the desire for 2 patches...
Well, since I was apparently fine with the change when reviewing the same changes to nodedev, I guess I should be fine with it now too, I don't know what made me change my opinion as time has passed, nevermind, it's not a deal breaker for me.
OK - thanks - I did note while looking deeper that I didn't remove virInterfaceObjListFree() from src/conf/virinterfaceobj.h, so I removed it...
PS: I'm also sad, that we have two backends here just like we have in nodedev with one of them being essentially useless (just like in nodedev) we have this 'conf' generic code (just like in nodedev), yet in this case it's only used in the test driver. I'd very much appreciate if those backends could be adjusted in a way where we could make use of these functions. I can also imagine a cooperation of the udev backend with the nodedev driver where we have an active connection to the monitor, thus reacting to all events realtime, instead of defining a bunch of filters and then letting udev re-enumerate the list of interfaces each and every time (and by saying that I'm also aware that udev is actually the useless backend here).
Erik
Yeah - the driver code here is quite different/strange and could possibly use a bit more convergence. I feel too battered and bruised over this convergence right now though ;-).... Besides the differences
I hope it didn't sound like a request, it was meant to be more like a wish that we should do something about it (sometime).
Erik
I didn't take it that way... Just making sure no one would be waiting for patches from me any time soon that did that type of convergence ;-) Tks - John
participants (2)
-
Erik Skultety -
John Ferlan