[ovs-dev,5/6] ofproto: Consistently force off OFPPS_LIVE if port or link is down.

Message ID 20180824173521.19922-5-blp@ovn.org
State New
Headers show
Series
  • [ovs-dev,1/6] connmgr: Suppress duplicate port status notifications.
Related show

Commit Message

Ben Pfaff Aug. 24, 2018, 5:35 p.m.
It doesn't make sense for a port that is down to be "live" from OpenFlow's
point of view, but this could happen in OVS.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ofproto/ofproto-dpif.c     | 26 ++------------------------
 ofproto/ofproto-provider.h |  1 +
 ofproto/ofproto.c          | 43 +++++++++++++++++++++++++++++++++----------
 3 files changed, 36 insertions(+), 34 deletions(-)

Patch

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 868c728c0c14..1faf996b83c8 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2003,16 +2003,6 @@  port_modified(struct ofport *port_)
         bfd_set_netdev(port->bfd, netdev);
     }
 
-    /* Set liveness, unless the link is administratively or
-     * operationally down or link monitoring false */
-    if (!(port->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
-        !(port->up.pp.state & OFPUTIL_PS_LINK_DOWN) &&
-        port->up.may_enable) {
-        port->up.pp.state |= OFPUTIL_PS_LIVE;
-    } else {
-        port->up.pp.state &= ~OFPUTIL_PS_LIVE;
-    }
-
     ofproto_dpif_monitor_port_update(port, port->bfd, port->cfm,
                                      port->lldp, &port->up.pp.hw_addr);
 
@@ -2047,6 +2037,7 @@  port_reconfigured(struct ofport *port_, enum ofputil_port_config old_config)
             bundle_update(port->bundle);
         }
     }
+    port_run(port);
 }
 
 static int
@@ -3609,7 +3600,7 @@  port_run(struct ofport_dpif *ofport)
 
     bool enable = may_enable_port(ofport);
     if (ofport->up.may_enable != enable) {
-        ofport->up.may_enable = enable;
+        ofproto_port_set_enable(&ofport->up, enable);
 
         struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
         ofproto->backer->need_revalidate = REV_PORT_TOGGLED;
@@ -3617,19 +3608,6 @@  port_run(struct ofport_dpif *ofport)
         if (ofport->rstp_port) {
             rstp_port_set_mac_operational(ofport->rstp_port, enable);
         }
-
-        /* Propagate liveness, unless the link is administratively or
-         * operationally down. */
-        if (!(ofport->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
-            !(ofport->up.pp.state & OFPUTIL_PS_LINK_DOWN)) {
-            enum ofputil_port_state of_state = ofport->up.pp.state;
-            if (enable) {
-                of_state |= OFPUTIL_PS_LIVE;
-            } else {
-                of_state &= ~OFPUTIL_PS_LIVE;
-            }
-            ofproto_port_set_state(&ofport->up, of_state);
-        }
     }
 }
 
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 28627cbd3902..4fd8cb14ed40 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -165,6 +165,7 @@  struct ofport {
     bool may_enable;            /* May be live (OFPPS_LIVE) if link is up. */
 };
 
+void ofproto_port_set_enable(struct ofport *, bool enable);
 void ofproto_port_set_state(struct ofport *, enum ofputil_port_state);
 
 /* OpenFlow table flags:
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 94e8b9576b06..9594427a2fa6 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2457,11 +2457,34 @@  ofport_remove_with_name(struct ofproto *ofproto, const char *name)
     }
 }
 
+static enum ofputil_port_state
+normalize_state(enum ofputil_port_config config,
+                enum ofputil_port_state state,
+                bool may_enable)
+{
+    return (config & OFPUTIL_PC_PORT_DOWN
+            || state & OFPUTIL_PS_LINK_DOWN
+            || !may_enable
+            ? state & ~OFPUTIL_PS_LIVE
+            : state | OFPUTIL_PS_LIVE);
+}
+
+void
+ofproto_port_set_enable(struct ofport *port, bool enable)
+{
+    if (enable != port->may_enable) {
+        port->may_enable = enable;
+        ofproto_port_set_state(port, normalize_state(port->pp.config,
+                                                     port->pp.state,
+                                                     port->may_enable));
+    }
+}
 
 /* Update OpenFlow 'state' in 'port' and notify controller. */
 void
 ofproto_port_set_state(struct ofport *port, enum ofputil_port_state state)
 {
+    state = normalize_state(port->pp.config, state, port->may_enable);
     if (port->pp.state != state) {
         struct ofputil_phy_port old_pp = port->pp;
         port->pp.state = state;
@@ -2611,16 +2634,18 @@  update_port(struct ofproto *ofproto, const char *name)
         port = ofproto_get_port(ofproto, ofproto_port.ofp_port);
         if (port && !strcmp(netdev_get_name(port->netdev), name)) {
             struct netdev *old_netdev = port->netdev;
-            struct ofputil_phy_port old_pp = port->pp;
 
             /* ofport_open() only sets OFPUTIL_PC_PORT_DOWN and
-             * OFPUTIL_PS_LINK_DOWN.  Keep the other config and state bits. */
+             * OFPUTIL_PS_LINK_DOWN.  Keep the other config and state bits (but
+             * a port that is down cannot be live). */
             pp.config |= port->pp.config & ~OFPUTIL_PC_PORT_DOWN;
             pp.state |= port->pp.state & ~OFPUTIL_PS_LINK_DOWN;
+            pp.state = normalize_state(pp.config, pp.state, port->may_enable);
 
             /* 'name' hasn't changed location.  Any properties changed? */
-            bool port_changed = !ofport_equal(&port->pp, &pp);
-            if (port_changed) {
+            if (!ofport_equal(&port->pp, &pp)) {
+                connmgr_send_port_status(port->ofproto->connmgr, NULL,
+                                         &port->pp, &pp, OFPPR_MODIFY);
                 port->pp = pp;
             }
 
@@ -2636,12 +2661,6 @@  update_port(struct ofproto *ofproto, const char *name)
                 port->ofproto->ofproto_class->port_modified(port);
             }
 
-            /* Send status update, if any port property changed */
-            if (port_changed) {
-                connmgr_send_port_status(port->ofproto->connmgr, NULL,
-                                         &old_pp, &port->pp, OFPPR_MODIFY);
-            }
-
             netdev_close(old_netdev);
         } else {
             /* If 'port' is nonnull then its name differs from 'name' and thus
@@ -3636,7 +3655,11 @@  update_port_config(struct ofconn *ofconn, struct ofport *port,
 
     if (toggle) {
         struct ofputil_phy_port old_pp = port->pp;
+
         port->pp.config ^= toggle;
+        port->pp.state = normalize_state(port->pp.config, port->pp.state,
+                                         port->may_enable);
+
         port->ofproto->ofproto_class->port_reconfigured(port, old_pp.config);
         connmgr_send_port_status(port->ofproto->connmgr, ofconn, &old_pp,
                                  &port->pp, OFPPR_MODIFY);