ofproto: Rework and fix bugs in port change detection.

The OpenFlow port change detection code in update_port() is supposed to
send out an OFPT_PORT_STATUS message whenever an OpenFlow port is added or
removed or changes in some way.  This commit fixes a number of bugs that
have persisted until now.

First, if a port with a given name is removed from the datapath and a new
port with the same name but a different port number is added to the
datapath, then update_port() would report this as a port "modify" change.
Reporting this as a "modify" seems likely to confuse controllers, which
have no reason to realize that the old port was deleted and may not
understand why a port that has not been reported as added would be
modified.  (This scenario is more likely than before, because the Linux
datapath implementation no longer quickly reuses port numbers.  This
problem has actually been reported in testing.)  This commit fixes the
problem by changing update_port() to report a "delete" of the old port
followed by an "add" of the new port.

Second, suppose that a datapath initially has "eth1" on port 1 and "eth2"
on port 2.  Then, "eth1" gets removed and "eth2" is reassigned to port 1.
If update_port() is first passed "eth2", then the old implementation would
have sent out an OpenFlow "modify" notification instead of "delete"
followed by "add", which is the same as the previous scenario.  But as a
further wrinkle, it would have failed to remove "eth1", which meant that we
ended up with two "ofports" with port number 1!  This commit fixes this
problem too.

Reported-by: David Tsai <dtsai@nicira.com>
Bug #5466.
NIC-372.

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index a6ad93396bf017aff7b4da7342e1e50043969ee0..39e2b30e1ddad90e7b108ca864292c6f321393c8 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1061,12 +1061,13 @@ reinit_ports(struct ofproto *p)
     sset_destroy(&devnames);
 }
 
-static struct ofport *
-make_ofport(const struct dpif_port *dpif_port)
+/* Opens and returns a netdev for 'dpif_port', or a null pointer if the netdev
+ * cannot be opened.  On success, also fills in 'opp', in *HOST* byte order. */
+static struct netdev *
+ofport_open(const struct dpif_port *dpif_port, struct ofp_phy_port *opp)
 {
     struct netdev_options netdev_options;
     enum netdev_flags flags;
-    struct ofport *ofport;
     struct netdev *netdev;
     int error;
 
@@ -1084,22 +1085,16 @@ make_ofport(const struct dpif_port *dpif_port)
         return NULL;
     }
 
-    ofport = xzalloc(sizeof *ofport);
-    ofport->netdev = netdev;
-    ofport->odp_port = dpif_port->port_no;
-    ofport->opp.port_no = odp_port_to_ofp_port(dpif_port->port_no);
-    netdev_get_etheraddr(netdev, ofport->opp.hw_addr);
-    ovs_strlcpy(ofport->opp.name, dpif_port->name, sizeof ofport->opp.name);
-
     netdev_get_flags(netdev, &flags);
-    ofport->opp.config = flags & NETDEV_UP ? 0 : OFPPC_PORT_DOWN;
 
-    ofport->opp.state = netdev_get_carrier(netdev) ? 0 : OFPPS_LINK_DOWN;
-
-    netdev_get_features(netdev,
-                        &ofport->opp.curr, &ofport->opp.advertised,
-                        &ofport->opp.supported, &ofport->opp.peer);
-    return ofport;
+    opp->port_no = odp_port_to_ofp_port(dpif_port->port_no);
+    netdev_get_etheraddr(netdev, opp->hw_addr);
+    ovs_strzcpy(opp->name, dpif_port->name, sizeof opp->name);
+    opp->config = flags & NETDEV_UP ? 0 : OFPPC_PORT_DOWN;
+    opp->state = netdev_get_carrier(netdev) ? 0 : OFPPS_LINK_DOWN;
+    netdev_get_features(netdev, &opp->curr, &opp->advertised,
+                        &opp->supported, &opp->peer);
+    return netdev;
 }
 
 static bool
@@ -1118,29 +1113,42 @@ ofport_conflicts(const struct ofproto *p, const struct dpif_port *dpif_port)
     }
 }
 
-static int
-ofport_equal(const struct ofport *a_, const struct ofport *b_)
+/* Returns true if most fields of 'a' and 'b' are equal.  Differences in name,
+ * port number, and 'config' bits other than OFPPC_PORT_DOWN are
+ * disregarded. */
+static bool
+ofport_equal(const struct ofp_phy_port *a, const struct ofp_phy_port *b)
 {
-    const struct ofp_phy_port *a = &a_->opp;
-    const struct ofp_phy_port *b = &b_->opp;
-
     BUILD_ASSERT_DECL(sizeof *a == 48); /* Detect ofp_phy_port changes. */
-    return (a->port_no == b->port_no
-            && !memcmp(a->hw_addr, b->hw_addr, sizeof a->hw_addr)
-            && !strcmp(a->name, b->name)
+    return (!memcmp(a->hw_addr, b->hw_addr, sizeof a->hw_addr)
             && a->state == b->state
-            && a->config == b->config
+            && !((a->config ^ b->config) & OFPPC_PORT_DOWN)
             && a->curr == b->curr
             && a->advertised == b->advertised
             && a->supported == b->supported
             && a->peer == b->peer);
 }
 
+/* Adds an ofport to 'p' initialized based on the given 'netdev' and 'opp'.
+ * The caller must ensure that 'p' does not have a conflicting ofport (that is,
+ * one with the same name or port number). */
 static void
-ofport_install(struct ofproto *p, struct ofport *ofport)
+ofport_install(struct ofproto *p,
+               struct netdev *netdev, const struct ofp_phy_port *opp)
 {
-    const char *netdev_name = netdev_get_name(ofport->netdev);
+    const char *netdev_name = netdev_get_name(netdev);
+    struct ofport *ofport;
+
+    connmgr_send_port_status(p->connmgr, opp, OFPPR_ADD);
 
+    /* Create ofport. */
+    ofport = xmalloc(sizeof *ofport);
+    ofport->netdev = netdev;
+    ofport->opp = *opp;
+    ofport->odp_port = ofp_port_to_odp_port(opp->port_no);
+    ofport->cfm = NULL;
+
+    /* Add port to 'p'. */
     netdev_monitor_add(p->netdev_monitor, ofport->netdev);
     hmap_insert(&p->ports, &ofport->hmap_node, hash_int(ofport->odp_port, 0));
     shash_add(&p->port_by_name, netdev_name, ofport);
@@ -1149,9 +1157,12 @@ ofport_install(struct ofproto *p, struct ofport *ofport)
     }
 }
 
+/* Removes 'ofport' from 'p' and destroys it. */
 static void
 ofport_remove(struct ofproto *p, struct ofport *ofport)
 {
+    connmgr_send_port_status(p->connmgr, &ofport->opp, OFPPR_DELETE);
+
     netdev_monitor_remove(p->netdev_monitor, ofport->netdev);
     hmap_remove(&p->ports, &ofport->hmap_node);
     shash_delete(&p->port_by_name,
@@ -1160,6 +1171,42 @@ ofport_remove(struct ofproto *p, struct ofport *ofport)
     if (p->sflow) {
         ofproto_sflow_del_port(p->sflow, ofport->odp_port);
     }
+
+    ofport_free(ofport);
+}
+
+/* If 'ofproto' contains an ofport named 'name', removes it from 'ofproto' and
+ * destroys it. */
+static void
+ofport_remove_with_name(struct ofproto *ofproto, const char *name)
+{
+    struct ofport *port = shash_find_data(&ofproto->port_by_name, name);
+    if (port) {
+        ofport_remove(ofproto, port);
+    }
+}
+
+/* Updates 'port' within 'ofproto' with the new 'netdev' and 'opp'.
+ *
+ * Does not handle a name or port number change.  The caller must implement
+ * such a change as a delete followed by an add.  */
+static void
+ofport_modified(struct ofproto *ofproto, struct ofport *port,
+                struct netdev *netdev, struct ofp_phy_port *opp)
+{
+    memcpy(port->opp.hw_addr, opp->hw_addr, ETH_ADDR_LEN);
+    port->opp.config = ((port->opp.config & ~OFPPC_PORT_DOWN)
+                        | (opp->config & OFPPC_PORT_DOWN));
+    port->opp.state = opp->state;
+    port->opp.curr = opp->curr;
+    port->opp.advertised = opp->advertised;
+    port->opp.supported = opp->supported;
+    port->opp.peer = opp->peer;
+
+    netdev_close(port->netdev);
+    port->netdev = netdev;
+
+    connmgr_send_port_status(ofproto->connmgr, &port->opp, OFPPR_MODIFY);
 }
 
 static void
@@ -1215,75 +1262,42 @@ get_port(const struct ofproto *ofproto, uint16_t odp_port)
 }
 
 static void
-update_port(struct ofproto *p, const char *devname)
+update_port(struct ofproto *ofproto, const char *name)
 {
     struct dpif_port dpif_port;
-    struct ofport *old_ofport;
-    struct ofport *new_ofport;
-    int error;
+    struct ofp_phy_port opp;
+    struct netdev *netdev;
+    struct ofport *port;
 
     COVERAGE_INC(ofproto_update_port);
 
-    /* Query the datapath for port information. */
-    error = dpif_port_query_by_name(p->dpif, devname, &dpif_port);
-
-    /* Find the old ofport. */
-    old_ofport = shash_find_data(&p->port_by_name, devname);
-    if (!error) {
-        if (!old_ofport) {
-            /* There's no port named 'devname' but there might be a port with
-             * the same port number.  This could happen if a port is deleted
-             * and then a new one added in its place very quickly, or if a port
-             * is renamed.  In the former case we want to send an OFPPR_DELETE
-             * and an OFPPR_ADD, and in the latter case we want to send a
-             * single OFPPR_MODIFY.  We can distinguish the cases by comparing
-             * the old port's ifindex against the new port, or perhaps less
-             * reliably but more portably by comparing the old port's MAC
-             * against the new port's MAC.  However, this code isn't that smart
-             * and always sends an OFPPR_MODIFY (XXX). */
-            old_ofport = get_port(p, dpif_port.port_no);
-        }
-    } else if (error != ENOENT && error != ENODEV) {
-        VLOG_WARN_RL(&rl, "dpif_port_query_by_name returned unexpected error "
-                     "%s", strerror(error));
-        goto exit;
-    }
-
-    /* Create a new ofport. */
-    new_ofport = !error ? make_ofport(&dpif_port) : NULL;
-
-    /* Eliminate a few pathological cases. */
-    if (!old_ofport && !new_ofport) {
-        goto exit;
-    } else if (old_ofport && new_ofport) {
-        /* Most of the 'config' bits are OpenFlow soft state, but
-         * OFPPC_PORT_DOWN is maintained by the kernel.  So transfer the
-         * OpenFlow bits from old_ofport.  (make_ofport() only sets
-         * OFPPC_PORT_DOWN and leaves the other bits 0.)  */
-        new_ofport->opp.config |= old_ofport->opp.config & ~OFPPC_PORT_DOWN;
-
-        if (ofport_equal(old_ofport, new_ofport)) {
-            /* False alarm--no change. */
-            ofport_free(new_ofport);
-            goto exit;
+    /* Fetch 'name''s location and properties from the datapath. */
+    netdev = (!dpif_port_query_by_name(ofproto->dpif, name, &dpif_port)
+              ? ofport_open(&dpif_port, &opp)
+              : NULL);
+    if (netdev) {
+        port = get_port(ofproto, dpif_port.port_no);
+        if (port && !strcmp(netdev_get_name(port->netdev), name)) {
+            /* 'name' hasn't changed location.  Any properties changed? */
+            if (!ofport_equal(&port->opp, &opp)) {
+                ofport_modified(ofproto, port, netdev, &opp);
+            } else {
+                netdev_close(netdev);
+            }
+        } else {
+            /* If 'port' is nonnull then its name differs from 'name' and thus
+             * we should delete it.  If we think there's a port named 'name'
+             * then its port number must be wrong now so delete it too. */
+            if (port) {
+                ofport_remove(ofproto, port);
+            }
+            ofport_remove_with_name(ofproto, name);
+            ofport_install(ofproto, netdev, &opp);
         }
+    } else {
+        /* Any port named 'name' is gone now. */
+        ofport_remove_with_name(ofproto, name);
     }
-
-    /* Now deal with the normal cases. */
-    if (old_ofport) {
-        ofport_remove(p, old_ofport);
-    }
-    if (new_ofport) {
-        ofport_install(p, new_ofport);
-    }
-    connmgr_send_port_status(p->connmgr,
-                             new_ofport ? &new_ofport->opp : &old_ofport->opp,
-                             (!old_ofport ? OFPPR_ADD
-                              : !new_ofport ? OFPPR_DELETE
-                              : OFPPR_MODIFY));
-    ofport_free(old_ofport);
-
-exit:
     dpif_port_destroy(&dpif_port);
 }
 
@@ -1295,9 +1309,12 @@ init_ports(struct ofproto *p)
 
     DPIF_PORT_FOR_EACH (&dpif_port, &dump, p->dpif) {
         if (!ofport_conflicts(p, &dpif_port)) {
-            struct ofport *ofport = make_ofport(&dpif_port);
-            if (ofport) {
-                ofport_install(p, ofport);
+            struct ofp_phy_port opp;
+            struct netdev *netdev;
+
+            netdev = ofport_open(&dpif_port, &opp);
+            if (netdev) {
+                ofport_install(p, netdev, &opp);
             }
         }
     }