[ovs-dev] [STP 5/9] Add back 802.1D Spanning Tree Protocol (STP) library code.

[Ben Pfaff]

On Mon, Oct 17, 2011 at 10:31:45PM -0700, Justin Pettit wrote:
> At one point, the OVS distribution contained an IEEE 802.1D Spanning
> Tree Protocol (STP) library written by Ben Pfaff and based on the
> 802.1D-1998 reference code.  It was never integrated into ovs-vswitchd,
> so it was removed as part of commit ba18611 (Remove vestigial support
> for Spanning Tree Protocol.)
> 
> This commit reintroduces the library, cleans up a few spots, and makes
> it build cleanly against new code.  A future commit will have
> ovs-vswitchd use this library.

"git am" says:

    Applying: Add back 802.1D Spanning Tree Protocol (STP) library code.
    /home/blp/db/.git/rebase-apply/patch:241: trailing whitespace.
     * arguments to 'send_bpdu' are an STP BPDU encapsulated in 'bpdu', 
    /home/blp/db/.git/rebase-apply/patch:1547: trailing whitespace.
    # 
    /home/blp/db/.git/rebase-apply/patch:1995: trailing whitespace.
    dump_lan_tree(struct test_case *tc, struct lan *lan, int level) 
    /home/blp/db/.git/rebase-apply/patch:2259: trailing whitespace.
				lan = tc->lans[*token - 'a']; 
    /home/blp/db/.git/rebase-apply/patch:1440: new blank line at EOF.
    +
    warning: 5 lines add whitespace errors.

"sparse" says:

    ../lib/stp.c:980:1: warning: symbol 'stp_received_config_bpdu' was not
    declared. Should it be static?
    ../lib/stp.c:1018:1: warning: symbol 'stp_received_tcn_bpdu' was not
    declared. Should it be static?

You should add 2011 to the copyright notices, at least if you changed
anything.

You should use ovs_be<N> for big-endian values:

diff --git a/lib/stp.c b/lib/stp.c
index f58d640..0bdd54b 100644
--- a/lib/stp.c
+++ b/lib/stp.c
@@ -40,7 +40,7 @@ VLOG_DEFINE_THIS_MODULE(stp);
 #define STP_TYPE_TCN 0x80
 
 struct stp_bpdu_header {
-    uint16_t protocol_id;       /* STP_PROTOCOL_ID. */
+    ovs_be16 protocol_id;       /* STP_PROTOCOL_ID. */
     uint8_t protocol_version;   /* STP_PROTOCOL_VERSION. */
     uint8_t bpdu_type;          /* One of STP_TYPE_*. */
 } __attribute__((packed));
@@ -54,14 +54,14 @@ enum stp_config_bpdu_flags {
 struct stp_config_bpdu {
     struct stp_bpdu_header header; /* Type STP_TYPE_CONFIG. */
     uint8_t flags;                 /* STP_CONFIG_* flags. */
-    uint64_t root_id;              /* 8.5.1.1: Bridge believed to be root. */
-    uint32_t root_path_cost;       /* 8.5.1.2: Cost of path to root. */
-    uint64_t bridge_id;            /* 8.5.1.3: ID of transmitting bridge. */
-    uint16_t port_id;              /* 8.5.1.4: Port transmitting the BPDU. */
-    uint16_t message_age;          /* 8.5.1.5: Age of BPDU at tx time. */
-    uint16_t max_age;              /* 8.5.1.6: Timeout for received data. */
-    uint16_t hello_time;           /* 8.5.1.7: Time between BPDU generation. */
-    uint16_t forward_delay;        /* 8.5.1.8: State progression delay. */
+    ovs_be64 root_id;              /* 8.5.1.1: Bridge believed to be root. */
+    ovs_be32 root_path_cost;       /* 8.5.1.2: Cost of path to root. */
+    ovs_be64 bridge_id;            /* 8.5.1.3: ID of transmitting bridge. */
+    ovs_be16 port_id;              /* 8.5.1.4: Port transmitting the BPDU. */
+    ovs_be16 message_age;          /* 8.5.1.5: Age of BPDU at tx time. */
+    ovs_be16 max_age;              /* 8.5.1.6: Timeout for received data. */
+    ovs_be16 hello_time;           /* 8.5.1.7: Time between BPDU generation. */
+    ovs_be16 forward_delay;        /* 8.5.1.8: State progression delay. */
 } __attribute__((packed));
 BUILD_ASSERT_DECL(sizeof(struct stp_config_bpdu) == 35);
 
and then we get some useful warnings from sparse (at least with the
patch to include/linux/types.h that I posted separately):

    ../lib/stp.c:718:26: warning: invalid assignment: |=
    ../lib/stp.c:718:26:    left side has type unsigned char
    ../lib/stp.c:718:26:    right side has type restricted __be16
    ../lib/stp.c:721:26: warning: invalid assignment: |=
    ../lib/stp.c:721:26:    left side has type unsigned char
    ../lib/stp.c:721:26:    right side has type restricted __be16
    ../lib/stp.c:779:49: warning: restricted __be16 degrades to integer
    ../lib/stp.c:1007:42: warning: restricted __be16 degrades to integer

I think I saw that you fixed those in the next commit though.

I didn't really read most of this patch, assuming that it was pretty
much the same as what was in the tree before.