From e640c1c497dca270356d1597fa0be12374721e1c Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 20 Aug 2010 10:01:34 -0700 Subject: [PATCH] Make flow table iteration functions propagate return values to caller. 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 | 57 ++++++++++++++++++++++++++++++++--------- lib/classifier.h | 10 ++++---- ofproto/ofproto.c | 26 ++++++++++++------- ofproto/wdp-provider.h | 10 +++++--- ofproto/wdp-xflow.c | 27 +++++++++++-------- ofproto/wdp.c | 9 +++++-- ofproto/wdp.h | 8 +++--- tests/test-classifier.c | 3 ++- 8 files changed, 104 insertions(+), 46 deletions(-) diff --git a/lib/classifier.c b/lib/classifier.c index 7661ca348..d0d21d541 100644 --- a/lib/classifier.c +++ b/lib/classifier.c @@ -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; } static struct cls_bucket *create_bucket(struct hmap *, size_t hash, diff --git a/lib/classifier.h b/lib/classifier.h index b515bf5b9..f4bd47723 100644 --- a/lib/classifier.h +++ b/lib/classifier.h @@ -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); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index cb0874a46..e6f4cbe5d 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -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 diff --git a/ofproto/wdp-provider.h b/ofproto/wdp-provider.h index 80110be72..fc86bd623 100644 --- a/ofproto/wdp-provider.h +++ b/ofproto/wdp-provider.h @@ -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, diff --git a/ofproto/wdp-xflow.c b/ofproto/wdp-xflow.c index b2208a476..494aab8de 100644 --- a/ofproto/wdp-xflow.c +++ b/ofproto/wdp-xflow.c @@ -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 diff --git a/ofproto/wdp.c b/ofproto/wdp.c index a5f3d3e17..5f8078b06 100644 --- a/ofproto/wdp.c +++ b/ofproto/wdp.c @@ -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 diff --git a/ofproto/wdp.h b/ofproto/wdp.h index 0342d7734..11ab028ca 100644 --- a/ofproto/wdp.h +++ b/ofproto/wdp.h @@ -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 *); diff --git a/tests/test-classifier.c b/tests/test-classifier.c index 27fe97e19..116e58940 100644 --- a/tests/test-classifier.c +++ b/tests/test-classifier.c @@ -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 -- 2.43.0