>
> @@ -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.
>
Yes, you analysis sounds correct to me.
> 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.
>
Thx for the comments, will refine the patch accordingly,
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
