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

[John Ferlan]

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