[ovs-dev] [PATCH v2] ofp-actions: Split ofpacts_check__() into many functions.

Ben Pfaff blp at ovn.org
Fri Jun 15 23:29:22 UTC 2018


ofpacts_check__() was a huge switch statement with special cases for many
different kinds of actions.  This made it unwieldy and put the special
cases far away from the rest of the code related to a given action.  This
commit refactors the code to avoid the problem.

Signed-off-by: Ben Pfaff <blp at ovn.org>
---
I'm reposting this in hope of getting a review this time.

 include/openvswitch/ofp-actions.h |  20 +-
 lib/ofp-actions.c                 | 973 ++++++++++++++++++++++++--------------
 lib/ofp-flow.c                    |  19 +-
 ofproto/ofproto-dpif-trace.c      |  20 +-
 ofproto/ofproto.c                 |  12 +-
 utilities/ovs-ofctl.c             |  19 +-
 6 files changed, 678 insertions(+), 385 deletions(-)

diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index b3dd0959d53e..8f5ebaa2be51 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -1029,14 +1029,22 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
                                    const struct vl_mff_map *vl_mff_map,
                                    uint64_t *ofpacts_tlv_bitmap,
                                    struct ofpbuf *ofpacts);
+
+struct ofpact_check_params {
+    /* Input. */
+    struct match *match;
+    ofp_port_t max_ports;
+    uint8_t table_id;
+    uint8_t n_tables;
+
+    /* Output. */
+    enum ofputil_protocol usable_protocols;
+};
 enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
-                          struct match *, ofp_port_t max_ports,
-                          uint8_t table_id, uint8_t n_tables,
-                          enum ofputil_protocol *usable_protocols);
+                          struct ofpact_check_params *);
 enum ofperr ofpacts_check_consistency(struct ofpact[], size_t ofpacts_len,
-                                      struct match *, ofp_port_t max_ports,
-                                      uint8_t table_id, uint8_t n_tables,
-                                      enum ofputil_protocol usable_protocols);
+                                      enum ofputil_protocol needed_protocols,
+                                      struct ofpact_check_params *);
 enum ofperr ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports);
 
 /* Converting ofpacts to OpenFlow. */
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 87797bc9a4fd..a5967ea777fa 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -430,6 +430,8 @@ static char * OVS_WARN_UNUSED_RESULT ofpacts_parse_copy(
     const char *s_, const struct ofpact_parse_params *pp,
     bool allow_instructions, enum ofpact_type outer_action);
 
+static void inconsistent_match(enum ofputil_protocol *usable_protocols);
+
 /* Returns the ofpact following 'ofpact', except that if 'ofpact' contains
  * nested ofpacts it returns the first one. */
 struct ofpact *
@@ -687,6 +689,13 @@ format_OUTPUT(const struct ofpact_output *a,
         ds_put_format(fp->s, ":%"PRIu16, a->max_len);
     }
 }
+
+static enum ofperr
+check_OUTPUT(const struct ofpact_output *a,
+             const struct ofpact_check_params *cp)
+{
+    return ofpact_check_output_port(a->port, cp->max_ports);
+}
 
 /* Group actions. */
 
@@ -719,6 +728,13 @@ format_GROUP(const struct ofpact_group *a,
     ds_put_format(fp->s, "%sgroup:%s%"PRIu32,
                   colors.special, colors.end, a->group_id);
 }
+
+static enum ofperr
+check_GROUP(const struct ofpact_group *a OVS_UNUSED,
+            const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Action structure for NXAST_CONTROLLER.
  *
@@ -1010,6 +1026,13 @@ format_CONTROLLER(const struct ofpact_controller *a,
         ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
     }
 }
+
+static enum ofperr
+check_CONTROLLER(const struct ofpact_controller *a OVS_UNUSED,
+                 const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Enqueue action. */
 struct ofp10_action_enqueue {
@@ -1090,6 +1113,18 @@ format_ENQUEUE(const struct ofpact_enqueue *a,
     ofputil_format_port(a->port, fp->port_map, fp->s);
     ds_put_format(fp->s, ":%"PRIu32, a->queue);
 }
+
+static enum ofperr
+check_ENQUEUE(const struct ofpact_enqueue *a,
+              const struct ofpact_check_params *cp)
+{
+    if (ofp_to_u16(a->port) >= ofp_to_u16(cp->max_ports)
+        && a->port != OFPP_IN_PORT
+        && a->port != OFPP_LOCAL) {
+        return OFPERR_OFPBAC_BAD_OUT_PORT;
+    }
+    return 0;
+}
 
 /* Action structure for NXAST_OUTPUT_REG.
  *
@@ -1243,6 +1278,13 @@ format_OUTPUT_REG(const struct ofpact_output_reg *a,
     ds_put_format(fp->s, "%soutput:%s", colors.special, colors.end);
     mf_format_subfield(&a->src, fp->s);
 }
+
+static enum ofperr
+check_OUTPUT_REG(const struct ofpact_output_reg *a,
+                 const struct ofpact_check_params *cp)
+{
+    return mf_check_src(&a->src, cp->match);
+}
 
 /* Action structure for NXAST_BUNDLE and NXAST_BUNDLE_LOAD.
  *
@@ -1461,6 +1503,13 @@ format_BUNDLE(const struct ofpact_bundle *a,
 {
     bundle_format(a, fp->port_map, fp->s);
 }
+
+static enum ofperr
+check_BUNDLE(const struct ofpact_bundle *a,
+             const struct ofpact_check_params *cp)
+{
+    return bundle_check(a, cp->max_ports, cp->match);
+}
 
 /* Set VLAN actions. */
 
@@ -1552,6 +1601,23 @@ format_SET_VLAN_VID(const struct ofpact_vlan_vid *a,
                   a->push_vlan_if_needed ? "mod_vlan_vid" : "set_vlan_vid",
                   colors.end, a->vlan_vid);
 }
+
+static enum ofperr
+check_SET_VLAN_VID(struct ofpact_vlan_vid *a, struct ofpact_check_params *cp)
+{
+    /* Remember if we saw a vlan tag in the flow to aid translating to OpenFlow
+     * 1.1+ if need be. */
+    ovs_be16 *tci = &cp->match->flow.vlans[0].tci;
+    a->flow_has_vlan = (*tci & htons(VLAN_CFI)) != 0;
+    if (!a->flow_has_vlan && !a->push_vlan_if_needed) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+
+    /* Temporarily mark that we have a vlan tag. */
+    *tci |= htons(VLAN_CFI);
+
+    return 0;
+}
 
 /* Set PCP actions. */
 
@@ -1643,6 +1709,23 @@ format_SET_VLAN_PCP(const struct ofpact_vlan_pcp *a,
                   a->push_vlan_if_needed ? "mod_vlan_pcp" : "set_vlan_pcp",
                   colors.end, a->vlan_pcp);
 }
+
+static enum ofperr
+check_SET_VLAN_PCP(struct ofpact_vlan_pcp *a, struct ofpact_check_params *cp)
+{
+    /* Remember if we saw a vlan tag in the flow to aid translating to OpenFlow
+     * 1.1+ if need be. */
+    ovs_be16 *tci = &cp->match->flow.vlans[0].tci;
+    a->flow_has_vlan = (*tci & htons(VLAN_CFI)) != 0;
+    if (!a->flow_has_vlan && !a->push_vlan_if_needed) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+
+    /* Temporarily mark that we have a vlan tag. */
+    *tci |= htons(VLAN_CFI);
+
+    return 0;
+}
 
 /* Strip VLAN actions. */
 
@@ -1694,6 +1777,17 @@ format_STRIP_VLAN(const struct ofpact_null *a,
                     : "%sstrip_vlan%s"),
                   colors.value, colors.end);
 }
+
+static enum ofperr
+check_STRIP_VLAN(const struct ofpact_null *a OVS_UNUSED,
+                 struct ofpact_check_params *cp)
+{
+    if (!(cp->match->flow.vlans[0].tci & htons(VLAN_CFI))) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+    flow_pop_vlan(&cp->match->flow, NULL);
+    return 0;
+}
 
 /* Push VLAN action. */
 
@@ -1751,6 +1845,21 @@ format_PUSH_VLAN(const struct ofpact_push_vlan *push_vlan,
     ds_put_format(fp->s, "%spush_vlan:%s%#"PRIx16,
                   colors.param, colors.end, ntohs(push_vlan->ethertype));
 }
+
+static enum ofperr
+check_PUSH_VLAN(const struct ofpact_push_vlan *a OVS_UNUSED,
+                struct ofpact_check_params *cp)
+{
+    struct flow *flow = &cp->match->flow;
+    if (flow->vlans[FLOW_MAX_VLAN_HEADERS - 1].tci & htons(VLAN_CFI)) {
+        /* Support maximum (FLOW_MAX_VLAN_HEADERS) VLAN headers. */
+        return OFPERR_OFPBAC_BAD_TAG;
+    }
+    /* Temporary mark that we have a vlan tag. */
+    flow_push_vlan_uninit(flow, NULL);
+    flow->vlans[0].tci |= htons(VLAN_CFI);
+    return 0;
+}
 
 /* Action structure for OFPAT10_SET_DL_SRC/DST and OFPAT11_SET_DL_SRC/DST. */
 struct ofp_action_dl_addr {
@@ -1840,6 +1949,20 @@ format_SET_ETH_DST(const struct ofpact_mac *a,
     ds_put_format(fp->s, "%smod_dl_dst:%s"ETH_ADDR_FMT,
                   colors.param, colors.end, ETH_ADDR_ARGS(a->mac));
 }
+
+static enum ofperr
+check_SET_ETH_SRC(const struct ofpact_mac *a OVS_UNUSED,
+                  const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
+
+static enum ofperr
+check_SET_ETH_DST(const struct ofpact_mac *a OVS_UNUSED,
+                  const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Set IPv4 address actions. */
 
@@ -1918,6 +2041,30 @@ format_SET_IPV4_DST(const struct ofpact_ipv4 *a,
     ds_put_format(fp->s, "%smod_nw_dst:%s"IP_FMT,
                   colors.param, colors.end, IP_ARGS(a->ipv4));
 }
+
+static enum ofperr
+check_set_ipv4(struct ofpact_check_params *cp)
+{
+    ovs_be16 dl_type = get_dl_type(&cp->match->flow);
+    if (dl_type != htons(ETH_TYPE_IP)) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+    return 0;
+}
+
+static enum ofperr
+check_SET_IPV4_SRC(const struct ofpact_ipv4 *a OVS_UNUSED,
+                   struct ofpact_check_params *cp)
+{
+    return check_set_ipv4(cp);
+}
+
+static enum ofperr
+check_SET_IPV4_DST(const struct ofpact_ipv4 *a OVS_UNUSED,
+                   struct ofpact_check_params *cp)
+{
+    return check_set_ipv4(cp);
+}
 
 /* Set IPv4/v6 TOS actions. */
 
@@ -1971,6 +2118,22 @@ format_SET_IP_DSCP(const struct ofpact_dscp *a,
     ds_put_format(fp->s, "%smod_nw_tos:%s%d",
                   colors.param, colors.end, a->dscp);
 }
+
+static enum ofperr
+check_set_ip(struct ofpact_check_params *cp)
+{
+    if (!is_ip_any(&cp->match->flow)) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+    return 0;
+}
+
+static enum ofperr
+check_SET_IP_DSCP(const struct ofpact_dscp *a OVS_UNUSED,
+                  struct ofpact_check_params *cp)
+{
+    return check_set_ip(cp);
+}
 
 /* Set IPv4/v6 ECN actions. */
 
@@ -2028,6 +2191,13 @@ format_SET_IP_ECN(const struct ofpact_ecn *a,
     ds_put_format(fp->s, "%smod_nw_ecn:%s%d",
                   colors.param, colors.end, a->ecn);
 }
+
+static enum ofperr
+check_SET_IP_ECN(const struct ofpact_ecn *a OVS_UNUSED,
+                 struct ofpact_check_params *cp)
+{
+    return check_set_ip(cp);
+}
 
 /* Set IPv4/v6 TTL actions. */
 
@@ -2076,6 +2246,13 @@ format_SET_IP_TTL(const struct ofpact_ip_ttl *a,
     ds_put_format(fp->s, "%smod_nw_ttl:%s%d",
                   colors.param, colors.end, a->ttl);
 }
+
+static enum ofperr
+check_SET_IP_TTL(const struct ofpact_ip_ttl *a OVS_UNUSED,
+                 struct ofpact_check_params *cp)
+{
+    return check_set_ip(cp);
+}
 
 /* Set TCP/UDP/SCTP port actions. */
 
@@ -2167,6 +2344,37 @@ format_SET_L4_DST_PORT(const struct ofpact_l4_port *a,
     ds_put_format(fp->s, "%smod_tp_dst:%s%d",
                   colors.param, colors.end, a->port);
 }
+
+static enum ofperr
+check_set_l4_port(struct ofpact_l4_port *a, struct ofpact_check_params *cp)
+{
+    const struct flow *flow = &cp->match->flow;
+    if (!is_ip_any(flow)
+        || flow->nw_frag & FLOW_NW_FRAG_LATER
+        || (flow->nw_proto != IPPROTO_TCP &&
+            flow->nw_proto != IPPROTO_UDP &&
+            flow->nw_proto != IPPROTO_SCTP)) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+
+    /* Note the transport protocol in use, to allow this action to be converted
+     * to an OF1.2 set_field action later if necessary. */
+    a->flow_ip_proto = flow->nw_proto;
+
+    return 0;
+}
+
+static enum ofperr
+check_SET_L4_SRC_PORT(struct ofpact_l4_port *a, struct ofpact_check_params *cp)
+{
+    return check_set_l4_port(a, cp);
+}
+
+static enum ofperr
+check_SET_L4_DST_PORT(struct ofpact_l4_port *a, struct ofpact_check_params *cp)
+{
+    return check_set_l4_port(a, cp);
+}
 
 /* Action structure for OFPAT_COPY_FIELD. */
 struct ofp15_action_copy_field {
@@ -2465,6 +2673,13 @@ format_REG_MOVE(const struct ofpact_reg_move *a,
 {
     nxm_format_reg_move(a, fp->s);
 }
+
+static enum ofperr
+check_REG_MOVE(const struct ofpact_reg_move *a,
+               const struct ofpact_check_params *cp)
+{
+    return nxm_reg_move_check(a, cp->match);
+}
 
 /* Action structure for OFPAT12_SET_FIELD. */
 struct ofp12_action_set_field {
@@ -3084,6 +3299,34 @@ format_SET_FIELD(const struct ofpact_set_field *a,
     }
 }
 
+static enum ofperr
+check_SET_FIELD(struct ofpact_set_field *a,
+                const struct ofpact_check_params *cp)
+{
+    const struct mf_field *mf = a->field;
+    struct flow *flow = &cp->match->flow;
+    ovs_be16 *tci = &flow->vlans[0].tci;
+
+    /* Require OXM_OF_VLAN_VID to have an existing VLAN header. */
+    if (!mf_are_prereqs_ok(mf, flow, NULL)
+        || (mf->id == MFF_VLAN_VID && !(*tci & htons(VLAN_CFI)))) {
+        VLOG_WARN_RL(&rl, "set_field %s lacks correct prerequisites",
+                     mf->name);
+        return OFPERR_OFPBAC_MATCH_INCONSISTENT;
+    }
+
+    /* Remember if we saw a VLAN tag in the flow, to aid translating to
+     * OpenFlow 1.1 if need be. */
+    a->flow_has_vlan = (*tci & htons(VLAN_CFI)) != 0;
+    if (mf->id == MFF_VLAN_TCI) {
+        /* The set field may add or remove the VLAN tag,
+         * Mark the status temporarily. */
+        *tci = a->value->be16;
+    }
+
+    return 0;
+}
+
 /* Appends an OFPACT_SET_FIELD ofpact with enough space for the value and mask
  * for the 'field' to 'ofpacts' and returns it.  Fills in the value from
  * 'value', if non-NULL, and mask from 'mask' if non-NULL.  If 'value' is
@@ -3268,6 +3511,20 @@ format_STACK_POP(const struct ofpact_stack *a,
 {
     nxm_format_stack_pop(a, fp->s);
 }
+
+static enum ofperr
+check_STACK_PUSH(const struct ofpact_stack *a,
+                 const struct ofpact_check_params *cp)
+{
+    return nxm_stack_push_check(a, cp->match);
+}
+
+static enum ofperr
+check_STACK_POP(const struct ofpact_stack *a,
+                const struct ofpact_check_params *cp)
+{
+    return nxm_stack_pop_check(a, cp->match);
+}
 
 /* Action structure for NXAST_DEC_TTL_CNT_IDS.
  *
@@ -3432,6 +3689,13 @@ format_DEC_TTL(const struct ofpact_cnt_ids *a,
         ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
     }
 }
+
+static enum ofperr
+check_DEC_TTL(const struct ofpact_cnt_ids *a OVS_UNUSED,
+              struct ofpact_check_params *cp)
+{
+    return check_set_ip(cp);
+}
 
 /* Set MPLS label actions. */
 
@@ -3477,6 +3741,23 @@ format_SET_MPLS_LABEL(const struct ofpact_mpls_label *a,
                   colors.paren, colors.end, ntohl(a->label),
                   colors.paren, colors.end);
 }
+
+static enum ofperr
+check_set_mpls(struct ofpact_check_params *cp)
+{
+    ovs_be16 dl_type = get_dl_type(&cp->match->flow);
+    if (!eth_type_mpls(dl_type)) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+    return 0;
+}
+
+static enum ofperr
+check_SET_MPLS_LABEL(const struct ofpact_mpls_label *a OVS_UNUSED,
+                     struct ofpact_check_params *cp)
+{
+    return check_set_mpls(cp);
+}
 
 /* Set MPLS TC actions. */
 
@@ -3521,6 +3802,13 @@ format_SET_MPLS_TC(const struct ofpact_mpls_tc *a,
                   colors.paren, colors.end, a->tc,
                   colors.paren, colors.end);
 }
+
+static enum ofperr
+check_SET_MPLS_TC(const struct ofpact_mpls_tc *a OVS_UNUSED,
+                  struct ofpact_check_params *cp)
+{
+    return check_set_mpls(cp);
+}
 
 /* Set MPLS TTL actions. */
 
@@ -3566,6 +3854,13 @@ format_SET_MPLS_TTL(const struct ofpact_mpls_ttl *a,
                   colors.paren, colors.end, a->ttl,
                   colors.paren, colors.end);
 }
+
+static enum ofperr
+check_SET_MPLS_TTL(const struct ofpact_mpls_ttl *a OVS_UNUSED,
+                   struct ofpact_check_params *cp)
+{
+    return check_set_mpls(cp);
+}
 
 /* Decrement MPLS TTL actions. */
 
@@ -3596,6 +3891,13 @@ format_DEC_MPLS_TTL(const struct ofpact_null *a OVS_UNUSED,
 {
     ds_put_format(fp->s, "%sdec_mpls_ttl%s", colors.value, colors.end);
 }
+
+static enum ofperr
+check_DEC_MPLS_TTL(const struct ofpact_null *a OVS_UNUSED,
+                   struct ofpact_check_params *cp)
+{
+    return check_set_mpls(cp);
+}
 
 /* Push MPLS label action. */
 
@@ -3642,6 +3944,25 @@ format_PUSH_MPLS(const struct ofpact_push_mpls *a,
     ds_put_format(fp->s, "%spush_mpls:%s0x%04"PRIx16,
                   colors.param, colors.end, ntohs(a->ethertype));
 }
+
+static enum ofperr
+check_PUSH_MPLS(const struct ofpact_push_mpls *a,
+                struct ofpact_check_params *cp)
+{
+    struct flow *flow = &cp->match->flow;
+
+    if (flow->packet_type != htonl(PT_ETH)) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+    flow->dl_type = a->ethertype;
+
+    /* The packet is now MPLS and the MPLS payload is opaque.
+     * Thus nothing can be assumed about the network protocol.
+     * Temporarily mark that we have no nw_proto. */
+    flow->nw_proto = 0;
+
+    return 0;
+}
 
 /* Pop MPLS label action. */
 
@@ -3681,6 +4002,19 @@ format_POP_MPLS(const struct ofpact_pop_mpls *a,
     ds_put_format(fp->s, "%spop_mpls:%s0x%04"PRIx16,
                   colors.param, colors.end, ntohs(a->ethertype));
 }
+
+static enum ofperr
+check_POP_MPLS(const struct ofpact_pop_mpls *a, struct ofpact_check_params *cp)
+{
+    struct flow *flow = &cp->match->flow;
+    ovs_be16 dl_type = get_dl_type(flow);
+
+    if (flow->packet_type != htonl(PT_ETH) || !eth_type_mpls(dl_type)) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+    flow->dl_type = a->ethertype;
+    return 0;
+}
 
 /* Set tunnel ID actions. */
 
@@ -3750,6 +4084,13 @@ format_SET_TUNNEL(const struct ofpact_tunnel *a,
                    || a->ofpact.raw == NXAST_RAW_SET_TUNNEL64 ? "64" : ""),
                   colors.end, a->tun_id);
 }
+
+static enum ofperr
+check_SET_TUNNEL(const struct ofpact_tunnel *a OVS_UNUSED,
+                 const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Set queue action. */
 
@@ -3782,6 +4123,13 @@ format_SET_QUEUE(const struct ofpact_queue *a,
     ds_put_format(fp->s, "%sset_queue:%s%"PRIu32,
                   colors.param, colors.end, a->queue_id);
 }
+
+static enum ofperr
+check_SET_QUEUE(const struct ofpact_queue *a OVS_UNUSED,
+                const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Pop queue action. */
 
@@ -3813,6 +4161,13 @@ format_POP_QUEUE(const struct ofpact_null *a OVS_UNUSED,
 {
     ds_put_format(fp->s, "%spop_queue%s", colors.value, colors.end);
 }
+
+static enum ofperr
+check_POP_QUEUE(const struct ofpact_null *a OVS_UNUSED,
+                const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Action structure for NXAST_FIN_TIMEOUT.
  *
@@ -3913,6 +4268,17 @@ format_FIN_TIMEOUT(const struct ofpact_fin_timeout *a,
     ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
 }
 
+
+static enum ofperr
+check_FIN_TIMEOUT(const struct ofpact_fin_timeout *a OVS_UNUSED,
+                  struct ofpact_check_params *cp)
+{
+    if (cp->match->flow.nw_proto != IPPROTO_TCP) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+    return 0;
+}
+
 /* Action structure for NXAST_ENCAP */
 struct nx_action_encap {
     ovs_be16 type;         /* OFPAT_VENDOR. */
@@ -4105,6 +4471,20 @@ format_ENCAP(const struct ofpact_encap *a,
     ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
 }
 
+static enum ofperr
+check_ENCAP(const struct ofpact_encap *a, struct ofpact_check_params *cp)
+{
+    struct flow *flow = &cp->match->flow;
+    flow->packet_type = a->new_pkt_type;
+    if (pt_ns(flow->packet_type) == OFPHTN_ETHERTYPE) {
+        flow->dl_type = htons(pt_ns_type(flow->packet_type));
+    }
+    if (!is_ip_any(flow)) {
+        flow->nw_proto = 0;
+    }
+    return 0;
+}
+
 /* Action structure for NXAST_DECAP */
 struct nx_action_decap {
     ovs_be16 type;         /* OFPAT_VENDOR. */
@@ -4207,6 +4587,24 @@ format_DECAP(const struct ofpact_decap *a,
     ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
 }
 
+static enum ofperr
+check_DECAP(const struct ofpact_decap *a OVS_UNUSED,
+            struct ofpact_check_params *cp)
+{
+    struct flow *flow = &cp->match->flow;
+    if (flow->packet_type == htonl(PT_ETH)) {
+        /* Adjust the packet_type to allow subsequent actions. */
+        flow->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE,
+                                           ntohs(flow->dl_type));
+    } else {
+        /* The actual packet_type is only known after decapsulation.
+         * Do not allow subsequent actions that depend on packet headers. */
+        flow->packet_type = htonl(PT_UNKNOWN);
+        flow->dl_type = OVS_BE16_MAX;
+    }
+    return 0;
+}
+
 /* Action dec_nsh_ttl */
 
 static enum ofperr
@@ -4237,6 +4635,17 @@ format_DEC_NSH_TTL(const struct ofpact_null *a OVS_UNUSED,
     ds_put_format(fp->s, "%sdec_nsh_ttl%s", colors.special, colors.end);
 }
 
+static enum ofperr
+check_DEC_NSH_TTL(const struct ofpact_null *a OVS_UNUSED,
+                  struct ofpact_check_params *cp)
+{
+    struct flow *flow = &cp->match->flow;
+    if (flow->packet_type != htonl(PT_NSH) &&
+        flow->dl_type != htons(ETH_TYPE_NSH)) {
+        inconsistent_match(&cp->usable_protocols);
+    }
+    return 0;
+}
 
 /* Action structures for NXAST_RESUBMIT, NXAST_RESUBMIT_TABLE, and
  * NXAST_RESUBMIT_TABLE_CT.
@@ -4447,6 +4856,17 @@ format_RESUBMIT(const struct ofpact_resubmit *a,
         ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
     }
 }
+
+static enum ofperr
+check_RESUBMIT(const struct ofpact_resubmit *a,
+               const struct ofpact_check_params *cp)
+{
+    if (a->with_ct_orig && !is_ct_valid(&cp->match->flow, &cp->match->wc,
+                                        NULL)) {
+        return OFPERR_OFPBAC_MATCH_INCONSISTENT;
+    }
+    return 0;
+}
 
 /* Action structure for NXAST_LEARN and NXAST_LEARN2.
  *
@@ -5031,6 +5451,13 @@ format_LEARN(const struct ofpact_learn *a,
 {
     learn_format(a, fp->port_map, fp->table_map, fp->s);
 }
+
+static enum ofperr
+check_LEARN(const struct ofpact_learn *a,
+            const struct ofpact_check_params *cp)
+{
+    return learn_check(a, cp->match);
+}
 
 /* Action structure for NXAST_CONJUNCTION. */
 struct nx_action_conjunction {
@@ -5117,6 +5544,13 @@ parse_CONJUNCTION(const char *arg, const struct ofpact_parse_params *pp)
     add_conjunction(pp->ofpacts, id, clause - 1, n_clauses);
     return NULL;
 }
+
+static enum ofperr
+check_CONJUNCTION(const struct ofpact_conjunction *a OVS_UNUSED,
+                  const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Action structure for NXAST_MULTIPATH.
  *
@@ -5244,6 +5678,13 @@ format_MULTIPATH(const struct ofpact_multipath *a,
 {
     multipath_format(a, fp->s);
 }
+
+static enum ofperr
+check_MULTIPATH(const struct ofpact_multipath *a,
+                const struct ofpact_check_params *cp)
+{
+    return multipath_check(a, cp->match);
+}
 
 /* Action structure for NXAST_NOTE.
  *
@@ -5318,6 +5759,13 @@ format_NOTE(const struct ofpact_note *a,
     ds_put_format(fp->s, "%snote:%s", colors.param, colors.end);
     format_hex_arg(fp->s, a->data, a->length);
 }
+
+static enum ofperr
+check_NOTE(const struct ofpact_note *a OVS_UNUSED,
+           const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Exit action. */
 
@@ -5348,6 +5796,13 @@ format_EXIT(const struct ofpact_null *a OVS_UNUSED,
 {
     ds_put_format(fp->s, "%sexit%s", colors.special, colors.end);
 }
+
+static enum ofperr
+check_EXIT(const struct ofpact_null *a OVS_UNUSED,
+           const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Unroll xlate action. */
 
@@ -5378,6 +5833,14 @@ format_UNROLL_XLATE(const struct ofpact_unroll_xlate *a,
                   colors.param,   colors.end, ntohll(a->rule_cookie),
                   colors.paren,   colors.end);
 }
+
+static enum ofperr
+check_UNROLL_XLATE(const struct ofpact_unroll_xlate *a OVS_UNUSED,
+                   const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    /* UNROLL is an internal action that should never be seen via OpenFlow. */
+    return OFPERR_OFPBAC_BAD_TYPE;
+}
 
 /* The NXAST_CLONE action is "struct ext_action_header", followed by zero or
  * more embedded OpenFlow actions. */
@@ -5451,6 +5914,22 @@ format_CLONE(const struct ofpact_nest *a,
     ofpacts_format(a->actions, ofpact_nest_get_action_len(a), fp);
     ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
 }
+
+static enum ofperr
+check_subactions(struct ofpact *ofpacts, size_t ofpacts_len,
+                 struct ofpact_check_params *cp)
+{
+    struct ofpact_check_params sub = *cp;
+    enum ofperr error = ofpacts_check(ofpacts, ofpacts_len, &sub);
+    cp->usable_protocols &= sub.usable_protocols;
+    return error;
+}
+
+static enum ofperr
+check_CLONE(struct ofpact_nest *a, struct ofpact_check_params *cp)
+{
+    return check_subactions(a->actions, ofpact_nest_get_action_len(a), cp);
+}
 
 /* Action structure for NXAST_SAMPLE.
  *
@@ -5676,6 +6155,13 @@ format_SAMPLE(const struct ofpact_sample *a,
     }
     ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
 }
+
+static enum ofperr
+check_SAMPLE(const struct ofpact_sample *a OVS_UNUSED,
+             const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* debug instructions. */
 
@@ -5720,6 +6206,13 @@ format_DEBUG_RECIRC(const struct ofpact_null *a OVS_UNUSED,
     ds_put_format(fp->s, "%sdebug_recirc%s", colors.value, colors.end);
 }
 
+static enum ofperr
+check_DEBUG_RECIRC(const struct ofpact_null *a OVS_UNUSED,
+                   const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
+
 static enum ofperr
 decode_NXAST_RAW_DEBUG_SLOW(struct ofpbuf *out)
 {
@@ -5753,6 +6246,13 @@ format_DEBUG_SLOW(const struct ofpact_null *a OVS_UNUSED,
     ds_put_format(fp->s, "%sdebug_slow%s", colors.value, colors.end);
 }
 
+static enum ofperr
+check_DEBUG_SLOW(const struct ofpact_null *a OVS_UNUSED,
+                 const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
+
 /* Action structure for NXAST_CT.
  *
  * Pass traffic to the connection tracker.
@@ -6157,6 +6657,28 @@ format_CT(const struct ofpact_conntrack *a,
     ds_chomp(fp->s, ',');
     ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
 }
+
+static enum ofperr
+check_CT(struct ofpact_conntrack *a, struct ofpact_check_params *cp)
+{
+    struct flow *flow = &cp->match->flow;
+
+    if (!dl_type_is_ip_any(get_dl_type(flow))
+        || (flow->ct_state & CS_INVALID && a->flags & NX_CT_F_COMMIT)
+        || (a->alg == IPPORT_FTP && flow->nw_proto != IPPROTO_TCP)
+        || (a->alg == IPPORT_TFTP && flow->nw_proto != IPPROTO_UDP)) {
+        /* We can't downgrade to OF1.0 and expect inconsistent CT actions
+         * be silently discarded.  Instead, datapath flow install fails, so
+         * it is better to flag inconsistent CT actions as hard errors. */
+        return OFPERR_OFPBAC_MATCH_INCONSISTENT;
+    }
+
+    if (a->zone_src.field) {
+        return mf_check_src(&a->zone_src, cp->match);
+    }
+
+    return check_subactions(a->actions, ofpact_ct_get_action_len(a), cp);
+}
 
 /* ct_clear action. */
 
@@ -6188,6 +6710,13 @@ format_CT_CLEAR(const struct ofpact_null *a OVS_UNUSED,
 {
     ds_put_format(fp->s, "%sct_clear%s", colors.value, colors.end);
 }
+
+static enum ofperr
+check_CT_CLEAR(const struct ofpact_null *a OVS_UNUSED,
+               const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* NAT action. */
 
@@ -6541,6 +7070,18 @@ parse_NAT(char *arg, const struct ofpact_parse_params *pp)
     return NULL;
 }
 
+static enum ofperr
+check_NAT(const struct ofpact_nat *a, const struct ofpact_check_params *cp)
+{
+    ovs_be16 dl_type = get_dl_type(&cp->match->flow);
+    if (!dl_type_is_ip_any(dl_type) ||
+        (a->range_af == AF_INET && dl_type != htons(ETH_TYPE_IP)) ||
+        (a->range_af == AF_INET6 && dl_type != htons(ETH_TYPE_IPV6))) {
+        return OFPERR_OFPBAC_MATCH_INCONSISTENT;
+    }
+    return 0;
+}
+
 /* Truncate output action. */
 struct nx_action_output_trunc {
     ovs_be16 type;              /* OFPAT_VENDOR. */
@@ -6598,6 +7139,12 @@ format_OUTPUT_TRUNC(const struct ofpact_output_trunc *a,
     ds_put_format(fp->s, ",max_len=%"PRIu32")", a->max_len);
 }
 
+static enum ofperr
+check_OUTPUT_TRUNC(const struct ofpact_output_trunc *a,
+                   const struct ofpact_check_params *cp)
+{
+    return ofpact_check_output_port(a->port, cp->max_ports);
+}
 
 /* Meter instruction. */
 
@@ -6624,6 +7171,14 @@ format_METER(const struct ofpact_meter *a,
     ds_put_format(fp->s, "%smeter:%s%"PRIu32,
                   colors.param, colors.end, a->meter_id);
 }
+
+static enum ofperr
+check_METER(const struct ofpact_meter *a,
+            const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    uint32_t mid = a->meter_id;
+    return mid == 0 || mid > OFPM13_MAX ? OFPERR_OFPMMFC_INVALID_METER : 0;
+}
 
 /* Clear-Actions instruction. */
 
@@ -6650,6 +7205,13 @@ format_CLEAR_ACTIONS(const struct ofpact_null *a OVS_UNUSED,
 {
     ds_put_format(fp->s, "%sclear_actions%s", colors.value, colors.end);
 }
+
+static enum ofperr
+check_CLEAR_ACTIONS(const struct ofpact_null *a OVS_UNUSED,
+                    const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Write-Actions instruction. */
 
@@ -6706,6 +7268,16 @@ format_WRITE_ACTIONS(const struct ofpact_nest *a,
     ofpacts_format(a->actions, ofpact_nest_get_action_len(a), fp);
     ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
 }
+
+static enum ofperr
+check_WRITE_ACTIONS(struct ofpact_nest *a,
+                    const struct ofpact_check_params *cp)
+{
+    /* Use a temporary copy of 'cp' to avoid updating 'cp->usable_protocols',
+     * since we can't check consistency of an action set. */
+    struct ofpact_check_params tmp = *cp;
+    return ofpacts_check(a->actions, ofpact_nest_get_action_len(a), &tmp);
+}
 
 /* Action structure for NXAST_WRITE_METADATA.
  *
@@ -6792,6 +7364,13 @@ format_WRITE_METADATA(const struct ofpact_metadata *a,
         ds_put_format(fp->s, "/%#"PRIx64, ntohll(a->mask));
     }
 }
+
+static enum ofperr
+check_WRITE_METADATA(const struct ofpact_metadata *a OVS_UNUSED,
+                     const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
 
 /* Goto-Table instruction. */
 
@@ -6831,6 +7410,17 @@ format_GOTO_TABLE(const struct ofpact_goto_table *a,
     ds_put_format(fp->s, "%sgoto_table:%s", colors.param, colors.end);
     ofputil_format_table(a->table_id, fp->table_map, fp->s);
 }
+
+static enum ofperr
+check_GOTO_TABLE(const struct ofpact_goto_table *a,
+                 const struct ofpact_check_params *cp)
+{
+    if ((cp->table_id != 255 && a->table_id <= cp->table_id)
+        || (cp->n_tables != 255 && a->table_id >= cp->n_tables)) {
+        return OFPERR_OFPBIC_BAD_TABLE_ID;
+    }
+    return 0;
+}
 
 static void
 log_bad_action(const struct ofp_action_header *actions, size_t actions_len,
@@ -7693,333 +8283,14 @@ inconsistent_match(enum ofputil_protocol *usable_protocols)
  * Modifies some actions, filling in fields that could not be properly set
  * without context. */
 static enum ofperr
-ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
-               struct match *match, ofp_port_t max_ports,
-               uint8_t table_id, uint8_t n_tables)
+ofpact_check__(struct ofpact *a, struct ofpact_check_params *cp)
 {
-    struct flow *flow = &match->flow;
-    const struct ofpact_enqueue *enqueue;
-    const struct mf_field *mf;
-    ovs_be16 dl_type = get_dl_type(flow);
-
     switch (a->type) {
-    case OFPACT_OUTPUT:
-        return ofpact_check_output_port(ofpact_get_OUTPUT(a)->port,
-                                        max_ports);
-
-    case OFPACT_CONTROLLER:
-        return 0;
-
-    case OFPACT_ENQUEUE:
-        enqueue = ofpact_get_ENQUEUE(a);
-        if (ofp_to_u16(enqueue->port) >= ofp_to_u16(max_ports)
-            && enqueue->port != OFPP_IN_PORT
-            && enqueue->port != OFPP_LOCAL) {
-            return OFPERR_OFPBAC_BAD_OUT_PORT;
-        }
-        return 0;
-
-    case OFPACT_OUTPUT_REG:
-        return mf_check_src(&ofpact_get_OUTPUT_REG(a)->src, match);
-
-    case OFPACT_OUTPUT_TRUNC:
-        return ofpact_check_output_port(ofpact_get_OUTPUT_TRUNC(a)->port,
-                                        max_ports);
-
-    case OFPACT_BUNDLE:
-        return bundle_check(ofpact_get_BUNDLE(a), max_ports, match);
-
-    case OFPACT_SET_VLAN_VID:
-        /* Remember if we saw a vlan tag in the flow to aid translating to
-         * OpenFlow 1.1+ if need be. */
-        ofpact_get_SET_VLAN_VID(a)->flow_has_vlan =
-            (flow->vlans[0].tci & htons(VLAN_CFI)) == htons(VLAN_CFI);
-        if (!(flow->vlans[0].tci & htons(VLAN_CFI)) &&
-            !ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
-            inconsistent_match(usable_protocols);
-        }
-        /* Temporary mark that we have a vlan tag. */
-        flow->vlans[0].tci |= htons(VLAN_CFI);
-        return 0;
-
-    case OFPACT_SET_VLAN_PCP:
-        /* Remember if we saw a vlan tag in the flow to aid translating to
-         * OpenFlow 1.1+ if need be. */
-        ofpact_get_SET_VLAN_PCP(a)->flow_has_vlan =
-            (flow->vlans[0].tci & htons(VLAN_CFI)) == htons(VLAN_CFI);
-        if (!(flow->vlans[0].tci & htons(VLAN_CFI)) &&
-            !ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
-            inconsistent_match(usable_protocols);
-        }
-        /* Temporary mark that we have a vlan tag. */
-        flow->vlans[0].tci |= htons(VLAN_CFI);
-        return 0;
-
-    case OFPACT_STRIP_VLAN:
-        if (!(flow->vlans[0].tci & htons(VLAN_CFI))) {
-            inconsistent_match(usable_protocols);
-        }
-        flow_pop_vlan(flow, NULL);
-        return 0;
-
-    case OFPACT_PUSH_VLAN:
-        if (flow->vlans[FLOW_MAX_VLAN_HEADERS - 1].tci & htons(VLAN_CFI)) {
-            /* Support maximum (FLOW_MAX_VLAN_HEADERS) VLAN headers. */
-            return OFPERR_OFPBAC_BAD_TAG;
-        }
-        /* Temporary mark that we have a vlan tag. */
-        flow_push_vlan_uninit(flow, NULL);
-        flow->vlans[0].tci |= htons(VLAN_CFI);
-        return 0;
-
-    case OFPACT_SET_ETH_SRC:
-    case OFPACT_SET_ETH_DST:
-        return 0;
-
-    case OFPACT_SET_IPV4_SRC:
-    case OFPACT_SET_IPV4_DST:
-        if (dl_type != htons(ETH_TYPE_IP)) {
-            inconsistent_match(usable_protocols);
-        }
-        return 0;
-
-    case OFPACT_SET_IP_DSCP:
-    case OFPACT_SET_IP_ECN:
-    case OFPACT_SET_IP_TTL:
-    case OFPACT_DEC_TTL:
-        if (!is_ip_any(flow)) {
-            inconsistent_match(usable_protocols);
-        }
-        return 0;
-
-    case OFPACT_SET_L4_SRC_PORT:
-    case OFPACT_SET_L4_DST_PORT:
-        if (!is_ip_any(flow) || (flow->nw_frag & FLOW_NW_FRAG_LATER) ||
-            (flow->nw_proto != IPPROTO_TCP && flow->nw_proto != IPPROTO_UDP
-             && flow->nw_proto != IPPROTO_SCTP)) {
-            inconsistent_match(usable_protocols);
-        }
-        /* Note on which transport protocol the port numbers are set.
-         * This allows this set action to be converted to an OF1.2 set field
-         * action. */
-        if (a->type == OFPACT_SET_L4_SRC_PORT) {
-            ofpact_get_SET_L4_SRC_PORT(a)->flow_ip_proto = flow->nw_proto;
-        } else {
-            ofpact_get_SET_L4_DST_PORT(a)->flow_ip_proto = flow->nw_proto;
-        }
-        return 0;
-
-    case OFPACT_REG_MOVE:
-        return nxm_reg_move_check(ofpact_get_REG_MOVE(a), match);
-
-    case OFPACT_SET_FIELD:
-        mf = ofpact_get_SET_FIELD(a)->field;
-        /* Require OXM_OF_VLAN_VID to have an existing VLAN header. */
-        if (!mf_are_prereqs_ok(mf, flow, NULL) ||
-            (mf->id == MFF_VLAN_VID &&
-             !(flow->vlans[0].tci & htons(VLAN_CFI)))) {
-            VLOG_WARN_RL(&rl, "set_field %s lacks correct prerequisites",
-                         mf->name);
-            return OFPERR_OFPBAC_MATCH_INCONSISTENT;
-        }
-        /* Remember if we saw a vlan tag in the flow to aid translating to
-         * OpenFlow 1.1 if need be. */
-        ofpact_get_SET_FIELD(a)->flow_has_vlan =
-            (flow->vlans[0].tci & htons(VLAN_CFI)) == htons(VLAN_CFI);
-        if (mf->id == MFF_VLAN_TCI) {
-            /* The set field may add or remove the vlan tag,
-             * Mark the status temporarily. */
-            flow->vlans[0].tci = ofpact_get_SET_FIELD(a)->value->be16;
-        }
-        return 0;
-
-    case OFPACT_STACK_PUSH:
-        return nxm_stack_push_check(ofpact_get_STACK_PUSH(a), match);
-
-    case OFPACT_STACK_POP:
-        return nxm_stack_pop_check(ofpact_get_STACK_POP(a), match);
-
-    case OFPACT_SET_MPLS_LABEL:
-    case OFPACT_SET_MPLS_TC:
-    case OFPACT_SET_MPLS_TTL:
-    case OFPACT_DEC_MPLS_TTL:
-        if (!eth_type_mpls(dl_type)) {
-            inconsistent_match(usable_protocols);
-        }
-        return 0;
-
-    case OFPACT_SET_TUNNEL:
-    case OFPACT_SET_QUEUE:
-    case OFPACT_POP_QUEUE:
-        return 0;
-
-    case OFPACT_RESUBMIT: {
-        struct ofpact_resubmit *resubmit = ofpact_get_RESUBMIT(a);
-
-        if (resubmit->with_ct_orig && !is_ct_valid(flow, &match->wc, NULL)) {
-            return OFPERR_OFPBAC_MATCH_INCONSISTENT;
-        }
-        return 0;
-    }
-    case OFPACT_FIN_TIMEOUT:
-        if (flow->nw_proto != IPPROTO_TCP) {
-            inconsistent_match(usable_protocols);
-        }
-        return 0;
-
-    case OFPACT_LEARN:
-        return learn_check(ofpact_get_LEARN(a), match);
-
-    case OFPACT_CONJUNCTION:
-        return 0;
-
-    case OFPACT_MULTIPATH:
-        return multipath_check(ofpact_get_MULTIPATH(a), match);
-
-    case OFPACT_NOTE:
-    case OFPACT_EXIT:
-        return 0;
-
-    case OFPACT_PUSH_MPLS:
-        if (flow->packet_type != htonl(PT_ETH)) {
-            inconsistent_match(usable_protocols);
-        }
-        flow->dl_type = ofpact_get_PUSH_MPLS(a)->ethertype;
-        /* The packet is now MPLS and the MPLS payload is opaque.
-         * Thus nothing can be assumed about the network protocol.
-         * Temporarily mark that we have no nw_proto. */
-        flow->nw_proto = 0;
-        return 0;
-
-    case OFPACT_POP_MPLS:
-        if (flow->packet_type != htonl(PT_ETH)
-            || !eth_type_mpls(dl_type)) {
-            inconsistent_match(usable_protocols);
-        }
-        flow->dl_type = ofpact_get_POP_MPLS(a)->ethertype;
-        return 0;
-
-    case OFPACT_SAMPLE:
-        return 0;
-
-    case OFPACT_CLONE: {
-        struct ofpact_nest *on = ofpact_get_CLONE(a);
-        return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
-                             match, max_ports, table_id, n_tables,
-                             usable_protocols);
-    }
-
-    case OFPACT_CT: {
-        struct ofpact_conntrack *oc = ofpact_get_CT(a);
-
-        if (!dl_type_is_ip_any(dl_type)
-            || (flow->ct_state & CS_INVALID && oc->flags & NX_CT_F_COMMIT)
-            || (oc->alg == IPPORT_FTP && flow->nw_proto != IPPROTO_TCP)
-            || (oc->alg == IPPORT_TFTP && flow->nw_proto != IPPROTO_UDP)) {
-            /* We can't downgrade to OF1.0 and expect inconsistent CT actions
-             * be silently discarded.  Instead, datapath flow install fails, so
-             * it is better to flag inconsistent CT actions as hard errors. */
-            return OFPERR_OFPBAC_MATCH_INCONSISTENT;
-        }
-
-        if (oc->zone_src.field) {
-            return mf_check_src(&oc->zone_src, match);
-        }
-
-        return ofpacts_check(oc->actions, ofpact_ct_get_action_len(oc),
-                             match, max_ports, table_id, n_tables,
-                             usable_protocols);
-    }
-
-    case OFPACT_CT_CLEAR:
-        return 0;
-
-    case OFPACT_NAT: {
-        struct ofpact_nat *on = ofpact_get_NAT(a);
-
-        if (!dl_type_is_ip_any(dl_type) ||
-            (on->range_af == AF_INET && dl_type != htons(ETH_TYPE_IP)) ||
-            (on->range_af == AF_INET6
-             && dl_type != htons(ETH_TYPE_IPV6))) {
-            return OFPERR_OFPBAC_MATCH_INCONSISTENT;
-        }
-        return 0;
-    }
-
-    case OFPACT_CLEAR_ACTIONS:
-        return 0;
-
-    case OFPACT_WRITE_ACTIONS: {
-        /* Use a temporary copy of 'usable_protocols' because we can't check
-         * consistency of an action set. */
-        struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
-        enum ofputil_protocol p = *usable_protocols;
-        return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
-                             match, max_ports, table_id, n_tables, &p);
-    }
-
-    case OFPACT_WRITE_METADATA:
-        return 0;
-
-    case OFPACT_METER: {
-        uint32_t mid = ofpact_get_METER(a)->meter_id;
-        if (mid == 0 || mid > OFPM13_MAX) {
-            return OFPERR_OFPMMFC_INVALID_METER;
-        }
-        return 0;
-    }
-
-    case OFPACT_GOTO_TABLE: {
-        uint8_t goto_table = ofpact_get_GOTO_TABLE(a)->table_id;
-        if ((table_id != 255 && goto_table <= table_id)
-            || (n_tables != 255 && goto_table >= n_tables)) {
-            return OFPERR_OFPBIC_BAD_TABLE_ID;
-        }
-        return 0;
-    }
-
-    case OFPACT_GROUP:
-        return 0;
-
-    case OFPACT_UNROLL_XLATE:
-        /* UNROLL is an internal action that should never be seen via
-         * OpenFlow. */
-        return OFPERR_OFPBAC_BAD_TYPE;
-
-    case OFPACT_DEBUG_RECIRC:
-    case OFPACT_DEBUG_SLOW:
-        return 0;
-
-    case OFPACT_ENCAP:
-        flow->packet_type = ofpact_get_ENCAP(a)->new_pkt_type;
-        if (pt_ns(flow->packet_type) == OFPHTN_ETHERTYPE) {
-            flow->dl_type = htons(pt_ns_type(flow->packet_type));
-        }
-        if (!is_ip_any(flow)) {
-            flow->nw_proto = 0;
-        }
-        return 0;
-
-    case OFPACT_DECAP:
-        if (flow->packet_type == htonl(PT_ETH)) {
-            /* Adjust the packet_type to allow subsequent actions. */
-            flow->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE,
-                                               ntohs(flow->dl_type));
-        } else {
-            /* The actual packet_type is only known after decapsulation.
-             * Do not allow subsequent actions that depend on packet headers. */
-            flow->packet_type = htonl(PT_UNKNOWN);
-            flow->dl_type = OVS_BE16_MAX;
-        }
-        return 0;
-
-    case OFPACT_DEC_NSH_TTL:
-        if ((flow->packet_type != htonl(PT_NSH)) &&
-            (flow->dl_type != htons(ETH_TYPE_NSH))) {
-            inconsistent_match(usable_protocols);
-        }
-        return 0;
-
+#define OFPACT(ENUM, STRUCT, MEMBER, NAME)                  \
+        case OFPACT_##ENUM:                                 \
+            return check_##ENUM(ofpact_get_##ENUM(a), cp);
+        OFPACTS
+#undef OFPACT
     default:
         OVS_NOT_REACHED();
     }
@@ -8040,31 +8311,33 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
  * returning. */
 enum ofperr
 ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len,
-              struct match *match, ofp_port_t max_ports,
-              uint8_t table_id, uint8_t n_tables,
-              enum ofputil_protocol *usable_protocols)
+              struct ofpact_check_params *cp)
 {
-    struct ofpact *a;
-    ovs_be32 packet_type = match->flow.packet_type;
-    ovs_be16 dl_type = match->flow.dl_type;
-    uint8_t nw_proto = match->flow.nw_proto;
-    enum ofperr error = 0;
+    /* Save fields that might temporarily be modified. */
+    struct flow *flow = &cp->match->flow;
+    ovs_be32 packet_type = flow->packet_type;
+    ovs_be16 dl_type = flow->dl_type;
+    uint8_t nw_proto = flow->nw_proto;
     union flow_vlan_hdr vlans[FLOW_MAX_VLAN_HEADERS];
+    memcpy(vlans, flow->vlans, sizeof vlans);
 
-    memcpy(&vlans, &match->flow.vlans, sizeof(vlans));
-
+    /* Check all the actions. */
+    cp->usable_protocols = OFPUTIL_P_ANY;
+    enum ofperr error = 0;
+    struct ofpact *a;
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
-        error = ofpact_check__(usable_protocols, a, match,
-                               max_ports, table_id, n_tables);
+        error = ofpact_check__(a, cp);
         if (error) {
             break;
         }
     }
+
     /* Restore fields that may have been modified. */
-    match->flow.packet_type = packet_type;
-    match->flow.dl_type = dl_type;
-    memcpy(&match->flow.vlans, &vlans, sizeof(vlans));
-    match->flow.nw_proto = nw_proto;
+    flow->packet_type = packet_type;
+    flow->dl_type = dl_type;
+    memcpy(flow->vlans, vlans, sizeof vlans);
+    flow->nw_proto = nw_proto;
+
     return error;
 }
 
@@ -8072,18 +8345,14 @@ ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len,
  * OFPERR_OFPBAC_MATCH_INCONSISTENT rather than clearing bits. */
 enum ofperr
 ofpacts_check_consistency(struct ofpact ofpacts[], size_t ofpacts_len,
-                          struct match *match, ofp_port_t max_ports,
-                          uint8_t table_id, uint8_t n_tables,
-                          enum ofputil_protocol usable_protocols)
+                          enum ofputil_protocol needed_protocols,
+                          struct ofpact_check_params *cp)
 {
-    enum ofputil_protocol p = usable_protocols;
-    enum ofperr error;
-
-    error = ofpacts_check(ofpacts, ofpacts_len, match, max_ports,
-                          table_id, n_tables, &p);
-    return (error ? error
-            : p != usable_protocols ? OFPERR_OFPBAC_MATCH_INCONSISTENT
-            : 0);
+    enum ofperr error = ofpacts_check(ofpacts, ofpacts_len, cp);
+    if (!error && needed_protocols & ~cp->usable_protocols) {
+        return OFPERR_OFPBAC_MATCH_INCONSISTENT;
+    }
+    return error;
 }
 
 /* Returns the destination field that 'ofpact' would write to, or NULL
diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c
index 4b893ce959d4..f6b3f556eadf 100644
--- a/lib/ofp-flow.c
+++ b/lib/ofp-flow.c
@@ -343,9 +343,14 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
                 : OFPERR_OFPFMFC_TABLE_FULL);
     }
 
+    struct ofpact_check_params cp = {
+        .match = &match,
+        .max_ports = max_port,
+        .table_id = fm->table_id,
+        .n_tables = max_table
+    };
     error = ofpacts_check_consistency(fm->ofpacts, fm->ofpacts_len,
-                                      &match, max_port,
-                                      fm->table_id, max_table, protocol);
+                                      protocol, &cp);
     if (!error) {
         minimatch_init(&fm->match, &match);
     }
@@ -1723,8 +1728,14 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
         if (!error) {
             enum ofperr err;
 
-            err = ofpacts_check(ofpacts.data, ofpacts.size, &match,
-                                OFPP_MAX, fm->table_id, 255, usable_protocols);
+            struct ofpact_check_params cp = {
+                .match = &match,
+                .max_ports = OFPP_MAX,
+                .table_id = fm->table_id,
+                .n_tables = 255,
+            };
+            err = ofpacts_check(ofpacts.data, ofpacts.size, &cp);
+            *usable_protocols &= cp.usable_protocols;
             if (!err && !*usable_protocols) {
                 err = OFPERR_OFPBAC_MATCH_INCONSISTENT;
             }
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
index 5f38cae16e22..e8fe0c6c17e9 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -538,16 +538,16 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
         unixctl_command_reply_error(conn, "invalid in_port");
         goto exit;
     }
-    if (enforce_consistency) {
-        retval = ofpacts_check_consistency(ofpacts.data, ofpacts.size, &match,
-                                           u16_to_ofp(ofproto->up.max_ports),
-                                           0, ofproto->up.n_tables,
-                                           usable_protocols);
-    } else {
-        retval = ofpacts_check(ofpacts.data, ofpacts.size, &match,
-                               u16_to_ofp(ofproto->up.max_ports), 0,
-                               ofproto->up.n_tables, &usable_protocols);
-    }
+
+    struct ofpact_check_params cp = {
+        .match = &match,
+        .max_ports = u16_to_ofp(ofproto->up.max_ports),
+        .table_id = 0,
+        .n_tables = ofproto->up.n_tables,
+    };
+    retval = ofpacts_check_consistency(
+        ofpacts.data, ofpacts.size,
+        enforce_consistency ? usable_protocols : 0, &cp);
     if (!retval) {
         ovs_mutex_lock(&ofproto_mutex);
         retval = ofproto_check_ofpacts(&ofproto->up, ofpacts.data,
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 829ccd8663a7..928180c2d6a8 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3493,10 +3493,14 @@ ofproto_packet_out_init(struct ofproto *ofproto,
      * check instructions (e.g., goto-table), which can't appear on the action
      * list of a packet-out. */
     match_wc_init(&match, opo->flow);
-    error = ofpacts_check_consistency(po->ofpacts, po->ofpacts_len, &match,
-                                      u16_to_ofp(ofproto->max_ports), 0,
-                                      ofproto->n_tables,
-                                      ofconn_get_protocol(ofconn));
+    struct ofpact_check_params cp = {
+        .match = &match,
+        .max_ports = u16_to_ofp(ofproto->max_ports),
+        .table_id = 0,
+        .n_tables = ofproto->n_tables
+    };
+    error = ofpacts_check_consistency(po->ofpacts, po->ofpacts_len,
+                                      ofconn_get_protocol(ofconn), &cp);
     if (error) {
         dp_packet_delete(opo->packet);
         free(opo->flow);
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 029a1c8b97a1..47dbe0b11ca3 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -4250,15 +4250,16 @@ ofctl_parse_actions__(const char *version_s, bool instructions)
                      &of_in, of_in.size, version, NULL, NULL, &ofpacts);
         if (!error && instructions) {
             /* Verify actions, enforce consistency. */
-            enum ofputil_protocol protocol;
-            struct match match;
-
-            memset(&match, 0, sizeof match);
-            protocol = ofputil_protocols_from_ofp_version(version);
-            error = ofpacts_check_consistency(ofpacts.data, ofpacts.size,
-                                              &match, OFPP_MAX,
-                                              table_id ? atoi(table_id) : 0,
-                                              OFPTT_MAX + 1, protocol);
+            struct match match = MATCH_CATCHALL_INITIALIZER;
+            struct ofpact_check_params cp = {
+                .match = &match,
+                .max_ports = OFPP_MAX,
+                .table_id = table_id ? atoi(table_id) : 0,
+                .n_tables = OFPTT_MAX + 1,
+            };
+            error = ofpacts_check_consistency(
+                ofpacts.data, ofpacts.size,
+                ofputil_protocols_from_ofp_version(version), &cp);
         }
         if (error) {
             printf("bad %s %s: %s\n\n",
-- 
2.16.1



More information about the dev mailing list