[ovs-dev] [PATCH v2] ofproto/ofproto.c: After port_mod + barrier, port config may not be updated

Isaku Yamahata yamahata at valinux.co.jp
Wed Oct 17 23:08:52 UTC 2012


The following OF packets may produce the wrong result as follows.
It depends on how ovs-vswitchd serves OF packets. Sending the OF packets
in single TCP packet will increase the possiblity.

OF request:
  feature request
  port_mod with config LINK DOWN
  barrier
  feature request

The replies:
  feature reply with port config UP
  barrier
  feature reply with port config UP (this should be DOWN because
                                     it's after barrier)
  port status with port config = DOWN and port status = DOWN

The direct cause is updte_port_config() @ ofproto/ofproto.c doesn't update
ofputil_phy_port::config. And later the config member is updated
by update_port() in main loop().
It seems that it tries to produce port_status event due to config change.
But changing other OFPC flags doesn't generate port_status event, so it's
consistent not to generate the event when OFPC_LINK_DOWN flag is changed.

Signed-off-by: Isaku Yamahata <yamahata at valinux.co.jp>
---
Change v1 -> v2:
- generate port_mod event
- added error check
---
 ofproto/ofproto.c |   34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 2fb2fc8..8c0eabc 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2161,28 +2161,43 @@ exit:
     return error;
 }
 
-static void
+static enum ofperr
 update_port_config(struct ofport *port,
                    enum ofputil_port_config config,
-                   enum ofputil_port_config mask)
+                   enum ofputil_port_config mask, bool *generate_port_mod)
 {
     enum ofputil_port_config old_config = port->pp.config;
     enum ofputil_port_config toggle;
+    int error = 0;
 
+    *generate_port_mod = false;
     toggle = (config ^ port->pp.config) & mask;
     if (toggle & OFPUTIL_PC_PORT_DOWN) {
+        *generate_port_mod = true;
         if (config & OFPUTIL_PC_PORT_DOWN) {
-            netdev_turn_flags_off(port->netdev, NETDEV_UP, true);
+            error = netdev_turn_flags_off(port->netdev, NETDEV_UP, true);
         } else {
-            netdev_turn_flags_on(port->netdev, NETDEV_UP, true);
+            error = netdev_turn_flags_on(port->netdev, NETDEV_UP, true);
+        }
+        if (error) {
+            char name[sizeof port->pp.name];
+            ovs_strlcpy(name, port->pp.name, sizeof port->pp.name);
+            VLOG_WARN("can't %s port %s %s",
+                      config & OFPUTIL_PC_PORT_DOWN ? "DOWN" : "UP",
+                      name, strerror(error));
+            toggle &= ~OFPUTIL_PC_PORT_DOWN;
         }
-        toggle &= ~OFPUTIL_PC_PORT_DOWN;
     }
 
     port->pp.config ^= toggle;
     if (port->pp.config != old_config) {
+        *generate_port_mod = true;
         port->ofproto->ofproto_class->port_reconfigured(port, old_config);
     }
+    if (error) {
+        return OFPERR_OFPPMFC_BAD_CONFIG;
+    }
+    return 0;
 }
 
 static enum ofperr
@@ -2209,12 +2224,17 @@ handle_port_mod(struct ofconn *ofconn, const struct ofp_header *oh)
     } else if (!eth_addr_equals(port->pp.hw_addr, pm.hw_addr)) {
         return OFPERR_OFPPMFC_BAD_HW_ADDR;
     } else {
-        update_port_config(port, pm.config, pm.mask);
+        bool generate_port_mod = false;
+        error = update_port_config(port, pm.config, pm.mask,
+                                   &generate_port_mod);
         if (pm.advertise) {
             netdev_set_advertisements(port->netdev, pm.advertise);
         }
+        if (!error && generate_port_mod) {
+            ofport_modified(port, &port->pp);
+        }
     }
-    return 0;
+    return error;
 }
 
 static enum ofperr
-- 
1.7.10.4




More information about the dev mailing list