On Oct 18, 2011, at 10:12 AM, Ben Pfaff wrote:
> STP_ID_ARGS should parenthesize the name of its argument, in case
> someone passes in a complicated expression (unlikely as that is).
Whoops.
> But can't the ID formatting macros be simplified to:
>
> #define STP_ID_FMT "%04"PRIx16"."012"PRIx64
> #define STP_ID_ARG(stp_id) \
> (uint16_t)((stp_id) >> 48), \
> (uint64_t)((stp_id) & 0xffffffffffffULL)
That's cleaner. Thanks.
> and I'm not sure that we really need port id macros at all. I think
> that "%04"PRIx16 works OK. (But it might make sense to use macros for
> that; it's a reasonable choice.)
I wanted to provide a macro since there's no consistency in switch UIs about
whether they should be in decimal or hex. I figured this would make it less
likely that we'd introduce an inconsistency.
> Can the STP_ROLE_* constants have some comments? Maybe "Port closest
> to root bridge.", "Forwards frames toward/away from root.", "Not used
> for forwarding." or similar.
How about these explanations?
STP_ROLE_ROOT, /* Path to root bridge. */
STP_ROLE_DESIGNATED, /* Path to LAN segments. */
STP_ROLE_ALTERNATE /* Backup path to root bridge. */
STP_ROLE_DISABLED, /* Port does not participate in STP. */
> Should there be a role for "disabled" ports? I think that they are
> technically different from alternate ports.
In the current code, a disabled port wouldn't be considered to have a role.
However, I think it would be better to still display a role even if the port is
disabled on a bridge running STP. I'll make that change.
The spec didn't provide a concise description of what's allowed in the states,
so I've also added the following patch:
-=-=-=-=-=-=-=-=-=-
@@ -97,6 +92,20 @@ bool stp_get_changed_port(struct stp *, struct stp_port **por
* participate in the spanning tree, but it still forwards traffic as if
* it were in the STP_FORWARDING state. This may be different from
* other implementations.
+ *
+ * The following diagram describes the various states and what they are
+ * allowed to do in OVS:
+ *
+ * FWD LRN TX_BPDU RX_BPDU
+ * --- --- ------- -------
+ * Disabled Y N N N
+ * Blocking N N N Y
+ * Listening N N Y Y
+ * Learning N Y Y Y
+ * Forwarding Y Y Y Y
+ *
+ * Once again, note that the disabled state forwards traffic, which is
+ * likely different than the spec would indicate.
*/
enum stp_state {
STP_DISABLED = 1 << 0, /* 8.4.5: See note above. */
-=-=-=-=-=-=-=-=-=-
Let me know if you see any problems with it.
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
