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