On 12/08/2017 09:01 AM, John Ferlan wrote:

Commit id '46a811db07' added code to check if the filter by Name already existed with a different UUID, to go along with the existing found filter by UUID and compare the Names match thus making it impossible to reach this find by Name condition without both the Name and UUID already matching, so remove the need to "filter" out the UUID for the virNWFilterDefEqual.

I can't be certain, but it looks to me like the original nwfilter author intended to allow changing the uuid of an existing nwfilter, and put in all the code checking for "matches everything except uuid" specifically to make that work correctly, but a bug report (https://bugzilla.redhat.com/1077009) convinced someone else to prohibit that, thus rendering all the stuff you've removed (and I think a bit more) irrelevant. What it's been reduced to now is "if the new definition is *exactly* identical to the old definition, then replace the old with the new but don't rebuild client filters", which seems mostly pointless, but also harmless (it's just replacing the original with a new copy that is exactly identical).

Signed-off-by: John Ferlan <jferlan@redhat.com>

Reviewed-by: Laine Stump <laine@laine.org>

--- src/conf/virnwfilterobj.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 408e575ca..87d7e7270 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -287,18 +287,11 @@ virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj)

static bool virNWFilterDefEqual(const virNWFilterDef *def1, - virNWFilterDefPtr def2, - bool cmpUUIDs) + virNWFilterDefPtr def2) { bool ret = false; - unsigned char rem_uuid[VIR_UUID_BUFLEN]; - char *xml1, *xml2 = NULL; - - if (!cmpUUIDs) { - /* make sure the UUIDs are equal */ - memcpy(rem_uuid, def2->uuid, sizeof(rem_uuid)); - memcpy(def2->uuid, def1->uuid, sizeof(def2->uuid)); - } + char *xml1 = NULL; + char *xml2 = NULL;

if (!(xml1 = virNWFilterDefFormat(def1)) || !(xml2 = virNWFilterDefFormat(def2))) @@ -307,9 +300,6 @@ virNWFilterDefEqual(const virNWFilterDef *def1, ret = STREQ(xml1, xml2);

cleanup: - if (!cmpUUIDs) - memcpy(def2->uuid, rem_uuid, sizeof(rem_uuid)); - VIR_FREE(xml1); VIR_FREE(xml2);

@@ -360,7 +350,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {

objdef = obj->def; - if (virNWFilterDefEqual(def, objdef, false)) { + if (virNWFilterDefEqual(def, objdef)) { virNWFilterDefFree(objdef); obj->def = def; return obj;

On 01/31/2018 05:52 PM, Stefan Berger wrote:

On 12/08/2017 09:01 AM, John Ferlan wrote:

...

Commit id '46a811db07' added code to check if the filter by Name already existed with a different UUID, to go along with the existing found filter by UUID and compare the Names match thus making it impossible to reach this find by Name condition without both the Name and UUID already matching, so remove the need to "filter" out the UUID for the virNWFilterDefEqual.

Sorry, but difficult to parse this sentence.

I'll reword before pushing... Suffice to say it was a confusing check when I came across it while working through the changes... This particular commit was written in July 2017! New wording: Remove the unnecessary check as since commit id '46a811db07' it is not possible to add or alter a filter using the same name, but with a different UUID. NB: It's not required to provide a UUID for a filter by name, but if one is provided, then it must match the existing. If not provided, then one is generated during ParseXML processing.

...

Signed-off-by: John Ferlan <jferlan@redhat.com> ---   src/conf/virnwfilterobj.c | 18 ++++--------------   1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 408e575ca..87d7e7270 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -287,18 +287,11 @@ virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj)

  static bool   virNWFilterDefEqual(const virNWFilterDef *def1, -                    virNWFilterDefPtr def2, -                    bool cmpUUIDs) +                    virNWFilterDefPtr def2)   {       bool ret = false; -    unsigned char rem_uuid[VIR_UUID_BUFLEN]; -    char *xml1, *xml2 = NULL; - -    if (!cmpUUIDs) { -        /* make sure the UUIDs are equal */ -        memcpy(rem_uuid, def2->uuid, sizeof(rem_uuid)); -        memcpy(def2->uuid, def1->uuid, sizeof(def2->uuid)); -    } +    char *xml1 = NULL; +    char *xml2 = NULL;

      if (!(xml1 = virNWFilterDefFormat(def1)) ||           !(xml2 = virNWFilterDefFormat(def2))) @@ -307,9 +300,6 @@ virNWFilterDefEqual(const virNWFilterDef *def1,       ret = STREQ(xml1, xml2);

Thinking out lout here (and below): virNWFilterDefFormat() will write the UUID into the XML. The STREQ will only ever be equal if def1 and def2 have the same UUID.

So, IIRC, what I determined at the time was that virNWFilterDefEqual was only ever called with @cmpUUIDs == false. Looking through history, there never was a case where @cmpUUIDs == true that I could find - all the way back to your original implementation in commit '823b90339'.

...

   cleanup: -    if (!cmpUUIDs) -        memcpy(def2->uuid, rem_uuid, sizeof(rem_uuid)); -       VIR_FREE(xml1);       VIR_FREE(xml2);

@@ -360,7 +350,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,       if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {

We will not get here if a filter with the same uuid and name exists or same name and different uuid. So we can have the same uuid and a different name at this point.

Not sure I agree with your last sentence. The first search in virNWFilterObjAssignDef is FindByUUID. If a filter in the nwfilters list matches the incoming @def->uuid, then a check is made that the filter name's match. If the names don't match an error is returned. Otherwise, if a filter by the incoming UUID is not found, then a FindByName is done. If one is found, then a check is made that the UUID's match. If the UUID's don't match, then an error is returned (likely!)... If the UUID's matched, then something's wrong because we failed the FindByUUID. If we get here, then we are assured that the uuid's and name's match, so what we're checking is whether the incoming definition changes anything else if a definition is found in the nwfilters list.

...

          objdef = obj->def; -        if (virNWFilterDefEqual(def, objdef, false)) { +        if (virNWFilterDefEqual(def, objdef)) {

This call doesn't need to 'shadow' the uuid. It's the only caller. so your change seems to be correct.

Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>

Thanks - I'll push this first patch sometime soon. John

...

              virNWFilterDefFree(objdef);               obj->def = def;               return obj;

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@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 ? 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.

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 ?

+} + + +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 ...?

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.

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.

+ 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?

}

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 ?

@@ -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 ...

return NULL; }

+ virNWFilterObjPromoteToWrite(obj); virNWFilterDefFree(objdef); obj->def = def; obj->newDef = NULL;

demote to read here ?

@@ -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); } obj->def = def; return obj; We take the additional reference because we just successfully put it onto the list.

}

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

On 02/02/2018 02:31 PM, John Ferlan wrote:

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@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.

I didn't recognize it... and, hm, unless one peeks into that functions, it may be difficult to follow. For clarity I would return NULL explicitly here ...

...

...

}

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.

... could otherwise prevent others from grabbing the read lock...

...

...

@@ -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 */

Sounds good to me.

On 12/08/2017 09:01 AM, John Ferlan wrote:

Implement the self locking object list for nwfilter object lists that uses two hash tables to store the nwfilter object by UUID or by Name.

As part of this alter the uuid argument to virNWFilterObjLookupByUUID to expect an already formatted uuidstr.

Alter the existing list traversal code to implement the hash table find/lookup/search functionality.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 402 ++++++++++++++++++++++++++++------------- src/conf/virnwfilterobj.h | 2 +- src/nwfilter/nwfilter_driver.c | 5 +- 3 files changed, 282 insertions(+), 127 deletions(-)

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 6b4758656..a4e6a03d2 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -43,12 +43,21 @@ struct _virNWFilterObj { };

struct _virNWFilterObjList { - size_t count; - virNWFilterObjPtr *objs; + virObjectRWLockable parent; + + /* uuid string -> virNWFilterObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; + + /* name -> virNWFilterObj mapping for O(1), + * lockless lookup-by-name */ + virHashTable *objsName; };

static virClassPtr virNWFilterObjClass; +static virClassPtr virNWFilterObjListClass; static void virNWFilterObjDispose(void *opaque); +static void virNWFilterObjListDispose(void *opaque);

static int @@ -60,6 +69,12 @@ virNWFilterObjOnceInit(void) virNWFilterObjDispose))) return -1;

+ if (!(virNWFilterObjListClass = virClassNew(virClassForObjectRWLockable(), + "virNWFilterObjList", + sizeof(virNWFilterObjList), + virNWFilterObjListDispose))) + return -1; + return 0; }

@@ -144,14 +159,20 @@ virNWFilterObjDispose(void *opaque) }

+static void +virNWFilterObjListDispose(void *opaque) +{ + virNWFilterObjListPtr nwfilters = opaque; + + virHashFree(nwfilters->objs); + virHashFree(nwfilters->objsName); +} + + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { - size_t i; - for (i = 0; i < nwfilters->count; i++) - virObjectUnref(nwfilters->objs[i]); - VIR_FREE(nwfilters->objs); - VIR_FREE(nwfilters); + virObjectUnref(nwfilters); }

@@ -160,8 +181,23 @@ virNWFilterObjListNew(void) { virNWFilterObjListPtr nwfilters;

- if (VIR_ALLOC(nwfilters) < 0) + if (virNWFilterObjInitialize() < 0) + return NULL; + + if (!(nwfilters = virObjectRWLockableNew(virNWFilterObjListClass))) + return NULL; + + if (!(nwfilters->objs = virHashCreate(10, virObjectFreeHashData))) { + virObjectUnref(nwfilters); + return NULL; + } + + if (!(nwfilters->objsName = virHashCreate(10, virObjectFreeHashData))) { + virHashFree(nwfilters->objs); + virObjectUnref(nwfilters); return NULL; + } + return nwfilters; }

@@ -170,83 +206,105 @@ void virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj) { - size_t i; - - virObjectRWUnlock(obj); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virNWFilterDefPtr def;

- for (i = 0; i < nwfilters->count; i++) { - virObjectRWLockWrite(nwfilters->objs[i]); - if (nwfilters->objs[i] == obj) { - virObjectRWUnlock(nwfilters->objs[i]); - virObjectUnref(nwfilters->objs[i]); + if (!obj) + return; + def = obj->def;

- VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); - break; - } - virObjectRWUnlock(nwfilters->objs[i]); - } + virUUIDFormat(def->uuid, uuidstr); + virObjectRef(obj); + virObjectRWUnlock(obj); + virObjectRWLockWrite(nwfilters); + virObjectRWLockWrite(obj); + virHashRemoveEntry(nwfilters->objs, uuidstr); Unref here after successful removal from hash bucket? + virHashRemoveEntry(nwfilters->objsName, def->name); Again unref here after successful removal ? + virObjectRWUnlock(obj); + virObjectUnref(obj); + virObjectRWUnlock(nwfilters); }

/** - * virNWFilterObjListFindByUUID + * virNWFilterObjListFindByUUID[Locked] * @nwfilters: Pointer to filter list - * @uuid: UUID to use to lookup the object + * @uuidstr: UUID to use to lookup the object + * + * The static [Locked] version would only be used when the Object List is + * already locked, such as is the case during virNWFilterObjListAssignDef. + * The caller is thus responsible for locking the object. * * Search for the object by uuidstr in the hash table and return a read * locked copy of the object. * + * Returns: A reffed object or NULL on error + */ +static virNWFilterObjPtr +virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters, + const char *uuidstr) +{ + return virObjectRef(virHashLookup(nwfilters->objs, uuidstr)); +} + + +/* * Returns: A reffed and read locked object or NULL on error */ virNWFilterObjPtr virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, - const unsigned char *uuid) + const char *uuidstr) { - size_t i; virNWFilterObjPtr obj; - virNWFilterDefPtr def;

- for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; + virObjectRWLockRead(nwfilters); + obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuidstr); + virObjectRWUnlock(nwfilters); + if (obj) virObjectRWLockRead(obj); - def = obj->def; - if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return virObjectRef(obj); - virObjectRWUnlock(obj); - }

- return NULL; + return obj; }

/** - * virNWFilterObjListFindByName + * virNWFilterObjListFindByName[Locked] * @nwfilters: Pointer to filter list * @name: filter name to use to lookup the object * + * The static [Locked] version would only be used when the Object List is + * already locked, such as is the case during virNWFilterObjListAssignDef. + * The caller is thus responsible for locking the object. + * * Search for the object by name in the hash table and return a read * locked copy of the object. * + * Returns: A reffed object or NULL on error + */ +static virNWFilterObjPtr +virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters, + const char *name) +{ + return virObjectRef(virHashLookup(nwfilters->objsName, name)); +} + + +/* * Returns: A reffed and read locked object or NULL on error */ virNWFilterObjPtr virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, const char *name) { - size_t i; virNWFilterObjPtr obj; - virNWFilterDefPtr def;

- for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; + virObjectRWLockRead(nwfilters); + obj = virNWFilterObjListFindByNameLocked(nwfilters, name); + virObjectRWUnlock(nwfilters); + if (obj) virObjectRWLockRead(obj); - def = obj->def; - if (STREQ_NULLABLE(def->name, name)) - return virObjectRef(obj); - virObjectRWUnlock(obj); - }

- return NULL; + return obj; }

@@ -391,8 +449,11 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, { virNWFilterObjPtr obj; virNWFilterDefPtr objdef; + char uuidstr[VIR_UUID_STRING_BUFLEN];

- if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) { + virUUIDFormat(def->uuid, uuidstr); + + if ((obj = virNWFilterObjListFindByUUID(nwfilters, uuidstr))) { objdef = obj->def;

if (STRNEQ(def->name, objdef->name)) { @@ -406,10 +467,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterObjEndAPI(&obj); } else { if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - objdef = obj->def; - virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); @@ -424,11 +482,13 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, return NULL; }

- - /* Get a READ lock and immediately promote to WRITE while we adjust - * data within. */ - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { - virNWFilterObjPromoteToWrite(obj); + /* We're about to make some changes to objects on the list - so get + * the list READ lock in order to Find the object and WRITE lock it + * while we adjust data within. */ + virObjectRWLockRead(nwfilters); + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { + virObjectRWUnlock(nwfilters); + virObjectRWLockWrite(obj);

objdef = obj->def; if (virNWFilterDefEqual(def, objdef)) {

I think you should have left the above PromoteToWrite(obj) rather than doing a virObjectRWLockWrite(obj) now and would then adapt the code here as mentioned in review of 2/4.

@@ -458,37 +518,112 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, return obj; }

+ /* Promote the nwfilters to add a new object */ + virObjectRWUnlock(nwfilters); + virObjectRWLockWrite(nwfilters); if (!(obj = virNWFilterObjNew())) - return NULL; + goto cleanup;

- if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, - nwfilters->count, obj) < 0) { - virNWFilterObjEndAPI(&obj); - return NULL; + if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0) + goto error; + virObjectRef(obj);

good, here you take a reference because obj ended in the hash bucket. Therefore, further above, you have to unref when taking it out of the hash bucket.

+ + if (virHashAddEntry(nwfilters->objsName, def->name, obj) < 0) { + virHashRemoveEntry(nwfilters->objs, uuidstr); + goto error; } + virObjectRef(obj); + Same comment here. Looks also better to me since taking the reference is done 'closer' to virHashAddEntry() call.

obj->def = def;

- return virObjectRef(obj); + cleanup: + virObjectRWUnlock(nwfilters); + return obj; + + error: + virObjectRWUnlock(obj); + virObjectUnref(obj); + virObjectRWUnlock(nwfilters); + return NULL; }

+struct virNWFilterCountData { + virConnectPtr conn; + virNWFilterObjListFilter filter; + int nelems; +}; + +static int +virNWFilterObjListNumOfNWFiltersCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + struct virNWFilterCountData *data = opaque; + + virObjectRWLockRead(obj); + if (!data->filter || data->filter(data->conn, obj->def)) + data->nelems++; + virObjectRWUnlock(obj); + return 0; +} + int virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters, virConnectPtr conn, virNWFilterObjListFilter filter) { - size_t i; - int nfilters = 0; + struct virNWFilterCountData data = { .conn = conn, + .filter = filter, .nelems = 0 };

style: one of these per line ?

- for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virObjectRWLockRead(obj); - if (!filter || filter(conn, obj->def)) - nfilters++; - virObjectRWUnlock(obj); + virObjectRWLockRead(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListNumOfNWFiltersCallback, + &data); + virObjectRWUnlock(nwfilters); + + return data.nelems; +} + + +struct virNWFilterListData { + virConnectPtr conn; + virNWFilterObjListFilter filter; + int nelems; + char **elems; + int maxelems; + bool error; +}; + +static int +virNWFilterObjListGetNamesCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + virNWFilterDefPtr def; + struct virNWFilterListData *data = opaque; + + if (data->error) + return 0; + + if (data->maxelems >= 0 && data->nelems == data->maxelems) + return 0; + + virObjectRWLockRead(obj); + def = obj->def; + + if (!data->filter || data->filter(data->conn, def)) { + if (VIR_STRDUP(data->elems[data->nelems], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nelems++; }

- return nfilters; + cleanup: + virObjectRWUnlock(obj); + return 0; }

@@ -499,82 +634,103 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters, char **const names, int maxnames) { - int nnames = 0; - size_t i; - virNWFilterDefPtr def; + struct virNWFilterListData data = { .conn = conn, .filter = filter, + .nelems = 0, .elems = names, .maxelems = maxnames, .error = false };

- for (i = 0; i < nwfilters->count && nnames < maxnames; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virObjectRWLockRead(obj); - def = obj->def; - if (!filter || filter(conn, def)) { - if (VIR_STRDUP(names[nnames], def->name) < 0) { - virObjectRWUnlock(obj); - goto failure; - } - nnames++; - } - virObjectRWUnlock(obj); - } + virObjectRWLockRead(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListGetNamesCallback, &data); + virObjectRWUnlock(nwfilters);

- return nnames; + if (data.error) + goto error;

- failure: - while (--nnames >= 0) - VIR_FREE(names[nnames]); + return data.nelems;

+ error: + while (--data.nelems >= 0) + VIR_FREE(data.elems[data.nelems]); return -1; }

+struct virNWFilterExportData { + virConnectPtr conn; + virNWFilterObjListFilter filter; + virNWFilterPtr *filters; + int nfilters; + bool error; +}; + +static int +virNWFilterObjListExportCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + virNWFilterDefPtr def; +

nit: indentation error

+ struct virNWFilterExportData *data = opaque; + virNWFilterPtr nwfilter; + + if (data->error) + return 0; + + virObjectRWLockRead(obj); + def = obj->def; + + if (data->filter && !data->filter(data->conn, def)) + goto cleanup; + + if (!data->filters) { + data->nfilters++; + goto cleanup; + } + + if (!(nwfilter = virGetNWFilter(data->conn, def->name, def->uuid))) { + data->error = true; + goto cleanup; + } + data->filters[data->nfilters++] = nwfilter; + + cleanup: + virObjectRWUnlock(obj); + return 0; +} + + int virNWFilterObjListExport(virConnectPtr conn, virNWFilterObjListPtr nwfilters, virNWFilterPtr **filters, virNWFilterObjListFilter filter) { - virNWFilterPtr *tmp_filters = NULL; - int nfilters = 0; - virNWFilterPtr nwfilter = NULL; - virNWFilterObjPtr obj = NULL; - virNWFilterDefPtr def; - size_t i; - int ret = -1; + struct virNWFilterExportData data = { .conn = conn, .filter = filter, + .filters = NULL, .nfilters = 0, .error = false };

- if (!filters) { - ret = nwfilters->count; - goto cleanup; + virObjectRWLockRead(nwfilters); + if (filters && + VIR_ALLOC_N(data.filters, virHashSize(nwfilters->objs) + 1) < 0) { + virObjectRWUnlock(nwfilters); + return -1; }

- if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0) - goto cleanup; + virHashForEach(nwfilters->objs, virNWFilterObjListExportCallback, &data); + virObjectRWUnlock(nwfilters);

- for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; - virObjectRWLockRead(obj); - def = obj->def; - if (!filter || filter(conn, def)) { - if (!(nwfilter = virGetNWFilter(conn, def->name, def->uuid))) { - virObjectRWUnlock(obj); - goto cleanup; - } - tmp_filters[nfilters++] = nwfilter; - } - virObjectRWUnlock(obj); + if (data.error) + goto cleanup; + + if (data.filters) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(data.filters, data.nfilters + 1));

can we ignore errors here and not return -1 in case an error occurs ?

+ *filters = data.filters; }

- *filters = tmp_filters; - tmp_filters = NULL; - ret = nfilters; + return data.nfilters;

cleanup: - if (tmp_filters) { - for (i = 0; i < nfilters; i ++) - virObjectUnref(tmp_filters[i]); - } - VIR_FREE(tmp_filters); - - return ret; + virObjectListFree(data.filters); + return -1; }

diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 0281bc5f5..cabb42a71 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -65,7 +65,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,

virNWFilterObjPtr virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, - const unsigned char *uuid); + const char *uuidstr);

virNWFilterObjPtr virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index b38740b15..b24f59379 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -369,11 +369,10 @@ nwfilterObjFromNWFilter(const unsigned char *uuid) virNWFilterObjPtr obj; char uuidstr[VIR_UUID_STRING_BUFLEN];

- if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuid))) { - virUUIDFormat(uuid, uuidstr); + virUUIDFormat(uuid, uuidstr); + if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuidstr))) virReportError(VIR_ERR_NO_NWFILTER, _("no nwfilter with matching uuid '%s'"), uuidstr); - } return obj; }

On 02/02/2018 02:33 PM, John Ferlan wrote:

...

On 12/08/2017 09:01 AM, John Ferlan wrote:

...

Implement the self locking object list for nwfilter object lists that uses two hash tables to store the nwfilter object by UUID or by Name.

As part of this alter the uuid argument to virNWFilterObjLookupByUUID to expect an already formatted uuidstr.

Alter the existing list traversal code to implement the hash table find/lookup/search functionality.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 402 ++++++++++++++++++++++++++++------------- src/conf/virnwfilterobj.h | 2 +- src/nwfilter/nwfilter_driver.c | 5 +- 3 files changed, 282 insertions(+), 127 deletions(-)

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 6b4758656..a4e6a03d2 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -43,12 +43,21 @@ struct _virNWFilterObj { };

struct _virNWFilterObjList { - size_t count; - virNWFilterObjPtr *objs; + virObjectRWLockable parent; + + /* uuid string -> virNWFilterObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; + + /* name -> virNWFilterObj mapping for O(1), + * lockless lookup-by-name */ + virHashTable *objsName; };

static virClassPtr virNWFilterObjClass; +static virClassPtr virNWFilterObjListClass; static void virNWFilterObjDispose(void *opaque); +static void virNWFilterObjListDispose(void *opaque);

static int @@ -60,6 +69,12 @@ virNWFilterObjOnceInit(void) virNWFilterObjDispose))) return -1;

+ if (!(virNWFilterObjListClass = virClassNew(virClassForObjectRWLockable(), + "virNWFilterObjList", + sizeof(virNWFilterObjList), + virNWFilterObjListDispose))) + return -1; + return 0; }

@@ -144,14 +159,20 @@ virNWFilterObjDispose(void *opaque) }

+static void +virNWFilterObjListDispose(void *opaque) +{ + virNWFilterObjListPtr nwfilters = opaque; + + virHashFree(nwfilters->objs); + virHashFree(nwfilters->objsName); +} + + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { - size_t i; - for (i = 0; i < nwfilters->count; i++) - virObjectUnref(nwfilters->objs[i]); - VIR_FREE(nwfilters->objs); - VIR_FREE(nwfilters); + virObjectUnref(nwfilters); }

@@ -160,8 +181,23 @@ virNWFilterObjListNew(void) { virNWFilterObjListPtr nwfilters;

- if (VIR_ALLOC(nwfilters) < 0) + if (virNWFilterObjInitialize() < 0) + return NULL; + + if (!(nwfilters = virObjectRWLockableNew(virNWFilterObjListClass))) + return NULL; + + if (!(nwfilters->objs = virHashCreate(10, virObjectFreeHashData))) { [1] As part of the magic of hash tables, when an element is removed from

On 01/31/2018 10:10 PM, Stefan Berger wrote: the hash table the virObjectFreeHashData is called which does a virObjectUnref...

Ah. I was looking for 'symmetry' ... so ideally we would have automatic increase of the refcount as well. Stefan