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
© 2016 - 2025 Red Hat, Inc.