[libvirt] [PATCH 09/14] nwfilter: convert the gentech driver code to use virNWFilterBinding
Daniel P. Berrangé
berrange at redhat.com
Thu May 3 13:57:04 UTC 2018
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.
> > @@ -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?
I'm undecided on this point. Ultimately I'll probably want to push
the virDomainNetDefPtr -> virNWFilterBindingPtr conversion into the
virt drivers, so they can parse an XML doc of the virNWFilterBindingPtr
into the nwfilter daemon. So it'll almost certainly end up outside
the nwfilter codebase, but not 100% decided where yet.
>
> > +{
> > + 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;
> > +}
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 :|
More information about the libvir-list
mailing list