[ovs-dev,2/6] ofp-protocol: Better abstract changing the protocol used for flow matches.

Message ID 20180216225445.28688-2-blp@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev,1/6] ofp-util: Use consistent naming convention.
Related show

Commit Message

Ben Pfaff Feb. 16, 2018, 10:54 p.m.
The previous interface here required the client to understand, to some
extent, the low-level NXFF_* values and the encoding format for the
NXT_SET_FLOW_FORMAT and NXT_SET_FLOW_MOD_TABLE_ID messages.  This commit
changes the interface so that the client only has to understand the
ofputil_protocol type used elsewhere and none of the encoding otherwise.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 include/openflow/nicira-ext.h      |  60 +-----------------
 include/openvswitch/ofp-flow.h     |   9 ---
 include/openvswitch/ofp-msgs.h     |   4 +-
 include/openvswitch/ofp-protocol.h |  11 ++++
 lib/ofp-flow.c                     |  72 +--------------------
 lib/ofp-print.c                    |  27 ++++----
 lib/ofp-protocol.c                 | 126 +++++++++++++++++++++++++++++++++++--
 ofproto/ofproto.c                  |  20 ++----
 8 files changed, 153 insertions(+), 176 deletions(-)

Comments

Justin Pettit March 13, 2018, 1:11 a.m. | #1
> On Feb 16, 2018, at 2:54 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> diff --git a/lib/ofp-protocol.c b/lib/ofp-protocol.c
> index 175318380f3b..32cf1c3fca40 100644
> --- a/lib/ofp-protocol.c
> +++ b/lib/ofp-protocol.c
> ...
> +/* Returns an NXT_SET_FLOW_FORMAT message that can be used to set the flow
> + * format to 'nxff'.  */
> +struct ofpbuf *
> +ofputil_encode_nx_set_flow_format(enum ofputil_protocol protocol)
> +{
> +    struct ofpbuf *msg = ofpraw_alloc(OFPRAW_NXT_SET_FLOW_FORMAT,
> +                                      OFP10_VERSION, 0);
> +    ovs_be32 *nxff = ofpbuf_put_uninit(msg, sizeof *nxff);
> +    if (protocol == OFPUTIL_P_OF10_STD) {
> +        *nxff = htonl(NXFF_OPENFLOW10);
> +    } else if (protocol == OFPUTIL_P_OF10_NXM) {
> +        *nxff = htonl(NXFF_NXM);
> +    } else {
> +        OVS_NOT_REACHED();
> +    }
> +
> +    return msg;
> +}

The comment describing the function is a little strange, since it references 'nxff' which is now just a local variable in the function.

> +/* Decodes the NXT_SET_MOD_TABLE_ID message at 'oh'.  Returns the message's
> + * argument, that is, whether the flow_mod_table_id feature should be
> + * enabled. */
> +bool
> +ofputil_decode_nx_flow_mod_table_id(const struct ofp_header *oh)
> +{
> +    struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
> +    ovs_assert(ofpraw_pull_assert(&b) == OFPRAW_NXT_FLOW_MOD_TABLE_ID);
> +    uint8_t *enable = ofpbuf_pull(&b, 8);
> +    return *enable != 0;

Should that function description be referencing "NXT_FLOW_MOD_TABLE_ID" instead?

Acked-by: Justin Pettit <jpettit@ovn.org>

--Justin
Ben Pfaff March 14, 2018, 6:29 p.m. | #2
On Mon, Mar 12, 2018 at 06:11:48PM -0700, Justin Pettit wrote:
> 
> > On Feb 16, 2018, at 2:54 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > diff --git a/lib/ofp-protocol.c b/lib/ofp-protocol.c
> > index 175318380f3b..32cf1c3fca40 100644
> > --- a/lib/ofp-protocol.c
> > +++ b/lib/ofp-protocol.c
> > ...
> > +/* Returns an NXT_SET_FLOW_FORMAT message that can be used to set the flow
> > + * format to 'nxff'.  */
> > +struct ofpbuf *
> > +ofputil_encode_nx_set_flow_format(enum ofputil_protocol protocol)
> > +{
> > +    struct ofpbuf *msg = ofpraw_alloc(OFPRAW_NXT_SET_FLOW_FORMAT,
> > +                                      OFP10_VERSION, 0);
> > +    ovs_be32 *nxff = ofpbuf_put_uninit(msg, sizeof *nxff);
> > +    if (protocol == OFPUTIL_P_OF10_STD) {
> > +        *nxff = htonl(NXFF_OPENFLOW10);
> > +    } else if (protocol == OFPUTIL_P_OF10_NXM) {
> > +        *nxff = htonl(NXFF_NXM);
> > +    } else {
> > +        OVS_NOT_REACHED();
> > +    }
> > +
> > +    return msg;
> > +}
> 
> The comment describing the function is a little strange, since it references 'nxff' which is now just a local variable in the function.
> 
> > +/* Decodes the NXT_SET_MOD_TABLE_ID message at 'oh'.  Returns the message's
> > + * argument, that is, whether the flow_mod_table_id feature should be
> > + * enabled. */
> > +bool
> > +ofputil_decode_nx_flow_mod_table_id(const struct ofp_header *oh)
> > +{
> > +    struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
> > +    ovs_assert(ofpraw_pull_assert(&b) == OFPRAW_NXT_FLOW_MOD_TABLE_ID);
> > +    uint8_t *enable = ofpbuf_pull(&b, 8);
> > +    return *enable != 0;
> 
> Should that function description be referencing "NXT_FLOW_MOD_TABLE_ID" instead?

Thanks for looking this over carefully.  I fixed both comments.

> Acked-by: Justin Pettit <jpettit@ovn.org>

I'll apply this to master in a bit.

Patch

diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 71a1de035d18..ddb68aa25e79 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -113,51 +113,6 @@  enum nx_hash_fields {
 
 };
 
-/* This command enables or disables an Open vSwitch extension that allows a
- * controller to specify the OpenFlow table to which a flow should be added,
- * instead of having the switch decide which table is most appropriate as
- * required by OpenFlow 1.0.  Because NXM was designed as an extension to
- * OpenFlow 1.0, the extension applies equally to ofp10_flow_mod and
- * nx_flow_mod.  By default, the extension is disabled.
- *
- * When this feature is enabled, Open vSwitch treats struct ofp10_flow_mod's
- * and struct nx_flow_mod's 16-bit 'command' member as two separate fields.
- * The upper 8 bits are used as the table ID, the lower 8 bits specify the
- * command as usual.  A table ID of 0xff is treated like a wildcarded table ID.
- *
- * The specific treatment of the table ID depends on the type of flow mod:
- *
- *    - OFPFC_ADD: Given a specific table ID, the flow is always placed in that
- *      table.  If an identical flow already exists in that table only, then it
- *      is replaced.  If the flow cannot be placed in the specified table,
- *      either because the table is full or because the table cannot support
- *      flows of the given type, the switch replies with an OFPFMFC_TABLE_FULL
- *      error.  (A controller can distinguish these cases by comparing the
- *      current and maximum number of entries reported in ofp_table_stats.)
- *
- *      If the table ID is wildcarded, the switch picks an appropriate table
- *      itself.  If an identical flow already exist in the selected flow table,
- *      then it is replaced.  The choice of table might depend on the flows
- *      that are already in the switch; for example, if one table fills up then
- *      the switch might fall back to another one.
- *
- *    - OFPFC_MODIFY, OFPFC_DELETE: Given a specific table ID, only flows
- *      within that table are matched and modified or deleted.  If the table ID
- *      is wildcarded, flows within any table may be matched and modified or
- *      deleted.
- *
- *    - OFPFC_MODIFY_STRICT, OFPFC_DELETE_STRICT: Given a specific table ID,
- *      only a flow within that table may be matched and modified or deleted.
- *      If the table ID is wildcarded and exactly one flow within any table
- *      matches, then it is modified or deleted; if flows in more than one
- *      table match, then none is modified or deleted.
- */
-struct nx_flow_mod_table_id {
-    uint8_t set;                /* Nonzero to enable, zero to disable. */
-    uint8_t pad[7];
-};
-OFP_ASSERT(sizeof(struct nx_flow_mod_table_id) == 8);
-
 enum nx_packet_in_format {
     NXPIF_STANDARD = 0,         /* OFPT_PACKET_IN for this OpenFlow version. */
     NXPIF_NXT_PACKET_IN = 1,    /* NXT_PACKET_IN (since OVS v1.1). */
@@ -618,17 +573,6 @@  OFP_ASSERT(sizeof(struct nx_async_config) == 24);
 /* ## Requests and replies. ## */
 /* ## --------------------- ## */
 
-enum nx_flow_format {
-    NXFF_OPENFLOW10 = 0,         /* Standard OpenFlow 1.0 compatible. */
-    NXFF_NXM = 2                 /* Nicira extended match. */
-};
-
-/* NXT_SET_FLOW_FORMAT request. */
-struct nx_set_flow_format {
-    ovs_be32 format;            /* One of NXFF_*. */
-};
-OFP_ASSERT(sizeof(struct nx_set_flow_format) == 4);
-
 /* NXT_FLOW_MOD (analogous to OFPT_FLOW_MOD).
  *
  * It is possible to limit flow deletions and modifications to certain
@@ -637,8 +581,8 @@  OFP_ASSERT(sizeof(struct nx_set_flow_format) == 4);
  */
 struct nx_flow_mod {
     ovs_be64 cookie;              /* Opaque controller-issued identifier. */
-    ovs_be16 command;             /* OFPFC_* + possibly a table ID (see comment
-                                   * on struct nx_flow_mod_table_id). */
+    ovs_be16 command;             /* OFPFC_*, and table ID if flow_mod_table_id
+                                   * is enabled. */
     ovs_be16 idle_timeout;        /* Idle time before discarding (seconds). */
     ovs_be16 hard_timeout;        /* Max time before discarding (seconds). */
     ovs_be16 priority;            /* Priority level of flow entry. */
diff --git a/include/openvswitch/ofp-flow.h b/include/openvswitch/ofp-flow.h
index abaf4a87a400..28aa77bad820 100644
--- a/include/openvswitch/ofp-flow.h
+++ b/include/openvswitch/ofp-flow.h
@@ -31,15 +31,6 @@  struct ofputil_table_map;
 extern "C" {
 #endif
 
-/* nx_flow_format */
-struct ofpbuf *ofputil_encode_nx_set_flow_format(enum nx_flow_format);
-enum ofputil_protocol ofputil_nx_flow_format_to_protocol(enum nx_flow_format);
-bool ofputil_nx_flow_format_is_valid(enum nx_flow_format);
-const char *ofputil_nx_flow_format_to_string(enum nx_flow_format);
-
-/* NXT_FLOW_MOD_TABLE_ID extension. */
-struct ofpbuf *ofputil_make_flow_mod_table_id(bool flow_mod_table_id);
-
 /* Protocol-independent flow_mod flags. */
 enum ofputil_flow_mod_flags {
     /* Flags that are maintained with a flow as part of its state.
diff --git a/include/openvswitch/ofp-msgs.h b/include/openvswitch/ofp-msgs.h
index 3f92f2a67751..23f3015aee20 100644
--- a/include/openvswitch/ofp-msgs.h
+++ b/include/openvswitch/ofp-msgs.h
@@ -443,10 +443,10 @@  enum ofpraw {
  * Nicira extensions that correspond to standard OpenFlow messages are listed
  * alongside the standard versions above. */
 
-    /* NXT 1.0 (12): struct nx_set_flow_format. */
+    /* NXT 1.0 (12): ovs_be32. */
     OFPRAW_NXT_SET_FLOW_FORMAT,
 
-    /* NXT 1.0+ (15): struct nx_flow_mod_table_id. */
+    /* NXT 1.0+ (15): uint8_t[8]. */
     OFPRAW_NXT_FLOW_MOD_TABLE_ID,
 
     /* NXT 1.0+ (16): struct nx_set_packet_in_format. */
diff --git a/include/openvswitch/ofp-protocol.h b/include/openvswitch/ofp-protocol.h
index bd1bedf32e69..a351660ac216 100644
--- a/include/openvswitch/ofp-protocol.h
+++ b/include/openvswitch/ofp-protocol.h
@@ -162,11 +162,22 @@  enum ofputil_protocol ofputil_protocols_from_string(const char *s);
 const char *ofputil_version_to_string(enum ofp_version ofp_version);
 uint32_t ofputil_versions_from_string(const char *s);
 uint32_t ofputil_versions_from_strings(char ** const s, size_t count);
+
+/* Messages for changing the protocol. */
 
+/* Changing the protocol at a high level.  */
 struct ofpbuf *ofputil_encode_set_protocol(enum ofputil_protocol current,
                                            enum ofputil_protocol want,
                                            enum ofputil_protocol *next);
 
+/* Changing the protocol at a low level. */
+struct ofpbuf *ofputil_encode_nx_set_flow_format(enum ofputil_protocol);
+enum ofputil_protocol ofputil_decode_nx_set_flow_format(
+    const struct ofp_header *);
+
+struct ofpbuf *ofputil_encode_nx_flow_mod_table_id(bool enable);
+bool ofputil_decode_nx_flow_mod_table_id(const struct ofp_header *);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c
index c29f5b7cb05f..af6be74909e6 100644
--- a/lib/ofp-flow.c
+++ b/lib/ofp-flow.c
@@ -34,77 +34,7 @@ 
 VLOG_DEFINE_THIS_MODULE(ofp_flow);
 
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-
-/* Returns an NXT_SET_FLOW_FORMAT message that can be used to set the flow
- * format to 'nxff'.  */
-struct ofpbuf *
-ofputil_encode_nx_set_flow_format(enum nx_flow_format nxff)
-{
-    struct nx_set_flow_format *sff;
-    struct ofpbuf *msg;
-
-    ovs_assert(ofputil_nx_flow_format_is_valid(nxff));
-
-    msg = ofpraw_alloc(OFPRAW_NXT_SET_FLOW_FORMAT, OFP10_VERSION, 0);
-    sff = ofpbuf_put_zeros(msg, sizeof *sff);
-    sff->format = htonl(nxff);
-
-    return msg;
-}
-
-/* Returns the base protocol if 'flow_format' is a valid NXFF_* value, false
- * otherwise. */
-enum ofputil_protocol
-ofputil_nx_flow_format_to_protocol(enum nx_flow_format flow_format)
-{
-    switch (flow_format) {
-    case NXFF_OPENFLOW10:
-        return OFPUTIL_P_OF10_STD;
-
-    case NXFF_NXM:
-        return OFPUTIL_P_OF10_NXM;
-
-    default:
-        return 0;
-    }
-}
-
-/* Returns true if 'flow_format' is a valid NXFF_* value, false otherwise. */
-bool
-ofputil_nx_flow_format_is_valid(enum nx_flow_format flow_format)
-{
-    return ofputil_nx_flow_format_to_protocol(flow_format) != 0;
-}
-
-/* Returns a string version of 'flow_format', which must be a valid NXFF_*
- * value. */
-const char *
-ofputil_nx_flow_format_to_string(enum nx_flow_format flow_format)
-{
-    switch (flow_format) {
-    case NXFF_OPENFLOW10:
-        return "openflow10";
-    case NXFF_NXM:
-        return "nxm";
-    default:
-        OVS_NOT_REACHED();
-    }
-}
-
-/* Returns an OpenFlow message that can be used to turn the flow_mod_table_id
- * extension on or off (according to 'flow_mod_table_id'). */
-struct ofpbuf *
-ofputil_make_flow_mod_table_id(bool flow_mod_table_id)
-{
-    struct nx_flow_mod_table_id *nfmti;
-    struct ofpbuf *msg;
-
-    msg = ofpraw_alloc(OFPRAW_NXT_FLOW_MOD_TABLE_ID, OFP10_VERSION, 0);
-    nfmti = ofpbuf_put_zeros(msg, sizeof *nfmti);
-    nfmti->set = flow_mod_table_id;
-    return msg;
-}
-
+
 struct ofputil_flow_mod_flag {
     uint16_t raw_flag;
     enum ofp_version min_version, max_version;
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 2f96b1863928..b13bc380386a 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2267,26 +2267,21 @@  ofp_print_role_status_message(struct ds *string, const struct ofp_header *oh)
 }
 
 static enum ofperr
-ofp_print_nxt_flow_mod_table_id(struct ds *string,
-                                const struct nx_flow_mod_table_id *nfmti)
+ofp_print_nxt_flow_mod_table_id(struct ds *string, const struct ofp_header *oh)
 {
-    ds_put_format(string, " %s", nfmti->set ? "enable" : "disable");
+    bool enable = ofputil_decode_nx_flow_mod_table_id(oh);
+    ds_put_format(string, " %s", enable ? "enable" : "disable");
     return 0;
 }
 
 static enum ofperr
-ofp_print_nxt_set_flow_format(struct ds *string,
-                              const struct nx_set_flow_format *nsff)
+ofp_print_nxt_set_flow_format(struct ds *string, const struct ofp_header *oh)
 {
-    uint32_t format = ntohl(nsff->format);
-
-    ds_put_cstr(string, " format=");
-    if (ofputil_nx_flow_format_is_valid(format)) {
-        ds_put_cstr(string, ofputil_nx_flow_format_to_string(format));
-    } else {
-        ds_put_format(string, "%"PRIu32, format);
-    }
-
+    enum ofputil_protocol p = ofputil_decode_nx_set_flow_format(oh);
+    ds_put_format(string, " format=%s",
+                  p == OFPUTIL_P_OF10_STD ? "openflow10"
+                  : p == OFPUTIL_P_OF10_NXM ? "nxm"
+                  : "(unknown)");
     return 0;
 }
 
@@ -3732,10 +3727,10 @@  ofp_to_string__(const struct ofp_header *oh,
         return ofp_print_ofpst_port_desc_reply(string, oh);
 
     case OFPTYPE_FLOW_MOD_TABLE_ID:
-        return ofp_print_nxt_flow_mod_table_id(string, ofpmsg_body(oh));
+        return ofp_print_nxt_flow_mod_table_id(string, oh);
 
     case OFPTYPE_SET_FLOW_FORMAT:
-        return ofp_print_nxt_set_flow_format(string, ofpmsg_body(oh));
+        return ofp_print_nxt_set_flow_format(string, oh);
 
     case OFPTYPE_SET_PACKET_IN_FORMAT:
         return ofp_print_nxt_set_packet_in_format(string, ofpmsg_body(oh));
diff --git a/lib/ofp-protocol.c b/lib/ofp-protocol.c
index 175318380f3b..32cf1c3fca40 100644
--- a/lib/ofp-protocol.c
+++ b/lib/ofp-protocol.c
@@ -19,11 +19,15 @@ 
 #include <ctype.h>
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/ofp-flow.h"
+#include "openvswitch/ofp-msgs.h"
+#include "openvswitch/ofpbuf.h"
 #include "openvswitch/vlog.h"
 #include "util.h"
 
 VLOG_DEFINE_THIS_MODULE(ofp_protocol);
 
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+
 /* Protocols. */
 
 struct proto_abbrev {
@@ -555,7 +559,7 @@  ofputil_format_version_bitmap_names(struct ds *msg, uint32_t bitmap)
 {
     ofputil_format_version_bitmap__(msg, bitmap, ofputil_format_version_name);
 }
-
+
 /* Returns an OpenFlow message that, sent on an OpenFlow connection whose
  * protocol is 'current', at least partly transitions the protocol to 'want'.
  * Stores in '*next' the protocol that will be in effect on the OpenFlow
@@ -585,13 +589,10 @@  ofputil_encode_set_protocol(enum ofputil_protocol current,
     want_base = ofputil_protocol_to_base(want);
     if (cur_base != want_base) {
         *next = ofputil_protocol_set_base(current, want_base);
-
         switch (want_base) {
         case OFPUTIL_P_OF10_NXM:
-            return ofputil_encode_nx_set_flow_format(NXFF_NXM);
-
         case OFPUTIL_P_OF10_STD:
-            return ofputil_encode_nx_set_flow_format(NXFF_OPENFLOW10);
+            return ofputil_encode_nx_set_flow_format(want_base);
 
         case OFPUTIL_P_OF11_STD:
         case OFPUTIL_P_OF12_OXM:
@@ -613,7 +614,7 @@  ofputil_encode_set_protocol(enum ofputil_protocol current,
     want_tid = (want & OFPUTIL_P_TID) != 0;
     if (cur_tid != want_tid) {
         *next = ofputil_protocol_set_tid(current, want_tid);
-        return ofputil_make_flow_mod_table_id(want_tid);
+        return ofputil_encode_nx_flow_mod_table_id(want_tid);
     }
 
     ovs_assert(current == want);
@@ -621,3 +622,116 @@  ofputil_encode_set_protocol(enum ofputil_protocol current,
     *next = current;
     return NULL;
 }
+
+enum nx_flow_format {
+    NXFF_OPENFLOW10 = 0,         /* Standard OpenFlow 1.0 compatible. */
+    NXFF_NXM = 2                 /* Nicira extended match. */
+};
+
+/* Returns an NXT_SET_FLOW_FORMAT message that can be used to set the flow
+ * format to 'nxff'.  */
+struct ofpbuf *
+ofputil_encode_nx_set_flow_format(enum ofputil_protocol protocol)
+{
+    struct ofpbuf *msg = ofpraw_alloc(OFPRAW_NXT_SET_FLOW_FORMAT,
+                                      OFP10_VERSION, 0);
+    ovs_be32 *nxff = ofpbuf_put_uninit(msg, sizeof *nxff);
+    if (protocol == OFPUTIL_P_OF10_STD) {
+        *nxff = htonl(NXFF_OPENFLOW10);
+    } else if (protocol == OFPUTIL_P_OF10_NXM) {
+        *nxff = htonl(NXFF_NXM);
+    } else {
+        OVS_NOT_REACHED();
+    }
+
+    return msg;
+}
+
+/* Returns the protocol specified in the NXT_SET_FLOW_FORMAT message at 'oh'
+ * (either OFPUTIL_P_OF10_STD or OFPUTIL_P_OF10_NXM) or 0 if the message is
+ * invalid. */
+enum ofputil_protocol
+ofputil_decode_nx_set_flow_format(const struct ofp_header *oh)
+{
+    struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
+    ovs_assert(ofpraw_pull_assert(&b) == OFPRAW_NXT_SET_FLOW_FORMAT);
+
+    ovs_be32 *flow_formatp = ofpbuf_pull(&b, sizeof *flow_formatp);
+    uint32_t flow_format = ntohl(*flow_formatp);
+    switch (flow_format) {
+    case NXFF_OPENFLOW10:
+        return OFPUTIL_P_OF10_STD;
+
+    case NXFF_NXM:
+        return OFPUTIL_P_OF10_NXM;
+
+    default:
+        VLOG_WARN_RL(&rl, "NXT_SET_FLOW_FORMAT message specified invalid "
+                     "flow format %"PRIu32, flow_format);
+        return 0;
+    }
+}
+
+/* These functions work with the Open vSwitch extension feature called
+ * "flow_mod_table_id", which allows a controller to specify the OpenFlow table
+ * to which a flow should be added, instead of having the switch decide which
+ * table is most appropriate as required by OpenFlow 1.0.  Because NXM was
+ * designed as an extension to OpenFlow 1.0, the extension applies equally to
+ * ofp10_flow_mod and nx_flow_mod.  By default, the extension is disabled.
+ *
+ * When this feature is enabled, Open vSwitch treats struct ofp10_flow_mod's
+ * and struct nx_flow_mod's 16-bit 'command' member as two separate fields.
+ * The upper 8 bits are used as the table ID, the lower 8 bits specify the
+ * command as usual.  A table ID of 0xff is treated like a wildcarded table ID.
+ *
+ * The specific treatment of the table ID depends on the type of flow mod:
+ *
+ *    - OFPFC_ADD: Given a specific table ID, the flow is always placed in that
+ *      table.  If an identical flow already exists in that table only, then it
+ *      is replaced.  If the flow cannot be placed in the specified table,
+ *      either because the table is full or because the table cannot support
+ *      flows of the given type, the switch replies with an OFPFMFC_TABLE_FULL
+ *      error.  (A controller can distinguish these cases by comparing the
+ *      current and maximum number of entries reported in ofp_table_stats.)
+ *
+ *      If the table ID is wildcarded, the switch picks an appropriate table
+ *      itself.  If an identical flow already exist in the selected flow table,
+ *      then it is replaced.  The choice of table might depend on the flows
+ *      that are already in the switch; for example, if one table fills up then
+ *      the switch might fall back to another one.
+ *
+ *    - OFPFC_MODIFY, OFPFC_DELETE: Given a specific table ID, only flows
+ *      within that table are matched and modified or deleted.  If the table ID
+ *      is wildcarded, flows within any table may be matched and modified or
+ *      deleted.
+ *
+ *    - OFPFC_MODIFY_STRICT, OFPFC_DELETE_STRICT: Given a specific table ID,
+ *      only a flow within that table may be matched and modified or deleted.
+ *      If the table ID is wildcarded and exactly one flow within any table
+ *      matches, then it is modified or deleted; if flows in more than one
+ *      table match, then none is modified or deleted.
+ */
+
+/* Returns an OpenFlow message that can be used to turn the flow_mod_table_id
+ * extension on or off (according to 'enable'). */
+struct ofpbuf *
+ofputil_encode_nx_flow_mod_table_id(bool enable)
+{
+    struct ofpbuf *msg = ofpraw_alloc(OFPRAW_NXT_FLOW_MOD_TABLE_ID,
+                                      OFP10_VERSION, 0);
+    uint8_t *p = ofpbuf_put_zeros(msg, 8);
+    *p = enable;
+    return msg;
+}
+
+/* Decodes the NXT_SET_MOD_TABLE_ID message at 'oh'.  Returns the message's
+ * argument, that is, whether the flow_mod_table_id feature should be
+ * enabled. */
+bool
+ofputil_decode_nx_flow_mod_table_id(const struct ofp_header *oh)
+{
+    struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
+    ovs_assert(ofpraw_pull_assert(&b) == OFPRAW_NXT_FLOW_MOD_TABLE_ID);
+    uint8_t *enable = ofpbuf_pull(&b, 8);
+    return *enable != 0;
+}
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index f4604209e43f..09e5cac6a0c7 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5875,12 +5875,9 @@  static enum ofperr
 handle_nxt_flow_mod_table_id(struct ofconn *ofconn,
                              const struct ofp_header *oh)
 {
-    const struct nx_flow_mod_table_id *msg = ofpmsg_body(oh);
-    enum ofputil_protocol cur, next;
-
-    cur = ofconn_get_protocol(ofconn);
-    next = ofputil_protocol_set_tid(cur, msg->set != 0);
-    ofconn_set_protocol(ofconn, next);
+    bool enable = ofputil_decode_nx_flow_mod_table_id(oh);
+    enum ofputil_protocol cur = ofconn_get_protocol(ofconn);
+    ofconn_set_protocol(ofconn, ofputil_protocol_set_tid(cur, enable));
 
     return 0;
 }
@@ -5888,18 +5885,13 @@  handle_nxt_flow_mod_table_id(struct ofconn *ofconn,
 static enum ofperr
 handle_nxt_set_flow_format(struct ofconn *ofconn, const struct ofp_header *oh)
 {
-    const struct nx_set_flow_format *msg = ofpmsg_body(oh);
-    enum ofputil_protocol cur, next;
-    enum ofputil_protocol next_base;
-
-    next_base = ofputil_nx_flow_format_to_protocol(ntohl(msg->format));
+    enum ofputil_protocol next_base = ofputil_decode_nx_set_flow_format(oh);
     if (!next_base) {
         return OFPERR_OFPBRC_EPERM;
     }
 
-    cur = ofconn_get_protocol(ofconn);
-    next = ofputil_protocol_set_base(cur, next_base);
-    ofconn_set_protocol(ofconn, next);
+    enum ofputil_protocol cur = ofconn_get_protocol(ofconn);
+    ofconn_set_protocol(ofconn, ofputil_protocol_set_base(cur, next_base));
 
     return 0;
 }