diff mbox

[ovs-dev] rstp: Show root bridge info.

Message ID 20170713230504.GT29918@ovn.org
State Accepted
Headers show

Commit Message

Ben Pfaff July 13, 2017, 11:05 p.m. UTC 
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 <blp@ovn.org>
> Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech>

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.

Comments

nickcooper-zhangtonghao July 14, 2017, 2:30 a.m. UTC | #1 
> On Jul 14, 2017, at 7:05 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> 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.


Looks good to me. Thanks.

Acked-by: nickcooper-zhangtonghao <nic@opencloud.tech>
Ben Pfaff July 14, 2017, 4:21 a.m. UTC | #2 
On Fri, Jul 14, 2017 at 10:30:52AM +0800, nickcooper-zhangtonghao wrote:
> 
> > On Jul 14, 2017, at 7:05 AM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > 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.
> 
> 
> Looks good to me. Thanks.
> 
> Acked-by: nickcooper-zhangtonghao <nic@opencloud.tech>

Thanks.  I applied this to master.
diff mbox

Patch

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",