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.
>
>> 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
