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.
Found by openflow_protocol_messages.ModifyStateDelete in OFTest.
Signed-off-by: Ben Pfaff <b...@nicira.com>
---
ofproto/ofproto-provider.h | 23 +++++++++++++++++++++++
ofproto/ofproto.c | 32 ++++++++++++++++++++++++++++++--
tests/ofproto.at | 12 +++++-------
3 files changed, 58 insertions(+), 9 deletions(-)
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index a62473b..5de36ae 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -116,6 +116,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 444717e..d14d1d3 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -349,6 +349,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;
@@ -414,7 +415,14 @@ ofproto_create(const char *datapath_name, const char
*datapath_type,
return error;
}
+ /* 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);
@@ -2012,14 +2020,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) {
@@ -2244,6 +2264,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.
@@ -2272,9 +2293,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);
}
@@ -2284,7 +2312,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 cbf07bc..5debeba 100644
--- 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
@@ -91,7 +91,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
@@ -397,7 +397,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
@@ -406,9 +406,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(
@@ -422,7 +420,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
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
