From 5d27908667cfa34f7786fdc88e9e09078453637a Mon Sep 17 00:00:00 2001
From: Andrew Evans <aevans@nicira.com>
Date: Thu, 30 Jun 2011 15:15:46 -0700
Subject: [PATCH] bridge: Update controller connection status correctly.

Updates to status-related columns in the Controller table can be lost if there
are multiple bridges with different sets of controllers. This commit fixes this
behavior by first accumulating status for all controllers on all bridges, then
making one pass over all rows in the Controller tables, updating the status of
each.

Bug #6185.
Reported-by: Michael Hu <mhu@nicira.com>
---
 ofproto/connmgr.c | 56 ++++++++++++++++++++++++-----------------------
 vswitchd/bridge.c | 13 ++++++++---
 2 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 745e0a66..7c043a4a 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -373,43 +373,45 @@ connmgr_get_controller_info(struct connmgr *mgr, struct shash *info)
 {
     const struct ofconn *ofconn;
 
-    shash_init(info);
-
     HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) {
         const struct rconn *rconn = ofconn->rconn;
-        time_t now = time_now();
-        time_t last_connection = rconn_get_last_connection(rconn);
-        time_t last_disconnect = rconn_get_last_disconnect(rconn);
-        int last_error = rconn_get_last_error(rconn);
-        struct ofproto_controller_info *cinfo = xmalloc(sizeof *cinfo);
+        const char *target = rconn_get_target(rconn);
 
-        shash_add(info, rconn_get_target(rconn), cinfo);
+        if (!shash_find(info, target)) {
+            struct ofproto_controller_info *cinfo = xmalloc(sizeof *cinfo);
+            time_t now = time_now();
+            time_t last_connection = rconn_get_last_connection(rconn);
+            time_t last_disconnect = rconn_get_last_disconnect(rconn);
+            int last_error = rconn_get_last_error(rconn);
 
-        cinfo->is_connected = rconn_is_connected(rconn);
-        cinfo->role = ofconn->role;
+            shash_add(info, target, cinfo);
 
-        cinfo->pairs.n = 0;
+            cinfo->is_connected = rconn_is_connected(rconn);
+            cinfo->role = ofconn->role;
 
-        if (last_error) {
-            cinfo->pairs.keys[cinfo->pairs.n] = "last_error";
-            cinfo->pairs.values[cinfo->pairs.n++] =
-                xstrdup(ovs_retval_to_string(last_error));
-        }
+            cinfo->pairs.n = 0;
 
-        cinfo->pairs.keys[cinfo->pairs.n] = "state";
-        cinfo->pairs.values[cinfo->pairs.n++] =
-            xstrdup(rconn_get_state(rconn));
+            if (last_error) {
+                cinfo->pairs.keys[cinfo->pairs.n] = "last_error";
+                cinfo->pairs.values[cinfo->pairs.n++]
+                    = xstrdup(ovs_retval_to_string(last_error));
+            }
 
-        if (last_connection != TIME_MIN) {
-            cinfo->pairs.keys[cinfo->pairs.n] = "sec_since_connect";
+            cinfo->pairs.keys[cinfo->pairs.n] = "state";
             cinfo->pairs.values[cinfo->pairs.n++]
-                = xasprintf("%ld", (long int) (now - last_connection));
-        }
+                = xstrdup(rconn_get_state(rconn));
 
-        if (last_disconnect != TIME_MIN) {
-            cinfo->pairs.keys[cinfo->pairs.n] = "sec_since_disconnect";
-            cinfo->pairs.values[cinfo->pairs.n++]
-                = xasprintf("%ld", (long int) (now - last_disconnect));
+            if (last_connection != TIME_MIN) {
+                cinfo->pairs.keys[cinfo->pairs.n] = "sec_since_connect";
+                cinfo->pairs.values[cinfo->pairs.n++]
+                    = xasprintf("%ld", (long int) (now - last_connection));
+            }
+
+            if (last_disconnect != TIME_MIN) {
+                cinfo->pairs.keys[cinfo->pairs.n] = "sec_since_disconnect";
+                cinfo->pairs.values[cinfo->pairs.n++]
+                    = xasprintf("%ld", (long int) (now - last_disconnect));
+            }
         }
     }
 }
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 446f4fc0..449957a9 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1348,13 +1348,20 @@ nx_role_to_str(enum nx_role role)
 }
 
 static void
-bridge_refresh_controller_status(const struct bridge *br)
+refresh_controller_status(void)
 {
+    struct bridge *br;
     struct shash info;
     const struct ovsrec_controller *cfg;
 
-    ofproto_get_ofproto_controller_info(br->ofproto, &info);
+    shash_init(&info);
+
+    /* Accumulate status for controllers on all bridges. */
+    HMAP_FOR_EACH (br, node, &all_bridges) {
+        ofproto_get_ofproto_controller_info(br->ofproto, &info);
+    }
 
+    /* Update each controller in the database with current status. */
     OVSREC_CONTROLLER_FOR_EACH(cfg, idl) {
         struct ofproto_controller_info *cinfo =
             shash_find_data(&info, cfg->target);
@@ -1448,9 +1455,9 @@ bridge_run(void)
                         iface_refresh_status(iface);
                     }
                 }
-                bridge_refresh_controller_status(br);
             }
             refresh_system_stats(cfg);
+            refresh_controller_status();
             ovsdb_idl_txn_commit(txn);
             ovsdb_idl_txn_destroy(txn); /* XXX */
         }
-- 
2.30.2