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