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

[Daniel P. Berrangé]

On Thu, May 03, 2018 at 02:57:04PM +0100, Daniel P. Berrangé wrote:
> On Mon, Apr 30, 2018 at 05:05:40PM +0200, Jiri Denemark wrote:
> > 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 at 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_driver.c b/src/nwfilter/nwfilter_driver.c
> > > index d17a8ec00b..a375e9bda8 100644
> > > --- a/src/nwfilter/nwfilter_driver.c
> > > +++ b/src/nwfilter/nwfilter_driver.c
> 
> > >  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.
> 
> Your point is right, but it isn't worth doing as this is just temporary
> code that's deleted again in patch 13 :-)
> 
> > > 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
> 
> > >  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.
> 
> We still make a copy of filterparams higher up in the call stack.
> virNWFilterBindingForNet() deep-copies what is in the virDomainNetPtr
> object - I discovered the need for this the hard way when I saw the
> domain XML gaining these two parameters :-)
> 
> 
> > > @@ -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.
> 
> This code gets removed in the last patch in this series too.

Actually in this case, it is critical to use virNWFilterBindingForNet
to avoid net->filterparams getting splattered during rebuild. So I must
make this change now to preserve git bisectability.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|