From c84d8691141fa698100480043ace76c553fff280 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Fri, 29 May 2015 11:28:39 -0700 Subject: [PATCH] ofproto: Split add_flow(). Split add_flow() to add_flow_begin() which does all the error checking, and add_flow_finish() which can not fail. Since we still want to send an error response for an unknown 'buffer_id', send_buffered_packet() now send the error response itself. Signed-off-by: Jarno Rajahalme Acked-by: Ben Pfaff --- ofproto/ofproto.c | 211 +++++++++++++++++++++++++++------------------- 1 file changed, 122 insertions(+), 89 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 3eca3b04a..0eda3f086 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -249,7 +249,7 @@ static void ofproto_rule_remove__(struct ofproto *, struct rule *) * meaningful and thus supplied as NULL. */ struct flow_mod_requester { struct ofconn *ofconn; /* Connection on which flow_mod arrived. */ - ovs_be32 xid; /* OpenFlow xid of flow_mod request. */ + const struct ofp_header *request; }; /* OpenFlow. */ @@ -269,8 +269,8 @@ static void delete_flows__(const struct rule_collection *, const struct flow_mod_requester *) OVS_REQUIRES(ofproto_mutex); -static enum ofperr send_buffered_packet(struct ofconn *, uint32_t buffer_id, - struct rule *) +static void send_buffered_packet(const struct flow_mod_requester *, + uint32_t buffer_id, struct rule *) OVS_REQUIRES(ofproto_mutex); static bool ofproto_group_exists__(const struct ofproto *ofproto, @@ -4335,20 +4335,9 @@ set_conjunctions(struct rule *rule, const struct cls_conjunction *conjs, cls_rule_set_conjunctions(cr, conjs, n_conjs); } -/* Implements OFPFC_ADD and the cases for OFPFC_MODIFY and OFPFC_MODIFY_STRICT - * 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 on failure. - * - * The caller retains ownership of 'fm->ofpacts'. - * - * 'ofconn' is used to retrieve the packet buffer specified in ofm->buffer_id, - * if any. */ static enum ofperr -add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm, - const struct flow_mod_requester *req) +add_flow_begin(struct ofproto *ofproto, struct ofputil_flow_mod *fm, + struct rule **rulep, bool *modify) OVS_REQUIRES(ofproto_mutex) { struct oftable *table; @@ -4387,93 +4376,139 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm, return OFPERR_OFPBRC_EPERM; } - if (!(fm->flags & OFPUTIL_FF_HIDDEN_FIELDS)) { - if (!match_has_default_hidden_fields(&fm->match)) { - VLOG_WARN_RL(&rl, "%s: (add_flow) only internal flows can set " - "non-default values to hidden fields", ofproto->name); - return OFPERR_OFPBRC_EPERM; - } + if (!(fm->flags & OFPUTIL_FF_HIDDEN_FIELDS) + && !match_has_default_hidden_fields(&fm->match)) { + VLOG_WARN_RL(&rl, "%s: (add_flow) only internal flows can set " + "non-default values to hidden fields", ofproto->name); + return OFPERR_OFPBRC_EPERM; } cls_rule_init(&cr, &fm->match, fm->priority); - /* Transform "add" into "modify" if there's an existing identical flow. */ + /* Check for the existence of an identical rule. */ rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls, &cr)); if (rule) { + /* Transform "add" into "modify" of an existing identical flow. */ cls_rule_destroy(&cr); fm->modify_cookie = true; error = modify_flow_check__(ofproto, fm, rule); - if (!error) { - struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies); + if (error) { + return error; + } - modify_flow__(ofproto, fm, rule, req, &dead_cookies); - learned_cookies_flush(ofproto, &dead_cookies); + *modify = true; + } else { /* New rule. */ + struct cls_conjunction *conjs; + size_t n_conjs; - goto send_packet; + /* Check for overlap, if requested. */ + if (fm->flags & OFPUTIL_FF_CHECK_OVERLAP + && classifier_rule_overlaps(&table->cls, &cr)) { + cls_rule_destroy(&cr); + return OFPERR_OFPFMFC_OVERLAP; } - return error; - } - /* Check for overlap, if requested. */ - if (fm->flags & OFPUTIL_FF_CHECK_OVERLAP) { - if (classifier_rule_overlaps(&table->cls, &cr)) { + /* If necessary, evict an existing rule to clear out space. */ + error = evict_rules_from_table(table, 1); + if (error) { cls_rule_destroy(&cr); - return OFPERR_OFPFMFC_OVERLAP; + return error; } - } - /* If necessary, evict an existing rule to clear out space. */ - error = evict_rules_from_table(table, 1); - if (error) { - cls_rule_destroy(&cr); - return error; - } + /* Allocate new rule. */ + error = ofproto_rule_create(ofproto, fm, &cr, table - ofproto->tables, + &rule); + if (error) { + return error; + } - /* Allocate new rule. */ - error = ofproto_rule_create(ofproto, fm, &cr, table - ofproto->tables, - &rule); - if (error) { - return error; + /* Insert flow to the classifier, so that later flow_mods may relate + * to it. This is reversible, in case later errors require this to + * be reverted. */ + ofproto_rule_insert__(ofproto, rule); + /* Make the new rule invisible for classifier lookups. */ + classifier_defer(&table->cls); + get_conjunctions(fm, &conjs, &n_conjs); + classifier_insert(&table->cls, &rule->cr, conjs, n_conjs); + free(conjs); + + error = ofproto->ofproto_class->rule_insert(rule); + if (error) { + oftable_remove_rule(rule); + ofproto_rule_unref(rule); + return error; + } + + *modify = false; } - ofproto_rule_insert__(ofproto, rule); + *rulep = rule; + return 0; +} - classifier_defer(&table->cls); +static void +add_flow_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm, + const struct flow_mod_requester *req, + struct rule *rule, bool modify) + OVS_REQUIRES(ofproto_mutex) +{ + if (modify) { + struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies); - struct cls_conjunction *conjs; - size_t n_conjs; - get_conjunctions(fm, &conjs, &n_conjs); - classifier_insert(&table->cls, &rule->cr, conjs, n_conjs); - free(conjs); + modify_flow__(ofproto, fm, rule, req, &dead_cookies); + learned_cookies_flush(ofproto, &dead_cookies); + } else { + struct oftable *table = &ofproto->tables[rule->table_id]; - error = ofproto->ofproto_class->rule_insert(rule); - if (error) { - oftable_remove_rule(rule); - ofproto_rule_unref(rule); - return error; - } - cls_rule_make_visible(&rule->cr); - classifier_publish(&table->cls); + cls_rule_make_visible(&rule->cr); + classifier_publish(&table->cls); - learned_cookies_inc(ofproto, rule_get_actions(rule)); + learned_cookies_inc(ofproto, rule_get_actions(rule)); - if (minimask_get_vid_mask(&rule->cr.match.mask) == VLAN_VID_MASK) { - if (ofproto->vlan_bitmap) { - uint16_t vid = miniflow_get_vid(&rule->cr.match.flow); - if (!bitmap_is_set(ofproto->vlan_bitmap, vid)) { - bitmap_set1(ofproto->vlan_bitmap, vid); + if (minimask_get_vid_mask(&rule->cr.match.mask) == VLAN_VID_MASK) { + if (ofproto->vlan_bitmap) { + uint16_t vid = miniflow_get_vid(&rule->cr.match.flow); + + if (!bitmap_is_set(ofproto->vlan_bitmap, vid)) { + bitmap_set1(ofproto->vlan_bitmap, vid); + ofproto->vlans_changed = true; + } + } else { ofproto->vlans_changed = true; } - } else { - ofproto->vlans_changed = true; } + + ofmonitor_report(ofproto->connmgr, rule, NXFME_ADDED, 0, + req ? req->ofconn : NULL, + req ? req->request->xid : 0, NULL); } - ofmonitor_report(ofproto->connmgr, rule, NXFME_ADDED, 0, - req ? req->ofconn : NULL, req ? req->xid : 0, NULL); -send_packet: - return req ? send_buffered_packet(req->ofconn, fm->buffer_id, rule) : 0; + send_buffered_packet(req, fm->buffer_id, rule); +} + +/* Implements OFPFC_ADD and the cases for OFPFC_MODIFY and OFPFC_MODIFY_STRICT + * in which no matching flow already exists in the flow table. + * + * Adds the flow specified by 'fm', to the ofproto's flow table. Returns 0 on + * success, or an OpenFlow error code on failure. + * + * The caller retains ownership of 'fm->ofpacts'. */ +static enum ofperr +add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm, + const struct flow_mod_requester *req) + OVS_REQUIRES(ofproto_mutex) +{ + struct rule *rule; + bool modify; + enum ofperr error; + + error = add_flow_begin(ofproto, fm, &rule, &modify); + if (!error) { + add_flow_finish(ofproto, fm, req, rule, modify); + } + + return error; } /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */ @@ -4618,7 +4653,7 @@ modify_flow__(struct ofproto *ofproto, struct ofputil_flow_mod *fm, if (event != NXFME_MODIFIED || change_actions || change_cookie) { ofmonitor_report(ofproto->connmgr, rule, event, 0, - req ? req->ofconn : NULL, req ? req->xid : 0, + req ? req->ofconn : NULL, req ? req->request->xid : 0, change_actions ? actions : NULL); } @@ -4672,11 +4707,7 @@ modify_flows(struct ofproto *ofproto, struct ofputil_flow_mod *fm, error = modify_flows_check__(ofproto, fm, rules); if (!error) { modify_flows__(ofproto, fm, rules, req); - - if (fm->buffer_id != UINT32_MAX && req) { - error = send_buffered_packet(req->ofconn, fm->buffer_id, - rules->rules[0]); - } + send_buffered_packet(req, fm->buffer_id, rules->rules[0]); } } else { error = modify_flows_add__(ofproto, fm, req); @@ -4765,8 +4796,8 @@ delete_flows__(const struct rule_collection *rules, ofproto_rule_send_removed(rule, reason); ofmonitor_report(ofproto->connmgr, rule, NXFME_DELETED, reason, - req ? req->ofconn : NULL, req ? req->xid : 0, - NULL); + req ? req->ofconn : NULL, + req ? req->request->xid : 0, NULL); if (next_table == rule->table_id) { classifier_defer(cls); @@ -4952,7 +4983,7 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh) struct flow_mod_requester req; req.ofconn = ofconn; - req.xid = oh->xid; + req.request = oh; error = handle_flow_mod__(ofproto, &fm, &req); } if (error) { @@ -6668,18 +6699,19 @@ handle_openflow(struct ofconn *ofconn, const struct ofpbuf *ofp_msg) /* Asynchronous operations. */ -static enum ofperr -send_buffered_packet(struct ofconn *ofconn, uint32_t buffer_id, +static void +send_buffered_packet(const struct flow_mod_requester *req, uint32_t buffer_id, struct rule *rule) OVS_REQUIRES(ofproto_mutex) { - enum ofperr error = 0; - if (ofconn && buffer_id != UINT32_MAX) { - struct ofproto *ofproto = ofconn_get_ofproto(ofconn); + if (req && req->ofconn && buffer_id != UINT32_MAX) { + struct ofproto *ofproto = ofconn_get_ofproto(req->ofconn); struct dp_packet *packet; ofp_port_t in_port; + enum ofperr error; - error = ofconn_pktbuf_retrieve(ofconn, buffer_id, &packet, &in_port); + error = ofconn_pktbuf_retrieve(req->ofconn, buffer_id, &packet, + &in_port); if (packet) { struct rule_execute *re; @@ -6696,9 +6728,10 @@ send_buffered_packet(struct ofconn *ofconn, uint32_t buffer_id, dp_packet_delete(re->packet); free(re); } + } else { + ofconn_send_error(req->ofconn, req->request, error); } } - return error; } static uint64_t -- 2.39.5