On 05/15/2018 01:43 PM, Daniel P. Berrangé wrote:
Now that the nwfilter driver keeps a list of bindings that it has created, there is no need for the complex virt driver callbacks. It is possible to simply iterate of the list of recorded filter bindings.
This means that rebuilding filters no longer has to acquire any locks on the virDomainObj objects, as they're never touched.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/nwfilter_conf.c | 169 +++++++------------------ src/conf/nwfilter_conf.h | 51 +------- src/conf/virnwfilterobj.c | 4 +- src/libvirt_private.syms | 7 +- src/lxc/lxc_driver.c | 28 ---- src/nwfilter/nwfilter_driver.c | 21 +-- src/nwfilter/nwfilter_gentech_driver.c | 164 +++++++++++++++--------- src/nwfilter/nwfilter_gentech_driver.h | 4 +- src/qemu/qemu_driver.c | 25 ---- src/uml/uml_driver.c | 29 ----- 10 files changed, 173 insertions(+), 329 deletions(-)
Oh and this is where the magic happens ;-)... and there is light at the end of this long tunnel! [...]
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index de26a6d034..29aacba98d 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c
[...]
+static virNWFilterTriggerRebuildCallback rebuildCallback; +static void *rebuildOpaque;
int -virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, +virNWFilterConfLayerInit(virNWFilterTriggerRebuildCallback cb, void *opaque) { if (initialized) return -1;
- virNWFilterDomainFWUpdateCB = domUpdateCB; - virNWFilterDomainFWUpdateOpaque = opaque; + rebuildCallback = cb; + rebuildOpaque = opaque;
initialized = true;
@@ -3233,8 +3120,50 @@ virNWFilterConfLayerShutdown(void) virRWLockDestroy(&updateLock);
initialized = false; - virNWFilterDomainFWUpdateOpaque = NULL; - virNWFilterDomainFWUpdateCB = NULL; + rebuildCallback = NULL; + rebuildOpaque = NULL; +} +
Two blank lines
+int +virNWFilterTriggerRebuild(void) +{ +#if 0
Did you mean to keep the #if 0 - just in case?
+ size_t i; + int ret = 0; + struct domUpdateCBStruct cb = { + .opaque = virNWFilterDomainFWUpdateOpaque, + .step = STEP_APPLY_NEW, + .skipInterfaces = virHashCreate(0, NULL), + }; + + if (!cb.skipInterfaces) + return -1; + + for (i = 0; i < nCallbackDriver; i++) { + if (callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, + &cb) < 0) + ret = -1; + } + + if (ret < 0) { + cb.step = STEP_TEAR_NEW; /* rollback */ + + for (i = 0; i < nCallbackDriver; i++) + callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, + &cb); + } else { + cb.step = STEP_TEAR_OLD; /* switch over */ + + for (i = 0; i < nCallbackDriver; i++) + callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, + &cb); + } + + virHashFree(cb.skipInterfaces); + + return ret; +#endif + return rebuildCallback(rebuildOpaque); }
[...]
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 30ae3970fb..de7361f3dd 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -153,9 +153,9 @@ virNWFilterVarHashmapAddStdValues(virHashTablePtr table, if (!val) return -1;
- if (virHashAddEntry(table, - NWFILTER_STD_VAR_MAC, - val) < 0) { + if (virHashUpdateEntry(table, + NWFILTER_STD_VAR_MAC, + val) < 0) { virNWFilterVarValueFree(val); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add variable 'MAC' to hashmap")); @@ -168,9 +168,9 @@ virNWFilterVarHashmapAddStdValues(virHashTablePtr table, if (!val) return -1;
- if (virHashAddEntry(table, - NWFILTER_STD_VAR_IP, - val) < 0) { + if (virHashUpdateEntry(table, + NWFILTER_STD_VAR_IP, + val) < 0) { virNWFilterVarValueFree(val); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add variable 'IP' to hashmap")); @@ -1000,68 +1000,110 @@ virNWFilterTeardownFilter(virNWFilterBindingDefPtr binding) return ret; }
+enum { + STEP_APPLY_NEW, + STEP_TEAR_NEW, + STEP_TEAR_OLD,
We could take the opportunity to rename from TEAR_NEW to ROLL_BACK and TEAR_OLD to SWITCH_OVER... IDC, just a thought since the comments from virNWFilterTriggerVMFilterRebuild (that are currently copied inside the #if 0 in virNWFilterTriggerRebuild) perhaps more accurately describe what's going on...
+ STEP_APPLY_CURRENT, +};
[...]
+int +virNWFilterBuildAll(virNWFilterDriverStatePtr driver, + bool newFilters) +{ + struct virNWFilterBuildData data = { + .driver = driver, + }; + int ret = 0; + + VIR_DEBUG("Build all filters newFilters=%d", newFilters); + + if (newFilters) { + if (!(data.skipInterfaces = virHashCreate(0, NULL))) + return -1; + + data.step = STEP_APPLY_NEW; + if (virNWFilterBindingObjListForEach(driver->bindings, + virNWFilterBuildIter, + &data) < 0) + ret = -1; + + if (ret == -1) { + data.step = STEP_TEAR_NEW; + virNWFilterBindingObjListForEach(driver->bindings, + virNWFilterBuildIter, + &data); + } else { + data.step = STEP_TEAR_OLD; + virNWFilterBindingObjListForEach(driver->bindings, + virNWFilterBuildIter, + &data); + } + } else { + data.step = STEP_APPLY_CURRENT; + if (virNWFilterBindingObjListForEach(driver->bindings, + virNWFilterBuildIter, + &data) < 0) + ret = -1; + }
Does this need to virHashFree(data.skipInterfaces); similar to what virNWFilterTriggerVMFilterRebuild did previously, since it's local here?
return ret; }
[...] With a couple of minor adjustments and since it doesn't appear any of the changes would be too controversial... Reviewed-by: John Ferlan <jferlan@redhat.com> John Testing the heck out of this is the real challenge! I'll look to shake the cobwebs off my virt_test/avocado environment...