ofproto: Revert change in flow cookie when an ofoperation fails.
[openvswitch] / ofproto / ofproto.c
index b23c79b8a770f356caf4722a27047a53bf3b35b8..66e8bb86c62cf73842347ed0ae5b93ee9bc43580 100644 (file)
@@ -87,6 +87,7 @@ struct ofopgroup {
     struct ofproto *ofproto;    /* Owning ofproto. */
     struct list ofproto_node;   /* In ofproto's "pending" list. */
     struct list ops;            /* List of "struct ofoperation"s. */
+    int n_running;              /* Number of ops still pending. */
 
     /* Data needed to send OpenFlow reply on failure or to send a buffered
      * packet on success.
@@ -101,7 +102,6 @@ struct ofopgroup {
     struct ofconn *ofconn;      /* ofconn for reply (but see note above). */
     struct ofp_header *request; /* Original request (truncated at 64 bytes). */
     uint32_t buffer_id;         /* Buffer id from original request. */
-    int error;                  /* 0 if no error yet, otherwise error code. */
 };
 
 static struct ofopgroup *ofopgroup_create_unattached(struct ofproto *);
@@ -109,7 +109,7 @@ 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 *);
+static void ofopgroup_complete(struct ofopgroup *);
 
 /* A single flow table operation. */
 struct ofoperation {
@@ -118,14 +118,16 @@ struct ofoperation {
     struct hmap_node hmap_node; /* In ofproto's "deletions" hmap. */
     struct rule *rule;          /* Rule being operated upon. */
     enum ofoperation_type type; /* Type of operation. */
-    struct rule *victim;        /* OFOPERATION_ADDING: Replaced rule. */
-    struct ofpact *ofpacts;     /* OFOPERATION_MODIFYING: Replaced actions. */
-    size_t ofpacts_len;         /* OFOPERATION_MODIFYING: Bytes of ofpacts. */
+    struct rule *victim;        /* OFOPERATION_ADD: Replaced rule. */
+    struct ofpact *ofpacts;     /* OFOPERATION_MODIFY: Replaced actions. */
+    size_t ofpacts_len;         /* OFOPERATION_MODIFY: Bytes of ofpacts. */
     ovs_be64 flow_cookie;       /* Rule's old flow cookie. */
+    enum ofperr error;          /* 0 if no error. */
 };
 
-static void ofoperation_create(struct ofopgroup *, struct rule *,
-                               enum ofoperation_type);
+static struct ofoperation *ofoperation_create(struct ofopgroup *,
+                                              struct rule *,
+                                              enum ofoperation_type);
 static void ofoperation_destroy(struct ofoperation *);
 
 /* oftable. */
@@ -405,7 +407,6 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
     assert(ofproto->n_tables);
 
     ofproto->datapath_id = pick_datapath_id(ofproto);
-    VLOG_INFO("using datapath ID %016"PRIx64, ofproto->datapath_id);
     init_ports(ofproto);
 
     *ofprotop = ofproto;
@@ -427,14 +428,18 @@ ofproto_init_tables(struct ofproto *ofproto, int n_tables)
     }
 }
 
+uint64_t
+ofproto_get_datapath_id(const struct ofproto *ofproto)
+{
+    return ofproto->datapath_id;
+}
+
 void
 ofproto_set_datapath_id(struct ofproto *p, uint64_t datapath_id)
 {
     uint64_t old_dpid = p->datapath_id;
     p->datapath_id = datapath_id ? datapath_id : pick_datapath_id(p);
     if (p->datapath_id != old_dpid) {
-        VLOG_INFO("datapath ID changed to %016"PRIx64, p->datapath_id);
-
         /* Force all active connections to reconnect, since there is no way to
          * notify a controller that the datapath ID has changed. */
         ofproto_reconnect_controllers(p);
@@ -529,41 +534,40 @@ ofproto_set_desc(struct ofproto *p,
 
     if (mfr_desc) {
         if (strlen(mfr_desc) >= sizeof ods->mfr_desc) {
-            VLOG_WARN("truncating mfr_desc, must be less than %zu characters",
-                    sizeof ods->mfr_desc);
+            VLOG_WARN("%s: truncating mfr_desc, must be less than %zu bytes",
+                      p->name, sizeof ods->mfr_desc);
         }
         free(p->mfr_desc);
         p->mfr_desc = xstrdup(mfr_desc);
     }
     if (hw_desc) {
         if (strlen(hw_desc) >= sizeof ods->hw_desc) {
-            VLOG_WARN("truncating hw_desc, must be less than %zu characters",
-                    sizeof ods->hw_desc);
+            VLOG_WARN("%s: truncating hw_desc, must be less than %zu bytes",
+                      p->name, sizeof ods->hw_desc);
         }
         free(p->hw_desc);
         p->hw_desc = xstrdup(hw_desc);
     }
     if (sw_desc) {
         if (strlen(sw_desc) >= sizeof ods->sw_desc) {
-            VLOG_WARN("truncating sw_desc, must be less than %zu characters",
-                    sizeof ods->sw_desc);
+            VLOG_WARN("%s: truncating sw_desc, must be less than %zu bytes",
+                      p->name, sizeof ods->sw_desc);
         }
         free(p->sw_desc);
         p->sw_desc = xstrdup(sw_desc);
     }
     if (serial_desc) {
         if (strlen(serial_desc) >= sizeof ods->serial_num) {
-            VLOG_WARN("truncating serial_desc, must be less than %zu "
-                    "characters",
-                    sizeof ods->serial_num);
+            VLOG_WARN("%s: truncating serial_desc, must be less than %zu "
+                      "bytes", p->name, sizeof ods->serial_num);
         }
         free(p->serial_desc);
         p->serial_desc = xstrdup(serial_desc);
     }
     if (dp_desc) {
         if (strlen(dp_desc) >= sizeof ods->dp_desc) {
-            VLOG_WARN("truncating dp_desc, must be less than %zu characters",
-                    sizeof ods->dp_desc);
+            VLOG_WARN("%s: truncating dp_desc, must be less than %zu bytes",
+                      p->name, sizeof ods->dp_desc);
         }
         free(p->dp_desc);
         p->dp_desc = xstrdup(dp_desc);
@@ -1125,7 +1129,7 @@ ofproto_run(struct ofproto *p)
         s.length -= 2;
         ds_put_char(&s, ')');
 
-        VLOG_INFO("%s", ds_cstr(&s));
+        VLOG_INFO("%s: %s", p->name, ds_cstr(&s));
         ds_destroy(&s);
 
         p->n_add = p->n_delete = p->n_modify = 0;
@@ -1480,10 +1484,12 @@ reinit_ports(struct ofproto *p)
     sset_destroy(&devnames);
 }
 
-/* Opens and returns a netdev for 'ofproto_port', or a null pointer if the
- * netdev cannot be opened.  On success, also fills in 'opp'.  */
+/* Opens and returns a netdev for 'ofproto_port' in 'ofproto', or a null
+ * pointer if the netdev cannot be opened.  On success, also fills in
+ * 'opp'.  */
 static struct netdev *
-ofport_open(const struct ofproto_port *ofproto_port,
+ofport_open(const struct ofproto *ofproto,
+            const struct ofproto_port *ofproto_port,
             struct ofputil_phy_port *pp)
 {
     enum netdev_flags flags;
@@ -1492,8 +1498,9 @@ ofport_open(const struct ofproto_port *ofproto_port,
 
     error = netdev_open(ofproto_port->name, ofproto_port->type, &netdev);
     if (error) {
-        VLOG_WARN_RL(&rl, "ignoring port %s (%"PRIu16") because netdev %s "
+        VLOG_WARN_RL(&rl, "%s: ignoring port %s (%"PRIu16") because netdev %s "
                      "cannot be opened (%s)",
+                     ofproto->name,
                      ofproto_port->name, ofproto_port->ofp_port,
                      ofproto_port->name, strerror(error));
         return NULL;
@@ -1714,7 +1721,7 @@ update_port(struct ofproto *ofproto, const char *name)
 
     /* Fetch 'name''s location and properties from the datapath. */
     netdev = (!ofproto_port_query_by_name(ofproto, name, &ofproto_port)
-              ? ofport_open(&ofproto_port, &pp)
+              ? ofport_open(ofproto, &ofproto_port, &pp)
               : NULL);
     if (netdev) {
         port = ofproto_get_port(ofproto, ofproto_port.ofp_port);
@@ -1765,16 +1772,16 @@ init_ports(struct ofproto *p)
     OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, p) {
         uint16_t ofp_port = ofproto_port.ofp_port;
         if (ofproto_get_port(p, ofp_port)) {
-            VLOG_WARN_RL(&rl, "ignoring duplicate port %"PRIu16" in datapath",
-                         ofp_port);
+            VLOG_WARN_RL(&rl, "%s: ignoring duplicate port %"PRIu16" "
+                         "in datapath", p->name, ofp_port);
         } else if (shash_find(&p->port_by_name, ofproto_port.name)) {
-            VLOG_WARN_RL(&rl, "ignoring duplicate device %s in datapath",
-                         ofproto_port.name);
+            VLOG_WARN_RL(&rl, "%s: ignoring duplicate device %s in datapath",
+                         p->name, ofproto_port.name);
         } else {
             struct ofputil_phy_port pp;
             struct netdev *netdev;
 
-            netdev = ofport_open(&ofproto_port, &pp);
+            netdev = ofport_open(p, &ofproto_port, &pp);
             if (netdev) {
                 ofport_install(p, netdev, &pp);
             }
@@ -2290,7 +2297,7 @@ check_table_id(const struct ofproto *ofproto, uint8_t table_id)
 }
 
 static struct oftable *
-next_visible_table(struct ofproto *ofproto, uint8_t table_id)
+next_visible_table(const struct ofproto *ofproto, uint8_t table_id)
 {
     struct oftable *table;
 
@@ -2306,7 +2313,7 @@ next_visible_table(struct ofproto *ofproto, uint8_t table_id)
 }
 
 static struct oftable *
-first_matching_table(struct ofproto *ofproto, uint8_t table_id)
+first_matching_table(const struct ofproto *ofproto, uint8_t table_id)
 {
     if (table_id == 0xff) {
         return next_visible_table(ofproto, 0);
@@ -2318,8 +2325,8 @@ first_matching_table(struct ofproto *ofproto, uint8_t table_id)
 }
 
 static struct oftable *
-next_matching_table(struct ofproto *ofproto,
-                    struct oftable *table, uint8_t table_id)
+next_matching_table(const struct ofproto *ofproto,
+                    const struct oftable *table, uint8_t table_id)
 {
     return (table_id == 0xff
             ? next_visible_table(ofproto, (table - ofproto->tables) + 1)
@@ -2851,6 +2858,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
     } else if (victim && victim->pending) {
         error = OFPROTO_POSTPONE;
     } else {
+        struct ofoperation *op;
         struct rule *evict;
 
         if (classifier_count(&table->cls) > table->max_flows) {
@@ -2873,11 +2881,12 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
         }
 
         group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id);
-        ofoperation_create(group, rule, OFOPERATION_ADD);
-        rule->pending->victim = victim;
+        op = ofoperation_create(group, rule, OFOPERATION_ADD);
+        op->victim = victim;
 
         error = ofproto->ofproto_class->rule_construct(rule);
         if (error) {
+            op->group->n_running--;
             ofoperation_destroy(rule->pending);
         } else if (evict) {
             delete_flow__(evict, group);
@@ -2924,9 +2933,11 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
 
         if (!ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
                            rule->ofpacts, rule->ofpacts_len)) {
-            ofoperation_create(group, rule, OFOPERATION_MODIFY);
-            rule->pending->ofpacts = rule->ofpacts;
-            rule->pending->ofpacts_len = rule->ofpacts_len;
+            struct ofoperation *op;
+
+            op = ofoperation_create(group, rule, OFOPERATION_MODIFY);
+            op->ofpacts = rule->ofpacts;
+            op->ofpacts_len = rule->ofpacts_len;
             rule->ofpacts = xmemdup(fm->ofpacts, fm->ofpacts_len);
             rule->ofpacts_len = fm->ofpacts_len;
             rule->ofproto->ofproto_class->rule_modify_actions(rule);
@@ -3218,8 +3229,9 @@ handle_flow_mod__(struct ofproto *ofproto, struct ofconn *ofconn,
 
     default:
         if (fm->command > 0xff) {
-            VLOG_WARN_RL(&rl, "flow_mod has explicit table_id but "
-                         "flow_mod_table_id extension is not enabled");
+            VLOG_WARN_RL(&rl, "%s: flow_mod has explicit table_id but "
+                         "flow_mod_table_id extension is not enabled",
+                         ofproto->name);
         }
         return OFPERR_OFPFMFC_BAD_COMMAND;
     }
@@ -3559,8 +3571,8 @@ ofopgroup_create(struct ofproto *ofproto, struct ofconn *ofconn,
 static void
 ofopgroup_submit(struct ofopgroup *group)
 {
-    if (list_is_empty(&group->ops)) {
-        ofopgroup_destroy(group);
+    if (!group->n_running) {
+        ofopgroup_complete(group);
     } else {
         list_push_back(&group->ofproto->pending, &group->ofproto_node);
         group->ofproto->n_pending++;
@@ -3568,28 +3580,114 @@ ofopgroup_submit(struct ofopgroup *group)
 }
 
 static void
-ofopgroup_destroy(struct ofopgroup *group)
+ofopgroup_complete(struct ofopgroup *group)
 {
-    assert(list_is_empty(&group->ops));
+    struct ofproto *ofproto = group->ofproto;
+    struct ofoperation *op, *next_op;
+    int error;
+
+    assert(!group->n_running);
+
+    error = 0;
+    LIST_FOR_EACH (op, group_node, &group->ops) {
+        if (op->error) {
+            error = op->error;
+            break;
+        }
+    }
+
+    if (!error && group->ofconn && group->buffer_id != UINT32_MAX) {
+        LIST_FOR_EACH (op, group_node, &group->ops) {
+            if (op->type != OFOPERATION_DELETE) {
+                struct ofpbuf *packet;
+                uint16_t in_port;
+
+                error = ofconn_pktbuf_retrieve(group->ofconn, group->buffer_id,
+                                               &packet, &in_port);
+                if (packet) {
+                    assert(!error);
+                    error = rule_execute(op->rule, in_port, packet);
+                }
+                break;
+            }
+        }
+    }
+
+    LIST_FOR_EACH_SAFE (op, next_op, group_node, &group->ops) {
+        struct rule *rule = op->rule;
+        rule->pending = NULL;
+
+        switch (op->type) {
+        case OFOPERATION_ADD:
+            if (!op->error) {
+                ofproto_rule_destroy__(op->victim);
+                if ((rule->cr.wc.vlan_tci_mask & htons(VLAN_VID_MASK))
+                    == htons(VLAN_VID_MASK)) {
+                    if (ofproto->vlan_bitmap) {
+                        uint16_t vid = vlan_tci_to_vid(rule->cr.flow.vlan_tci);
+
+                        if (!bitmap_is_set(ofproto->vlan_bitmap, vid)) {
+                            bitmap_set1(ofproto->vlan_bitmap, vid);
+                            ofproto->vlans_changed = true;
+                        }
+                    } else {
+                        ofproto->vlans_changed = true;
+                    }
+                }
+            } else {
+                oftable_substitute_rule(rule, op->victim);
+                ofproto_rule_destroy__(rule);
+            }
+            break;
+
+        case OFOPERATION_DELETE:
+            assert(!op->error);
+            ofproto_rule_destroy__(rule);
+            op->rule = NULL;
+            break;
+
+        case OFOPERATION_MODIFY:
+            if (!op->error) {
+                rule->modified = time_msec();
+            } else {
+                rule->flow_cookie = op->flow_cookie;
+                free(rule->ofpacts);
+                rule->ofpacts = op->ofpacts;
+                rule->ofpacts_len = op->ofpacts_len;
+                op->ofpacts = NULL;
+                op->ofpacts_len = 0;
+            }
+            break;
+
+        default:
+            NOT_REACHED();
+        }
+
+        ofoperation_destroy(op);
+    }
+
     if (!list_is_empty(&group->ofproto_node)) {
-        assert(group->ofproto->n_pending > 0);
-        group->ofproto->n_pending--;
+        assert(ofproto->n_pending > 0);
+        ofproto->n_pending--;
         list_remove(&group->ofproto_node);
     }
     if (!list_is_empty(&group->ofconn_node)) {
         list_remove(&group->ofconn_node);
-        if (group->error) {
-            ofconn_send_error(group->ofconn, group->request, group->error);
+        if (error) {
+            ofconn_send_error(group->ofconn, group->request, error);
         }
-        connmgr_retry(group->ofproto->connmgr);
+        connmgr_retry(ofproto->connmgr);
     }
     free(group->request);
     free(group);
 }
 
 /* Initiates a new operation on 'rule', of the specified 'type', within
- * 'group'.  Prior to calling, 'rule' must not have any pending operation. */
-static void
+ * 'group'.  Prior to calling, 'rule' must not have any pending operation.
+ *
+ * Returns the newly created ofoperation (which is also available as
+ * rule->pending). */
+static struct ofoperation *
 ofoperation_create(struct ofopgroup *group, struct rule *rule,
                    enum ofoperation_type type)
 {
@@ -3605,10 +3703,14 @@ ofoperation_create(struct ofopgroup *group, struct rule *rule,
     op->type = type;
     op->flow_cookie = rule->flow_cookie;
 
+    group->n_running++;
+
     if (type == OFOPERATION_DELETE) {
         hmap_insert(&ofproto->deletions, &op->hmap_node,
                     cls_rule_hash(&rule->cr, rule->table_id));
     }
+
+    return op;
 }
 
 static void
@@ -3625,10 +3727,6 @@ ofoperation_destroy(struct ofoperation *op)
     list_remove(&op->group_node);
     free(op->ofpacts);
     free(op);
-
-    if (list_is_empty(&group->ops) && !list_is_empty(&group->ofproto_node)) {
-        ofopgroup_destroy(group);
-    }
 }
 
 /* Indicates that 'op' completed with status 'error', which is either 0 to
@@ -3664,76 +3762,15 @@ void
 ofoperation_complete(struct ofoperation *op, enum ofperr error)
 {
     struct ofopgroup *group = op->group;
-    struct rule *rule = op->rule;
-    struct ofproto *ofproto = rule->ofproto;
-
-    assert(rule->pending == op);
-
-    if (!error
-        && !group->error
-        && op->type != OFOPERATION_DELETE
-        && group->ofconn
-        && group->buffer_id != UINT32_MAX
-        && list_is_singleton(&op->group_node)) {
-        struct ofpbuf *packet;
-        uint16_t in_port;
-
-        error = ofconn_pktbuf_retrieve(group->ofconn, group->buffer_id,
-                                       &packet, &in_port);
-        if (packet) {
-            assert(!error);
-            error = rule_execute(rule, in_port, packet);
-        }
-    }
-    if (!group->error) {
-        group->error = error;
-    }
-
-    switch (op->type) {
-    case OFOPERATION_ADD:
-        if (!error) {
-            ofproto_rule_destroy__(op->victim);
-            if ((rule->cr.wc.vlan_tci_mask & htons(VLAN_VID_MASK))
-                == htons(VLAN_VID_MASK)) {
-                if (ofproto->vlan_bitmap) {
-                    uint16_t vid = vlan_tci_to_vid(rule->cr.flow.vlan_tci);
-
-                    if (!bitmap_is_set(ofproto->vlan_bitmap, vid)) {
-                        bitmap_set1(ofproto->vlan_bitmap, vid);
-                        ofproto->vlans_changed = true;
-                    }
-                } else {
-                    ofproto->vlans_changed = true;
-                }
-            }
-        } else {
-            oftable_substitute_rule(rule, op->victim);
-            ofproto_rule_destroy__(rule);
-            op->rule = NULL;
-        }
-        break;
 
-    case OFOPERATION_DELETE:
-        assert(!error);
-        ofproto_rule_destroy__(rule);
-        op->rule = NULL;
-        break;
+    assert(op->rule->pending == op);
+    assert(group->n_running > 0);
+    assert(!error || op->type != OFOPERATION_DELETE);
 
-    case OFOPERATION_MODIFY:
-        if (!error) {
-            rule->modified = time_msec();
-        } else {
-            free(rule->ofpacts);
-            rule->ofpacts = op->ofpacts;
-            rule->ofpacts_len = op->ofpacts_len;
-            op->ofpacts = NULL;
-        }
-        break;
-
-    default:
-        NOT_REACHED();
+    op->error = error;
+    if (!--group->n_running && !list_is_empty(&group->ofproto_node)) {
+        ofopgroup_complete(group);
     }
-    ofoperation_destroy(op);
 }
 
 struct rule *
@@ -3757,8 +3794,9 @@ pick_datapath_id(const struct ofproto *ofproto)
         if (!error) {
             return eth_addr_to_uint64(ea);
         }
-        VLOG_WARN("could not get MAC address for %s (%s)",
-                  netdev_get_name(port->netdev), strerror(error));
+        VLOG_WARN("%s: could not get MAC address for %s (%s)",
+                  ofproto->name, netdev_get_name(port->netdev),
+                  strerror(error));
     }
     return ofproto->fallback_dpid;
 }