[libvirt] [PATCH v4 2/4] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable
John Ferlan
jferlan at redhat.com
Fri Feb 2 19:31:43 UTC 2018
On 01/31/2018 09:15 PM, Stefan Berger wrote:
> On 12/08/2017 09:01 AM, John Ferlan wrote:
>> Unlike it's counterparts, the nwfilter object code needs to be able
>> to support recursive read locks while processing and checking new
>> filters against the existing environment. Thus instead of using a
>> virObjectLockable which uses pure mutexes, use the virObjectRWLockable
>> and primarily use RWLockRead when obtaining the object lock since
>> RWLockRead locks can be recursively obtained (up to a limit) as long
>> as there's a corresponding unlock.
>>
>> Since all the object management is within the virnwfilterobj code, we
>> can limit the number of Write locks on the object to very small areas
>> of code to ensure we don't run into deadlock's with other threads and
>> domain code that will check/use the filters (it's a very delicate
>> balance). This limits the write locks to AssignDef and Remove processing.
>>
>> This patch will introduce a new API virNWFilterObjEndAPI to unlock
>> and deref the object.
>>
>> This patch introduces two helpers to promote/demote the read/write lock.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> src/conf/virnwfilterobj.c | 191
>> +++++++++++++++++++++++----------
>> src/conf/virnwfilterobj.h | 9 +-
>> src/libvirt_private.syms | 3 +-
>> src/nwfilter/nwfilter_driver.c | 15 +--
>> src/nwfilter/nwfilter_gentech_driver.c | 11 +-
>> 5 files changed, 149 insertions(+), 80 deletions(-)
>>
>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>> index 87d7e7270..6b4758656 100644
>> --- a/src/conf/virnwfilterobj.c
>> +++ b/src/conf/virnwfilterobj.c
>> @@ -34,7 +34,7 @@
>> VIR_LOG_INIT("conf.virnwfilterobj");
>>
>> struct _virNWFilterObj {
>> - virMutex lock;
>> + virObjectRWLockable parent;
>>
>> bool wantRemoved;
>>
>> @@ -47,27 +47,69 @@ struct _virNWFilterObjList {
>> virNWFilterObjPtr *objs;
>> };
>>
>> +static virClassPtr virNWFilterObjClass;
>> +static void virNWFilterObjDispose(void *opaque);
>> +
>> +
>> +static int
>> +virNWFilterObjOnceInit(void)
>> +{
>> + if (!(virNWFilterObjClass =
>> virClassNew(virClassForObjectRWLockable(),
>> + "virNWFilterObj",
>> + sizeof(virNWFilterObj),
>> + virNWFilterObjDispose)))
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virNWFilterObj)
>> +
>>
>> static virNWFilterObjPtr
>> virNWFilterObjNew(void)
>> {
>> virNWFilterObjPtr obj;
>>
>> - if (VIR_ALLOC(obj) < 0)
>> + if (virNWFilterObjInitialize() < 0)
> Is this function available at this point ?
It's "hidden" by that VIR_ONCE_GLOBAL_INIT macro... Fairly common when
using referenced object classes. See src/util/virthread.h.
>> return NULL;
>>
>> - if (virMutexInitRecursive(&obj->lock) < 0) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - "%s", _("cannot initialize mutex"));
>> - VIR_FREE(obj);
>> + if (!(obj = virObjectRWLockableNew(virNWFilterObjClass)))
>> return NULL;
>> - }
>>
>> - virNWFilterObjLock(obj);
>> + virObjectRWLockWrite(obj);
>
> Why do you create it with the write-lock held? I suppose if we do that
> we would always have to demote from writer. But I see the pattern of
> promoting *to* writer then demoting to reader in the rest of the patch.
>
Essentially a common way of handling a lockable object - if you search
on vir.*ObjNew (where .* is domain, interface, network, nodedevice,
secret, storagevol, and storagepool) you'll see a common theme.
Also a bit of paranoia. Since the nwfilters list will eventually be
write locked too during add, it shouldn't really matter.
We only ever do this when adding a new nwfilter. The two existing
callers of virNWFilterObjListAssignDef are virNWFilterObjListLoadConfig
via virNWFilterObjListLoadAllConfigs and nwfilterDefineXML do not demote
the lock from write to read. Both end up calling virNWFilterObjEndAPI to
release the lock when they're done.
>> return obj;
>> }
>>
>>
>> +static void
>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>> +{
>> + virObjectRWUnlock(obj);
>> + virObjectRWLockWrite(obj);
> Do we need to unlock here or can we have it locked for reading 5 times
> and then lock for writing ? I suppose this isn't trying to decrease the
> read-locks to zero and then only we are able to grab a write lock, right ?
>
That's my assumption on this too - trying to get read-locks refcnt to
zero. If there were a bunch of readers, then obtaining the write lock
will wait for all those readers to complete. If we were the only reader,
then getting the write will happen. It would be really nice to have the
dlm available - that code was generated using the OpenVMS lock manager
which I actually got to know quite well in a previous job...
In the long run, I believe this ends up being one of those
pthread_rwlock_wrlock type "things" related to refcnt's and which thread
we're talking about. The man page states:
"The pthread_rwlock_wrlock() function applies a write lock to the
read-write lock referenced by rwlock. The calling thread acquires the
write lock if no other thread (reader or writer) holds the read-write
lock rwlock. Otherwise, the thread blocks (that is, does not return from
the pthread_rwlock_wrlock() call) until it can acquire the lock. Results
are undefined if the calling thread holds the read-write lock (whether a
read or write lock) at the time the call is made."
So my "choice" is demote/promote mainly because of that "undefined" part
and because of other 'research' which seemed to indicate that promoting
to wr while holding rd doesn't work reliably.
>> +}
>> +
>> +
>> +static void
>> +virNWFilterObjDemoteFromWrite(virNWFilterObjPtr obj)
>> +{
>> + virObjectRWUnlock(obj);
>> + virObjectRWLockRead(obj);
>> +}
>> +
>> +
>> +void
>> +virNWFilterObjEndAPI(virNWFilterObjPtr *obj)
>> +{
>> + if (!*obj)
>> + return;
>> +
>> + virObjectRWUnlock(*obj);
>> + virObjectUnref(*obj);
>> + *obj = NULL;
>> +}
>> +
>> +
>> virNWFilterDefPtr
>> virNWFilterObjGetDef(virNWFilterObjPtr obj)
>> {
>> @@ -90,17 +132,15 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj)
>>
>>
>> static void
>> -virNWFilterObjFree(virNWFilterObjPtr obj)
>> +virNWFilterObjDispose(void *opaque)
>> {
>> + virNWFilterObjPtr obj = opaque;
>> +
>> if (!obj)
>> return;
>>
>> virNWFilterDefFree(obj->def);
>> virNWFilterDefFree(obj->newDef);
>> -
>> - virMutexDestroy(&obj->lock);
>> -
>> - VIR_FREE(obj);
>> }
>>
>>
>> @@ -109,7 +149,7 @@ virNWFilterObjListFree(virNWFilterObjListPtr
>> nwfilters)
>> {
>> size_t i;
>> for (i = 0; i < nwfilters->count; i++)
>> - virNWFilterObjFree(nwfilters->objs[i]);
>> + virObjectUnref(nwfilters->objs[i]);
>> VIR_FREE(nwfilters->objs);
>> VIR_FREE(nwfilters);
>> }
>> @@ -132,22 +172,32 @@ virNWFilterObjListRemove(virNWFilterObjListPtr
>> nwfilters,
>> {
>> size_t i;
>>
>> - virNWFilterObjUnlock(obj);
>> + virObjectRWUnlock(obj);
>>
>> for (i = 0; i < nwfilters->count; i++) {
>> - virNWFilterObjLock(nwfilters->objs[i]);
>> + virObjectRWLockWrite(nwfilters->objs[i]);
>
> The conversion is ok, but I am not sure why we are write-locking here ...?
>
This code essentially mirrors how other vir.*ObjListRemove API's handle
removing an object from the List (or what eventually will be a Hash Table).
I agree that getting the lock is superfluous, but to a degree this
change is part of an overall larger set of changes (already complete)
that have converted from linked lists to hash tables. The nwfilter code
has lagged because no one really wants to look at it!
>> if (nwfilters->objs[i] == obj) {
>> - virNWFilterObjUnlock(nwfilters->objs[i]);
>> - virNWFilterObjFree(nwfilters->objs[i]);
>> + virObjectRWUnlock(nwfilters->objs[i]);
>> + virObjectUnref(nwfilters->objs[i]);
>>
>> VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
>
> The nwfilters->count and nwfilters->objs is actually protected by
> nwfilterDriverLock(). So all good here to mess with the list.
>
True for now, but in two patches, that lock is removed. In the next
patch the nwfilters gets rwlock capabilities too.
Maybe once all this is done - I can go back and remove all the
unnecessary Lock/Unlock once list is RWLock'd.
>> break;
>> }
>> - virNWFilterObjUnlock(nwfilters->objs[i]);
>> + virObjectRWUnlock(nwfilters->objs[i]);
>> }
>> }
>>
>>
>> +/**
>> + * virNWFilterObjListFindByUUID
>> + * @nwfilters: Pointer to filter list
>> + * @uuid: UUID to use to lookup the object
>> + *
>> + * Search for the object by uuidstr in the hash table and return a read
>> + * locked copy of the object.
>> + *
>> + * Returns: A reffed and read locked object or NULL on error
>> + */
>> virNWFilterObjPtr
>> virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
>> const unsigned char *uuid)
>> @@ -158,17 +208,27 @@
>> virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
>>
>> for (i = 0; i < nwfilters->count; i++) {
>> obj = nwfilters->objs[i];
>> - virNWFilterObjLock(obj);
>> + virObjectRWLockRead(obj);
>> def = obj->def;
>> if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
>> - return obj;
>> - virNWFilterObjUnlock(obj);
>> + return virObjectRef(obj);
>
> All callers to this function seem to properly Unref the obj now. Good.
>
Yes, this has been a painful exercise... The domain code still is messed
up, but that's a different problem.
>> + virObjectRWUnlock(obj);
>> }
>>
>> return NULL;
>> }
>>
>>
>> +/**
>> + * virNWFilterObjListFindByName
>> + * @nwfilters: Pointer to filter list
>> + * @name: filter name to use to lookup the object
>> + *
>> + * Search for the object by name in the hash table and return a read
>> + * locked copy of the object.
>> + *
>> + * Returns: A reffed and read locked object or NULL on error
>> + */
>> virNWFilterObjPtr
>> virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
>> const char *name)
>> @@ -179,11 +239,11 @@
>> virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
>>
>> for (i = 0; i < nwfilters->count; i++) {
>> obj = nwfilters->objs[i];
>> - virNWFilterObjLock(obj);
>> + virObjectRWLockRead(obj);
>> def = obj->def;
>> if (STREQ_NULLABLE(def->name, name))
>> - return obj;
>> - virNWFilterObjUnlock(obj);
>> + return virObjectRef(obj);
>
> Also good here...
>
>> + virObjectRWUnlock(obj);
>> }
>>
>> return NULL;
>> @@ -205,8 +265,7 @@
>> virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
>> if (virNWFilterObjWantRemoved(obj)) {
>> virReportError(VIR_ERR_NO_NWFILTER,
>> _("Filter '%s' is in use."), filtername);
>> - virNWFilterObjUnlock(obj);
>> - return NULL;
>> + virNWFilterObjEndAPI(&obj);
>
> I think we MUST still return NULL here. This would otherwise change the
> behaviour of the code and why unref only in this case?
>
We do, see virNWFilterObjEndAPI, after it Unref and Unlock it:
*obj = NULL;
thus obj = NULL when we leave.
Every time we call virNWFilterObjListFindByName we return with a locked
and reffed @obj. Once we are done with that obj, we have to remove the
lock and ref.
It's a fairly common process with all the other driver code.
>> }
>>
>> return obj;
>> @@ -240,7 +299,7 @@
>> _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters,
>> if (obj) {
>> rc = _virNWFilterObjListDefLoopDetect(nwfilters,
>> obj->def,
>> filtername);
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>> if (rc < 0)
>> break;
>> }
>> @@ -269,17 +328,36 @@
>> virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters,
>> }
>>
>>
>> +/* virNWFilterObjTestUnassignDef
>> + * @obj: A read locked nwfilter object
>> + *
>> + * Cause the rebuild to occur because we're about to undefine the
>> nwfilter.
>> + * The rebuild code is designed to check if the filter is currently
>> in use
>> + * by a domain and thus disallow the unassign.
>> + *
>> + * NB: Although we enter with the UPDATE lock from UNDEFINE, let's still
>> + * promote to a WRITE lock before changing *this* object's
>> wantRemoved
>> + * value that will be used in the
>> virNWFilterObjListFindInstantiateFilter
>> + * processing to determine whether we can really remove this
>> filter or not.
>> + *
>> + * Returns 0 if we can continue with the unassign, -1 if it's in use
>> + */
>> int
>> virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj)
>> {
>> int rc = 0;
>>
>> + virNWFilterObjPromoteToWrite(obj);
>> obj->wantRemoved = true;
>> + virNWFilterObjDemoteFromWrite(obj);
>> +
>> /* trigger the update on VMs referencing the filter */
>> if (virNWFilterTriggerVMFilterRebuild() < 0)
>> rc = -1;
>>
>> + virNWFilterObjPromoteToWrite(obj);
>> obj->wantRemoved = false;
>> + virNWFilterObjDemoteFromWrite(obj);
>>
>> return rc;
>> }
>> @@ -322,10 +400,10 @@
>> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>> _("filter with same UUID but different
>> name "
>> "('%s') already exists"),
>> objdef->name);
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>> return NULL;
>> }
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>> } else {
>> if ((obj = virNWFilterObjListFindByName(nwfilters,
>> def->name))) {
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> @@ -335,7 +413,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr
>> nwfilters,
>> virReportError(VIR_ERR_OPERATION_FAILED,
>> _("filter '%s' already exists with uuid
>> %s"),
>> def->name, uuidstr);
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>> return NULL;
>> }
>> }
>> @@ -347,7 +425,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr
>> nwfilters,
>> }
>>
>>
>> + /* Get a READ lock and immediately promote to WRITE while we adjust
>> + * data within. */
>> if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>> + virNWFilterObjPromoteToWrite(obj);
>>
>> objdef = obj->def;
>> if (virNWFilterDefEqual(def, objdef)) {
>
> Shouldn't we demote to read after this virNWFilterDefEqual() call and
> before the 'return obj;' that's not shown in the patch ?
>
True that could/should be done since we're no longer updating @obj. I
can add that.
>> @@ -357,13 +438,20 @@
>> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>> }
>>
>> obj->newDef = def;
>> +
>> + /* Demote while the trigger runs since it may need to grab a
>> read
>> + * lock on this object and promote before returning. */
>> + virNWFilterObjDemoteFromWrite(obj);
>> +
>> /* trigger the update on VMs referencing the filter */
>> if (virNWFilterTriggerVMFilterRebuild() < 0) {
>> + virNWFilterObjPromoteToWrite(obj);
>> obj->newDef = NULL;
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>
> Not a bug, but to make this clearer I think we may want to call
>
> virNWFilterObjDemoteFromWrite(obj);
> virNWFilterObjEndAPI(&obj);
>
> I know it's redundant ...
>
This one I'm not sure I agree as what we're doing then is:
Unlock(WR)
Lock(RD)
Unlock(RD)
Instead of just
Unlock(WR)
I'm not 100% against it, but I'm not sure I see the reason. How about
if I add before the EndAPI:
/* NB: We're done, no need to Demote and End, just End */
>> return NULL;
>> }
>>
>> + virNWFilterObjPromoteToWrite(obj);
>> virNWFilterDefFree(objdef);
>> obj->def = def;
>> obj->newDef = NULL;
>
> demote to read here ?
>
Sure...
>> @@ -375,13 +463,12 @@
>> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>>
>> if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs,
>> nwfilters->count, obj) < 0) {
>> - virNWFilterObjUnlock(obj);
>> - virNWFilterObjFree(obj);
>> + virNWFilterObjEndAPI(&obj);
>> return NULL;
>> }
>> obj->def = def;
>>
>> - return obj;
>> + return virObjectRef(obj);
>
> This is correct but again , to make this clearer I think we should write:
>
> if (VIR_APPEND...) {
> [...]
> } else {
> virObjectRef(obj);
> }
>
Technically we wouldn't need the else if the if returns NULL...
> obj->def = def;
>
> return obj;
>
> We take the additional reference because we just successfully put it
> onto the list.
>
True, but that is handled that way in the next patch (essentially).
I'll make the change for clarity in this patch though.
>> }
>>
>>
>> @@ -395,10 +482,10 @@
>> virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters,
>>
>> for (i = 0; i < nwfilters->count; i++) {
>> virNWFilterObjPtr obj = nwfilters->objs[i];
>> - virNWFilterObjLock(obj);
>> + virObjectRWLockRead(obj);
>> if (!filter || filter(conn, obj->def))
>> nfilters++;
>> - virNWFilterObjUnlock(obj);
>> + virObjectRWUnlock(obj);
>> }
>>
>> return nfilters;
>> @@ -418,16 +505,16 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr
>> nwfilters,
>>
>> for (i = 0; i < nwfilters->count && nnames < maxnames; i++) {
>> virNWFilterObjPtr obj = nwfilters->objs[i];
>> - virNWFilterObjLock(obj);
>> + virObjectRWLockRead(obj);
>> def = obj->def;
>> if (!filter || filter(conn, def)) {
>> if (VIR_STRDUP(names[nnames], def->name) < 0) {
>> - virNWFilterObjUnlock(obj);
>> + virObjectRWUnlock(obj);
>> goto failure;
>> }
>> nnames++;
>> }
>> - virNWFilterObjUnlock(obj);
>> + virObjectRWUnlock(obj);
>> }
>>
>> return nnames;
>> @@ -464,16 +551,16 @@ virNWFilterObjListExport(virConnectPtr conn,
>>
>> for (i = 0; i < nwfilters->count; i++) {
>> obj = nwfilters->objs[i];
>> - virNWFilterObjLock(obj);
>> + virObjectRWLockRead(obj);
>> def = obj->def;
>> if (!filter || filter(conn, def)) {
>> if (!(nwfilter = virGetNWFilter(conn, def->name,
>> def->uuid))) {
>> - virNWFilterObjUnlock(obj);
>> + virObjectRWUnlock(obj);
>> goto cleanup;
>> }
>> tmp_filters[nfilters++] = nwfilter;
>> }
>> - virNWFilterObjUnlock(obj);
>> + virObjectRWUnlock(obj);
>> }
>>
>> *filters = tmp_filters;
>> @@ -551,24 +638,10 @@
>> virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters,
>> continue;
>>
>> obj = virNWFilterObjListLoadConfig(nwfilters, configDir,
>> entry->d_name);
>> - if (obj)
>> - virNWFilterObjUnlock(obj);
>> +
>> + virNWFilterObjEndAPI(&obj);
>> }
>>
>> VIR_DIR_CLOSE(dir);
>> return ret;
>> }
>> -
>> -
>> -void
>> -virNWFilterObjLock(virNWFilterObjPtr obj)
>> -{
>> - virMutexLock(&obj->lock);
>> -}
>> -
>> -
>> -void
>> -virNWFilterObjUnlock(virNWFilterObjPtr obj)
>> -{
>> - virMutexUnlock(&obj->lock);
>> -}
>> diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
>> index 8e79518ed..0281bc5f5 100644
>> --- a/src/conf/virnwfilterobj.h
>> +++ b/src/conf/virnwfilterobj.h
>> @@ -41,6 +41,9 @@ struct _virNWFilterDriverState {
>> bool watchingFirewallD;
>> };
>>
>> +void
>> +virNWFilterObjEndAPI(virNWFilterObjPtr *obj);
>> +
>> virNWFilterDefPtr
>> virNWFilterObjGetDef(virNWFilterObjPtr obj);
>>
>> @@ -105,10 +108,4 @@ int
>> virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters,
>> const char *configDir);
>>
>> -void
>> -virNWFilterObjLock(virNWFilterObjPtr obj);
>> -
>> -void
>> -virNWFilterObjUnlock(virNWFilterObjPtr obj);
>> -
>> #endif /* VIRNWFILTEROBJ_H */
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index de4ec4d44..132244878 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1022,6 +1022,7 @@ virNodeDeviceObjListRemove;
>>
>>
>> # conf/virnwfilterobj.h
>> +virNWFilterObjEndAPI;
>> virNWFilterObjGetDef;
>> virNWFilterObjGetNewDef;
>> virNWFilterObjListAssignDef;
>> @@ -1035,9 +1036,7 @@ virNWFilterObjListLoadAllConfigs;
>> virNWFilterObjListNew;
>> virNWFilterObjListNumOfNWFilters;
>> virNWFilterObjListRemove;
>> -virNWFilterObjLock;
>> virNWFilterObjTestUnassignDef;
>> -virNWFilterObjUnlock;
>> virNWFilterObjWantRemoved;
>>
>>
>> diff --git a/src/nwfilter/nwfilter_driver.c
>> b/src/nwfilter/nwfilter_driver.c
>> index 885dbcc28..b38740b15 100644
>> --- a/src/nwfilter/nwfilter_driver.c
>> +++ b/src/nwfilter/nwfilter_driver.c
>> @@ -400,7 +400,7 @@ nwfilterLookupByUUID(virConnectPtr conn,
>> nwfilter = virGetNWFilter(conn, def->name, def->uuid);
>>
>> cleanup:
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>> return nwfilter;
>> }
>>
>> @@ -430,7 +430,7 @@ nwfilterLookupByName(virConnectPtr conn,
>> nwfilter = virGetNWFilter(conn, def->name, def->uuid);
>>
>> cleanup:
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>> return nwfilter;
>> }
>>
>> @@ -517,6 +517,8 @@ nwfilterDefineXML(virConnectPtr conn,
>>
>> if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) {
>> virNWFilterObjListRemove(driver->nwfilters, obj);
>> + virObjectUnref(obj);
>> + obj = NULL;
>> goto cleanup;
>> }
>>
>> @@ -524,8 +526,7 @@ nwfilterDefineXML(virConnectPtr conn,
>>
>> cleanup:
>> virNWFilterDefFree(def);
>> - if (obj)
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>>
>> virNWFilterCallbackDriversUnlock();
>> virNWFilterUnlockFilterUpdates();
>> @@ -563,12 +564,12 @@ nwfilterUndefine(virNWFilterPtr nwfilter)
>> goto cleanup;
>>
>> virNWFilterObjListRemove(driver->nwfilters, obj);
>> + virObjectUnref(obj);
>> obj = NULL;
>> ret = 0;
>>
>> cleanup:
>> - if (obj)
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>>
>> virNWFilterCallbackDriversUnlock();
>> virNWFilterUnlockFilterUpdates();
>> @@ -601,7 +602,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter,
>> ret = virNWFilterDefFormat(def);
>>
>> cleanup:
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>> return ret;
>> }
>>
>> diff --git a/src/nwfilter/nwfilter_gentech_driver.c
>> b/src/nwfilter/nwfilter_gentech_driver.c
>> index 840d419bb..48d0e1769 100644
>> --- a/src/nwfilter/nwfilter_gentech_driver.c
>> +++ b/src/nwfilter/nwfilter_gentech_driver.c
>> @@ -316,7 +316,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst)
>> size_t i;
>>
>> for (i = 0; i < inst->nfilters; i++)
>> - virNWFilterObjUnlock(inst->filters[i]);
>> + virNWFilterObjEndAPI(&inst->filters[i]);
>> VIR_FREE(inst->filters);
>> inst->nfilters = 0;
>>
>> @@ -426,8 +426,7 @@
>> virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver,
>> if (ret < 0)
>> virNWFilterInstReset(inst);
>> virNWFilterHashTableFree(tmpvars);
>> - if (obj)
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>> return ret;
>> }
>>
>> @@ -541,7 +540,7 @@
>> virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
>>
>> /* create a temporary hashmap for depth-first tree
>> traversal */
>> if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params,
>> vars))) {
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>> return -1;
>> }
>>
>> @@ -565,7 +564,7 @@
>> virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
>>
>> virNWFilterHashTableFree(tmpvars);
>>
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>> if (rc < 0)
>> return -1;
>> }
>> @@ -839,7 +838,7 @@
>> virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver,
>> virNWFilterHashTableFree(vars1);
>>
>> err_exit:
>> - virNWFilterObjUnlock(obj);
>> + virNWFilterObjEndAPI(&obj);
>>
>> VIR_FREE(str_ipaddr);
>> VIR_FREE(str_macaddr);
>
> The call to virNWFilterObjListFindInstantiateFilter() now returns an
> additional reference, so these last 4 calls to virNWFilterObjEndApi()
> are unreferencing that -- good.
>
>
> Stefan
>
Thanks for the review... I appreciate it especially since nwfilter has
languished in this "project" I undertook a bit over a year ago to
convert driver objects to using objects/classes and hash tables instead
of VIR_ALLOC[_N] and forward linked lists... It's the last code to
convert although domainobj[list] processing still doesn't 100% use the
style. Not sure if I still have the same energy/desire to tackle domain
though.
John
More information about the libvir-list
mailing list