ofproto: Better hide hidden tables.

ofproto has a concept of "hidden" OpenFlow tables.  Currently these are
used internally only for ofproto-dpif for a couple of unimportant
purposes.  However, hidden tables were not hidden well enough, because
OFTest was able to spot ofproto-dpif's hidden table and, seeing that it
had a couple of flows in it even after OFTest had tried to delete all
flows, failed at least one test.

This commit hides the tables better:

    - The number of tables reported in a feature reply no longer counts
      hidden tables.

    - Table stats replies omit hidden tables.

This commit introduces the requirement that hidden tables, if any, be the
highest-numbered tables.  This is because it's not clear to me that
OpenFlow intends to allow tables to be numbered noncontiguously.

We could take this further, by not exposing hidden tables in any way, but
I have this pet theory that being able to get the statistics for these
tables will come in handy for debugging someday.

Found by openflow_protocol_messages.ModifyStateDelete in OFTest.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Kyle Mestery <kmestery@cisco.com>

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 0a6d3c04dcdde2e69dab2110ddc09d115098f80e..26343cf0e126c8232d6f31e6fcd802c75fe94e8f 100644 (file)
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -119,6 +119,29 @@ struct ofport {
 
 void ofproto_port_set_state(struct ofport *, enum ofputil_port_state);
 
+/* OpenFlow table flags:
+ *
+ *   - "Hidden" tables are not included in OpenFlow operations that operate on
+ *     "all tables".  For example, a request for flow stats on all tables will
+ *     omit flows in hidden tables, table stats requests will omit the table
+ *     entirely, and the switch features reply will not count the hidden table.
+ *
+ *     However, operations that specifically name the particular table still
+ *     operate on it.  For example, flow_mods and flow stats requests on a
+ *     hidden table work.
+ *
+ *     To avoid gaps in table IDs (which have unclear validity in OpenFlow),
+ *     hidden tables must be the highest-numbered tables that a provider
+ *     implements.
+ *
+ *   - "Read-only" tables can't be changed through OpenFlow operations.  (At
+ *     the moment all flow table operations go effectively through OpenFlow, so
+ *     this means that read-only tables can't be changed at all after the
+ *     read-only flag is set.)
+ *
+ * The generic ofproto layer never sets these flags.  An ofproto provider can
+ * set them if it is appropriate.
+ */
 enum oftable_flags {
     OFTABLE_HIDDEN = 1 << 0,   /* Hide from most OpenFlow operations. */
     OFTABLE_READONLY = 1 << 1  /* Don't allow OpenFlow to change this table. */

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index bf11ab3b8926b013befa4ab5606049484caaab95..83fd46efacff733fc36fc7ee03c9b5fdac04b4c8 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -374,6 +374,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
     const struct ofproto_class *class;
     struct ofproto *ofproto;
     int error;
+    int i;
 
     *ofprotop = NULL;
 
@@ -444,7 +445,14 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
     ofproto->ofp_port_ids = bitmap_allocate(ofproto->max_ports);
     bitmap_set1(ofproto->ofp_port_ids, 0);
 
+    /* Check that hidden tables, if any, are at the end. */
     assert(ofproto->n_tables);
+    for (i = 0; i + 1 < ofproto->n_tables; i++) {
+        enum oftable_flags flags = ofproto->tables[i].flags;
+        enum oftable_flags next_flags = ofproto->tables[i + 1].flags;
+
+        assert(!(flags & OFTABLE_HIDDEN) || next_flags & OFTABLE_HIDDEN);
+    }
 
     ofproto->datapath_id = pick_datapath_id(ofproto);
     init_ports(ofproto);
@@ -2179,14 +2187,26 @@ handle_features_request(struct ofconn *ofconn, const struct ofp_header *oh)
     struct ofport *port;
     bool arp_match_ip;
     struct ofpbuf *b;
+    int n_tables;
+    int i;
 
     ofproto->ofproto_class->get_features(ofproto, &arp_match_ip,
                                          &features.actions);
     assert(features.actions & OFPUTIL_A_OUTPUT); /* sanity check */
 
+    /* Count only non-hidden tables in the number of tables.  (Hidden tables,
+     * if present, are always at the end.) */
+    n_tables = ofproto->n_tables;
+    for (i = 0; i < ofproto->n_tables; i++) {
+        if (ofproto->tables[i].flags & OFTABLE_HIDDEN) {
+            n_tables = i;
+            break;
+        }
+    }
+
     features.datapath_id = ofproto->datapath_id;
     features.n_buffers = pktbuf_capacity();
-    features.n_tables = ofproto->n_tables;
+    features.n_tables = n_tables;
     features.capabilities = (OFPUTIL_C_FLOW_STATS | OFPUTIL_C_TABLE_STATS |
                              OFPUTIL_C_PORT_STATS | OFPUTIL_C_QUEUE_STATS);
     if (arp_match_ip) {
@@ -2411,6 +2431,7 @@ handle_table_stats_request(struct ofconn *ofconn,
     struct ofproto *p = ofconn_get_ofproto(ofconn);
     struct ofp12_table_stats *ots;
     struct ofpbuf *msg;
+    int n_tables;
     size_t i;
 
     /* Set up default values.
@@ -2439,9 +2460,16 @@ handle_table_stats_request(struct ofconn *ofconn,
 
     p->ofproto_class->get_tables(p, ots);
 
+    /* Post-process the tables, dropping hidden tables. */
+    n_tables = p->n_tables;
     for (i = 0; i < p->n_tables; i++) {
         const struct oftable *table = &p->tables[i];
 
+        if (table->flags & OFTABLE_HIDDEN) {
+            n_tables = i;
+            break;
+        }
+
         if (table->name) {
             ovs_strzcpy(ots[i].name, table->name, sizeof ots[i].name);
         }
@@ -2451,7 +2479,7 @@ handle_table_stats_request(struct ofconn *ofconn,
         }
     }
 
-    msg = ofputil_encode_table_stats_reply(ots, p->n_tables, request);
+    msg = ofputil_encode_table_stats_reply(ots, n_tables, request);
     ofconn_send_reply(ofconn, msg);
 
     free(ots);

diff --git a/tests/ofproto.at b/tests/ofproto.at
index ada1ae8f11998a90b7f6d4717b8d56be0d5d413a..01205c72521a63a9bff1c338b281c881cdde7ff2 100644 (file)
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -11,7 +11,7 @@ OVS_VSWITCHD_START
 AT_CHECK([ovs-ofctl -vwarn show br0], [0], [stdout])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl
 OFPT_FEATURES_REPLY: dpid:fedcba9876543210
-n_tables:255, n_buffers:256
+n_tables:254, n_buffers:256
 capabilities: FLOW_STATS TABLE_STATS PORT_STATS QUEUE_STATS ARP_MATCH_IP
 actions: OUTPUT SET_VLAN_VID SET_VLAN_PCP STRIP_VLAN SET_DL_SRC SET_DL_DST SET_NW_SRC SET_NW_DST SET_NW_TOS SET_TP_SRC SET_TP_DST ENQUEUE
  LOCAL(br0): addr:aa:55:aa:55:00:00
@@ -33,7 +33,7 @@ s/ (xid=0x[0-9a-fA-F]*)//
 s/00:0.$/00:0x/' < stdout]],
       [0], [dnl
 OFPT_FEATURES_REPLY: dpid:fedcba9876543210
-n_tables:255, n_buffers:256
+n_tables:254, n_buffers:256
 capabilities: FLOW_STATS TABLE_STATS PORT_STATS QUEUE_STATS ARP_MATCH_IP
 actions: OUTPUT SET_VLAN_VID SET_VLAN_PCP STRIP_VLAN SET_DL_SRC SET_DL_DST SET_NW_SRC SET_NW_DST SET_NW_TOS SET_TP_SRC SET_TP_DST ENQUEUE
  1(p1): addr:aa:55:aa:55:00:0x
@@ -122,7 +122,7 @@ do
     AT_CHECK([ovs-ofctl -vwarn show br0], [0], [stdout])
     AT_CHECK_UNQUOTED([STRIP_XIDS stdout], [0], [dnl
 OFPT_FEATURES_REPLY: dpid:fedcba9876543210
-n_tables:255, n_buffers:256
+n_tables:254, n_buffers:256
 capabilities: FLOW_STATS TABLE_STATS PORT_STATS QUEUE_STATS ARP_MATCH_IP
 actions: OUTPUT SET_VLAN_VID SET_VLAN_PCP STRIP_VLAN SET_DL_SRC SET_DL_DST SET_NW_SRC SET_NW_DST SET_NW_TOS SET_TP_SRC SET_TP_DST ENQUEUE
  LOCAL(br0): addr:aa:55:aa:55:00:00
@@ -428,7 +428,7 @@ AT_CLEANUP
 AT_SETUP([ofproto - flow table configuration])
 OVS_VSWITCHD_START
 # Check the default configuration.
-(echo "OFPST_TABLE reply (xid=0x2): 255 tables
+(echo "OFPST_TABLE reply (xid=0x2): 254 tables
   0: classifier: wild=0x3fffff, max=1000000, active=0
                lookup=0, matched=0"
  x=1
@@ -437,9 +437,7 @@ OVS_VSWITCHD_START
                lookup=0, matched=0
 " $x table$x
    x=`expr $x + 1`
- done
- echo "  254: table254: wild=0x3fffff, max=1000000, active=2
-               lookup=0, matched=0") > expout
+ done) > expout
 AT_CHECK([ovs-ofctl dump-tables br0], [0], [expout])
 # Change the configuration.
 AT_CHECK(
@@ -453,7 +451,7 @@ AT_CHECK(
 ])
 # Check that the configuration was updated.
 mv expout orig-expout
-(echo "OFPST_TABLE reply (xid=0x2): 255 tables
+(echo "OFPST_TABLE reply (xid=0x2): 254 tables
   0: main    : wild=0x3fffff, max=1000000, active=0
                lookup=0, matched=0
   1: table1  : wild=0x3fffff, max=  1024, active=0