[libvirt] [PATCH V1 5/9] Add support for STP filtering
Stefan Berger
stefanb at linux.vnet.ibm.com
Tue Nov 22 14:51:49 UTC 2011
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 at linux.vnet.ibm.com>
>>
>> ---
>> 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.
>
More information about the libvir-list
mailing list