ofproto-dpif: LACP registration should cause revalidation.
[sliver-openvswitch.git] / ofproto / ofproto.c
index 27c370e..1cc1e4e 100644 (file)
 #include "ofp-print.h"
 #include "ofp-util.h"
 #include "ofpbuf.h"
+#include "ofproto-provider.h"
 #include "openflow/nicira-ext.h"
 #include "openflow/openflow.h"
 #include "packets.h"
 #include "pinsched.h"
 #include "pktbuf.h"
 #include "poll-loop.h"
-#include "private.h"
 #include "shash.h"
 #include "sset.h"
 #include "timeval.h"
@@ -97,10 +97,10 @@ struct ofopgroup {
     int error;                  /* 0 if no error yet, otherwise error code. */
 };
 
-static struct ofopgroup *ofopgroup_create(struct ofproto *);
-static struct ofopgroup *ofopgroup_create_for_ofconn(struct ofconn *,
-                                                     const struct ofp_header *,
-                                                     uint32_t buffer_id);
+static struct ofopgroup *ofopgroup_create_unattached(struct ofproto *);
+static struct ofopgroup *ofopgroup_create(struct ofproto *, struct ofconn *,
+                                          const struct ofp_header *,
+                                          uint32_t buffer_id);
 static void ofopgroup_submit(struct ofopgroup *);
 static void ofopgroup_destroy(struct ofopgroup *);
 
@@ -135,23 +135,19 @@ static void ofproto_rule_send_removed(struct rule *, uint8_t reason);
 
 static void ofopgroup_destroy(struct ofopgroup *);
 
-static int add_flow(struct ofproto *, struct ofconn *, struct flow_mod *,
+static int add_flow(struct ofproto *, struct ofconn *,
+                    const struct ofputil_flow_mod *,
                     const struct ofp_header *);
 
-/* This return value tells handle_openflow() that processing of the current
- * OpenFlow message must be postponed until some ongoing operations have
- * completed.
- *
- * This particular value is a good choice because it is negative (so it won't
- * collide with any errno value or any value returned by ofp_mkerr()) and large
- * (so it won't accidentally collide with EOF or a negative errno value). */
-enum { OFPROTO_POSTPONE = -100000 };
-
 static bool handle_openflow(struct ofconn *, struct ofpbuf *);
+static int handle_flow_mod__(struct ofproto *, struct ofconn *,
+                             const struct ofputil_flow_mod *,
+                             const struct ofp_header *);
 
 static void update_port(struct ofproto *, const char *devname);
 static int init_ports(struct ofproto *);
 static void reinit_ports(struct ofproto *);
+static void set_internal_devs_mtu(struct ofproto *);
 
 static void ofproto_unixctl_init(void);
 
@@ -289,7 +285,9 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
                struct ofproto **ofprotop)
 {
     const struct ofproto_class *class;
+    struct classifier *table;
     struct ofproto *ofproto;
+    int n_tables;
     int error;
 
     *ofprotop = NULL;
@@ -320,6 +318,9 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
     hmap_insert(&all_ofprotos, &ofproto->hmap_node,
                 hash_string(ofproto->name, 0));
     ofproto->datapath_id = 0;
+    ofproto_set_flow_eviction_threshold(ofproto,
+                                        OFPROTO_FLOW_EVICTON_THRESHOLD_DEFAULT);
+    ofproto->forward_bpdu = false;
     ofproto->fallback_dpid = pick_fallback_dpid();
     ofproto->mfr_desc = xstrdup(DEFAULT_MFR_DESC);
     ofproto->hw_desc = xstrdup(DEFAULT_HW_DESC);
@@ -333,16 +334,23 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
     ofproto->connmgr = connmgr_create(ofproto, datapath_name, datapath_name);
     ofproto->state = S_OPENFLOW;
     list_init(&ofproto->pending);
+    ofproto->n_pending = 0;
     hmap_init(&ofproto->deletions);
 
-    error = ofproto->ofproto_class->construct(ofproto);
+    error = ofproto->ofproto_class->construct(ofproto, &n_tables);
     if (error) {
         VLOG_ERR("failed to open datapath %s: %s",
                  datapath_name, strerror(error));
         ofproto_destroy__(ofproto);
         return error;
     }
-    assert(ofproto->n_tables > 0);
+
+    assert(n_tables >= 1 && n_tables <= 255);
+    ofproto->n_tables = n_tables;
+    ofproto->tables = xmalloc(n_tables * sizeof *ofproto->tables);
+    OFPROTO_FOR_EACH_TABLE (table, ofproto) {
+        classifier_init(table);
+    }
 
     ofproto->datapath_id = pick_datapath_id(ofproto);
     VLOG_INFO("using datapath ID %016"PRIx64, ofproto->datapath_id);
@@ -407,6 +415,33 @@ ofproto_set_in_band_queue(struct ofproto *ofproto, int queue_id)
     connmgr_set_in_band_queue(ofproto->connmgr, queue_id);
 }
 
+/* Sets the number of flows at which eviction from the kernel flow table
+ * will occur. */
+void
+ofproto_set_flow_eviction_threshold(struct ofproto *ofproto, unsigned threshold)
+{
+    if (threshold < OFPROTO_FLOW_EVICTION_THRESHOLD_MIN) {
+        ofproto->flow_eviction_threshold = OFPROTO_FLOW_EVICTION_THRESHOLD_MIN;
+    } else {
+        ofproto->flow_eviction_threshold = threshold;
+    }
+}
+
+/* If forward_bpdu is true, the NORMAL action will forward frames with
+ * reserved (e.g. STP) destination Ethernet addresses. if forward_bpdu is false,
+ * the NORMAL action will drop these frames. */
+void
+ofproto_set_forward_bpdu(struct ofproto *ofproto, bool forward_bpdu)
+{
+    bool old_val = ofproto->forward_bpdu;
+    ofproto->forward_bpdu = forward_bpdu;
+    if (old_val != ofproto->forward_bpdu) {
+        if (ofproto->ofproto_class->forward_bpdu_changed) {
+            ofproto->ofproto_class->forward_bpdu_changed(ofproto);
+        }
+    }
+}
+
 void
 ofproto_set_desc(struct ofproto *p,
                  const char *mfr_desc, const char *hw_desc,
@@ -652,9 +687,8 @@ ofproto_flush__(struct ofproto *ofproto)
         ofproto->ofproto_class->flush(ofproto);
     }
 
-    group = ofopgroup_create(ofproto);
-    for (table = ofproto->tables; table < &ofproto->tables[ofproto->n_tables];
-         table++) {
+    group = ofopgroup_create_unattached(ofproto);
+    OFPROTO_FOR_EACH_TABLE (table, ofproto) {
         struct rule *rule, *next_rule;
         struct cls_cursor cursor;
 
@@ -673,9 +707,10 @@ ofproto_flush__(struct ofproto *ofproto)
 static void
 ofproto_destroy__(struct ofproto *ofproto)
 {
-    size_t i;
+    struct classifier *table;
 
     assert(list_is_empty(&ofproto->pending));
+    assert(!ofproto->n_pending);
 
     connmgr_destroy(ofproto->connmgr);
 
@@ -690,9 +725,9 @@ ofproto_destroy__(struct ofproto *ofproto)
     hmap_destroy(&ofproto->ports);
     shash_destroy(&ofproto->port_by_name);
 
-    for (i = 0; i < ofproto->n_tables; i++) {
-        assert(classifier_is_empty(&ofproto->tables[i]));
-        classifier_destroy(&ofproto->tables[i]);
+    OFPROTO_FOR_EACH_TABLE (table, ofproto) {
+        assert(classifier_is_empty(table));
+        classifier_destroy(table);
     }
     free(ofproto->tables);
 
@@ -845,16 +880,7 @@ ofproto_get_ofproto_controller_info(const struct ofproto *ofproto,
 void
 ofproto_free_ofproto_controller_info(struct shash *info)
 {
-    struct shash_node *node;
-
-    SHASH_FOR_EACH (node, info) {
-        struct ofproto_controller_info *cinfo = node->data;
-        while (cinfo->pairs.n) {
-            free((char *) cinfo->pairs.values[--cinfo->pairs.n]);
-        }
-        free(cinfo);
-    }
-    shash_destroy(info);
+    connmgr_free_controller_info(info);
 }
 
 /* Makes a deep copy of 'old' into 'port'. */
@@ -1020,7 +1046,7 @@ ofproto_add_flow(struct ofproto *ofproto, const struct cls_rule *cls_rule,
                                     &ofproto->tables[0], cls_rule));
     if (!rule || !ofputil_actions_equal(rule->actions, rule->n_actions,
                                         actions, n_actions)) {
-        struct flow_mod fm;
+        struct ofputil_flow_mod fm;
 
         memset(&fm, 0, sizeof fm);
         fm.cr = *cls_rule;
@@ -1031,6 +1057,18 @@ ofproto_add_flow(struct ofproto *ofproto, const struct cls_rule *cls_rule,
     }
 }
 
+/* Executes the flow modification specified in 'fm'.  Returns 0 on success, an
+ * OpenFlow error code as encoded by ofp_mkerr() on failure, or
+ * OFPROTO_POSTPONE if the operation cannot be initiated now but may be retried
+ * later.
+ *
+ * This is a helper function for in-band control and fail-open. */
+int
+ofproto_flow_mod(struct ofproto *ofproto, const struct ofputil_flow_mod *fm)
+{
+    return handle_flow_mod__(ofproto, NULL, fm, NULL);
+}
+
 /* Searches for a rule with matching criteria exactly equal to 'target' in
  * ofproto's table 0 and, if it finds one, deletes it.
  *
@@ -1051,7 +1089,7 @@ ofproto_delete_flow(struct ofproto *ofproto, const struct cls_rule *target)
         return false;
     } else {
         /* Initiate deletion -> success. */
-        struct ofopgroup *group = ofopgroup_create(ofproto);
+        struct ofopgroup *group = ofopgroup_create_unattached(ofproto);
         ofoperation_create(group, rule, OFOPERATION_DELETE);
         classifier_remove(&ofproto->tables[rule->table_id], &rule->cr);
         rule->ofproto->ofproto_class->rule_destruct(rule);
@@ -1102,17 +1140,11 @@ static struct netdev *
 ofport_open(const struct ofproto_port *ofproto_port, struct ofp_phy_port *opp)
 {
     uint32_t curr, advertised, supported, peer;
-    struct netdev_options netdev_options;
     enum netdev_flags flags;
     struct netdev *netdev;
     int error;
 
-    memset(&netdev_options, 0, sizeof netdev_options);
-    netdev_options.name = ofproto_port->name;
-    netdev_options.type = ofproto_port->type;
-    netdev_options.ethertype = NETDEV_ETH_TYPE_NONE;
-
-    error = netdev_open(&netdev_options, &netdev);
+    error = netdev_open(ofproto_port->name, ofproto_port->type, &netdev);
     if (error) {
         VLOG_WARN_RL(&rl, "ignoring port %s (%"PRIu16") because netdev %s "
                      "cannot be opened (%s)",
@@ -1162,6 +1194,7 @@ ofport_install(struct ofproto *p,
 {
     const char *netdev_name = netdev_get_name(netdev);
     struct ofport *ofport;
+    int dev_mtu;
     int error;
 
     /* Create ofport. */
@@ -1180,6 +1213,13 @@ ofport_install(struct ofproto *p,
     hmap_insert(&p->ports, &ofport->hmap_node, hash_int(ofport->ofp_port, 0));
     shash_add(&p->port_by_name, netdev_name, ofport);
 
+    if (!netdev_get_mtu(netdev, &dev_mtu)) {
+        set_internal_devs_mtu(p);
+        ofport->mtu = dev_mtu;
+    } else {
+        ofport->mtu = 0;
+    }
+
     /* Let the ofproto_class initialize its private data. */
     error = p->ofproto_class->port_construct(ofport);
     if (error) {
@@ -1306,12 +1346,22 @@ 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;
+            int dev_mtu;
 
             /* 'name' hasn't changed location.  Any properties changed? */
             if (!ofport_equal(&port->opp, &opp)) {
                 ofport_modified(port, &opp);
             }
 
+            /* If this is a non-internal port and the MTU changed, check
+             * if the datapath's MTU needs to be updated. */
+            if (strcmp(netdev_get_type(netdev), "internal")
+                    && !netdev_get_mtu(netdev, &dev_mtu)
+                    && port->mtu != dev_mtu) {
+                set_internal_devs_mtu(ofproto);
+                port->mtu = dev_mtu;
+            }
+
             /* Install the newly opened netdev in case it has changed.
              * Don't close the old netdev yet in case port_modified has to
              * remove a retained reference to it.*/
@@ -1367,6 +1417,52 @@ init_ports(struct ofproto *p)
 
     return 0;
 }
+
+/* Find the minimum MTU of all non-datapath devices attached to 'p'.
+ * Returns ETH_PAYLOAD_MAX or the minimum of the ports. */
+static int
+find_min_mtu(struct ofproto *p)
+{
+    struct ofport *ofport;
+    int mtu = 0;
+
+    HMAP_FOR_EACH (ofport, hmap_node, &p->ports) {
+        struct netdev *netdev = ofport->netdev;
+        int dev_mtu;
+
+        /* Skip any internal ports, since that's what we're trying to
+         * set. */
+        if (!strcmp(netdev_get_type(netdev), "internal")) {
+            continue;
+        }
+
+        if (netdev_get_mtu(netdev, &dev_mtu)) {
+            continue;
+        }
+        if (!mtu || dev_mtu < mtu) {
+            mtu = dev_mtu;
+        }
+    }
+
+    return mtu ? mtu: ETH_PAYLOAD_MAX;
+}
+
+/* Set the MTU of all datapath devices on 'p' to the minimum of the
+ * non-datapath ports. */
+static void
+set_internal_devs_mtu(struct ofproto *p)
+{
+    struct ofport *ofport;
+    int mtu = find_min_mtu(p);
+
+    HMAP_FOR_EACH (ofport, hmap_node, &p->ports) {
+        struct netdev *netdev = ofport->netdev;
+
+        if (!strcmp(netdev_get_type(netdev), "internal")) {
+            netdev_set_mtu(netdev, mtu);
+        }
+    }
+}
 \f
 static void
 ofproto_rule_destroy__(struct rule *rule)
@@ -1378,8 +1474,8 @@ ofproto_rule_destroy__(struct rule *rule)
 /* This function allows an ofproto implementation to destroy any rules that
  * remain when its ->destruct() function is called.  The caller must have
  * already uninitialized any derived members of 'rule' (step 5 described in the
- * large comment in ofproto/private.h titled "Life Cycle").  This function
- * implements steps 6 and 7.
+ * large comment in ofproto/ofproto-provider.h titled "Life Cycle").
+ * This function implements steps 6 and 7.
  *
  * This function should only be called from an ofproto implementation's
  * ->destruct() function.  It is not suitable elsewhere. */
@@ -1398,13 +1494,12 @@ static bool
 rule_has_out_port(const struct rule *rule, uint16_t out_port)
 {
     const union ofp_action *oa;
-    struct actions_iterator i;
+    size_t left;
 
     if (out_port == OFPP_NONE) {
         return true;
     }
-    for (oa = actions_first(&i, rule->actions, rule->n_actions); oa;
-         oa = actions_next(&i)) {
+    OFPUTIL_ACTION_FOR_EACH_UNSAFE (oa, left, rule->actions, rule->n_actions) {
         if (action_outputs_to_port(oa, htons(out_port))) {
             return true;
         }
@@ -1560,7 +1655,6 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
     struct ofpbuf request;
     struct flow flow;
     size_t n_ofp_actions;
-    uint16_t in_port;
     int error;
 
     COVERAGE_INC(ofproto_packet_out);
@@ -1584,7 +1678,7 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
     /* Get payload. */
     if (opo->buffer_id != htonl(UINT32_MAX)) {
         error = ofconn_pktbuf_retrieve(ofconn, ntohl(opo->buffer_id),
-                                       &buffer, &in_port);
+                                       &buffer, NULL);
         if (error || !buffer) {
             return error;
         }
@@ -1878,7 +1972,7 @@ handle_flow_stats_request(struct ofconn *ofconn,
                           const struct ofp_stats_msg *osm)
 {
     struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
-    struct flow_stats_request fsr;
+    struct ofputil_flow_stats_request fsr;
     struct list replies;
     struct list rules;
     struct rule *rule;
@@ -1921,7 +2015,6 @@ static void
 flow_stats_ds(struct rule *rule, struct ds *results)
 {
     uint64_t packet_count, byte_count;
-    size_t act_len = sizeof *rule->actions * rule->n_actions;
 
     rule->ofproto->ofproto_class->rule_get_stats(rule,
                                                  &packet_count, &byte_count);
@@ -1936,8 +2029,8 @@ flow_stats_ds(struct rule *rule, struct ds *results)
     ds_put_format(results, "n_bytes=%"PRIu64", ", byte_count);
     cls_rule_format(&rule->cr, results);
     ds_put_char(results, ',');
-    if (act_len > 0) {
-        ofp_print_actions(results, &rule->actions->header, act_len);
+    if (rule->n_actions > 0) {
+        ofp_print_actions(results, rule->actions, rule->n_actions);
     } else {
         ds_put_cstr(results, "drop");
     }
@@ -1951,7 +2044,7 @@ ofproto_get_all_flows(struct ofproto *p, struct ds *results)
 {
     struct classifier *cls;
 
-    for (cls = &p->tables[0]; cls < &p->tables[p->n_tables]; cls++) {
+    OFPROTO_FOR_EACH_TABLE (cls, p) {
         struct cls_cursor cursor;
         struct rule *rule;
 
@@ -1983,12 +2076,31 @@ ofproto_port_get_cfm_fault(const struct ofproto *ofproto, uint16_t ofp_port)
             : -1);
 }
 
+/* Gets the MPIDs of the remote maintenance points broadcasting to 'ofp_port'
+ * within 'ofproto'.  Populates 'rmps' with an array of MPIDs owned by
+ * 'ofproto', and 'n_rmps' with the number of MPIDs in 'rmps'.  Returns a
+ * number less than 0 if CFM is not enabled on 'ofp_port'. */
+int
+ofproto_port_get_cfm_remote_mpids(const struct ofproto *ofproto,
+                                  uint16_t ofp_port, const uint64_t **rmps,
+                                  size_t *n_rmps)
+{
+    struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
+
+    *rmps = NULL;
+    *n_rmps = 0;
+    return (ofport && ofproto->ofproto_class->get_cfm_remote_mpids
+            ? ofproto->ofproto_class->get_cfm_remote_mpids(ofport, rmps,
+                                                           n_rmps)
+            : -1);
+}
+
 static int
 handle_aggregate_stats_request(struct ofconn *ofconn,
                                const struct ofp_stats_msg *osm)
 {
     struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
-    struct flow_stats_request request;
+    struct ofputil_flow_stats_request request;
     struct ofputil_aggregate_stats stats;
     bool unknown_packets, unknown_bytes;
     struct ofpbuf *reply;
@@ -2148,14 +2260,15 @@ is_flow_deletion_pending(const struct ofproto *ofproto,
  * in which no matching flow already exists in the flow table.
  *
  * Adds the flow specified by 'ofm', which is followed by 'n_actions'
- * ofp_actions, to the ofproto's flow table.  Returns 0 on success or an
- * OpenFlow error code as encoded by ofp_mkerr() on failure.
+ * ofp_actions, to the ofproto's flow table.  Returns 0 on success, an OpenFlow
+ * error code as encoded by ofp_mkerr() on failure, or OFPROTO_POSTPONE if the
+ * operation cannot be initiated now but may be retried later.
  *
  * 'ofconn' is used to retrieve the packet buffer specified in ofm->buffer_id,
  * if any. */
 static int
-add_flow(struct ofproto *ofproto, struct ofconn *ofconn, struct flow_mod *fm,
-         const struct ofp_header *request)
+add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
+         const struct ofputil_flow_mod *fm, const struct ofp_header *request)
 {
     struct classifier *table;
     struct ofopgroup *group;
@@ -2177,7 +2290,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn, struct flow_mod *fm,
     /* Pick table. */
     if (fm->table_id == 0xff) {
         uint8_t table_id;
-        if (ofproto->n_tables > 1) {
+        if (ofproto->ofproto_class->rule_choose_table) {
             error = ofproto->ofproto_class->rule_choose_table(ofproto, &fm->cr,
                                                               &table_id);
             if (error) {
@@ -2210,7 +2323,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn, struct flow_mod *fm,
     rule->cr = fm->cr;
     rule->pending = NULL;
     rule->flow_cookie = fm->cookie;
-    rule->created = time_msec();
+    rule->created = rule->modified = time_msec();
     rule->idle_timeout = fm->idle_timeout;
     rule->hard_timeout = fm->hard_timeout;
     rule->table_id = table - ofproto->tables;
@@ -2223,9 +2336,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn, struct flow_mod *fm,
     if (victim && victim->pending) {
         error = OFPROTO_POSTPONE;
     } else {
-        group = (ofconn
-                 ? ofopgroup_create_for_ofconn(ofconn, request, fm->buffer_id)
-                 : ofopgroup_create(ofproto));
+        group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id);
         ofoperation_create(group, rule, OFOPERATION_ADD);
         rule->pending->victim = victim;
 
@@ -2258,13 +2369,14 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn, struct flow_mod *fm,
  *
  * Returns 0 on success, otherwise an OpenFlow error code. */
 static int
-modify_flows__(struct ofconn *ofconn, const struct flow_mod *fm,
+modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
+               const struct ofputil_flow_mod *fm,
                const struct ofp_header *request, struct list *rules)
 {
     struct ofopgroup *group;
     struct rule *rule;
 
-    group = ofopgroup_create_for_ofconn(ofconn, request, fm->buffer_id);
+    group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id);
     LIST_FOR_EACH (rule, ofproto_node, rules) {
         if (!ofputil_actions_equal(fm->actions, fm->n_actions,
                                    rule->actions, rule->n_actions)) {
@@ -2274,6 +2386,8 @@ modify_flows__(struct ofconn *ofconn, const struct flow_mod *fm,
             rule->actions = ofputil_actions_clone(fm->actions, fm->n_actions);
             rule->n_actions = fm->n_actions;
             rule->ofproto->ofproto_class->rule_modify_actions(rule);
+        } else {
+            rule->modified = time_msec();
         }
         rule->flow_cookie = fm->cookie;
     }
@@ -2288,17 +2402,18 @@ modify_flows__(struct ofconn *ofconn, const struct flow_mod *fm,
  * 'ofconn' is used to retrieve the packet buffer specified in fm->buffer_id,
  * if any. */
 static int
-modify_flows_loose(struct ofconn *ofconn, struct flow_mod *fm,
+modify_flows_loose(struct ofproto *ofproto, struct ofconn *ofconn,
+                   const struct ofputil_flow_mod *fm,
                    const struct ofp_header *request)
 {
-    struct ofproto *p = ofconn_get_ofproto(ofconn);
     struct list rules;
     int error;
 
-    error = collect_rules_loose(p, fm->table_id, &fm->cr, OFPP_NONE, &rules);
+    error = collect_rules_loose(ofproto, fm->table_id, &fm->cr, OFPP_NONE,
+                                &rules);
     return (error ? error
-            : list_is_empty(&rules) ? add_flow(p, ofconn, fm, request)
-            : modify_flows__(ofconn, fm, request, &rules));
+            : list_is_empty(&rules) ? add_flow(ofproto, ofconn, fm, request)
+            : modify_flows__(ofproto, ofconn, fm, request, &rules));
 }
 
 /* Implements OFPFC_MODIFY_STRICT.  Returns 0 on success or an OpenFlow error
@@ -2307,18 +2422,19 @@ modify_flows_loose(struct ofconn *ofconn, struct flow_mod *fm,
  * 'ofconn' is used to retrieve the packet buffer specified in fm->buffer_id,
  * if any. */
 static int
-modify_flow_strict(struct ofconn *ofconn, struct flow_mod *fm,
+modify_flow_strict(struct ofproto *ofproto, struct ofconn *ofconn,
+                   const struct ofputil_flow_mod *fm,
                    const struct ofp_header *request)
 {
-    struct ofproto *p = ofconn_get_ofproto(ofconn);
     struct list rules;
     int error;
 
-    error = collect_rules_strict(p, fm->table_id, &fm->cr, OFPP_NONE, &rules);
+    error = collect_rules_strict(ofproto, fm->table_id, &fm->cr, OFPP_NONE,
+                                 &rules);
     return (error ? error
-            : list_is_empty(&rules) ? add_flow(p, ofconn, fm, request)
-            : list_is_singleton(&rules) ? modify_flows__(ofconn, fm, request,
-                                                         &rules)
+            : list_is_empty(&rules) ? add_flow(ofproto, ofconn, fm, request)
+            : list_is_singleton(&rules) ? modify_flows__(ofproto, ofconn,
+                                                         fm, request, &rules)
             : 0);
 }
 \f
@@ -2328,14 +2444,13 @@ modify_flow_strict(struct ofconn *ofconn, struct flow_mod *fm,
  *
  * Returns 0 on success, otherwise an OpenFlow error code. */
 static int
-delete_flows__(struct ofconn *ofconn, const struct ofp_header *request,
-               struct list *rules)
+delete_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
+               const struct ofp_header *request, struct list *rules)
 {
-    struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
     struct rule *rule, *next;
     struct ofopgroup *group;
 
-    group = ofopgroup_create_for_ofconn(ofconn, request, UINT32_MAX);
+    group = ofopgroup_create(ofproto, ofconn, request, UINT32_MAX);
     LIST_FOR_EACH_SAFE (rule, next, ofproto_node, rules) {
         ofproto_rule_send_removed(rule, OFPRR_DELETE);
 
@@ -2350,34 +2465,35 @@ delete_flows__(struct ofconn *ofconn, const struct ofp_header *request,
 
 /* Implements OFPFC_DELETE. */
 static int
-delete_flows_loose(struct ofconn *ofconn, const struct flow_mod *fm,
+delete_flows_loose(struct ofproto *ofproto, struct ofconn *ofconn,
+                   const struct ofputil_flow_mod *fm,
                    const struct ofp_header *request)
 {
-    struct ofproto *p = ofconn_get_ofproto(ofconn);
     struct list rules;
     int error;
 
-    error = collect_rules_loose(p, fm->table_id, &fm->cr, fm->out_port,
+    error = collect_rules_loose(ofproto, fm->table_id, &fm->cr, fm->out_port,
                                 &rules);
     return (error ? error
-            : !list_is_empty(&rules) ? delete_flows__(ofconn, request, &rules)
+            : !list_is_empty(&rules) ? delete_flows__(ofproto, ofconn, request,
+                                                      &rules)
             : 0);
 }
 
 /* Implements OFPFC_DELETE_STRICT. */
 static int
-delete_flow_strict(struct ofconn *ofconn, struct flow_mod *fm,
+delete_flow_strict(struct ofproto *ofproto, struct ofconn *ofconn,
+                   const struct ofputil_flow_mod *fm,
                    const struct ofp_header *request)
 {
-    struct ofproto *p = ofconn_get_ofproto(ofconn);
     struct list rules;
     int error;
 
-    error = collect_rules_strict(p, fm->table_id, &fm->cr, fm->out_port,
+    error = collect_rules_strict(ofproto, fm->table_id, &fm->cr, fm->out_port,
                                  &rules);
     return (error ? error
-            : list_is_singleton(&rules) ? delete_flows__(ofconn, request,
-                                                         &rules)
+            : list_is_singleton(&rules) ? delete_flows__(ofproto, ofconn,
+                                                         request, &rules)
             : 0);
 }
 
@@ -2417,7 +2533,7 @@ ofproto_rule_expire(struct rule *rule, uint8_t reason)
 
     ofproto_rule_send_removed(rule, reason);
 
-    group = ofopgroup_create(ofproto);
+    group = ofopgroup_create_unattached(ofproto);
     ofoperation_create(group, rule, OFOPERATION_DELETE);
     classifier_remove(&ofproto->tables[rule->table_id], &rule->cr);
     rule->ofproto->ofproto_class->rule_destruct(rule);
@@ -2427,8 +2543,7 @@ ofproto_rule_expire(struct rule *rule, uint8_t reason)
 static int
 handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
 {
-    struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
-    struct flow_mod fm;
+    struct ofputil_flow_mod fm;
     int error;
 
     error = reject_slave_controller(ofconn, "flow_mod");
@@ -2436,10 +2551,6 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
         return error;
     }
 
-    if (list_size(&ofproto->pending) >= 50) {
-        return OFPROTO_POSTPONE;
-    }
-
     error = ofputil_decode_flow_mod(&fm, oh,
                                     ofconn_get_flow_mod_table_id(ofconn));
     if (error) {
@@ -2454,24 +2565,37 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
         return ofp_mkerr(OFPET_FLOW_MOD_FAILED, OFPFMFC_ALL_TABLES_FULL);
     }
 
-    switch (fm.command) {
+    return handle_flow_mod__(ofconn_get_ofproto(ofconn), ofconn, &fm, oh);
+}
+
+static int
+handle_flow_mod__(struct ofproto *ofproto, struct ofconn *ofconn,
+                  const struct ofputil_flow_mod *fm,
+                  const struct ofp_header *oh)
+{
+    if (ofproto->n_pending >= 50) {
+        assert(!list_is_empty(&ofproto->pending));
+        return OFPROTO_POSTPONE;
+    }
+
+    switch (fm->command) {
     case OFPFC_ADD:
-        return add_flow(ofproto, ofconn, &fm, oh);
+        return add_flow(ofproto, ofconn, fm, oh);
 
     case OFPFC_MODIFY:
-        return modify_flows_loose(ofconn, &fm, oh);
+        return modify_flows_loose(ofproto, ofconn, fm, oh);
 
     case OFPFC_MODIFY_STRICT:
-        return modify_flow_strict(ofconn, &fm, oh);
+        return modify_flow_strict(ofproto, ofconn, fm, oh);
 
     case OFPFC_DELETE:
-        return delete_flows_loose(ofconn, &fm, oh);
+        return delete_flows_loose(ofproto, ofconn, fm, oh);
 
     case OFPFC_DELETE_STRICT:
-        return delete_flow_strict(ofconn, &fm, oh);
+        return delete_flow_strict(ofproto, ofconn, fm, oh);
 
     default:
-        if (fm.command > 0xff) {
+        if (fm->command > 0xff) {
             VLOG_WARN_RL(&rl, "flow_mod has explicit table_id but "
                          "flow_mod_table_id extension is not enabled");
         }
@@ -2639,7 +2763,7 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg)
     case OFPUTIL_OFPST_QUEUE_REQUEST:
         return handle_queue_stats_request(ofconn, msg->data);
 
-    case OFPUTIL_INVALID:
+    case OFPUTIL_MSG_INVALID:
     case OFPUTIL_OFPT_HELLO:
     case OFPUTIL_OFPT_ERROR:
     case OFPUTIL_OFPT_FEATURES_REPLY:
@@ -2693,7 +2817,7 @@ handle_openflow(struct ofconn *ofconn, struct ofpbuf *ofp_msg)
  * The caller should add operations to the returned group with
  * ofoperation_create() and then submit it with ofopgroup_submit(). */
 static struct ofopgroup *
-ofopgroup_create(struct ofproto *ofproto)
+ofopgroup_create_unattached(struct ofproto *ofproto)
 {
     struct ofopgroup *group = xzalloc(sizeof *group);
     group->ofproto = ofproto;
@@ -2703,26 +2827,33 @@ ofopgroup_create(struct ofproto *ofproto)
     return group;
 }
 
-/* Creates and returns a new ofopgroup that is associated with 'ofconn'.  If
- * the ofopgroup eventually fails, then the error reply will include 'request'.
- * If the ofopgroup eventually succeeds, then the packet with buffer id
- * 'buffer_id' on 'ofconn' will be sent by 'ofconn''s ofproto.
+/* Creates and returns a new ofopgroup for 'ofproto'.
+ *
+ * If 'ofconn' is NULL, the new ofopgroup is not associated with any OpenFlow
+ * connection.  The 'request' and 'buffer_id' arguments are ignored.
+ *
+ * If 'ofconn' is nonnull, then the new ofopgroup is associated with 'ofconn'.
+ * If the ofopgroup eventually fails, then the error reply will include
+ * 'request'.  If the ofopgroup eventually succeeds, then the packet with
+ * buffer id 'buffer_id' on 'ofconn' will be sent by 'ofconn''s ofproto.
  *
  * The caller should add operations to the returned group with
  * ofoperation_create() and then submit it with ofopgroup_submit(). */
 static struct ofopgroup *
-ofopgroup_create_for_ofconn(struct ofconn *ofconn,
-                            const struct ofp_header *request,
-                            uint32_t buffer_id)
+ofopgroup_create(struct ofproto *ofproto, struct ofconn *ofconn,
+                 const struct ofp_header *request, uint32_t buffer_id)
 {
-    struct ofopgroup *group = ofopgroup_create(ofconn_get_ofproto(ofconn));
-    size_t request_len = ntohs(request->length);
+    struct ofopgroup *group = ofopgroup_create_unattached(ofproto);
+    if (ofconn) {
+        size_t request_len = ntohs(request->length);
 
-    ofconn_add_opgroup(ofconn, &group->ofconn_node);
-    group->ofconn = ofconn;
-    group->request = xmemdup(request, MIN(request_len, 64));
-    group->buffer_id = buffer_id;
+        assert(ofconn_get_ofproto(ofconn) == ofproto);
 
+        ofconn_add_opgroup(ofconn, &group->ofconn_node);
+        group->ofconn = ofconn;
+        group->request = xmemdup(request, MIN(request_len, 64));
+        group->buffer_id = buffer_id;
+    }
     return group;
 }
 
@@ -2739,6 +2870,7 @@ ofopgroup_submit(struct ofopgroup *group)
         ofopgroup_destroy(group);
     } else {
         list_push_back(&group->ofproto->pending, &group->ofproto_node);
+        group->ofproto->n_pending++;
     }
 }
 
@@ -2747,6 +2879,8 @@ ofopgroup_destroy(struct ofopgroup *group)
 {
     assert(list_is_empty(&group->ops));
     if (!list_is_empty(&group->ofproto_node)) {
+        assert(group->ofproto->n_pending > 0);
+        group->ofproto->n_pending--;
         list_remove(&group->ofproto_node);
     }
     if (!list_is_empty(&group->ofconn_node)) {
@@ -2808,11 +2942,32 @@ ofoperation_destroy(struct ofoperation *op)
  * indicate success or an OpenFlow error code (constructed with
  * e.g. ofp_mkerr()).
  *
- * If 'op' is a "delete flow" operation, 'error' must be 0.  That is, flow
- * deletions are not allowed to fail.
+ * If 'error' is 0, indicating success, the operation will be committed
+ * permanently to the flow table.  There is one interesting subcase:
+ *
+ *   - If 'op' is an "add flow" operation that is replacing an existing rule in
+ *     the flow table (the "victim" rule) by a new one, then the caller must
+ *     have uninitialized any derived state in the victim rule, as in step 5 in
+ *     the "Life Cycle" in ofproto/ofproto-provider.h.  ofoperation_complete()
+ *     performs steps 6 and 7 for the victim rule, most notably by calling its
+ *     ->rule_dealloc() function.
+ *
+ * If 'error' is nonzero, then generally the operation will be rolled back:
  *
- * Please see the large comment in ofproto/private.h titled "Asynchronous
- * Operation Support" for more information. */
+ *   - If 'op' is an "add flow" operation, ofproto removes the new rule or
+ *     restores the original rule.  The caller must have uninitialized any
+ *     derived state in the new rule, as in step 5 of in the "Life Cycle" in
+ *     ofproto/ofproto-provider.h.  ofoperation_complete() performs steps 6 and
+ *     and 7 for the new rule, calling its ->rule_dealloc() function.
+ *
+ *   - If 'op' is a "modify flow" operation, ofproto restores the original
+ *     actions.
+ *
+ *   - 'op' must not be a "delete flow" operation.  Removing a rule is not
+ *     allowed to fail.  It must always succeed.
+ *
+ * Please see the large comment in ofproto/ofproto-provider.h titled
+ * "Asynchronous Operation Support" for more information. */
 void
 ofoperation_complete(struct ofoperation *op, int error)
 {
@@ -2869,7 +3024,9 @@ ofoperation_complete(struct ofoperation *op, int error)
         break;
 
     case OFOPERATION_MODIFY:
-        if (error) {
+        if (!error) {
+            rule->modified = time_msec();
+        } else {
             free(rule->actions);
             rule->actions = op->actions;
             rule->n_actions = op->n_actions;
@@ -2958,5 +3115,5 @@ ofproto_unixctl_init(void)
     }
     registered = true;
 
-    unixctl_command_register("ofproto/list", ofproto_unixctl_list, NULL);
+    unixctl_command_register("ofproto/list", "", ofproto_unixctl_list, NULL);
 }