6:45 a.m.
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(a)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
9:07 a.m.
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(a)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
9:48 a.m.
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(a)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
10:54 a.m.
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(a)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
11:09 a.m.
[...]
>>
>> With the patch split in 2 introducing 2 distinct changes + the NULL check:
>> Reviewed-by: Erik Skultety <eskultet(a)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
2626
days inactive
2632
days old
4 comments
2 participants
participants (2)
-
Erik Skultety -
John Ferlan