Thanks for the review. I've supplied an incremental below. The series has
changed enough that I'm going to resend the entire thing for completeness.
> Let's add a requirement that pad[] in struct nx_action_output_reg be
> all-bytes-zero, in case we want to extend it later.
I was wondering about this. Historically we haven't taken this approach with
padding fields, but I did notice we did so for the resubmit table action. I
don't expect we will need extensions for this particular action, but I'm fine
with enforcing it anyways.
> The nxm_read_field() that you defined in a previous commit didn't, as
> I recall, shift the selected bits down to the bottom of the field, so
> I think that specifying any part of a register that doesn't include
> the least-significant bit will have surprising results in
> xlate_output_reg_action(). ??I don't see any test for that case,
> although the manpage suggests using output:NXM_NX_REG0[16..31].
As mentioned, that was a bug in nxm_read_field(). I added a test case which
would have caught it to this patch.
> If the field is wider than 16 bits, I think that
> xlate_output_reg_action() will just discard the upper bits, so that
> outputting to port 0x10001 will actually output to port 1. ??It's
> probably better to just not output at all if the value read is greater
> than UINT16_MAX.
Fixed and test case added.
8<------------------------------------------------------------------>8
NEWS | 3 ++-
include/openflow/nicira-ext.h | 8 +++++---
lib/ofp-util.c | 8 ++++++++
ofproto/ofproto-dpif.c | 7 +++++--
tests/ofproto-dpif.at | 3 ++-
utilities/ovs-ofctl.8.in | 3 ++-
6 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/NEWS b/NEWS
index 0318d4a..ae6f55e 100644
--- a/NEWS
+++ b/NEWS
@@ -1,7 +1,8 @@
Post-v1.2.0
------------------------
- OpenFlow:
- - "output" action now accepts NXM fields.
+ - Added an OpenFlow extension which allows the "output" action to accept
+ NXM fields.
- ovs-appctl:
- New "version" command to determine version of running daemon
- ovs-vswitchd:
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index be73dbf..6fcf1e7 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -793,8 +793,10 @@ enum nx_bd_algorithm {
* nxm_header values for the 'src' field of NXAST_REG_MOVE.
*
* The 'max_len' field indicates the number of bytes to send when the chosen
- * port is OFPP_CONTROLLER. It's semantics are equivalent to the 'max_len'
- * field of OFPAT_OUTPUT. */
+ * port is OFPP_CONTROLLER. Its semantics are equivalent to the 'max_len'
+ * field of OFPAT_OUTPUT.
+ *
+ * The 'zero' field is required to zeroed for forward compatibility. */
struct nx_action_output_reg {
ovs_be16 type; /* OFPAT_VENDOR. */
ovs_be16 len; /* 24. */
@@ -806,7 +808,7 @@ struct nx_action_output_reg {
ovs_be16 max_len; /* Max length to send to controller. */
- uint8_t pad[6];
+ uint8_t zero[6]; /* Reserved, must be zero. */
};
OFP_ASSERT(sizeof(struct nx_action_output_reg) == 24);
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 0299e19..b0e7405 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1983,6 +1983,14 @@ static int
check_output_reg(const struct nx_action_output_reg *naor,
const struct flow *flow)
{
+ size_t i;
+
+ for (i = 0; i < sizeof naor->zero; i++) {
+ if (naor->zero[i]) {
+ return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_ARGUMENT);
+ }
+ }
+
return nxm_src_check(naor->src, nxm_decode_ofs(naor->ofs_nbits),
nxm_decode_n_bits(naor->ofs_nbits), flow);
}
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 2b80a45..fdef4af 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3023,10 +3023,13 @@ static void
xlate_output_reg_action(struct action_xlate_ctx *ctx,
const struct nx_action_output_reg *naor)
{
- uint16_t ofp_port;
+ uint64_t ofp_port;
ofp_port = nxm_read_field_bits(naor->src, naor->ofs_nbits, &ctx->flow);
- xlate_output_action__(ctx, ofp_port, ntohs(naor->max_len));
+
+ if (ofp_port <= UINT16_MAX) {
+ xlate_output_action__(ctx, ofp_port, ntohs(naor->max_len));
+ }
}
static void
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index a422174..a987256 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -46,12 +46,13 @@ AT_CLEANUP
AT_SETUP([ofproto-dpif - output])
OFPROTO_START
AT_DATA([flows.txt], [dnl
-in_port=1 actions=resubmit:2,resubmit:3,resubmit:4,resubmit:5,resubmit:6
+in_port=1
actions=resubmit:2,resubmit:3,resubmit:4,resubmit:5,resubmit:6,resubmit:7
in_port=2 actions=output:9
in_port=3
actions=load:55->NXM_NX_REG0[[]],output:NXM_NX_REG0[[]],load:66->NXM_NX_REG1[[]]
in_port=4
actions=output:10,output:NXM_NX_REG0[[]],output:NXM_NX_REG1[[]],output:11
in_port=5 actions=load:77->NXM_NX_REG0[[0..15]],load:88->NXM_NX_REG0[[16..31]]
in_port=6 actions=output:NXM_NX_REG0[[0..15]],output:NXM_NX_REG0[[16..31]]
+in_port=7 actions=load:0x110000ff->NXM_NX_REG0[[]],output:NXM_NX_REG0[[]]
])
AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
AT_CHECK([ovs-appctl -t test-openflowd ofproto/trace br0
'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0),icmp(type=8,code=0)'],
[0], [stdout])
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index f0a2382..3262593 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -574,7 +574,8 @@ of the following keywords:
.IQ \fBoutput\fR:\fIsrc\fR
Outputs the packet. If \fIport\fR is an OpenFlow port number, outputs directly
to it. Otherwise, outputs to the OpenFlow port number read from \fIsrc\fR
-which must be an NXM field as described above.
+which must be an NXM field as described above. Outputting to an NXM field is
+an OpenFlow extension which is not supported by standard OpenFlow switches.
.IP
Example: \fBoutput:NXM_NX_REG0[16..31]\fR outputs to the OpenFlow port number
written in the upper half of register 0.
--
1.7.6
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
