ofp-packet: Better abstract packet-in format.

This commit relieves the caller of code that deals with the format of
packet-in messages from some of the burden of understanding the packet
format.  It also renames the constants to appear to be at a higher level of
abstraction.

Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>

10 files changed, 97 insertions, 99 deletions

diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index ddb68aa25..1f5cbbf67 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -113,31 +113,6 @@ enum nx_hash_fields {
 
 };
 
-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). */
-    NXPIF_NXT_PACKET_IN2 = 2,   /* NXT_PACKET_IN2 (since OVS v2.6). */
-};
-
-/* NXT_SET_PACKET_IN_FORMAT request.
- *
- * For any given OpenFlow version, Open vSwitch supports multiple formats for
- * "packet-in" messages.  The default is always the standard format for the
- * OpenFlow version in question, but NXT_SET_PACKET_IN_FORMAT can be used to
- * set an alternative format.
- *
- * From OVS v1.1 to OVS v2.5, this request was only honored for OpenFlow 1.0.
- * Requests to set format NXPIF_NXT_PACKET_IN were accepted for OF1.1+ but they
- * had no effect.  (Requests to set formats other than NXPIF_STANDARD or
- * NXPIF_NXT_PACKET_IN were rejected with OFPBRC_EPERM.)
- *
- * From OVS v2.6 onward, this request is honored for all OpenFlow versions.
- */
-struct nx_set_packet_in_format {
-    ovs_be32 format;            /* One of NXPIF_*. */
-};
-OFP_ASSERT(sizeof(struct nx_set_packet_in_format) == 4);
-
 /* NXT_PACKET_IN (analogous to OFPT_PACKET_IN).
  *
  * NXT_PACKET_IN is similar to the OpenFlow 1.2 OFPT_PACKET_IN.  The
diff --git a/include/openvswitch/ofp-msgs.h b/include/openvswitch/ofp-msgs.h
index 23f3015ae..5998c4c22 100644
--- a/include/openvswitch/ofp-msgs.h
+++ b/include/openvswitch/ofp-msgs.h
@@ -449,7 +449,7 @@ enum ofpraw {
     /* NXT 1.0+ (15): uint8_t[8]. */
     OFPRAW_NXT_FLOW_MOD_TABLE_ID,
 
-    /* NXT 1.0+ (16): struct nx_set_packet_in_format. */
+    /* NXT 1.0+ (16): ovs_be32. */
     OFPRAW_NXT_SET_PACKET_IN_FORMAT,
 
     /* NXT 1.0+ (18): void. */
diff --git a/include/openvswitch/ofp-packet.h b/include/openvswitch/ofp-packet.h
index d6e78fb9e..f96e36613 100644
--- a/include/openvswitch/ofp-packet.h
+++ b/include/openvswitch/ofp-packet.h
@@ -30,12 +30,33 @@ extern "C" {
 
 struct vl_mff_map;
 struct ofputil_table_map;
+
+/* Packet-in format.
+ *
+ * For any given OpenFlow version, Open vSwitch supports multiple formats for
+ * "packet-in" messages.  The default is always the standard format for the
+ * OpenFlow version in question, but the Open vSwitch extension request
+ * NXT_SET_PACKET_IN_FORMAT can be used to set an alternative format.
+ *
+ * From OVS v1.1 to OVS v2.5, this request was only honored for OpenFlow 1.0.
+ * Requests to set format NXPIF_NXT_PACKET_IN were accepted for OF1.1+ but they
+ * had no effect.  (Requests to set formats other than NXPIF_STANDARD or
+ * NXPIF_NXT_PACKET_IN were rejected with OFPBRC_EPERM.)
+ *
+ * From OVS v2.6 onward, this request is honored for all OpenFlow versions.
+ */
+enum ofputil_packet_in_format {
+    OFPUTIL_PACKET_IN_STD = 0,  /* OFPT_PACKET_IN for this OpenFlow version. */
+    OFPUTIL_PACKET_IN_NXT = 1,  /* NXT_PACKET_IN (since OVS v1.1). */
+    OFPUTIL_PACKET_IN_NXT2 = 2, /* NXT_PACKET_IN2 (since OVS v2.6). */
+};
 
-bool ofputil_packet_in_format_is_valid(enum nx_packet_in_format);
 int ofputil_packet_in_format_from_string(const char *);
-const char *ofputil_packet_in_format_to_string(enum nx_packet_in_format);
-struct ofpbuf *ofputil_make_set_packet_in_format(enum ofp_version,
-                                                 enum nx_packet_in_format);
+const char *ofputil_packet_in_format_to_string(enum ofputil_packet_in_format);
+struct ofpbuf *ofputil_encode_set_packet_in_format(
+    enum ofp_version, enum ofputil_packet_in_format);
+enum ofperr ofputil_decode_set_packet_in_format(
+    const struct ofp_header *, enum ofputil_packet_in_format *);
 
 /* Abstract packet-in message.
  *
@@ -124,7 +145,7 @@ struct ofputil_packet_in_private {
 struct ofpbuf *ofputil_encode_packet_in_private(
     const struct ofputil_packet_in_private *,
     enum ofputil_protocol protocol,
-    enum nx_packet_in_format);
+    enum ofputil_packet_in_format);
 
 enum ofperr ofputil_decode_packet_in_private(
     const struct ofp_header *, bool loose,
diff --git a/lib/ofp-packet.c b/lib/ofp-packet.c
index 05cbea15b..100f7c569 100644
--- a/lib/ofp-packet.c
+++ b/lib/ofp-packet.c
@@ -34,28 +34,15 @@ VLOG_DEFINE_THIS_MODULE(ofp_packet);
 
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 
-bool
-ofputil_packet_in_format_is_valid(enum nx_packet_in_format packet_in_format)
-{
-    switch (packet_in_format) {
-    case NXPIF_STANDARD:
-    case NXPIF_NXT_PACKET_IN:
-    case NXPIF_NXT_PACKET_IN2:
-        return true;
-    }
-
-    return false;
-}
-
 const char *
-ofputil_packet_in_format_to_string(enum nx_packet_in_format packet_in_format)
+ofputil_packet_in_format_to_string(enum ofputil_packet_in_format format)
 {
-    switch (packet_in_format) {
-    case NXPIF_STANDARD:
+    switch (format) {
+    case OFPUTIL_PACKET_IN_STD:
         return "standard";
-    case NXPIF_NXT_PACKET_IN:
+    case OFPUTIL_PACKET_IN_NXT:
         return "nxt_packet_in";
-    case NXPIF_NXT_PACKET_IN2:
+    case OFPUTIL_PACKET_IN_NXT2:
         return "nxt_packet_in2";
     default:
         OVS_NOT_REACHED();
@@ -66,27 +53,48 @@ int
 ofputil_packet_in_format_from_string(const char *s)
 {
     return (!strcmp(s, "standard") || !strcmp(s, "openflow10")
-            ? NXPIF_STANDARD
+            ? OFPUTIL_PACKET_IN_STD
             : !strcmp(s, "nxt_packet_in") || !strcmp(s, "nxm")
-            ? NXPIF_NXT_PACKET_IN
+            ? OFPUTIL_PACKET_IN_NXT
             : !strcmp(s, "nxt_packet_in2")
-            ? NXPIF_NXT_PACKET_IN2
+            ? OFPUTIL_PACKET_IN_NXT2
             : -1);
 }
 
 struct ofpbuf *
-ofputil_make_set_packet_in_format(enum ofp_version ofp_version,
-                                  enum nx_packet_in_format packet_in_format)
+ofputil_encode_set_packet_in_format(enum ofp_version ofp_version,
+                                    enum ofputil_packet_in_format format)
 {
-    struct nx_set_packet_in_format *spif;
-    struct ofpbuf *msg;
-
-    msg = ofpraw_alloc(OFPRAW_NXT_SET_PACKET_IN_FORMAT, ofp_version, 0);
-    spif = ofpbuf_put_zeros(msg, sizeof *spif);
-    spif->format = htonl(packet_in_format);
+    struct ofpbuf *msg = ofpraw_alloc(OFPRAW_NXT_SET_PACKET_IN_FORMAT,
+                                      ofp_version, 0);
+    ovs_be32 *spif = ofpbuf_put_uninit(msg, sizeof *spif);
+    *spif = htonl(format);
 
     return msg;
 }
+
+enum ofperr
+ofputil_decode_set_packet_in_format(const struct ofp_header *oh,
+                                    enum ofputil_packet_in_format *format)
+{
+    struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
+    ovs_assert(ofpraw_pull_assert(&b) == OFPRAW_NXT_SET_PACKET_IN_FORMAT);
+    ovs_be32 *spifp = ofpbuf_pull(&b, sizeof *spifp);
+    uint32_t spif = ntohl(*spifp);
+
+    switch (spif) {
+    case OFPUTIL_PACKET_IN_STD:
+    case OFPUTIL_PACKET_IN_NXT:
+    case OFPUTIL_PACKET_IN_NXT2:
+        *format = spif;
+        return 0;
+
+    default:
+        VLOG_WARN_RL(&rl, "NXT_SET_PACKET_IN_FORMAT message specified invalid "
+                     "packet-in format %"PRIu32, spif);
+        return OFPERR_OFPBRC_EPERM;
+    }
+}
 
 /* The caller has done basic initialization of '*pin'; the other output
  * arguments needs to be initialized. */
@@ -619,7 +627,7 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
 }
 
 /* Converts abstract ofputil_packet_in_private 'pin' into a PACKET_IN message
- * for 'protocol', using the packet-in format specified by 'packet_in_format'.
+ * for 'protocol', using the packet-in format specified by 'format'.
  *
  * This function is really meant only for use by ovs-vswitchd.  To any other
  * code, the "continuation" data, i.e. the data that is in struct
@@ -632,13 +640,13 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
 struct ofpbuf *
 ofputil_encode_packet_in_private(const struct ofputil_packet_in_private *pin,
                                  enum ofputil_protocol protocol,
-                                 enum nx_packet_in_format packet_in_format)
+                                 enum ofputil_packet_in_format format)
 {
     enum ofp_version version = ofputil_protocol_to_ofp_version(protocol);
 
     struct ofpbuf *msg;
-    switch (packet_in_format) {
-    case NXPIF_STANDARD:
+    switch (format) {
+    case OFPUTIL_PACKET_IN_STD:
         switch (protocol) {
         case OFPUTIL_P_OF10_STD:
         case OFPUTIL_P_OF10_STD_TID:
@@ -664,11 +672,11 @@ ofputil_encode_packet_in_private(const struct ofputil_packet_in_private *pin,
         }
         break;
 
-    case NXPIF_NXT_PACKET_IN:
+    case OFPUTIL_PACKET_IN_NXT:
         msg = ofputil_encode_nx_packet_in(&pin->base, version);
         break;
 
-    case NXPIF_NXT_PACKET_IN2:
+    case OFPUTIL_PACKET_IN_NXT2:
         return ofputil_encode_nx_packet_in2(pin, version,
                                             pin->base.packet_len);
 
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index b13bc3803..c0bfa9284 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2287,18 +2287,15 @@ ofp_print_nxt_set_flow_format(struct ds *string, const struct ofp_header *oh)
 
 static enum ofperr
 ofp_print_nxt_set_packet_in_format(struct ds *string,
-                                   const struct nx_set_packet_in_format *nspf)
+                                   const struct ofp_header *oh)
 {
-    uint32_t format = ntohl(nspf->format);
-
-    ds_put_cstr(string, " format=");
-    if (ofputil_packet_in_format_is_valid(format)) {
-        ds_put_cstr(string, ofputil_packet_in_format_to_string(format));
-    } else {
-        ds_put_format(string, "%"PRIu32, format);
+    enum ofputil_packet_in_format format;
+    enum ofperr error = ofputil_decode_set_packet_in_format(oh, &format);
+    if (!error) {
+        ds_put_format(string, " format=%s",
+                      ofputil_packet_in_format_to_string(format));
     }
-
-    return 0;
+    return error;
 }
 
 /* Returns a string form of 'reason'.  The return value is either a statically
@@ -3733,7 +3730,7 @@ ofp_to_string__(const struct ofp_header *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));
+        return ofp_print_nxt_set_packet_in_format(string, oh);
 
     case OFPTYPE_FLOW_AGE:
         break;
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index bf43fbd73..964b8c8d8 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -73,7 +73,7 @@ struct ofconn {
     /* OpenFlow state. */
     enum ofp12_controller_role role;           /* Role. */
     enum ofputil_protocol protocol; /* Current protocol variant. */
-    enum nx_packet_in_format packet_in_format; /* OFPT_PACKET_IN format. */
+    enum ofputil_packet_in_format packet_in_format;
 
     /* OFPT_PACKET_IN related data. */
     struct rconn_packet_counter *packet_in_counter; /* # queued on 'rconn'. */
@@ -1044,7 +1044,7 @@ ofconn_set_protocol(struct ofconn *ofconn, enum ofputil_protocol protocol)
  * NXPIF_*.
  *
  * The default, if no other format has been set, is NXPIF_STANDARD. */
-enum nx_packet_in_format
+enum ofputil_packet_in_format
 ofconn_get_packet_in_format(struct ofconn *ofconn)
 {
     return ofconn->packet_in_format;
@@ -1054,7 +1054,7 @@ ofconn_get_packet_in_format(struct ofconn *ofconn)
  * NXPIF_*). */
 void
 ofconn_set_packet_in_format(struct ofconn *ofconn,
-                            enum nx_packet_in_format packet_in_format)
+                            enum ofputil_packet_in_format packet_in_format)
 {
     ofconn->packet_in_format = packet_in_format;
 }
@@ -1303,7 +1303,7 @@ ofconn_flush(struct ofconn *ofconn)
 
     ofconn->role = OFPCR12_ROLE_EQUAL;
     ofconn_set_protocol(ofconn, OFPUTIL_P_NONE);
-    ofconn->packet_in_format = NXPIF_STANDARD;
+    ofconn->packet_in_format = OFPUTIL_PACKET_IN_STD;
 
     rconn_packet_counter_destroy(ofconn->packet_in_counter);
     ofconn->packet_in_counter = rconn_packet_counter_create();
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index c41189216..2405fbd79 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -112,8 +112,9 @@ void ofconn_set_role(struct ofconn *, enum ofp12_controller_role);
 enum ofputil_protocol ofconn_get_protocol(const struct ofconn *);
 void ofconn_set_protocol(struct ofconn *, enum ofputil_protocol);
 
-enum nx_packet_in_format ofconn_get_packet_in_format(struct ofconn *);
-void ofconn_set_packet_in_format(struct ofconn *, enum nx_packet_in_format);
+enum ofputil_packet_in_format ofconn_get_packet_in_format(struct ofconn *);
+void ofconn_set_packet_in_format(struct ofconn *,
+                                 enum ofputil_packet_in_format);
 
 void ofconn_set_controller_id(struct ofconn *, uint16_t controller_id);
 
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 531e2e7f1..57ce3c81f 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5890,16 +5890,12 @@ static enum ofperr
 handle_nxt_set_packet_in_format(struct ofconn *ofconn,
                                 const struct ofp_header *oh)
 {
-    const struct nx_set_packet_in_format *msg = ofpmsg_body(oh);
-    uint32_t format;
-
-    format = ntohl(msg->format);
-    if (!ofputil_packet_in_format_is_valid(format)) {
-        return OFPERR_OFPBRC_EPERM;
+    enum ofputil_packet_in_format format;
+    enum ofperr error = ofputil_decode_set_packet_in_format(oh, &format);
+    if (!error) {
+        ofconn_set_packet_in_format(ofconn, format);
     }
-
-    ofconn_set_packet_in_format(ofconn, format);
-    return 0;
+    return error;
 }
 
 static enum ofperr
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index b4dbd8c29..e8bf1c698 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -130,8 +130,8 @@ pinctrl_setup(void)
                            rconn_get_version(swconn), 0));
 
     /* Set a packet-in format that supports userdata.  */
-    queue_msg(ofputil_make_set_packet_in_format(rconn_get_version(swconn),
-                                                NXPIF_NXT_PACKET_IN2));
+    queue_msg(ofputil_encode_set_packet_in_format(rconn_get_version(swconn),
+                                                  OFPUTIL_PACKET_IN_NXT2));
 }
 
 static void
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 358177ed5..77aa49649 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1820,13 +1820,13 @@ ofctl_del_flows(struct ovs_cmdl_context *ctx)
 
 static bool
 set_packet_in_format(struct vconn *vconn,
-                     enum nx_packet_in_format packet_in_format,
+                     enum ofputil_packet_in_format packet_in_format,
                      bool must_succeed)
 {
     struct ofpbuf *spif;
 
-    spif = ofputil_make_set_packet_in_format(vconn_get_version(vconn),
-                                             packet_in_format);
+    spif = ofputil_encode_set_packet_in_format(vconn_get_version(vconn),
+                                               packet_in_format);
     if (must_succeed) {
         transact_noreply(vconn, spif);
     } else {
@@ -2303,13 +2303,13 @@ ofctl_monitor(struct ovs_cmdl_context *ctx)
         set_packet_in_format(vconn, preferred_packet_in_format, true);
     } else {
         /* Otherwise, we always prefer NXT_PACKET_IN2. */
-        if (!set_packet_in_format(vconn, NXPIF_NXT_PACKET_IN2, false)) {
+        if (!set_packet_in_format(vconn, OFPUTIL_PACKET_IN_NXT2, false)) {
             /* We can't get NXT_PACKET_IN2.  For OpenFlow 1.0 only, request
              * NXT_PACKET_IN.  (Before 2.6, Open vSwitch will accept a request
              * for NXT_PACKET_IN with OF1.1+, but even after that it still
              * sends packet-ins in the OpenFlow native format.) */
             if (vconn_get_version(vconn) == OFP10_VERSION) {
-                set_packet_in_format(vconn, NXPIF_NXT_PACKET_IN, false);
+                set_packet_in_format(vconn, OFPUTIL_PACKET_IN_NXT, false);
             }
         }
     }