Re: [libvirt] [PATCH] nwfilter: fix adding std MAC and IP values to filter binding

On Thu, Mar 21, 2019 at 10:32:07AM +0300, Nikolay Shirokovskiy wrote: > Commit d1a7c08eb changed filter instantiation code to ignore MAC and IP > variables explicitly specified for filter binding. It just replaces > explicit values with values associated with the binding. Before the > commit virNWFilterCreateVarsFrom was used so that explicit value > take precedence. Let's bring old behavior back. > > This is useful. For example if domain has two interfaces it makes > sense to list both mac adresses in MAC var of every interface > filterref. So that if guest make a bond of these interfaces > and start sending frames with one of the mac adresses from > both interfaces we can pass outgress traffic from both > interfaces too. I'm not seeing what is supposed to be broken by d1a7c08eb. I have a guest which has a filter defined <interface type='network'> <mac address='52:54:00:7b:35:93'/> <source network='default' bridge='virbr0'/> <target dev='vnet0'/> <model type='rtl8139'/> <filterref filter='clean-traffic'> <parameter name='IP' value='104.207.129.11'/> </filterref> <alias name='net0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </interface> This IP parameter is reflected in the binding # virsh nwfilter-binding-dumpxml vnet0 <filterbinding> <owner> <name>memtest</name> <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid> </owner> <portdev name='vnet0'/> <mac address='52:54:00:7b:35:93'/> <filterref filter='clean-traffic'> <parameter name='IP' value='104.207.129.11'/> <parameter name='MAC' value='52:54:00:7b:35:93'/> </filterref> </filterbinding> and in the ebtables rules Bridge chain: I-vnet0-ipv4-ip, entries: 3, policy: ACCEPT -p IPv4 --ip-src 0.0.0.0 --ip-proto udp -j RETURN -p IPv4 --ip-src 104.207.129.11 -j RETURN -j DROP So what's the bug ?

> > Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > --- > src/nwfilter/nwfilter_gentech_driver.c | 92 ++++++++++++---------------------- > 1 file changed, 32 insertions(+), 60 deletions(-) > > diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c > index 655f088..6d68189 100644 > --- a/src/nwfilter/nwfilter_gentech_driver.c > +++ b/src/nwfilter/nwfilter_gentech_driver.c > @@ -127,60 +127,6 @@ virNWFilterRuleInstFree(virNWFilterRuleInstPtr inst) > > > /** > - * virNWFilterVarHashmapAddStdValues: > - * @tables: pointer to hash tabel to add values to > - * @macaddr: The string of the MAC address to add to the hash table, > - * may be NULL > - * @ipaddr: The string of the IP address to add to the hash table; > - * may be NULL > - * > - * Returns 0 in case of success, -1 in case an error happened with > - * error having been reported. > - * > - * Adds a couple of standard keys (MAC, IP) to the hash table. > - */ > -static int > -virNWFilterVarHashmapAddStdValues(virHashTablePtr table, > - const char *macaddr, > - const virNWFilterVarValue *ipaddr) > -{ > - virNWFilterVarValue *val; > - > - if (macaddr) { > - val = virNWFilterVarValueCreateSimpleCopyValue(macaddr); > - if (!val) > - return -1; > - > - if (virHashUpdateEntry(table, > - NWFILTER_STD_VAR_MAC, > - val) < 0) { > - virNWFilterVarValueFree(val); > - virReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("Could not add variable 'MAC' to hashmap")); > - return -1; > - } > - } > - > - if (ipaddr) { > - val = virNWFilterVarValueCopy(ipaddr); > - if (!val) > - return -1; > - > - if (virHashUpdateEntry(table, > - NWFILTER_STD_VAR_IP, > - val) < 0) { > - virNWFilterVarValueFree(val); > - virReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("Could not add variable 'IP' to hashmap")); > - return -1; > - } > - } > - > - return 0; > -} > - > - > -/** > * Convert a virHashTable into a string of comma-separated > * variable names. > */ > @@ -705,6 +651,28 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, > } > > > +static int > +virNWFilterVarHashmapAddStdValue(virHashTablePtr table, > + const char *var, > + const char *value) > +{ > + virNWFilterVarValue *val; > + > + if (virHashLookup(table, var)) > + return 0; > + > + if (!(val = virNWFilterVarValueCreateSimpleCopyValue(value))) > + return -1; > + > + if (virHashAddEntry(table, var, val) < 0) { > + virNWFilterVarValueFree(val); > + return -1; > + } > + > + return 0; > +} > + > + > /* > * Call this function while holding the NWFilter filter update lock > */ > @@ -717,7 +685,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, > bool forceWithPendingReq, > bool *foundNewFilter) > { > - int rc; > + int rc = -1; > const char *drvname = EBIPTABLES_DRIVER_ID; > virNWFilterTechDriverPtr techdriver; > virNWFilterObjPtr obj; > @@ -743,14 +711,18 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, > return -1; > > virMacAddrFormat(&binding->mac, vmmacaddr); > + if (virNWFilterVarHashmapAddStdValue(binding->filterparams, > + NWFILTER_STD_VAR_MAC, > + vmmacaddr) < 0) > + goto err_exit; > > ipaddr = virNWFilterIPAddrMapGetIPAddr(binding->portdevname); > - > - if (virNWFilterVarHashmapAddStdValues(binding->filterparams, > - vmmacaddr, ipaddr) < 0) { > - rc = -1; > + if (ipaddr && > + virNWFilterVarHashmapAddStdValue(binding->filterparams, > + NWFILTER_STD_VAR_IP, > + virNWFilterVarValueGetSimple(ipaddr)) < 0) > goto err_exit; > - } > + > > filter = virNWFilterObjGetDef(obj); > > -- > 1.8.3.1 > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list Regards, Daniel