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é <berra...@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 <jfer...@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;
>  

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to