Make flow table iteration functions propagate return values to caller.
authorBen Pfaff <blp@nicira.com>
Fri, 20 Aug 2010 17:01:34 +0000 (10:01 -0700)
committerBen Pfaff <blp@nicira.com>
Fri, 20 Aug 2010 17:01:34 +0000 (10:01 -0700)
The following commit will propagate errors from OpenFlow flow table
operations to the OpenFlow controller.  To do so efficiently in cases where
the error occurs during a flow table iteration, it makes sense to stop
iterating as soon as an error occurs.  This commit makes this possible.

lib/classifier.c
lib/classifier.h
ofproto/ofproto.c
ofproto/wdp-provider.h
ofproto/wdp-xflow.c
ofproto/wdp.c
ofproto/wdp.h
tests/test-classifier.c

index 7661ca3..d0d21d5 100644 (file)
@@ -400,8 +400,12 @@ classifier_rule_overlaps(const struct classifier *cls, const flow_t *target)
  * it must not delete (or move) any other rules in 'cls' that are in the same
  * table as the argument rule.  Two rules are in the same table if their
  * cls_rule structs have the same table_idx; as a special case, a rule with
- * wildcards and an exact-match rule will never be in the same table. */
-void
+ * wildcards and an exact-match rule will never be in the same table.
+ *
+ * If 'callback' returns nonzero then the iteration stops immediately and
+ * classifier_for_each_match() passes up the return value.  Otherwise,
+ * classifier_for_each_match() returns 0 after completing the iteration. */
+int
 classifier_for_each_match(const struct classifier *cls,
                           const flow_t *target_flow,
                           int include, cls_cb_func *callback, void *aux)
@@ -433,13 +437,19 @@ classifier_for_each_match(const struct classifier *cls,
                                &bucket->rules) {
                     if (rules_match_1wild(rule, &target, 0)) {
                         if (prev_rule) {
-                            callback(prev_rule, aux);
+                            int retval = callback(prev_rule, aux);
+                            if (retval) {
+                                return retval;
+                            }
                         }
                         prev_rule = rule;
                     }
                 }
                 if (prev_rule) {
-                    callback(prev_rule, aux);
+                    int retval = callback(prev_rule, aux);
+                    if (retval) {
+                        return retval;
+                    }
                 }
             }
         }
@@ -452,7 +462,10 @@ classifier_for_each_match(const struct classifier *cls,
             HMAP_FOR_EACH_SAFE (rule, next_rule, struct cls_rule, node.hmap,
                                 &cls->exact_table) {
                 if (rules_match_1wild(rule, &target, 0)) {
-                    callback(rule, aux);
+                    int retval = callback(rule, aux);
+                    if (retval) {
+                        return retval;
+                    }
                 }
             }
         } else {
@@ -462,20 +475,29 @@ classifier_for_each_match(const struct classifier *cls,
             struct cls_rule *rule = search_exact_table(cls, hash,
                                                        &target.flow);
             if (rule) {
-                callback(rule, aux);
+                int retval = callback(rule, aux);
+                if (retval) {
+                    return retval;
+                }
             }
         }
     }
+
+    return 0;
 }
 
 /* 'callback' is allowed to delete the rule that is passed as its argument, but
  * it must not delete (or move) any other rules in 'cls' that are in the same
  * table as the argument rule.  Two rules are in the same table if their
  * cls_rule structs have the same table_idx; as a special case, a rule with
- * wildcards and an exact-match rule will never be in the same table. */
-void
+ * wildcards and an exact-match rule will never be in the same table.
+ *
+ * If 'callback' returns nonzero then the iteration stops immediately and
+ * classifier_for_each() passes up the return value.  Otherwise,
+ * classifier_for_each() returns 0 after completing the iteration. */
+int
 classifier_for_each(const struct classifier *cls, int include,
-                    void (*callback)(struct cls_rule *, void *aux),
+                    int (*callback)(struct cls_rule *, void *aux),
                     void *aux)
 {
     if (include & CLS_INC_WILD) {
@@ -496,12 +518,18 @@ classifier_for_each(const struct classifier *cls, int include,
                 LIST_FOR_EACH (rule, struct cls_rule, node.list,
                                &bucket->rules) {
                     if (prev_rule) {
-                        callback(prev_rule, aux);
+                        int retval = callback(prev_rule, aux);
+                        if (retval) {
+                            return retval;
+                        }
                     }
                     prev_rule = rule;
                 }
                 if (prev_rule) {
-                    callback(prev_rule, aux);
+                    int retval = callback(prev_rule, aux);
+                    if (retval) {
+                        return retval;
+                    }
                 }
             }
         }
@@ -512,9 +540,14 @@ classifier_for_each(const struct classifier *cls, int include,
 
         HMAP_FOR_EACH_SAFE (rule, next_rule,
                             struct cls_rule, node.hmap, &cls->exact_table) {
-            callback(rule, aux);
+            int retval = callback(rule, aux);
+            if (retval) {
+                return retval;
+            }
         }
     }
+
+    return 0;
 }
 \f
 static struct cls_bucket *create_bucket(struct hmap *, size_t hash,
index b515bf5..f4bd477 100644 (file)
@@ -153,17 +153,17 @@ struct cls_rule *classifier_lookup_exact(const struct classifier *,
                                          const flow_t *);
 bool classifier_rule_overlaps(const struct classifier *, const flow_t *);
 
-typedef void cls_cb_func(struct cls_rule *, void *aux);
+typedef int cls_cb_func(struct cls_rule *, void *aux);
 
 enum {
     CLS_INC_EXACT = 1 << 0,     /* Include exact-match flows? */
     CLS_INC_WILD = 1 << 1,      /* Include flows with wildcards? */
     CLS_INC_ALL = CLS_INC_EXACT | CLS_INC_WILD
 };
-void classifier_for_each(const struct classifier *, int include,
-                         cls_cb_func *, void *aux);
-void classifier_for_each_match(const struct classifier *, const flow_t *,
-                               int include, cls_cb_func *, void *aux);
+int classifier_for_each(const struct classifier *, int include,
+                        cls_cb_func *, void *aux);
+int classifier_for_each_match(const struct classifier *, const flow_t *,
+                              int include, cls_cb_func *, void *aux);
 struct cls_rule *classifier_find_rule_exactly(const struct classifier *,
                                               const flow_t *target);
 
index cb0874a..e6f4cbe 100644 (file)
@@ -1779,7 +1779,7 @@ query_stats(struct ofproto *p, struct wdp_rule *rule,
     }
 }
 
-static void
+static int
 flow_stats_cb(struct wdp_rule *rule, void *cbdata_)
 {
     struct flow_stats_cbdata *cbdata = cbdata_;
@@ -1792,7 +1792,7 @@ flow_stats_cb(struct wdp_rule *rule, void *cbdata_)
 
     if (rule_is_hidden(rule)
         || !rule_has_out_port(rule, cbdata->out_port)) {
-        return;
+        return 0;
     }
 
     act_len = sizeof *rule->actions * rule->n_actions;
@@ -1816,6 +1816,8 @@ flow_stats_cb(struct wdp_rule *rule, void *cbdata_)
     ofs->packet_count = htonll(packet_count);
     ofs->byte_count = htonll(byte_count);
     memcpy(ofs->actions, rule->actions, act_len);
+
+    return 0;
 }
 
 static unsigned int
@@ -1864,7 +1866,7 @@ struct flow_stats_ds_cbdata {
     struct ds *results;
 };
 
-static void
+static int
 flow_stats_ds_cb(struct wdp_rule *rule, void *cbdata_)
 {
     struct flow_stats_ds_cbdata *cbdata = cbdata_;
@@ -1885,6 +1887,8 @@ flow_stats_ds_cb(struct wdp_rule *rule, void *cbdata_)
     ofp_print_match(results, &match, true);
     ofp_print_actions(results, &rule->actions->header, act_len);
     ds_put_cstr(results, "\n");
+
+    return 0;
 }
 
 /* Adds a pretty-printed description of all flows to 'results', including 
@@ -1915,14 +1919,14 @@ struct aggregate_stats_cbdata {
     uint32_t n_flows;
 };
 
-static void
+static int
 aggregate_stats_cb(struct wdp_rule *rule, void *cbdata_)
 {
     struct aggregate_stats_cbdata *cbdata = cbdata_;
     uint64_t packet_count, byte_count;
 
     if (rule_is_hidden(rule) || !rule_has_out_port(rule, cbdata->out_port)) {
-        return;
+        return 0;
     }
 
     query_stats(cbdata->ofproto, rule, &packet_count, &byte_count);
@@ -1930,6 +1934,8 @@ aggregate_stats_cb(struct wdp_rule *rule, void *cbdata_)
     cbdata->packet_count += packet_count;
     cbdata->byte_count += byte_count;
     cbdata->n_flows++;
+
+    return 0;
 }
 
 static int
@@ -2210,7 +2216,7 @@ struct modify_flows_cbdata {
 
 static int modify_flow(struct ofproto *, const struct ofp_flow_mod *,
                        size_t n_actions, struct wdp_rule *);
-static void modify_flows_cb(struct wdp_rule *, void *cbdata_);
+static int modify_flows_cb(struct wdp_rule *, void *cbdata_);
 
 /* Implements OFPFC_ADD and OFPFC_MODIFY.  Returns 0 on success or an OpenFlow
  * error code as encoded by ofp_mkerr() on failure.
@@ -2266,7 +2272,7 @@ modify_flow_strict(struct ofproto *p, struct ofconn *ofconn,
 }
 
 /* Callback for modify_flows_loose(). */
-static void
+static int
 modify_flows_cb(struct wdp_rule *rule, void *cbdata_)
 {
     struct modify_flows_cbdata *cbdata = cbdata_;
@@ -2275,6 +2281,7 @@ modify_flows_cb(struct wdp_rule *rule, void *cbdata_)
         cbdata->match = rule;
         modify_flow(cbdata->ofproto, cbdata->ofm, cbdata->n_actions, rule);
     }
+    return 0;
 }
 
 /* Implements core of OFPFC_MODIFY and OFPFC_MODIFY_STRICT where 'rule' has
@@ -2314,7 +2321,7 @@ struct delete_flows_cbdata {
     uint16_t out_port;
 };
 
-static void delete_flows_cb(struct wdp_rule *, void *cbdata_);
+static int delete_flows_cb(struct wdp_rule *, void *cbdata_);
 static void delete_flow_core(struct ofproto *, struct wdp_rule *,
                              uint16_t out_port);
 
@@ -2350,12 +2357,13 @@ delete_flow_strict(struct ofproto *p, const struct ofconn *ofconn,
 }
 
 /* Callback for delete_flows_loose(). */
-static void
+static int
 delete_flows_cb(struct wdp_rule *rule, void *cbdata_)
 {
     struct delete_flows_cbdata *cbdata = cbdata_;
 
     delete_flow_core(cbdata->ofproto, rule, cbdata->out_port);
+    return 0;
 }
 
 /* Implements core of OFPFC_DELETE and OFPFC_DELETE_STRICT where 'rule' has
index 80110be..fc86bd6 100644 (file)
@@ -273,6 +273,10 @@ struct wdp_class {
      * flow that matches the specified search criteria to 'callback' along with
      * 'aux'.
      *
+     * If 'callback' returns nonzero, then this must stop the iteration and the
+     * return value must be propagated to the caller.  Otherwise, this function
+     * must return zero after all matching flows (if any) have been visited.
+     *
      * Flows are filtered out in two ways.  First, based on the bit-mask in
      * 'include': wdp_rule 'wr' is included only if 'include & (1u <<
      * wr->ofp_table_id)' is nonzero.
@@ -288,9 +292,9 @@ struct wdp_class {
      * It may modify any flow in 'wdp', e.g. changing their actions.
      * 'callback' must not delete flows from 'wdp' other than its argument
      * flow, nor may it insert new flows into 'wdp'. */
-    void (*flow_for_each_match)(const struct wdp *wdp, const flow_t *flow,
-                                unsigned int include,
-                                wdp_flow_cb_func *callback, void *aux);
+    int (*flow_for_each_match)(const struct wdp *wdp, const flow_t *flow,
+                               unsigned int include,
+                               wdp_flow_cb_func *callback, void *aux);
 
     /* Retrieves flow statistics for 'rule', which must be in 'wdp''s flow
      * table, and stores them into '*stats'.  Returns 0 if successful,
index b2208a4..494aab8 100644 (file)
@@ -1065,7 +1065,7 @@ uninstall_idle_flow(struct wx *wx, struct wx_rule *rule)
     }
 }
 
-static void
+static int
 expire_rule(struct cls_rule *cls_rule, void *wx_)
 {
     struct wx *wx = wx_;
@@ -1089,7 +1089,7 @@ expire_rule(struct cls_rule *cls_rule, void *wx_)
             //XXX active_timeout(wx, rule);
         }
 
-        return;
+        return 0;
     }
 
     COVERAGE_INC(wx_expired);
@@ -1113,6 +1113,8 @@ expire_rule(struct cls_rule *cls_rule, void *wx_)
     }
 #endif
     wx_rule_remove(wx, rule);
+
+    return 0;
 }
 
 struct revalidate_cbdata {
@@ -1150,7 +1152,7 @@ revalidate_rule(struct wx *wx, struct wx_rule *rule)
     return true;
 }
 
-static void
+static int
 revalidate_cb(struct cls_rule *sub_, void *cbdata_)
 {
     struct wx_rule *sub = wx_rule_cast(sub_);
@@ -1161,6 +1163,7 @@ revalidate_cb(struct cls_rule *sub_, void *cbdata_)
         || tag_set_intersects(&cbdata->revalidate_set, sub->tags)) {
         revalidate_rule(cbdata->wx, sub);
     }
+    return 0;
 }
 
 static void
@@ -1332,7 +1335,7 @@ wx_get_features(const struct wdp *wdp, struct ofpbuf **featuresp)
     return 0;
 }
 
-static void
+static int
 count_subrules(struct cls_rule *cls_rule, void *n_subrules_)
 {
     struct wx_rule *rule = wx_rule_cast(cls_rule);
@@ -1341,6 +1344,7 @@ count_subrules(struct cls_rule *cls_rule, void *n_subrules_)
     if (rule->super) {
         (*n_subrules)++;
     }
+    return 0;
 }
 
 static int
@@ -1593,18 +1597,19 @@ struct wx_for_each_thunk_aux {
     void *client_aux;
 };
 
-static void
+static int
 wx_for_each_thunk(struct cls_rule *cls_rule, void *aux_)
 {
     struct wx_for_each_thunk_aux *aux = aux_;
     struct wx_rule *rule = wx_rule_cast(cls_rule);
 
     if (!wx_rule_is_hidden(rule)) {
-        aux->client_callback(&rule->wr, aux->client_aux);
+        return aux->client_callback(&rule->wr, aux->client_aux);
     }
+    return 0;
 }
 
-static void
+static int
 wx_flow_for_each_match(const struct wdp *wdp, const flow_t *target,
                        unsigned int include,
                        wdp_flow_cb_func *client_callback, void *client_aux)
@@ -1623,8 +1628,8 @@ wx_flow_for_each_match(const struct wdp *wdp, const flow_t *target,
 
     aux.client_callback = client_callback;
     aux.client_aux = client_aux;
-    classifier_for_each_match(&wx->cls, target, cls_include,
-                              wx_for_each_thunk, &aux);
+    return classifier_for_each_match(&wx->cls, target, cls_include,
+                                     wx_for_each_thunk, &aux);
 }
 
 /* Obtains statistic counters for 'rule' within 'wx' and stores them into
@@ -1772,7 +1777,7 @@ wx_flow_delete(struct wdp *wdp, struct wdp_rule *wdp_rule,
     return 0;
 }
 
-static void
+static int
 wx_flush_rule(struct cls_rule *cls_rule, void *wx_)
 {
     struct wx_rule *rule = wx_rule_cast(cls_rule);
@@ -1785,6 +1790,8 @@ wx_flush_rule(struct cls_rule *cls_rule, void *wx_)
     rule->installed = false;
 
     wx_rule_remove(wx, rule);
+
+    return 0;
 }
 
 static int
index a5f3d3e..5f8078b 100644 (file)
@@ -802,6 +802,10 @@ wdp_flow_match(struct wdp *wdp, const flow_t *flow)
 
 /* Iterates through all of the flows in 'wdp''s flow table, passing each flow
  * that matches the specified search criteria to 'callback' along with 'aux'.
+ * If 'callback' returns nonzero, then this stops the iteration and
+ * wdp_flow_for_each_match() passes the return value along.  Otherwise
+ * wdp_flow_for_each_match() returns zero after all matching flows have been
+ * visited.
  *
  * Flows are filtered out in two ways.  First, based on the bit-mask in
  * 'include': wdp_rule 'wr' is included only if 'include & (1u <<
@@ -818,12 +822,13 @@ wdp_flow_match(struct wdp *wdp, const flow_t *flow)
  * may modify any flow in 'wdp', e.g. changing their actions.  'callback' must
  * not delete flows from 'wdp' other than its argument flow, nor may it insert
  * new flows into 'wdp'. */
-void
+int
 wdp_flow_for_each_match(const struct wdp *wdp, const flow_t *target,
                         unsigned int include,
                         wdp_flow_cb_func *callback, void *aux)
 {
-    wdp->wdp_class->flow_for_each_match(wdp, target, include, callback, aux);
+    return wdp->wdp_class->flow_for_each_match(wdp, target, include,
+                                               callback, aux);
 }
 
 int
index 0342d77..11ab028 100644 (file)
@@ -145,10 +145,10 @@ struct wdp_rule *wdp_flow_get(struct wdp *, const flow_t *,
                               unsigned int include);
 struct wdp_rule *wdp_flow_match(struct wdp *, const flow_t *);
 
-typedef void wdp_flow_cb_func(struct wdp_rule *, void *aux);
-void wdp_flow_for_each_match(const struct wdp *, const flow_t *,
-                             unsigned int include, wdp_flow_cb_func *,
-                             void *aux);
+typedef int wdp_flow_cb_func(struct wdp_rule *, void *aux);
+int wdp_flow_for_each_match(const struct wdp *, const flow_t *,
+                            unsigned int include, wdp_flow_cb_func *,
+                            void *aux);
 
 int wdp_flow_get_stats(const struct wdp *, const struct wdp_rule *,
                          struct wdp_flow_stats *);
index 27fe97e..116e589 100644 (file)
@@ -406,11 +406,12 @@ compare_classifiers(struct classifier *cls, struct tcls *tcls)
     }
 }
 
-static void
+static int
 free_rule(struct cls_rule *cls_rule, void *cls)
 {
     classifier_remove(cls, cls_rule);
     free(test_rule_from_cls_rule(cls_rule));
+    return 0;
 }
 
 static void