Re: [libvirt] [PATCH 09/14] nwfilter: convert the gentech driver code to use virNWFilterBinding

On Fri, Apr 27, 2018 at 16:25:08 +0100, Daniel P. Berrangé wrote:

Use the virNWFilterBinding struct in the gentech driver code directly.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 35 +++--- src/nwfilter/nwfilter_driver.c | 21 +++- src/nwfilter/nwfilter_gentech_driver.c | 211 +++++++++++++++++---------------- src/nwfilter/nwfilter_gentech_driver.h | 22 ++-- src/nwfilter/nwfilter_learnipaddr.c | 16 +-- 5 files changed, 168 insertions(+), 137 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index aec68ab847..dc4e3cb834 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -486,15 +486,18 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,

/* instantiate the filters */

- if (req->ifname) + if (req->ifname) { + virNWFilterBinding binding = { + .portdevname = req->ifname, + .linkdevname = req->linkdev, + .mac = req->macaddr, + .filter = req->filtername, + .filterparams = req->vars, + }; rc = virNWFilterInstantiateFilterLate(req->driver, - NULL, - req->ifname, - req->ifindex, - req->linkdev, - &req->macaddr, - req->filtername, - req->vars); + &binding, + req->ifindex); + }

exit_snooprequnlock: virNWFilterSnoopReqUnlock(req); @@ -873,14 +876,16 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, goto skip_instantiate;

if (ipAddrLeft) { + virNWFilterBinding binding = { + .portdevname = req->ifname, + .linkdevname = req->linkdev, + .mac = req->macaddr, + .filter = req->filtername, + .filterparams = req->vars, + }; ret = virNWFilterInstantiateFilterLate(req->driver, - NULL, - req->ifname, - req->ifindex, - req->linkdev, - &req->macaddr, - req->filtername, - req->vars); + &binding, + req->ifindex); } else { virNWFilterVarValuePtr dhcpsrvrs = virHashLookup(req->vars, NWFILTER_VARNAME_DHCPSERVER); diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index d17a8ec00b..a375e9bda8 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -642,19 +642,34 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter,

static int -nwfilterInstantiateFilter(const char *vmname ATTRIBUTE_UNUSED, +nwfilterInstantiateFilter(const char *vmname, const unsigned char *vmuuid, virDomainNetDefPtr net) { - return virNWFilterInstantiateFilter(driver, vmuuid, net); + virNWFilterBindingPtr binding; + int ret; + + if (!(binding = virNWFilterBindingForNet(vmname, vmuuid, net))) + return -1; + ret = virNWFilterInstantiateFilter(driver, binding); + virNWFilterBindingFree(binding); + return ret; }

static void nwfilterTeardownFilter(virDomainNetDefPtr net) { + virNWFilterBinding 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, + };

I think using virNWFilterBindingForNet or its variant which doesn't copy the arguments would be a bit better than open coding it here. But for consistency and safety reasons I'd prefer if we used virNWFilterBindingForNet everywhere without introducing a non-copying version.

if ((net->ifname) && (net->filter)) - virNWFilterTeardownFilter(net); + virNWFilterTeardownFilter(&binding); }

diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index af4411d4db..c755350586 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -577,12 +577,9 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,

/** * virNWFilterDoInstantiate: - * @vmuuid: The UUID of the VM * @techdriver: The driver to use for instantiation + * @binding: description of port to bind the filter to * @filter: The filter to instantiate - * @ifname: The name of the interface to apply the rules to - * @vars: A map holding variable names and values used for instantiating - * the filter and its subfilters. * @forceWithPendingReq: Ignore the check whether a pending learn request * is active; 'true' only when the rules are applied late * @@ -596,17 +593,13 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, * Call this function while holding the NWFilter filter update lock */ static int -virNWFilterDoInstantiate(const unsigned char *vmuuid, - virNWFilterTechDriverPtr techdriver, +virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, + virNWFilterBindingPtr binding, virNWFilterDefPtr filter, - const char *ifname, int ifindex, - const char *linkdev, - virHashTablePtr vars, enum instCase useNewFilter, bool *foundNewFilter, bool teardownOld, - const virMacAddr *macaddr, virNWFilterDriverStatePtr driver, bool forceWithPendingReq) { @@ -628,14 +621,14 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, }

rc = virNWFilterDetermineMissingVarsRec(filter, - vars, + binding->filterparams, missing_vars, useNewFilter, driver); if (rc < 0) goto err_exit;

- lv = virHashLookup(vars, NWFILTER_VARNAME_CTRL_IP_LEARNING); + lv = virHashLookup(binding->filterparams, NWFILTER_VARNAME_CTRL_IP_LEARNING); if (lv) learning = virNWFilterVarValueGetNthValue(lv, 0); else @@ -652,19 +645,20 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, goto err_unresolvable_vars; } if (STRCASEEQ(learning, "dhcp")) { - rc = virNWFilterDHCPSnoopReq(techdriver, ifname, linkdev, - vmuuid, macaddr, - filter->name, vars, driver); + rc = virNWFilterDHCPSnoopReq(techdriver, binding->portdevname, + binding->linkdevname, + binding->owneruuid, &binding->mac, + filter->name, binding->filterparams, driver); goto err_exit; } else if (STRCASEEQ(learning, "any")) { if (!virNWFilterHasLearnReq(ifindex)) { rc = virNWFilterLearnIPAddress(techdriver, - ifname, + binding->portdevname, ifindex, - linkdev, - macaddr, + binding->linkdevname, + &binding->mac, filter->name, - vars, driver, + binding->filterparams, driver, DETECT_DHCP|DETECT_STATIC); } goto err_exit; @@ -688,7 +682,7 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid,

rc = virNWFilterDefToInst(driver, filter, - vars, + binding->filterparams, useNewFilter, foundNewFilter, &inst);

@@ -705,22 +699,22 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, }

if (instantiate) { - if (virNWFilterLockIface(ifname) < 0) + if (virNWFilterLockIface(binding->portdevname) < 0) goto err_exit;

- rc = techdriver->applyNewRules(ifname, inst.rules, inst.nrules); + rc = techdriver->applyNewRules(binding->portdevname, inst.rules, inst.nrules);

if (teardownOld && rc == 0) - techdriver->tearOldRules(ifname); + techdriver->tearOldRules(binding->portdevname);

- if (rc == 0 && (virNetDevValidateConfig(ifname, NULL, ifindex) <= 0)) { + if (rc == 0 && (virNetDevValidateConfig(binding->portdevname, NULL, ifindex) <= 0)) { virResetLastError(); /* interface changed/disppeared */ - techdriver->allTeardown(ifname); + techdriver->allTeardown(binding->portdevname); rc = -1; }

- virNWFilterUnlockIface(ifname); + virNWFilterUnlockIface(binding->portdevname); }

err_exit: @@ -749,14 +743,9 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, */ static int virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, bool teardownOld, - const char *ifname, + virNWFilterBindingPtr binding, int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams, enum instCase useNewFilter, bool forceWithPendingReq, bool *foundNewFilter) @@ -765,7 +754,6 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, const char *drvname = EBIPTABLES_DRIVER_ID; virNWFilterTechDriverPtr techdriver; virNWFilterObjPtr obj; - virHashTablePtr vars, vars1; virNWFilterDefPtr filter; virNWFilterDefPtr newFilter; char vmmacaddr[VIR_MAC_STRING_BUFLEN] = {0}; @@ -781,29 +769,22 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, return -1; }

- VIR_DEBUG("filter name: %s", filtername); + VIR_DEBUG("filter name: %s", binding->filter);

if (!(obj = virNWFilterObjListFindInstantiateFilter(driver->nwfilters, - filtername))) + binding->filter))) return -1;

- virMacAddrFormat(macaddr, vmmacaddr); + virMacAddrFormat(&binding->mac, vmmacaddr);

- ipaddr = virNWFilterIPAddrMapGetIPAddr(ifname); + ipaddr = virNWFilterIPAddrMapGetIPAddr(binding->portdevname);

The following change should be in a separate patch with an explanation why it is safe. Originally, we made a copy of filterparams and added NWFILTER_STD_VAR_MAC and NWFILTER_STD_VAR_IP into the copy. But now this code just operates directly on filterparams possibly modifying net-filterparams. This doesn't look like something we should do IMHO.

- vars1 = virNWFilterCreateVarHashmap(vmmacaddr, ipaddr); - if (!vars1) { + if (virNWFilterVarHashmapAddStdValues(binding->filterparams, + vmmacaddr, ipaddr) < 0) { rc = -1; goto err_exit; }

- vars = virNWFilterCreateVarsFrom(vars1, - filterparams); - if (!vars) { - rc = -1; - goto err_exit_vars1; - } - filter = virNWFilterObjGetDef(obj);

switch (useNewFilter) { @@ -819,17 +800,11 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, break; }

- rc = virNWFilterDoInstantiate(vmuuid, techdriver, filter, - ifname, ifindex, linkdev, - vars, useNewFilter, foundNewFilter, - teardownOld, macaddr, driver, + rc = virNWFilterDoInstantiate(techdriver, binding, filter, + ifindex, useNewFilter, foundNewFilter, + teardownOld, driver, forceWithPendingReq);

- virHashFree(vars); - - err_exit_vars1: - virHashFree(vars1); - err_exit: virNWFilterObjUnlock(obj);

@@ -839,15 +814,11 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver,

static int virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net, + virNWFilterBindingPtr binding, bool teardownOld, enum instCase useNewFilter, bool *foundNewFilter) { - const char *linkdev = (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) - ? net->data.direct.linkdev - : NULL; int ifindex; int rc;

@@ -856,8 +827,8 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, /* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed (while holding the lock) and we don't want to build new ones */ - if (virNetDevExists(net->ifname) != 1 || - virNetDevGetIndex(net->ifname, &ifindex) < 0) { + if (virNetDevExists(binding->portdevname) != 1 || + virNetDevGetIndex(binding->portdevname, &ifindex) < 0) { /* interfaces / VMs can disappear during filter instantiation; don't mark it as an error */ virResetLastError(); @@ -865,10 +836,10 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, goto cleanup; }

- rc = virNWFilterInstantiateFilterUpdate(driver, vmuuid, teardownOld, - net->ifname, ifindex, linkdev, - &net->mac, net->filter, - net->filterparams, useNewFilter, + rc = virNWFilterInstantiateFilterUpdate(driver, teardownOld, + binding, + ifindex, + useNewFilter, false, foundNewFilter);

cleanup: @@ -880,13 +851,8 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver,

int virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const char *ifname, - int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams) + virNWFilterBindingPtr binding, + int ifindex) { int rc; bool foundNewFilter = false; @@ -894,18 +860,17 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, virNWFilterReadLockFilterUpdates(); virMutexLock(&updateMutex);

- rc = virNWFilterInstantiateFilterUpdate(driver, vmuuid, true, - ifname, ifindex, linkdev, - macaddr, filtername, filterparams, + rc = virNWFilterInstantiateFilterUpdate(driver, true, + binding, ifindex, INSTANTIATE_ALWAYS, true, &foundNewFilter); if (rc < 0) { /* something went wrong... 'DOWN' the interface */ - if ((virNetDevValidateConfig(ifname, NULL, ifindex) <= 0) || - (virNetDevSetOnline(ifname, false) < 0)) { + if ((virNetDevValidateConfig(binding->portdevname, NULL, ifindex) <= 0) || + (virNetDevSetOnline(binding->portdevname, false) < 0)) { virResetLastError(); /* assuming interface disappeared... */ - _virNWFilterTeardownFilter(ifname); + _virNWFilterTeardownFilter(binding->portdevname); } }

@@ -918,12 +883,11 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,

int virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net) + virNWFilterBindingPtr binding) { bool foundNewFilter = false;

- return virNWFilterInstantiateFilterInternal(driver, vmuuid, net, + return virNWFilterInstantiateFilterInternal(driver, binding, 1, INSTANTIATE_ALWAYS, &foundNewFilter); @@ -932,13 +896,12 @@ virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,

int virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net, + virNWFilterBindingPtr binding, bool *skipIface) { bool foundNewFilter = false;

- int rc = virNWFilterInstantiateFilterInternal(driver, vmuuid, net, + int rc = virNWFilterInstantiateFilterInternal(driver, binding, 0, INSTANTIATE_FOLLOW_NEWFILTER, &foundNewFilter); @@ -948,7 +911,7 @@ virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver, }

static int -virNWFilterRollbackUpdateFilter(const virDomainNetDef *net) +virNWFilterRollbackUpdateFilter(virNWFilterBindingPtr binding) { const char *drvname = EBIPTABLES_DRIVER_ID; int ifindex; @@ -964,17 +927,17 @@ virNWFilterRollbackUpdateFilter(const virDomainNetDef *net) }

/* don't tear anything while the address is being learned */ - if (virNetDevGetIndex(net->ifname, &ifindex) < 0) + if (virNetDevGetIndex(binding->portdevname, &ifindex) < 0) virResetLastError(); else if (virNWFilterHasLearnReq(ifindex)) return 0;

- return techdriver->tearNewRules(net->ifname); + return techdriver->tearNewRules(binding->portdevname); }

static int -virNWFilterTearOldFilter(virDomainNetDefPtr net) +virNWFilterTearOldFilter(virNWFilterBindingPtr binding) { const char *drvname = EBIPTABLES_DRIVER_ID; int ifindex; @@ -990,12 +953,12 @@ virNWFilterTearOldFilter(virDomainNetDefPtr net) }

/* don't tear anything while the address is being learned */ - if (virNetDevGetIndex(net->ifname, &ifindex) < 0) + if (virNetDevGetIndex(binding->portdevname, &ifindex) < 0) virResetLastError(); else if (virNWFilterHasLearnReq(ifindex)) return 0;

- return techdriver->tearOldRules(net->ifname); + return techdriver->tearOldRules(binding->portdevname); }

@@ -1032,11 +995,11 @@ _virNWFilterTeardownFilter(const char *ifname)

int -virNWFilterTeardownFilter(const virDomainNetDef *net) +virNWFilterTeardownFilter(virNWFilterBindingPtr binding) { int ret; virMutexLock(&updateMutex); - ret = _virNWFilterTeardownFilter(net->ifname); + ret = _virNWFilterTeardownFilter(binding->portdevname); virMutexUnlock(&updateMutex); return ret; } @@ -1057,12 +1020,21 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj, if (virDomainObjIsActive(obj)) { for (i = 0; i < vm->nnets; i++) { virDomainNetDefPtr net = vm->nets[i]; + virNWFilterBinding binding = { + .ownername = vm->name, + .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, + }; + memcpy(binding.owneruuid, vm->uuid, sizeof(binding.owneruuid));

I'd prefer virNWFilterBindingForNet here too.

if ((net->filter) && (net->ifname)) { switch (cb->step) { case STEP_APPLY_NEW: ret = virNWFilterUpdateInstantiateFilter(cb->opaque, - vm->uuid, - net, + &binding, &skipIface); if (ret == 0 && skipIface) { /* filter tree unchanged -- no update needed */ @@ -1074,18 +1046,17 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj,

case STEP_TEAR_NEW: if (!virHashLookup(cb->skipInterfaces, net->ifname)) - ret = virNWFilterRollbackUpdateFilter(net); + ret = virNWFilterRollbackUpdateFilter(&binding); break;

case STEP_TEAR_OLD: if (!virHashLookup(cb->skipInterfaces, net->ifname)) - ret = virNWFilterTearOldFilter(net); + ret = virNWFilterTearOldFilter(&binding); break;

case STEP_APPLY_CURRENT: ret = virNWFilterInstantiateFilter(cb->opaque, - vm->uuid, - net); + &binding); if (ret) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failure while applying current filter on " @@ -1101,3 +1072,45 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj, virObjectUnlock(obj); return ret; } + + +virNWFilterBindingPtr virNWFilterBindingForNet(const char *vmname,

The type should be on a separate line.

+ const unsigned char *vmuuid, + virDomainNetDefPtr net)

This function would better fit in nwfilter_conf.c next to virNWFilterBindingCopy and it could even be added in the same patch. Or is there a good reason for having this function in nwfilter_gentech_driver.c?

+{ + virNWFilterBindingPtr 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 && + net->data.direct.linkdev &&

VIR_STRDUP accepts NULL source.

+ 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: + virNWFilterBindingFree(ret); + return NULL; +} diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 86cc677e79..0d846dc92f 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -37,25 +37,17 @@ enum instCase { INSTANTIATE_FOLLOW_NEWFILTER, };

-

Unrelated.

int virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net); + virNWFilterBindingPtr binding); int virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net, + virNWFilterBindingPtr binding, bool *skipIface);

int virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const char *ifname, - int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams); + virNWFilterBindingPtr binding, + int ifindex);

-int virNWFilterTeardownFilter(const virDomainNetDef *net); +int virNWFilterTeardownFilter(virNWFilterBindingPtr binding);

virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, const virNWFilterVarValue *value); @@ -63,4 +55,8 @@ virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, int virNWFilterDomainFWUpdateCB(virDomainObjPtr vm, void *data);

+virNWFilterBindingPtr virNWFilterBindingForNet(const char *vmname, + const unsigned char *vmuuid, + virDomainNetDefPtr net); + #endif diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index cc3bfd971c..4b13370661 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -643,19 +643,21 @@ learnIPAddressThread(void *arg) virNWFilterUnlockIface(req->ifname);

if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) { + virNWFilterBinding binding = { + .portdevname = req->ifname, + .linkdevname = req->linkdev, + .mac = req->macaddr, + .filter = req->filtername, + .filterparams = req->filterparams, + }; if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) { VIR_ERROR(_("Failed to add IP address %s to IP address " "cache for interface %s"), inetaddr, req->ifname); }

ret = virNWFilterInstantiateFilterLate(req->driver, - NULL, - req->ifname, - req->ifindex, - req->linkdev, - &req->macaddr, - req->filtername, - req->filterparams); + &binding, + req->ifindex); VIR_DEBUG("Result from applying firewall rules on " "%s with IP addr %s : %d", req->ifname, inetaddr, ret); VIR_FREE(inetaddr);

Jirka