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.
> @@ -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.
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.
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).
> + 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.
> @@ -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.
> @@ -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).
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.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list