[ovs-dev] [PATCH] ofproto: Return error codes for Rule insertions
Aravind Prasad
raja.avi at gmail.com
Thu Jul 12 18:14:16 UTC 2018
Hi Aaron,
Thanks a lot for the review.
Submitted the patch with comments addressed as below.
> /doesnot/doesn't/ or /doesnot/does not/
> Or maybe even just remove that sentence.
> :)
Addressed. Changed to "does not"
> This second if looks strange. I think it should be removed.
Addressed.
> I don't know if there are any implications to passing 0 for an ofperr.
> I don't see an issue in this patch, but I'm not sure if any callers in
> the future may be affected. There was previously a 0 value used for
> OFPBIC_BAD_EXP_TYPE - I don't even know if it would matter now. Maybe
> it's better to have these return an integer type and then it might make
> the interface clearer. WDYT?
As far as i had walk thro'd the code, error=0 is considered the
success value and non-zero values as failure. Hence, the same was
followed.
Also, I feel your suggestion for using a integer type for success is
valid but has to be handled as a separate cleanup patch which fixes
this behavior across the entire codebase.
Thanks,
Aravind Prasad S
On Thu, Jul 12, 2018 at 8:22 PM, Aaron Conole <aconole at bytheb.org> wrote:
> Aravind Prasad S <raja.avi at gmail.com> writes:
>
> > Currently, rule_insert() API doesnot have return value. There are some
> possible
>
> /doesnot/doesn't/ or /doesnot/does not/
>
> Or maybe even just remove that sentence.
>
> :)
>
> > scenarios where rule insertions can fail at run-time even though the
> static
> > checks during rule_construct() had passed previously.
> > Some possible scenarios for failure of rule insertions:
> > **) Rule insertions can fail dynamically in Hybrid mode (both Openflow
> and
> > Normal switch functioning coexist) where the CAM space could get suddenly
> > filled up by Normal switch functioning and Openflow gets devoid of
> > available space.
> > **) Some deployments could have separate independent layers for HW rule
> > insertions and application layer to interact with OVS. HW layer
> > could face any dynamic issue during rule handling which application could
> > not have predicted/captured in rule-construction phase.
> > Rule-insert errors for bundles are also handled in this patch.
> >
> > Signed-off-by: Aravind Prasad S <raja.avi at gmail.com>
> > ---
> > ofproto/ofproto-dpif.c | 4 +-
> > ofproto/ofproto-provider.h | 6 +--
> > ofproto/ofproto.c | 100 ++++++++++++++++++++++++++++++
> +++++----------
> > 3 files changed, 84 insertions(+), 26 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index ad1e8af..d1678ed 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -4443,7 +4443,7 @@ rule_construct(struct rule *rule_)
> > return 0;
> > }
> >
> > -static void
> > +static enum ofperr
> > rule_insert(struct rule *rule_, struct rule *old_rule_, bool
> forward_counts)
> > OVS_REQUIRES(ofproto_mutex)
> > {
> > @@ -4473,6 +4473,8 @@ rule_insert(struct rule *rule_, struct rule
> *old_rule_, bool forward_counts)
> > ovs_mutex_unlock(&rule->stats_mutex);
> > ovs_mutex_unlock(&old_rule->stats_mutex);
> > }
> > +
> > + return 0;
> > }
> >
> > static void
> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index 2b77b89..3f3d110 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -1297,8 +1297,8 @@ struct ofproto_class {
> > struct rule *(*rule_alloc)(void);
> > enum ofperr (*rule_construct)(struct rule *rule)
> > /* OVS_REQUIRES(ofproto_mutex) */;
> > - void (*rule_insert)(struct rule *rule, struct rule *old_rule,
> > - bool forward_counts)
> > + enum ofperr (*rule_insert)(struct rule *rule, struct rule *old_rule,
> > + bool forward_counts)
> > /* OVS_REQUIRES(ofproto_mutex) */;
> > void (*rule_delete)(struct rule *rule) /*
> OVS_REQUIRES(ofproto_mutex) */;
> > void (*rule_destruct)(struct rule *rule);
> > @@ -1952,7 +1952,7 @@ enum ofperr ofproto_flow_mod_learn_start(struct
> ofproto_flow_mod *ofm)
> > OVS_REQUIRES(ofproto_mutex);
> > void ofproto_flow_mod_learn_revert(struct ofproto_flow_mod *ofm)
> > OVS_REQUIRES(ofproto_mutex);
> > -void ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm,
> > +enum ofperr ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm,
> > struct ofproto *orig_ofproto)
> > OVS_REQUIRES(ofproto_mutex);
> > void ofproto_add_flow(struct ofproto *, const struct match *, int
> priority,
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index f946e27..03597dc 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -245,10 +245,12 @@ static void replace_rule_revert(struct ofproto *,
> struct rule *old_rule,
> > struct rule *new_rule)
> > OVS_REQUIRES(ofproto_mutex);
> >
> > -static void replace_rule_finish(struct ofproto *, struct
> ofproto_flow_mod *,
> > - const struct openflow_mod_requester *,
> > - struct rule *old_rule, struct rule
> *new_rule,
> > - struct ovs_list *dead_cookies)
> > +static enum ofperr replace_rule_finish(struct ofproto *,
> > + struct ofproto_flow_mod *,
> > + const struct
> openflow_mod_requester *,
> > + struct rule *old_rule,
> > + struct rule *new_rule,
> > + struct ovs_list *dead_cookies)
> > OVS_REQUIRES(ofproto_mutex);
> > static void delete_flows__(struct rule_collection *,
> > enum ofp_flow_removed_reason,
> > @@ -270,7 +272,7 @@ static enum ofperr ofproto_flow_mod_start(struct
> ofproto *,
> > static void ofproto_flow_mod_revert(struct ofproto *,
> > struct ofproto_flow_mod *)
> > OVS_REQUIRES(ofproto_mutex);
> > -static void ofproto_flow_mod_finish(struct ofproto *,
> > +static enum ofperr ofproto_flow_mod_finish(struct ofproto *,
> > struct ofproto_flow_mod *,
> > const struct openflow_mod_requester
> *)
> > OVS_REQUIRES(ofproto_mutex);
> > @@ -4855,7 +4857,7 @@ add_flow_revert(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm)
> > }
> >
> > /* To be called after version bump. */
> > -static void
> > +static enum ofperr
> > add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
> > const struct openflow_mod_requester *req)
> > OVS_REQUIRES(ofproto_mutex)
> > @@ -4864,8 +4866,14 @@ add_flow_finish(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> > ? rule_collection_rules(&ofm->old_rules)[0] : NULL;
> > struct rule *new_rule = rule_collection_rules(&ofm->new_rules)[0];
> > struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
> > + enum ofperr error = 0;
> > +
> > + error = replace_rule_finish(ofproto, ofm, req, old_rule, new_rule,
> > + &dead_cookies);
> > + if (error) {
> > + return error;
> > + }
> >
> > - replace_rule_finish(ofproto, ofm, req, old_rule, new_rule,
> &dead_cookies);
> > learned_cookies_flush(ofproto, &dead_cookies);
> >
> > if (old_rule) {
> > @@ -4878,6 +4886,8 @@ add_flow_finish(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> > /* Send Vacancy Events for OF1.4+. */
> > send_table_status(ofproto, new_rule->table_id);
> > }
> > +
> > + return error;
> > }
> >
> > /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
> > @@ -5074,22 +5084,25 @@ ofproto_flow_mod_learn_revert(struct
> ofproto_flow_mod *ofm)
> > ofproto_flow_mod_revert(rule->ofproto, ofm);
> > }
> >
> > -void
> > +enum ofperr
> > ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm,
> > struct ofproto *orig_ofproto)
> > OVS_REQUIRES(ofproto_mutex)
> > {
> > struct rule *rule = rule_collection_rules(&ofm->new_rules)[0];
> > + enum ofperr error = 0;
> >
> > /* If learning on a different bridge, must bump its version
> > * number and flush connmgr afterwards. */
> > if (rule->ofproto != orig_ofproto) {
> > ofproto_bump_tables_version(rule->ofproto);
> > }
> > - ofproto_flow_mod_finish(rule->ofproto, ofm, NULL);
> > + error = ofproto_flow_mod_finish(rule->ofproto, ofm, NULL);
> > if (rule->ofproto != orig_ofproto) {
> > ofmonitor_flush(rule->ofproto->connmgr);
> > }
> > +
> > + return error;
> > }
> >
> > /* Refresh 'ofm->temp_rule', for which the caller holds a reference, if
> already
> > @@ -5144,7 +5157,7 @@ ofproto_flow_mod_learn(struct ofproto_flow_mod
> *ofm, bool keep_ref,
> >
> > error = ofproto_flow_mod_learn_start(ofm);
> > if (!error) {
> > - ofproto_flow_mod_learn_finish(ofm, NULL);
> > + error = ofproto_flow_mod_learn_finish(ofm, NULL);
> > }
> > } else {
> > static struct vlog_rate_limit rll = VLOG_RATE_LIMIT_INIT(1,
> 5);
> > @@ -5244,7 +5257,7 @@ replace_rule_revert(struct ofproto *ofproto,
> > }
> >
> > /* Adds the 'new_rule', replacing the 'old_rule'. */
> > -static void
> > +static enum ofperr
> > replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod
> *ofm,
> > const struct openflow_mod_requester *req,
> > struct rule *old_rule, struct rule *new_rule,
> > @@ -5252,6 +5265,8 @@ replace_rule_finish(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm,
> > OVS_REQUIRES(ofproto_mutex)
> > {
> > struct rule *replaced_rule;
> > + enum ofperr error = 0;
> > + struct oftable *table = &ofproto->tables[new_rule->table_id];
> >
> > replaced_rule = (old_rule && old_rule->removed_reason !=
> OFPRR_EVICTION)
> > ? old_rule : NULL;
> > @@ -5261,8 +5276,15 @@ replace_rule_finish(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm,
> > * link the packet and byte counts from the old rule to the new one
> if
> > * 'modify_keep_counts' is 'true'. The 'replaced_rule' will be
> deleted
> > * right after this call. */
> > - ofproto->ofproto_class->rule_insert(new_rule, replaced_rule,
> > - ofm->modify_keep_counts);
> > + error = ofproto->ofproto_class->rule_insert(new_rule,
> replaced_rule,
> > +
> ofm->modify_keep_counts);
> > + if (error) {
> > + if (classifier_remove(&table->cls, &new_rule->cr)) {
> > + ofproto_rule_destroy__(new_rule);
> > + }
> > + return error;
> > + }
> > +
> > learned_cookies_inc(ofproto, rule_get_actions(new_rule));
> >
> > if (old_rule) {
> > @@ -5298,6 +5320,8 @@ replace_rule_finish(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm,
> > req ? req->request->xid : 0, NULL);
> > }
> > }
> > +
> > + return error;
> > }
> >
> > /* ofm->temp_rule is consumed only in the successful case. */
> > @@ -5448,17 +5472,18 @@ modify_flows_revert(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm)
> > }
> > }
> >
> > -static void
> > +static enum ofperr
> > modify_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod
> *ofm,
> > const struct openflow_mod_requester *req)
> > OVS_REQUIRES(ofproto_mutex)
> > {
> > struct rule_collection *old_rules = &ofm->old_rules;
> > struct rule_collection *new_rules = &ofm->new_rules;
> > + enum ofperr error = 0;
> >
> > if (rule_collection_n(old_rules) == 0
> > && rule_collection_n(new_rules) == 1) {
> > - add_flow_finish(ofproto, ofm, req);
> > + error = add_flow_finish(ofproto, ofm, req);
> > } else if (rule_collection_n(old_rules) > 0) {
> > struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_
> cookies);
> >
> > @@ -5467,12 +5492,17 @@ modify_flows_finish(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm,
> >
> > struct rule *old_rule, *new_rule;
> > RULE_COLLECTIONS_FOR_EACH (old_rule, new_rule, old_rules,
> new_rules) {
> > - replace_rule_finish(ofproto, ofm, req, old_rule, new_rule,
> > - &dead_cookies);
> > + error = replace_rule_finish(ofproto, ofm, req, old_rule,
> new_rule,
> > + &dead_cookies);
> > + if (error) {
> > + return error;
> > + }
> > }
> > learned_cookies_flush(ofproto, &dead_cookies);
> > remove_rules_postponed(old_rules);
> > }
> > +
> > + return error;
> > }
> >
> > static enum ofperr
> > @@ -5838,7 +5868,7 @@ handle_flow_mod__(struct ofproto *ofproto, const
> struct ofputil_flow_mod *fm,
> > error = ofproto_flow_mod_start(ofproto, &ofm);
> > if (!error) {
> > ofproto_bump_tables_version(ofproto);
> > - ofproto_flow_mod_finish(ofproto, &ofm, req);
> > + error = ofproto_flow_mod_finish(ofproto, &ofm, req);
> > ofmonitor_flush(ofproto->connmgr);
> > }
> > ovs_mutex_unlock(&ofproto_mutex);
> > @@ -7668,19 +7698,21 @@ ofproto_flow_mod_revert(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm)
> > rule_collection_destroy(&ofm->new_rules);
> > }
> >
> > -static void
> > +static enum ofperr
> > ofproto_flow_mod_finish(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> > const struct openflow_mod_requester *req)
> > OVS_REQUIRES(ofproto_mutex)
> > {
> > + enum ofperr error = 0;
> > +
> > switch (ofm->command) {
> > case OFPFC_ADD:
> > - add_flow_finish(ofproto, ofm, req);
> > + error = add_flow_finish(ofproto, ofm, req);
> > break;
> >
> > case OFPFC_MODIFY:
> > case OFPFC_MODIFY_STRICT:
> > - modify_flows_finish(ofproto, ofm, req);
> > + error = modify_flows_finish(ofproto, ofm, req);
> > break;
> >
> > case OFPFC_DELETE:
> > @@ -7698,6 +7730,8 @@ ofproto_flow_mod_finish(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm,
> > if (req) {
> > ofconn_report_flow_mod(req->ofconn, ofm->command);
> > }
> > +
> > + return error;
> > }
> >
> > /* Commit phases (all while locking ofproto_mutex):
> > @@ -7827,13 +7861,35 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t
> id, uint16_t flags)
> > struct openflow_mod_requester req = { ofconn,
> be->msg };
> >
> > if (be->type == OFPTYPE_FLOW_MOD) {
> > - ofproto_flow_mod_finish(ofproto, &be->ofm,
> &req);
> > + error = ofproto_flow_mod_finish(ofproto,
> &be->ofm,
> > + &req);
> > } else if (be->type == OFPTYPE_GROUP_MOD) {
> > ofproto_group_mod_finish(ofproto, &be->ogm,
> &req);
> > } else if (be->type == OFPTYPE_PACKET_OUT) {
> > ofproto_packet_out_finish(ofproto, &be->opo);
> > }
> > }
> > + if (error) {
> > + break;
> > + }
> > + }
> > + if (error) {
> > + /* Send error referring to the original message. */
> > + if (error) {
>
> This second if looks strange. I think it should be removed.
>
> I don't know if there are any implications to passing 0 for an ofperr.
> I don't see an issue in this patch, but I'm not sure if any callers in
> the future may be affected. There was previously a 0 value used for
> OFPBIC_BAD_EXP_TYPE - I don't even know if it would matter now. Maybe
> it's better to have these return an integer type and then it might make
> the interface clearer. WDYT?
>
> > + ofconn_send_error(ofconn, be->msg, error);
> > + error = OFPERR_OFPBFC_MSG_FAILED;
> > + }
> > + /* Revert. Undo all the changes made above. */
> > + LIST_FOR_EACH_REVERSE_CONTINUE (be, node,
> &bundle->msg_list) {
> > + if (be->type == OFPTYPE_FLOW_MOD) {
> > + ofproto_flow_mod_revert(ofproto, &be->ofm);
> > + } else if (be->type == OFPTYPE_GROUP_MOD) {
> > + ofproto_group_mod_revert(ofproto, &be->ogm);
> > + } else if (be->type == OFPTYPE_PACKET_OUT) {
> > + ofproto_packet_out_revert(ofproto, &be->opo);
> > + }
> > + /* Nothing needs to be reverted for a port mod. */
> > + }
> > }
> > }
>
More information about the dev
mailing list