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