On 06/02/2017 12:25 PM, John Ferlan wrote: s/,/ fails,/ This @objdef variable looks redundant to me. Can't we access obj->def directly? Esp. since you're introducing a variable just for a two times use. Then again, we often use obj->def->... when it comes to domain objects, why not here? ACK if you drop the @objdef variable. Michal

On 07/14/2017 01:50 AM, John Ferlan wrote: Well, then don't run the following: libvirt.git $ git grep -np "\(\w\+\(->\|\.\)\)\{10\}\w\+" I mean, that's one extreme, but we usually have "vm->def->name" so obj->def->uuid_specified shouldn't be a problem. But I don't care that much. So your call after all. Michal

After returning from virNWFilterObjListAssignDef the @obj is locked; however, if virNWFilterSaveConfig fails to save the generated UUID the code jumped to error and returns NULL meaning the caller will not call virNWFilterObjUnlock on the object leaving the object locked on list and ripe for a deadlock on any subsequent Find of all objects or object removal. So rather than jumping to error alter the comment prior to the virNWFilterSaveConfig and just provide a VIR_INFO message for anyone that cares, but allow the code to continue and a subsequent LoadConfig to once again attempt the save of a newly generated UUID. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 3fb6046..0343c0a 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -507,10 +507,13 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, def = NULL; objdef = obj->def; - /* We generated a UUID, make it permanent by saving the config to disk */ + /* We generated a UUID, atttempt to make it permanent by saving the + * config to disk. If not successful, no need to fail or remove the + * object as a future load would regenerate a UUID and try again, + * but the existing config would still exist and can be used. */ if (!objdef->uuid_specified && virNWFilterSaveConfig(configDir, objdef) < 0) - goto error; + VIR_INFO("failed to save generated UUID for filter '%s'", objdef->name); VIR_FREE(configFile); return obj; -- 2.9.4

On 07/13/2017 10:41 AM, Michal Privoznik wrote: IIRC: this one's a bit weird since if the UUID doesn't exist we "can" generate one. If we generate one, then we should save it. However, failing to save shouldn't be called an error because having a UUID isn't required it's just something we try to "enforce" at some point in time of the nwfilter. I kind of didn't want to "adjust" that logic. That's a different patch ;-> John

On 07/14/2017 08:09 AM, Michal Privoznik wrote: Ahhh, I see. Interesting commit... Then of course there's b3e71a8830 which is the actual self inflicted shot. I need to revert that one ASAP (and do the same for the 3.3, 3.4, and 3.5 -maint trees because there's an awful double free bug for @def). John

On 06/02/2017 12:25 PM, John Ferlan wrote: How can this work? objdef is pointer to def not obj. Anyway, since we are not going to save config here: ACK Michal

On 07/13/2017 10:41 AM, Michal Privoznik wrote: Since patches 2 & 3 are going to be dropped and patch 6 is deemed unnecessary, I'm going to drop this one too. John

No need to goto cleanup and check "if (obj)" if we know (obj) isn't there, so just return immediately. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/nwfilter/nwfilter_driver.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index c740c53..d527769 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -391,7 +391,7 @@ nwfilterLookupByUUID(virConnectPtr conn, nwfilterDriverUnlock(); if (!obj) - goto cleanup; + return NULL; def = virNWFilterObjGetDef(obj); if (virNWFilterLookupByUUIDEnsureACL(conn, def) < 0) @@ -400,8 +400,7 @@ nwfilterLookupByUUID(virConnectPtr conn, ret = virGetNWFilter(conn, def->name, def->uuid); cleanup: - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjUnlock(obj); return ret; } @@ -421,7 +420,7 @@ nwfilterLookupByName(virConnectPtr conn, if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, _("no nwfilter with matching name '%s'"), name); - goto cleanup; + return NULL; } def = virNWFilterObjGetDef(obj); @@ -431,8 +430,7 @@ nwfilterLookupByName(virConnectPtr conn, ret = virGetNWFilter(conn, def->name, def->uuid); cleanup: - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjUnlock(obj); return ret; } @@ -595,7 +593,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, nwfilterDriverUnlock(); if (!obj) - goto cleanup; + return NULL; def = virNWFilterObjGetDef(obj); if (virNWFilterGetXMLDescEnsureACL(nwfilter->conn, def) < 0) @@ -604,8 +602,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, ret = virNWFilterDefFormat(def); cleanup: - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjUnlock(obj); return ret; } -- 2.9.4

Use @nwfilter always for the name Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/nwfilter/nwfilter_driver.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index d527769..7db4f87 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -384,7 +384,7 @@ nwfilterLookupByUUID(virConnectPtr conn, { virNWFilterObjPtr obj; virNWFilterDefPtr def; - virNWFilterPtr ret = NULL; + virNWFilterPtr nwfilter = NULL; nwfilterDriverLock(); obj = nwfilterObjFromNWFilter(uuid); @@ -397,11 +397,11 @@ nwfilterLookupByUUID(virConnectPtr conn, if (virNWFilterLookupByUUIDEnsureACL(conn, def) < 0) goto cleanup; - ret = virGetNWFilter(conn, def->name, def->uuid); + nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: virNWFilterObjUnlock(obj); - return ret; + return nwfilter; } @@ -411,7 +411,7 @@ nwfilterLookupByName(virConnectPtr conn, { virNWFilterObjPtr obj; virNWFilterDefPtr def; - virNWFilterPtr ret = NULL; + virNWFilterPtr nwfilter = NULL; nwfilterDriverLock(); obj = virNWFilterObjListFindByName(driver->nwfilters, name); @@ -427,11 +427,11 @@ nwfilterLookupByName(virConnectPtr conn, if (virNWFilterLookupByNameEnsureACL(conn, def) < 0) goto cleanup; - ret = virGetNWFilter(conn, def->name, def->uuid); + nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: virNWFilterObjUnlock(obj); - return ret; + return nwfilter; } @@ -467,7 +467,7 @@ nwfilterConnectListNWFilters(virConnectPtr conn, static int nwfilterConnectListAllNWFilters(virConnectPtr conn, - virNWFilterPtr **filters, + virNWFilterPtr **nwfilters, unsigned int flags) { int ret; @@ -478,7 +478,7 @@ nwfilterConnectListAllNWFilters(virConnectPtr conn, return -1; nwfilterDriverLock(); - ret = virNWFilterObjListExport(conn, driver->nwfilters, filters, + ret = virNWFilterObjListExport(conn, driver->nwfilters, nwfilters, virConnectListAllNWFiltersCheckACL); nwfilterDriverUnlock(); @@ -492,7 +492,7 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterDefPtr def; virNWFilterObjPtr obj = NULL; virNWFilterDefPtr objdef; - virNWFilterPtr ret = NULL; + virNWFilterPtr nwfilter = NULL; if (!driver->privileged) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -521,7 +521,7 @@ nwfilterDefineXML(virConnectPtr conn, goto cleanup; } - ret = virGetNWFilter(conn, objdef->name, objdef->uuid); + nwfilter = virGetNWFilter(conn, objdef->name, objdef->uuid); cleanup: virNWFilterDefFree(def); @@ -531,7 +531,7 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(); - return ret; + return nwfilter; } -- 2.9.4

Rename to virNWFilterDoInstantiate to better describe the action. Also fix the @vmuuid parameter to not have the ATTRIBUTE_UNUSED since it is used in the call to virNWFilterDHCPSnoopReq. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/nwfilter/nwfilter_gentech_driver.c | 46 +++++++++++++++------------------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 82e20de..9c11cb3 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -59,7 +59,7 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { /* Serializes instantiation of filters. This is necessary * to avoid lock ordering deadlocks. eg __virNWFilterInstantiateFilter * will hold a lock on a virNWFilterObjPtr. This in turn invokes - * virNWFilterInstantiate which invokes virNWFilterDetermineMissingVarsRec + * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec * which invokes virNWFilterObjListFindByName. This iterates over every single * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a * filter in parallel, they'll both hold 1 lock at the top level in @@ -604,7 +604,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, /** - * virNWFilterInstantiate: + * virNWFilterDoInstantiate: * @vmuuid: The UUID of the VM * @techdriver: The driver to use for instantiation * @filter: The filter to instantiate @@ -624,18 +624,19 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, * Call this function while holding the NWFilter filter update lock */ static int -virNWFilterInstantiate(const unsigned char *vmuuid ATTRIBUTE_UNUSED, - virNWFilterTechDriverPtr techdriver, - virNWFilterDefPtr filter, - const char *ifname, - int ifindex, - const char *linkdev, - virNWFilterHashTablePtr vars, - enum instCase useNewFilter, bool *foundNewFilter, - bool teardownOld, - const virMacAddr *macaddr, - virNWFilterDriverStatePtr driver, - bool forceWithPendingReq) +virNWFilterDoInstantiate(const unsigned char *vmuuid, + virNWFilterTechDriverPtr techdriver, + virNWFilterDefPtr filter, + const char *ifname, + int ifindex, + const char *linkdev, + virNWFilterHashTablePtr vars, + enum instCase useNewFilter, + bool *foundNewFilter, + bool teardownOld, + const virMacAddr *macaddr, + virNWFilterDriverStatePtr driver, + bool forceWithPendingReq) { int rc; virNWFilterInst inst; @@ -867,18 +868,11 @@ __virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, break; } - rc = virNWFilterInstantiate(vmuuid, - techdriver, - filter, - ifname, - ifindex, - linkdev, - vars, - useNewFilter, foundNewFilter, - teardownOld, - macaddr, - driver, - forceWithPendingReq); + rc = virNWFilterDoInstantiate(vmuuid, techdriver, filter, + ifname, ifindex, linkdev, + vars, useNewFilter, foundNewFilter, + teardownOld, macaddr, driver, + forceWithPendingReq); virNWFilterHashTableFree(vars); -- 2.9.4

Rename to virNWFilterInstantiateFilterUpdate and alter the callers to not have one parameter per line. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/nwfilter/nwfilter_gentech_driver.c | 62 +++++++++++++--------------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 9c11cb3..81b12bb 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -57,13 +57,13 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { }; /* Serializes instantiation of filters. This is necessary - * to avoid lock ordering deadlocks. eg __virNWFilterInstantiateFilter + * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate * will hold a lock on a virNWFilterObjPtr. This in turn invokes * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec * which invokes virNWFilterObjListFindByName. This iterates over every single * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a * filter in parallel, they'll both hold 1 lock at the top level in - * __virNWFilterInstantiateFilter which will cause the other thread + * virNWFilterInstantiateFilterUpdate which will cause the other thread * to deadlock in virNWFilterObjListFindByName. * * XXX better long term solution is to make virNWFilterObjList use a @@ -776,18 +776,18 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, * Call this function while holding the NWFilter filter update lock */ static int -__virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - bool teardownOld, - const char *ifname, - int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virNWFilterHashTablePtr filterparams, - enum instCase useNewFilter, - bool forceWithPendingReq, - bool *foundNewFilter) +virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, + const unsigned char *vmuuid, + bool teardownOld, + const char *ifname, + int ifindex, + const char *linkdev, + const virMacAddr *macaddr, + const char *filtername, + virNWFilterHashTablePtr filterparams, + enum instCase useNewFilter, + bool forceWithPendingReq, + bool *foundNewFilter) { int rc; const char *drvname = EBIPTABLES_DRIVER_ID; @@ -917,18 +917,11 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, goto cleanup; } - rc = __virNWFilterInstantiateFilter(driver, - vmuuid, - teardownOld, - net->ifname, - ifindex, - linkdev, - &net->mac, - net->filter, - net->filterparams, - useNewFilter, - false, - foundNewFilter); + rc = virNWFilterInstantiateFilterUpdate(driver, vmuuid, teardownOld, + net->ifname, ifindex, linkdev, + &net->mac, net->filter, + net->filterparams, useNewFilter, + false, foundNewFilter); cleanup: virMutexUnlock(&updateMutex); @@ -953,18 +946,11 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, virNWFilterReadLockFilterUpdates(); virMutexLock(&updateMutex); - rc = __virNWFilterInstantiateFilter(driver, - vmuuid, - true, - ifname, - ifindex, - linkdev, - macaddr, - filtername, - filterparams, - INSTANTIATE_ALWAYS, - true, - &foundNewFilter); + rc = virNWFilterInstantiateFilterUpdate(driver, vmuuid, true, + ifname, ifindex, linkdev, + macaddr, filtername, filterparams, + INSTANTIATE_ALWAYS, true, + &foundNewFilter); if (rc < 0) { /* something went wrong... 'DOWN' the interface */ if ((virNetDevValidateConfig(ifname, NULL, ifindex) <= 0) || -- 2.9.4

On 06/02/2017 12:25 PM, John Ferlan wrote: No, please move the function call onto the same line as if(!(obj =. 83 chars long line a not EOW (as we know it) Nor 84 chars long line. ACK if you fix it. Michal

On 06/02/2017 12:25 PM, John Ferlan wrote: Technically, this doesn't need to be virAtomic. Bare assignment would work as: a) the object is locked at this point, b) there's no other reference to the object (we are just creating the first one). But I'm fine with leaving this as is, just wanted to point out my comment. This can hardly work. The condition needs to be inverted. Can we remove this "return NULL" line and rely on "return obj" which follow immediately (not to be seen in the context here though)? I've got an unrelated question: here, after this line you can find the following code: if (virNWFilterDefEqual(def, objdef, false)) { virNWFilterDefFree(objdef); obj->def = def; return obj; } Firstly, I think we can s/false/true/ because if UUIDs were not the same, we would have errored out way sooner. But more importantly, if definition is equal what's the point in replacing it? ACK Michal

Perhaps a bit out of order from the normal convert driver object to virObjectLockable, then convert the driver object list. However, as it turns out nwfilter objects have been using a recursive mutex for which the common virObject code does not want to use. So, if we convert the nwfilter object list to use virObjectLockable, then it will be more possible to make the necessary adjustments to the virNWFilterObjListFindInstantiateFilter algorithm in order to handle a recursive lock operation required as part of the <rule> and <filterref> (or "inc" list) processing. This patch takes the plunge, modifying the nwfilter object list handling code to utilize hash tables for both the UUID and Name for which an nwfilter def would utilize. This makes lookup by either "key" possible without needing to first lock the object in order to find a match as long as of course the object list itself is locked. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/virnwfilterobj.c | 395 +++++++++++++++++++++++++++++------------ src/nwfilter/nwfilter_driver.c | 4 + 2 files changed, 286 insertions(+), 113 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 99be59c..84ef7d1 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -53,10 +53,34 @@ struct _virNWFilterObj { }; struct _virNWFilterObjList { - size_t count; - virNWFilterObjPtr *objs; + virObjectLockable 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 virNWFilterObjListClass; +static void virNWFilterObjListDispose(void *opaque); + +static int +virNWFilterObjOnceInit(void) +{ + if (!(virNWFilterObjListClass = virClassNew(virClassForObjectLockable(), + "virNWFilterObjList", + sizeof(virNWFilterObjList), + virNWFilterObjListDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNWFilterObj) + static virNWFilterObjPtr virNWFilterObjNew(virNWFilterDefPtr def) @@ -151,14 +175,30 @@ virNWFilterObjUnref(virNWFilterObjPtr obj) } +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++) - virNWFilterObjUnref(nwfilters->objs[i]); - VIR_FREE(nwfilters->objs); - VIR_FREE(nwfilters); + virObjectUnref(nwfilters); +} + + +static void +virNWFilterObjListObjsFreeData(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + virNWFilterObjPtr obj = payload; + + virNWFilterObjUnref(obj); } @@ -167,8 +207,24 @@ virNWFilterObjListNew(void) { virNWFilterObjListPtr nwfilters; - if (VIR_ALLOC(nwfilters) < 0) + if (virNWFilterObjInitialize() < 0) + return NULL; + + if (!(nwfilters = virObjectLockableNew(virNWFilterObjListClass))) + return NULL; + + if (!(nwfilters->objs = virHashCreate(10, virNWFilterObjListObjsFreeData))) { + virObjectUnref(nwfilters); + return NULL; + } + + if (!(nwfilters->objsName = + virHashCreate(10, virNWFilterObjListObjsFreeData))) { + virHashFree(nwfilters->objs); + virObjectUnref(nwfilters); return NULL; + } + return nwfilters; } @@ -177,21 +233,34 @@ void virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj) { - size_t i; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virNWFilterDefPtr def; + if (!obj) + return; + def = obj->def; + + virUUIDFormat(def->uuid, uuidstr); + virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); + virObjectLock(nwfilters); + virNWFilterObjLock(obj); + virHashRemoveEntry(nwfilters->objs, uuidstr); + virHashRemoveEntry(nwfilters->objsName, def->name); + virNWFilterObjUnlock(obj); + virNWFilterObjUnref(obj); + virObjectUnlock(nwfilters); +} - for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjLock(nwfilters->objs[i]); - if (nwfilters->objs[i] == obj) { - virNWFilterObjUnlock(nwfilters->objs[i]); - virNWFilterObjUnref(nwfilters->objs[i]); - VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); - break; - } - virNWFilterObjUnlock(nwfilters->objs[i]); - } +static virNWFilterObjPtr +virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters, + const unsigned char *uuid) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(uuid, uuidstr); + return virNWFilterObjRef(virHashLookup(nwfilters->objs, uuidstr)); } @@ -199,20 +268,22 @@ virNWFilterObjPtr virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, const unsigned char *uuid) { - size_t i; virNWFilterObjPtr obj; - virNWFilterDefPtr def; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; + virObjectLock(nwfilters); + obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuid); + virObjectUnlock(nwfilters); + if (obj) virNWFilterObjLock(obj); - def = obj->def; - if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return virNWFilterObjRef(obj); - virNWFilterObjUnlock(obj); - } + return obj; +} - return NULL; + +static virNWFilterObjPtr +virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters, + const char *name) +{ + return virNWFilterObjRef(virHashLookup(nwfilters->objsName, name)); } @@ -220,20 +291,15 @@ 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]; + virObjectLock(nwfilters); + obj = virNWFilterObjListFindByNameLocked(nwfilters, name); + virObjectUnlock(nwfilters); + if (obj) virNWFilterObjLock(obj); - def = obj->def; - if (STREQ_NULLABLE(def->name, name)) - return virNWFilterObjRef(obj); - virNWFilterObjUnlock(obj); - } - return NULL; + return obj; } @@ -282,9 +348,10 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, break; } - obj = virNWFilterObjListFindByName(nwfilters, - entry->include->filterref); + obj = virNWFilterObjListFindByNameLocked(nwfilters, + entry->include->filterref); if (obj) { + virObjectLock(obj); rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def, filtername); virNWFilterObjEndAPI(&obj); @@ -306,6 +373,8 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, * Detect a loop introduced through the filters being able to * reference each other. * + * NB: Enter with nwfilters locked + * * Returns 0 in case no loop was detected, -1 otherwise. */ static int @@ -371,8 +440,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, { virNWFilterObjPtr obj; virNWFilterDefPtr objdef; + char uuidstr[VIR_UUID_STRING_BUFLEN]; - if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) { + virObjectLock(nwfilters); + + if ((obj = virNWFilterObjListFindByUUIDLocked(nwfilters, def->uuid))) { + virNWFilterObjLock(obj); objdef = obj->def; if (STRNEQ(def->name, objdef->name)) { @@ -381,19 +454,21 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, "('%s') already exists"), objdef->name); virNWFilterObjEndAPI(&obj); + virObjectUnlock(nwfilters); return NULL; } virNWFilterObjEndAPI(&obj); } else { - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { + virNWFilterObjLock(obj); objdef = obj->def; virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); virNWFilterObjEndAPI(&obj); + virObjectUnlock(nwfilters); return NULL; } } @@ -401,16 +476,18 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (virNWFilterObjListDefLoopDetect(nwfilters, def) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("filter would introduce a loop")); + virObjectUnlock(nwfilters); return NULL; } - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { + virNWFilterObjLock(obj); objdef = obj->def; if (virNWFilterDefEqual(def, objdef, false)) { virNWFilterDefFree(objdef); obj->def = def; - return obj; + goto cleanup; } obj->newDef = def; @@ -418,13 +495,14 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (virNWFilterTriggerVMFilterRebuild() < 0) { obj->newDef = NULL; virNWFilterObjEndAPI(&obj); + virObjectUnlock(nwfilters); return NULL; } virNWFilterDefFree(objdef); obj->def = def; obj->newDef = NULL; - return obj; + goto cleanup; } if (!(obj = virNWFilterObjNew(def))) @@ -435,36 +513,107 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (!(obj->configFile = virFileBuildPath(configDir, objdef->name, ".xml"))) goto error; - if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) + virUUIDFormat(def->uuid, uuidstr); + if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0) + goto error; + virNWFilterObjRef(obj); + + if (virHashAddEntry(nwfilters->objsName, objdef->name, obj) < 0) { + virHashRemoveEntry(nwfilters->objs, uuidstr); goto error; + } + virNWFilterObjRef(obj); - return virNWFilterObjRef(obj); + cleanup: + virObjectUnlock(nwfilters); + return obj; error: obj->def = NULL; virNWFilterObjUnlock(obj); virNWFilterObjUnref(obj); + virObjectUnlock(nwfilters); return NULL; } +struct virNWFilterCountData { + virConnectPtr conn; + virNWFilterObjListFilter aclfilter; + int nelems; +}; + +static int +virNWFilterObjListNumOfNWFiltersCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + struct virNWFilterCountData *data = opaque; + + virObjectLock(obj); + if (!data->aclfilter || data->aclfilter(data->conn, obj->def)) + data->nelems++; + virObjectUnlock(obj); + return 0; +} + + int virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters, virConnectPtr conn, virNWFilterObjListFilter aclfilter) { - size_t i; - int nfilters = 0; + struct virNWFilterCountData data = { .conn = conn, + .aclfilter = aclfilter, .nelems = 0 }; - for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); - if (!aclfilter || aclfilter(conn, obj->def)) - nfilters++; - virNWFilterObjUnlock(obj); + virObjectLock(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListNumOfNWFiltersCallback, + &data); + virObjectUnlock(nwfilters); + + return data.nelems; +} + + +struct virNWFilterListData { + virConnectPtr conn; + virNWFilterObjListFilter aclfilter; + 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; + + virObjectLock(obj); + def = obj->def; + + if (!data->aclfilter || data->aclfilter(data->conn, def)) { + if (VIR_STRDUP(data->elems[data->nelems], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nelems++; } - return nfilters; + cleanup: + virObjectUnlock(obj); + return 0; } @@ -475,82 +624,102 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters, char **const names, int maxnames) { - int nnames = 0; - size_t i; - virNWFilterDefPtr def; + struct virNWFilterListData data = { .conn = conn, .aclfilter = aclfilter, + .nelems = 0, .elems = names, .maxelems = maxnames, .error = false }; - for (i = 0; i < nwfilters->count && nnames < maxnames; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); - def = obj->def; - if (!aclfilter || aclfilter(conn, def)) { - if (VIR_STRDUP(names[nnames], def->name) < 0) { - virNWFilterObjUnlock(obj); - goto failure; - } - nnames++; - } - virNWFilterObjUnlock(obj); - } + virObjectLock(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListGetNamesCallback, &data); + virObjectUnlock(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; } -int -virNWFilterObjListExport(virConnectPtr conn, - virNWFilterObjListPtr nwfilters, - virNWFilterPtr **filters, - virNWFilterObjListFilter aclfilter) +struct virNWFilterExportData { + virConnectPtr conn; + virNWFilterObjListFilter aclfilter; + virNWFilterPtr *filters; + int nfilters; + bool error; +}; + +static int +virNWFilterObjListExportCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) { - virNWFilterPtr *tmp_filters = NULL; - int nfilters = 0; - virNWFilterPtr filter = NULL; - virNWFilterObjPtr obj = NULL; + virNWFilterObjPtr obj = payload; virNWFilterDefPtr def; - size_t i; - int ret = -1; + struct virNWFilterExportData *data = opaque; + virNWFilterPtr nwfilter; + + if (data->error) + return 0; + + virObjectLock(obj); + def = obj->def; - if (!filters) { - ret = nwfilters->count; + if (data->aclfilter && !data->aclfilter(data->conn, def)) + goto cleanup; + + if (!data->filters) { + data->nfilters++; goto cleanup; } - if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0) + if (!(nwfilter = virGetNWFilter(data->conn, def->name, def->uuid))) { + data->error = true; goto cleanup; + } + data->filters[data->nfilters++] = nwfilter; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); - def = obj->def; - if (!aclfilter || aclfilter(conn, def)) { - if (!(filter = virGetNWFilter(conn, def->name, def->uuid))) { - virNWFilterObjUnlock(obj); - goto cleanup; - } - tmp_filters[nfilters++] = filter; - } - virNWFilterObjUnlock(obj); + cleanup: + virObjectUnlock(obj); + return 0; +} + + +int +virNWFilterObjListExport(virConnectPtr conn, + virNWFilterObjListPtr nwfilters, + virNWFilterPtr **filters, + virNWFilterObjListFilter aclfilter) +{ + struct virNWFilterExportData data = { .conn = conn, .aclfilter = aclfilter, + .filters = NULL, .nfilters = 0, .error = false }; + + virObjectLock(nwfilters); + if (filters && + VIR_ALLOC_N(data.filters, virHashSize(nwfilters->objs) + 1) < 0) { + virObjectUnlock(nwfilters); + return -1; } - *filters = tmp_filters; - tmp_filters = NULL; - ret = nfilters; + virHashForEach(nwfilters->objs, virNWFilterObjListExportCallback, &data); + virObjectUnlock(nwfilters); - cleanup: - if (tmp_filters) { - for (i = 0; i < nfilters; i ++) - virObjectUnref(tmp_filters[i]); + 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)); + *filters = data.filters; } - VIR_FREE(tmp_filters); - return ret; + return data.nfilters; + + cleanup: + virObjectListFree(data.filters); + return -1; } diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index b6e11e6..3f05797 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -518,6 +518,8 @@ nwfilterDefineXML(virConnectPtr conn, if (virNWFilterObjSaveConfig(obj) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); + virNWFilterObjUnref(obj); + obj = NULL; goto cleanup; } @@ -563,6 +565,8 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; virNWFilterObjListRemove(driver->nwfilters, obj); + virNWFilterObjUnref(obj); + obj = NULL; ret = 0; cleanup: -- 2.9.4

The current algorithm required usage of recursive locks because there was no other mechanism available to ensure that the object wasn't deleted whilst the instantiation was taking place. Now that we have object references, lets use those to ensure the object isn't deleted whilst we're working through the instantiation thus removing the need for recursive locks. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/virnwfilterobj.c | 13 +++++++++-- src/nwfilter/nwfilter_gentech_driver.c | 40 +++++++++++++++++++--------------- 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 84ef7d1..ea1323d 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -303,13 +303,22 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, } +/** + * To avoid the need to have recursive locks as a result of how the + * virNWFilterInstantiateFilter processing works, this API will not + * lock the returned object, instead just increase the refcnt of the + * object to the caller to allow the caller to handle. + */ virNWFilterObjPtr virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, const char *filtername) { virNWFilterObjPtr obj; - if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) { + virObjectLock(nwfilters); + obj = virNWFilterObjListFindByNameLocked(nwfilters, filtername); + virObjectUnlock(nwfilters); + if (!obj) { virReportError(VIR_ERR_INTERNAL_ERROR, _("referenced filter '%s' is missing"), filtername); return NULL; @@ -318,7 +327,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); return NULL; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 4c228ea..40da4cc 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -56,19 +56,25 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { NULL }; -/* Serializes instantiation of filters. This is necessary - * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate - * will hold a lock on a virNWFilterObjPtr. This in turn invokes - * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec - * which invokes virNWFilterObjListFindInstantiateFilter. This iterates over - * every single virNWFilterObjPtr in the list. So if 2 threads try to - * instantiate a filter in parallel, they'll both hold 1 lock at the top level - * in virNWFilterInstantiateFilterUpdate which will cause the other thread - * to deadlock in virNWFilterObjListFindInstantiateFilter. +/* NB: Usage of virNWFilterObjListFindInstantiateFilter will only not lock + * the returned object - only increase the refcnt. This is necessary to avoid + * lock ordering deadlocks or the need for recursive locks for filter objects. + * Since the objects are only used to access the @def for <filterref> and + * <rule> elements and having the @def disappear would not be good, but + * keeping the locks causes too many problems. * - * XXX better long term solution is to make virNWFilterObjList use a - * hash table as is done for virDomainObjList. You can then get - * lockless lookup of objects by name. + * During virNWFilterDefToInst processing an object is fetched and placed + * into the inst->filters lookaside list during virNWFilterIncludeDefToRuleInst + * processing which then recursively calls virNWFilterDefToInst. When the + * object is removed during virNWFilterInstReset, the refcnt is decremented. + * + * Since virNWFilterDefToInst is called during virNWFilterDoInstantiate + * processing which can also invoke virNWFilterDetermineMissingVarsRec + * which invokes virNWFilterObjListFindInstantiateFilter also taking + * reference. In addition the virNWFilterInstantiateFilterUpdate could + * have caused locking collisions, so removing the locks from the + * equation and replacing with just references ensures that the object + * cannot be removed whilst this instantiation is taking place. */ static virMutex updateMutex; @@ -315,7 +321,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst) size_t i; for (i = 0; i < inst->nfilters; i++) - virNWFilterObjEndAPI(&inst->filters[i]); + virNWFilterObjUnref(inst->filters[i]); VIR_FREE(inst->filters); inst->nfilters = 0; @@ -425,7 +431,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, if (ret < 0) virNWFilterInstReset(inst); virNWFilterHashTableFree(tmpvars); - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); return ret; } @@ -546,7 +552,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, vars); if (!tmpvars) { rc = -1; - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); break; } @@ -570,7 +576,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterHashTableFree(tmpvars); - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); if (rc < 0) break; } @@ -844,7 +850,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, virNWFilterHashTableFree(vars1); err_exit: - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); VIR_FREE(str_ipaddr); VIR_FREE(str_macaddr); -- 2.9.4

On 07/13/2017 10:40 AM, Michal Privoznik wrote: I wish this were fresher in my mind ;-)... But, IIRC the problem is that without recursive locks and without being able to lock the hash table (e.g. nwfilters list), then there was no way to find the filter by Name because we'd already have the lock. So my proposed solution is to skip locking altogether and use object references in order to perform the searches. But of course this points out the tragic flaw in the process. What if I introduce something that uses pthread_mutex_trylock. For the "short term" it's no different than pthread_mutex_lock since this is still a recursive lock. The trylock would only be called from this path thus further localizing the issue. Once converting from a recursive to a normal lock, the code would need to handle the EBUSY return indicating the lock was already taken. Since we don't know if it was taken by ourselves or some other thread, then devise a mechanism to ensure no other thread could attempt to lock whilst the processing for virNWFilterObjListFindInstantiateFilter occurs. Of course I'm typing this without having the entire algorithm in mind right now. I'll focus some more on it and hopefully come up with something so that the v2 would be "more correct". Tks- John