Sent from my iPhone
> On Sep 11, 2014, at 2:08 AM, Daniele Venturino <daniele.ventur...@m3s.it>
> wrote:
>
>
>> Il giorno 10/set/2014, alle ore 18:54, Jarno Rajahalme
>> <jrajaha...@nicira.com> ha scritto:
>>
>> Daniele,
>>
>> See comments below.
>>
>> Also, it would be preferable to send related changes, or multiple unrelated
>> changes to any given subsystem, as a series of patches instead of individual
>> ones. “git format-patch” does that automatically.
>>
>> Jarno
>
> I’ll send the next group of patches as a series.
>
>>> On Sep 10, 2014, at 1:28 AM, Daniele Venturino <daniele.ventur...@m3s.it>
>>> wrote:
>>>
>>> See commit bacdb85ad82f981697245eefb40a3b360cfe379b.
>>>
>>> Signed-off by: Daniele Venturino <daniele.ventur...@m3s.it>
>>>
>>> ---
>>> lib/rstp.h | 42 +++++++++++++++++++++++++++++++++++++++---
>>> ofproto/ofproto-dpif-xlate.c | 6 +++---
>>> 2 files changed, 42 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/rstp.h b/lib/rstp.h
>>> index 364a181..b15d22f 100644
>>> --- a/lib/rstp.h
>>> +++ b/lib/rstp.h
>>> @@ -99,6 +99,38 @@ typedef uint64_t rstp_identifier;
>>>
>>> #define RSTP_PORT_ID_FMT "%04"PRIx16
>>>
>>> +/* State of an RSTP port.
>>> + *
>>> + * The RSTP_DISABLED state means that the port is disabled by management.
>>> + * In our implementation, this state means that the port does not
>>> + * participate in the spanning tree, but it still forwards traffic as if
>>> + * it were in the RSTP_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 FWD_BPDU
>>> + * --- --- ------- ------- --------
>>> + * Disabled Y - - - Y
>>> + * Discarding - - Y Y Y
>>> + * Learning - Y Y Y Y
>>> + * Forwarding Y Y Y Y Y
>>> + *
>>> + *
>>> + * FWD: the port should forward any incoming non-rstp-BPDU
>>> + * packets.
>>> + *
>>> + * LRN: the port should conduct MAC learning on packets
>>> received.
>>> + *
>>> + * TX_BPDU/RX_BPDU: the port could generate/consume bpdus.
>>> + *
>>> + * FWD_BPDU: the port should should always forward the BPDUS,
>>> + * whether they are generated by the port or received
>>> + * as incoming packets.
>>> + *
>>> + */
>>> +
>>> enum rstp_state {
>>> RSTP_DISABLED,
>>> RSTP_LEARNING,
>>> @@ -128,7 +160,7 @@ const char *rstp_state_name(enum rstp_state);
>>> const char *rstp_port_role_name(enum rstp_port_role);
>>> static inline bool rstp_forward_in_state(enum rstp_state);
>>> static inline bool rstp_learn_in_state(enum rstp_state);
>>> -static inline bool rstp_should_manage_bpdu(enum rstp_state state);
>>> +static inline bool rstp_should_forward_bpdu(enum rstp_state state);
>>>
>>> void rstp_init(void)
>>> OVS_EXCLUDED(rstp_mutex);
>>> @@ -255,12 +287,16 @@ void rstp_port_set_state(struct rstp_port *p, enum
>>> rstp_state state)
>>> /* Inline functions. */
>>> /* Returns true if 'state' is one in which BPDU packets should be received
>>> * and transmitted on a port, false otherwise.
>>> + *
>>> + * Returns true if 'state' is RSTP_DISABLED, since in that case the port
>>> does
>>> + * not generate the bpdu and should just forward it (e.g. patch port on pif
>>> + * bridge).
>>> */
>>> static inline bool
>>> -rstp_should_manage_bpdu(enum rstp_state state)
>>> +rstp_should_forward_bpdu(enum rstp_state state)
>>> {
>>> return (state == RSTP_DISCARDING || state == RSTP_LEARNING ||
>>> - state == RSTP_FORWARDING);
>>> + state == RSTP_FORWARDING || state == RSTP_DISABLED);
>>> }
>>
>> This now returns true for all the possible states, so this should be removed.
>>
>>>
>>> /* Returns true if 'state' is one in which packets received on a port should
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index 786494d..51d25d4 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -1193,9 +1193,9 @@ xport_rstp_forward_state(const struct xport *xport)
>>> }
>>>
>>> static bool
>>> -xport_rstp_should_manage_bpdu(const struct xport *xport)
>>> +xport_rstp_should_forward_bpdu(const struct xport *xport)
>>> {
>>> - return rstp_should_manage_bpdu(xport_get_rstp_port_state(xport));
>>> + return rstp_should_forward_bpdu(xport_get_rstp_port_state(xport));
>>> }
>>
>> Ditto.
>
> You’re right. I left it there to show that the check was done on both
> protocols, but I guess it can be removed since it’s always true.
>
>>
>>> static void
>>> @@ -2493,7 +2493,7 @@ compose_output_action__(struct xlate_ctx *ctx,
>>> ofp_port_t ofp_port,
>>> } else if (check_stp) {
>>> if (is_stp(&ctx->base_flow)) {
>>> if (!xport_stp_should_forward_bpdu(xport) &&
>>> - !xport_rstp_should_manage_bpdu(xport)) {
>>> + !xport_rstp_should_forward_bpdu(xport)) {
>>
>> Maybe the same is true for sport_stp_should_forward_bpdu()? At least the
>> comment in stp.h states that BPDUs should always be forwarded.
>
> Right now STP forwards BPDUs in all states but the initial state STP_BLOCKING.
I don't recall it now offhand, but that state could be transitory and in effect
only during initialization, I.e., never when BPDUs are received.
Jarno
>
>>
>>> if (ctx->xbridge->stp != NULL) {
>>> xlate_report(ctx, "STP not in listening state, "
>>> "skipping bpdu output");
>>> --
>>> 1.8.1.2
>
> Daniele
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
