[libvirt] [PATCH v2 17/21] nwfilter: keep track of active filter bindings

[John Ferlan]

On 05/15/2018 01:43 PM, Daniel P. Berrangé wrote:
> Currently the nwfilter driver does not keep any record of what filter
> bindings it has active. This means that when it needs to recreate
> filters, it has to rely on triggering callbacks provided by the virt
> drivers. This introduces a hash table recording the virNWFilterBinding
> objects so the driver has a record of all active filters.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/conf/virnwfilterobj.h      |  4 ++
>  src/nwfilter/nwfilter_driver.c | 83 ++++++++++++++++++++++++----------
>  2 files changed, 64 insertions(+), 23 deletions(-)
> 
> diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
> index 433b0402d0..4a54dd50da 100644
> --- a/src/conf/virnwfilterobj.h
> +++ b/src/conf/virnwfilterobj.h
> @@ -22,6 +22,7 @@
>  # include "internal.h"
>  
>  # include "nwfilter_conf.h"
> +# include "virnwfilterbindingobjlist.h"
>  
>  typedef struct _virNWFilterObj virNWFilterObj;
>  typedef virNWFilterObj *virNWFilterObjPtr;
> @@ -37,7 +38,10 @@ struct _virNWFilterDriverState {
>  
>      virNWFilterObjListPtr nwfilters;
>  
> +    virNWFilterBindingObjListPtr bindings;
> +
>      char *configDir;
> +    char *bindingDir;
>  };
>  
>  virNWFilterDefPtr
> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index b57e5dd00d..67e07d2dec 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c

[...]

> @@ -647,13 +656,38 @@ nwfilterInstantiateFilter(const char *vmname,
>                            const unsigned char *vmuuid,
>                            virDomainNetDefPtr net)
>  {
> -    virNWFilterBindingDefPtr binding;
> +    virNWFilterBindingObjPtr obj;
> +    virNWFilterBindingDefPtr def;
>      int ret;
>  
> -    if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
> +    obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname);
> +    if (obj) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Filter already present for NIC %s"), net->ifname);
> +        virNWFilterBindingObjEndAPI(&obj);
> +        return -1;
> +    }
> +
> +    if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
> +        return -1;
> +
> +    obj = virNWFilterBindingObjListAdd(driver->bindings,
> +                                       def);
> +    if (!obj) {
> +        virNWFilterBindingDefFree(def);
>          return -1;
> -    ret = virNWFilterInstantiateFilter(driver, binding);
> -    virNWFilterBindingDefFree(binding);
> +    }
> +    def = NULL;
> +
> +    ret = virNWFilterInstantiateFilter(driver, obj->def);

If _virNWFilterBindingObj becomes private, then this is where that fetch
of @objdef from non nwfilter and domain drivers is done. So rather than
obj->def - s/b an accessor.

> +
> +    if (ret < 0)
> +        virNWFilterBindingObjListRemove(driver->bindings, obj);
> +
> +    virNWFilterBindingObjSave(obj, driver->bindingDir);

if ret < 0, do we really want to Save?

> +
> +    virNWFilterBindingObjEndAPI(&obj);
> +
>      return ret;
>  }
>  
> @@ -661,16 +695,19 @@ nwfilterInstantiateFilter(const char *vmname,
>  static void
>  nwfilterTeardownFilter(virDomainNetDefPtr net)
>  {
> -    virNWFilterBindingDef binding = {
> -        .portdevname = net->ifname,
> -        .linkdevname = (net->type == VIR_DOMAIN_NET_TYPE_DIRECT ?
> -                        net->data.direct.linkdev : NULL),
> -        .mac = net->mac,
> -        .filter = net->filter,
> -        .filterparams = net->filterparams,
> -    };
> -    if ((net->ifname) && (net->filter))
> -        virNWFilterTeardownFilter(&binding);
> +    virNWFilterBindingObjPtr obj;
> +    if (!net->ifname)
> +        return;
> +
> +    obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname);
> +    if (!obj)
> +        return;
> +
> +    virNWFilterTeardownFilter(obj->def);

Again use of accessor for @obj

> +    virNWFilterBindingObjDelete(obj, driver->bindingDir);
> +
> +    virNWFilterBindingObjListRemove(driver->bindings, obj);
> +    virNWFilterBindingObjEndAPI(&obj);
>  }
>  
>  
> 

I think this is essentially clean - the use of accessors is an easy
change and I trust you'd to the right thing if instantiate fails w/r/t
whether to save the filter obj...

Reviewed-by: John Ferlan <jferlan at redhat.com>

John