Re: [libvirt] [PATCH v2 18/21] nwfilter: remove virt driver callback layer for rebuilding filters

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(a)redhat.com&gt; --- 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(-)

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; +} +

+int +virNWFilterTriggerRebuild(void) +{ +#if 0

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

+ 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; + }

return ret; }