Native Set-Field action.
[cascardo/ovs.git] / lib / ofp-actions.c
index ae63d00..761cea5 100644 (file)
@@ -501,6 +501,8 @@ ofpact_from_openflow10(const union ofp_action *a, struct ofpbuf *out)
 {
     enum ofputil_action_code code;
     enum ofperr error;
+    struct ofpact_vlan_vid *vlan_vid;
+    struct ofpact_vlan_pcp *vlan_pcp;
 
     error = decode_openflow10_action(a, &code);
     if (error) {
@@ -520,18 +522,24 @@ ofpact_from_openflow10(const union ofp_action *a, struct ofpbuf *out)
         if (a->vlan_vid.vlan_vid & ~htons(0xfff)) {
             return OFPERR_OFPBAC_BAD_ARGUMENT;
         }
-        ofpact_put_SET_VLAN_VID(out)->vlan_vid = ntohs(a->vlan_vid.vlan_vid);
+        vlan_vid = ofpact_put_SET_VLAN_VID(out);
+        vlan_vid->vlan_vid = ntohs(a->vlan_vid.vlan_vid);
+        vlan_vid->push_vlan_if_needed = true;
+        vlan_vid->ofpact.compat = code;
         break;
 
     case OFPUTIL_OFPAT10_SET_VLAN_PCP:
         if (a->vlan_pcp.vlan_pcp & ~7) {
             return OFPERR_OFPBAC_BAD_ARGUMENT;
         }
-        ofpact_put_SET_VLAN_PCP(out)->vlan_pcp = a->vlan_pcp.vlan_pcp;
+        vlan_pcp = ofpact_put_SET_VLAN_PCP(out);
+        vlan_pcp->vlan_pcp = a->vlan_pcp.vlan_pcp;
+        vlan_pcp->push_vlan_if_needed = true;
+        vlan_pcp->ofpact.compat = code;
         break;
 
     case OFPUTIL_OFPAT10_STRIP_VLAN:
-        ofpact_put_STRIP_VLAN(out);
+        ofpact_put_STRIP_VLAN(out)->ofpact.compat = code;
         break;
 
     case OFPUTIL_OFPAT10_SET_DL_SRC:
@@ -751,6 +759,117 @@ decode_openflow11_action(const union ofp_action *a,
     }
 }
 
+static enum ofperr
+set_field_from_openflow(const struct ofp12_action_set_field *oasf,
+                        struct ofpbuf *ofpacts)
+{
+    uint16_t oasf_len = ntohs(oasf->len);
+    uint32_t oxm_header = ntohl(oasf->dst);
+    uint8_t oxm_length = NXM_LENGTH(oxm_header);
+    struct ofpact_set_field *sf;
+    const struct mf_field *mf;
+
+    /* ofp12_action_set_field is padded to 64 bits by zero */
+    if (oasf_len != ROUND_UP(sizeof *oasf + oxm_length, 8)) {
+        return OFPERR_OFPBAC_BAD_SET_LEN;
+    }
+    if (!is_all_zeros((const uint8_t *)oasf + sizeof *oasf + oxm_length,
+                      oasf_len - oxm_length - sizeof *oasf)) {
+        return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
+    }
+
+    if (NXM_HASMASK(oxm_header)) {
+        return OFPERR_OFPBAC_BAD_SET_TYPE;
+    }
+    mf = mf_from_nxm_header(oxm_header);
+    if (!mf) {
+        return OFPERR_OFPBAC_BAD_SET_TYPE;
+    }
+    ovs_assert(mf->n_bytes == oxm_length);
+    /* oxm_length is now validated to be compatible with mf_value. */
+    if (!mf->writable) {
+        VLOG_WARN_RL(&rl, "destination field %s is not writable", mf->name);
+        return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
+    }
+    sf = ofpact_put_SET_FIELD(ofpacts);
+    sf->field = mf;
+    memcpy(&sf->value, oasf + 1, mf->n_bytes);
+
+    /* The value must be valid for match and must have the OFPVID_PRESENT bit
+     * on for OXM_OF_VLAN_VID. */
+    if (!mf_is_value_valid(mf, &sf->value)
+        || (mf->id == MFF_VLAN_VID
+            && !(sf->value.be16 & htons(OFPVID12_PRESENT)))) {
+        struct ds ds = DS_EMPTY_INITIALIZER;
+        mf_format(mf, &sf->value, NULL, &ds);
+        VLOG_WARN_RL(&rl, "Invalid value for set field %s: %s",
+                     mf->name, ds_cstr(&ds));
+        ds_destroy(&ds);
+
+        return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
+    }
+    return 0;
+}
+
+static void
+set_field_to_openflow12(const struct ofpact_set_field *sf,
+                        struct ofpbuf *openflow)
+{
+    uint16_t padded_value_len = ROUND_UP(sf->field->n_bytes, 8);
+    struct ofp12_action_set_field *oasf;
+    char *value;
+
+    oasf = ofputil_put_OFPAT12_SET_FIELD(openflow);
+    oasf->dst = htonl(sf->field->oxm_header);
+    oasf->len = htons(sizeof *oasf + padded_value_len);
+
+    value = ofpbuf_put_zeros(openflow, padded_value_len);
+    memcpy(value, &sf->value, sf->field->n_bytes);
+}
+
+/* Convert 'sf' to one or two REG_LOADs. */
+static void
+set_field_to_nxast(const struct ofpact_set_field *sf, struct ofpbuf *openflow)
+{
+    const struct mf_field *mf = sf->field;
+    struct nx_action_reg_load *narl;
+
+    if (mf->n_bits > 64) {
+        ovs_assert(mf->n_bytes == 16); /* IPv6 addr. */
+        /* Split into 64bit chunks */
+        /* Lower bits first. */
+        narl = ofputil_put_NXAST_REG_LOAD(openflow);
+        narl->ofs_nbits = nxm_encode_ofs_nbits(0, 64);
+        narl->dst = htonl(mf->nxm_header);
+        memcpy(&narl->value, &sf->value.ipv6.s6_addr[8], sizeof narl->value);
+        /* Higher bits next. */
+        narl = ofputil_put_NXAST_REG_LOAD(openflow);
+        narl->ofs_nbits = nxm_encode_ofs_nbits(64, mf->n_bits - 64);
+        narl->dst = htonl(mf->nxm_header);
+        memcpy(&narl->value, &sf->value.ipv6.s6_addr[0], sizeof narl->value);
+    } else {
+        narl = ofputil_put_NXAST_REG_LOAD(openflow);
+        narl->ofs_nbits = nxm_encode_ofs_nbits(0, mf->n_bits);
+        narl->dst = htonl(mf->nxm_header);
+        memset(&narl->value, 0, 8 - mf->n_bytes);
+        memcpy((char*)&narl->value + (8 - mf->n_bytes),
+               &sf->value, mf->n_bytes);
+    }
+}
+
+static void
+set_field_to_openflow(const struct ofpact_set_field *sf,
+                      struct ofpbuf *openflow)
+{
+    struct ofp_header *oh = (struct ofp_header *)openflow->l2;
+
+    if (oh->version >= OFP12_VERSION) {
+        set_field_to_openflow12(sf, openflow);
+    } else {
+        set_field_to_nxast(sf, openflow);
+    }
+}
+
 static enum ofperr
 output_from_openflow11(const struct ofp11_action_output *oao,
                        struct ofpbuf *out)
@@ -774,6 +893,8 @@ ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out)
 {
     enum ofputil_action_code code;
     enum ofperr error;
+    struct ofpact_vlan_vid *vlan_vid;
+    struct ofpact_vlan_pcp *vlan_pcp;
 
     error = decode_openflow11_action(a, &code);
     if (error) {
@@ -793,14 +914,20 @@ ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out)
         if (a->vlan_vid.vlan_vid & ~htons(0xfff)) {
             return OFPERR_OFPBAC_BAD_ARGUMENT;
         }
-        ofpact_put_SET_VLAN_VID(out)->vlan_vid = ntohs(a->vlan_vid.vlan_vid);
+        vlan_vid = ofpact_put_SET_VLAN_VID(out);
+        vlan_vid->vlan_vid = ntohs(a->vlan_vid.vlan_vid);
+        vlan_vid->push_vlan_if_needed = false;
+        vlan_vid->ofpact.compat = code;
         break;
 
     case OFPUTIL_OFPAT11_SET_VLAN_PCP:
         if (a->vlan_pcp.vlan_pcp & ~7) {
             return OFPERR_OFPBAC_BAD_ARGUMENT;
         }
-        ofpact_put_SET_VLAN_PCP(out)->vlan_pcp = a->vlan_pcp.vlan_pcp;
+        vlan_pcp = ofpact_put_SET_VLAN_PCP(out);
+        vlan_pcp->vlan_pcp = a->vlan_pcp.vlan_pcp;
+        vlan_pcp->push_vlan_if_needed = false;
+        vlan_pcp->ofpact.compat = code;
         break;
 
     case OFPUTIL_OFPAT11_PUSH_VLAN:
@@ -812,7 +939,7 @@ ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out)
         break;
 
     case OFPUTIL_OFPAT11_POP_VLAN:
-        ofpact_put_STRIP_VLAN(out);
+        ofpact_put_STRIP_VLAN(out)->ofpact.compat = code;
         break;
 
     case OFPUTIL_OFPAT11_SET_QUEUE:
@@ -869,7 +996,7 @@ ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out)
         break;
 
     case OFPUTIL_OFPAT12_SET_FIELD:
-        return nxm_reg_load_from_openflow12_set_field(&a->set_field, out);
+        return set_field_from_openflow(&a->set_field, out);
 
     case OFPUTIL_OFPAT11_SET_MPLS_TTL:
         ofpact_put_SET_MPLS_TTL(out)->ttl = a->ofp11_mpls_ttl.mpls_ttl;
@@ -917,6 +1044,7 @@ static bool
 ofpact_is_set_action(const struct ofpact *a)
 {
     switch (a->type) {
+    case OFPACT_SET_FIELD:
     case OFPACT_REG_LOAD:
     case OFPACT_SET_ETH_DST:
     case OFPACT_SET_ETH_SRC:
@@ -981,6 +1109,7 @@ ofpact_is_allowed_in_actions_set(const struct ofpact *a)
     case OFPACT_PUSH_MPLS:
     case OFPACT_PUSH_VLAN:
     case OFPACT_REG_LOAD:
+    case OFPACT_SET_FIELD:
     case OFPACT_SET_ETH_DST:
     case OFPACT_SET_ETH_SRC:
     case OFPACT_SET_IP_DSCP:
@@ -1262,6 +1391,7 @@ ovs_instruction_type_from_ofpact_type(enum ofpact_type type)
     case OFPACT_SET_L4_DST_PORT:
     case OFPACT_REG_MOVE:
     case OFPACT_REG_LOAD:
+    case OFPACT_SET_FIELD:
     case OFPACT_STACK_PUSH:
     case OFPACT_STACK_POP:
     case OFPACT_DEC_TTL:
@@ -1538,12 +1668,16 @@ exit:
     return error;
 }
 \f
-/* May modify flow->dl_type, caller must restore it. */
+/* May modify flow->dl_type and flow->vlan_tci, caller must restore them.
+ *
+ * Modifies some actions, filling in fields that could not be properly set
+ * without context. */
 static enum ofperr
-ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
-               uint8_t table_id)
+ofpact_check__(struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
+               uint8_t table_id, bool enforce_consistency)
 {
     const struct ofpact_enqueue *enqueue;
+    const struct mf_field *mf;
 
     switch (a->type) {
     case OFPACT_OUTPUT:
@@ -1569,18 +1703,75 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
         return bundle_check(ofpact_get_BUNDLE(a), max_ports, flow);
 
     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->vlan_tci & htons(VLAN_CFI)) == htons(VLAN_CFI);
+        if (!(flow->vlan_tci & htons(VLAN_CFI)) &&
+            !ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
+            goto inconsistent;
+        }
+        /* Temporary mark that we have a vlan tag. */
+        flow->vlan_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->vlan_tci & htons(VLAN_CFI)) == htons(VLAN_CFI);
+        if (!(flow->vlan_tci & htons(VLAN_CFI)) &&
+            !ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
+            goto inconsistent;
+        }
+        /* Temporary mark that we have a vlan tag. */
+        flow->vlan_tci |= htons(VLAN_CFI);
+        return 0;
+
     case OFPACT_STRIP_VLAN:
+        if (!(flow->vlan_tci & htons(VLAN_CFI))) {
+            goto inconsistent;
+        }
+        /* Temporary mark that we have no vlan tag. */
+        flow->vlan_tci = htons(0);
+        return 0;
+
     case OFPACT_PUSH_VLAN:
+        if (flow->vlan_tci & htons(VLAN_CFI)) {
+            /* Multiple VLAN headers not supported. */
+            return OFPERR_OFPBAC_BAD_TAG;
+        }
+        /* Temporary mark that we have a vlan tag. */
+        flow->vlan_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 (flow->dl_type != htons(ETH_TYPE_IP)) {
+            goto inconsistent;
+        }
+        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)) {
+            goto inconsistent;
+        }
+        return 0;
+
     case OFPACT_SET_L4_SRC_PORT:
     case OFPACT_SET_L4_DST_PORT:
+        if (!is_ip_any(flow) ||
+            (flow->nw_proto != IPPROTO_TCP && flow->nw_proto != IPPROTO_UDP
+             && flow->nw_proto != IPPROTO_SCTP)) {
+            goto inconsistent;
+        }
         return 0;
 
     case OFPACT_REG_MOVE:
@@ -1589,22 +1780,42 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
     case OFPACT_REG_LOAD:
         return nxm_reg_load_check(ofpact_get_REG_LOAD(a), flow);
 
+    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) ||
+            (mf->id == MFF_VLAN_VID && !(flow->vlan_tci & htons(VLAN_CFI)))) {
+            VLOG_WARN_RL(&rl, "set_field %s lacks correct prerequisities",
+                         mf->name);
+            return OFPERR_OFPBAC_MATCH_INCONSISTENT;
+        }
+        return 0;
+
     case OFPACT_STACK_PUSH:
         return nxm_stack_push_check(ofpact_get_STACK_PUSH(a), flow);
 
     case OFPACT_STACK_POP:
         return nxm_stack_pop_check(ofpact_get_STACK_POP(a), flow);
 
-    case OFPACT_DEC_TTL:
     case OFPACT_SET_MPLS_TTL:
     case OFPACT_DEC_MPLS_TTL:
+        if (!eth_type_mpls(flow->dl_type)) {
+            goto inconsistent;
+        }
+        return 0;
+
     case OFPACT_SET_TUNNEL:
     case OFPACT_SET_QUEUE:
     case OFPACT_POP_QUEUE:
-    case OFPACT_FIN_TIMEOUT:
     case OFPACT_RESUBMIT:
         return 0;
 
+    case OFPACT_FIN_TIMEOUT:
+        if (flow->nw_proto != IPPROTO_TCP) {
+            goto inconsistent;
+        }
+        return 0;
+
     case OFPACT_LEARN:
         return learn_check(ofpact_get_LEARN(a), flow);
 
@@ -1621,6 +1832,9 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
 
     case OFPACT_POP_MPLS:
         flow->dl_type = ofpact_get_POP_MPLS(a)->ethertype;
+        if (!eth_type_mpls(flow->dl_type)) {
+            goto inconsistent;
+        }
         return 0;
 
     case OFPACT_SAMPLE:
@@ -1632,7 +1846,7 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
     case OFPACT_WRITE_ACTIONS: {
         struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
         return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
-                             flow, max_ports, table_id);
+                             flow, max_ports, table_id, false);
     }
 
     case OFPACT_WRITE_METADATA:
@@ -1658,28 +1872,41 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
     default:
         NOT_REACHED();
     }
+
+ inconsistent:
+    if (enforce_consistency) {
+        return OFPERR_OFPBAC_MATCH_INCONSISTENT;
+    }
+    return 0;
 }
 
 /* Checks that the 'ofpacts_len' bytes of actions in 'ofpacts' are
  * appropriate for a packet with the prerequisites satisfied by 'flow' in a
  * switch with no more than 'max_ports' ports.
  *
+ * May annotate ofpacts with information gathered from the 'flow'.
+ *
  * May temporarily modify 'flow', but restores the changes before returning. */
 enum ofperr
-ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len,
-              struct flow *flow, ofp_port_t max_ports, uint8_t table_id)
+ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len,
+              struct flow *flow, ofp_port_t max_ports, uint8_t table_id,
+              bool enforce_consistency)
 {
-    const struct ofpact *a;
+    struct ofpact *a;
     ovs_be16 dl_type = flow->dl_type;
+    ovs_be16 vlan_tci = flow->vlan_tci;
     enum ofperr error = 0;
 
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
-        error = ofpact_check__(a, flow, max_ports, table_id);
+        error = ofpact_check__(a, flow, max_ports, table_id,
+                               enforce_consistency);
         if (error) {
             break;
         }
     }
-    flow->dl_type = dl_type; /* Restore. */
+    /* Restore fields that may have been modified. */
+    flow->dl_type = dl_type;
+    flow->vlan_tci = vlan_tci;
     return error;
 }
 
@@ -1877,6 +2104,10 @@ ofpact_to_nxast(const struct ofpact *a, struct ofpbuf *out)
         nxm_reg_load_to_nxast(ofpact_get_REG_LOAD(a), out);
         break;
 
+    case OFPACT_SET_FIELD:
+        set_field_to_openflow(ofpact_get_SET_FIELD(a), out);
+        break;
+
     case OFPACT_STACK_PUSH:
         nxm_stack_push_to_nxast(ofpact_get_STACK_PUSH(a), out);
         break;
@@ -2063,6 +2294,10 @@ ofpact_to_openflow10(const struct ofpact *a, struct ofpbuf *out)
         break;
 
     case OFPACT_PUSH_VLAN:
+        /* PUSH is a side effect of a SET_VLAN_VID/PCP, which should
+         * follow this action. */
+        break;
+
     case OFPACT_CLEAR_ACTIONS:
     case OFPACT_WRITE_ACTIONS:
     case OFPACT_GOTO_TABLE:
@@ -2078,6 +2313,7 @@ ofpact_to_openflow10(const struct ofpact *a, struct ofpbuf *out)
     case OFPACT_BUNDLE:
     case OFPACT_REG_MOVE:
     case OFPACT_REG_LOAD:
+    case OFPACT_SET_FIELD:
     case OFPACT_STACK_PUSH:
     case OFPACT_STACK_POP:
     case OFPACT_DEC_TTL:
@@ -2155,11 +2391,23 @@ ofpact_to_openflow11(const struct ofpact *a, struct ofpbuf *out)
         break;
 
     case OFPACT_SET_VLAN_VID:
+        /* Push a VLAN tag, if one was not seen at action validation time. */
+        if (!ofpact_get_SET_VLAN_VID(a)->flow_has_vlan
+            && ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
+            ofputil_put_OFPAT11_PUSH_VLAN(out)->ethertype
+                = htons(ETH_TYPE_VLAN_8021Q);
+        }
         ofputil_put_OFPAT11_SET_VLAN_VID(out)->vlan_vid
             = htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid);
         break;
 
     case OFPACT_SET_VLAN_PCP:
+        /* Push a VLAN tag, if one was not seen at action validation time. */
+        if (!ofpact_get_SET_VLAN_PCP(a)->flow_has_vlan
+            && ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
+            ofputil_put_OFPAT11_PUSH_VLAN(out)->ethertype
+                = htons(ETH_TYPE_VLAN_8021Q);
+        }
         ofputil_put_OFPAT11_SET_VLAN_PCP(out)->vlan_pcp
             = ofpact_get_SET_VLAN_PCP(a)->vlan_pcp;
         break;
@@ -2268,6 +2516,7 @@ ofpact_to_openflow11(const struct ofpact *a, struct ofpbuf *out)
     case OFPACT_BUNDLE:
     case OFPACT_REG_MOVE:
     case OFPACT_REG_LOAD:
+    case OFPACT_SET_FIELD:
     case OFPACT_STACK_PUSH:
     case OFPACT_STACK_POP:
     case OFPACT_SET_TUNNEL:
@@ -2424,6 +2673,7 @@ ofpact_outputs_to_port(const struct ofpact *ofpact, ofp_port_t port)
     case OFPACT_SET_L4_DST_PORT:
     case OFPACT_REG_MOVE:
     case OFPACT_REG_LOAD:
+    case OFPACT_SET_FIELD:
     case OFPACT_STACK_PUSH:
     case OFPACT_STACK_POP:
     case OFPACT_DEC_TTL:
@@ -2579,6 +2829,8 @@ ofpact_format(const struct ofpact *a, struct ds *s)
     const struct ofpact_metadata *metadata;
     const struct ofpact_tunnel *tunnel;
     const struct ofpact_sample *sample;
+    const struct ofpact_set_field *set_field;
+    const struct mf_field *mf;
     ofp_port_t port;
 
     switch (a->type) {
@@ -2639,17 +2891,24 @@ ofpact_format(const struct ofpact *a, struct ds *s)
         break;
 
     case OFPACT_SET_VLAN_VID:
-        ds_put_format(s, "mod_vlan_vid:%"PRIu16,
+        ds_put_format(s, "%s:%"PRIu16,
+                      (a->compat == OFPUTIL_OFPAT11_SET_VLAN_VID
+                       ? "set_vlan_vid"
+                       : "mod_vlan_vid"),
                       ofpact_get_SET_VLAN_VID(a)->vlan_vid);
         break;
 
     case OFPACT_SET_VLAN_PCP:
-        ds_put_format(s, "mod_vlan_pcp:%"PRIu8,
+        ds_put_format(s, "%s:%"PRIu8,
+                      (a->compat == OFPUTIL_OFPAT11_SET_VLAN_PCP
+                       ? "set_vlan_pcp"
+                       : "mod_vlan_pcp"),
                       ofpact_get_SET_VLAN_PCP(a)->vlan_pcp);
         break;
 
     case OFPACT_STRIP_VLAN:
-        ds_put_cstr(s, "strip_vlan");
+        ds_put_cstr(s, a->compat == OFPUTIL_OFPAT11_POP_VLAN
+                    ? "pop_vlan" : "strip_vlan");
         break;
 
     case OFPACT_PUSH_VLAN:
@@ -2705,6 +2964,14 @@ ofpact_format(const struct ofpact *a, struct ds *s)
         nxm_format_reg_load(ofpact_get_REG_LOAD(a), s);
         break;
 
+    case OFPACT_SET_FIELD:
+        set_field = ofpact_get_SET_FIELD(a);
+        mf = set_field->field;
+        ds_put_format(s, "set_field:");
+        mf_format(mf, &set_field->value, NULL, s);
+        ds_put_format(s, "->%s", mf->name);
+        break;
+
     case OFPACT_STACK_PUSH:
         nxm_format_stack_push(ofpact_get_STACK_PUSH(a), s);
         break;
@@ -2922,15 +3189,3 @@ ofpact_pad(struct ofpbuf *ofpacts)
         ofpbuf_put_zeros(ofpacts, OFPACT_ALIGNTO - rem);
     }
 }
-
-void
-ofpact_set_field_init(struct ofpact_reg_load *load, const struct mf_field *mf,
-                      const void *src)
-{
-    load->ofpact.compat = OFPUTIL_OFPAT12_SET_FIELD;
-    load->dst.field = mf;
-    load->dst.ofs = 0;
-    load->dst.n_bits = mf->n_bits;
-    bitwise_copy(src, mf->n_bytes, load->dst.ofs,
-                 &load->subvalue, sizeof load->subvalue, 0, mf->n_bits);
-}