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

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.

> 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.

> @@ -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.

> @@ -1101,3 +1072,45 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj, > virObjectUnlock(obj); > return ret; > } > + > + > +virNWFilterBindingPtr virNWFilterBindingForNet(const char *vmname, The type should be on a separate line. > + const unsigned char *vmuuid, > + virDomainNetDefPtr net) This function would better fit in nwfilter_conf.c next to virNWFilterBindingCopy and it could even be added in the same patch. Or is there a good reason for having this function in nwfilter_gentech_driver.c?

> +{ > + virNWFilterBindingPtr ret; > + > + if (VIR_ALLOC(ret) < 0) > + return NULL; > + > + if (VIR_STRDUP(ret->ownername, vmname) < 0) > + goto error; > + > + memcpy(ret->owneruuid, vmuuid, sizeof(ret->owneruuid)); > + > + if (VIR_STRDUP(ret->portdevname, net->ifname) < 0) > + goto error; > + > + if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT && > + net->data.direct.linkdev && VIR_STRDUP accepts NULL source. > + VIR_STRDUP(ret->linkdevname, net->data.direct.linkdev) < 0) > + goto error; > + > + ret->mac = net->mac; > + > + if (VIR_STRDUP(ret->filter, net->filter) < 0) > + goto error; > + > + if (!(ret->filterparams = virNWFilterHashTableCreate(0))) > + goto error; > + > + if (net->filterparams && > + virNWFilterHashTablePutAll(net->filterparams, ret->filterparams) < 0) > + goto error; > + > + return ret; > + > + error: > + virNWFilterBindingFree(ret); > + return NULL; > +}