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