ofp-util: Abstract flow_mod OFPFF_* flags.

The OFPFF_* flags used in flow_mods are just confusing enough that it
seems worthwhile to try to abstract them out.  In particular:

    * OFPFF_EMERG was introduced in OF1.0, deleted in OF1.1, and then
      its bit was reused for a different purpose in OF1.2.

    * OFPFF_RESET_COUNTS was introduced in OF1.2 but the semantics that it
      specifies are implied by "add" commands in earlier versions, so
      proper translation requires the OpenFlow version number and flow_mod
      command.

This commit does the abstraction.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>

7 files changed, 162 insertions, 53 deletions

diff --git a/lib/learn.c b/lib/learn.c
index 49d9efdba..68d95cbed 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -97,12 +97,23 @@ learn_from_openflow(const struct nx_action_learn *nal, struct ofpbuf *ofpacts)
     learn->hard_timeout = ntohs(nal->hard_timeout);
     learn->priority = ntohs(nal->priority);
     learn->cookie = ntohll(nal->cookie);
-    learn->flags = ntohs(nal->flags);
     learn->table_id = nal->table_id;
     learn->fin_idle_timeout = ntohs(nal->fin_idle_timeout);
     learn->fin_hard_timeout = ntohs(nal->fin_hard_timeout);
 
-    if (learn->flags & ~OFPFF_SEND_FLOW_REM || learn->table_id == 0xff) {
+    /* We only support "send-flow-removed" for now. */
+    switch (ntohs(nal->flags)) {
+    case 0:
+        learn->flags = 0;
+        break;
+    case OFPFF_SEND_FLOW_REM:
+        learn->flags = OFPUTIL_FF_SEND_FLOW_REM;
+        break;
+    default:
+        return OFPERR_OFPBAC_BAD_ARGUMENT;
+    }
+
+    if (learn->table_id == 0xff) {
         return OFPERR_OFPBAC_BAD_ARGUMENT;
     }
 
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index ca33ca897..101c33dd1 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -414,7 +414,7 @@ struct ofpact_learn {
     uint16_t hard_timeout;      /* Max time before discarding (seconds). */
     uint16_t priority;          /* Priority level of flow entry. */
     uint64_t cookie;            /* Cookie for new flow. */
-    uint16_t flags;             /* Either 0 or OFPFF_SEND_FLOW_REM. */
+    enum ofputil_flow_mod_flags flags;
     uint8_t table_id;           /* Table to insert flow entry. */
     uint16_t fin_idle_timeout;  /* Idle timeout after FIN, if nonzero. */
     uint16_t fin_hard_timeout;  /* Hard timeout after FIN, if nonzero. */
diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
index 79acd303d..c80a75eb0 100644
--- a/lib/ofp-errors.h
+++ b/lib/ofp-errors.h
@@ -349,7 +349,8 @@ enum ofperr {
     /* OF1.0(3,4), OF1.1+(5,6).  Unsupported or unknown command. */
     OFPERR_OFPFMFC_BAD_COMMAND,
 
-    /* OF1.2+(5,7).  Unsupported or unknown flags. */
+    /* NX1.0(3,258), NX1.1(5,258), OF1.2+(5,7).  Unsupported or unknown
+     * flags. */
     OFPERR_OFPFMFC_BAD_FLAGS,
 
     /* OF1.0(3,5).  Unsupported action list - cannot process in the order
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index dd0738ca2..c854066d8 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1161,15 +1161,15 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string)
                 match_set_nw_proto(&fm->match, p->nw_proto);
             }
         } else if (fields & F_FLAGS && !strcmp(name, "send_flow_rem")) {
-            fm->flags |= OFPFF_SEND_FLOW_REM;
+            fm->flags |= OFPUTIL_FF_SEND_FLOW_REM;
         } else if (fields & F_FLAGS && !strcmp(name, "check_overlap")) {
-            fm->flags |= OFPFF_CHECK_OVERLAP;
+            fm->flags |= OFPUTIL_FF_CHECK_OVERLAP;
         } else if (fields & F_FLAGS && !strcmp(name, "reset_counts")) {
-            fm->flags |= OFPFF12_RESET_COUNTS;
+            fm->flags |= OFPUTIL_FF_RESET_COUNTS;
         } else if (fields & F_FLAGS && !strcmp(name, "no_packet_counts")) {
-            fm->flags |= OFPFF13_NO_PKT_COUNTS;
+            fm->flags |= OFPUTIL_FF_NO_PKT_COUNTS;
         } else if (fields & F_FLAGS && !strcmp(name, "no_byte_counts")) {
-            fm->flags |= OFPFF13_NO_BYT_COUNTS;
+            fm->flags |= OFPUTIL_FF_NO_BYT_COUNTS;
         } else {
             char *value;
 
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 23ba9d468..560762bfa 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -724,30 +724,23 @@ ofp10_match_to_string(const struct ofp10_match *om, int verbosity)
 }
 
 static void
-ofp_print_flow_flags(struct ds *s, uint16_t flags)
+ofp_print_flow_flags(struct ds *s, enum ofputil_flow_mod_flags flags)
 {
-    if (flags & OFPFF_SEND_FLOW_REM) {
+    if (flags & OFPUTIL_FF_SEND_FLOW_REM) {
         ds_put_cstr(s, "send_flow_rem ");
     }
-    if (flags & OFPFF_CHECK_OVERLAP) {
+    if (flags & OFPUTIL_FF_CHECK_OVERLAP) {
         ds_put_cstr(s, "check_overlap ");
     }
-    if (flags & OFPFF12_RESET_COUNTS) {
+    if (flags & OFPUTIL_FF_RESET_COUNTS) {
         ds_put_cstr(s, "reset_counts ");
     }
-    if (flags & OFPFF13_NO_PKT_COUNTS) {
+    if (flags & OFPUTIL_FF_NO_PKT_COUNTS) {
         ds_put_cstr(s, "no_packet_counts ");
     }
-    if (flags & OFPFF13_NO_BYT_COUNTS) {
+    if (flags & OFPUTIL_FF_NO_BYT_COUNTS) {
         ds_put_cstr(s, "no_byte_counts ");
     }
-
-    flags &= ~(OFPFF_SEND_FLOW_REM | OFPFF_CHECK_OVERLAP
-               | OFPFF12_RESET_COUNTS
-               | OFPFF13_NO_PKT_COUNTS | OFPFF13_NO_BYT_COUNTS);
-    if (flags) {
-        ds_put_format(s, "flags:0x%"PRIx16" ", flags);
-    }
 }
 
 static void
@@ -848,9 +841,14 @@ ofp_print_flow_mod(struct ds *s, const struct ofp_header *oh, int verbosity)
         ofputil_format_port(fm.out_port, s);
         ds_put_char(s, ' ');
     }
-    if (fm.flags != 0) {
-        ofp_print_flow_flags(s, fm.flags);
+
+    if (oh->version == OFP10_VERSION || oh->version == OFP11_VERSION) {
+        /* Don't print the reset_counts flag for OF1.0 and OF1.1 because those
+         * versions don't really have such a flag and printing one is likely to
+         * confuse people. */
+        fm.flags &= ~OFPUTIL_FF_RESET_COUNTS;
     }
+    ofp_print_flow_flags(s, fm.flags);
 
     ofpacts_format(fm.ofpacts, fm.ofpacts_len, s);
     ofpbuf_uninit(&ofpacts);
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index edda3389c..6c48d171b 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1554,6 +1554,74 @@ ofputil_make_flow_mod_table_id(bool flow_mod_table_id)
     return msg;
 }
 
+struct ofputil_flow_mod_flag {
+    uint16_t raw_flag;
+    enum ofp_version min_version, max_version;
+    enum ofputil_flow_mod_flags flag;
+};
+
+static const struct ofputil_flow_mod_flag ofputil_flow_mod_flags[] = {
+    { OFPFF_SEND_FLOW_REM,   OFP10_VERSION, 0, OFPUTIL_FF_SEND_FLOW_REM },
+    { OFPFF_CHECK_OVERLAP,   OFP10_VERSION, 0, OFPUTIL_FF_CHECK_OVERLAP },
+    { OFPFF10_EMERG,         OFP10_VERSION, OFP10_VERSION,
+      OFPUTIL_FF_EMERG },
+    { OFPFF12_RESET_COUNTS,  OFP12_VERSION, 0, OFPUTIL_FF_RESET_COUNTS },
+    { OFPFF13_NO_PKT_COUNTS, OFP13_VERSION, 0, OFPUTIL_FF_NO_PKT_COUNTS },
+    { OFPFF13_NO_BYT_COUNTS, OFP13_VERSION, 0, OFPUTIL_FF_NO_BYT_COUNTS },
+    { 0, 0, 0, 0 },
+};
+
+static enum ofperr
+ofputil_decode_flow_mod_flags(ovs_be16 raw_flags_,
+                              enum ofp_flow_mod_command command,
+                              enum ofp_version version,
+                              enum ofputil_flow_mod_flags *flagsp)
+{
+    uint16_t raw_flags = ntohs(raw_flags_);
+    const struct ofputil_flow_mod_flag *f;
+
+    *flagsp = 0;
+    for (f = ofputil_flow_mod_flags; f->raw_flag; f++) {
+        if (raw_flags & f->raw_flag
+            && version >= f->min_version
+            && (!f->max_version || version <= f->max_version)) {
+            raw_flags &= ~f->raw_flag;
+            *flagsp |= f->flag;
+        }
+    }
+
+    /* In OF1.0 and OF1.1, "add" always resets counters, and other commands
+     * never do.
+     *
+     * In OF1.2 and later, OFPFF12_RESET_COUNTS controls whether each command
+     * resets counters. */
+    if ((version == OFP10_VERSION || version == OFP11_VERSION)
+        && command == OFPFC_ADD) {
+        *flagsp |= OFPUTIL_FF_RESET_COUNTS;
+    }
+
+    return raw_flags ? OFPERR_OFPFMFC_BAD_FLAGS : 0;
+}
+
+static ovs_be16
+ofputil_encode_flow_mod_flags(enum ofputil_flow_mod_flags flags,
+                              enum ofp_version version)
+{
+    const struct ofputil_flow_mod_flag *f;
+    uint16_t raw_flags;
+
+    raw_flags = 0;
+    for (f = ofputil_flow_mod_flags; f->raw_flag; f++) {
+        if (f->flag & flags
+            && version >= f->min_version
+            && (!f->max_version || version <= f->max_version)) {
+            raw_flags |= f->raw_flag;
+        }
+    }
+
+    return htons(raw_flags);
+}
+
 /* Converts an OFPT_FLOW_MOD or NXT_FLOW_MOD message 'oh' into an abstract
  * flow_mod in 'fm'.  Returns 0 if successful, otherwise an OpenFlow error
  * code.
@@ -1570,7 +1638,8 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
                         enum ofputil_protocol protocol,
                         struct ofpbuf *ofpacts)
 {
-    uint16_t command;
+    ovs_be16 raw_flags;
+    enum ofperr error;
     struct ofpbuf b;
     enum ofpraw raw;
 
@@ -1579,7 +1648,6 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
     if (raw == OFPRAW_OFPT11_FLOW_MOD) {
         /* Standard OpenFlow 1.1+ flow_mod. */
         const struct ofp11_flow_mod *ofm;
-        enum ofperr error;
 
         ofm = ofpbuf_pull(&b, sizeof *ofm);
 
@@ -1626,12 +1694,13 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
             && ofm->out_group != htonl(OFPG_ANY)) {
             return OFPERR_OFPFMFC_UNKNOWN;
         }
-        fm->flags = ntohs(ofm->flags);
+        raw_flags = ofm->flags;
     } else {
+        uint16_t command;
+
         if (raw == OFPRAW_OFPT10_FLOW_MOD) {
             /* Standard OpenFlow 1.0 flow_mod. */
             const struct ofp10_flow_mod *ofm;
-            enum ofperr error;
 
             /* Get the ofp10_flow_mod. */
             ofm = ofpbuf_pull(&b, sizeof *ofm);
@@ -1661,11 +1730,10 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
             fm->hard_timeout = ntohs(ofm->hard_timeout);
             fm->buffer_id = ntohl(ofm->buffer_id);
             fm->out_port = u16_to_ofp(ntohs(ofm->out_port));
-            fm->flags = ntohs(ofm->flags);
+            raw_flags = ofm->flags;
         } else if (raw == OFPRAW_NXT_FLOW_MOD) {
             /* Nicira extended flow_mod. */
             const struct nx_flow_mod *nfm;
-            enum ofperr error;
 
             /* Dissect the message. */
             nfm = ofpbuf_pull(&b, sizeof *nfm);
@@ -1692,23 +1760,11 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
             fm->hard_timeout = ntohs(nfm->hard_timeout);
             fm->buffer_id = ntohl(nfm->buffer_id);
             fm->out_port = u16_to_ofp(ntohs(nfm->out_port));
-            fm->flags = ntohs(nfm->flags);
+            raw_flags = nfm->flags;
         } else {
             NOT_REACHED();
         }
 
-        if (fm->flags & OFPFF10_EMERG) {
-            /* We do not support the OpenFlow 1.0 emergency flow cache, which
-             * is not required in OpenFlow 1.0.1 and removed from OpenFlow 1.1.
-             *
-             * OpenFlow 1.0 specifies the error code to use when idle_timeout
-             * or hard_timeout is nonzero.  Otherwise, there is no good error
-             * code, so just state that the flow table is full. */
-            return (fm->hard_timeout || fm->idle_timeout
-                    ? OFPERR_OFPFMFC_BAD_EMERG_TIMEOUT
-                    : OFPERR_OFPFMFC_TABLE_FULL);
-        }
-
         fm->modify_cookie = fm->new_cookie != htonll(UINT64_MAX);
         if (protocol & OFPUTIL_P_TID) {
             fm->command = command & 0xff;
@@ -1722,6 +1778,24 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
     fm->ofpacts = ofpacts->data;
     fm->ofpacts_len = ofpacts->size;
 
+    error = ofputil_decode_flow_mod_flags(raw_flags, fm->command,
+                                          oh->version, &fm->flags);
+    if (error) {
+        return error;
+    }
+
+    if (fm->flags & OFPUTIL_FF_EMERG) {
+        /* We do not support the OpenFlow 1.0 emergency flow cache, which
+         * is not required in OpenFlow 1.0.1 and removed from OpenFlow 1.1.
+         *
+         * OpenFlow 1.0 specifies the error code to use when idle_timeout
+         * or hard_timeout is nonzero.  Otherwise, there is no good error
+         * code, so just state that the flow table is full. */
+        return (fm->hard_timeout || fm->idle_timeout
+                ? OFPERR_OFPFMFC_BAD_EMERG_TIMEOUT
+                : OFPERR_OFPFMFC_TABLE_FULL);
+    }
+
     return 0;
 }
 
@@ -2105,6 +2179,8 @@ struct ofpbuf *
 ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
                         enum ofputil_protocol protocol)
 {
+    enum ofp_version version = ofputil_protocol_to_ofp_version(protocol);
+    ovs_be16 raw_flags = ofputil_encode_flow_mod_flags(fm->flags, version);
     struct ofpbuf *msg;
 
     switch (protocol) {
@@ -2115,9 +2191,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
         int tailroom;
 
         tailroom = ofputil_match_typical_len(protocol) + fm->ofpacts_len;
-        msg = ofpraw_alloc(OFPRAW_OFPT11_FLOW_MOD,
-                           ofputil_protocol_to_ofp_version(protocol),
-                           tailroom);
+        msg = ofpraw_alloc(OFPRAW_OFPT11_FLOW_MOD, version, tailroom);
         ofm = ofpbuf_put_zeros(msg, sizeof *ofm);
         if ((protocol == OFPUTIL_P_OF11_STD
              && (fm->command == OFPFC_MODIFY ||
@@ -2137,7 +2211,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
         ofm->buffer_id = htonl(fm->buffer_id);
         ofm->out_port = ofputil_port_to_ofp11(fm->out_port);
         ofm->out_group = htonl(OFPG11_ANY);
-        ofm->flags = htons(fm->flags);
+        ofm->flags = raw_flags;
         ofputil_put_ofp11_match(msg, &fm->match, protocol);
         ofpacts_put_openflow11_instructions(fm->ofpacts, fm->ofpacts_len, msg);
         break;
@@ -2158,7 +2232,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
         ofm->priority = htons(fm->priority);
         ofm->buffer_id = htonl(fm->buffer_id);
         ofm->out_port = htons(ofp_to_u16(fm->out_port));
-        ofm->flags = htons(fm->flags);
+        ofm->flags = raw_flags;
         ofpacts_put_openflow10(fm->ofpacts, fm->ofpacts_len, msg);
         break;
     }
@@ -2180,7 +2254,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
         nfm->priority = htons(fm->priority);
         nfm->buffer_id = htonl(fm->buffer_id);
         nfm->out_port = htons(ofp_to_u16(fm->out_port));
-        nfm->flags = htons(fm->flags);
+        nfm->flags = raw_flags;
         nfm->match_len = htons(match_len);
         ofpacts_put_openflow10(fm->ofpacts, fm->ofpacts_len, msg);
         break;
@@ -2446,6 +2520,7 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
                                 bool flow_age_extension,
                                 struct ofpbuf *ofpacts)
 {
+    const struct ofp_header *oh;
     enum ofperr error;
     enum ofpraw raw;
 
@@ -2455,6 +2530,7 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
     if (error) {
         return error;
     }
+    oh = msg->l2;
 
     if (!msg->size) {
         return EOF;
@@ -2495,7 +2571,15 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         fs->duration_nsec = ntohl(ofs->duration_nsec);
         fs->idle_timeout = ntohs(ofs->idle_timeout);
         fs->hard_timeout = ntohs(ofs->hard_timeout);
-        fs->flags = (raw == OFPRAW_OFPST13_FLOW_REPLY) ? ntohs(ofs->flags) : 0;
+        if (raw == OFPRAW_OFPST13_FLOW_REPLY) {
+            error = ofputil_decode_flow_mod_flags(ofs->flags, -1, oh->version,
+                                                  &fs->flags);
+            if (error) {
+                return error;
+            }
+        } else {
+            fs->flags = 0;
+        }
         fs->idle_age = -1;
         fs->hard_age = -1;
         fs->cookie = ofs->cookie;
@@ -2616,6 +2700,7 @@ ofputil_append_flow_stats_reply(const struct ofputil_flow_stats *fs,
 
     ofpraw_decode_partial(&raw, reply->data, reply->size);
     if (raw == OFPRAW_OFPST11_FLOW_REPLY || raw == OFPRAW_OFPST13_FLOW_REPLY) {
+        const struct ofp_header *oh = reply->data;
         struct ofp11_flow_stats *ofs;
 
         ofpbuf_put_uninit(reply, sizeof *ofs);
@@ -2632,7 +2717,11 @@ ofputil_append_flow_stats_reply(const struct ofputil_flow_stats *fs,
         ofs->priority = htons(fs->priority);
         ofs->idle_timeout = htons(fs->idle_timeout);
         ofs->hard_timeout = htons(fs->hard_timeout);
-        ofs->flags = (raw == OFPRAW_OFPST13_FLOW_REPLY) ? htons(fs->flags) : 0;
+        if (raw == OFPRAW_OFPST13_FLOW_REPLY) {
+            ofs->flags = ofputil_encode_flow_mod_flags(fs->flags, oh->version);
+        } else {
+            ofs->flags = 0;
+        }
         memset(ofs->pad2, 0, sizeof ofs->pad2);
         ofs->cookie = fs->cookie;
         ofs->packet_count = htonll(unknown_to_zero(fs->packet_count));
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index f3348c082..7fb47de17 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -207,6 +207,16 @@ struct ofpbuf *ofputil_make_set_packet_in_format(enum ofp_version,
 /* 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 {
+    OFPUTIL_FF_SEND_FLOW_REM = 1 << 0, /* All versions. */
+    OFPUTIL_FF_CHECK_OVERLAP = 1 << 1, /* All versions. */
+    OFPUTIL_FF_EMERG         = 1 << 2, /* OpenFlow 1.0 only. */
+    OFPUTIL_FF_RESET_COUNTS  = 1 << 3, /* OpenFlow 1.2+. */
+    OFPUTIL_FF_NO_PKT_COUNTS = 1 << 4, /* OpenFlow 1.3+. */
+    OFPUTIL_FF_NO_BYT_COUNTS = 1 << 5  /* OpenFlow 1.3+. */
+};
+
 /* Protocol-independent flow_mod.
  *
  * The handling of cookies across multiple versions of OpenFlow is a bit
@@ -248,7 +258,7 @@ struct ofputil_flow_mod {
     uint16_t hard_timeout;
     uint32_t buffer_id;
     ofp_port_t out_port;
-    uint16_t flags;
+    enum ofputil_flow_mod_flags flags;
     struct ofpact *ofpacts;     /* Series of "struct ofpact"s. */
     size_t ofpacts_len;         /* Length of ofpacts, in bytes. */
 };
@@ -296,7 +306,7 @@ struct ofputil_flow_stats {
     uint64_t byte_count;        /* Byte count, UINT64_MAX if unknown. */
     struct ofpact *ofpacts;
     size_t ofpacts_len;
-    uint16_t flags;             /* Added for OF 1.3 */
+    enum ofputil_flow_mod_flags flags;
 };
 
 int ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *,