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é <berra...@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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
