On 16 July 2014 14:19, Alex Wang <al...@nicira.com> wrote:
> Commit 0d1cee123a84 (stp: Fix bpdu tx problem in listening state)
> makes ovs drop the stp bpdu packets if stp is not enabled on the
> input port. However, when pif bridge is used and stp is enabled
> on the integration bridge. The flow translation of stp bpdu
> packets will go through a level of resubmission which changes
> the input port to the corresponding peer port. Since, the
> patch port on the pif bridge does not have stp enabled, the
> flow translation will drop the bpdu packets.
>
> This commit fixes the issue by making ovs forward stp bpdu packets
> on stp-disabled port.
>
Wow, that was a lot more complicated than I expected it to be. Thanks for
finding this.
@@ -685,12 +685,16 @@ stp_learn_in_state(enum stp_state state)
> }
>
> /* Returns true if 'state' is one in which rx&tx bpdu should be done on
> - * on a port, false otherwise. */
> + * on a port, false otherwise.
> + *
> + * Returns true if 'state' is STP_DISABLED, since presumably in that case
> the
> + * port should still work, just not have STP applied to it. */
> bool
> stp_listen_in_state(enum stp_state state)
> {
> return (state &
> - (STP_LISTENING | STP_LEARNING | STP_FORWARDING)) != 0;
> + ( STP_DISABLED | STP_LISTENING | STP_LEARNING
> + | STP_FORWARDING)) != 0;
> }
>
I think we could improve the readability of this quite a bit. I'll explain
my understanding, correct me if I've misread something:
RX_BPDU and TX_BPDU, as per lib/stp.h, mean that OVS will generate and
consume STP BPDUs. We are changing this function to cover this case, and
also to forward BPDUs received from external sources, as a "dumb hub" would
do. So, we should update the comment above this function to describe this
behaviour.
Furthermore, it would be nice to rename this function to make it more clear
what it's doing. Something along the lines of allowing/forwarding BPDUs.
lib/stp.h leaves a bit of ambiguity on how this actually works, a couple of
suggestions (Not necessary for this bugfix, but would be good to update
this to make it more clear):
Firstly, we could add an additional column to the table:
* FWD_BPDU...
* -------- ...
* Disabled Y...
* Blocking -...
* Listening -...
* Learning -...
* Forwarding -...
Secondly, maybe explicitly mention above this table that RX_BPDU and
TX_BPDU means OVS-generated/consumed STP.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
