ofproto: Consistently force off OFPPS_LIVE if port or link is down.

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 <[email protected]>
Acked-by: Numan Siddique <[email protected]>

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,27 +3600,14 @@ 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;
 
         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);
-        }
     }
 }
 

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:

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);