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(a)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.