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

Daniel P. Berrangé posted 21 patches 6 years, 12 months ago
There is a newer version of this series
[libvirt] [PATCH v2 17/21] nwfilter: keep track of active filter bindings
Posted by Daniel P. Berrangé 6 years, 12 months ago
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@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
@@ -38,7 +38,6 @@
 #include "domain_conf.h"
 #include "domain_nwfilter.h"
 #include "nwfilter_driver.h"
-#include "virnwfilterbindingdef.h"
 #include "nwfilter_gentech_driver.h"
 #include "configmake.h"
 #include "virfile.h"
@@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged,
                         virStateInhibitCallback callback ATTRIBUTE_UNUSED,
                         void *opaque ATTRIBUTE_UNUSED)
 {
-    char *base = NULL;
     DBusConnection *sysbus = NULL;
 
     if (virDBusHasSystemBus() &&
@@ -191,6 +189,9 @@ nwfilterStateInitialize(bool privileged,
     if (!(driver->nwfilters = virNWFilterObjListNew()))
         goto error;
 
+    if (!(driver->bindings = virNWFilterBindingObjListNew()))
+        goto error;
+
     if (!privileged)
         return 0;
 
@@ -230,30 +231,35 @@ nwfilterStateInitialize(bool privileged,
         goto error;
     }
 
-    if (VIR_STRDUP(base, SYSCONFDIR "/libvirt") < 0)
+    if (VIR_STRDUP(driver->configDir, SYSCONFDIR "/libvirt/nwfilter") < 0)
         goto error;
 
-    if (virAsprintf(&driver->configDir,
-                    "%s/nwfilter", base) == -1)
+    if (virFileMakePathWithMode(driver->configDir, S_IRWXU) < 0) {
+        virReportSystemError(errno, _("cannot create config directory '%s'"),
+                             driver->configDir);
         goto error;
+    }
 
-    VIR_FREE(base);
+    if (VIR_STRDUP(driver->bindingDir, LOCALSTATEDIR "/run/libvirt/nwfilter-binding") < 0)
+        goto error;
 
-    if (virFileMakePathWithMode(driver->configDir, S_IRWXU) < 0) {
+    if (virFileMakePathWithMode(driver->bindingDir, S_IRWXU) < 0) {
         virReportSystemError(errno, _("cannot create config directory '%s'"),
-                             driver->configDir);
+                             driver->bindingDir);
         goto error;
     }
 
     if (virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir) < 0)
         goto error;
 
+    if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0)
+        goto error;
+
     nwfilterDriverUnlock();
 
     return 0;
 
  error:
-    VIR_FREE(base);
     nwfilterDriverUnlock();
     nwfilterStateCleanup();
 
@@ -333,9 +339,12 @@ nwfilterStateCleanup(void)
         nwfilterDriverRemoveDBusMatches();
 
         VIR_FREE(driver->configDir);
+        VIR_FREE(driver->bindingDir);
         nwfilterDriverUnlock();
     }
 
+    virObjectUnref(driver->bindings);
+
     /* free inactive nwfilters */
     virNWFilterObjListFree(driver->nwfilters);
 
@@ -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 (ret < 0)
+        virNWFilterBindingObjListRemove(driver->bindings, obj);
+
+    virNWFilterBindingObjSave(obj, driver->bindingDir);
+
+    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);
+    virNWFilterBindingObjDelete(obj, driver->bindingDir);
+
+    virNWFilterBindingObjListRemove(driver->bindings, obj);
+    virNWFilterBindingObjEndAPI(&obj);
 }
 
 
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 17/21] nwfilter: keep track of active filter bindings
Posted by John Ferlan 6 years, 12 months ago

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@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@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list