On 10 November 2015 at 13:56, Joe Stringer <[email protected]> wrote: > On 9 November 2015 at 17:25, Jarno Rajahalme <[email protected]> wrote: >> >>> On Nov 7, 2015, at 12:05 PM, Joe Stringer <[email protected]> wrote: >>> >>> Disallow installing rules that execute ct() if conntrack is unsupported >>> in the datapath. >>> >>> Reported-by: Ravindra Kenchappa <[email protected]> >>> Signed-off-by: Joe Stringer <[email protected]> >>> --- >>> ofproto/ofproto-dpif.c | 44 +++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 43 insertions(+), 1 deletion(-) >>> >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>> index 2f75b93d9694..e09c28bb15ed 100644 >>> --- a/ofproto/ofproto-dpif.c >>> +++ b/ofproto/ofproto-dpif.c >>> @@ -4048,6 +4048,44 @@ check_flow(const struct ofproto_dpif *ofproto, const >>> struct miniflow *flow, >>> } >>> >>> static enum ofperr >>> +check_actions(const struct ofproto_dpif *ofproto, >>> + const struct rule_actions *const actions) >>> +{ >>> + const struct ofpact *ofpact; >>> + >>> + OFPACT_FOR_EACH (ofpact, actions->ofpacts, actions->ofpacts_len) { >>> + const struct odp_support *support; >>> + const struct ofpact_conntrack *ct; >>> + const struct ofpact *a; >>> + >>> + if (ofpact->type != OFPACT_CT) { >>> + continue; >>> + } >>> + >>> + ct = CONTAINER_OF(ofpact, struct ofpact_conntrack, ofpact); >>> + support = &ofproto_dpif_get_support(ofproto)->odp; >>> + >>> + if (!support->ct_state) { >>> + return OFPERR_OFPBAC_BAD_TYPE; >>> + } >>> + if ((ct->zone_imm || ct->zone_src.field) && !support->ct_zone) { >>> + return OFPERR_OFPBAC_BAD_ARGUMENT; >>> + } >>> + >>> + OFPACT_FOR_EACH(a, ct->actions, ofpact_ct_get_action_len(ct)) { >>> + const struct mf_field *dst = ofpact_get_mf_dst(a); >>> + >>> + if (dst && ((dst->id == MFF_CT_MARK && !support->ct_mark) >>> + || (dst->id == MFF_CT_LABEL && >>> !support->ct_label))) { >>> + return OFPERR_OFPBAC_BAD_SET_ARGUMENT; >>> + } >>> + } >>> + } >>> + >>> + return 0; >>> +} >> >> >> We already loop through the actions for a similar purpose in >> ofproto_check_ofpacts(). Maybe make something like is_action_supported() >> accessible via the ofproto class and call that for the conntrack action from >> ofproto_check_ofpacts()? > > Makes sense, I'll rework this patch do to it like this.
Taking another look at this, I'm on the fence about whether it's better conceptually than what we have currently. For one, it effectively shifts some of the error checking that is currently part of the ofproto's ->rule_construct() method into a separate method on the ofproto class, to avoid an extra iteration on the actions. This spreads the error checking across more locations, and makes the rule lifecycle slightly more complex (actions must be checked before rule construction). I'll send out the series with this feedback applied anyway, and we can look at it again. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
