We didn’t merge this patch back in september. Were you suggesting to remove both stp_should_forward_bpdu() and rstp_should_manage_bpdu() ?
Regards, Daniele > Il giorno 11/set/2014, alle ore 16:11, Jarno Rajahalme > <jrajaha...@nicira.com> ha scritto: > > > > Sent from my iPhone > > On Sep 11, 2014, at 2:08 AM, Daniele Venturino <daniele.ventur...@m3s.it > <mailto:daniele.ventur...@m3s.it>> wrote: > >> >> Il giorno 10/set/2014, alle ore 18:54, Jarno Rajahalme >> <jrajaha...@nicira.com <mailto: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 >>> <mailto:daniele.ventur...@m3s.it>> wrote: >>> >>>> See commit bacdb85ad82f981697245eefb40a3b360cfe379b. >>>> >>>> Signed-off by: Daniele Venturino <daniele.ventur...@m3s.it >>>> <mailto: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
