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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
