On 05/15/2018 01:43 PM, Daniel P. Berrangé wrote:
Use the virNWFilterBindingDefPtr 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 | 22 ++- src/nwfilter/nwfilter_gentech_driver.c | 209 +++++++++++++------------ src/nwfilter/nwfilter_gentech_driver.h | 22 ++- src/nwfilter/nwfilter_learnipaddr.c | 16 +- 5 files changed, 167 insertions(+), 137 deletions(-)
There's a couple questions/nits below that are easily addressable, so Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index aff062ca7c..f24fec1638 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -497,15 +497,18 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
/* instantiate the filters */
- if (req->ifname) + if (req->ifname) { + virNWFilterBindingDef binding = { + .portdevname = req->ifname, + .linkdevname = req->linkdev, + .mac = req->macaddr, + .filter = req->filtername, + .filterparams = req->vars,
Should ownername & owneruuid be initialized? Or is this a case where we have some compiler option to make the contents of binding be NULL. Similar for other defs like this obviously. Looking at the path : virNWFilterInstantiateFilterLate -> virNWFilterInstantiateFilterUpdate -> virNWFilterDoInstantiate -> virNWFilterDHCPSnoopReq(..., binding->owneruuid, ... ) The *Late function used to pass NULL for @vmuuid, but honestly it's not clear that the *SnoopReq would be called because if it ever was I don't think the code would happy in that case if @vmuuid == NULL. Still bug for bug compatibility...
+ }; rc = virNWFilterInstantiateFilterLate(req->driver, - NULL, - req->ifname, - req->ifindex, - req->linkdev, - &req->macaddr, - req->filtername, - req->vars); + &binding, + req->ifindex); + }
exit_snooprequnlock: virNWFilterSnoopReqUnlock(req);
[...]
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index af4411d4db..dc925dee16 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
Existing, but not all args are described.
* @@ -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, + virNWFilterBindingDefPtr 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)
[...]
static int virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, bool teardownOld, - const char *ifname, + virNWFilterBindingDefPtr 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);
- vars1 = virNWFilterCreateVarHashmap(vmmacaddr, ipaddr);
^^ This is the last consumer of virNWFilterCreateVarHashmap... So it can either be trashed now or later in a separate path, IDC.
- 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, + virNWFilterBindingDefPtr binding, bool teardownOld, enum instCase useNewFilter, bool *foundNewFilter) { - const char *linkdev = (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) - ? net->data.direct.linkdev - : NULL;
I see this is replaced by the virNWFilterBindingDefForNet... I started thinking about whether it matters for the *Late path, but I don't think so. The names of functions are so similar one has to pay close attention to xxxUpdateInstantiateFilter and xxxFilterInstantiateUpdate, never mind the Late path 8-/ Found my trusting nwfilter function map very handy while reviewing all this ;-)... Some day soon I hope I can burn it!!
int ifindex; int rc;
[...]