OpenFlow 1.1 set vlan actions only modify existing vlan
headers, while OF 1.0 actions push a new vlan header if one
does not exist already.
Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
---
lib/ofp-actions.c | 91 +++++++++++++++++++++++++++++++++++++-----
lib/ofp-actions.h | 20 ++++++++--
lib/ofp-parse.c | 12 +++++-
lib/ofp-util.def | 4 +-
ofproto/ofproto-dpif-xlate.c | 19 +++++----
ofproto/ofproto.c | 2 +-
tests/ofp-actions.at | 4 +-
7 files changed, 124 insertions(+), 28 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index cccd6b1..c7322f6 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -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,14 +522,20 @@ 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:
@@ -774,6 +782,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 +803,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:
@@ -1538,9 +1554,9 @@ exit:
return error;
}
-/* May modify flow->dl_type, caller must restore it. */
+/* May modify flow->dl_type and flow->vlan_tci, caller must restore them. */
static enum ofperr
-ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
+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;
@@ -1569,13 +1585,37 @@ 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:
@@ -1583,6 +1623,8 @@ ofpact_check__(const struct ofpact *a, struct flow *flow,
ofp_port_t max_ports,
/* 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:
@@ -1713,14 +1755,17 @@ ofpact_check__(const struct ofpact *a, struct flow
*flow, ofp_port_t max_ports,
* 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,
+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) {
@@ -1730,7 +1775,9 @@ ofpacts_check(const struct ofpact ofpacts[], size_t
ofpacts_len,
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;
}
@@ -2114,6 +2161,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:
@@ -2206,11 +2257,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;
@@ -2690,12 +2753,18 @@ 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,
+ ofpact_get_SET_VLAN_VID(a)->ofpact.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,
+ ofpact_get_SET_VLAN_PCP(a)->ofpact.compat
+ == OFPUTIL_OFPAT11_SET_VLAN_PCP ?
+ "set_vlan_pcp" : "mod_vlan_pcp",
ofpact_get_SET_VLAN_PCP(a)->vlan_pcp);
break;
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index e097468..7be4e92 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -257,18 +257,32 @@ struct ofpact_bundle {
/* OFPACT_SET_VLAN_VID.
*
- * Used for OFPAT10_SET_VLAN_VID. */
+ * We keep track if vlan was present at action validation time to avoid a
+ * PUSH_VLAN when translating to OpenFlow 1.1+.
+ *
+ * We also keep the originating OFPUTIL action code in ofpact.compat.
+ *
+ * Used for OFPAT10_SET_VLAN_VID and OFPAT11_SET_VLAN_VID. */
struct ofpact_vlan_vid {
struct ofpact ofpact;
uint16_t vlan_vid; /* VLAN VID in low 12 bits, 0 in other bits. */
+ bool push_vlan_if_needed; /* OF 1.0 semantics if true. */
+ bool flow_has_vlan; /* VLAN present at action validation time? */
};
/* OFPACT_SET_VLAN_PCP.
*
- * Used for OFPAT10_SET_VLAN_PCP. */
+ * We keep track if vlan was present at action validation time to avoid a
+ * PUSH_VLAN when translating to OpenFlow 1.1+.
+ *
+ * We also keep the originating OFPUTIL action code in ofpact.compat.
+ *
+ * Used for OFPAT10_SET_VLAN_PCP and OFPAT11_SET_VLAN_PCP. */
struct ofpact_vlan_pcp {
struct ofpact ofpact;
uint8_t vlan_pcp; /* VLAN PCP in low 3 bits, 0 in other bits. */
+ bool push_vlan_if_needed; /* OF 1.0 semantics if true. */
+ bool flow_has_vlan; /* VLAN present at action validation time? */
};
/* OFPACT_SET_ETH_SRC, OFPACT_SET_ETH_DST.
@@ -563,7 +577,7 @@ enum ofperr ofpacts_pull_openflow11_instructions(struct
ofpbuf *openflow,
enum ofp_version version,
unsigned int instructions_len,
struct ofpbuf *ofpacts);
-enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
+enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
struct flow *, ofp_port_t max_ports,
uint8_t table_id, bool enforce_consistency);
enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len);
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 757b971..73a56b8 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -599,6 +599,8 @@ parse_named_action(enum ofputil_action_code code,
{
size_t orig_size = ofpacts->size;
struct ofpact_tunnel *tunnel;
+ struct ofpact_vlan_vid *vlan_vid;
+ struct ofpact_vlan_pcp *vlan_pcp;
char *error = NULL;
uint16_t ethertype = 0;
uint16_t vid = 0;
@@ -624,7 +626,10 @@ parse_named_action(enum ofputil_action_code code,
if (vid & ~VLAN_VID_MASK) {
return xasprintf("%s: not a valid VLAN VID", arg);
}
- ofpact_put_SET_VLAN_VID(ofpacts)->vlan_vid = vid;
+ vlan_vid = ofpact_put_SET_VLAN_VID(ofpacts);
+ vlan_vid->vlan_vid = vid;
+ vlan_vid->ofpact.compat = code;
+ vlan_vid->push_vlan_if_needed = code == OFPUTIL_OFPAT10_SET_VLAN_VID;
break;
case OFPUTIL_OFPAT10_SET_VLAN_PCP:
@@ -637,7 +642,10 @@ parse_named_action(enum ofputil_action_code code,
if (pcp & ~7) {
return xasprintf("%s: not a valid VLAN PCP", arg);
}
- ofpact_put_SET_VLAN_PCP(ofpacts)->vlan_pcp = pcp;
+ vlan_pcp = ofpact_put_SET_VLAN_PCP(ofpacts);
+ vlan_pcp->vlan_pcp = pcp;
+ vlan_pcp->ofpact.compat = code;
+ vlan_pcp->push_vlan_if_needed = code == OFPUTIL_OFPAT10_SET_VLAN_PCP;
break;
case OFPUTIL_OFPAT12_SET_FIELD:
diff --git a/lib/ofp-util.def b/lib/ofp-util.def
index 752fd06..631a6b1 100644
--- a/lib/ofp-util.def
+++ b/lib/ofp-util.def
@@ -20,8 +20,8 @@ OFPAT10_ACTION(OFPAT10_ENQUEUE, ofp10_action_enqueue,
"enqueue")
#define OFPAT11_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME)
#endif
OFPAT11_ACTION(OFPAT11_OUTPUT, ofp11_action_output, 0, "output")
-OFPAT11_ACTION(OFPAT11_SET_VLAN_VID, ofp_action_vlan_vid, 0, "mod_vlan_vid")
-OFPAT11_ACTION(OFPAT11_SET_VLAN_PCP, ofp_action_vlan_pcp, 0, "mod_vlan_pcp")
+OFPAT11_ACTION(OFPAT11_SET_VLAN_VID, ofp_action_vlan_vid, 0, "set_vlan_vid")
+OFPAT11_ACTION(OFPAT11_SET_VLAN_PCP, ofp_action_vlan_pcp, 0, "set_vlan_pcp")
OFPAT11_ACTION(OFPAT11_SET_DL_SRC, ofp_action_dl_addr, 0, "mod_dl_src")
OFPAT11_ACTION(OFPAT11_SET_DL_DST, ofp_action_dl_addr, 0, "mod_dl_dst")
OFPAT11_ACTION(OFPAT11_SET_NW_SRC, ofp_action_nw_addr, 0, "mod_nw_src")
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7be691c..3340eea 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2320,17 +2320,22 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
case OFPACT_SET_VLAN_VID:
wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
- flow->vlan_tci &= ~htons(VLAN_VID_MASK);
- flow->vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
- | htons(VLAN_CFI));
+ if (flow->vlan_tci & htons(VLAN_CFI) ||
+ ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
+ flow->vlan_tci &= ~htons(VLAN_VID_MASK);
+ flow->vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
+ | htons(VLAN_CFI));
+ }
break;
case OFPACT_SET_VLAN_PCP:
wc->masks.vlan_tci |= htons(VLAN_PCP_MASK | VLAN_CFI);
- flow->vlan_tci &= ~htons(VLAN_PCP_MASK);
- flow->vlan_tci |=
- htons((ofpact_get_SET_VLAN_PCP(a)->vlan_pcp << VLAN_PCP_SHIFT)
- | VLAN_CFI);
+ if (flow->vlan_tci & htons(VLAN_CFI) ||
+ ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
+ flow->vlan_tci &= ~htons(VLAN_PCP_MASK);
+ flow->vlan_tci |= htons((ofpact_get_SET_VLAN_PCP(a)->vlan_pcp
+ << VLAN_PCP_SHIFT) | VLAN_CFI);
+ }
break;
case OFPACT_STRIP_VLAN:
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 402b38d..f40f053 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2822,7 +2822,7 @@ reject_slave_controller(struct ofconn *ofconn)
*/
static enum ofperr
ofproto_check_ofpacts(struct ofproto *ofproto,
- const struct ofpact ofpacts[], size_t ofpacts_len,
+ struct ofpact ofpacts[], size_t ofpacts_len,
struct flow *flow, uint8_t table_id,
bool enforce_consistency)
{
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 0909ce1..e3fc2bc 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -138,10 +138,10 @@ AT_DATA([test-data], [dnl
# actions=CONTROLLER:1234
0000 0010 fffffffd 04d2 000000000000
-# actions=mod_vlan_vid:9
+# actions=set_vlan_vid:9
0001 0008 0009 0000
-# actions=mod_vlan_pcp:6
+# actions=set_vlan_pcp:6
0002 0008 06 000000
# actions=mod_dl_src:00:11:22:33:44:55
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
