On Fri, Apr 27, 2018 at 16:25:02 +0100, Daniel P. Berrangé wrote: ... One space between virHashTablePtr and vars would be enough. Or fix the alignment if you want to keep it consistently aligned. ... The indentation could be updated here as well. ... Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com&gt;

On Fri, Apr 27, 2018 at 16:25:05 +0100, Daniel P. Berrangé wrote: ... The alignment needs some adjustment. However, the function has no users outside nwfilter_gentech_driver.c so a separate patch could make it static. Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com&gt;

On Fri, Apr 27, 2018 at 16:25:06 +0100, Daniel P. Berrangé wrote: Two empty lines between functions, please. The type should be on a separate line. Two empty lines between functions, please. The type should be on a separate line. VIR_STRDUP can be safely used on NULL source, you don't need to guard it with the extra check. The types should be on separate lines for consistency with the rest of this header file. See my replies to the following patches for possible modifications... Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com&gt;

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_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index aec68ab847..dc4e3cb834 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -486,15 +486,18 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl, /* instantiate the filters */ - if (req->ifname) + if (req->ifname) { + virNWFilterBinding binding = { + .portdevname = req->ifname, + .linkdevname = req->linkdev, + .mac = req->macaddr, + .filter = req->filtername, + .filterparams = req->vars, + }; rc = virNWFilterInstantiateFilterLate(req->driver, - NULL, - req->ifname, - req->ifindex, - req->linkdev, - &req->macaddr, - req->filtername, - req->vars); + &binding, + req->ifindex); + } exit_snooprequnlock: virNWFilterSnoopReqUnlock(req); @@ -873,14 +876,16 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, goto skip_instantiate; if (ipAddrLeft) { + virNWFilterBinding binding = { + .portdevname = req->ifname, + .linkdevname = req->linkdev, + .mac = req->macaddr, + .filter = req->filtername, + .filterparams = req->vars, + }; ret = virNWFilterInstantiateFilterLate(req->driver, - NULL, - req->ifname, - req->ifindex, - req->linkdev, - &req->macaddr, - req->filtername, - req->vars); + &binding, + req->ifindex); } else { virNWFilterVarValuePtr dhcpsrvrs = virHashLookup(req->vars, NWFILTER_VARNAME_DHCPSERVER); 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 @@ -642,19 +642,34 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, static int -nwfilterInstantiateFilter(const char *vmname ATTRIBUTE_UNUSED, +nwfilterInstantiateFilter(const char *vmname, const unsigned char *vmuuid, virDomainNetDefPtr net) { - return virNWFilterInstantiateFilter(driver, vmuuid, net); + virNWFilterBindingPtr binding; + int ret; + + if (!(binding = virNWFilterBindingForNet(vmname, vmuuid, net))) + return -1; + ret = virNWFilterInstantiateFilter(driver, binding); + virNWFilterBindingFree(binding); + return ret; } 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, + }; if ((net->ifname) && (net->filter)) - virNWFilterTeardownFilter(net); + virNWFilterTeardownFilter(&binding); } 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 @@ -577,12 +577,9 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, /** * virNWFilterDoInstantiate: - * @vmuuid: The UUID of the VM * @techdriver: The driver to use for instantiation + * @binding: description of port to bind the filter to * @filter: The filter to instantiate - * @ifname: The name of the interface to apply the rules to - * @vars: A map holding variable names and values used for instantiating - * the filter and its subfilters. * @forceWithPendingReq: Ignore the check whether a pending learn request * is active; 'true' only when the rules are applied late * @@ -596,17 +593,13 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, * Call this function while holding the NWFilter filter update lock */ static int -virNWFilterDoInstantiate(const unsigned char *vmuuid, - virNWFilterTechDriverPtr techdriver, +virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, + virNWFilterBindingPtr binding, virNWFilterDefPtr filter, - const char *ifname, int ifindex, - const char *linkdev, - virHashTablePtr vars, enum instCase useNewFilter, bool *foundNewFilter, bool teardownOld, - const virMacAddr *macaddr, virNWFilterDriverStatePtr driver, bool forceWithPendingReq) { @@ -628,14 +621,14 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, } rc = virNWFilterDetermineMissingVarsRec(filter, - vars, + binding->filterparams, missing_vars, useNewFilter, driver); if (rc < 0) goto err_exit; - lv = virHashLookup(vars, NWFILTER_VARNAME_CTRL_IP_LEARNING); + lv = virHashLookup(binding->filterparams, NWFILTER_VARNAME_CTRL_IP_LEARNING); if (lv) learning = virNWFilterVarValueGetNthValue(lv, 0); else @@ -652,19 +645,20 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, goto err_unresolvable_vars; } if (STRCASEEQ(learning, "dhcp")) { - rc = virNWFilterDHCPSnoopReq(techdriver, ifname, linkdev, - vmuuid, macaddr, - filter->name, vars, driver); + rc = virNWFilterDHCPSnoopReq(techdriver, binding->portdevname, + binding->linkdevname, + binding->owneruuid, &binding->mac, + filter->name, binding->filterparams, driver); goto err_exit; } else if (STRCASEEQ(learning, "any")) { if (!virNWFilterHasLearnReq(ifindex)) { rc = virNWFilterLearnIPAddress(techdriver, - ifname, + binding->portdevname, ifindex, - linkdev, - macaddr, + binding->linkdevname, + &binding->mac, filter->name, - vars, driver, + binding->filterparams, driver, DETECT_DHCP|DETECT_STATIC); } goto err_exit; @@ -688,7 +682,7 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, rc = virNWFilterDefToInst(driver, filter, - vars, + binding->filterparams, useNewFilter, foundNewFilter, &inst); @@ -705,22 +699,22 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, } if (instantiate) { - if (virNWFilterLockIface(ifname) < 0) + if (virNWFilterLockIface(binding->portdevname) < 0) goto err_exit; - rc = techdriver->applyNewRules(ifname, inst.rules, inst.nrules); + rc = techdriver->applyNewRules(binding->portdevname, inst.rules, inst.nrules); if (teardownOld && rc == 0) - techdriver->tearOldRules(ifname); + techdriver->tearOldRules(binding->portdevname); - if (rc == 0 && (virNetDevValidateConfig(ifname, NULL, ifindex) <= 0)) { + if (rc == 0 && (virNetDevValidateConfig(binding->portdevname, NULL, ifindex) <= 0)) { virResetLastError(); /* interface changed/disppeared */ - techdriver->allTeardown(ifname); + techdriver->allTeardown(binding->portdevname); rc = -1; } - virNWFilterUnlockIface(ifname); + virNWFilterUnlockIface(binding->portdevname); } err_exit: @@ -749,14 +743,9 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, */ 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); - vars1 = virNWFilterCreateVarHashmap(vmmacaddr, ipaddr); - if (!vars1) { + if (virNWFilterVarHashmapAddStdValues(binding->filterparams, + vmmacaddr, ipaddr) < 0) { rc = -1; goto err_exit; } - vars = virNWFilterCreateVarsFrom(vars1, - filterparams); - if (!vars) { - rc = -1; - goto err_exit_vars1; - } - filter = virNWFilterObjGetDef(obj); switch (useNewFilter) { @@ -819,17 +800,11 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, break; } - rc = virNWFilterDoInstantiate(vmuuid, techdriver, filter, - ifname, ifindex, linkdev, - vars, useNewFilter, foundNewFilter, - teardownOld, macaddr, driver, + rc = virNWFilterDoInstantiate(techdriver, binding, filter, + ifindex, useNewFilter, foundNewFilter, + teardownOld, driver, forceWithPendingReq); - virHashFree(vars); - - err_exit_vars1: - virHashFree(vars1); - err_exit: virNWFilterObjUnlock(obj); @@ -839,15 +814,11 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, static int virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net, + virNWFilterBindingPtr binding, bool teardownOld, enum instCase useNewFilter, bool *foundNewFilter) { - const char *linkdev = (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) - ? net->data.direct.linkdev - : NULL; int ifindex; int rc; @@ -856,8 +827,8 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, /* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed (while holding the lock) and we don't want to build new ones */ - if (virNetDevExists(net->ifname) != 1 || - virNetDevGetIndex(net->ifname, &ifindex) < 0) { + if (virNetDevExists(binding->portdevname) != 1 || + virNetDevGetIndex(binding->portdevname, &ifindex) < 0) { /* interfaces / VMs can disappear during filter instantiation; don't mark it as an error */ virResetLastError(); @@ -865,10 +836,10 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, goto cleanup; } - rc = virNWFilterInstantiateFilterUpdate(driver, vmuuid, teardownOld, - net->ifname, ifindex, linkdev, - &net->mac, net->filter, - net->filterparams, useNewFilter, + rc = virNWFilterInstantiateFilterUpdate(driver, teardownOld, + binding, + ifindex, + useNewFilter, false, foundNewFilter); cleanup: @@ -880,13 +851,8 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, int virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const char *ifname, - int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams) + virNWFilterBindingPtr binding, + int ifindex) { int rc; bool foundNewFilter = false; @@ -894,18 +860,17 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, virNWFilterReadLockFilterUpdates(); virMutexLock(&updateMutex); - rc = virNWFilterInstantiateFilterUpdate(driver, vmuuid, true, - ifname, ifindex, linkdev, - macaddr, filtername, filterparams, + rc = virNWFilterInstantiateFilterUpdate(driver, true, + binding, ifindex, INSTANTIATE_ALWAYS, true, &foundNewFilter); if (rc < 0) { /* something went wrong... 'DOWN' the interface */ - if ((virNetDevValidateConfig(ifname, NULL, ifindex) <= 0) || - (virNetDevSetOnline(ifname, false) < 0)) { + if ((virNetDevValidateConfig(binding->portdevname, NULL, ifindex) <= 0) || + (virNetDevSetOnline(binding->portdevname, false) < 0)) { virResetLastError(); /* assuming interface disappeared... */ - _virNWFilterTeardownFilter(ifname); + _virNWFilterTeardownFilter(binding->portdevname); } } @@ -918,12 +883,11 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, int virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net) + virNWFilterBindingPtr binding) { bool foundNewFilter = false; - return virNWFilterInstantiateFilterInternal(driver, vmuuid, net, + return virNWFilterInstantiateFilterInternal(driver, binding, 1, INSTANTIATE_ALWAYS, &foundNewFilter); @@ -932,13 +896,12 @@ virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net, + virNWFilterBindingPtr binding, bool *skipIface) { bool foundNewFilter = false; - int rc = virNWFilterInstantiateFilterInternal(driver, vmuuid, net, + int rc = virNWFilterInstantiateFilterInternal(driver, binding, 0, INSTANTIATE_FOLLOW_NEWFILTER, &foundNewFilter); @@ -948,7 +911,7 @@ virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver, } static int -virNWFilterRollbackUpdateFilter(const virDomainNetDef *net) +virNWFilterRollbackUpdateFilter(virNWFilterBindingPtr binding) { const char *drvname = EBIPTABLES_DRIVER_ID; int ifindex; @@ -964,17 +927,17 @@ virNWFilterRollbackUpdateFilter(const virDomainNetDef *net) } /* don't tear anything while the address is being learned */ - if (virNetDevGetIndex(net->ifname, &ifindex) < 0) + if (virNetDevGetIndex(binding->portdevname, &ifindex) < 0) virResetLastError(); else if (virNWFilterHasLearnReq(ifindex)) return 0; - return techdriver->tearNewRules(net->ifname); + return techdriver->tearNewRules(binding->portdevname); } static int -virNWFilterTearOldFilter(virDomainNetDefPtr net) +virNWFilterTearOldFilter(virNWFilterBindingPtr binding) { const char *drvname = EBIPTABLES_DRIVER_ID; int ifindex; @@ -990,12 +953,12 @@ virNWFilterTearOldFilter(virDomainNetDefPtr net) } /* don't tear anything while the address is being learned */ - if (virNetDevGetIndex(net->ifname, &ifindex) < 0) + if (virNetDevGetIndex(binding->portdevname, &ifindex) < 0) virResetLastError(); else if (virNWFilterHasLearnReq(ifindex)) return 0; - return techdriver->tearOldRules(net->ifname); + return techdriver->tearOldRules(binding->portdevname); } @@ -1032,11 +995,11 @@ _virNWFilterTeardownFilter(const char *ifname) int -virNWFilterTeardownFilter(const virDomainNetDef *net) +virNWFilterTeardownFilter(virNWFilterBindingPtr binding) { int ret; virMutexLock(&updateMutex); - ret = _virNWFilterTeardownFilter(net->ifname); + ret = _virNWFilterTeardownFilter(binding->portdevname); virMutexUnlock(&updateMutex); return ret; } @@ -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)); if ((net->filter) && (net->ifname)) { switch (cb->step) { case STEP_APPLY_NEW: ret = virNWFilterUpdateInstantiateFilter(cb->opaque, - vm->uuid, - net, + &binding, &skipIface); if (ret == 0 && skipIface) { /* filter tree unchanged -- no update needed */ @@ -1074,18 +1046,17 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj, case STEP_TEAR_NEW: if (!virHashLookup(cb->skipInterfaces, net->ifname)) - ret = virNWFilterRollbackUpdateFilter(net); + ret = virNWFilterRollbackUpdateFilter(&binding); break; case STEP_TEAR_OLD: if (!virHashLookup(cb->skipInterfaces, net->ifname)) - ret = virNWFilterTearOldFilter(net); + ret = virNWFilterTearOldFilter(&binding); break; case STEP_APPLY_CURRENT: ret = virNWFilterInstantiateFilter(cb->opaque, - vm->uuid, - net); + &binding); if (ret) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failure while applying current filter on " @@ -1101,3 +1072,45 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj, virObjectUnlock(obj); return ret; } + + +virNWFilterBindingPtr virNWFilterBindingForNet(const char *vmname, + const unsigned char *vmuuid, + virDomainNetDefPtr net) +{ + 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(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; +} diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 86cc677e79..0d846dc92f 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -37,25 +37,17 @@ enum instCase { INSTANTIATE_FOLLOW_NEWFILTER, }; - int virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net); + virNWFilterBindingPtr binding); int virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net, + virNWFilterBindingPtr binding, bool *skipIface); int virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const char *ifname, - int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams); + virNWFilterBindingPtr binding, + int ifindex); -int virNWFilterTeardownFilter(const virDomainNetDef *net); +int virNWFilterTeardownFilter(virNWFilterBindingPtr binding); virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, const virNWFilterVarValue *value); @@ -63,4 +55,8 @@ virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, int virNWFilterDomainFWUpdateCB(virDomainObjPtr vm, void *data); +virNWFilterBindingPtr virNWFilterBindingForNet(const char *vmname, + const unsigned char *vmuuid, + virDomainNetDefPtr net); + #endif diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index cc3bfd971c..4b13370661 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -643,19 +643,21 @@ learnIPAddressThread(void *arg) virNWFilterUnlockIface(req->ifname); if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) { + virNWFilterBinding binding = { + .portdevname = req->ifname, + .linkdevname = req->linkdev, + .mac = req->macaddr, + .filter = req->filtername, + .filterparams = req->filterparams, + }; if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) { VIR_ERROR(_("Failed to add IP address %s to IP address " "cache for interface %s"), inetaddr, req->ifname); } ret = virNWFilterInstantiateFilterLate(req->driver, - NULL, - req->ifname, - req->ifindex, - req->linkdev, - &req->macaddr, - req->filtername, - req->filterparams); + &binding, + req->ifindex); VIR_DEBUG("Result from applying firewall rules on " "%s with IP addr %s : %d", req->ifname, inetaddr, ret); VIR_FREE(inetaddr); -- 2.14.3

On Mon, Apr 30, 2018 at 05:05:40PM +0200, Jiri Denemark wrote: Your point is right, but it isn't worth doing as this is just temporary code that's deleted again in patch 13 :-) 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 :-) This code gets removed in the last patch in this series too. 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. 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 :|

Use the virNWFilterBinding struct i nthe IP address learning code directly. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/nwfilter/nwfilter_gentech_driver.c | 7 +-- src/nwfilter/nwfilter_learnipaddr.c | 98 ++++++++++------------------------ src/nwfilter/nwfilter_learnipaddr.h | 6 +-- 3 files changed, 30 insertions(+), 81 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index c755350586..514315f781 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -653,12 +653,9 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, } else if (STRCASEEQ(learning, "any")) { if (!virNWFilterHasLearnReq(ifindex)) { rc = virNWFilterLearnIPAddress(techdriver, - binding->portdevname, + binding, ifindex, - binding->linkdevname, - &binding->mac, - filter->name, - binding->filterparams, driver, + driver, DETECT_DHCP|DETECT_STATIC); } goto err_exit; diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 4b13370661..0e76921648 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -137,12 +137,8 @@ typedef struct _virNWFilterIPAddrLearnReq virNWFilterIPAddrLearnReq; typedef virNWFilterIPAddrLearnReq *virNWFilterIPAddrLearnReqPtr; struct _virNWFilterIPAddrLearnReq { virNWFilterTechDriverPtr techdriver; - char ifname[IF_NAMESIZE]; int ifindex; - char linkdev[IF_NAMESIZE]; - virMacAddr macaddr; - char *filtername; - virHashTablePtr filterparams; + virNWFilterBindingPtr binding; virNWFilterDriverStatePtr driver; enum howDetect howDetect; @@ -232,8 +228,7 @@ virNWFilterIPAddrLearnReqFree(virNWFilterIPAddrLearnReqPtr req) if (!req) return; - VIR_FREE(req->filtername); - virHashFree(req->filterparams); + virNWFilterBindingFree(req->binding); VIR_FREE(req); } @@ -404,8 +399,9 @@ learnIPAddressThread(void *arg) virNWFilterIPAddrLearnReqPtr req = arg; uint32_t vmaddr = 0, bcastaddr = 0; unsigned int ethHdrSize; - char *listen_if = (strlen(req->linkdev) != 0) ? req->linkdev - : req->ifname; + char *listen_if = (req->binding->linkdevname ? + req->binding->linkdevname : + req->binding->portdevname); int dhcp_opts_len; char macaddr[VIR_MAC_STRING_BUFLEN]; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -415,13 +411,13 @@ learnIPAddressThread(void *arg) enum howDetect howDetected = 0; virNWFilterTechDriverPtr techdriver = req->techdriver; - if (virNWFilterLockIface(req->ifname) < 0) + if (virNWFilterLockIface(req->binding->portdevname) < 0) goto err_no_lock; req->status = 0; /* anything change to the VM's interface -- check at least once */ - if (virNetDevValidateConfig(req->ifname, NULL, req->ifindex) <= 0) { + if (virNetDevValidateConfig(req->binding->portdevname, NULL, req->ifindex) <= 0) { virResetLastError(); req->status = ENODEV; goto done; @@ -435,12 +431,12 @@ learnIPAddressThread(void *arg) goto done; } - virMacAddrFormat(&req->macaddr, macaddr); + virMacAddrFormat(&req->binding->mac, macaddr); switch (req->howDetect) { case DETECT_DHCP: - if (techdriver->applyDHCPOnlyRules(req->ifname, - &req->macaddr, + if (techdriver->applyDHCPOnlyRules(req->binding->portdevname, + &req->binding->mac, NULL, false) < 0) { req->status = EINVAL; goto done; @@ -448,8 +444,8 @@ learnIPAddressThread(void *arg) virBufferAddLit(&buf, "src port 67 and dst port 68"); break; case DETECT_STATIC: - if (techdriver->applyBasicRules(req->ifname, - &req->macaddr) < 0) { + if (techdriver->applyBasicRules(req->binding->portdevname, + &req->binding->mac) < 0) { req->status = EINVAL; goto done; } @@ -495,7 +491,7 @@ learnIPAddressThread(void *arg) } /* check whether VM's dev is still there */ - if (virNetDevValidateConfig(req->ifname, NULL, req->ifindex) <= 0) { + if (virNetDevValidateConfig(req->binding->portdevname, NULL, req->ifindex) <= 0) { virResetLastError(); req->status = ENODEV; showError = false; @@ -527,7 +523,7 @@ learnIPAddressThread(void *arg) continue; } - if (virMacAddrCmpRaw(&req->macaddr, ether_hdr->ether_shost) == 0) { + if (virMacAddrCmpRaw(&req->binding->mac, ether_hdr->ether_shost) == 0) { /* packets from the VM */ if (etherType == ETHERTYPE_IP && @@ -566,7 +562,7 @@ learnIPAddressThread(void *arg) break; } } - } else if (virMacAddrCmpRaw(&req->macaddr, + } else if (virMacAddrCmpRaw(&req->binding->mac, ether_hdr->ether_dhost) == 0 || /* allow Broadcast replies from DHCP server */ virMacAddrIsBroadcastRaw(ether_hdr->ether_dhost)) { @@ -596,7 +592,7 @@ learnIPAddressThread(void *arg) ((char *)udphdr + sizeof(udphdr)); if (dhcp->op == 2 /* BOOTREPLY */ && virMacAddrCmpRaw( - &req->macaddr, + &req->binding->mac, &dhcp->chaddr[0]) == 0) { dhcp_opts_len = header.len - (ethHdrSize + iphdr->ihl * 4 + @@ -640,26 +636,19 @@ learnIPAddressThread(void *arg) * Also it is safe to unlock interface here because we stopped * capturing and applied necessary rules on the interface, while * instantiating a new filter doesn't require a locked interface.*/ - virNWFilterUnlockIface(req->ifname); + virNWFilterUnlockIface(req->binding->portdevname); if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) { - virNWFilterBinding binding = { - .portdevname = req->ifname, - .linkdevname = req->linkdev, - .mac = req->macaddr, - .filter = req->filtername, - .filterparams = req->filterparams, - }; - if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) { + if (virNWFilterIPAddrMapAddIPAddr(req->binding->portdevname, inetaddr) < 0) { VIR_ERROR(_("Failed to add IP address %s to IP address " - "cache for interface %s"), inetaddr, req->ifname); + "cache for interface %s"), inetaddr, req->binding->portdevname); } ret = virNWFilterInstantiateFilterLate(req->driver, - &binding, + req->binding, req->ifindex); VIR_DEBUG("Result from applying firewall rules on " - "%s with IP addr %s : %d", req->ifname, inetaddr, ret); + "%s with IP addr %s : %d", req->binding->portdevname, inetaddr, ret); VIR_FREE(inetaddr); } } else { @@ -667,13 +656,13 @@ learnIPAddressThread(void *arg) virReportSystemError(req->status, _("encountered an error on interface %s " "index %d"), - req->ifname, req->ifindex); + req->binding->portdevname, req->ifindex); - techdriver->applyDropAllRules(req->ifname); - virNWFilterUnlockIface(req->ifname); + techdriver->applyDropAllRules(req->binding->portdevname); + virNWFilterUnlockIface(req->binding->portdevname); } - VIR_DEBUG("pcap thread terminating for interface %s", req->ifname); + VIR_DEBUG("pcap thread terminating for interface %s", req->binding->portdevname); err_no_lock: @@ -706,19 +695,14 @@ learnIPAddressThread(void *arg) */ int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, - const char *ifname, + virNWFilterBindingPtr binding, int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams, virNWFilterDriverStatePtr driver, enum howDetect howDetect) { int rc; virThread thread; virNWFilterIPAddrLearnReqPtr req = NULL; - virHashTablePtr ht = NULL; if (howDetect == 0) return -1; @@ -734,37 +718,11 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, if (VIR_ALLOC(req) < 0) goto err_no_req; - ht = virNWFilterHashTableCreate(0); - if (ht == NULL) + if (!(req->binding = virNWFilterBindingCopy(binding))) goto err_free_req; - if (virNWFilterHashTablePutAll(filterparams, ht) < 0) - goto err_free_ht; - - if (VIR_STRDUP(req->filtername, filtername) < 0) - goto err_free_ht; - - if (virStrcpyStatic(req->ifname, ifname) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Destination buffer for ifname ('%s') " - "not large enough"), ifname); - goto err_free_ht; - } - - if (linkdev) { - if (virStrcpyStatic(req->linkdev, linkdev) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Destination buffer for linkdev ('%s') " - "not large enough"), linkdev); - goto err_free_ht; - } - } - req->ifindex = ifindex; - virMacAddrSet(&req->macaddr, macaddr); req->driver = driver; - req->filterparams = ht; - ht = NULL; req->howDetect = howDetect; req->techdriver = techdriver; @@ -783,8 +741,6 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, err_dereg_req: virNWFilterDeregisterLearnReq(ifindex); - err_free_ht: - virHashFree(ht); err_free_req: virNWFilterIPAddrLearnReqFree(req); err_no_req: diff --git a/src/nwfilter/nwfilter_learnipaddr.h b/src/nwfilter/nwfilter_learnipaddr.h index 06fea5bff8..2646019ade 100644 --- a/src/nwfilter/nwfilter_learnipaddr.h +++ b/src/nwfilter/nwfilter_learnipaddr.h @@ -36,12 +36,8 @@ enum howDetect { }; int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, - const char *ifname, + virNWFilterBindingPtr binding, int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams, virNWFilterDriverStatePtr driver, enum howDetect howDetect); -- 2.14.3

If a <interface> includes a filter name but the nwfilter driver is not present we silently do nothing. This is very bad, because an application that thinks it is protected by malicious guest traffic will in fact be vulnerable. Reporting an error gives the administrator the ability to know there is a problem and fix it. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/conf/domain_nwfilter.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c index e360aceeba..7570e0ae83 100644 --- a/src/conf/domain_nwfilter.c +++ b/src/conf/domain_nwfilter.c @@ -28,6 +28,9 @@ #include "datatypes.h" #include "domain_conf.h" #include "domain_nwfilter.h" +#include "virerror.h" + +#define VIR_FROM_THIS VIR_FROM_NWFILTER static virDomainConfNWFilterDriverPtr nwfilterDriver; @@ -44,8 +47,10 @@ virDomainConfNWFilterInstantiate(const char *vmname, { if (nwfilterDriver != NULL) return nwfilterDriver->instantiateFilter(vmname, vmuuid, net); - /* driver module not available -- don't indicate failure */ - return 0; + + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("No network filter driver available")); + return -1; } void -- 2.14.3

Now that the nwfilter driver keeps a list of bindings that it has created, there is no need for the complex virt driver callbacks. It is possible to simply iterate of the list of recorded filter bindings. This means that rebuilding filters no longer has to acquire any locks on the virDomainObj objects, as they're never touched. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/conf/nwfilter_conf.c | 169 ++++++++++----------------------- src/conf/nwfilter_conf.h | 51 +--------- src/conf/virnwfilterobj.c | 4 +- src/libvirt_private.syms | 3 +- src/lxc/lxc_driver.c | 28 ------ src/nwfilter/nwfilter_driver.c | 22 +++-- src/nwfilter/nwfilter_gentech_driver.c | 169 ++++++++++++++++++++------------- src/nwfilter/nwfilter_gentech_driver.h | 4 +- src/qemu/qemu_driver.c | 25 ----- src/uml/uml_driver.c | 29 ------ 10 files changed, 175 insertions(+), 329 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 3d2ae9d0f3..83c9ff920f 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2819,121 +2819,6 @@ virNWFilterSaveConfig(const char *configDir, } -int nCallbackDriver; -#define MAX_CALLBACK_DRIVER 10 -static virNWFilterCallbackDriverPtr callbackDrvArray[MAX_CALLBACK_DRIVER]; - -void -virNWFilterRegisterCallbackDriver(virNWFilterCallbackDriverPtr cbd) -{ - if (nCallbackDriver < MAX_CALLBACK_DRIVER) - callbackDrvArray[nCallbackDriver++] = cbd; -} - - -void -virNWFilterUnRegisterCallbackDriver(virNWFilterCallbackDriverPtr cbd) -{ - size_t i = 0; - - while (i < nCallbackDriver && callbackDrvArray[i] != cbd) - i++; - - if (i < nCallbackDriver) { - memmove(&callbackDrvArray[i], &callbackDrvArray[i+1], - (nCallbackDriver - i - 1) * sizeof(callbackDrvArray[i])); - callbackDrvArray[i] = 0; - nCallbackDriver--; - } -} - - -void -virNWFilterCallbackDriversLock(void) -{ - size_t i; - - for (i = 0; i < nCallbackDriver; i++) - callbackDrvArray[i]->vmDriverLock(); -} - - -void -virNWFilterCallbackDriversUnlock(void) -{ - size_t i; - - for (i = 0; i < nCallbackDriver; i++) - callbackDrvArray[i]->vmDriverUnlock(); -} - - -static virDomainObjListIterator virNWFilterDomainFWUpdateCB; -static void *virNWFilterDomainFWUpdateOpaque; - -/** - * virNWFilterInstFiltersOnAllVMs: - * Apply all filters on all running VMs. Don't terminate in case of an - * error. This should be called upon reloading of the driver. - */ -int -virNWFilterInstFiltersOnAllVMs(void) -{ - size_t i; - struct domUpdateCBStruct cb = { - .opaque = virNWFilterDomainFWUpdateOpaque, - .step = STEP_APPLY_CURRENT, - .skipInterfaces = NULL, /* not needed */ - }; - - for (i = 0; i < nCallbackDriver; i++) - callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb); - - return 0; -} - - -int -virNWFilterTriggerVMFilterRebuild(void) -{ - size_t i; - int ret = 0; - struct domUpdateCBStruct cb = { - .opaque = virNWFilterDomainFWUpdateOpaque, - .step = STEP_APPLY_NEW, - .skipInterfaces = virHashCreate(0, NULL), - }; - - if (!cb.skipInterfaces) - return -1; - - for (i = 0; i < nCallbackDriver; i++) { - if (callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb) < 0) - ret = -1; - } - - if (ret < 0) { - cb.step = STEP_TEAR_NEW; /* rollback */ - - for (i = 0; i < nCallbackDriver; i++) - callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb); - } else { - cb.step = STEP_TEAR_OLD; /* switch over */ - - for (i = 0; i < nCallbackDriver; i++) - callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb); - } - - virHashFree(cb.skipInterfaces); - - return ret; -} - - int virNWFilterDeleteDef(const char *configDir, virNWFilterDefPtr def) @@ -3204,16 +3089,18 @@ virNWFilterDefFormat(const virNWFilterDef *def) return NULL; } +static virNWFilterTriggerRebuildCallback rebuildCallback; +static void *rebuildOpaque; int -virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, +virNWFilterConfLayerInit(virNWFilterTriggerRebuildCallback cb, void *opaque) { if (initialized) return -1; - virNWFilterDomainFWUpdateCB = domUpdateCB; - virNWFilterDomainFWUpdateOpaque = opaque; + rebuildCallback = cb; + rebuildOpaque = opaque; initialized = true; @@ -3233,8 +3120,50 @@ virNWFilterConfLayerShutdown(void) virRWLockDestroy(&updateLock); initialized = false; - virNWFilterDomainFWUpdateOpaque = NULL; - virNWFilterDomainFWUpdateCB = NULL; + rebuildCallback = NULL; + rebuildOpaque = NULL; +} + +int +virNWFilterTriggerRebuild(void) +{ +#if 0 + size_t i; + int ret = 0; + struct domUpdateCBStruct cb = { + .opaque = virNWFilterDomainFWUpdateOpaque, + .step = STEP_APPLY_NEW, + .skipInterfaces = virHashCreate(0, NULL), + }; + + if (!cb.skipInterfaces) + return -1; + + for (i = 0; i < nCallbackDriver; i++) { + if (callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, + &cb) < 0) + ret = -1; + } + + if (ret < 0) { + cb.step = STEP_TEAR_NEW; /* rollback */ + + for (i = 0; i < nCallbackDriver; i++) + callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, + &cb); + } else { + cb.step = STEP_TEAR_OLD; /* switch over */ + + for (i = 0; i < nCallbackDriver; i++) + callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, + &cb); + } + + virHashFree(cb.skipInterfaces); + + return ret; +#endif + return rebuildCallback(rebuildOpaque); } diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 8c5421ee62..3b36a02a78 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -559,20 +559,6 @@ struct virNWFilterBinding { }; -typedef enum { - STEP_APPLY_NEW, - STEP_TEAR_NEW, - STEP_TEAR_OLD, - STEP_APPLY_CURRENT, -} UpdateStep; - -struct domUpdateCBStruct { - void *opaque; - UpdateStep step; - virHashTablePtr skipInterfaces; -}; - - void virNWFilterRuleDefFree(virNWFilterRuleDefPtr def); @@ -580,7 +566,7 @@ void virNWFilterDefFree(virNWFilterDefPtr def); int -virNWFilterTriggerVMFilterRebuild(void); +virNWFilterTriggerRebuild(void); int virNWFilterDeleteDef(const char *configDir, @@ -612,44 +598,15 @@ virNWFilterReadLockFilterUpdates(void); void virNWFilterUnlockFilterUpdates(void); +typedef int (*virNWFilterTriggerRebuildCallback)(void *opaque); + int -virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, +virNWFilterConfLayerInit(virNWFilterTriggerRebuildCallback cb, void *opaque); void virNWFilterConfLayerShutdown(void); -int -virNWFilterInstFiltersOnAllVMs(void); - -typedef int -(*virNWFilterRebuild)(virDomainObjListIterator domUpdateCB, - void *data); - -typedef void -(*virNWFilterVoidCall)(void); - -typedef struct _virNWFilterCallbackDriver virNWFilterCallbackDriver; -typedef virNWFilterCallbackDriver *virNWFilterCallbackDriverPtr; -struct _virNWFilterCallbackDriver { - const char *name; - - virNWFilterRebuild vmFilterRebuild; - virNWFilterVoidCall vmDriverLock; - virNWFilterVoidCall vmDriverUnlock; -}; - -void -virNWFilterRegisterCallbackDriver(virNWFilterCallbackDriverPtr); - -void -virNWFilterUnRegisterCallbackDriver(virNWFilterCallbackDriverPtr); - -void -virNWFilterCallbackDriversLock(void); - -void -virNWFilterCallbackDriversUnlock(void); char * virNWFilterPrintTCPFlags(uint8_t flags); diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 87d7e72703..0136a0d56c 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -276,7 +276,7 @@ virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj) obj->wantRemoved = true; /* trigger the update on VMs referencing the filter */ - if (virNWFilterTriggerVMFilterRebuild() < 0) + if (virNWFilterTriggerRebuild() < 0) rc = -1; obj->wantRemoved = false; @@ -358,7 +358,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, obj->newDef = def; /* trigger the update on VMs referencing the filter */ - if (virNWFilterTriggerVMFilterRebuild() < 0) { + if (virNWFilterTriggerRebuild() < 0) { obj->newDef = NULL; virNWFilterObjUnlock(obj); return NULL; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9fc0aa470d..fd5edc86ad 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -792,7 +792,6 @@ virNWFilterDefFree; virNWFilterDefParseFile; virNWFilterDefParseString; virNWFilterDeleteDef; -virNWFilterInstFiltersOnAllVMs; virNWFilterJumpTargetTypeToString; virNWFilterPrintStateMatchFlags; virNWFilterPrintTCPFlags; @@ -805,7 +804,7 @@ virNWFilterRuleIsProtocolIPv4; virNWFilterRuleIsProtocolIPv6; virNWFilterRuleProtocolTypeToString; virNWFilterSaveConfig; -virNWFilterTriggerVMFilterRebuild; +virNWFilterTriggerRebuild; virNWFilterUnlockFilterUpdates; virNWFilterUnRegisterCallbackDriver; virNWFilterWriteLockFilterUpdates; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index ca01d369d5..a6b689aef0 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -66,7 +66,6 @@ #include "virfdstream.h" #include "domain_audit.h" #include "domain_nwfilter.h" -#include "nwfilter_conf.h" #include "virinitctl.h" #include "virnetdev.h" #include "virnetdevtap.h" @@ -95,31 +94,6 @@ static int lxcStateInitialize(bool privileged, static int lxcStateCleanup(void); virLXCDriverPtr lxc_driver = NULL; -/* callbacks for nwfilter */ -static int -lxcVMFilterRebuild(virDomainObjListIterator iter, void *data) -{ - return virDomainObjListForEach(lxc_driver->domains, iter, data); -} - -static void -lxcVMDriverLock(void) -{ - lxcDriverLock(lxc_driver); -} - -static void -lxcVMDriverUnlock(void) -{ - lxcDriverUnlock(lxc_driver); -} - -static virNWFilterCallbackDriver lxcCallbackDriver = { - .name = "LXC", - .vmFilterRebuild = lxcVMFilterRebuild, - .vmDriverLock = lxcVMDriverLock, - .vmDriverUnlock = lxcVMDriverUnlock, -}; /** * lxcDomObjFromDomain: @@ -1691,7 +1665,6 @@ static int lxcStateInitialize(bool privileged, NULL, NULL) < 0) goto cleanup; - virNWFilterRegisterCallbackDriver(&lxcCallbackDriver); virObjectUnref(caps); return 0; @@ -1764,7 +1737,6 @@ static int lxcStateCleanup(void) if (lxc_driver == NULL) return -1; - virNWFilterUnRegisterCallbackDriver(&lxcCallbackDriver); virObjectUnref(lxc_driver->domains); virObjectUnref(lxc_driver->domainEventState); diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index ccbcfbbf67..92389840fd 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -170,6 +170,15 @@ static void virNWFilterBindingDataFree(void *payload, const void *name ATTRIBUTE virNWFilterBindingFree(binding); } + +static int virNWFilterTriggerRebuildImpl(void *opaque) +{ + virNWFilterDriverStatePtr nwdriver = opaque; + + return virNWFilterBuildAll(nwdriver, true); +} + + /** * nwfilterStateInitialize: * @@ -216,7 +225,7 @@ nwfilterStateInitialize(bool privileged, if (virNWFilterTechDriversInit(privileged) < 0) goto err_dhcpsnoop_shutdown; - if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB, + if (virNWFilterConfLayerInit(virNWFilterTriggerRebuildImpl, driver) < 0) goto err_techdrivers_shutdown; @@ -306,15 +315,14 @@ nwfilterStateReload(void) nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); - virNWFilterCallbackDriversLock(); virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir); - virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); - nwfilterDriverUnlock(); - virNWFilterInstFiltersOnAllVMs(); + virNWFilterBuildAll(driver, false); + + nwfilterDriverUnlock(); return 0; } @@ -550,7 +558,6 @@ nwfilterDefineXML(virConnectPtr conn, nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); - virNWFilterCallbackDriversLock(); if (!(def = virNWFilterDefParseString(xml))) goto cleanup; @@ -575,7 +582,6 @@ nwfilterDefineXML(virConnectPtr conn, if (obj) virNWFilterObjUnlock(obj); - virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(); return nwfilter; @@ -591,7 +597,6 @@ nwfilterUndefine(virNWFilterPtr nwfilter) nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); - virNWFilterCallbackDriversLock(); if (!(obj = nwfilterObjFromNWFilter(nwfilter->uuid))) goto cleanup; @@ -618,7 +623,6 @@ nwfilterUndefine(virNWFilterPtr nwfilter) if (obj) virNWFilterObjUnlock(obj); - virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(); return ret; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 0dc51d16c5..5c83b06504 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -153,9 +153,9 @@ virNWFilterVarHashmapAddStdValues(virHashTablePtr table, if (!val) return -1; - if (virHashAddEntry(table, - NWFILTER_STD_VAR_MAC, - val) < 0) { + if (virHashUpdateEntry(table, + NWFILTER_STD_VAR_MAC, + val) < 0) { virNWFilterVarValueFree(val); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add variable 'MAC' to hashmap")); @@ -168,9 +168,9 @@ virNWFilterVarHashmapAddStdValues(virHashTablePtr table, if (!val) return -1; - if (virHashAddEntry(table, - NWFILTER_STD_VAR_IP, - val) < 0) { + if (virHashUpdateEntry(table, + NWFILTER_STD_VAR_IP, + val) < 0) { virNWFilterVarValueFree(val); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add variable 'IP' to hashmap")); @@ -1000,72 +1000,111 @@ virNWFilterTeardownFilter(virNWFilterBindingPtr binding) return ret; } +enum { + STEP_APPLY_NEW, + STEP_TEAR_NEW, + STEP_TEAR_OLD, + STEP_APPLY_CURRENT, +}; -int -virNWFilterDomainFWUpdateCB(virDomainObjPtr obj, - void *data) +static int +virNWFilterBuildOne(virNWFilterDriverStatePtr driver, + virNWFilterBindingPtr binding, + virHashTablePtr skipInterfaces, + int step) { - virDomainDefPtr vm = obj->def; - struct domUpdateCBStruct *cb = data; - size_t i; bool skipIface; int ret = 0; - - virObjectLock(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)); - if ((net->filter) && (net->ifname)) { - switch (cb->step) { - case STEP_APPLY_NEW: - ret = virNWFilterUpdateInstantiateFilter(cb->opaque, - &binding, - &skipIface); - if (ret == 0 && skipIface) { - /* filter tree unchanged -- no update needed */ - ret = virHashAddEntry(cb->skipInterfaces, - net->ifname, - (void *)~0); - } - break; - - case STEP_TEAR_NEW: - if (!virHashLookup(cb->skipInterfaces, net->ifname)) - ret = virNWFilterRollbackUpdateFilter(&binding); - break; - - case STEP_TEAR_OLD: - if (!virHashLookup(cb->skipInterfaces, net->ifname)) - ret = virNWFilterTearOldFilter(&binding); - break; - - case STEP_APPLY_CURRENT: - ret = virNWFilterInstantiateFilter(cb->opaque, - &binding); - if (ret) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failure while applying current filter on " - "VM %s"), vm->name); - break; - } - if (ret) - break; - } + VIR_DEBUG("Building filter for portdev=%s step=%d", binding->portdevname, step); + + switch (step) { + case STEP_APPLY_NEW: + ret = virNWFilterUpdateInstantiateFilter(driver, + binding, + &skipIface); + if (ret == 0 && skipIface) { + /* filter tree unchanged -- no update needed */ + ret = virHashAddEntry(skipInterfaces, + binding->portdevname, + (void *)~0); } + break; + + case STEP_TEAR_NEW: + if (!virHashLookup(skipInterfaces, binding->portdevname)) + ret = virNWFilterRollbackUpdateFilter(binding); + break; + + case STEP_TEAR_OLD: + if (!virHashLookup(skipInterfaces, binding->portdevname)) + ret = virNWFilterTearOldFilter(binding); + break; + + case STEP_APPLY_CURRENT: + ret = virNWFilterInstantiateFilter(driver, + binding); + break; } - virObjectUnlock(obj); + return ret; +} + + +struct virNWFilterBuildData { + virNWFilterDriverStatePtr driver; + virHashTablePtr skipInterfaces; + int step; +}; + +static int +virNWFilterBuildIter(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) +{ + virNWFilterBindingPtr binding = payload; + struct virNWFilterBuildData *data = opaque; + + return virNWFilterBuildOne(data->driver, binding, + data->skipInterfaces, data->step); +} + +int +virNWFilterBuildAll(virNWFilterDriverStatePtr driver, + bool newFilters) +{ + struct virNWFilterBuildData data = { + .driver = driver, + }; + int ret = 0; + + VIR_DEBUG("Build all filters newFilters=%d", newFilters); + + if (newFilters) { + if (!(data.skipInterfaces = virHashCreate(0, NULL))) + return -1; + + data.step = STEP_APPLY_NEW; + if (virHashForEach(driver->bindings, + virNWFilterBuildIter, + &data) < 0) + ret = -1; + + if (ret == -1) { + data.step = STEP_TEAR_NEW; + virHashForEach(driver->bindings, + virNWFilterBuildIter, + &data); + } else { + data.step = STEP_TEAR_OLD; + virHashForEach(driver->bindings, + virNWFilterBuildIter, + &data); + } + } else { + data.step = STEP_APPLY_CURRENT; + if (virHashForEach(driver->bindings, + virNWFilterBuildIter, + &data) < 0) + ret = -1; + } return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 0d846dc92f..8bfc323808 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -52,8 +52,8 @@ int virNWFilterTeardownFilter(virNWFilterBindingPtr binding); virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, const virNWFilterVarValue *value); -int virNWFilterDomainFWUpdateCB(virDomainObjPtr vm, - void *data); +int virNWFilterBuildAll(virNWFilterDriverStatePtr driver, + bool newFilters); virNWFilterBindingPtr virNWFilterBindingForNet(const char *vmname, const unsigned char *vmuuid, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7484b00e23..668891a119 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -84,7 +84,6 @@ #include "cpu/cpu.h" #include "virsysinfo.h" #include "domain_nwfilter.h" -#include "nwfilter_conf.h" #include "virhook.h" #include "virstoragefile.h" #include "virfile.h" @@ -164,28 +163,6 @@ static int qemuARPGetInterfaces(virDomainObjPtr vm, static virQEMUDriverPtr qemu_driver; - -static void -qemuVMDriverLock(void) -{} -static void -qemuVMDriverUnlock(void) -{} - -static int -qemuVMFilterRebuild(virDomainObjListIterator iter, void *data) -{ - return virDomainObjListForEach(qemu_driver->domains, iter, data); -} - -static virNWFilterCallbackDriver qemuCallbackDriver = { - .name = QEMU_DRIVER_NAME, - .vmFilterRebuild = qemuVMFilterRebuild, - .vmDriverLock = qemuVMDriverLock, - .vmDriverUnlock = qemuVMDriverUnlock, -}; - - /** * qemuDomObjFromDomain: * @domain: Domain pointer that has to be looked up @@ -938,7 +915,6 @@ qemuStateInitialize(bool privileged, if (!qemu_driver->workerPool) goto error; - virNWFilterRegisterCallbackDriver(&qemuCallbackDriver); return 0; error: @@ -1078,7 +1054,6 @@ qemuStateCleanup(void) if (!qemu_driver) return -1; - virNWFilterUnRegisterCallbackDriver(&qemuCallbackDriver); virThreadPoolFree(qemu_driver->workerPool); virObjectUnref(qemu_driver->config); virObjectUnref(qemu_driver->hostdevMgr); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index b50ba1ba64..4da8fdf473 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -55,7 +55,6 @@ #include "datatypes.h" #include "virlog.h" #include "domain_nwfilter.h" -#include "nwfilter_conf.h" #include "virfile.h" #include "virfdstream.h" #include "configmake.h" @@ -145,25 +144,6 @@ static int umlMonitorCommand(const struct uml_driver *driver, static struct uml_driver *uml_driver; -static int -umlVMFilterRebuild(virDomainObjListIterator iter, void *data) -{ - return virDomainObjListForEach(uml_driver->domains, iter, data); -} - -static void -umlVMDriverLock(void) -{ - umlDriverLock(uml_driver); -} - -static void -umlVMDriverUnlock(void) -{ - umlDriverUnlock(uml_driver); -} - - static virDomainObjPtr umlDomObjFromDomainLocked(struct uml_driver *driver, const unsigned char *uuid) @@ -196,13 +176,6 @@ umlDomObjFromDomain(struct uml_driver *driver, } -static virNWFilterCallbackDriver umlCallbackDriver = { - .name = "UML", - .vmFilterRebuild = umlVMFilterRebuild, - .vmDriverLock = umlVMDriverLock, - .vmDriverUnlock = umlVMDriverUnlock, -}; - struct umlAutostartData { struct uml_driver *driver; virConnectPtr conn; @@ -615,7 +588,6 @@ umlStateInitialize(bool privileged, VIR_FREE(userdir); - virNWFilterRegisterCallbackDriver(&umlCallbackDriver); return 0; out_of_memory: @@ -709,7 +681,6 @@ umlStateCleanup(void) return -1; umlDriverLock(uml_driver); - virNWFilterRegisterCallbackDriver(&umlCallbackDriver); if (uml_driver->inotifyWatch != -1) virEventRemoveHandle(uml_driver->inotifyWatch); VIR_FORCE_CLOSE(uml_driver->inotifyFD); -- 2.14.3