On 05/15/2018 01:43 PM, 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 | 83 +++-------------- src/nwfilter/nwfilter_gentech_driver.c | 42 --------- src/nwfilter/nwfilter_gentech_driver.h | 4 - 6 files changed, 120 insertions(+), 147 deletions(-)
Will need a followup patch for news.xml...
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)
Could/Should this go in virnwfilterbindingdef.c ? It's just a copy from nwfilter_gentech_driver.c... Probably something we could have done earlier or at least separable for this series.
{ - 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 net->ifname is NULL, then we don't have a portdevname and things fall part fairly quickly... Although in *InstantiateFilterInternal and calls to virNetDevExists "failing" we'd just return 0 as if interface or vm disappeared. If net->filter then NULL, prior to this series, InstantiateFilterUpdate would use net->filter as filtername rather unconditionally... Should they then be an error?
+ + if (!conn) + goto cleanup; + + if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net))) + goto cleanup; + + if (!(xml = virNWFilterBindingDefFormat(def))) + goto cleanup; +
[...]
void virDomainConfVMNWFilterTeardown(virDomainObjPtr vm) { size_t i; + virConnectPtr conn = virGetConnectNWFilter(); + + if (!conn) + return; +
Remove an empty 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 c3c52ae5f3..9ee5c57d9f 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -654,66 +654,6 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter,
[...]
static virNWFilterBindingPtr nwfilterBindingLookupByPortDev(virConnectPtr conn, const char *portdev) @@ -723,8 +663,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);
Noted in patch 19 review...
goto cleanup; + }
if (virNWFilterBindingLookupByPortDevEnsureACL(conn, obj->def) < 0) goto cleanup; @@ -768,8 +711,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);
<sigh> should have noted this in my review of patch 19 too.
goto cleanup; + }
if (virNWFilterBindingGetXMLDescEnsureACL(binding->conn, obj->def) < 0) goto cleanup; @@ -839,8 +785,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);
Noted in patch 20 review.
return -1; + }
[...] In general, code looks OK - perhaps some placement differences and movement of one error message output to an earlier patch... Reviewed-by: John Ferlan <jferlan@redhat.com> John