Message ID | 1446088078-32610-4-git-send-email-jrajahalme@nicira.com |
---|---|
State | Accepted |
Headers | show |
On 28 October 2015 at 20:07, Jarno Rajahalme <jrajahalme@nicira.com> wrote: > Restrictions from embedded actions should be folded in rather than discarded. > > Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> Thanks for catching this, one comment below. Acked-by: Joe Stringer <joestringer@nicira.com> > @@ -6152,6 +6158,7 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a, > case OFPACT_CT: { > struct ofpact_conntrack *oc = ofpact_get_CT(a); > enum ofputil_protocol p = *usable_protocols; > + enum ofperr err; > > if (!dl_type_is_ip_any(flow->dl_type) > || (flow->ct_state & CS_INVALID && oc->flags & NX_CT_F_COMMIT)) { > @@ -6162,8 +6169,10 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a, > return mf_check_src(&oc->zone_src, flow); > } > > - return ofpacts_check(oc->actions, ofpact_ct_get_action_len(oc), > - flow, max_ports, table_id, n_tables, &p); > + err = ofpacts_check(oc->actions, ofpact_ct_get_action_len(oc), > + flow, max_ports, table_id, n_tables, &p); > + *usable_protocols &= p; /* Fold any new restrictions back. */ > + return err; > } I think 'p' should probably just be removed, and pass usable_protocols directly?
Pushed with the change proposed, thanks for the review! Jarno > On Nov 4, 2015, at 11:38 AM, Joe Stringer <joestringer@nicira.com> wrote: > > On 28 October 2015 at 20:07, Jarno Rajahalme <jrajahalme@nicira.com> wrote: >> Restrictions from embedded actions should be folded in rather than discarded. >> >> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> > > Thanks for catching this, one comment below. > > Acked-by: Joe Stringer <joestringer@nicira.com> > >> @@ -6152,6 +6158,7 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a, >> case OFPACT_CT: { >> struct ofpact_conntrack *oc = ofpact_get_CT(a); >> enum ofputil_protocol p = *usable_protocols; >> + enum ofperr err; >> >> if (!dl_type_is_ip_any(flow->dl_type) >> || (flow->ct_state & CS_INVALID && oc->flags & NX_CT_F_COMMIT)) { >> @@ -6162,8 +6169,10 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a, >> return mf_check_src(&oc->zone_src, flow); >> } >> >> - return ofpacts_check(oc->actions, ofpact_ct_get_action_len(oc), >> - flow, max_ports, table_id, n_tables, &p); >> + err = ofpacts_check(oc->actions, ofpact_ct_get_action_len(oc), >> + flow, max_ports, table_id, n_tables, &p); >> + *usable_protocols &= p; /* Fold any new restrictions back. */ >> + return err; >> } > > I think 'p' should probably just be removed, and pass usable_protocols directly?
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 5f72fda..9af3415 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -4852,9 +4852,15 @@ parse_CT(char *arg, struct ofpbuf *ofpacts, } else if (!strcmp(key, "exec")) { /* Hide existing actions from ofpacts_parse_copy(), so the * nesting can be handled transparently. */ + enum ofputil_protocol usable_protocols2 = *usable_protocols; + ofpbuf_pull(ofpacts, sizeof(*oc)); - error = ofpacts_parse_copy(value, ofpacts, usable_protocols, false, - OFPACT_CT); + /* Initializes 'usable_protocol2', fold it back to + * '*usable_protocols' afterwards, so that we do not lose + * restrictions already in there. */ + error = ofpacts_parse_copy(value, ofpacts, &usable_protocols2, + false, OFPACT_CT); + *usable_protocols &= usable_protocols2; ofpact_pad(ofpacts); ofpacts->header = ofpbuf_push_uninit(ofpacts, sizeof(*oc)); oc = ofpacts->header; @@ -6152,6 +6158,7 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a, case OFPACT_CT: { struct ofpact_conntrack *oc = ofpact_get_CT(a); enum ofputil_protocol p = *usable_protocols; + enum ofperr err; if (!dl_type_is_ip_any(flow->dl_type) || (flow->ct_state & CS_INVALID && oc->flags & NX_CT_F_COMMIT)) { @@ -6162,8 +6169,10 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a, return mf_check_src(&oc->zone_src, flow); } - return ofpacts_check(oc->actions, ofpact_ct_get_action_len(oc), - flow, max_ports, table_id, n_tables, &p); + err = ofpacts_check(oc->actions, ofpact_ct_get_action_len(oc), + flow, max_ports, table_id, n_tables, &p); + *usable_protocols &= p; /* Fold any new restrictions back. */ + return err; } case OFPACT_CLEAR_ACTIONS:
Restrictions from embedded actions should be folded in rather than discarded. Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> --- lib/ofp-actions.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)