> On Nov 11, 2015, at 10:21 AM, Joe Stringer <joestrin...@nicira.com> wrote:
>
> On 10 November 2015 at 13:56, Joe Stringer <joestrin...@nicira.com> wrote:
>> On 9 November 2015 at 17:25, Jarno Rajahalme <ja...@ovn.org> wrote:
>>>
>>>> On Nov 7, 2015, at 12:05 PM, Joe Stringer <joestrin...@nicira.com> wrote:
>>>>
>>>> Disallow installing rules that execute ct() if conntrack is unsupported
>>>> in the datapath.
>>>>
>>>> Reported-by: Ravindra Kenchappa <ravindra.kencha...@hpe.com>
>>>> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
>>>> ---
>>>> 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 would do the check call only for actions we know might not be supported.
Jarno
> I'll send out the series with this feedback applied anyway, and we can
> look at it again.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
