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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
