On Nov 14, 2014, at 9:36 AM, Daniele Venturino <daniele.ventur...@m3s.it> wrote:
> A possible commit message:
> "Admin-port-state is the Administrative Bridge Port state variable defined in
> the 802.1D-2004 standard.
> It can be set to include or exclude a port from the active topology by
> management (see 7.4).
>
IMO we should use the user-visible (configuration database) names in the commit
messages ('rstp-admin-p2p-mac' and 'rstp-admin-port-state’).
> operPointToPointMAC and adminPointToPointMAC are a pair of parameters that
> permit inspection of, and control over, the administrative and operational
> state of the point-to-point status of the MAC entity by the MAC Relay Entity.
> adminPointToPointMAC can be set by management and its value is reflected on
> operPointToPointMAC."
>
I still do not see ‘admin_point_to_point_mac’ being used for anything, and I do
not see operPointToPointMAC anywhere in this patch. Are these added in the
following patches? If so, we should mention that in the commit message.
> The AUTO mode should work as reported in the 802.1D-2004 standard (6.4.3), by
> setting operPointToPointMAC to false if the described procedure is not
> available.
> For now, I would only add this change:
>
> diff --git a/lib/rstp.c b/lib/rstp.c
> index 144f2ba..2a2a60c 100644
> --- a/lib/rstp.c
> +++ b/lib/rstp.c
> @@ -1217,8 +1217,18 @@ static void
> rstp_port_set_admin_point_to_point_mac__(struct rstp_port *port,
> rstp_port_set_oper_point_to_point_mac__(port,
> RSTP_OPER_P2P_MAC_STATE_DISABLED);
> } else if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_AUTO) {
> + /* If adminPointToPointMAC is set to Auto, then the value of
> + * operPointToPointMAC is determined in accordance with the
> + * specific procedures defined for the MAC entity concerned, as
> + * defined in 6.5. If these procedures determine that the MAC
> + * entity is connected to a point-to-point LAN, then
> + * operPointToPointMAC is set TRUE; otherwise it is set FALSE.
> + * In the absence of a specific definition of how to determine
> + * whether the MAC is connected to a point-to-point LAN or not,
> + * the value of operPointToPointMAC shall be FALSE. */
> port->admin_point_to_point_mac = admin_p2p_mac_state;
> - /* FIXME add auto procedure*/
> + rstp_port_set_oper_point_to_point_mac__(port,
> + RSTP_OPER_P2P_MAC_STATE_DISABLED);
> }
> }
> }
>
> but we are going to implement this mechanism soon.
OK, I added the above.
Jarno
>
> Here an extract from the 802.1D-2004 standard for a better explanation:
>
> operPointToPointMAC: This parameter can take two values, as follows:
> a) True. The MAC is connected to a point-to-point LAN; i.e., there is at most
> one other system attached to the LAN.
> b) False. The MAC is connected to a non-point-to-point LAN; i.e., there can
> be more than one other system attached to the LAN.
>
> adminPointToPointMAC: This parameter can take three values, as follows:
> a) ForceTrue. The administrator requires the MAC to be treated as if it is
> connected to a point-to-point LAN, regardless of any indications to the
> contrary that are generated by the MAC entity.
> b) ForceFalse. The administrator requires the MAC to be treated as connected
> to a non-point-to-point LAN, regardless of any indications to the contrary
> that are generated by the MAC entity.
> c) Auto. The administrator requires the point-to-point status of the MAC to
> be determined in accordance with the specific MAC procedures defined in 6.5.
> If adminPointToPointMAC is set to ForceTrue, then operPointToPointMAC shall
> be set True. If adminPointToPointMAC is set to ForceFalse, then
> operPointToPointMAC shall be set False.
> If adminPointToPointMAC is set to Auto, then the value of operPointToPointMAC
> is determined in accordance with the specific procedures defined for the MAC
> entity concerned, as defined in 6.5. If these procedures determine that the
> MAC entity is connected to a point-to-point LAN, then
> operPointToPointMAC is set TRUE; otherwise it is set FALSE. In the absence of
> a specific definition of how to determine whether the MAC is connected to a
> point-to-point LAN or not, the value of
> operPointToPointMAC shall be FALSE.
>
> Regards,
> Daniele
>
> 2014-11-14 0:07 GMT+01:00 Jarno Rajahalme <jrajaha...@nicira.com>:
> This patch needs a proper commit message. The user visible changes are in the
> corresponding configuration settings. Maybe briefly introduce them in the
> commit message?
>
> More comments inline below.
>
> I have a working copy with the white-space changes, so if you provide the
> commit message and the answer to the AUTO question below, I can accept and
> push this patch.
>
> Thanks!
>
> Jarno
>
> On Nov 6, 2014, at 7:31 AM, Daniele Venturino <daniele.ventur...@m3s.it>
> wrote:
>
> > Signed-off-by: Daniele Venturino <daniele.ventur...@m3s.it>
> > ---
> > lib/rstp-common.h | 6 ------
> > lib/rstp.c | 31 ++++++++++++++++++++++++++++++-
> > lib/rstp.h | 9 ++++++++-
> > ofproto/ofproto-dpif.c | 4 +++-
> > ofproto/ofproto.h | 2 ++
> > utilities/ovs-vsctl.8.in | 9 +++++++++
> > vswitchd/bridge.c | 10 ++++++++++
> > 7 files changed, 62 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/rstp-common.h b/lib/rstp-common.h
> > index 587f88c..cd43079 100644
> > --- a/lib/rstp-common.h
> > +++ b/lib/rstp-common.h
> > @@ -240,12 +240,6 @@ struct rstp_bpdu {
> > uint8_t padding[7];
> > });
> >
> > -enum rstp_admin_point_to_point_mac_state {
> > - RSTP_ADMIN_P2P_MAC_FORCE_TRUE,
> > - RSTP_ADMIN_P2P_MAC_FORCE_FALSE,
> > - RSTP_ADMIN_P2P_MAC_FORCE_AUTO
> > -};
> > -
> > enum rstp_info_is {
> > INFO_IS_DISABLED,
> > INFO_IS_RECEIVED,
> > diff --git a/lib/rstp.c b/lib/rstp.c
> > index 0f96749..e3007e2 100644
> > --- a/lib/rstp.c
> > +++ b/lib/rstp.c
> > @@ -110,6 +110,9 @@ static void rstp_port_set_admin_edge__(struct rstp_port
> > *, bool admin_edge)
> > OVS_REQUIRES(rstp_mutex);
> > static void rstp_port_set_auto_edge__(struct rstp_port *, bool auto_edge)
> > OVS_REQUIRES(rstp_mutex);
> > +static void rstp_port_set_admin_point_to_point_mac__(struct rstp_port *,
> > + enum rstp_admin_point_to_point_mac_state admin_p2p_mac_state)
> > + OVS_REQUIRES(rstp_mutex);
> > static void rstp_port_set_mcheck__(struct rstp_port *, bool mcheck)
> > OVS_REQUIRES(rstp_mutex);
> > static void reinitialize_port__(struct rstp_port *p)
> > @@ -922,6 +925,8 @@ rstp_port_set_administrative_bridge_port__(struct
> > rstp_port *p,
> > uint8_t admin_port_state)
> > OVS_REQUIRES(rstp_mutex)
> > {
> > + VLOG_DBG("%s, port %u: set RSTP port admin-port-state to %d",
> > + p->rstp->name, p->port_number, admin_port_state);
>
> Add an empty line here.
>
> > if (admin_port_state == RSTP_ADMIN_BRIDGE_PORT_STATE_DISABLED
> > || admin_port_state == RSTP_ADMIN_BRIDGE_PORT_STATE_ENABLED) {
> >
> > @@ -1120,6 +1125,27 @@ rstp_port_set_auto_edge__(struct rstp_port *port,
> > bool auto_edge)
> > }
> > }
> >
> > +/* Sets the port admin_point_to_point_mac parameter. */
> > +static void rstp_port_set_admin_point_to_point_mac__(struct rstp_port
> > *port,
> > + enum rstp_admin_point_to_point_mac_state admin_p2p_mac_state)
> > + OVS_REQUIRES(rstp_mutex)
> > +{
> > + VLOG_DBG("%s, port %u: set RSTP port admin-point-to-point-mac to %d",
> > + port->rstp->name, port->port_number, admin_p2p_mac_state);
>
> Add an empty line here.
>
> > + if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_FORCE_TRUE) {
> > + port->admin_point_to_point_mac = admin_p2p_mac_state;
>
> Remove extra space after ‘=‘.
>
> > + rstp_port_set_oper_point_to_point_mac__(port,
> > + RSTP_OPER_P2P_MAC_STATE_ENABLED);
> > + } else if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_FORCE_FALSE) {
> > + port->admin_point_to_point_mac = admin_p2p_mac_state;
>
> Remove extra space after ‘=‘.
>
> > + rstp_port_set_oper_point_to_point_mac__(port,
> > + RSTP_OPER_P2P_MAC_STATE_DISABLED);
> > + } else if (admin_p2p_mac_state == RSTP_ADMIN_P2P_MAC_AUTO) {
> > + port->admin_point_to_point_mac = admin_p2p_mac_state;
> > + /* FIXME fix auto behaviour */
>
> Elaborate on this. How the AUTO mode should work? Why not implement it now?
>
> > + }
> > +}
> > +
> > /* Sets the port mcheck parameter.
> > * [17.19.13] May be set by management to force the Port Protocol Migration
> > * state machine to transmit RST BPDUs for a MigrateTime (17.13.9) period,
> > to
> > @@ -1289,7 +1315,8 @@ rstp_port_get_status(const struct rstp_port *p,
> > uint16_t *id,
> > void
> > rstp_port_set(struct rstp_port *port, uint16_t port_num, int priority,
> > uint32_t path_cost, bool is_admin_edge, bool is_auto_edge,
> > - bool do_mcheck, void *aux)
> > + enum rstp_admin_point_to_point_mac_state admin_p2p_mac_state,
> > + bool admin_port_state, bool do_mcheck, void *aux)
> > OVS_EXCLUDED(rstp_mutex)
> > {
> > ovs_mutex_lock(&rstp_mutex);
> > @@ -1299,6 +1326,8 @@ rstp_port_set(struct rstp_port *port, uint16_t
> > port_num, int priority,
> > rstp_port_set_path_cost__(port, path_cost);
> > rstp_port_set_admin_edge__(port, is_admin_edge);
> > rstp_port_set_auto_edge__(port, is_auto_edge);
> > + rstp_port_set_admin_point_to_point_mac__(port, admin_p2p_mac_state);
> > + rstp_port_set_administrative_bridge_port__(port, admin_port_state);
> > rstp_port_set_mcheck__(port, do_mcheck);
> > ovs_mutex_unlock(&rstp_mutex);
> > }
> > diff --git a/lib/rstp.h b/lib/rstp.h
> > index ccf8292..458aecf 100644
> > --- a/lib/rstp.h
> > +++ b/lib/rstp.h
> > @@ -120,6 +120,12 @@ enum rstp_port_role {
> > ROLE_DISABLED
> > };
> >
> > +enum rstp_admin_point_to_point_mac_state {
> > + RSTP_ADMIN_P2P_MAC_FORCE_FALSE,
> > + RSTP_ADMIN_P2P_MAC_FORCE_TRUE,
> > + RSTP_ADMIN_P2P_MAC_AUTO
> > +};
> > +
> > struct rstp;
> > struct rstp_port;
> > struct ofproto_rstp_settings;
> > @@ -211,7 +217,8 @@ uint32_t rstp_convert_speed_to_cost(unsigned int speed);
> >
> > void rstp_port_set(struct rstp_port *, uint16_t port_num, int priority,
> > uint32_t path_cost, bool is_admin_edge, bool
> > is_auto_edge,
> > - bool do_mcheck, void *aux)
> > + enum rstp_admin_point_to_point_mac_state
> > admin_p2p_mac_state,
> > + bool admin_port_state, bool do_mcheck, void *aux)
> > OVS_EXCLUDED(rstp_mutex);
> >
> > enum rstp_state rstp_port_get_state(const struct rstp_port *)
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 2302073..95298f2 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -2402,7 +2402,9 @@ set_rstp_port(struct ofport *ofport_,
> > }
> >
> > rstp_port_set(rp, s->port_num, s->priority, s->path_cost,
> > - s->admin_edge_port, s->auto_edge, s->mcheck, ofport);
> > + s->admin_edge_port, s->auto_edge,
> > + s->admin_p2p_mac_state, s->admin_port_state, s->mcheck,
> > + ofport);
> > update_rstp_port_state(ofport);
> > /* Synchronize operational status. */
> > rstp_port_set_mac_operational(rp, ofport->may_enable);
> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > index 989747d..bb28376 100644
> > --- a/ofproto/ofproto.h
> > +++ b/ofproto/ofproto.h
> > @@ -126,6 +126,8 @@ struct ofproto_port_rstp_settings {
> > bool admin_edge_port;
> > bool auto_edge;
> > bool mcheck;
> > + uint8_t admin_p2p_mac_state;
> > + bool admin_port_state;
> > };
> >
> > struct ofproto_stp_settings {
> > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > index 8cf13ae..2902a27 100644
> > --- a/utilities/ovs-vsctl.8.in
> > +++ b/utilities/ovs-vsctl.8.in
> > @@ -1073,6 +1073,15 @@ Set the auto edge value of port \fBeth0\fR:
> > .IP
> > .B "ovs\-vsctl set Port eth0 other_config:rstp-port-auto-edge=true"
> > .PP
> > +Set the admin point to point mac value of port \fBeth0\fR.
>
> Remove extra space after ‘value‘.
>
> > +Acceptable values are 0 (force false), 1 (force true) or 2 (auto).
> > +.IP
> > +.B "ovs\-vsctl set Port eth0 other_config:rstp-admin-p2p-mac=1"
> > +.PP
> > +Set the admin port state value of port \fBeth0\fR.
> > +.IP
> > +.B "ovs\-vsctl set Port eth0 other_config:rstp-admin-port-state=true"
> > +.PP
> > Set the mcheck value of port \fBeth0\fR:
> > .IP
> > .B "ovs\-vsctl set Port eth0 other_config:rstp-port-mcheck=true"
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 20cfcba..a5fb361 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -1398,6 +1398,16 @@ port_configure_rstp(const struct ofproto *ofproto,
> > struct port *port,
> > port_s->priority = RSTP_DEFAULT_PORT_PRIORITY;
> > }
> >
> > + config_str = smap_get(&port->cfg->other_config, "rstp-admin-p2p-mac");
> > + if (config_str) {
> > + port_s->admin_p2p_mac_state = strtoul(config_str, NULL, 0);
> > + } else {
> > + port_s->admin_p2p_mac_state = RSTP_ADMIN_P2P_MAC_FORCE_TRUE;
>
> Remove extra space after ‘=‘.
>
> > + }
> > +
> > + port_s->admin_port_state = smap_get_bool(&port->cfg->other_config,
> > + "rstp-admin-port-state",
> > true);
> > +
> > port_s->admin_edge_port = smap_get_bool(&port->cfg->other_config,
> > "rstp-port-admin-edge", false);
> > port_s->auto_edge = smap_get_bool(&port->cfg->other_config,
> > --
> > 1.8.1.2
> >
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
