On Mon, Jun 19, 2017 at 08:04:32PM -0700, nickcooper-zhangtonghao wrote:
> When we use the 'ovs-appctl rstp/show', the root bridge
> of rstp is always 'unknown root port'. We don't expect
> that. The reason is that the committer added the check
> for var 'p'. In the rstp, if a bridge is root bridge,
> there is not root port, and we don't use the root port
> 'p', 'rstp/show' in the same case. If we check only rstp
> root port, the root info will not shown any more.
>
> CC: Ben Pfaff <[email protected]>
> Signed-off-by: nickcooper-zhangtonghao <[email protected]>
This change makes me nervous about dereferencing a null 'p'. I think
that it will not do so, for a root bridge, but it takes some study to
see that.
How about this, instead? It has a small amount of code duplication, but
it is easier to see that it avoids a null dereference.
Thanks,
Ben.
diff --git a/lib/rstp.c b/lib/rstp.c
index 9280b3a23bb0..5d71a437412e 100644
--- a/lib/rstp.c
+++ b/lib/rstp.c
@@ -1600,30 +1600,25 @@ rstp_print_details(struct ds *ds, const struct rstp
*rstp)
{
ds_put_format(ds, "---- %s ----\n", rstp->name);
- bool is_root = rstp_is_root_bridge__(rstp);
- struct rstp_port *p = rstp_get_root_port__(rstp);
- if (!p) {
- ds_put_cstr(ds, "unknown root port\n");
- return;
- }
-
- rstp_identifier bridge_id =
- is_root ? rstp->bridge_identifier : rstp_get_root_id__(rstp);
- uint16_t hello_time =
- is_root ? rstp->bridge_hello_time : p->designated_times.hello_time;
- uint16_t max_age =
- is_root ? rstp->bridge_max_age : p->designated_times.max_age;
- uint16_t forward_delay =
- (is_root
- ? rstp->bridge_forward_delay
- : p->designated_times.forward_delay);
-
ds_put_cstr(ds, "Root ID:\n");
- rstp_bridge_id_details(ds, bridge_id, hello_time, max_age, forward_delay);
- if (is_root) {
+ if (rstp_is_root_bridge__(rstp)) {
+ rstp_bridge_id_details(ds, rstp->bridge_identifier,
+ rstp->bridge_hello_time,
+ rstp->bridge_max_age,
+ rstp->bridge_forward_delay);
ds_put_cstr(ds, "\tThis bridge is the root\n");
} else {
- ds_put_format(ds, "\troot-port\t%s\n", p->port_name);
+ struct rstp_port *root_port = rstp_get_root_port__(rstp);
+ if (!root_port) {
+ ds_put_cstr(ds, "unknown root port\n");
+ return;
+ }
+
+ rstp_bridge_id_details(ds, rstp_get_root_id__(rstp),
+ root_port->designated_times.hello_time,
+ root_port->designated_times.max_age,
+ root_port->designated_times.forward_delay);
+ ds_put_format(ds, "\troot-port\t%s\n", root_port->port_name);
ds_put_format(ds, "\troot-path-cost\t%u\n",
rstp_get_root_path_cost__(rstp));
}
@@ -1639,6 +1634,8 @@ rstp_print_details(struct ds *ds, const struct rstp *rstp)
ds_put_format(ds, "\t%-11.10s%-11.10s%-11.10s%-9.8s%-8.7s\n",
"Interface", "Role", "State", "Cost", "Pri.Nbr");
ds_put_cstr(ds, "\t---------- ---------- ---------- -------- -------\n");
+
+ struct rstp_port *p;
HMAP_FOR_EACH (p, node, &rstp->ports) {
if (p->rstp_state != RSTP_DISABLED) {
ds_put_format(ds, "\t%-11.10s",
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
