[ovs-dev] [PATCH 13/13] ofproto: Add NXM_NX_TUN_GBP_ID and NXM_NX_TUN_GBP_FLAGS

Ben Pfaff blp at nicira.com
Tue Feb 3 23:22:49 UTC 2015


On Fri, Jan 30, 2015 at 03:36:43PM +0100, Thomas Graf wrote:
> From: Madhu Challa <challa at noironetworks.com>
> 
> Introduces two new NXMs to represent VXLAN-GBP [0] fields.
> 
>   actions=load:0x10->NXM_NX_TUN_GBP_ID[],NORMAL
>   tun_gbp_id=0x10,actions=drop
> 
> This enables existing VXLAN tunnels to carry security label
> information such as a SELinux context to other network peers.
> 
> The values are carried to/from the datapath using the attribute
> OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS.
> 
> [0] https://tools.ietf.org/html/draft-smith-vxlan-group-policy-00
> 
> Signed-off-by: Madhu Challa <challa at noironetworks.com>

Applying only this patch and the one before it, I get build assertions
in flow.h:

    In file included from ../ofproto/bond.c:19:
    In file included from ../ofproto/bond.h:22:
    In file included from ../ofproto/ofproto-provider.h:37:
    In file included from ../lib/classifier.h:217:
    In file included from ../lib/match.h:20:
    ../lib/flow.h:141:1: error: bit-field 'build_assert_failed' has negative width
          (-1)
    BUILD_ASSERT_DECL(sizeof(struct flow) % sizeof(uint64_t) == 0);
    ^
    ../lib/util.h:55:42: note: expanded from macro 'BUILD_ASSERT_DECL'
            extern int (*build_assert(void))[BUILD_ASSERT__(EXPR)]
                                             ^
    ../lib/util.h:48:38: note: expanded from macro 'BUILD_ASSERT__'
            sizeof(struct { unsigned int build_assert_failed : (EXPR) ? 1 : -1; })
                                         ^

I build on 32-bit so that's probably key to the problem.  (That's
actually the only reason I stick with a 32-bit OS: I find problems that
others don't, early.)

I see other compiler errors but I think that's only because I didn't
apply the other patches.

Please use a 0x prefix (or "%#"PRIx8 in this case) for printing fields
in hex, so that output is unambiguous:
+    if (wc->masks.tunnel.gbp_flags) {
+        ds_put_format(s, "tun_gbp_flags=%"PRIx8",", tnl->gbp_flags);
+    }

ofp_print_packet_in() prints gbp_flags in decimal whereas it's in
hexadecimal everywhere else.

Here, I think the hole is shown in the wrong place; I don't think a hole
would go in the middle of a bunch of 1-byte members:
 struct flow_tnl {
     ovs_be64 tun_id;
     ovs_be32 ip_src;
     ovs_be32 ip_dst;
     uint16_t flags;
+    ovs_be16 gbp_id;
+    uint8_t  gbp_flags;
+    /* 1 byte hole */
     uint8_t ip_tos;
     uint8_t ip_ttl;
     ovs_be16 tp_src;
     ovs_be16 tp_dst;
 };

The diagram added to the manpage looks ugly in PDF or HTML output.  You
can use \f(CR (which is unfortunately groff-only as far as I know) to
fix that, e.g.:

diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index acb282a..d548923 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -1111,11 +1111,11 @@ VXLAN tunnel endpoint.
 The \fBtun_gbp_flags\fR field has the following format:
 .IP
 .in +2
-+-+-+-+-+-+-+-+-+
+\f(CR+-+-+-+-+-+-+-+-+\fR
 .br
-|-|D|-|-|A|-|-|-|
+\f(CR|-|D|-|-|A|-|-|-|\fR
 .br
-+-+-+-+-+-+-+-+-+
+\f(CR+-+-+-+-+-+-+-+-+\fR
 
 .B D :=
 Don't Learn bit. When set, this bit indicates that the egress



More information about the dev mailing list