On Tue, Jul 28, 2015 at 10:32:50PM -0300, Flavio Leitner wrote: > On Tue, Jul 28, 2015 at 03:22:46PM -0700, Ben Pfaff wrote: > > This is implied by the list of ports that are valid for output in the > > various versions of the OpenFlow specification. > > > > Found by OFTest. > > Looks good to me. > I wonder how one could set the output port to none. > > Acked-by: Flavio Leitner <[email protected]>
Thanks for the review. I took a closer look on this one too and ended up adding a pair of tests and adjusting the "bundle" action, which used ofpact_check_output_port() but had specific behavior for OFPP_NONE. Here's what I applied to master and branch-2.4: --8<--------------------------cut here-------------------------->8-- From: Ben Pfaff <[email protected]> Date: Wed, 29 Jul 2015 08:36:07 -0700 Subject: [PATCH] ofp-actions: OFPP_ANY (aka OFPP_NONE) is not a valid output port. This is implied by the list of ports that are valid for output in the various versions of the OpenFlow specification. Found by OFTest. Signed-off-by: Ben Pfaff <[email protected]> Acked-by: Flavio Leitner <[email protected]> --- lib/bundle.c | 14 +++++++------- lib/ofp-actions.c | 4 +++- tests/ofp-actions.at | 8 ++++++++ 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/lib/bundle.c b/lib/bundle.c index f1b1478..baf6bbf 100644 --- a/lib/bundle.c +++ b/lib/bundle.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2011, 2012, 2013, 2014 Nicira, Inc. +/* Copyright (c) 2011, 2012, 2013, 2014, 2015 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -117,14 +117,14 @@ bundle_check(const struct ofpact_bundle *bundle, ofp_port_t max_ports, for (i = 0; i < bundle->n_slaves; i++) { ofp_port_t ofp_port = bundle->slaves[i]; - enum ofperr error; - error = ofpact_check_output_port(ofp_port, max_ports); - if (error) { - VLOG_WARN_RL(&rl, "invalid slave %"PRIu16, ofp_port); - return error; + if (ofp_port != OFPP_NONE) { + enum ofperr error = ofpact_check_output_port(ofp_port, max_ports); + if (error) { + VLOG_WARN_RL(&rl, "invalid slave %"PRIu16, ofp_port); + return error; + } } - /* Controller slaves are unsupported due to the lack of a max_len * argument. This may or may not change in the future. There doesn't * seem to be a real-world use-case for supporting it. */ diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index eef3389..14a2802 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -5396,10 +5396,12 @@ ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports) case OFPP_FLOOD: case OFPP_ALL: case OFPP_CONTROLLER: - case OFPP_NONE: case OFPP_LOCAL: return 0; + case OFPP_NONE: + return OFPERR_OFPBAC_BAD_OUT_PORT; + default: if (ofp_to_u16(port) < ofp_to_u16(max_ports)) { return 0; diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at index 39401f3..6308682 100644 --- a/tests/ofp-actions.at +++ b/tests/ofp-actions.at @@ -135,6 +135,9 @@ ffff 0018 00002320 001d 3039 00005BA0 00008707 0000B26E & ofp_actions|WARN|OpenFlow action NXAST_DEC_TTL_CNT_IDS length 17 is not a multiple of 8 ffff 0011 00002320 0015 0001 00000000 0000000000000000 +# bad OpenFlow10 actions: OFPBAC_BAD_OUT_PORT +0000 0008 ffff 0000 + ]) sed '/^[[#&]]/d' < test-data > input.txt sed -n 's/^# //p; /^$/p' < test-data > expout @@ -297,6 +300,11 @@ ffff 0020 00002320 0015 000500000000 80003039005A02fd 0400000000000000 # actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678) ffff 0018 00002320 001d 3039 00005BA0 00008707 0000B26E +# bad OpenFlow11 actions: OFPBAC_BAD_OUT_PORT +& ofp_actions|WARN|bad action at offset 0 (OFPBAC_BAD_OUT_PORT): +& 00000000 00 00 00 10 ff ff ff ff-00 00 00 00 00 00 00 00 +0000 0010 ffffffff 0000 000000000000 + ]) sed '/^[[#&]]/d' < test-data > input.txt sed -n 's/^# //p; /^$/p' < test-data > expout -- 2.1.3 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
