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