[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

> @@ -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 redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]