From c2f0373a8248d0c20d2f876d7f3bc61a400d9924 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 2 Nov 2012 10:37:59 -0700 Subject: [PATCH] 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 Acked-by: Kyle Mestery --- ofproto/ofproto-provider.h | 23 +++++++++++++++++++++++ ofproto/ofproto.c | 32 ++++++++++++++++++++++++++++++-- tests/ofproto.at | 14 ++++++-------- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 0a6d3c04..26343cf0 100644 --- 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 bf11ab3b..83fd46ef 100644 --- 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 ada1ae8f..01205c72 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 @@ -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 -- 2.30.2