Re: [libvirt] [PATCH v3] interfaces: Convert virInterfaceObjList to virObjectRWLockable

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&gt; 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