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

[Erik Skultety]

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 at 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