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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
John
Testing the heck out of this is the real challenge! I'll look to shake
the cobwebs off my virt_test/avocado environment...
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
