[ovs-dev] [PATCH] ofp-actions: OFPP_ANY (aka OFPP_NONE) is not a valid output port.

[Ben Pfaff]

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 <fbl at sysclose.org>

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 <blp at nicira.com>
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 <blp at nicira.com>
Acked-by: Flavio Leitner <fbl at sysclose.org>
---
 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