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

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(a)redhat.com&gt; > > --- > > 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.