[libvirt] [PATCH v3 12/12] nodedev: Convert virNodeDeviceObj to use virObjectLockable

[Erik Skultety]

On Thu, Jun 29, 2017 at 12:11:47PM -0400, John Ferlan wrote:
>
>
> On 06/03/2017 09:12 AM, John Ferlan wrote:
> > Now that we have a bit more control, let's convert our object into
> > a lockable object and let that magic handle the create and lock/unlock.
> >
> > This also involves creating a virNodeDeviceEndAPI in order to handle
> > the object cleaup for API's that use the Add or Find API's in order
> > to get a locked/reffed object. The EndAPI will unlock and unref the
> > object returning NULL to indicate to the caller to not use the obj.
> >
> > For now this also involves a forward reference to the Unlock, but
> > that'll be removed shortly when the object is convert to use the
> > virObjectLockable shortly.
> >
> > Signed-off-by: John Ferlan <jferlan at redhat.com>
> > ---
> >  src/conf/virnodedeviceobj.c          | 156 ++++++++++++++++++-----------------
> >  src/conf/virnodedeviceobj.h          |  11 +--
> >  src/libvirt_private.syms             |   4 +-
> >  src/node_device/node_device_driver.c |  17 ++--
> >  src/node_device/node_device_hal.c    |   6 +-
> >  src/node_device/node_device_udev.c   |  12 +--
> >  src/test/test_driver.c               |  40 ++++-----
> >  7 files changed, 122 insertions(+), 124 deletions(-)
> >
>
> [...]
>
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 67fe252..7b67523 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
>
> [...]
>
> Oh my... looks like I forgot to go back and revisit the following hunk
> ;-), but of course tripped across it while working through merging
> changes for patch 1.
>
> > @@ -5597,28 +5596,29 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
> >       * taken, so we have to dup the parent's name and drop the lock
> >       * before calling it.  We don't need the reference to the object
> >       * any more once we have the parent's name.  */
> > -    virNodeDeviceObjUnlock(obj);
> > +    virNodeDeviceObjEndAPI(&obj);
>
> This should be "virObjectUnlock(obj);"
>
> >
> >      /* We do this just for basic validation, but also avoid finding a
> >       * vport capable HBA if for some reason our vHBA doesn't exist */
> >      if (virNodeDeviceObjListGetParentHost(driver->devs, def,
> > -                                          EXISTING_DEVICE) < 0) {
> > -        obj = NULL;
> > +                                          EXISTING_DEVICE) < 0)
>
> This would require calling virObjectLock(obj);
>
> >          goto cleanup;
> > -    }
> >
> >      event = virNodeDeviceEventLifecycleNew(dev->name,
> >                                             VIR_NODE_DEVICE_EVENT_DELETED,
> >                                             0);
> >
> > -    virNodeDeviceObjLock(obj);
>
> and calling virObjectLock(obj) here prior to calling ObjListRemove and
> setting obj = NULL;
>
> > +    /*
> > +     *
> > +     * REVIEW THIS
> > +     *
> > +     */
> >      virNodeDeviceObjListRemove(driver->devs, obj);
> > -    virNodeDeviceObjFree(obj);
> > +    virObjectUnref(obj);
> >      obj = NULL;
>
> Hope this makes sense... I'm guessing some of this series will need to
> be reposted in a v4 anyway - including the "next" logical step to alter
> the ObjList which is what you're after anyway, but was further down in
> my original stack of patches.

Well, these would be really hard to track, so I agree that posting a v4 would
make things easier, since 10 (11 technically) out of 12 patches have already
been ACK'd, so it wouldn't prolong the overall process more than just those 2
patches we discuss.

Erik

>
> John
> >
> >   cleanup:
> > -    if (obj)
> > -        virNodeDeviceObjUnlock(obj);
> > +    virNodeDeviceObjEndAPI(&obj);
> >      testObjectEventQueue(driver, event);
> >      VIR_FREE(parent_name);
> >      VIR_FREE(wwnn);
> >
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list