Better abstract OFPT_SET_CONFIG and OFPT_GET_CONFIG_REPLY, make stric…

…ter.

The OFPT_SET_CONFIG and OFPT_GET_CONFIG_REPLY messages, which have the
same format, have a 'flags' field in which OpenFlow defines some bits,
which change somewhat from one version to another, and does not define
others.  Until now, Open vSwitch has not abstracted these messages at all
and has ignored the bits that OpenFlow leaves undefined.  This commit
abstracts the messages in the same way as other OpenFlow messages and
validates in OFPT_SET_CONFIG messages that the undefined bits are set to
zero.

OpenFlow 1.1 and 1.2, but not OpenFlow 1.0, define a flag named
OFPC_INVALID_TTL_TO_CONTROLLER.  Open vSwitch has until now also
implemented this as an extension to OpenFlow 1.0, and this commit retains
that extension.

Reported-by: Manpreet Singh <[email protected]>
Signed-off-by: Ben Pfaff <[email protected]>
Acked-by: Andy Zhou <[email protected]>

lib/learning-switch.c

@@ -461,7 +461,6 @@ static void
 send_features_request(struct lswitch *sw)
 {
     struct ofpbuf *b;
-    struct ofp_switch_config *osc;
     int ofp_version = rconn_get_version(sw->rconn);
 
     ovs_assert(ofp_version > 0 && ofp_version < 0xff);
@@ -471,10 +470,10 @@ send_features_request(struct lswitch *sw)
     queue_tx(sw, b);
 
     /* Send OFPT_SET_CONFIG. */
-    b = ofpraw_alloc(OFPRAW_OFPT_SET_CONFIG, ofp_version, sizeof *osc);
-    osc = ofpbuf_put_zeros(b, sizeof *osc);
-    osc->miss_send_len = htons(OFP_DEFAULT_MISS_SEND_LEN);
-    queue_tx(sw, b);
+    struct ofputil_switch_config config = {
+        .miss_send_len = OFP_DEFAULT_MISS_SEND_LEN
+    };
+    queue_tx(sw, ofputil_encode_set_config(&config, ofp_version));
 }
 
 static void

lib/ofp-print.c

@@ -496,25 +496,39 @@ ofp_print_switch_features(struct ds *string, const struct ofp_header *oh)
 }
 
 static void
-ofp_print_switch_config(struct ds *string, const struct ofp_switch_config *osc)
+ofp_print_switch_config(struct ds *string,
+                        const struct ofputil_switch_config *config)
 {
-    enum ofp_config_flags flags;
+    ds_put_format(string, " frags=%s",
+                  ofputil_frag_handling_to_string(config->frag));
 
-    flags = ntohs(osc->flags);
-
-    ds_put_format(string, " frags=%s", ofputil_frag_handling_to_string(flags));
-    flags &= ~OFPC_FRAG_MASK;
-
-    if (flags & OFPC_INVALID_TTL_TO_CONTROLLER) {
+    if (config->invalid_ttl_to_controller > 0) {
         ds_put_format(string, " invalid_ttl_to_controller");
-        flags &= ~OFPC_INVALID_TTL_TO_CONTROLLER;
     }
 
-    if (flags) {
-        ds_put_format(string, " ***unknown flags 0x%04"PRIx16"***", flags);
+    ds_put_format(string, " miss_send_len=%"PRIu16"\n", config->miss_send_len);
+}
+
+static void
+ofp_print_set_config(struct ds *string, const struct ofp_header *oh)
+{
+    struct ofputil_switch_config config;
+    enum ofperr error;
+
+    error = ofputil_decode_set_config(oh, &config);
+    if (error) {
+        ofp_print_error(string, error);
+        return;
     }
+    ofp_print_switch_config(string, &config);
+}
 
-    ds_put_format(string, " miss_send_len=%"PRIu16"\n", ntohs(osc->miss_send_len));
+static void
+ofp_print_get_config_reply(struct ds *string, const struct ofp_header *oh)
+{
+    struct ofputil_switch_config config;
+    ofputil_decode_get_config_reply(oh, &config);
+    ofp_print_switch_config(string, &config);
 }
 
 static void print_wild(struct ds *string, const char *leader, int is_wild,
@@ -3212,8 +3226,11 @@ ofp_to_string__(const struct ofp_header *oh, enum ofpraw raw,
         break;
 
     case OFPTYPE_GET_CONFIG_REPLY:
+        ofp_print_get_config_reply(string, oh);
+        break;
+
     case OFPTYPE_SET_CONFIG:
-        ofp_print_switch_config(string, ofpmsg_body(oh));
+        ofp_print_set_config(string, oh);
         break;
 
     case OFPTYPE_PACKET_IN:

lib/ofp-util.c

@@ -4074,6 +4074,84 @@ ofputil_append_port_desc_stats_reply(const struct ofputil_phy_port *pp,
     ofpmp_postappend(replies, start_ofs);
 }
 
+/* ofputil_switch_config */
+
+/* Decodes 'oh', which must be an OFPT_GET_CONFIG_REPLY or OFPT_SET_CONFIG
+ * message, into 'config'.  Returns false if 'oh' contained any flags that
+ * aren't specified in its version of OpenFlow, true otherwise. */
+static bool
+ofputil_decode_switch_config(const struct ofp_header *oh,
+                             struct ofputil_switch_config *config)
+{
+    const struct ofp_switch_config *osc;
+    struct ofpbuf b;
+
+    ofpbuf_use_const(&b, oh, ntohs(oh->length));
+    ofpraw_pull_assert(&b);
+    osc = ofpbuf_pull(&b, sizeof *osc);
+
+    config->frag = ntohs(osc->flags) & OFPC_FRAG_MASK;
+    config->miss_send_len = ntohs(osc->miss_send_len);
+
+    ovs_be16 valid_mask = htons(OFPC_FRAG_MASK);
+    if (oh->version < OFP13_VERSION) {
+        const ovs_be16 ttl_bit = htons(OFPC_INVALID_TTL_TO_CONTROLLER);
+        valid_mask |= ttl_bit;
+        config->invalid_ttl_to_controller = (osc->flags & ttl_bit) != 0;
+    } else {
+        config->invalid_ttl_to_controller = -1;
+    }
+
+    return !(osc->flags & ~valid_mask);
+}
+
+void
+ofputil_decode_get_config_reply(const struct ofp_header *oh,
+                                struct ofputil_switch_config *config)
+{
+    ofputil_decode_switch_config(oh, config);
+}
+
+enum ofperr
+ofputil_decode_set_config(const struct ofp_header *oh,
+                          struct ofputil_switch_config *config)
+{
+    return (ofputil_decode_switch_config(oh, config)
+            ? 0
+            : OFPERR_OFPSCFC_BAD_FLAGS);
+}
+
+static struct ofpbuf *
+ofputil_put_switch_config(const struct ofputil_switch_config *config,
+                          struct ofpbuf *b)
+{
+    const struct ofp_header *oh = b->data;
+    struct ofp_switch_config *osc = ofpbuf_put_zeros(b, sizeof *osc);
+    osc->flags = htons(config->frag);
+    if (config->invalid_ttl_to_controller > 0 && oh->version < OFP13_VERSION) {
+        osc->flags |= htons(OFPC_INVALID_TTL_TO_CONTROLLER);
+    }
+    osc->miss_send_len = htons(config->miss_send_len);
+    return b;
+}
+
+struct ofpbuf *
+ofputil_encode_get_config_reply(const struct ofp_header *request,
+                                const struct ofputil_switch_config *config)
+{
+    struct ofpbuf *b = ofpraw_alloc_reply(OFPRAW_OFPT_GET_CONFIG_REPLY,
+                                          request, 0);
+    return ofputil_put_switch_config(config, b);
+}
+
+struct ofpbuf *
+ofputil_encode_set_config(const struct ofputil_switch_config *config,
+                          enum ofp_version version)
+{
+    struct ofpbuf *b = ofpraw_alloc(OFPRAW_OFPT_SET_CONFIG, version, 0);
+    return ofputil_put_switch_config(config, b);
+}
+
 /* ofputil_switch_features */
 
 #define OFPC_COMMON (OFPC_FLOW_STATS | OFPC_TABLE_STATS | OFPC_PORT_STATS | \
@@ -6453,29 +6531,30 @@ ofputil_encode_barrier_request(enum ofp_version ofp_version)
 }
 
 const char *
-ofputil_frag_handling_to_string(enum ofp_config_flags flags)
+ofputil_frag_handling_to_string(enum ofputil_frag_handling frag)
 {
-    switch (flags & OFPC_FRAG_MASK) {
-    case OFPC_FRAG_NORMAL:   return "normal";
-    case OFPC_FRAG_DROP:     return "drop";
-    case OFPC_FRAG_REASM:    return "reassemble";
-    case OFPC_FRAG_NX_MATCH: return "nx-match";
+    switch (frag) {
+    case OFPUTIL_FRAG_NORMAL:   return "normal";
+    case OFPUTIL_FRAG_DROP:     return "drop";
+    case OFPUTIL_FRAG_REASM:    return "reassemble";
+    case OFPUTIL_FRAG_NX_MATCH: return "nx-match";
     }
 
     OVS_NOT_REACHED();
 }
 
 bool
-ofputil_frag_handling_from_string(const char *s, enum ofp_config_flags *flags)
+ofputil_frag_handling_from_string(const char *s,
+                                  enum ofputil_frag_handling *frag)
 {
     if (!strcasecmp(s, "normal")) {
-        *flags = OFPC_FRAG_NORMAL;
+        *frag = OFPUTIL_FRAG_NORMAL;
     } else if (!strcasecmp(s, "drop")) {
-        *flags = OFPC_FRAG_DROP;
+        *frag = OFPUTIL_FRAG_DROP;
     } else if (!strcasecmp(s, "reassemble")) {
-        *flags = OFPC_FRAG_REASM;
+        *frag = OFPUTIL_FRAG_REASM;
     } else if (!strcasecmp(s, "nx-match")) {
-        *flags = OFPC_FRAG_NX_MATCH;
+        *frag = OFPUTIL_FRAG_NX_MATCH;
     } else {
         return false;
     }

lib/ofp-util.h

@@ -490,6 +490,41 @@ enum ofperr ofputil_decode_packet_out(struct ofputil_packet_out *,
 struct ofpbuf *ofputil_encode_packet_out(const struct ofputil_packet_out *,
                                          enum ofputil_protocol protocol);
 
+enum ofputil_frag_handling {
+    OFPUTIL_FRAG_NORMAL = OFPC_FRAG_NORMAL,    /* No special handling. */
+    OFPUTIL_FRAG_DROP = OFPC_FRAG_DROP,        /* Drop fragments. */
+    OFPUTIL_FRAG_REASM = OFPC_FRAG_REASM,      /* Reassemble (if supported). */
+    OFPUTIL_FRAG_NX_MATCH = OFPC_FRAG_NX_MATCH /* Match on frag bits. */
+};
+
+const char *ofputil_frag_handling_to_string(enum ofputil_frag_handling);
+bool ofputil_frag_handling_from_string(const char *,
+                                       enum ofputil_frag_handling *);
+
+/* Abstract struct ofp_switch_config. */
+struct ofputil_switch_config {
+    /* Fragment handling. */
+    enum ofputil_frag_handling frag;
+
+    /* 0: Do not send packet to controller when decrementing invalid IP TTL.
+     * 1: Do send packet to controller when decrementing invalid IP TTL.
+     * -1: Unspecified (only OpenFlow 1.1 and 1.2 support this setting. */
+    int invalid_ttl_to_controller;
+
+    /* Maximum bytes of packet to send to controller on miss. */
+    uint16_t miss_send_len;
+};
+
+void ofputil_decode_get_config_reply(const struct ofp_header *,
+                                     struct ofputil_switch_config *);
+struct ofpbuf *ofputil_encode_get_config_reply(
+    const struct ofp_header *request, const struct ofputil_switch_config *);
+
+enum ofperr ofputil_decode_set_config(const struct ofp_header *,
+                                      struct ofputil_switch_config *);
+struct ofpbuf *ofputil_encode_set_config(
+    const struct ofputil_switch_config *, enum ofp_version);
+
 enum ofputil_port_config {
     /* OpenFlow 1.0 and 1.1 share these values for these port config bits. */
     OFPUTIL_PC_PORT_DOWN    = 1 << 0, /* Port is administratively down. */
@@ -1035,10 +1070,6 @@ struct ofpbuf *make_echo_request(enum ofp_version);
 struct ofpbuf *make_echo_reply(const struct ofp_header *rq);
 
 struct ofpbuf *ofputil_encode_barrier_request(enum ofp_version);
-
-const char *ofputil_frag_handling_to_string(enum ofp_config_flags);
-bool ofputil_frag_handling_from_string(const char *, enum ofp_config_flags *);
-
 
 /* Actions. */
 

ofproto/ofproto-dpif.c

@@ -303,7 +303,7 @@ struct ofproto_dpif {
     /* Special OpenFlow rules. */
     struct rule_dpif *miss_rule; /* Sends flow table misses to controller. */
     struct rule_dpif *no_packet_in_rule; /* Drops flow table misses. */
-    struct rule_dpif *drop_frags_rule; /* Used in OFPC_FRAG_DROP mode. */
+    struct rule_dpif *drop_frags_rule; /* Used in OFPUTIL_FRAG_DROP mode. */
 
     /* Bridging. */
     struct netflow *netflow;
@@ -3902,13 +3902,13 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
     /* We always unwildcard nw_frag (for IP), so they
      * need not be unwildcarded here. */
     if (flow->nw_frag & FLOW_NW_FRAG_ANY
-        && ofproto->up.frag_handling != OFPC_FRAG_NX_MATCH) {
-        if (ofproto->up.frag_handling == OFPC_FRAG_NORMAL) {
+        && ofproto->up.frag_handling != OFPUTIL_FRAG_NX_MATCH) {
+        if (ofproto->up.frag_handling == OFPUTIL_FRAG_NORMAL) {
             /* We must pretend that transport ports are unavailable. */
             flow->tp_src = htons(0);
             flow->tp_dst = htons(0);
         } else {
-            /* Must be OFPC_FRAG_DROP (we don't have OFPC_FRAG_REASM).
+            /* Must be OFPUTIL_FRAG_DROP (we don't have OFPUTIL_FRAG_REASM).
              * Use the drop_frags_rule (which cannot disappear). */
             rule = ofproto->drop_frags_rule;
             if (stats) {
@@ -4400,10 +4400,10 @@ get_datapath_version(const struct ofproto *ofproto_)
 
 static bool
 set_frag_handling(struct ofproto *ofproto_,
-                  enum ofp_config_flags frag_handling)
+                  enum ofputil_frag_handling frag_handling)
 {
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
-    if (frag_handling != OFPC_FRAG_REASM) {
+    if (frag_handling != OFPUTIL_FRAG_REASM) {
         ofproto->backer->need_revalidate = REV_RECONFIGURE;
         return true;
     } else {

ofproto/ofproto-provider.h

@@ -78,7 +78,7 @@ struct ofproto {
     char *sw_desc;              /* Software version (NULL for default). */
     char *serial_desc;          /* Serial number (NULL for default). */
     char *dp_desc;              /* Datapath description (NULL for default). */
-    enum ofp_config_flags frag_handling; /* One of OFPC_*.  */
+    enum ofputil_frag_handling frag_handling;
 
     /* Datapath. */
     struct hmap ports;          /* Contains "struct ofport"s. */
@@ -1235,22 +1235,22 @@ struct ofproto_class {
      * which takes one of the following values, with the corresponding
      * meanings:
      *
-     *  - OFPC_FRAG_NORMAL: The switch should treat IP fragments the same way
-     *    as other packets, omitting TCP and UDP port numbers (always setting
-     *    them to 0).
+     *  - OFPUTIL_FRAG_NORMAL: The switch should treat IP fragments the same
+     *    way as other packets, omitting TCP and UDP port numbers (always
+     *    setting them to 0).
      *
-     *  - OFPC_FRAG_DROP: The switch should drop all IP fragments without
+     *  - OFPUTIL_FRAG_DROP: The switch should drop all IP fragments without
      *    passing them through the flow table.
      *
-     *  - OFPC_FRAG_REASM: The switch should reassemble IP fragments before
+     *  - OFPUTIL_FRAG_REASM: The switch should reassemble IP fragments before
      *    passing packets through the flow table.
      *
-     *  - OFPC_FRAG_NX_MATCH (a Nicira extension): Similar to OFPC_FRAG_NORMAL,
-     *    except that TCP and UDP port numbers should be included in fragments
-     *    with offset 0.
+     *  - OFPUTIL_FRAG_NX_MATCH (a Nicira extension): Similar to
+     *    OFPUTIL_FRAG_NORMAL, except that TCP and UDP port numbers should be
+     *    included in fragments with offset 0.
      *
      * Implementations are not required to support every mode.
-     * OFPC_FRAG_NORMAL is the default mode when an ofproto is created.
+     * OFPUTIL_FRAG_NORMAL is the default mode when an ofproto is created.
      *
      * At the time of the call to ->set_frag_handling(), the current mode is
      * available in 'ofproto->frag_handling'.  ->set_frag_handling() returns
@@ -1260,7 +1260,7 @@ struct ofproto_class {
      * reflect the new mode.
      */
     bool (*set_frag_handling)(struct ofproto *ofproto,
-                              enum ofp_config_flags frag_handling);
+                              enum ofputil_frag_handling frag_handling);
 
     /* Implements the OpenFlow OFPT_PACKET_OUT command.  The datapath should
      * execute the 'ofpacts_len' bytes of "struct ofpacts" in 'ofpacts'.