Remove the callbacks that the nwfilter driver registers with the domain
object config layer. Instead make the current helper methods call into
the public API for creating/deleting nwfilter bindings.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/conf/domain_nwfilter.c             | 124 +++++++++++++++++++++----
 src/conf/domain_nwfilter.h             |  13 ---
 src/libvirt_private.syms               |   1 -
 src/nwfilter/nwfilter_driver.c         |  84 +++--------------
 src/nwfilter/nwfilter_gentech_driver.c |  42 ---------
 src/nwfilter/nwfilter_gentech_driver.h |   4 -
 6 files changed, 120 insertions(+), 148 deletions(-)

diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c
index 7570e0ae83..ed45394918 100644
--- a/src/conf/domain_nwfilter.c
+++ b/src/conf/domain_nwfilter.c
@@ -28,45 +28,137 @@
 #include "datatypes.h"
 #include "domain_conf.h"
 #include "domain_nwfilter.h"
+#include "virnwfilterbindingdef.h"
 #include "virerror.h"
+#include "viralloc.h"
+#include "virstring.h"
+#include "virlog.h"
 
-#define VIR_FROM_THIS VIR_FROM_NWFILTER
 
-static virDomainConfNWFilterDriverPtr nwfilterDriver;
+VIR_LOG_INIT("conf.domain_nwfilter");
 
-void
-virDomainConfNWFilterRegister(virDomainConfNWFilterDriverPtr driver)
+#define VIR_FROM_THIS VIR_FROM_NWFILTER
+
+static virNWFilterBindingDefPtr
+virNWFilterBindingDefForNet(const char *vmname,
+                            const unsigned char *vmuuid,
+                            virDomainNetDefPtr net)
 {
-    nwfilterDriver = driver;
+    virNWFilterBindingDefPtr ret;
+
+    if (VIR_ALLOC(ret) < 0)
+        return NULL;
+
+    if (VIR_STRDUP(ret->ownername, vmname) < 0)
+        goto error;
+
+    memcpy(ret->owneruuid, vmuuid, sizeof(ret->owneruuid));
+
+    if (VIR_STRDUP(ret->portdevname, net->ifname) < 0)
+        goto error;
+
+    if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT &&
+        VIR_STRDUP(ret->linkdevname, net->data.direct.linkdev) < 0)
+        goto error;
+
+    ret->mac = net->mac;
+
+    if (VIR_STRDUP(ret->filter, net->filter) < 0)
+        goto error;
+
+    if (!(ret->filterparams = virNWFilterHashTableCreate(0)))
+        goto error;
+
+    if (net->filterparams &&
+        virNWFilterHashTablePutAll(net->filterparams, ret->filterparams) < 0)
+        goto error;
+
+    return ret;
+
+ error:
+    virNWFilterBindingDefFree(ret);
+    return NULL;
 }
 
+
 int
 virDomainConfNWFilterInstantiate(const char *vmname,
                                  const unsigned char *vmuuid,
                                  virDomainNetDefPtr net)
 {
-    if (nwfilterDriver != NULL)
-        return nwfilterDriver->instantiateFilter(vmname, vmuuid, net);
+    virConnectPtr conn = virGetConnectNWFilter();
+    virNWFilterBindingDefPtr def = NULL;
+    virNWFilterBindingPtr binding = NULL;
+    char *xml;
+    int ret = -1;
+
+    VIR_DEBUG("vmname=%s portdev=%s filter=%s",
+              vmname, NULLSTR(net->ifname), NULLSTR(net->filter));
+
+    if (!conn)
+        goto cleanup;
+
+    if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
+        goto cleanup;
+
+    if (!(xml = virNWFilterBindingDefFormat(def)))
+        goto cleanup;
+
+    if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0)))
+        goto cleanup;
+
+    ret = 0;
+
+ cleanup:
+    VIR_FREE(xml);
+    virNWFilterBindingDefFree(def);
+    virObjectUnref(binding);
+    virObjectUnref(conn);
+    return ret;
+}
+
+
+static void
+virDomainConfNWFilterTeardownImpl(virConnectPtr conn,
+                                  virDomainNetDefPtr net)
+{
+    virNWFilterBindingPtr binding;
 
-    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                   _("No network filter driver available"));
-    return -1;
+    binding = virNWFilterBindingLookupByPortDev(conn, net->ifname);
+    if (!binding)
+        return;
+
+    virNWFilterBindingDelete(binding);
+
+    virObjectUnref(binding);
 }
 
+
 void
 virDomainConfNWFilterTeardown(virDomainNetDefPtr net)
 {
-    if (nwfilterDriver != NULL)
-        nwfilterDriver->teardownFilter(net);
+    virConnectPtr conn = virGetConnectNWFilter();
+
+    if (!conn)
+        return;
+
+    virDomainConfNWFilterTeardownImpl(conn, net);
+
+    virObjectUnref(conn);
 }
 
 void
 virDomainConfVMNWFilterTeardown(virDomainObjPtr vm)
 {
     size_t i;
+    virConnectPtr conn = virGetConnectNWFilter();
+
+    if (!conn)
+        return;
+
+
+    for (i = 0; i < vm->def->nnets; i++)
+        virDomainConfNWFilterTeardownImpl(conn, vm->def->nets[i]);
 
-    if (nwfilterDriver != NULL) {
-        for (i = 0; i < vm->def->nnets; i++)
-            virDomainConfNWFilterTeardown(vm->def->nets[i]);
-    }
+    virObjectUnref(conn);
 }
diff --git a/src/conf/domain_nwfilter.h b/src/conf/domain_nwfilter.h
index 857cac6c2a..d2ebeff853 100644
--- a/src/conf/domain_nwfilter.h
+++ b/src/conf/domain_nwfilter.h
@@ -23,19 +23,6 @@
 #ifndef DOMAIN_NWFILTER_H
 # define DOMAIN_NWFILTER_H
 
-typedef int (*virDomainConfInstantiateNWFilter)(const char *vmname,
-                                                const unsigned char *vmuuid,
-                                                virDomainNetDefPtr net);
-typedef void (*virDomainConfTeardownNWFilter)(virDomainNetDefPtr net);
-
-typedef struct {
-    virDomainConfInstantiateNWFilter instantiateFilter;
-    virDomainConfTeardownNWFilter    teardownFilter;
-} virDomainConfNWFilterDriver;
-typedef virDomainConfNWFilterDriver *virDomainConfNWFilterDriverPtr;
-
-void virDomainConfNWFilterRegister(virDomainConfNWFilterDriverPtr driver);
-
 int virDomainConfNWFilterInstantiate(const char *vmname,
                                      const unsigned char *vmuuid,
                                      virDomainNetDefPtr net);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ecc60dbe73..9676ad3d13 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -652,7 +652,6 @@ virDomainQemuMonitorEventStateRegisterID;
 
 # conf/domain_nwfilter.h
 virDomainConfNWFilterInstantiate;
-virDomainConfNWFilterRegister;
 virDomainConfNWFilterTeardown;
 virDomainConfVMNWFilterTeardown;
 
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 2b6856a36c..26e6e76b3b 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -654,67 +654,6 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter,
 }
 
 
-static int
-nwfilterInstantiateFilter(const char *vmname,
-                          const unsigned char *vmuuid,
-                          virDomainNetDefPtr net)
-{
-    virNWFilterBindingObjPtr obj;
-    virNWFilterBindingDefPtr def;
-    int ret;
-
-    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, def);
-
-    if (ret >= 0)
-        virNWFilterBindingObjSave(obj, driver->bindingDir);
-    else
-        virNWFilterBindingObjListRemove(driver->bindings, obj);
-
-    virNWFilterBindingObjEndAPI(&obj);
-
-    return ret;
-}
-
-
-static void
-nwfilterTeardownFilter(virDomainNetDefPtr net)
-{
-    virNWFilterBindingObjPtr obj;
-    virNWFilterBindingDefPtr def;
-    if (!net->ifname)
-        return;
-
-    obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname);
-    if (!obj)
-        return;
-
-    def = virNWFilterBindingObjGetDef(obj);
-    virNWFilterTeardownFilter(def);
-    virNWFilterBindingObjDelete(obj, driver->bindingDir);
-
-    virNWFilterBindingObjListRemove(driver->bindings, obj);
-    virNWFilterBindingObjEndAPI(&obj);
-}
-
-
 static virNWFilterBindingPtr
 nwfilterBindingLookupByPortDev(virConnectPtr conn,
                                const char *portdev)
@@ -725,8 +664,11 @@ nwfilterBindingLookupByPortDev(virConnectPtr conn,
 
     obj = virNWFilterBindingObjListFindByPortDev(driver->bindings,
                                                  portdev);
-    if (!obj)
+    if (!obj) {
+        virReportError(VIR_ERR_NO_NWFILTER_BINDING,
+                       _("no nwfilter binding for port dev '%s'"), portdev);
         goto cleanup;
+    }
 
     def = virNWFilterBindingObjGetDef(obj);
     if (virNWFilterBindingLookupByPortDevEnsureACL(conn, def) < 0)
@@ -773,8 +715,11 @@ nwfilterBindingGetXMLDesc(virNWFilterBindingPtr binding,
 
     obj = virNWFilterBindingObjListFindByPortDev(driver->bindings,
                                                  binding->portdev);
-    if (!obj)
+    if (!obj) {
+        virReportError(VIR_ERR_NO_NWFILTER_BINDING,
+                       _("no nwfilter binding for port dev '%s'"), binding->portdev);
         goto cleanup;
+    }
 
     def = virNWFilterBindingObjGetDef(obj);
     if (virNWFilterBindingGetXMLDescEnsureACL(binding->conn, def) < 0)
@@ -846,8 +791,11 @@ nwfilterBindingDelete(virNWFilterBindingPtr binding)
     int ret = -1;
 
     obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, binding->portdev);
-    if (!obj)
+    if (!obj) {
+        virReportError(VIR_ERR_NO_NWFILTER_BINDING,
+                       _("no nwfilter binding for port dev '%s'"), binding->portdev);
         return -1;
+    }
 
     def = virNWFilterBindingObjGetDef(obj);
     if (virNWFilterBindingDeleteEnsureACL(binding->conn, def) < 0)
@@ -908,13 +856,6 @@ static virStateDriver stateDriver = {
     .stateReload = nwfilterStateReload,
 };
 
-
-static virDomainConfNWFilterDriver domainNWFilterDriver = {
-    .instantiateFilter = nwfilterInstantiateFilter,
-    .teardownFilter = nwfilterTeardownFilter,
-};
-
-
 int nwfilterRegister(void)
 {
     if (virRegisterConnectDriver(&nwfilterConnectDriver, false) < 0)
@@ -923,6 +864,5 @@ int nwfilterRegister(void)
         return -1;
     if (virRegisterStateDriver(&stateDriver) < 0)
         return -1;
-    virDomainConfNWFilterRegister(&domainNWFilterDriver);
     return 0;
 }
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
index d208d0188e..e5dea91f83 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -1082,45 +1082,3 @@ virNWFilterBuildAll(virNWFilterDriverStatePtr driver,
     }
     return ret;
 }
-
-
-virNWFilterBindingDefPtr
-virNWFilterBindingDefForNet(const char *vmname,
-                            const unsigned char *vmuuid,
-                            virDomainNetDefPtr net)
-{
-    virNWFilterBindingDefPtr ret;
-
-    if (VIR_ALLOC(ret) < 0)
-        return NULL;
-
-    if (VIR_STRDUP(ret->ownername, vmname) < 0)
-        goto error;
-
-    memcpy(ret->owneruuid, vmuuid, sizeof(ret->owneruuid));
-
-    if (VIR_STRDUP(ret->portdevname, net->ifname) < 0)
-        goto error;
-
-    if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT &&
-        VIR_STRDUP(ret->linkdevname, net->data.direct.linkdev) < 0)
-        goto error;
-
-    ret->mac = net->mac;
-
-    if (VIR_STRDUP(ret->filter, net->filter) < 0)
-        goto error;
-
-    if (!(ret->filterparams = virNWFilterHashTableCreate(0)))
-        goto error;
-
-    if (net->filterparams &&
-        virNWFilterHashTablePutAll(net->filterparams, ret->filterparams) < 0)
-        goto error;
-
-    return ret;
-
- error:
-    virNWFilterBindingDefFree(ret);
-    return NULL;
-}
diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h
index 481fdd2413..2cd19c90fc 100644
--- a/src/nwfilter/nwfilter_gentech_driver.h
+++ b/src/nwfilter/nwfilter_gentech_driver.h
@@ -57,8 +57,4 @@ virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr,
 int virNWFilterBuildAll(virNWFilterDriverStatePtr driver,
                         bool newFilters);
 
-virNWFilterBindingDefPtr virNWFilterBindingDefForNet(const char *vmname,
-                                                     const unsigned char *vmuuid,
-                                                     virDomainNetDefPtr net);
-
 #endif
-- 
2.17.0

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

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> Remove the callbacks that the nwfilter driver registers with the domain
> object config layer. Instead make the current helper methods call into
> the public API for creating/deleting nwfilter bindings.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/conf/domain_nwfilter.c             | 124 +++++++++++++++++++++----
>  src/conf/domain_nwfilter.h             |  13 ---
>  src/libvirt_private.syms               |   1 -
>  src/nwfilter/nwfilter_driver.c         |  84 +++--------------
>  src/nwfilter/nwfilter_gentech_driver.c |  42 ---------
>  src/nwfilter/nwfilter_gentech_driver.h |   4 -
>  6 files changed, 120 insertions(+), 148 deletions(-)
> 

I ran the more "aggressive" Avocado tests based on an old bz
(https://bugzilla.redhat.com/show_bug.cgi?id=1034807) and got the
following in my debug libvirtd output:

2018-06-15 15:46:18.683+0000: 18817: error :
virNWFilterBindingLookupByPortDev:589 : portdev in
virNWFilterBindingLookupByPortDev must not be NULL
2018-06-15 15:46:18.684+0000: 18817: error :
virNWFilterBindingLookupByPortDev:589 : portdev in
virNWFilterBindingLookupByPortDev must not be NULL

The first thing the test does is remove the defined interface:

   <interface type='bridge'>
      <mac address='52:54:00:9a:9b:9c'/>
      <source bridge='virbr0'/>
      <model type='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
function='0x0'/>
    </interface>

and then replaces/adds with a new interface:

<interface type="bridge">
  <mac address="52:54:00:9a:9b:9c" />
  <source bridge="virbr0" />
  <model type="virtio" />
  <address bus="0x00" domain="0x0000" function="0x0" slot="0x03"
type="pci" />
  <filterref filter="allow-arp" />
</interface>

for the test domain via device attach.

Then 2 threads are started - 1 that continually starts/destroys the
domain and 1 that continually removes/adds allow-arp. The actual logic
in the test is busted because starting up a domain without a defined
filter will fail and the thread doing the start/stop has no retry
(try/except) logic. When I added the try/except logic and toned down the
retry logic a bit I could get the test to pass with a few adjustments to
the libvirt code here.  Ironically, when the test goes too fast it
caused my CPU's to get hot and generate a false positive failure because
there were dmesg events related to core temperature above threshold.

Anyway, I've noted a couple of places I think adjustments could/should
be made - hopefully they make sense as I started looking "backwards" to
see where things go introduced.


> diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c
> index 7570e0ae83..ed45394918 100644
> --- a/src/conf/domain_nwfilter.c
> +++ b/src/conf/domain_nwfilter.c

[...]

>  
> +
>  int
>  virDomainConfNWFilterInstantiate(const char *vmname,
>                                   const unsigned char *vmuuid,
>                                   virDomainNetDefPtr net)

Revisiting my comment from patch 13 - it is possible to enter here with
net->filter being NULL. Is it worth returning 0 in that case under the
assumption that there is no filter so that the callers then don't have
to make that check? If portdev/ifname is NULL, not much is going to be
found as well.

>  {
> -    if (nwfilterDriver != NULL)
> -        return nwfilterDriver->instantiateFilter(vmname, vmuuid, net);
> +    virConnectPtr conn = virGetConnectNWFilter();
> +    virNWFilterBindingDefPtr def = NULL;
> +    virNWFilterBindingPtr binding = NULL;
> +    char *xml;
> +    int ret = -1;
> +
> +    VIR_DEBUG("vmname=%s portdev=%s filter=%s",
> +              vmname, NULLSTR(net->ifname), NULLSTR(net->filter));

Both being NULL probably is not a good thing - what filter for what portdev?

> +
> +    if (!conn)
> +        goto cleanup;
> +

Based on patch 16/19 comments and the thought above:

 if (!net->ifname ||
     (binding = virNWFilterBindingLookupByPortDev(conn, net->ifname))) {
     ret = 0;
     goto cleanup;
 }

where the !net->ifname may avoids the patch 13 comment issue and the ret
= 0 when finding the binding handles the filter already loaded issue.
The filter would be loaded for a running guest (after at least the
second libvirtd restart) by virNWFilterBindingObjListLoadAllConfigs.

> +    if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
> +        goto cleanup;
> +
> +    if (!(xml = virNWFilterBindingDefFormat(def)))
> +        goto cleanup;
> +
> +    if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0)))
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(xml);
> +    virNWFilterBindingDefFree(def);
> +    virObjectUnref(binding);
> +    virObjectUnref(conn);
> +    return ret;
> +}
> +
> +
> +static void
> +virDomainConfNWFilterTeardownImpl(virConnectPtr conn,
> +                                  virDomainNetDefPtr net)
> +{
> +    virNWFilterBindingPtr binding;
>  
> -    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                   _("No network filter driver available"));
> -    return -1;
> +    binding = virNWFilterBindingLookupByPortDev(conn, net->ifname);
> +    if (!binding)

virNWFilterBindingLookupByPortDev will generate an error when there's no
filter defined for @net (if you're running libvirtd in a debugger you
see it).

Shouldn't the call be guarded by a "if (!net->filter)"?

    if (!net->filter ||
        !binding = vir...())
        return;

if not, then we probably should reset the last error since we're just
returning (error as in [1]).

> +        return;
> +
> +    virNWFilterBindingDelete(binding);
> +
> +    virObjectUnref(binding);
>  }
>  
> +
>  void
>  virDomainConfNWFilterTeardown(virDomainNetDefPtr net)
>  {
> -    if (nwfilterDriver != NULL)
> -        nwfilterDriver->teardownFilter(net);
> +    virConnectPtr conn = virGetConnectNWFilter();
> +
> +    if (!conn)
> +        return;
> +
> +    virDomainConfNWFilterTeardownImpl(conn, net);
> +
> +    virObjectUnref(conn);
>  }
>  

may as well add the blank line here from ...

>  void
>  virDomainConfVMNWFilterTeardown(virDomainObjPtr vm)
>  {
>      size_t i;
> +    virConnectPtr conn = virGetConnectNWFilter();
> +
> +    if (!conn)
> +        return;
> +

... here... or at least remove this extra blank line.

> +
> +    for (i = 0; i < vm->def->nnets; i++)
> +        virDomainConfNWFilterTeardownImpl(conn, vm->def->nets[i]);
>  
> -    if (nwfilterDriver != NULL) {
> -        for (i = 0; i < vm->def->nnets; i++)
> -            virDomainConfNWFilterTeardown(vm->def->nets[i]);
> -    }
> +    virObjectUnref(conn);
>  }

[...]

>  
> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index 2b6856a36c..26e6e76b3b 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -654,67 +654,6 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter,
>  }
>  
>  

[...]

>  static virNWFilterBindingPtr
>  nwfilterBindingLookupByPortDev(virConnectPtr conn,
>                                 const char *portdev)
> @@ -725,8 +664,11 @@ nwfilterBindingLookupByPortDev(virConnectPtr conn,
>  
>      obj = virNWFilterBindingObjListFindByPortDev(driver->bindings,
>                                                   portdev);
> -    if (!obj)
> +    if (!obj) {
> +        virReportError(VIR_ERR_NO_NWFILTER_BINDING,
> +                       _("no nwfilter binding for port dev '%s'"), portdev);454

[1]
Adding this error here for someone running debug will cause those
virDomainConfNWFilterTeardownImpl consumers w/o a filter to display this
message. Of course removing it means the callers all have to add some
sort of message.

John

>          goto cleanup;
> +    }
>  
>      def = virNWFilterBindingObjGetDef(obj);
>      if (virNWFilterBindingLookupByPortDevEnsureACL(conn, def) < 0)

[...]

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