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@xxxxxxxxxx>
>
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
