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

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

Commit Message

ambikaarora92@gmail.com Nov. 24, 2015, 12:10 p.m. UTC
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>
---
1. Implemented review comments.
2. Rebased with latest master.

 include/openflow/openflow-1.4.h    |  3 ++
 include/openflow/openflow-common.h |  3 +-
 lib/ofp-errors.h                   | 13 +++++++
 lib/ofp-print.c                    | 22 ++++++++++-
 lib/ofp-util.c                     | 75 +++++++++++++++++++++++++++++++++++---
 lib/ofp-util.h                     | 16 ++++++++
 ofproto/ofproto.c                  |  6 ++-
 tests/ofp-print.at                 | 24 ++++++++++++
 8 files changed, 154 insertions(+), 8 deletions(-)

Comments

Ben Pfaff Nov. 30, 2015, 1:41 a.m. UTC | #1
On Tue, Nov 24, 2015 at 05:40:19PM +0530, Ambika Arora wrote:
> 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>
> ---
> 1. Implemented review comments.
> 2. Rebased with latest master.

Why does table status have a minimum value?  That is, why can't a
controller set it to 0?
ambikaarora92@gmail.com Nov. 30, 2015, 4:25 a.m. UTC | #2
Hi Ben,

As per OF1.4 specification section 7.4.5, the value for enum
ofp_table_reason with reasons OFPTR_VACANCY_DOWN =3 and OFPTR_VACANCY_UP=4.
So, the controller can't set the value to 0.

With Regards,
Ambika Arora


On Mon, Nov 30, 2015 at 7:11 AM, Ben Pfaff <blp@ovn.org> wrote:

> On Tue, Nov 24, 2015 at 05:40:19PM +0530, Ambika Arora wrote:
> > 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>
> > ---
> > 1. Implemented review comments.
> > 2. Rebased with latest master.
>
> Why does table status have a minimum value?  That is, why can't a
> controller set it to 0?
>
Ben Pfaff Nov. 30, 2015, 4:39 a.m. UTC | #3
The field we are discussing is a bit-mask.  Only the bits with indexes 3
and 4 may be set, but why is it a problem if no bits are set?

On Mon, Nov 30, 2015 at 09:55:36AM +0530, Ambika Arora wrote:
> Hi Ben,
> 
> As per OF1.4 specification section 7.4.5, the value for enum
> ofp_table_reason with reasons OFPTR_VACANCY_DOWN =3 and OFPTR_VACANCY_UP=4.
> So, the controller can't set the value to 0.
> 
> With Regards,
> Ambika Arora
> 
> 
> On Mon, Nov 30, 2015 at 7:11 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> > On Tue, Nov 24, 2015 at 05:40:19PM +0530, Ambika Arora wrote:
> > > 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>
> > > ---
> > > 1. Implemented review comments.
> > > 2. Rebased with latest master.
> >
> > Why does table status have a minimum value?  That is, why can't a
> > controller set it to 0?
> >

Patch
diff mbox

diff --git a/include/openflow/openflow-1.4.h b/include/openflow/openflow-1.4.h
index 9465b8e..e600cff 100644
--- a/include/openflow/openflow-1.4.h
+++ b/include/openflow/openflow-1.4.h
@@ -134,6 +134,7 @@  enum ofp14_table_mod_prop_eviction_flag {
 enum ofp14_table_reason {
     OFPTR_VACANCY_DOWN = 3,    /* Vacancy down threshold event. */
     OFPTR_VACANCY_UP   = 4,    /* Vacancy up threshold event. */
+    OFPTR_N_REASONS            /* Denotes number of reasons. */
 };
 
 struct ofp14_table_mod_prop_eviction {
@@ -259,6 +260,7 @@  OFP_ASSERT(sizeof(struct ofp14_async_config) == 8);
 enum ofp14_requestforward_reason {
     OFPRFR_GROUP_MOD = 0,      /* Forward group mod requests. */
     OFPRFR_METER_MOD = 1,      /* Forward meter mod requests. */
+    OFPRFR_N_REASONS           /* Denotes number of reasons. */
 };
 
 /* Async Config property types.
@@ -332,6 +334,7 @@  enum ofp14_controller_role_reason {
     OFPCRR_MASTER_REQUEST = 0,  /* Another controller asked to be master. */
     OFPCRR_CONFIG         = 1,  /* Configuration changed on the switch. */
     OFPCRR_EXPERIMENTER   = 2,  /* Experimenter data changed. */
+    OFPCRR_N_REASONS            /* Denotes number of reasons. */
 };
 
 /* Role property types.
diff --git a/include/openflow/openflow-common.h b/include/openflow/openflow-common.h
index b2b2d46..81f9120 100644
--- a/include/openflow/openflow-common.h
+++ b/include/openflow/openflow-common.h
@@ -308,7 +308,8 @@  enum ofp_flow_removed_reason {
 enum ofp_port_reason {
     OFPPR_ADD,              /* The port was added. */
     OFPPR_DELETE,           /* The port was removed. */
-    OFPPR_MODIFY            /* Some attribute of the port has changed. */
+    OFPPR_MODIFY,           /* Some attribute of the port has changed. */
+    OFPPR_N_REASONS         /* Denotes number of reasons. */
 };
 
 /* A physical port has changed in the datapath */
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 240ba84..e26d301 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1847,6 +1847,7 @@  ofp_print_role_status_message(struct ds *string, const struct ofp_header *oh)
     case OFPCRR_EXPERIMENTER:
         ds_put_cstr(string, "experimenter_data_changed");
         break;
+    case OFPCRR_N_REASONS:
     default:
         OVS_NOT_REACHED();
     }
@@ -1905,6 +1906,7 @@  ofp_port_reason_to_string(enum ofp_port_reason reason,
     case OFPPR_MODIFY:
         return "modify";
 
+    case OFPPR_N_REASONS:
     default:
         snprintf(reasonbuf, bufsize, "%d", (int) reason);
         return reasonbuf;
@@ -1928,6 +1930,7 @@  ofp_role_reason_to_string(enum ofp14_controller_role_reason reason,
     case OFPCRR_EXPERIMENTER:
         return "experimenter_data_changed";
 
+    case OFPCRR_N_REASONS:
     default:
         snprintf(reasonbuf, bufsize, "%d", (int) reason);
         return reasonbuf;
@@ -1948,6 +1951,7 @@  ofp_table_reason_to_string(enum ofp14_table_reason reason,
     case OFPTR_VACANCY_UP:
         return "vacancy_up";
 
+    case OFPTR_N_REASONS:
     default:
         snprintf(reasonbuf, bufsize, "%d", (int) reason);
         return reasonbuf;
@@ -1968,6 +1972,7 @@  ofp_requestforward_reason_to_string(enum ofp14_requestforward_reason reason,
     case OFPRFR_METER_MOD:
         return "meter_mod_request";
 
+    case OFPRFR_N_REASONS:
     default:
         snprintf(reasonbuf, bufsize, "%d", (int) reason);
         return reasonbuf;
@@ -2074,10 +2079,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");
@@ -3086,6 +3103,9 @@  ofp_print_requestforward(struct ds *string, const struct ofp_header *oh)
         ds_put_cstr(string, "meter_mod");
         ofp_print_meter_mod__(string, rf.meter_mod);
         break;
+
+    case OFPRFR_N_REASONS:
+        OVS_NOT_REACHED();
     }
     ofputil_destroy_requestforward(&rf);
 }
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 342be54..503d53a 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -134,6 +134,56 @@  log_property(bool loose, const char *message, ...)
     }
 }
 
+static enum ofperr
+ofputil_check_mask(uint16_t type, uint32_t mask)
+{
+    switch (type) {
+    case OFPACPT_PACKET_IN_SLAVE:
+    case OFPACPT_PACKET_IN_MASTER:
+        if (mask > MAXIMUM_MASK_PACKET_IN) {
+            return OFPERR_OFPACFC_INVALID;
+        }
+        break;
+
+    case OFPACPT_FLOW_REMOVED_SLAVE:
+    case OFPACPT_FLOW_REMOVED_MASTER:
+        if (mask > MAXIMUM_MASK_FLOW_REMOVED) {
+            return OFPERR_OFPACFC_INVALID;
+        }
+        break;
+
+    case OFPACPT_PORT_STATUS_SLAVE:
+    case OFPACPT_PORT_STATUS_MASTER:
+        if (mask > MAXIMUM_MASK_PORT_STATUS) {
+            return OFPERR_OFPACFC_INVALID;
+        }
+        break;
+
+    case OFPACPT_ROLE_STATUS_SLAVE:
+    case OFPACPT_ROLE_STATUS_MASTER:
+        if (mask > MAXIMUM_MASK_ROLE_STATUS) {
+            return OFPERR_OFPACFC_INVALID;
+        }
+        break;
+
+    case OFPACPT_TABLE_STATUS_SLAVE:
+    case OFPACPT_TABLE_STATUS_MASTER:
+        if  ((mask < MINIMUM_MASK_TABLE_STATUS) |
+            (mask > MAXIMUM_MASK_TABLE_STATUS)) {
+            return OFPERR_OFPACFC_INVALID;
+        }
+        break;
+
+    case OFPACPT_REQUESTFORWARD_SLAVE:
+    case OFPACPT_REQUESTFORWARD_MASTER:
+        if (mask > MAXIMUM_MASK_REQUESTFORWARD) {
+            return OFPERR_OFPACFC_INVALID;
+        }
+        break;
+    }
+    return 0;
+}
+
 static size_t
 start_property(struct ofpbuf *msg, uint16_t type)
 {
@@ -5394,6 +5444,7 @@  ofputil_encode_requestforward(const struct ofputil_requestforward *rf,
         inner = ofputil_encode_meter_mod(ofp_version, rf->meter_mod);
         break;
 
+    case OFPRFR_N_REASONS:
     default:
         OVS_NOT_REACHED();
     }
@@ -5492,6 +5543,10 @@  ofputil_destroy_requestforward(struct ofputil_requestforward *rf)
     case OFPRFR_METER_MOD:
         ofpbuf_uninit(&rf->bands);
         free(rf->meter_mod);
+        break;
+
+    case OFPRFR_N_REASONS:
+        OVS_NOT_REACHED();
     }
 }
 
@@ -9409,7 +9464,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],
@@ -9455,6 +9516,13 @@  ofputil_decode_set_async_config(const struct ofp_header *oh,
                 return OFPERR_OFPBRC_BAD_LEN;
             }
 
+            if (!loose) {
+                error = ofputil_check_mask(type, ntohl(msg->mask));
+                if (error) {
+                    return error;
+                }
+             }
+
             switch (type) {
             case OFPACPT_PACKET_IN_SLAVE:
                 slave[OAM_PACKET_IN] = ntohl(msg->mask);
@@ -9505,10 +9573,7 @@  ofputil_decode_set_async_config(const struct ofp_header *oh,
                 break;
 
             default:
-                error = loose ? 0 : OFPERR_OFPBPC_BAD_TYPE;
-                break;
-            }
-            if (error) {
+                error = loose ? 0 : OFPERR_OFPACFC_UNSUPPORTED;
                 return error;
             }
         }
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 8914342..0cf304d 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -140,6 +140,22 @@  enum ofputil_protocol {
                        OFPUTIL_P_ANY_OXM)
 };
 
+    /* Valid value of mask for asynchronous messages. */
+#define MAXIMUM_MASK_PACKET_IN ((1 << OFPR_N_REASONS) - 1)
+
+#define MAXIMUM_MASK_FLOW_REMOVED ((1 << OVS_OFPRR_NONE) - 1)
+
+#define MAXIMUM_MASK_PORT_STATUS ((1 << OFPPR_N_REASONS) - 1)
+
+#define MAXIMUM_MASK_ROLE_STATUS ((1 << OFPCRR_N_REASONS) - 1)
+
+#define MINIMUM_MASK_TABLE_STATUS (1 << OFPTR_VACANCY_DOWN)
+
+#define MAXIMUM_MASK_TABLE_STATUS ((1 << OFPTR_N_REASONS) - \
+                                   MINIMUM_MASK_TABLE_STATUS)
+
+#define MAXIMUM_MASK_REQUESTFORWARD ((1 << OFPRFR_N_REASONS) - 1)
+
 /* Protocols to use for flow dumps, from most to least preferred. */
 extern enum ofputil_protocol ofputil_flow_dump_protocols[];
 extern size_t ofputil_n_flow_dump_protocols;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 2cdc8d4..507399f 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5397,10 +5397,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 fce7671..60e74b7 100644
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -2824,6 +2824,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 "\