On 11/21/2011 05:50 PM, Eric Blake wrote:
On 10/26/2011 09:12 AM, Stefan Berger wrote:
This patch adds support for filtering of STP (spanning tree protocol) traffic
to the parser and makes us of the ebtables support for STP filtering. This code
now enables the filtering of traffic in chains with prefix 'stp'.
Signed-off-by: Stefan Berger<stefanb@xxxxxxxxxxxxxxxxxx>
---
docs/schemas/nwfilter.rng | 154 +++++++++++++++++++++++++
src/conf/nwfilter_conf.c | 178 ++++++++++++++++++++++++++++++
src/conf/nwfilter_conf.h | 41 ++++++
src/libvirt_private.syms | 1
src/nwfilter/nwfilter_ebiptables_driver.c | 90 +++++++++++++++
5 files changed, 461 insertions(+), 3 deletions(-)
Some conflict resolution, again in the rng, and also in context for
nwfilter_conf.c, but should be trivial.
You already pre-emptively mentioned STP chains in an earlier patch, and
I see the title of 7/9 mentions more about STP documentation, so I'll
assume that between those two, the new .rng additions are properly
documented.
@@ -1047,6 +1049,136 @@ static const virXMLAttr2Struct vlanAttri
}
};
+static const virXMLAttr2Struct stpAttributes[] = {
+ /* spanning tree uses a special destination MAC address */
+ {
+ .name = SRCMACADDR,
+ .datatype = DATATYPE_MACADDR,
+ .dataIdx = offsetof(virNWFilterRuleDef,
+ p.stpHdrFilter.ethHdr.dataSrcMACAddr),
+ },
+ {
+ .name = "forward-delay-hi",
+ .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX,
+ .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataFwdDelayHi),
+ },
+ COMMENT_PROP(stpHdrFilter),
I'm assuming this is an accurate layout mapping the on-the-wire struct
to named fields for reference in XML attributes, although I didn't
actually go hunt down an RFC to verify. Perhaps a comment pointing tot
the STP RFC might prove handy.
I don't think it's an RFC, but an IEEE standard. I couldn't find a
better page than this one, though:
http://www.javvin.com/protocolSTP.html
@@ -2979,6 +3149,14 @@ virNWFilterRuleDefDetailsFormat(virBuffe
item->u.u16);
break;
+ case DATATYPE_UINT32_HEX:
+ asHex = true;
+ /* fallthrough */
+ case DATATYPE_UINT32:
+ virBufferAsprintf(buf, asHex ? "0x%x" : "%d",
+ item->u.u32);
%u, not %d. Otherwise you introduce a spurious negative sign on values
with the most-significant-bit set.
Fixed.
Also, I'm not entirely sure whether %u and uint32_t always match, or if
there are some 32-bit platforms where uint32_t is long and this would
trigger a type mismatch warning from gcc. On the other hand, this code
only compiles on Linux where we know uint32_t is always int; using
<inttypes.h> for PRIu32 would be more portable, but that's a separate
cleanup.
@@ -290,6 +292,16 @@ _printDataType(virNWFilterVarCombIterPtr
}
break;
+ case DATATYPE_UINT32:
+ case DATATYPE_UINT32_HEX:
+ if (snprintf(buf, bufsize, asHex ? "0x%x" : "%d",
+ item->u.u32)>= bufsize) {
+ virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Buffer too small for uint32 type"));
Again, %u, not %d.
Fixed.
Also, this code tends to be called with a hard-coded allocation of
'number[20];', which is sufficient for uint32_t, but not long enough if
we ever expand to DATATYPE_UINT64. I'm wondering if we should use
"intprops.h" from gnulib, for the INT_BUFSIZE_BOUND() macro, rather than
a hard-coded 20. But at least this snprintf usage checked for error (I
noticed in 4/9 that you used snprintf without error checking).
Using that now with all occurrences of number[].
+ return 1;
Looks like this is code addition to an existing function with positive 1
return convention, so you can defer changing it to -1 until a later
patch that cleans up the entire function (I'm only worried about
completely new functions introduced by this patch).
+ case VIR_NWFILTER_RULE_PROTOCOL_STP:
+
+ /* cannot handle inout direction with srcmask set in reverse dir.
+ since this clashes with -d below... */
+ if (reverse&&
+ HAS_ENTRY_ITEM(&rule->p.stpHdrFilter.ethHdr.dataSrcMACAddr)) {
+ virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+ _("STP filtering in %s direction with "
+ "source MAC address set is not supported"),
+ virNWFilterRuleDirectionTypeToString(
+ VIR_NWFILTER_RULE_DIRECTION_INOUT));
+ return -1;
+ }
+
+ virBufferAsprintf(&buf,
+ CMD_DEF_PRE "%s -t %s -%%c %s %%s",
+ ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain);
Looks like you've got some rebasing to do, depending on whether you push
this or your env-var cleanup first.
That shell var replacement stuff is scheduled for after this patch series...
@@ -2907,7 +2992,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule
char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH];
char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP
: CHAINPREFIX_HOST_OUT_TEMP;
- char protostr[16] = { 0, };
+ char protostr[32] = { 0, };
PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname);
PRINT_CHAIN(chain, chainPrefix, ifname,
@@ -2916,6 +3001,9 @@ ebtablesCreateTmpSubChain(ebiptablesRule
switch (protoidx) {
case L2_PROTO_MAC_IDX:
break;
+ case L2_PROTO_STP_IDX:
+ snprintf(protostr, sizeof(protostr), "-d " NWFILTER_MAC_BGA);
+ break;
Ah - here's an unchecked snprintf, which is probably worth tweaking for
maintenance safety, especially since you just changed the size of
protostr above.
Fixed.
@@ -590,6 +609,121 @@
</interleave>
</define>
+<define name="stp-attributes">
+<interleave>
+<optional>
+<attribute name="type">
+<ref name="uint8range"/>
+</attribute>
+</optional>
If I understand RNG correctly,<interleave> isn't required for
attributes (only for sub-elements).
Fixed. I'll clean-up the rng at some point since I did this just about
everywhere...
We're starting to get enough comments and merge conflicts as I review
this, that I think it will help if you post a v2 with all of the
remaining patches from both this series and your $EBT patch rolled into
a single commit series showing the final order you plan to push in.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list