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_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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn 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_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,
> + };
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.
> 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);
>
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.
> - 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));
I'd prefer virNWFilterBindingForNet here too.
> 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,
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;
> +}
> 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,
> };
>
> -
Unrelated.
> 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);
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
> > @@ -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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.