diff mbox

[ovs-dev,3/3] ofproto-dpif: Check support for resubmit with conntrack action.

Message ID 1492215949-75081-3-git-send-email-jarno@ovn.org
State Accepted
Headers show

Commit Message

Jarno Rajahalme April 15, 2017, 12:25 a.m. UTC 
Use the existing probed support flag for the original direction tuple
to determine if resubmit(ct) can be executed or not.

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
 ofproto/ofproto-dpif.c | 86 ++++++++++++++++++++++++++------------------------
 1 file changed, 45 insertions(+), 41 deletions(-)

Comments

Ben Pfaff April 18, 2017, 3:56 p.m. UTC | #1 
On Fri, Apr 14, 2017 at 05:25:49PM -0700, Jarno Rajahalme wrote:
> Use the existing probed support flag for the original direction tuple
> to determine if resubmit(ct) can be executed or not.
> 
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>

Thank you!  I think that this will help to make for less confusing
errors in the future.  I did not study this carefully, but what I saw
looked good.

Acked-by: Ben Pfaff <blp@ovn.org>
Jarno Rajahalme April 18, 2017, 6:12 p.m. UTC | #2 
> On Apr 18, 2017, at 8:56 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Fri, Apr 14, 2017 at 05:25:49PM -0700, Jarno Rajahalme wrote:
>> Use the existing probed support flag for the original direction tuple
>> to determine if resubmit(ct) can be executed or not.
>> 
>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
> 
> Thank you!  I think that this will help to make for less confusing
> errors in the future.  I did not study this carefully, but what I saw
> looked good.
> 
> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks for the reviews!

Series pushed to master,

  Jarno
diff mbox

Patch

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 25f8adf..23afc76 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4155,13 +4155,12 @@  check_mask(struct ofproto_dpif *ofproto, const struct miniflow *flow)
 }
 
 static void
-report_unsupported_ct(const char *detail)
+report_unsupported_act(const char *action, const char *detail)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-    VLOG_WARN_RL(&rl, "Rejecting ct action because datapath does not support "
-                 "ct action%s%s (your kernel module may be out of date)",
-                 detail ? " " : "",
-                 detail ? detail : "");
+    VLOG_WARN_RL(&rl, "Rejecting %s action because datapath does not support"
+                 "%s%s (your kernel module may be out of date)",
+                 action, detail ? " " : "", detail ? detail : "");
 }
 
 static enum ofperr
@@ -4169,51 +4168,56 @@  check_actions(const struct ofproto_dpif *ofproto,
               const struct rule_actions *const actions)
 {
     const struct ofpact *ofpact;
+    const struct odp_support *support = &ofproto->backer->support.odp;
 
     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) {
+            const struct ofpact_conntrack *ct;
+            const struct ofpact *a;
 
-        if (ofpact->type != OFPACT_CT) {
-            continue;
-        }
+            ct = CONTAINER_OF(ofpact, struct ofpact_conntrack, ofpact);
 
-        ct = CONTAINER_OF(ofpact, struct ofpact_conntrack, ofpact);
-        support = &ofproto->backer->support.odp;
+            if (!support->ct_state) {
+                report_unsupported_act("ct", "ct action");
+                return OFPERR_OFPBAC_BAD_TYPE;
+            }
+            if ((ct->zone_imm || ct->zone_src.field) && !support->ct_zone) {
+                report_unsupported_act("ct", "ct zones");
+                return OFPERR_OFPBAC_BAD_ARGUMENT;
+            }
+            /* So far the force commit feature is implemented together with the
+             * original direction tuple feature by all datapaths, so we use the
+             * support flag for the 'ct_orig_tuple' to indicate support for the
+             * force commit feature as well. */
+            if ((ct->flags & NX_CT_F_FORCE) && !support->ct_orig_tuple) {
+                report_unsupported_act("ct", "force commit");
+                return OFPERR_OFPBAC_BAD_ARGUMENT;
+            }
 
-        if (!support->ct_state) {
-            report_unsupported_ct(NULL);
-            return OFPERR_OFPBAC_BAD_TYPE;
-        }
-        if ((ct->zone_imm || ct->zone_src.field) && !support->ct_zone) {
-            report_unsupported_ct("zone");
-            return OFPERR_OFPBAC_BAD_ARGUMENT;
-        }
-        /* So far the force commit feature is implemented together with the
-         * original direction tuple feature by all datapaths, so we use the
-         * support flag for the 'ct_orig_tuple' to indicate support for the
-         * force commit feature as well. */
-        if ((ct->flags & NX_CT_F_FORCE) && !support->ct_orig_tuple) {
-            report_unsupported_ct("force commit");
-            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);
 
-        OFPACT_FOR_EACH(a, ct->actions, ofpact_ct_get_action_len(ct)) {
-            const struct mf_field *dst = ofpact_get_mf_dst(a);
+                if (a->type == OFPACT_NAT && !support->ct_state_nat) {
+                    /* The backer doesn't seem to support the NAT bits in
+                     * 'ct_state': assume that it doesn't support the NAT
+                     * action. */
+                    report_unsupported_act("ct", "nat");
+                    return OFPERR_OFPBAC_BAD_TYPE;
+                }
+                if (dst && ((dst->id == MFF_CT_MARK && !support->ct_mark) ||
+                            (dst->id == MFF_CT_LABEL && !support->ct_label))) {
+                    report_unsupported_act("ct", "setting mark and/or label");
+                    return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
+                }
+            }
+        } else if (ofpact->type == OFPACT_RESUBMIT) {
+            struct ofpact_resubmit *resubmit = ofpact_get_RESUBMIT(ofpact);
 
-            if (a->type == OFPACT_NAT && !support->ct_state_nat) {
-                /* The backer doesn't seem to support the NAT bits in
-                 * 'ct_state': assume that it doesn't support the NAT
-                 * action. */
-                report_unsupported_ct("nat");
+            if (resubmit->with_ct_orig && !support->ct_orig_tuple) {
+                report_unsupported_act("resubmit",
+                                       "ct original direction tuple");
                 return OFPERR_OFPBAC_BAD_TYPE;
             }
-            if (dst && ((dst->id == MFF_CT_MARK && !support->ct_mark)
-                        || (dst->id == MFF_CT_LABEL && !support->ct_label))) {
-                report_unsupported_ct("setting mark and/or label");
-                return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
-            }
         }
     }