diff mbox

[ovs-dev,v1] ofproto: Implement OF1.4 error code for set-async-config

Message ID 1441107879-15904-1-git-send-email-ambika.arora@tcs.com
State Changes Requested
Headers show

Commit Message

ambikaarora92@gmail.com Sept. 1, 2015, 11:44 a.m. UTC 
From: Ambika Arora <ambika.arora@tcs.com>

This patch adds support for Openflow1.4 error codes for set-async-config.
In this patch, a new error type, OFPET_ASYNC_CONFIG_FAILED is introduced
that enables the switch to properly inform the controller when controller
tries to set invalid mask or unsupported configuration.

Signed-off-by: Ambika Arora <ambika.arora@tcs.com>
---
 lib/ofp-errors.h   | 13 +++++++++++++
 lib/ofp-print.c    | 14 +++++++++++++-
 lib/ofp-util.c     | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
 ofproto/ofproto.c  |  6 +++++-
 tests/ofp-print.at | 24 ++++++++++++++++++++++++
 5 files changed, 99 insertions(+), 7 deletions(-)

Comments

Ben Pfaff Sept. 9, 2015, 1:32 a.m. UTC | #1 
On Tue, Sep 01, 2015 at 05:14:39PM +0530, ambikaarora92@gmail.com wrote:
> From: Ambika Arora <ambika.arora@tcs.com>
> 
> This patch adds support for Openflow1.4 error codes for set-async-config.
> In this patch, a new error type, OFPET_ASYNC_CONFIG_FAILED is introduced
> that enables the switch to properly inform the controller when controller
> tries to set invalid mask or unsupported configuration.
> 
> Signed-off-by: Ambika Arora <ambika.arora@tcs.com>

Thanks for the patch.

This adds a ton of really redundant code.  Can you refactor it to avoid
that?

Thanks,

Ben.
diff mbox

Patch

diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
index c020f7a..9c108d0 100644
--- a/lib/ofp-errors.h
+++ b/lib/ofp-errors.h
@@ -623,6 +623,19 @@  enum ofperr {
     /* ONF1.3(4448), OF1.4+(14,8).  Permissions error. */
     OFPERR_OFPBPC_EPERM,
 
+/* ## -------------------------- ## */
+/* ## OFPET_ASYNC_CONFIG_FAILED  ## */
+/* ## -------------------------- ## */
+
+    /* OF1.4+(15,0).  One mask is invalid. */
+    OFPERR_OFPACFC_INVALID,
+
+    /* OF1.4+(15,1).  Requested configuration not supported. */
+    OFPERR_OFPACFC_UNSUPPORTED,
+
+    /* OF1.4+(15,2).  Permissions error. */
+    OFPERR_OFPACFC_EPERM,
+
 /* ## -------------------- ## */
 /* ## OFPET_BUNDLE_FAILED  ## */
 /* ## -------------------- ## */
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 6e32d4d..f19d9be 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2070,10 +2070,22 @@  ofp_print_nxt_set_async_config(struct ds *string,
         }
     } else if (raw == OFPRAW_OFPT14_SET_ASYNC ||
                raw == OFPRAW_OFPT14_GET_ASYNC_REPLY) {
+        enum ofperr error = 0;
         uint32_t role[2][OAM_N_TYPES] = {{0}};
         uint32_t type;
 
-        ofputil_decode_set_async_config(oh, role[0], role[1], true);
+        if (raw == OFPRAW_OFPT14_GET_ASYNC_REPLY) {
+            error = ofputil_decode_set_async_config(oh, role[0], role[1], true);
+        }
+        else if (raw == OFPRAW_OFPT14_SET_ASYNC) {
+            error = ofputil_decode_set_async_config(oh, role[0], role[1],
+                                                    false);
+        }
+        if (error) {
+            ofp_print_error(string, error);
+            return;
+        }
+
         for (i = 0; i < 2; i++) {
 
             ds_put_format(string, "\n %s:\n", i == 0 ? "master" : "slave");
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 5331f8c..4bb38d1 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -9288,7 +9288,13 @@  ofputil_uninit_geneve_table(struct ovs_list *mappings)
  * treats unknown properties and values as an error, as a switch would want to
  * do when interpreting a configuration request made by a controller.
  *
- * Returns 0 if successful, otherwise an OFPERR_* value. */
+ * Returns 0 if successful, otherwise an OFPERR_* value.
+ *
+ * Returns error code OFPERR_OFPACFC_INVALID if the value of mask is not in
+ * the valid range of mask.
+ *
+ * Returns error code OFPERR_OFPACFC_UNSUPPORTED if the configuration is not
+ * supported.*/
 enum ofperr
 ofputil_decode_set_async_config(const struct ofp_header *oh,
                                 uint32_t master[OAM_N_TYPES],
@@ -9336,58 +9342,91 @@  ofputil_decode_set_async_config(const struct ofp_header *oh,
 
             switch (type) {
             case OFPACPT_PACKET_IN_SLAVE:
+                if (ntohl(msg->mask) > 63) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 slave[OAM_PACKET_IN] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_PACKET_IN_MASTER:
+                if (ntohl(msg->mask) > 63) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 master[OAM_PACKET_IN] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_PORT_STATUS_SLAVE:
+                if (ntohl(msg->mask) > 7) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 slave[OAM_PORT_STATUS] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_PORT_STATUS_MASTER:
+                if (ntohl(msg->mask) > 7) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 master[OAM_PORT_STATUS] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_FLOW_REMOVED_SLAVE:
+                if (ntohl(msg->mask) > 63) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 slave[OAM_FLOW_REMOVED] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_FLOW_REMOVED_MASTER:
+                if (ntohl(msg->mask) > 63) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 master[OAM_FLOW_REMOVED] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_ROLE_STATUS_SLAVE:
+                if (ntohl(msg->mask) > 7) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 slave[OAM_ROLE_STATUS] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_ROLE_STATUS_MASTER:
+                if (ntohl(msg->mask) > 7) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 master[OAM_ROLE_STATUS] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_TABLE_STATUS_SLAVE:
+                if (ntohl(msg->mask) < 8 || ntohl(msg->mask) > 24) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 slave[OAM_TABLE_STATUS] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_TABLE_STATUS_MASTER:
+                if (ntohl(msg->mask) < 8 || ntohl(msg->mask) > 24) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 master[OAM_TABLE_STATUS] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_REQUESTFORWARD_SLAVE:
+                if (ntohl(msg->mask) > 3) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 slave[OAM_REQUESTFORWARD] = ntohl(msg->mask);
                 break;
 
             case OFPACPT_REQUESTFORWARD_MASTER:
+                if (ntohl(msg->mask) > 3) {
+                    return OFPERR_OFPACFC_INVALID;
+                }
                 master[OAM_REQUESTFORWARD] = ntohl(msg->mask);
                 break;
 
             default:
-                error = loose ? 0 : OFPERR_OFPBPC_BAD_TYPE;
-                break;
-            }
-            if (error) {
+                error = loose ? 0 : OFPERR_OFPACFC_UNSUPPORTED;
                 return error;
             }
         }
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index e6c0351..528310e 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5385,10 +5385,14 @@  handle_nxt_set_packet_in_format(struct ofconn *ofconn,
 static enum ofperr
 handle_nxt_set_async_config(struct ofconn *ofconn, const struct ofp_header *oh)
 {
+    enum ofperr error;
     uint32_t master[OAM_N_TYPES] = {0};
     uint32_t slave[OAM_N_TYPES] = {0};
 
-    ofputil_decode_set_async_config(oh, master, slave, false);
+    error = ofputil_decode_set_async_config(oh, master, slave, false);
+    if (error) {
+        return error;
+    }
 
     ofconn_set_async_config(ofconn, master, slave);
     if (ofconn_get_type(ofconn) == OFCONN_SERVICE &&
diff --git a/tests/ofp-print.at b/tests/ofp-print.at
index 35a6262..929628f 100644
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -2778,6 +2778,30 @@  OFPT_SET_ASYNC (OF1.4) (xid=0x2):
 ])
 AT_CLEANUP
 
+AT_SETUP([OFPT_SET_ASYNC_CONFIG - invalid mask - OF1.4])
+AT_KEYWORDS([ofp-print])
+AT_CHECK([ovs-ofctl ofp-print "\
+05 1c 00 38 00 00 00 02 00 00 00 08 00 00 00 40 \
+00 01 00 08 00 00 00 02 00 02 00 08 00 00 00 02 \
+00 03 00 08 00 00 00 05 00 04 00 08 00 00 00 1c \
+00 05 00 08 00 00 00 05 \
+"], [0], [dnl
+OFPT_SET_ASYNC (OF1.4) (xid=0x2): ***decode error: OFPACFC_INVALID***
+])
+AT_CLEANUP
+
+AT_SETUP([OFPT_SET_ASYNC_CONFIG - unsupported configuration - OF1.4])
+AT_KEYWORDS([ofp-print])
+AT_CHECK([ovs-ofctl ofp-print "\
+05 1c 00 38 00 00 00 02 00 00 00 08 00 00 00 05 \
+00 11 00 08 00 00 00 02 00 02 00 08 00 00 00 02 \
+00 03 00 08 00 00 00 05 00 04 00 08 00 00 00 1c \
+00 05 00 08 00 00 00 05\
+"], [0], [dnl
+OFPT_SET_ASYNC (OF1.4) (xid=0x2): ***decode error: OFPACFC_UNSUPPORTED***
+])
+AT_CLEANUP
+
 AT_SETUP([NXT_SET_CONTROLLER_ID])
 AT_KEYWORDS([ofp-print])
 AT_CHECK([ovs-ofctl ofp-print "\