port-array: Add port_array_delete() function.
[openvswitch] / vswitchd / bridge.c
index 31e6cb073452f1efca9408b85836d4aa308f6123..f6079928f4a7502a6ab2e62b9ad603bb3671834a 100644 (file)
@@ -20,6 +20,7 @@
 #include <arpa/inet.h>
 #include <ctype.h>
 #include <inttypes.h>
+#include <sys/socket.h>
 #include <net/if.h>
 #include <openflow/openflow.h>
 #include <signal.h>
@@ -122,7 +123,8 @@ struct port {
     struct bridge *bridge;
     size_t port_idx;
     int vlan;                   /* -1=trunk port, else a 12-bit VLAN ID. */
-    unsigned long *trunks;      /* Bitmap of trunked VLANs, if 'vlan' == -1. */
+    unsigned long *trunks;      /* Bitmap of trunked VLANs, if 'vlan' == -1.
+                                 * NULL if all VLANs are trunked. */
     char *name;
 
     /* An ordinary bridge port has 1 interface.
@@ -156,19 +158,11 @@ struct bridge {
     struct list node;           /* Node in global list of bridges. */
     char *name;                 /* User-specified arbitrary name. */
     struct mac_learning *ml;    /* MAC learning table. */
-    bool sent_config_request;   /* Successfully sent config request? */
     uint8_t default_ea[ETH_ADDR_LEN]; /* Default MAC. */
 
     /* OpenFlow switch processing. */
     struct ofproto *ofproto;    /* OpenFlow switch. */
 
-    /* Description strings. */
-    char *mfr_desc;             /* Manufacturer. */
-    char *hw_desc;              /* Hardware. */
-    char *sw_desc;              /* Software version. */
-    char *serial_desc;          /* Serial number. */
-    char *dp_desc;              /* Datapath description. */
-
     /* Kernel datapath information. */
     struct dpif *dpif;          /* Datapath. */
     struct port_array ifaces;   /* Indexed by kernel datapath port number. */
@@ -176,6 +170,8 @@ struct bridge {
     /* Bridge ports. */
     struct port **ports;
     size_t n_ports, allocated_ports;
+    struct shash iface_by_name; /* "struct iface"s indexed by name. */
+    struct shash port_by_name;  /* "struct port"s indexed by name. */
 
     /* Bonding. */
     bool has_bonded_ports;
@@ -183,9 +179,6 @@ struct bridge {
     /* Flow tracking. */
     bool flush;
 
-    /* Flow statistics gathering. */
-    time_t next_stats_request;
-
     /* Port mirroring. */
     struct mirror *mirrors[MAX_MIRRORS];
 
@@ -209,10 +202,10 @@ static size_t bridge_get_controllers(const struct ovsrec_open_vswitch *ovs_cfg,
                                      struct ovsrec_controller ***controllersp);
 static void bridge_reconfigure_one(const struct ovsrec_open_vswitch *,
                                    struct bridge *);
-static void bridge_reconfigure_controller(const struct ovsrec_open_vswitch *,
-                                          struct bridge *,
-                                          const struct sockaddr_in *managers,
-                                          size_t n_managers);
+static void bridge_reconfigure_remotes(const struct ovsrec_open_vswitch *,
+                                       struct bridge *,
+                                       const struct sockaddr_in *managers,
+                                       size_t n_managers);
 static void bridge_get_all_ifaces(const struct bridge *, struct shash *ifaces);
 static void bridge_fetch_dp_ifaces(struct bridge *);
 static void bridge_flush(struct bridge *);
@@ -236,6 +229,7 @@ static void bond_enable_slave(struct iface *iface, bool enable);
 
 static struct port *port_create(struct bridge *, const char *name);
 static void port_reconfigure(struct port *, const struct ovsrec_port *);
+static void port_del_ifaces(struct port *, const struct ovsrec_port *);
 static void port_destroy(struct port *);
 static struct port *port_lookup(const struct bridge *, const char *name);
 static struct iface *port_lookup_iface(const struct port *, const char *name);
@@ -391,10 +385,6 @@ set_up_iface(const struct ovsrec_interface *iface_cfg, struct iface *iface,
         }
         netdev_options.args = &options;
         netdev_options.ethertype = NETDEV_ETH_TYPE_NONE;
-        netdev_options.may_create = true;
-        if (iface_is_internal(iface->port->bridge, iface_cfg->name)) {
-            netdev_options.may_open = true;
-        }
 
         error = netdev_open(&netdev_options, &iface->netdev);
 
@@ -729,7 +719,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
         dpid = bridge_pick_datapath_id(br, ea, hw_addr_iface);
         ofproto_set_datapath_id(br->ofproto, dpid);
 
-        dpid_string = xasprintf("%012"PRIx64, dpid);
+        dpid_string = xasprintf("%016"PRIx64, dpid);
         ovsrec_bridge_set_datapath_id(br->cfg, dpid_string);
         free(dpid_string);
 
@@ -823,7 +813,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
             }
             ofproto_set_sflow(br->ofproto, &oso);
 
-            svec_destroy(&oso.targets);
+            /* Do not destroy oso.targets because it is owned by sflow_cfg. */
         } else {
             ofproto_set_sflow(br->ofproto, NULL);
         }
@@ -837,7 +827,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
          * yet; when a controller is configured, resetting the datapath ID will
          * immediately disconnect from the controller, so it's better to set
          * the datapath ID before the controller. */
-        bridge_reconfigure_controller(ovs_cfg, br, managers, n_managers);
+        bridge_reconfigure_remotes(ovs_cfg, br, managers, n_managers);
     }
     LIST_FOR_EACH (br, struct bridge, node, &all_bridges) {
         for (i = 0; i < br->n_ports; i++) {
@@ -1201,11 +1191,13 @@ bridge_create(const struct ovsrec_bridge *br_cfg)
     br->name = xstrdup(br_cfg->name);
     br->cfg = br_cfg;
     br->ml = mac_learning_create();
-    br->sent_config_request = false;
     eth_addr_nicira_random(br->default_ea);
 
     port_array_init(&br->ifaces);
 
+    shash_init(&br->port_by_name);
+    shash_init(&br->iface_by_name);
+
     br->flush = false;
 
     list_push_back(&all_bridges, &br->node);
@@ -1234,6 +1226,8 @@ bridge_destroy(struct bridge *br)
         ofproto_destroy(br->ofproto);
         mac_learning_destroy(br->ml);
         port_array_destroy(&br->ifaces);
+        shash_destroy(&br->port_by_name);
+        shash_destroy(&br->iface_by_name);
         free(br->ports);
         free(br->name);
         free(br);
@@ -1334,97 +1328,11 @@ bridge_get_controllers(const struct ovsrec_open_vswitch *ovs_cfg,
     return n_controllers;
 }
 
-static bool
-check_duplicate_ifaces(struct bridge *br, struct iface *iface, void *ifaces_)
-{
-    struct svec *ifaces = ifaces_;
-    if (!svec_contains(ifaces, iface->name)) {
-        svec_add(ifaces, iface->name);
-        svec_sort(ifaces);
-        return true;
-    } else {
-        VLOG_ERR("bridge %s: %s interface is on multiple ports, "
-                 "removing from %s",
-                 br->name, iface->name, iface->port->name);
-        return false;
-    }
-}
-
-static void
-bridge_update_desc(struct bridge *br OVS_UNUSED)
-{
-#if 0
-    bool changed = false;
-    const char *desc;
-
-    desc = cfg_get_string(0, "bridge.%s.mfr-desc", br->name);
-    if (desc != br->mfr_desc) {
-        free(br->mfr_desc);
-        if (desc) {
-            br->mfr_desc = xstrdup(desc);
-        } else {
-            br->mfr_desc = xstrdup(DEFAULT_MFR_DESC);
-        }
-        changed = true;
-    }
-
-    desc = cfg_get_string(0, "bridge.%s.hw-desc", br->name);
-    if (desc != br->hw_desc) {
-        free(br->hw_desc);
-        if (desc) {
-            br->hw_desc = xstrdup(desc);
-        } else {
-            br->hw_desc = xstrdup(DEFAULT_HW_DESC);
-        }
-        changed = true;
-    }
-
-    desc = cfg_get_string(0, "bridge.%s.sw-desc", br->name);
-    if (desc != br->sw_desc) {
-        free(br->sw_desc);
-        if (desc) {
-            br->sw_desc = xstrdup(desc);
-        } else {
-            br->sw_desc = xstrdup(DEFAULT_SW_DESC);
-        }
-        changed = true;
-    }
-
-    desc = cfg_get_string(0, "bridge.%s.serial-desc", br->name);
-    if (desc != br->serial_desc) {
-        free(br->serial_desc);
-        if (desc) {
-            br->serial_desc = xstrdup(desc);
-        } else {
-            br->serial_desc = xstrdup(DEFAULT_SERIAL_DESC);
-        }
-        changed = true;
-    }
-
-    desc = cfg_get_string(0, "bridge.%s.dp-desc", br->name);
-    if (desc != br->dp_desc) {
-        free(br->dp_desc);
-        if (desc) {
-            br->dp_desc = xstrdup(desc);
-        } else {
-            br->dp_desc = xstrdup(DEFAULT_DP_DESC);
-        }
-        changed = true;
-    }
-
-    if (changed) {
-        ofproto_set_desc(br->ofproto, br->mfr_desc, br->hw_desc,
-                br->sw_desc, br->serial_desc, br->dp_desc);
-    }
-#endif
-}
-
 static void
 bridge_reconfigure_one(const struct ovsrec_open_vswitch *ovs_cfg,
                        struct bridge *br)
 {
     struct shash old_ports, new_ports;
-    struct svec ifaces;
     struct svec listeners, old_listeners;
     struct svec snoops, old_snoops;
     struct shash_node *node;
@@ -1463,75 +1371,44 @@ bridge_reconfigure_one(const struct ovsrec_open_vswitch *ovs_cfg,
         }
     }
 
-    /* Get rid of deleted ports and add new ports. */
+    /* Get rid of deleted ports.
+     * Get rid of deleted interfaces on ports that still exist. */
     SHASH_FOR_EACH (node, &old_ports) {
-        if (!shash_find(&new_ports, node->name)) {
-            port_destroy(node->data);
+        struct port *port = node->data;
+        const struct ovsrec_port *port_cfg;
+
+        port_cfg = shash_find_data(&new_ports, node->name);
+        if (!port_cfg) {
+            port_destroy(port);
+        } else {
+            port_del_ifaces(port, port_cfg);
         }
     }
+
+    /* Create new ports.
+     * Add new interfaces to existing ports.
+     * Reconfigure existing ports. */
     SHASH_FOR_EACH (node, &new_ports) {
         struct port *port = shash_find_data(&old_ports, node->name);
         if (!port) {
             port = port_create(br, node->name);
         }
+
         port_reconfigure(port, node->data);
+        if (!port->n_ifaces) {
+            VLOG_WARN("bridge %s: port %s has no interfaces, dropping",
+                      br->name, port->name);
+            port_destroy(port);
+        }
     }
     shash_destroy(&old_ports);
     shash_destroy(&new_ports);
 
-    /* Check and delete duplicate interfaces. */
-    svec_init(&ifaces);
-    iterate_and_prune_ifaces(br, check_duplicate_ifaces, &ifaces);
-    svec_destroy(&ifaces);
-
     /* Delete all flows if we're switching from connected to standalone or vice
      * versa.  (XXX Should we delete all flows if we are switching from one
      * controller to another?) */
 
-#if 0
-    /* Configure OpenFlow management listeners. */
-    svec_init(&listeners);
-    cfg_get_all_strings(&listeners, "bridge.%s.openflow.listeners", br->name);
-    if (!listeners.n) {
-        svec_add_nocopy(&listeners, xasprintf("punix:%s/%s.mgmt",
-                                              ovs_rundir, br->name));
-    } else if (listeners.n == 1 && !strcmp(listeners.names[0], "none")) {
-        svec_clear(&listeners);
-    }
-    svec_sort_unique(&listeners);
-
-    svec_init(&old_listeners);
-    ofproto_get_listeners(br->ofproto, &old_listeners);
-    svec_sort_unique(&old_listeners);
-
-    if (!svec_equal(&listeners, &old_listeners)) {
-        ofproto_set_listeners(br->ofproto, &listeners);
-    }
-    svec_destroy(&listeners);
-    svec_destroy(&old_listeners);
-
-    /* Configure OpenFlow controller connection snooping. */
-    svec_init(&snoops);
-    cfg_get_all_strings(&snoops, "bridge.%s.openflow.snoops", br->name);
-    if (!snoops.n) {
-        svec_add_nocopy(&snoops, xasprintf("punix:%s/%s.snoop",
-                                           ovs_rundir, br->name));
-    } else if (snoops.n == 1 && !strcmp(snoops.names[0], "none")) {
-        svec_clear(&snoops);
-    }
-    svec_sort_unique(&snoops);
-
-    svec_init(&old_snoops);
-    ofproto_get_snoops(br->ofproto, &old_snoops);
-    svec_sort_unique(&old_snoops);
-
-    if (!svec_equal(&snoops, &old_snoops)) {
-        ofproto_set_snoops(br->ofproto, &snoops);
-    }
-    svec_destroy(&snoops);
-    svec_destroy(&old_snoops);
-#else
-    /* Default listener. */
+    /* Configure OpenFlow management listener. */
     svec_init(&listeners);
     svec_add_nocopy(&listeners, xasprintf("punix:%s/%s.mgmt",
                                           ovs_rundir, br->name));
@@ -1543,7 +1420,7 @@ bridge_reconfigure_one(const struct ovsrec_open_vswitch *ovs_cfg,
     svec_destroy(&listeners);
     svec_destroy(&old_listeners);
 
-    /* Default snoop. */
+    /* Configure OpenFlow controller connection snooping. */
     svec_init(&snoops);
     svec_add_nocopy(&snoops, xasprintf("punix:%s/%s.snoop",
                                        ovs_rundir, br->name));
@@ -1554,18 +1431,15 @@ bridge_reconfigure_one(const struct ovsrec_open_vswitch *ovs_cfg,
     }
     svec_destroy(&snoops);
     svec_destroy(&old_snoops);
-#endif
 
     mirror_reconfigure(br);
-
-    bridge_update_desc(br);
 }
 
 static void
-bridge_reconfigure_controller(const struct ovsrec_open_vswitch *ovs_cfg,
-                              struct bridge *br,
-                              const struct sockaddr_in *managers,
-                              size_t n_managers)
+bridge_reconfigure_remotes(const struct ovsrec_open_vswitch *ovs_cfg,
+                           struct bridge *br,
+                           const struct sockaddr_in *managers,
+                           size_t n_managers)
 {
     struct ovsrec_controller **controllers;
     size_t n_controllers;
@@ -1926,10 +1800,19 @@ bond_update_fake_iface_stats(struct port *port)
         struct netdev_stats slave_stats;
 
         if (!netdev_get_stats(port->ifaces[i]->netdev, &slave_stats)) {
-            bond_stats.rx_packets += slave_stats.rx_packets;
-            bond_stats.rx_bytes += slave_stats.rx_bytes;
-            bond_stats.tx_packets += slave_stats.tx_packets;
-            bond_stats.tx_bytes += slave_stats.tx_bytes;
+            /* XXX: We swap the stats here because they are swapped back when
+             * reported by the internal device.  The reason for this is
+             * internal devices normally represent packets going into the system
+             * but when used as fake bond device they represent packets leaving
+             * the system.  We really should do this in the internal device
+             * itself because changing it here reverses the counts from the
+             * perspective of the switch.  However, the internal device doesn't
+             * know what type of device it represents so we have to do it here
+             * for now. */
+            bond_stats.tx_packets += slave_stats.rx_packets;
+            bond_stats.tx_bytes += slave_stats.rx_bytes;
+            bond_stats.rx_packets += slave_stats.tx_packets;
+            bond_stats.rx_bytes += slave_stats.tx_bytes;
         }
     }
 
@@ -1982,11 +1865,11 @@ bond_wait(struct bridge *br)
         for (j = 0; j < port->n_ifaces; j++) {
             struct iface *iface = port->ifaces[j];
             if (iface->delay_expires != LLONG_MAX) {
-                poll_timer_wait(iface->delay_expires - time_msec());
+                poll_timer_wait_until(iface->delay_expires);
             }
         }
         if (port->bond_fake_iface) {
-            poll_timer_wait(port->bond_next_fake_iface_update - time_msec());
+            poll_timer_wait_until(port->bond_next_fake_iface_update);
         }
     }
 }
@@ -2070,7 +1953,8 @@ dst_is_duplicate(const struct dst *dsts, size_t n_dsts,
 static bool
 port_trunks_vlan(const struct port *port, uint16_t vlan)
 {
-    return port->vlan < 0 && bitmap_is_set(port->trunks, vlan);
+    return (port->vlan < 0
+            && (!port->trunks || bitmap_is_set(port->trunks, vlan)));
 }
 
 static bool
@@ -2244,12 +2128,34 @@ static int flow_get_vlan(struct bridge *br, const flow_t *flow,
     return vlan;
 }
 
+/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
+ * migration.  Older Citrix-patched Linux DomU used gratuitous ARP replies to
+ * indicate this; newer upstream kernels use gratuitous ARP requests. */
+static bool
+is_gratuitous_arp(const flow_t *flow)
+{
+    return (flow->dl_type == htons(ETH_TYPE_ARP)
+            && eth_addr_is_broadcast(flow->dl_dst)
+            && (flow->nw_proto == ARP_OP_REPLY
+                || (flow->nw_proto == ARP_OP_REQUEST
+                    && flow->nw_src == flow->nw_dst)));
+}
+
 static void
 update_learning_table(struct bridge *br, const flow_t *flow, int vlan,
                       struct port *in_port)
 {
-    tag_type rev_tag = mac_learning_learn(br->ml, flow->dl_src,
-                                          vlan, in_port->port_idx);
+    enum grat_arp_lock_type lock_type;
+    tag_type rev_tag;
+
+    /* We don't want to learn from gratuitous ARP packets that are reflected
+     * back over bond slaves so we lock the learning table. */
+    lock_type = !is_gratuitous_arp(flow) ? GRAT_ARP_LOCK_NONE :
+                    (in_port->n_ifaces == 1) ? GRAT_ARP_LOCK_SET :
+                                               GRAT_ARP_LOCK_CHECK;
+
+    rev_tag = mac_learning_learn(br->ml, flow->dl_src, vlan, in_port->port_idx,
+                                 lock_type);
     if (rev_tag) {
         /* The log messages here could actually be useful in debugging,
          * so keep the rate limit relatively high. */
@@ -2263,14 +2169,6 @@ update_learning_table(struct bridge *br, const flow_t *flow, int vlan,
     }
 }
 
-static bool
-is_bcast_arp_reply(const flow_t *flow)
-{
-    return (flow->dl_type == htons(ETH_TYPE_ARP)
-            && flow->nw_proto == ARP_OP_REPLY
-            && eth_addr_is_broadcast(flow->dl_dst));
-}
-
 /* Determines whether packets in 'flow' within 'br' should be forwarded or
  * dropped.  Returns true if they may be forwarded, false if they should be
  * dropped.
@@ -2349,6 +2247,7 @@ is_admissible(struct bridge *br, const flow_t *flow, bool have_packet,
     /* Packets received on bonds need special attention to avoid duplicates. */
     if (in_port->n_ifaces > 1) {
         int src_idx;
+        bool is_grat_arp_locked;
 
         if (eth_addr_is_multicast(flow->dl_dst)) {
             *tags |= in_port->active_iface_tag;
@@ -2360,11 +2259,15 @@ is_admissible(struct bridge *br, const flow_t *flow, bool have_packet,
 
         /* Drop all packets for which we have learned a different input
          * port, because we probably sent the packet on one slave and got
-         * it back on the other.  Broadcast ARP replies are an exception
-         * to this rule: the host has moved to another switch. */
-        src_idx = mac_learning_lookup(br->ml, flow->dl_src, vlan);
+         * it back on the other.  Gratuitous ARP packets are an exception
+         * to this rule: the host has moved to another switch.  The exception
+         * to the exception is if we locked the learning table to avoid
+         * reflections on bond slaves.  If this is the case, just drop the
+         * packet now. */
+        src_idx = mac_learning_lookup(br->ml, flow->dl_src, vlan,
+                                      &is_grat_arp_locked);
         if (src_idx != -1 && src_idx != in_port->port_idx &&
-            !is_bcast_arp_reply(flow)) {
+            (!is_gratuitous_arp(flow) || is_grat_arp_locked)) {
                 return false;
         }
     }
@@ -2397,7 +2300,8 @@ process_flow(struct bridge *br, const flow_t *flow,
     }
 
     /* Determine output port. */
-    out_port_idx = mac_learning_lookup_tag(br->ml, flow->dl_dst, vlan, tags);
+    out_port_idx = mac_learning_lookup_tag(br->ml, flow->dl_dst, vlan, tags,
+                                           NULL);
     if (out_port_idx >= 0 && out_port_idx < br->n_ports) {
         out_port = br->ports[out_port_idx];
     } else if (!packet && !eth_addr_is_multicast(flow->dl_dst)) {
@@ -3226,6 +3130,7 @@ port_create(struct bridge *br, const char *name)
                                sizeof *br->ports);
     }
     br->ports[br->n_ports++] = port;
+    shash_add_assert(&br->port_by_name, port->name, port);
 
     VLOG_INFO("created port %s on bridge %s", port->name, br->name);
     bridge_flush(br);
@@ -3244,31 +3149,43 @@ get_port_other_config(const struct ovsrec_port *port, const char *key,
     return value ? value : default_value;
 }
 
+static void
+port_del_ifaces(struct port *port, const struct ovsrec_port *cfg)
+{
+    struct shash new_ifaces;
+    size_t i;
+
+    /* Collect list of new interfaces. */
+    shash_init(&new_ifaces);
+    for (i = 0; i < cfg->n_interfaces; i++) {
+        const char *name = cfg->interfaces[i]->name;
+        shash_add_once(&new_ifaces, name, NULL);
+    }
+
+    /* Get rid of deleted interfaces. */
+    for (i = 0; i < port->n_ifaces; ) {
+        if (!shash_find(&new_ifaces, cfg->interfaces[i]->name)) {
+            iface_destroy(port->ifaces[i]);
+        } else {
+            i++;
+        }
+    }
+
+    shash_destroy(&new_ifaces);
+}
+
 static void
 port_reconfigure(struct port *port, const struct ovsrec_port *cfg)
 {
-    struct shash old_ifaces, new_ifaces;
+    struct shash new_ifaces;
     long long int next_rebalance;
-    struct shash_node *node;
     unsigned long *trunks;
     int vlan;
     size_t i;
 
     port->cfg = cfg;
 
-    /* Collect old and new interfaces. */
-    shash_init(&old_ifaces);
-    shash_init(&new_ifaces);
-    for (i = 0; i < port->n_ifaces; i++) {
-        shash_add(&old_ifaces, port->ifaces[i]->name, port->ifaces[i]);
-    }
-    for (i = 0; i < cfg->n_interfaces; i++) {
-        const char *name = cfg->interfaces[i]->name;
-        if (!shash_add_once(&new_ifaces, name, cfg->interfaces[i])) {
-            VLOG_WARN("port %s: %s specified twice as port interface",
-                      port->name, name);
-        }
-    }
+    /* Update settings. */
     port->updelay = cfg->bond_updelay;
     if (port->updelay < 0) {
         port->updelay = 0;
@@ -3287,23 +3204,32 @@ port_reconfigure(struct port *port, const struct ovsrec_port *cfg)
         port->bond_next_rebalance = next_rebalance;
     }
 
-    /* Get rid of deleted interfaces and add new interfaces. */
-    SHASH_FOR_EACH (node, &old_ifaces) {
-        if (!shash_find(&new_ifaces, node->name)) {
-            iface_destroy(node->data);
-        }
-    }
-    SHASH_FOR_EACH (node, &new_ifaces) {
-        const struct ovsrec_interface *if_cfg = node->data;
+    /* Add new interfaces and update 'cfg' member of existing ones. */
+    shash_init(&new_ifaces);
+    for (i = 0; i < cfg->n_interfaces; i++) {
+        const struct ovsrec_interface *if_cfg = cfg->interfaces[i];
         struct iface *iface;
 
-        iface = shash_find_data(&old_ifaces, if_cfg->name);
-        if (!iface) {
-            iface_create(port, if_cfg);
-        } else {
+        if (!shash_add_once(&new_ifaces, if_cfg->name, NULL)) {
+            VLOG_WARN("port %s: %s specified twice as port interface",
+                      port->name, if_cfg->name);
+            continue;
+        }
+
+        iface = iface_lookup(port->bridge, if_cfg->name);
+        if (iface) {
+            if (iface->port != port) {
+                VLOG_ERR("bridge %s: %s interface is on multiple ports, "
+                         "removing from %s",
+                         port->bridge->name, if_cfg->name, iface->port->name);
+                continue;
+            }
             iface->cfg = if_cfg;
+        } else {
+            iface_create(port, if_cfg);
         }
     }
+    shash_destroy(&new_ifaces);
 
     /* Get VLAN tag. */
     vlan = -1;
@@ -3329,7 +3255,7 @@ port_reconfigure(struct port *port, const struct ovsrec_port *cfg)
 
     /* Get trunked VLANs. */
     trunks = NULL;
-    if (vlan < 0) {
+    if (vlan < 0 && cfg->n_trunks) {
         size_t n_errors;
         size_t i;
 
@@ -3348,17 +3274,14 @@ port_reconfigure(struct port *port, const struct ovsrec_port *cfg)
                      port->name, cfg->n_trunks);
         }
         if (n_errors == cfg->n_trunks) {
-            if (n_errors) {
-                VLOG_ERR("port %s: no valid trunks, trunking all VLANs",
-                         port->name);
-            }
-            bitmap_set_multiple(trunks, 0, 4096, 1);
-        }
-    } else {
-        if (cfg->n_trunks) {
-            VLOG_ERR("port %s: ignoring trunks in favor of implicit vlan",
+            VLOG_ERR("port %s: no valid trunks, trunking all VLANs",
                      port->name);
+            bitmap_free(trunks);
+            trunks = NULL;
         }
+    } else if (vlan >= 0 && cfg->n_trunks) {
+        VLOG_ERR("port %s: ignoring trunks in favor of implicit vlan",
+                 port->name);
     }
     if (trunks == NULL
         ? port->trunks != NULL
@@ -3367,9 +3290,6 @@ port_reconfigure(struct port *port, const struct ovsrec_port *cfg)
     }
     bitmap_free(port->trunks);
     port->trunks = trunks;
-
-    shash_destroy(&old_ifaces);
-    shash_destroy(&new_ifaces);
 }
 
 static void
@@ -3394,6 +3314,8 @@ port_destroy(struct port *port)
             iface_destroy(port->ifaces[port->n_ifaces - 1]);
         }
 
+        shash_find_and_delete_assert(&br->port_by_name, port->name);
+
         del = br->ports[port->port_idx] = br->ports[--br->n_ports];
         del->port_idx = port->port_idx;
 
@@ -3415,29 +3337,14 @@ port_from_dp_ifidx(const struct bridge *br, uint16_t dp_ifidx)
 static struct port *
 port_lookup(const struct bridge *br, const char *name)
 {
-    size_t i;
-
-    for (i = 0; i < br->n_ports; i++) {
-        struct port *port = br->ports[i];
-        if (!strcmp(port->name, name)) {
-            return port;
-        }
-    }
-    return NULL;
+    return shash_find_data(&br->port_by_name, name);
 }
 
 static struct iface *
 port_lookup_iface(const struct port *port, const char *name)
 {
-    size_t j;
-
-    for (j = 0; j < port->n_ifaces; j++) {
-        struct iface *iface = port->ifaces[j];
-        if (!strcmp(iface->name, name)) {
-            return iface;
-        }
-    }
-    return NULL;
+    struct iface *iface = iface_lookup(port->bridge, name);
+    return iface && iface->port == port ? iface : NULL;
 }
 
 static void
@@ -3590,6 +3497,7 @@ port_update_vlan_compat(struct port *port)
 static struct iface *
 iface_create(struct port *port, const struct ovsrec_interface *if_cfg)
 {
+    struct bridge *br = port->bridge;
     struct iface *iface;
     char *name = if_cfg->name;
     int error;
@@ -3604,28 +3512,34 @@ iface_create(struct port *port, const struct ovsrec_interface *if_cfg)
     iface->netdev = NULL;
     iface->cfg = if_cfg;
 
+    shash_add_assert(&br->iface_by_name, iface->name, iface);
+
+    /* Attempt to create the network interface in case it doesn't exist yet. */
+    if (!iface_is_internal(br, iface->name)) {
+        error = set_up_iface(if_cfg, iface, true);
+        if (error) {
+            VLOG_WARN("could not create iface %s: %s", iface->name,
+                      strerror(error));
+
+            shash_find_and_delete_assert(&br->iface_by_name, iface->name);
+            free(iface->name);
+            free(iface);
+            return NULL;
+        }
+    }
+
     if (port->n_ifaces >= port->allocated_ifaces) {
         port->ifaces = x2nrealloc(port->ifaces, &port->allocated_ifaces,
                                   sizeof *port->ifaces);
     }
     port->ifaces[port->n_ifaces++] = iface;
     if (port->n_ifaces > 1) {
-        port->bridge->has_bonded_ports = true;
-    }
-
-    /* Attempt to create the network interface in case it
-     * doesn't exist yet. */
-    if (!iface_is_internal(port->bridge, iface->name)) {
-        error = set_up_iface(if_cfg, iface, true);
-        if (error) {
-            VLOG_WARN("could not create iface %s: %s", iface->name,
-                    strerror(error));
-        }
+        br->has_bonded_ports = true;
     }
 
     VLOG_DBG("attached network device %s to port %s", iface->name, port->name);
 
-    bridge_flush(port->bridge);
+    bridge_flush(br);
 
     return iface;
 }
@@ -3639,6 +3553,8 @@ iface_destroy(struct iface *iface)
         bool del_active = port->active_iface == iface->port_ifidx;
         struct iface *del;
 
+        shash_find_and_delete_assert(&br->iface_by_name, iface->name);
+
         if (iface->dp_ifidx >= 0) {
             port_array_set(&br->ifaces, iface->dp_ifidx, NULL);
         }
@@ -3664,18 +3580,7 @@ iface_destroy(struct iface *iface)
 static struct iface *
 iface_lookup(const struct bridge *br, const char *name)
 {
-    size_t i, j;
-
-    for (i = 0; i < br->n_ports; i++) {
-        struct port *port = br->ports[i];
-        for (j = 0; j < port->n_ifaces; j++) {
-            struct iface *iface = port->ifaces[j];
-            if (!strcmp(iface->name, name)) {
-                return iface;
-            }
-        }
-    }
-    return NULL;
+    return shash_find_data(&br->iface_by_name, name);
 }
 
 static struct iface *
@@ -3697,7 +3602,6 @@ iface_from_dp_ifidx(const struct bridge *br, uint16_t dp_ifidx)
 static bool
 iface_is_internal(const struct bridge *br, const char *if_name)
 {
-    /* XXX wastes time */
     struct iface *iface;
     struct port *port;