[libvirt] [PATCH V1 5/9] Add support for STP filtering

[Stefan Berger]

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.
>