From patchwork Thu Aug 30 20:00:49 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 964163 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 421YK357xnz9rvt for ; Fri, 31 Aug 2018 06:01:11 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 68252CB5; Thu, 30 Aug 2018 20:01:07 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 7B6ABBE6 for ; Thu, 30 Aug 2018 20:01:06 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id CD698466 for ; Thu, 30 Aug 2018 20:01:05 +0000 (UTC) X-Originating-IP: 173.228.112.177 Received: from sigabrt.gateway.sonic.net (173-228-112-177.dsl.dynamic.fusionbroadband.com [173.228.112.177]) (Authenticated sender: blp@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id B7ABC20005; Thu, 30 Aug 2018 20:01:02 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 30 Aug 2018 13:00:49 -0700 Message-Id: <20180830200056.15484-1-blp@ovn.org> X-Mailer: git-send-email 2.16.1 X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 1/8] vconn: Avoid null dereference on error path. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Sometimes the 'errors' list is passed as null, and in that case it should not be used. Found by inspection. Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- lib/vconn.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/vconn.c b/lib/vconn.c index e95ecbfa78a3..4d5f308d8836 100644 --- a/lib/vconn.c +++ b/lib/vconn.c @@ -772,7 +772,7 @@ vconn_recv_xid__(struct vconn *vconn, ovs_be32 xid, struct ofpbuf **replyp, } error = ofptype_decode(&type, oh); - if (!error && type == OFPTYPE_ERROR) { + if (!error && type == OFPTYPE_ERROR && errors) { ovs_list_push_back(errors, &reply->list_node); } else { VLOG_DBG_RL(&bad_ofmsg_rl, "%s: received reply with xid %08"PRIx32 From patchwork Thu Aug 30 20:00:50 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 964165 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 421YKc3XvLz9rvt for ; Fri, 31 Aug 2018 06:01:40 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 24CAECE0; Thu, 30 Aug 2018 20:01:10 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 1C7DCCD7 for ; Thu, 30 Aug 2018 20:01:08 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id C0E2C466 for ; Thu, 30 Aug 2018 20:01:06 +0000 (UTC) X-Originating-IP: 173.228.112.177 Received: from sigabrt.gateway.sonic.net (173-228-112-177.dsl.dynamic.fusionbroadband.com [173.228.112.177]) (Authenticated sender: blp@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 3DEC520008; Thu, 30 Aug 2018 20:01:03 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 30 Aug 2018 13:00:50 -0700 Message-Id: <20180830200056.15484-2-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20180830200056.15484-1-blp@ovn.org> References: <20180830200056.15484-1-blp@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 2/8] ofp-table: Better summarize table features and statistics. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Before this patch, most dump-table-stats outputs would contain about 250 lines of the form: table #: ditto With this patch, they have one line like this: tables 2...254: ditto which is much easier to read. Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- include/openvswitch/ofp-table.h | 5 +++- lib/ofp-print.c | 11 +++++--- lib/ofp-table.c | 42 +++++++++++++++++++++++++------ tests/ofproto-dpif.at | 56 +++++++++++++++++++---------------------- tests/ofproto.at | 25 ++++-------------- utilities/ovs-ofctl.c | 13 +++++++--- 6 files changed, 86 insertions(+), 66 deletions(-) diff --git a/include/openvswitch/ofp-table.h b/include/openvswitch/ofp-table.h index d06ccf5ce9e3..7b1152a2871c 100644 --- a/include/openvswitch/ofp-table.h +++ b/include/openvswitch/ofp-table.h @@ -276,7 +276,10 @@ void ofputil_table_features_format( const struct ofputil_table_features *prev_features, const struct ofputil_table_stats *stats, const struct ofputil_table_stats *prev_stats, - const struct ofputil_table_map *table_map); + const struct ofputil_table_map *table_map, + int *first_ditto, int *last_ditto); +void ofputil_table_features_format_finish(struct ds *, + int first_ditto, int last_ditto); /* Abstract table stats. * diff --git a/lib/ofp-print.c b/lib/ofp-print.c index cf93d2e2cb38..6fc2223c3e91 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -232,18 +232,19 @@ ofp_print_table_features_reply(struct ds *s, const struct ofp_header *oh, struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length)); struct ofputil_table_features prev; + int first_ditto = -1, last_ditto = -1; for (int i = 0; ; i++) { struct ofputil_table_features tf; int retval; retval = ofputil_decode_table_features(&b, &tf, true); if (retval) { + ofputil_table_features_format_finish(s, first_ditto, last_ditto); return retval != EOF ? retval : 0; } - ds_put_char(s, '\n'); ofputil_table_features_format(s, &tf, i ? &prev : NULL, NULL, NULL, - table_map); + table_map, &first_ditto, &last_ditto); prev = tf; } } @@ -573,6 +574,7 @@ ofp_print_table_stats_reply(struct ds *string, const struct ofp_header *oh, struct ofputil_table_features prev_features; struct ofputil_table_stats prev_stats; + int first_ditto = -1, last_ditto = -1; for (int i = 0;; i++) { struct ofputil_table_features features; struct ofputil_table_stats stats; @@ -580,14 +582,15 @@ ofp_print_table_stats_reply(struct ds *string, const struct ofp_header *oh, retval = ofputil_decode_table_stats_reply(&b, &stats, &features); if (retval) { + ofputil_table_features_format_finish(string, + first_ditto, last_ditto); return retval != EOF ? retval : 0; } - ds_put_char(string, '\n'); ofputil_table_features_format(string, &features, i ? &prev_features : NULL, &stats, i ? &prev_stats : NULL, - table_map); + table_map, &first_ditto, &last_ditto); prev_features = features; prev_stats = stats; } diff --git a/lib/ofp-table.c b/lib/ofp-table.c index 5f14fcc3a9f3..71197f644250 100644 --- a/lib/ofp-table.c +++ b/lib/ofp-table.c @@ -1395,21 +1395,31 @@ ofputil_table_features_format( const struct ofputil_table_features *prev_features, const struct ofputil_table_stats *stats, const struct ofputil_table_stats *prev_stats, - const struct ofputil_table_map *table_map) + const struct ofputil_table_map *table_map, + int *first_ditto, int *last_ditto) { - int i; + bool same_stats = !stats || (prev_stats + && table_stats_equal(stats, prev_stats)); + bool same_features = prev_features && table_features_equal(features, + prev_features); + if (same_stats && same_features && !features->name[0]) { + if (*first_ditto < 0) { + *first_ditto = features->table_id; + } + *last_ditto = features->table_id; + return; + } + ofputil_table_features_format_finish(s, *first_ditto, *last_ditto); + *first_ditto = -1; - ds_put_format(s, " table "); + ds_put_format(s, "\n table "); ofputil_format_table(features->table_id, table_map, s); if (features->name[0]) { ds_put_format(s, " (\"%s\")", features->name); } ds_put_char(s, ':'); - bool same_stats = prev_stats && table_stats_equal(stats, prev_stats); - bool same_features = prev_features && table_features_equal(features, - prev_features); - if ((!stats || same_stats) && same_features) { + if (same_stats && same_features) { ds_put_cstr(s, " ditto"); return; } @@ -1479,6 +1489,8 @@ ofputil_table_features_format( ds_put_cstr(s, " (same matching)\n"); } else { ds_put_cstr(s, " matching:\n"); + + int i; BITMAP_FOR_EACH_1 (i, MFF_N_IDS, features->match.bm) { const struct mf_field *f = mf_from_id(i); bool mask = bitmap_is_set(features->mask.bm, i); @@ -1493,6 +1505,22 @@ ofputil_table_features_format( } } } + +void +ofputil_table_features_format_finish(struct ds *s, + int first_ditto, int last_ditto) +{ + if (first_ditto < 0) { + return; + } + + ds_put_char(s, '\n'); + if (first_ditto == last_ditto) { + ds_put_format(s, " table %d: ditto\n", first_ditto); + } else { + ds_put_format(s, " tables %d...%d: ditto\n", first_ditto, last_ditto); + } +} /* Table stats. */ diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 362c58db437b..db9784276535 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -8913,17 +8913,16 @@ AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl NXST_FLOW reply: ]) -(echo "OFPST_TABLE reply (OF1.3) (xid=0x2): +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0], [0], + [OFPST_TABLE reply (OF1.3) (xid=0x2): table 0: active=1, lookup=0, matched=0 table 1: active=0, lookup=0, matched=0 -" - for i in `seq 2 253`; do - printf ' table %d: ditto\n' $i - done) > expout -AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0 ], [0], [expout]) + + tables 2...253: ditto +]) OVS_VSWITCHD_STOP AT_CLEANUP @@ -8959,26 +8958,24 @@ NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via no_match) data_l vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234 ]) -(echo "OFPST_TABLE reply (OF1.3) (xid=0x2): +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0], [0], [dnl +OFPST_TABLE reply (OF1.3) (xid=0x2): table 0: active=0, lookup=0, matched=0 -" - for i in `seq 1 253`; do - printf ' table %d: ditto\n' $i - done) > expout -AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0 ], [0], [expout]) -(echo "OFPST_TABLE reply (OF1.3) (xid=0x2): + tables 1...253: ditto +]) + +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br1], [0], [dnl +OFPST_TABLE reply (OF1.3) (xid=0x2): table 0: active=0, lookup=3, matched=0 table 1: active=0, lookup=0, matched=0 -" - for i in `seq 2 253`; do - printf ' table %d: ditto\n' $i - done) > expout -AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br1 ], [0], [expout]) + + tables 2...253: ditto +]) OVS_VSWITCHD_STOP AT_CLEANUP @@ -9070,18 +9067,18 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip | sort], [0], [dn OFPST_FLOW reply (OF1.3): ]) -(echo "OFPST_TABLE reply (OF1.3) (xid=0x2): +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0], [0], [dnl +OFPST_TABLE reply (OF1.3) (xid=0x2): table 0: active=1, lookup=3, matched=3 table 1: ditto + table 2: active=0, lookup=0, matched=0 -" - for i in `seq 3 253`; do - printf ' table %d: ditto\n' $i - done) > expout -AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0 ], [0], [expout]) + + tables 3...253: ditto +]) OVS_VSWITCHD_STOP AT_CLEANUP @@ -9120,7 +9117,8 @@ AT_CHECK([ovs-ofctl -O OpenFlow11 dump-flows br0 | ofctl_strip | sort], [0], [dn OFPST_FLOW reply (OF1.1): ]) -(echo "OFPST_TABLE reply (OF1.3) (xid=0x2): +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0], [0], [dnl +OFPST_TABLE reply (OF1.3) (xid=0x2): table 0: active=0, lookup=3, matched=0 @@ -9129,11 +9127,9 @@ OFPST_FLOW reply (OF1.1): table 2: active=0, lookup=0, matched=0 -" - for i in `seq 3 253`; do - printf ' table %d: ditto\n' $i - done) > expout -AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0 ], [0], [expout]) + + tables 3...253: ditto +]) OVS_VSWITCHD_STOP AT_CLEANUP diff --git a/tests/ofproto.at b/tests/ofproto.at index 9819bc577bdd..83da32383b68 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -2253,12 +2253,7 @@ head_table() { ' "$1" } -ditto() { - for i in `seq $1 $2`; do - printf ' table %d: ditto\n' $i - done -} -(head_table; ditto 1 253) > expout +(head_table; echo ' tables 1...253: ditto') > expout AT_CHECK([ovs-ofctl dump-tables br0], [0], [expout]) # Change the configuration. AT_CHECK( @@ -2280,7 +2275,7 @@ AT_CHECK( active=0, lookup=0, matched=0 max_entries=1000000 (same matching) -'; ditto 3 253) > expout +'; echo ' tables 3...253: ditto') > expout AT_CHECK([ovs-ofctl dump-tables br0], [0], [expout]) OVS_VSWITCHD_STOP AT_CLEANUP @@ -2329,12 +2324,7 @@ head_table() { ' } -ditto() { - for i in `seq $1 $2`; do - printf ' table %d: ditto\n' $i - done -} -(head_table; ditto 1 253) > expout +(head_table; echo ' tables 1...253: ditto') > expout AT_CHECK([ovs-ofctl dump-tables br0 | strip_xids], [0], [expout]) OVS_VSWITCHD_STOP(["/240\.0\.0\.1/d"]) AT_CLEANUP @@ -2393,11 +2383,6 @@ head_table() { ' "$1" } -ditto() { - for i in `seq $1 $2`; do - printf ' table %d: ditto\n' $i - done -} tail_table() { printf ' table 253: active=0, lookup=0, matched=0 @@ -2410,7 +2395,7 @@ tail_table() { (same matching) ' } -(head_table; ditto 1 252; tail_table) > expout +(head_table; printf ' tables 1...252: ditto\n\n'; tail_table) > expout AT_CHECK([ovs-ofctl -O OpenFlow12 dump-tables br0], [0], [expout]) # Change the configuration. AT_CHECK( @@ -2438,7 +2423,7 @@ AT_CHECK( max_entries=1000000 (same instructions) (same matching) -'; ditto 3 252; tail_table) > expout +'; printf ' tables 3...252: ditto\n\n'; tail_table) > expout AT_CHECK([ovs-ofctl -O OpenFlow12 dump-tables br0], [0], [expout]) OVS_VSWITCHD_STOP AT_CLEANUP diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index c018bd48fcfb..a29ea3a668b8 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -888,6 +888,8 @@ ofctl_dump_table_features(struct ovs_cmdl_context *ctx) bool done = false; struct ofputil_table_features prev; + int first_ditto = -1, last_ditto = -1; + struct ds s = DS_EMPTY_INITIALIZER; int n = 0; send_openflow_buffer(vconn, request); @@ -922,12 +924,10 @@ ofctl_dump_table_features(struct ovs_cmdl_context *ctx) break; } - struct ds s = DS_EMPTY_INITIALIZER; ofputil_table_features_format( &s, &tf, n ? &prev : NULL, NULL, NULL, - tables_to_show(ctx->argv[1])); - puts(ds_cstr(&s)); - ds_destroy(&s); + tables_to_show(ctx->argv[1]), + &first_ditto, &last_ditto); prev = tf; n++; @@ -946,6 +946,11 @@ ofctl_dump_table_features(struct ovs_cmdl_context *ctx) ofpbuf_delete(reply); } + ofputil_table_features_format_finish(&s, first_ditto, last_ditto); + const char *p = ds_cstr(&s); + puts(p + (*p == '\n')); + ds_destroy(&s); + vconn_close(vconn); } From patchwork Thu Aug 30 20:00:51 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 964166 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 421YLG16pxz9rvt for ; Fri, 31 Aug 2018 06:02:14 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id DC7ABCF1; Thu, 30 Aug 2018 20:01:12 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 48EAFCC6 for ; Thu, 30 Aug 2018 20:01:09 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 3F6E5148 for ; Thu, 30 Aug 2018 20:01:08 +0000 (UTC) X-Originating-IP: 173.228.112.177 Received: from sigabrt.gateway.sonic.net (173-228-112-177.dsl.dynamic.fusionbroadband.com [173.228.112.177]) (Authenticated sender: blp@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id D440020002; Thu, 30 Aug 2018 20:01:05 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 30 Aug 2018 13:00:51 -0700 Message-Id: <20180830200056.15484-3-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20180830200056.15484-1-blp@ovn.org> References: <20180830200056.15484-1-blp@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 3/8] ofp-table: Ignore bits that have to change according to OpenFlow. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org OpenFlow table feature replies contain a per-table bitmap that indicates which tables a flow can point to in goto_table actions. OpenFlow requires that a table only be able to go to higher-numbered tables. This means that a switch that is general as possible will always have different features for every table, since each one will have a different bitmap. This makes the output of "ovs-ofctl dump-table-features" pretty long and ugly because it has about 250 entries like this: table %d: metadata: match=0xffffffffffffffff write=0xffffffffffffffff max_entries=%d instructions (table miss and others): next tables: %d-253 (same instructions) (same actions) (same matching) This commit changes the logic that prints table features messages so that it considers two sequentially numbered tables to be the same if only the bit that necessarily must be tunred off changes. This reduces the hundreds of entries above to just: tables 1...253: ditto which is so much more readable. Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- lib/ofp-table.c | 66 +++++++++++++++++++++++++++++++++++++++++++++----------- tests/ofproto.at | 45 ++++++++++++++------------------------ 2 files changed, 70 insertions(+), 41 deletions(-) diff --git a/lib/ofp-table.c b/lib/ofp-table.c index 71197f644250..e5c8247717d7 100644 --- a/lib/ofp-table.c +++ b/lib/ofp-table.c @@ -1329,12 +1329,47 @@ print_table_instruction_features( } } +/* Compares bitmaps of next tables 'a' and 'b', for tables 'a_table_id' and + * 'b_table_id', respectively. Returns true if the bitmaps are equal. + * + * The bitmaps are considered equal if b_table_id == a_table_id + 1 and the bit + * for 'b_table_id' is set in 'a' but not in 'b'. This is because OpenFlow + * requires that a table not be able to do a goto_table back to its own table + * or an earlier one. Without considering these equivalent, every table will + * be different from every one in some way, which just isn't useful in printing + * table features. */ +static bool +table_instruction_features_next_equal(const unsigned long *a, int a_table_id, + const unsigned long *b, int b_table_id) +{ + if (b_table_id == a_table_id + 1 + && bitmap_is_set(a, b_table_id) + && !bitmap_is_set(b, b_table_id)) { + for (size_t i = 0; i < BITMAP_N_LONGS(255); i++) { + unsigned long diff = a[i] ^ b[i]; + if (i == b_table_id / BITMAP_ULONG_BITS) { + diff &= ~bitmap_bit__(b_table_id); + } + if (diff) { + return false; + } + } + return true; + } else if (a_table_id == b_table_id + 1) { + return table_instruction_features_next_equal(b, b_table_id, + a, a_table_id); + } else { + return bitmap_equal(a, b, 255); + } +} + static bool table_instruction_features_equal( - const struct ofputil_table_instruction_features *a, - const struct ofputil_table_instruction_features *b) + const struct ofputil_table_instruction_features *a, int a_table_id, + const struct ofputil_table_instruction_features *b, int b_table_id) { - return (bitmap_equal(a->next, b->next, 255) + return (table_instruction_features_next_equal(a->next, a_table_id, + b->next, b_table_id) && a->instructions == b->instructions && table_action_features_equal(&a->write, &b->write) && table_action_features_equal(&a->apply, &b->apply)); @@ -1360,8 +1395,10 @@ table_features_equal(const struct ofputil_table_features *a, && a->supports_eviction == b->supports_eviction && a->supports_vacancy_events == b->supports_vacancy_events && a->max_entries == b->max_entries - && table_instruction_features_equal(&a->nonmiss, &b->nonmiss) - && table_instruction_features_equal(&a->miss, &b->miss) + && table_instruction_features_equal(&a->nonmiss, a->table_id, + &b->nonmiss, b->table_id) + && table_instruction_features_equal(&a->miss, a->table_id, + &b->miss, b->table_id) && bitmap_equal(a->match.bm, b->match.bm, MFF_N_IDS)); } @@ -1398,22 +1435,25 @@ ofputil_table_features_format( const struct ofputil_table_map *table_map, int *first_ditto, int *last_ditto) { + int table = features->table_id; + int prev_table = prev_features ? prev_features->table_id : 0; + bool same_stats = !stats || (prev_stats && table_stats_equal(stats, prev_stats)); bool same_features = prev_features && table_features_equal(features, prev_features); if (same_stats && same_features && !features->name[0]) { if (*first_ditto < 0) { - *first_ditto = features->table_id; + *first_ditto = table; } - *last_ditto = features->table_id; + *last_ditto = table; return; } ofputil_table_features_format_finish(s, *first_ditto, *last_ditto); *first_ditto = -1; ds_put_format(s, "\n table "); - ofputil_format_table(features->table_id, table_map, s); + ofputil_format_table(table, table_map, s); if (features->name[0]) { ds_put_format(s, " (\"%s\")", features->name); } @@ -1466,13 +1506,15 @@ ofputil_table_features_format( const struct ofputil_table_instruction_features *prev_miss = prev_features ? &prev_features->miss : NULL; if (prev_features - && table_instruction_features_equal(&features->nonmiss, prev_nonmiss) - && table_instruction_features_equal(&features->miss, prev_miss)) { + && table_instruction_features_equal(&features->nonmiss, table, + prev_nonmiss, prev_table) + && table_instruction_features_equal(&features->miss, table, + prev_miss, prev_table)) { if (!table_instruction_features_empty(&features->nonmiss)) { ds_put_cstr(s, " (same instructions)\n"); } - } else if (!table_instruction_features_equal(&features->nonmiss, - &features->miss)) { + } else if (!table_instruction_features_equal(&features->nonmiss, table, + &features->miss, table)) { ds_put_cstr(s, " instructions (other than table miss):\n"); print_table_instruction_features(s, &features->nonmiss, prev_nonmiss); ds_put_cstr(s, " instructions (table miss):\n"); diff --git a/tests/ofproto.at b/tests/ofproto.at index 83da32383b68..2291bfb93225 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -2620,29 +2620,8 @@ metadata in_port in_port_oxm pkt_mark ct_mark ct_label reg0 reg1 reg2 reg3 reg4 ' "$1" } -ditto() { - printf ' table %d: - metadata: match=0xffffffffffffffff write=0xffffffffffffffff - max_entries=%d - instructions (table miss and others): - next tables: %d-253 - (same instructions) - (same actions) - (same matching) - -' $1 $2 `expr $1 + 1` -} tail_tables() { -echo ' table 252: - metadata: match=0xffffffffffffffff write=0xffffffffffffffff - max_entries=1000000 - instructions (table miss and others): - next tables: 253 - (same instructions) - (same actions) - (same matching) - - table 253: +echo ' table 253: metadata: match=0xffffffffffffffff write=0xffffffffffffffff max_entries=1000000 instructions (table miss and others): @@ -2652,9 +2631,7 @@ echo ' table 252: ' } (head_table - for i in `seq 1 251`; do - ditto $i 1000000 - done + printf ' tables 1...252: ditto\n\n' tail_tables) > expout AT_CHECK([ovs-ofctl -O OpenFlow13 dump-table-features br0], [0], [expout]) # Change the configuration. @@ -2669,10 +2646,20 @@ AT_CHECK( ]) # Check that the configuration was updated. (head_table ' ("main")' - ditto 1 1024 - for i in `seq 2 251`; do - ditto $i 1000000 - done + echo ' table 1: + metadata: match=0xffffffffffffffff write=0xffffffffffffffff + max_entries=1024 + (same instructions) + (same matching) + + table 2: + metadata: match=0xffffffffffffffff write=0xffffffffffffffff + max_entries=1000000 + (same instructions) + (same matching) + + tables 3...252: ditto +' tail_tables) > expout AT_CHECK([ovs-ofctl -O OpenFlow13 dump-table-features br0], [0], [expout]) OVS_VSWITCHD_STOP From patchwork Thu Aug 30 20:00:52 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 964167 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 421YLs667dz9rvt for ; Fri, 31 Aug 2018 06:02:45 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 9B9E8CFB; Thu, 30 Aug 2018 20:01:13 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 72527CE7 for ; Thu, 30 Aug 2018 20:01:10 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 99529148 for ; Thu, 30 Aug 2018 20:01:09 +0000 (UTC) X-Originating-IP: 173.228.112.177 Received: from sigabrt.gateway.sonic.net (173-228-112-177.dsl.dynamic.fusionbroadband.com [173.228.112.177]) (Authenticated sender: blp@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 5F1E92000A; Thu, 30 Aug 2018 20:01:07 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 30 Aug 2018 13:00:52 -0700 Message-Id: <20180830200056.15484-4-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20180830200056.15484-1-blp@ovn.org> References: <20180830200056.15484-1-blp@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 4/8] ofp-table: Always format the table number in table features. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Table features should indicate the table number as well as the table name. Before this, the first line for each table looked like this: table myname ("myname"): but it's more useful if it's: table 123 ("myname"): Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- include/openvswitch/ofp-table.h | 1 - lib/ofp-print.c | 14 ++++++-------- lib/ofp-table.c | 4 +--- utilities/ovs-ofctl.c | 1 - 4 files changed, 7 insertions(+), 13 deletions(-) diff --git a/include/openvswitch/ofp-table.h b/include/openvswitch/ofp-table.h index 7b1152a2871c..713ce26d014d 100644 --- a/include/openvswitch/ofp-table.h +++ b/include/openvswitch/ofp-table.h @@ -276,7 +276,6 @@ void ofputil_table_features_format( const struct ofputil_table_features *prev_features, const struct ofputil_table_stats *stats, const struct ofputil_table_stats *prev_stats, - const struct ofputil_table_map *table_map, int *first_ditto, int *last_ditto); void ofputil_table_features_format_finish(struct ds *, int first_ditto, int last_ditto); diff --git a/lib/ofp-print.c b/lib/ofp-print.c index 6fc2223c3e91..9d4141e3b747 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -226,8 +226,7 @@ ofp_print_get_config_reply(struct ds *string, const struct ofp_header *oh) } static enum ofperr -ofp_print_table_features_reply(struct ds *s, const struct ofp_header *oh, - const struct ofputil_table_map *table_map) +ofp_print_table_features_reply(struct ds *s, const struct ofp_header *oh) { struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length)); @@ -244,7 +243,7 @@ ofp_print_table_features_reply(struct ds *s, const struct ofp_header *oh, } ofputil_table_features_format(s, &tf, i ? &prev : NULL, NULL, NULL, - table_map, &first_ditto, &last_ditto); + &first_ditto, &last_ditto); prev = tf; } } @@ -566,8 +565,7 @@ ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh, } static enum ofperr -ofp_print_table_stats_reply(struct ds *string, const struct ofp_header *oh, - const struct ofputil_table_map *table_map) +ofp_print_table_stats_reply(struct ds *string, const struct ofp_header *oh) { struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length)); ofpraw_pull_assert(&b); @@ -590,7 +588,7 @@ ofp_print_table_stats_reply(struct ds *string, const struct ofp_header *oh, ofputil_table_features_format(string, &features, i ? &prev_features : NULL, &stats, i ? &prev_stats : NULL, - table_map, &first_ditto, &last_ditto); + &first_ditto, &last_ditto); prev_features = features; prev_stats = stats; } @@ -990,7 +988,7 @@ ofp_to_string__(const struct ofp_header *oh, case OFPTYPE_TABLE_FEATURES_STATS_REQUEST: case OFPTYPE_TABLE_FEATURES_STATS_REPLY: - return ofp_print_table_features_reply(string, oh, table_map); + return ofp_print_table_features_reply(string, oh); case OFPTYPE_TABLE_DESC_REQUEST: case OFPTYPE_TABLE_DESC_REPLY: @@ -1113,7 +1111,7 @@ ofp_to_string__(const struct ofp_header *oh, return ofp_print_ofpst_port_reply(string, oh, port_map, verbosity); case OFPTYPE_TABLE_STATS_REPLY: - return ofp_print_table_stats_reply(string, oh, table_map); + return ofp_print_table_stats_reply(string, oh); case OFPTYPE_AGGREGATE_STATS_REPLY: return ofp_print_aggregate_stats_reply(string, oh); diff --git a/lib/ofp-table.c b/lib/ofp-table.c index e5c8247717d7..88fc322408ee 100644 --- a/lib/ofp-table.c +++ b/lib/ofp-table.c @@ -1432,7 +1432,6 @@ ofputil_table_features_format( const struct ofputil_table_features *prev_features, const struct ofputil_table_stats *stats, const struct ofputil_table_stats *prev_stats, - const struct ofputil_table_map *table_map, int *first_ditto, int *last_ditto) { int table = features->table_id; @@ -1452,8 +1451,7 @@ ofputil_table_features_format( ofputil_table_features_format_finish(s, *first_ditto, *last_ditto); *first_ditto = -1; - ds_put_format(s, "\n table "); - ofputil_format_table(table, table_map, s); + ds_put_format(s, "\n table %"PRIu8, table); if (features->name[0]) { ds_put_format(s, " (\"%s\")", features->name); } diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index a29ea3a668b8..aa9a1291e60a 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -926,7 +926,6 @@ ofctl_dump_table_features(struct ovs_cmdl_context *ctx) ofputil_table_features_format( &s, &tf, n ? &prev : NULL, NULL, NULL, - tables_to_show(ctx->argv[1]), &first_ditto, &last_ditto); prev = tf; From patchwork Thu Aug 30 20:00:53 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 964168 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 421YMP3vvhz9rvt for ; Fri, 31 Aug 2018 06:03:13 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 5ACCECFD; Thu, 30 Aug 2018 20:01:14 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 06B86CDB for ; Thu, 30 Aug 2018 20:01:12 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id F310E466 for ; Thu, 30 Aug 2018 20:01:10 +0000 (UTC) X-Originating-IP: 173.228.112.177 Received: from sigabrt.gateway.sonic.net (173-228-112-177.dsl.dynamic.fusionbroadband.com [173.228.112.177]) (Authenticated sender: blp@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id B7BF220006; Thu, 30 Aug 2018 20:01:08 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 30 Aug 2018 13:00:53 -0700 Message-Id: <20180830200056.15484-5-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20180830200056.15484-1-blp@ovn.org> References: <20180830200056.15484-1-blp@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 5/8] ofp-table: Parse table features messages more carefully. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- include/openflow/openflow-1.3.h | 24 +++++++++++- include/openvswitch/ofp-table.h | 14 ++++++- lib/ofp-table.c | 83 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 3 deletions(-) diff --git a/include/openflow/openflow-1.3.h b/include/openflow/openflow-1.3.h index a521995da339..c48a8ea7f3e9 100644 --- a/include/openflow/openflow-1.3.h +++ b/include/openflow/openflow-1.3.h @@ -215,13 +215,24 @@ struct ofp13_table_stats { }; OFP_ASSERT(sizeof(struct ofp13_table_stats) == 24); +enum ofp15_table_features_command { + OFPTFC15_REPLACE = 0, /* Replace full pipeline. */ + OFPTFC15_MODIFY = 1, /* Modify flow tables capabilities. */ + OFPTFC15_ENABLE = 2, /* Enable flow tables in the pipeline. */ + OFPTFC15_DISABLE = 3, /* Disable flow tables in pipeline. */ +}; + /* Body for ofp_multipart_request of type OFPMP_TABLE_FEATURES./ * Body of reply to OFPMP_TABLE_FEATURES request. */ struct ofp13_table_features { ovs_be16 length; /* Length is padded to 64 bits. */ uint8_t table_id; /* Identifier of table. Lower numbered tables are consulted first. */ - uint8_t pad[5]; /* Align to 64-bits. */ + + /* Added in OF1.5. Earlier versions acted like OFPTFC15_REPLACE. */ + uint8_t command; /* One of OFPTFC15_*. */ + + uint8_t pad[4]; /* Align to 64-bits. */ char name[OFP_MAX_TABLE_NAME_LEN]; ovs_be64 metadata_match; /* Bits of metadata table can match. */ ovs_be64 metadata_write; /* Bits of metadata table can write. */ @@ -260,6 +271,17 @@ enum ofp13_table_feature_prop_type { OFPTFPT13_APPLY_SETFIELD_MISS = 15, /* Apply Set-Field for table-miss. */ OFPTFPT13_EXPERIMENTER = 0xFFFE, /* Experimenter property. */ OFPTFPT13_EXPERIMENTER_MISS = 0xFFFF, /* Experimenter for table-miss. */ + + /* OpenFlow says that each of these properties must occur exactly once. */ +#define OFPTFPT13_REQUIRED ((1u << OFPTFPT13_INSTRUCTIONS) | \ + (1u << OFPTFPT13_NEXT_TABLES) | \ + (1u << OFPTFPT13_WRITE_ACTIONS) | \ + (1u << OFPTFPT13_APPLY_ACTIONS) | \ + (1u << OFPTFPT13_MATCH) | \ + (1u << OFPTFPT13_WILDCARDS) | \ + (1u << OFPTFPT13_WRITE_SETFIELD) | \ + (1u << OFPTFPT13_APPLY_SETFIELD)) + }; /* Body of reply to OFPMP13_PORT request. If a counter is unsupported, set diff --git a/include/openvswitch/ofp-table.h b/include/openvswitch/ofp-table.h index 713ce26d014d..370ec85aec55 100644 --- a/include/openvswitch/ofp-table.h +++ b/include/openvswitch/ofp-table.h @@ -187,13 +187,23 @@ void ofputil_table_desc_format(struct ds *, * include support for a subset of ofp_table_features through OFPST_TABLE (aka * OFPMP_TABLE). */ struct ofputil_table_features { - uint8_t table_id; /* Identifier of table. Lower numbered tables - are consulted first. */ + /* Only for OFPT_TABLE_FEATURES requests and only the first table_features + * in such a request. */ + enum ofp15_table_features_command command; + + /* The following are always present in table features requests and + * replies. */ + uint8_t table_id; char name[OFP_MAX_TABLE_NAME_LEN]; ovs_be64 metadata_match; /* Bits of metadata table can match. */ ovs_be64 metadata_write; /* Bits of metadata table can write. */ uint32_t max_entries; /* Max number of entries supported. */ + /* True if the message included any properties. This is important for + * OFPT_TABLE_FEATURES requests, which change table properties only if any + * are included. */ + bool any_properties; + /* Flags. * * 'miss_config' is relevant for OpenFlow 1.1 and 1.2 only, because those diff --git a/lib/ofp-table.c b/lib/ofp-table.c index 88fc322408ee..afa9b9103b88 100644 --- a/lib/ofp-table.c +++ b/lib/ofp-table.c @@ -74,6 +74,33 @@ ofputil_table_vacancy_to_string(enum ofputil_table_vacancy vacancy) default: return "***error***"; } } + +static bool +ofp15_table_features_command_is_valid(enum ofp15_table_features_command cmd) +{ + switch (cmd) { + case OFPTFC15_REPLACE: + case OFPTFC15_MODIFY: + case OFPTFC15_ENABLE: + case OFPTFC15_DISABLE: + return true; + + default: + return false; + } +} + +static const char * +ofp15_table_features_command_to_string(enum ofp15_table_features_command cmd) +{ + switch (cmd) { + case OFPTFC15_REPLACE: return "replace"; + case OFPTFC15_MODIFY: return "modify"; + case OFPTFC15_ENABLE: return "enable"; + case OFPTFC15_DISABLE: return "disable"; + default: return "***bad command***"; + } +} /* ofputil_table_map. */ @@ -361,6 +388,15 @@ ofputil_decode_table_features(struct ofpbuf *msg, return OFPERR_OFPBPC_BAD_LEN; } + if (oh->version >= OFP15_VERSION) { + if (!ofp15_table_features_command_is_valid(otf->command)) { + return OFPERR_OFPTFFC_BAD_COMMAND; + } + tf->command = otf->command; + } else { + tf->command = OFPTFC15_REPLACE; + } + tf->table_id = otf->table_id; if (tf->table_id == OFPTT_ALL) { return OFPERR_OFPTFFC_BAD_TABLE; @@ -382,7 +418,9 @@ ofputil_decode_table_features(struct ofpbuf *msg, struct ofpbuf properties = ofpbuf_const_initializer(ofpbuf_pull(msg, len), len); + tf->any_properties = properties.size > 0; ofpbuf_pull(&properties, sizeof *otf); + uint32_t seen = 0; while (properties.size > 0) { struct ofpbuf payload; enum ofperr error; @@ -393,6 +431,14 @@ ofputil_decode_table_features(struct ofpbuf *msg, return error; } + if (type < 32) { + uint32_t bit = 1u << type; + if (seen & bit) { + return OFPERR_OFPTFFC_BAD_FEATURES; + } + seen |= bit; + } + switch ((enum ofp13_table_feature_prop_type) type) { case OFPTFPT13_INSTRUCTIONS: error = parse_instruction_ids(&payload, loose, @@ -464,6 +510,36 @@ ofputil_decode_table_features(struct ofpbuf *msg, } } + /* OpenFlow 1.3 and 1.4 always require all of the required properties. + * OpenFlow 1.5 requires all of them if any property is present. */ + if ((seen & OFPTFPT13_REQUIRED) != OFPTFPT13_REQUIRED + && (tf->any_properties || oh->version < OFP15_VERSION)) { + VLOG_WARN_RL(&rl, "table features message missing required property"); + return OFPERR_OFPTFFC_BAD_FEATURES; + } + + /* Copy nonmiss to miss when appropriate. */ + if (tf->any_properties) { + if (!(seen & (1u << OFPTFPT13_INSTRUCTIONS_MISS))) { + tf->miss.instructions = tf->nonmiss.instructions; + } + if (!(seen & (1u << OFPTFPT13_NEXT_TABLES_MISS))) { + memcpy(tf->miss.next, tf->nonmiss.next, sizeof tf->miss.next); + } + if (!(seen & (1u << OFPTFPT13_WRITE_ACTIONS_MISS))) { + tf->miss.write.ofpacts = tf->nonmiss.write.ofpacts; + } + if (!(seen & (1u << OFPTFPT13_APPLY_ACTIONS_MISS))) { + tf->miss.apply.ofpacts = tf->nonmiss.apply.ofpacts; + } + if (!(seen & (1u << OFPTFPT13_WRITE_SETFIELD_MISS))) { + tf->miss.write.set_fields = tf->nonmiss.write.set_fields; + } + if (!(seen & (1u << OFPTFPT13_APPLY_SETFIELD_MISS))) { + tf->miss.apply.set_fields = tf->nonmiss.apply.set_fields; + } + } + /* Fix inconsistencies: * * - Turn on 'match' bits that are set in 'mask', because maskable @@ -577,6 +653,7 @@ ofputil_append_table_features_reply(const struct ofputil_table_features *tf, otf = ofpbuf_put_zeros(reply, sizeof *otf); otf->table_id = tf->table_id; + otf->command = version >= OFP15_VERSION ? tf->command : 0; ovs_strlcpy_arrays(otf->name, tf->name); otf->metadata_match = tf->metadata_match; otf->metadata_write = tf->metadata_write; @@ -1434,6 +1511,12 @@ ofputil_table_features_format( const struct ofputil_table_stats *prev_stats, int *first_ditto, int *last_ditto) { + if (!prev_features && features->command != OFPTFC15_REPLACE) { + ds_put_format(s, "\n command: %s", + ofp15_table_features_command_to_string( + features->command)); + } + int table = features->table_id; int prev_table = prev_features ? prev_features->table_id : 0; From patchwork Thu Aug 30 20:00:54 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 964169 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 421YN00tJqz9rvt for ; Fri, 31 Aug 2018 06:03:44 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 32C92D09; Thu, 30 Aug 2018 20:01:17 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 8FFF2D00 for ; Thu, 30 Aug 2018 20:01:14 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id C7AD5466 for ; Thu, 30 Aug 2018 20:01:12 +0000 (UTC) X-Originating-IP: 173.228.112.177 Received: from sigabrt.gateway.sonic.net (173-228-112-177.dsl.dynamic.fusionbroadband.com [173.228.112.177]) (Authenticated sender: blp@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 3F3EF20008; Thu, 30 Aug 2018 20:01:09 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 30 Aug 2018 13:00:54 -0700 Message-Id: <20180830200056.15484-6-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20180830200056.15484-1-blp@ovn.org> References: <20180830200056.15484-1-blp@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 6/8] ofproto: Handle multipart requests with multiple parts. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org OpenFlow has a concept of multipart messages, that is, messages that can be broken into multiple pieces that are sent separately. Before OpenFlow 1.3, only replies could actually have multiple pieces. OpenFlow 1.3 introduced the idea that requests could have multiple pieces. This is only useful for multipart requests that take an array as part of the request, which amounts to only flow monitoring requests and table features requests. So far, OVS hasn't implemented the multipart versions of these (it just reports an error). This commit introduces the necessary infastructure to implement them properly. Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- include/openvswitch/ofp-msgs.h | 20 ++- lib/ofp-msgs.c | 340 +++++++++++++++++++++++++++++++++++------ ofproto/connmgr.c | 35 ++++- ofproto/connmgr.h | 2 +- ofproto/ofproto.c | 41 +++-- 5 files changed, 362 insertions(+), 76 deletions(-) diff --git a/include/openvswitch/ofp-msgs.h b/include/openvswitch/ofp-msgs.h index 8a32a3dc69fa..66897b1cdeab 100644 --- a/include/openvswitch/ofp-msgs.h +++ b/include/openvswitch/ofp-msgs.h @@ -45,6 +45,7 @@ extern "C" { #endif +struct hmap; struct ovs_list; /* Raw identifiers for OpenFlow messages. @@ -815,10 +816,27 @@ void ofpmp_postappend(struct ovs_list *, size_t start_ofs); enum ofp_version ofpmp_version(struct ovs_list *); enum ofpraw ofpmp_decode_raw(struct ovs_list *); -/* Decoding multipart replies. */ +/* Decoding multipart messages. */ uint16_t ofpmp_flags(const struct ofp_header *); bool ofpmp_more(const struct ofp_header *); +/* Multipart request assembler. + * + * OpenFlow 1.3 and later support making multipart requests that span more than + * one OpenFlow message. These functions reassemble such requests. + * + * A reassembler is simply an hmap. The following functions manipulate an hmap + * used for this purpose. */ + +void ofpmp_assembler_clear(struct hmap *assembler); + +struct ofpbuf *ofpmp_assembler_run(struct hmap *assembler, long long int now) + OVS_WARN_UNUSED_RESULT; +long long int ofpmp_assembler_wait(struct hmap *assembler); + +enum ofperr ofpmp_assembler_execute(struct hmap *assembler, struct ofpbuf *msg, + struct ovs_list *out, long long int now); + #ifdef __cplusplus } #endif diff --git a/lib/ofp-msgs.c b/lib/ofp-msgs.c index 6517210c2cdf..6b5dfee722a5 100644 --- a/lib/ofp-msgs.c +++ b/lib/ofp-msgs.c @@ -428,56 +428,16 @@ ofpraw_decode_assert(const struct ofp_header *oh) return raw; } -/* Determines the OFPRAW_* type of the OpenFlow message in 'msg', which starts - * at 'msg->data' and has length 'msg->size' bytes. On success, - * returns 0 and stores the type into '*rawp'. On failure, returns an OFPERR_* - * error code and zeros '*rawp'. - * - * This function checks that the message has a valid length for its particular - * type of message, and returns an error if not. - * - * In addition to setting '*rawp', this function pulls off the OpenFlow header - * (including the stats headers, vendor header, and any subtype header) with - * ofpbuf_pull(). It also sets 'msg->header' to the start of the OpenFlow - * header and 'msg->msg' just beyond the headers (that is, to the final value - * of msg->data). */ -enum ofperr -ofpraw_pull(enum ofpraw *rawp, struct ofpbuf *msg) +/* Checks that 'len' is a valid length for an OpenFlow message that corresponds + * to 'info' and 'instance'. Returns 0 if so, otherwise an OpenFlow error. */ +static enum ofperr +ofpraw_check_length(const struct raw_info *info, + const struct raw_instance *instance, + size_t len) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); - const struct raw_instance *instance; - const struct raw_info *info; - struct ofphdrs hdrs; - - unsigned int min_len; - unsigned int len; - - enum ofperr error; - enum ofpraw raw; - - /* Set default outputs. */ - msg->header = msg->data; - msg->msg = msg->header; - *rawp = 0; - - len = msg->size; - error = ofphdrs_decode(&hdrs, msg->data, len); - if (error) { - return error; - } - - error = ofpraw_from_ofphdrs(&raw, &hdrs); - if (error) { - return error; - } - - info = raw_info_get(raw); - instance = raw_instance_get(info, hdrs.version); - msg->header = ofpbuf_pull(msg, instance->hdrs_len); - msg->msg = msg->data; - - min_len = instance->hdrs_len + info->min_body; + size_t min_len = instance->hdrs_len + info->min_body; switch (info->extra_multiple) { case 0: if (len != min_len) { @@ -507,6 +467,51 @@ ofpraw_pull(enum ofpraw *rawp, struct ofpbuf *msg) break; } + return 0; +} + +/* Determines the OFPRAW_* type of the OpenFlow message in 'msg', which starts + * at 'msg->data' and has length 'msg->size' bytes. On success, + * returns 0 and stores the type into '*rawp'. On failure, returns an OFPERR_* + * error code and zeros '*rawp'. + * + * This function checks that the message has a valid length for its particular + * type of message, and returns an error if not. + * + * In addition to setting '*rawp', this function pulls off the OpenFlow header + * (including the stats headers, vendor header, and any subtype header) with + * ofpbuf_pull(). It also sets 'msg->header' to the start of the OpenFlow + * header and 'msg->msg' just beyond the headers (that is, to the final value + * of msg->data). */ +enum ofperr +ofpraw_pull(enum ofpraw *rawp, struct ofpbuf *msg) +{ + /* Set default outputs. */ + msg->header = msg->data; + msg->msg = msg->header; + *rawp = 0; + + struct ofphdrs hdrs; + enum ofperr error = ofphdrs_decode(&hdrs, msg->data, msg->size); + if (error) { + return error; + } + + enum ofpraw raw; + error = ofpraw_from_ofphdrs(&raw, &hdrs); + if (error) { + return error; + } + + const struct raw_info *info = raw_info_get(raw); + const struct raw_instance *instance = raw_instance_get(info, hdrs.version); + error = ofpraw_check_length(info, instance, msg->size); + if (error) { + return error; + } + + msg->header = ofpbuf_pull(msg, instance->hdrs_len); + msg->msg = msg->data; *rawp = raw; return 0; } @@ -1059,6 +1064,247 @@ ofpmp_more(const struct ofp_header *oh) return (ofpmp_flags(oh) & OFPSF_REPLY_MORE) != 0; } +/* Multipart request assembler. */ + +struct ofpmp_partial { + struct hmap_node hmap_node; /* In struct ofpmp_assembler's 'msgs'. */ + ovs_be32 xid; + enum ofpraw raw; + long long int timeout; + struct ovs_list msgs; + size_t size; + bool has_body; +}; + +static uint32_t +hash_xid(ovs_be32 xid) +{ + return hash_int((OVS_FORCE uint32_t) xid, 0); +} + +static struct ofpmp_partial * +ofpmp_assembler_find(struct hmap *assembler, ovs_be32 xid) +{ + if (hmap_is_empty(assembler)) { + /* Common case. */ + return NULL; + } + + struct ofpmp_partial *p; + HMAP_FOR_EACH_IN_BUCKET (p, hmap_node, hash_xid(xid), assembler) { + if (p->xid == xid) { + return p; + } + } + return NULL; +} + +static void +ofpmp_partial_destroy(struct hmap *assembler, struct ofpmp_partial *p) +{ + if (p) { + hmap_remove(assembler, &p->hmap_node); + ofpbuf_list_delete(&p->msgs); + free(p); + } +} + +static struct ofpbuf * +ofpmp_partial_error(struct hmap *assembler, struct ofpmp_partial *p, + enum ofperr error) +{ + const struct ofpbuf *head = ofpbuf_from_list(ovs_list_back(&p->msgs)); + const struct ofp_header *oh = head->data; + struct ofpbuf *reply = ofperr_encode_reply(error, oh); + + ofpmp_partial_destroy(assembler, p); + + return reply; +} + +/* Clears out and frees any messages currently being reassembled. Afterward, + * the caller may destroy the hmap, with hmap_destroy(), without risk of + * leaks. */ +void +ofpmp_assembler_clear(struct hmap *assembler) +{ + struct ofpmp_partial *p, *next; + HMAP_FOR_EACH_SAFE (p, next, hmap_node, assembler) { + ofpmp_partial_destroy(assembler, p); + } +} + +/* Does periodic maintenance on 'assembler'. If any partially assembled + * requests have timed out, returns an appropriate error message for the caller + * to send to the controller. + * + * 'now' should be the current time as returned by time_msec(). */ +struct ofpbuf * OVS_WARN_UNUSED_RESULT +ofpmp_assembler_run(struct hmap *assembler, long long int now) +{ + struct ofpmp_partial *p; + HMAP_FOR_EACH (p, hmap_node, assembler) { + if (now >= p->timeout) { + return ofpmp_partial_error( + assembler, p, OFPERR_OFPBRC_MULTIPART_REQUEST_TIMEOUT); + } + } + return NULL; +} + +/* Returns the time at which the next partially assembled request times out. + * The caller should pass this time to poll_timer_wait_until(). */ +long long int +ofpmp_assembler_wait(struct hmap *assembler) +{ + long long int timeout = LLONG_MAX; + + struct ofpmp_partial *p; + HMAP_FOR_EACH (p, hmap_node, assembler) { + timeout = MIN(timeout, p->timeout); + } + + return timeout; +} + +/* Submits 'msg' to 'assembler' for reassembly. + * + * If 'msg' was accepted, returns 0 and initializes 'out' either to an empty + * list (if 'msg' is being held for reassembly) or to a list of one or more + * reassembled messages. The reassembler takes ownership of 'msg'; the caller + * takes ownership of the messages in 'out'. + * + * If 'msg' was rejected, returns an OpenFlow error that the caller should + * reply to the caller and initializes 'out' as empty. The caller retains + * ownership of 'msg'. + * + * 'now' should be the current time as returned by time_msec(). */ +enum ofperr +ofpmp_assembler_execute(struct hmap *assembler, struct ofpbuf *msg, + struct ovs_list *out, long long int now) +{ + ovs_list_init(out); + + /* If the message is not a multipart request, pass it along without further + * inspection. + * + * We could also do this kind of early-out for multipart requests that have + * only a single piece, or for pre-OF1.3 multipart requests (since only + * OF1.3 introduced multipart requests with more than one piece), but we + * don't because this allows us to assure code that runs after us that + * invariants checked below on correct message lengths are always + * satisfied, even if there's only a single piece. */ + struct ofp_header *oh = msg->data; + if (!ofpmsg_is_stat_request(oh)) { + ovs_list_push_back(out, &msg->list_node); + return 0; + } + + /* Decode the multipart request. */ + struct ofphdrs hdrs; + enum ofperr error = ofphdrs_decode(&hdrs, msg->data, msg->size); + if (error) { + return error; + } + + enum ofpraw raw; + error = ofpraw_from_ofphdrs(&raw, &hdrs); + if (error) { + return error; + } + + /* If the message has a nonempty body, check that it is a valid length. + * + * The OpenFlow spec says that pieces with empty bodies are allowed + * anywhere in a multipart sequence, so for now we allow such messages even + * if the overall multipart request requires a body. */ + const struct raw_info *info = raw_info_get(raw); + const struct raw_instance *instance = raw_instance_get(info, hdrs.version); + unsigned int min_len = ofphdrs_len(&hdrs); + bool has_body = msg->size > min_len; + if (has_body) { + error = ofpraw_check_length(info, instance, msg->size); + if (error) { + return error; + } + } + + /* Find or create an ofpmp_partial record. */ + struct ofpmp_partial *p = ofpmp_assembler_find(assembler, oh->xid); + if (!p) { + p = xzalloc(sizeof *p); + hmap_insert(assembler, &p->hmap_node, hash_xid(oh->xid)); + p->xid = oh->xid; + ovs_list_init(&p->msgs); + p->raw = raw; + } + p->timeout = now + 1000; + + /* Check that the type is the same as any previous messages in this + * sequence. */ + if (p->raw != raw) { + ofpmp_partial_destroy(assembler, p); + return OFPERR_OFPBRC_BAD_STAT; + } + + /* Limit the size of a multipart sequence. + * + * (Table features requests can actually be over 1 MB.) */ + p->size += msg->size; + if (p->size > 4 * 1024 * 1024) { + ofpmp_partial_destroy(assembler, p); + return OFPERR_OFPBRC_MULTIPART_BUFFER_OVERFLOW; + } + + /* If a multipart request type requires a body, ensure that at least one of + * the pieces in a multipart request has one. */ + bool more = oh->version >= OFP13_VERSION && ofpmp_more(oh); + if (has_body) { + p->has_body = true; + } + if (!more && !p->has_body && info->min_body) { + ofpmp_partial_destroy(assembler, p); + return OFPERR_OFPBRC_BAD_LEN; + } + + /* Append the part to the list. + * + * If there are more pieces to come, we're done for now. */ + ovs_list_push_back(&p->msgs, &msg->list_node); + if (more) { + return 0; + } + + /* This multipart request is complete. Move the messages from 'p' to 'out' + * and discard 'p'. */ + ovs_list_move(out, &p->msgs); + ovs_list_init(&p->msgs); + ofpmp_partial_destroy(assembler, p); + + /* Delete pieces with empty bodies from 'out' (but leave at least one + * piece). + * + * Most types of multipart requests have fixed-size bodies. For example, + * OFPMP_PORT_DESCRIPTION has an 8-byte body. Thus, it doesn't really make + * sense for a controller to use multiple pieces for these messages, and + * it's simpler to implement OVS as if they weren't really multipart. + * + * However, the OpenFlow spec says that messages with empty bodies are + * allowed anywhere in a multipart sequence, so in theory a controller + * could send an OFPMP_PORT_DESCRIPTION with an 8-byte body bracketed + * on either side by parts with 0-byte bodies. We remove the 0-byte + * ones here to simplify processing later. + */ + struct ofpbuf *b, *next; + LIST_FOR_EACH_SAFE (b, next, list_node, out) { + if (b->size <= min_len && !ovs_list_is_short(out)) { + ovs_list_remove(&b->list_node); + ofpbuf_delete(b); + } + } + return 0; +} + static void ofpmsgs_init(void); static const struct raw_info * diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index f78b4c5ff411..1f5964d57cce 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -101,6 +101,9 @@ struct ofconn { long long int next_op_report; /* Time to report ops, or LLONG_MAX. */ long long int op_backoff; /* Earliest time to report ops again. */ + /* Reassembly of multipart requests. */ + struct hmap assembler; + /* Flow monitors (e.g. NXST_FLOW_MONITOR). */ /* Configuration. Contains "struct ofmonitor"s. */ @@ -156,7 +159,7 @@ static void ofconn_reconfigure(struct ofconn *, static void ofconn_run(struct ofconn *, void (*handle_openflow)(struct ofconn *, - const struct ofpbuf *ofp_msg)); + const struct ovs_list *msgs)); static void ofconn_wait(struct ofconn *); static void ofconn_log_flow_mods(struct ofconn *); @@ -347,7 +350,7 @@ connmgr_destroy(struct connmgr *mgr) void connmgr_run(struct connmgr *mgr, void (*handle_openflow)(struct ofconn *, - const struct ofpbuf *ofp_msg)) + const struct ovs_list *msgs)) OVS_EXCLUDED(ofproto_mutex) { struct ofconn *ofconn, *next_ofconn; @@ -1299,6 +1302,8 @@ ofconn_create(struct connmgr *mgr, struct rconn *rconn, enum ofconn_type type, hmap_init(&ofconn->bundles); ofconn->next_bundle_expiry_check = time_msec() + BUNDLE_EXPIRY_INTERVAL; + hmap_init(&ofconn->assembler); + ofconn_flush(ofconn); return ofconn; @@ -1353,6 +1358,8 @@ ofconn_flush(struct ofconn *ofconn) rconn_packet_counter_destroy(ofconn->monitor_counter); ofconn->monitor_counter = rconn_packet_counter_create(); ofpbuf_list_delete(&ofconn->updates); /* ...but it should be empty. */ + + ofpmp_assembler_clear(&ofconn->assembler); } static void @@ -1379,6 +1386,9 @@ ofconn_destroy(struct ofconn *ofconn) rconn_packet_counter_destroy(ofconn->packet_in_counter); rconn_packet_counter_destroy(ofconn->reply_counter); rconn_packet_counter_destroy(ofconn->monitor_counter); + + hmap_destroy(&ofconn->assembler); + free(ofconn); } @@ -1418,7 +1428,7 @@ ofconn_may_recv(const struct ofconn *ofconn) static void ofconn_run(struct ofconn *ofconn, void (*handle_openflow)(struct ofconn *, - const struct ofpbuf *ofp_msg)) + const struct ovs_list *msgs)) { struct connmgr *mgr = ofconn->connmgr; size_t i; @@ -1443,8 +1453,16 @@ ofconn_run(struct ofconn *ofconn, fail_open_maybe_recover(mgr->fail_open); } - handle_openflow(ofconn, of_msg); - ofpbuf_delete(of_msg); + struct ovs_list msgs; + enum ofperr error = ofpmp_assembler_execute(&ofconn->assembler, of_msg, + &msgs, time_msec()); + if (error) { + ofconn_send_error(ofconn, of_msg->data, error); + ofpbuf_delete(of_msg); + } else if (!ovs_list_is_empty(&msgs)) { + handle_openflow(ofconn, &msgs); + ofpbuf_list_delete(&msgs); + } } long long int now = time_msec(); @@ -1458,6 +1476,12 @@ ofconn_run(struct ofconn *ofconn, ofconn_log_flow_mods(ofconn); } + struct ofpbuf *error = ofpmp_assembler_run(&ofconn->assembler, + time_msec()); + if (error) { + ofconn_send(ofconn, error, NULL); + } + ovs_mutex_lock(&ofproto_mutex); if (!rconn_is_alive(ofconn->rconn)) { ofconn_destroy(ofconn); @@ -1482,6 +1506,7 @@ ofconn_wait(struct ofconn *ofconn) if (ofconn->next_op_report != LLONG_MAX) { poll_timer_wait_until(ofconn->next_op_report); } + poll_timer_wait_until(ofpmp_assembler_wait(&ofconn->assembler)); } static void diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h index eb3be16686c5..46c1c84d15fd 100644 --- a/ofproto/connmgr.h +++ b/ofproto/connmgr.h @@ -79,7 +79,7 @@ void connmgr_destroy(struct connmgr *) void connmgr_run(struct connmgr *, void (*handle_openflow)(struct ofconn *, - const struct ofpbuf *ofp_msg)); + const struct ovs_list *msgs)); void connmgr_wait(struct connmgr *); void connmgr_get_memory_usage(const struct connmgr *, struct simap *usage); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 9552a585d096..c9d73c10c0ae 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -258,7 +258,7 @@ static void delete_flows__(struct rule_collection *, static void ofproto_group_delete_all__(struct ofproto *) OVS_REQUIRES(ofproto_mutex); static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id); -static void handle_openflow(struct ofconn *, const struct ofpbuf *); +static void handle_openflow(struct ofconn *, const struct ovs_list *msgs); static enum ofperr ofproto_flow_mod_init(struct ofproto *, struct ofproto_flow_mod *, const struct ofputil_flow_mod *fm, @@ -8081,26 +8081,14 @@ handle_tlv_table_request(struct ofconn *ofconn, const struct ofp_header *oh) return 0; } +/* Processes the single-part OpenFlow message 'oh' that was received on + * 'ofconn'. Returns an ofperr that, if nonzero, the caller should send back + * to the controller. */ static enum ofperr -handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg) +handle_single_part_openflow(struct ofconn *ofconn, const struct ofp_header *oh, + enum ofptype type) OVS_EXCLUDED(ofproto_mutex) { - const struct ofp_header *oh = msg->data; - enum ofptype type; - enum ofperr error; - - error = ofptype_decode(&type, oh); - if (error) { - return error; - } - if (oh->version >= OFP13_VERSION && ofpmsg_is_stat_request(oh) - && ofpmp_more(oh)) { - /* We have no buffer implementation for multipart requests. - * Report overflow for requests which consists of multiple - * messages. */ - return OFPERR_OFPBRC_MULTIPART_BUFFER_OVERFLOW; - } - switch (type) { /* OpenFlow requests. */ case OFPTYPE_ECHO_REQUEST: @@ -8288,15 +8276,24 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg) } static void -handle_openflow(struct ofconn *ofconn, const struct ofpbuf *ofp_msg) +handle_openflow(struct ofconn *ofconn, const struct ovs_list *msgs) OVS_EXCLUDED(ofproto_mutex) { - enum ofperr error = handle_openflow__(ofconn, ofp_msg); + COVERAGE_INC(ofproto_recv_openflow); + struct ofpbuf *msg = ofpbuf_from_list(ovs_list_front(msgs)); + enum ofptype type; + enum ofperr error = ofptype_decode(&type, msg->data); + if (!error) { + if (!ovs_list_is_short(msgs)) { + error = OFPERR_OFPBRC_BAD_STAT; + } else { + error = handle_single_part_openflow(ofconn, msg->data, type); + } + } if (error) { - ofconn_send_error(ofconn, ofp_msg->data, error); + ofconn_send_error(ofconn, msg->data, error); } - COVERAGE_INC(ofproto_recv_openflow); } static uint64_t From patchwork Thu Aug 30 20:00:55 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 964171 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 421YNt6w0Sz9rvt for ; Fri, 31 Aug 2018 06:04:30 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id BA5ECD3E; Thu, 30 Aug 2018 20:01:21 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 23018D1C for ; Thu, 30 Aug 2018 20:01:19 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 05DCC7C4 for ; Thu, 30 Aug 2018 20:01:15 +0000 (UTC) X-Originating-IP: 173.228.112.177 Received: from sigabrt.gateway.sonic.net (173-228-112-177.dsl.dynamic.fusionbroadband.com [173.228.112.177]) (Authenticated sender: blp@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 014482000A; Thu, 30 Aug 2018 20:01:11 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 30 Aug 2018 13:00:55 -0700 Message-Id: <20180830200056.15484-7-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20180830200056.15484-1-blp@ovn.org> References: <20180830200056.15484-1-blp@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, T_FILL_THIS_FORM_SHORT autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff , Brad Cowie Subject: [ovs-dev] [PATCH 7/8] ovs-vswitchd: Implement OFPT_TABLE_FEATURES table modification request. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org This allows a controller to change the name of OpenFlow flow tables in the OVS software switch. CC: Brad Cowie Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- NEWS | 4 + include/openvswitch/ofp-errors.h | 3 + include/openvswitch/ofp-msgs.h | 2 +- include/openvswitch/ofp-table.h | 17 ++- include/openvswitch/vconn.h | 2 + lib/ofp-print.c | 5 +- lib/ofp-table.c | 177 ++++++++++++++++++++---- lib/vconn.c | 40 ++++++ ofproto/ofproto-dpif.c | 1 + ofproto/ofproto-provider.h | 19 +++ ofproto/ofproto.c | 284 +++++++++++++++++++++++++++++++++++---- tests/ofproto.at | 90 +++++++++++++ utilities/ovs-ofctl.8.in | 11 ++ utilities/ovs-ofctl.c | 127 ++++++++++++++--- 14 files changed, 701 insertions(+), 81 deletions(-) diff --git a/NEWS b/NEWS index 33b4d8a2368f..6afeeacd6a5e 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,10 @@ Post-v2.10.0 --------------------- - Linux datapath: * Support for the kernel versions 4.16.x and 4.17.x. + - OpenFlow: + * OFPMP_TABLE_FEATURES_REQUEST can now modify table features. + - ovs-ofctl: + * "mod-table" command can now change OpenFlow table names. - The environment variable OVS_SYSLOG_METHOD, if set, is now used as the default syslog method. - The environment variable OVS_CTL_TIMEOUT, if set, is now used diff --git a/include/openvswitch/ofp-errors.h b/include/openvswitch/ofp-errors.h index 6e8e55ab4f4f..2f00851f1b7a 100644 --- a/include/openvswitch/ofp-errors.h +++ b/include/openvswitch/ofp-errors.h @@ -674,6 +674,9 @@ enum ofperr { /* OF1.5+(13,10). Can't handle this many flow tables. */ OFPERR_OFPTFFC_TOO_MANY, + /* NX1.3+(44). Table specified multiple times. */ + OFPERR_NXTFFC_DUP_TABLE, + /* ## ------------------ ## */ /* ## OFPET_BAD_PROPERTY ## */ diff --git a/include/openvswitch/ofp-msgs.h b/include/openvswitch/ofp-msgs.h index 66897b1cdeab..067ab3f0b095 100644 --- a/include/openvswitch/ofp-msgs.h +++ b/include/openvswitch/ofp-msgs.h @@ -422,7 +422,7 @@ enum ofpraw { /* OFPST 1.3+ (11): struct ofp13_meter_features. */ OFPRAW_OFPST13_METER_FEATURES_REPLY, - /* OFPST 1.3+ (12): void. */ + /* OFPST 1.3+ (12): uint8_t[8][]. */ OFPRAW_OFPST13_TABLE_FEATURES_REQUEST, /* OFPST 1.3+ (12): struct ofp13_table_features, uint8_t[8][]. */ diff --git a/include/openvswitch/ofp-table.h b/include/openvswitch/ofp-table.h index 370ec85aec55..8e0a1cbe2e3a 100644 --- a/include/openvswitch/ofp-table.h +++ b/include/openvswitch/ofp-table.h @@ -155,7 +155,7 @@ struct ofpbuf *ofputil_encode_table_mod(const struct ofputil_table_mod *, enum ofputil_protocol); void ofputil_table_mod_format(struct ds *, const struct ofputil_table_mod *, const struct ofputil_table_map *); -char *parse_ofp_table_mod(struct ofputil_table_mod *, +char *parse_ofp_table_mod(struct ofputil_table_mod *, const char **namep, const char *table_id, const char *flow_miss_handling, const struct ofputil_table_map *, uint32_t *usable_versions) @@ -271,15 +271,18 @@ struct ofputil_table_features { struct mf_bitmap wildcard; /* Subset of 'match' that may be wildcarded. */ }; -int ofputil_decode_table_features(struct ofpbuf *, - struct ofputil_table_features *, bool loose); +int ofputil_decode_table_features( + struct ofpbuf *, struct ofputil_table_features *, + struct ofpbuf *raw_properties); struct ofpbuf *ofputil_encode_table_features_request(enum ofp_version); struct ofpbuf *ofputil_encode_table_desc_request(enum ofp_version); -void ofputil_append_table_features_reply( - const struct ofputil_table_features *tf, struct ovs_list *replies); +void ofputil_append_table_features( + const struct ofputil_table_features *tf, + const struct ofpbuf *raw_properties, + struct ovs_list *msgs); void ofputil_table_features_format( struct ds *, const struct ofputil_table_features *features, @@ -290,6 +293,10 @@ void ofputil_table_features_format( void ofputil_table_features_format_finish(struct ds *, int first_ditto, int last_ditto); +bool ofputil_table_features_are_superset( + const struct ofputil_table_features *super, + const struct ofputil_table_features *sub); + /* Abstract table stats. * * This corresponds to the OpenFlow 1.3 table statistics structure, which only diff --git a/include/openvswitch/vconn.h b/include/openvswitch/vconn.h index b30d46af0eb8..a9316b111dae 100644 --- a/include/openvswitch/vconn.h +++ b/include/openvswitch/vconn.h @@ -58,6 +58,8 @@ int vconn_transact(struct vconn *, struct ofpbuf *, struct ofpbuf **); int vconn_transact_noreply(struct vconn *, struct ofpbuf *, struct ofpbuf **); int vconn_transact_multiple_noreply(struct vconn *, struct ovs_list *requests, struct ofpbuf **replyp); +int vconn_transact_multipart(struct vconn *, struct ovs_list *request, + struct ovs_list *replies); int vconn_dump_flows(struct vconn *, const struct ofputil_flow_stats_request *, enum ofputil_protocol, diff --git a/lib/ofp-print.c b/lib/ofp-print.c index 9d4141e3b747..9d435336c2f2 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -234,9 +234,8 @@ ofp_print_table_features_reply(struct ds *s, const struct ofp_header *oh) int first_ditto = -1, last_ditto = -1; for (int i = 0; ; i++) { struct ofputil_table_features tf; - int retval; - - retval = ofputil_decode_table_features(&b, &tf, true); + struct ofpbuf raw_properties; + int retval = ofputil_decode_table_features(&b, &tf, &raw_properties); if (retval) { ofputil_table_features_format_finish(s, first_ditto, last_ditto); return retval != EOF ? retval : 0; diff --git a/lib/ofp-table.c b/lib/ofp-table.c index afa9b9103b88..e295441af20a 100644 --- a/lib/ofp-table.c +++ b/lib/ofp-table.c @@ -350,11 +350,15 @@ parse_oxms(struct ofpbuf *payload, bool loose, /* Converts an OFPMP_TABLE_FEATURES request or reply in 'msg' into an abstract * ofputil_table_features in 'tf'. * - * If 'loose' is true, this function ignores properties and values that it does - * not understand, as a controller would want to do when interpreting - * capabilities provided by a switch. If 'loose' is false, this function - * treats unknown properties and values as an error, as a switch would want to - * do when interpreting a configuration request made by a controller. + * If 'raw_properties' is nonnull function ignores properties and values that + * it does not understand, as a controller would want to do when interpreting + * capabilities provided by a switch. In this mode, on success, it initializes + * 'raw_properties' to contain the properties that were parsed; this allows the + * caller to later re-serialize the same properties without change. + * + * If 'raw_properties' is null, this function treats unknown properties and + * values as an error, as a switch would want to do when interpreting a + * configuration request made by a controller. * * A single OpenFlow message can specify features for multiple tables. Calling * this function multiple times for a single 'msg' iterates through the tables @@ -365,8 +369,11 @@ parse_oxms(struct ofpbuf *payload, bool loose, * a positive "enum ofperr" value. */ int ofputil_decode_table_features(struct ofpbuf *msg, - struct ofputil_table_features *tf, bool loose) + struct ofputil_table_features *tf, + struct ofpbuf *raw_properties) { + bool loose = raw_properties != NULL; + memset(tf, 0, sizeof *tf); if (!msg->header) { @@ -418,8 +425,11 @@ ofputil_decode_table_features(struct ofpbuf *msg, struct ofpbuf properties = ofpbuf_const_initializer(ofpbuf_pull(msg, len), len); - tf->any_properties = properties.size > 0; ofpbuf_pull(&properties, sizeof *otf); + tf->any_properties = properties.size > 0; + if (raw_properties) { + *raw_properties = properties; + } uint32_t seen = 0; while (properties.size > 0) { struct ofpbuf payload; @@ -514,7 +524,7 @@ ofputil_decode_table_features(struct ofpbuf *msg, * OpenFlow 1.5 requires all of them if any property is present. */ if ((seen & OFPTFPT13_REQUIRED) != OFPTFPT13_REQUIRED && (tf->any_properties || oh->version < OFP15_VERSION)) { - VLOG_WARN_RL(&rl, "table features message missing required property"); + VLOG_WARN_RL(&rl, "table features message missing required property %d", seen); return OFPERR_OFPTFFC_BAD_FEATURES; } @@ -642,16 +652,21 @@ put_table_instruction_features( OFPTFPT13_APPLY_SETFIELD, miss_offset, version); } +/* Appends a table features record to 'msgs', which must be a + * OFPT_TABLE_FEATURES request or reply. If 'raw_properties' is nonnull, then + * it uses its contents verbatim as the table features properties, ignoring the + * corresponding members of 'tf'. */ void -ofputil_append_table_features_reply(const struct ofputil_table_features *tf, - struct ovs_list *replies) +ofputil_append_table_features(const struct ofputil_table_features *tf, + const struct ofpbuf *raw_properties, + struct ovs_list *msgs) { - struct ofpbuf *reply = ofpbuf_from_list(ovs_list_back(replies)); - enum ofp_version version = ofpmp_version(replies); - size_t start_ofs = reply->size; + struct ofpbuf *msg = ofpbuf_from_list(ovs_list_back(msgs)); + enum ofp_version version = ofpmp_version(msgs); + size_t start_ofs = msg->size; struct ofp13_table_features *otf; - otf = ofpbuf_put_zeros(reply, sizeof *otf); + otf = ofpbuf_put_zeros(msg, sizeof *otf); otf->table_id = tf->table_id; otf->command = version >= OFP15_VERSION ? tf->command : 0; ovs_strlcpy_arrays(otf->name, tf->name); @@ -667,17 +682,21 @@ ofputil_append_table_features_reply(const struct ofputil_table_features *tf, } otf->max_entries = htonl(tf->max_entries); - put_table_instruction_features(reply, &tf->nonmiss, 0, version); - put_table_instruction_features(reply, &tf->miss, 1, version); + if (raw_properties) { + ofpbuf_put(msg, raw_properties->data, raw_properties->size); + } else if (tf->any_properties) { + put_table_instruction_features(msg, &tf->nonmiss, 0, version); + put_table_instruction_features(msg, &tf->miss, 1, version); - put_fields_property(reply, &tf->match, &tf->mask, - OFPTFPT13_MATCH, version); - put_fields_property(reply, &tf->wildcard, NULL, - OFPTFPT13_WILDCARDS, version); + put_fields_property(msg, &tf->match, &tf->mask, + OFPTFPT13_MATCH, version); + put_fields_property(msg, &tf->wildcard, NULL, + OFPTFPT13_WILDCARDS, version); + } - otf = ofpbuf_at_assert(reply, start_ofs, sizeof *otf); - otf->length = htons(reply->size - start_ofs); - ofpmp_postappend(replies, start_ofs); + otf = ofpbuf_at_assert(msg, start_ofs, sizeof *otf); + otf->length = htons(msg->size - start_ofs); + ofpmp_postappend(msgs, start_ofs); } static enum ofperr @@ -1236,7 +1255,8 @@ exit: /* Convert 'table_id' and 'setting' (as described for the "mod-table" command * in the ovs-ofctl man page) into 'tm' for sending a table_mod command to a - * switch. + * switch. If 'setting' sets the name of the table, puts the new name in + * '*namep' (a suffix of 'setting'), otherwise stores NULL. * * Stores a bitmap of the OpenFlow versions that are usable for 'tm' into * '*usable_versions'. @@ -1244,12 +1264,13 @@ exit: * Returns NULL if successful, otherwise a malloc()'d string describing the * error. The caller is responsible for freeing the returned string. */ char * OVS_WARN_UNUSED_RESULT -parse_ofp_table_mod(struct ofputil_table_mod *tm, const char *table_id, - const char *setting, +parse_ofp_table_mod(struct ofputil_table_mod *tm, const char **namep, + const char *table_id, const char *setting, const struct ofputil_table_map *table_map, uint32_t *usable_versions) { *usable_versions = 0; + *namep = NULL; if (!strcasecmp(table_id, "all")) { tm->table_id = OFPTT_ALL; } else if (!ofputil_table_from_string(table_id, table_map, @@ -1293,6 +1314,10 @@ parse_ofp_table_mod(struct ofputil_table_mod *tm, const char *table_id, } else if (!strcmp(setting, "novacancy")) { tm->vacancy = OFPUTIL_TABLE_VACANCY_OFF; *usable_versions = (1 << OFP14_VERSION) | (1u << OFP15_VERSION); + } else if (tm->table_id != OFPTT_ALL && !strncmp(setting, "name:", 5)) { + *namep = setting + 5; + *usable_versions = ((1 << OFP13_VERSION) | (1u << OFP14_VERSION) + | (1u << OFP15_VERSION)); } else { return xasprintf("invalid table_mod setting %s", setting); } @@ -1644,6 +1669,106 @@ ofputil_table_features_format_finish(struct ds *s, ds_put_format(s, " tables %d...%d: ditto\n", first_ditto, last_ditto); } } + +/* Returns true if the 1-bits in 'super' are a superset of the 1-bits in 'sub', + * false otherwise. */ +static bool +be64_is_superset(ovs_be64 super, ovs_be64 sub) +{ + return (super & sub) == sub; +} + +/* Returns true if the 1-bits in 'super' are a superset of the 1-bits in 'sub', + * false otherwise. */ +static bool +u32_is_superset(uint32_t super, uint32_t sub) +{ + return (super & sub) == sub; +} + +/* Returns true if the 1-bits in 'super' are a superset of the 1-bits in 'sub', + * false otherwise. */ +static bool +u64_is_superset(uint64_t super, uint64_t sub) +{ + return (super & sub) == sub; +} + +/* Returns true if the 1-bits in 'super' are a superset of the 1-bits in 'sub', + * false otherwise. */ +static bool +ulong_is_superset(unsigned long int super, unsigned long int sub) +{ + return (super & sub) == sub; +} + +/* Returns true if the 1-bits in 'super' are a superset of the 1-bits in 'sub', + * false otherwise. 'super' and 'sub' both have 'n_bits' bits. */ +static bool +bitmap_is_superset(const unsigned long int *super, + const unsigned long int *sub, size_t n_bits) +{ + size_t n_longs = bitmap_n_longs(n_bits); + for (size_t i = 0; i < n_longs; i++) { + if (!ulong_is_superset(super[i], sub[i])) { + return false; + } + } + return true; +} + +/* Returns true if the 1-bits in 'super' are a superset of the 1-bits in 'sub', + * false otherwise. */ +static bool +mf_bitmap_is_superset(const struct mf_bitmap *super, + const struct mf_bitmap *sub) +{ + return bitmap_is_superset(super->bm, sub->bm, MFF_N_IDS); +} + +/* Returns true if 'super' is a superset of 'sub', false otherwise. */ +static bool +ofputil_table_action_features_is_superset( + const struct ofputil_table_action_features *super, + const struct ofputil_table_action_features *sub) +{ + return (u64_is_superset(super->ofpacts, sub->ofpacts) + && mf_bitmap_is_superset(&super->set_fields, &sub->set_fields)); +} + +/* Returns true if 'super' is a superset of 'sub', false otherwise. */ +static bool +ofputil_table_instruction_features_is_superset( + const struct ofputil_table_instruction_features *super, + const struct ofputil_table_instruction_features *sub) +{ + return (bitmap_is_superset(super->next, sub->next, 255) + && u32_is_superset(super->instructions, sub->instructions) + && ofputil_table_action_features_is_superset(&super->write, + &sub->write) + && ofputil_table_action_features_is_superset(&super->apply, + &sub->apply)); +} + +/* Returns true if 'super' is a superset of 'sub', false otherwise. */ +bool +ofputil_table_features_are_superset( + const struct ofputil_table_features *super, + const struct ofputil_table_features *sub) +{ + return (be64_is_superset(super->metadata_match, sub->metadata_match) + && be64_is_superset(super->metadata_write, sub->metadata_write) + && super->max_entries >= sub->max_entries + && super->supports_eviction >= sub->supports_eviction + && super->supports_vacancy_events >= sub->supports_vacancy_events + && ofputil_table_instruction_features_is_superset(&super->nonmiss, + &sub->nonmiss) + && ofputil_table_instruction_features_is_superset(&super->miss, + &sub->miss) + && mf_bitmap_is_superset(&super->match, &sub->match) + && mf_bitmap_is_superset(&super->mask, &sub->mask) + && mf_bitmap_is_superset(&super->wildcard, &sub->wildcard)); +} /* Table stats. */ diff --git a/lib/vconn.c b/lib/vconn.c index 4d5f308d8836..85fe0249af42 100644 --- a/lib/vconn.c +++ b/lib/vconn.c @@ -934,6 +934,46 @@ vconn_transact_multiple_noreply(struct vconn *vconn, struct ovs_list *requests, return 0; } +/* Sends 'requests' (which should be a multipart request) on 'vconn' and waits + * for the replies, which are put into 'replies'. Returns 0 if successful, + * otherwise an errno value. */ +int +vconn_transact_multipart(struct vconn *vconn, + struct ovs_list *requests, + struct ovs_list *replies) +{ + struct ofpbuf *rq = ofpbuf_from_list(ovs_list_front(requests)); + ovs_be32 send_xid = ((struct ofp_header *) rq->data)->xid; + + ovs_list_init(replies); + + /* Send all the requests. */ + struct ofpbuf *b, *next; + LIST_FOR_EACH_SAFE (b, next, list_node, requests) { + ovs_list_remove(&b->list_node); + int error = vconn_send_block(vconn, b); + if (error) { + ofpbuf_delete(b); + } + } + + /* Receive all the replies. */ + bool more; + do { + struct ofpbuf *reply; + int error = vconn_recv_xid__(vconn, send_xid, &reply, NULL); + if (error) { + ofpbuf_list_delete(replies); + return error; + } + + ovs_list_push_back(replies, &reply->list_node); + more = ofpmsg_is_stat_reply(reply->data) && ofpmp_more(reply->data); + } while (more); + + return 0; +} + static int recv_flow_stats_reply(struct vconn *vconn, ovs_be32 send_xid, struct ofpbuf **replyp, diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 0a0c69a7aea0..280739d26856 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -6040,6 +6040,7 @@ const struct ofproto_class ofproto_dpif_class = { type_get_memory_usage, flush, query_tables, + NULL, /* modify_tables */ set_tables_version, port_alloc, port_construct, diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 2b77b8993ada..3a83d0407978 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -219,6 +219,7 @@ struct oftable { enum oftable_flags flags; struct classifier cls; /* Contains "struct rule"s. */ char *name; /* Table name exposed via OpenFlow, or NULL. */ + int name_level; /* 0=name unset, 1=via OF, 2=via OVSDB. */ /* Maximum number of flows or UINT_MAX if there is no limit besides any * limit imposed by resource limitations. */ @@ -932,6 +933,24 @@ struct ofproto_class { struct ofputil_table_features *features, struct ofputil_table_stats *stats); + /* Helper for the OFPT_TABLE_FEATURES request. + * + * A controller is requesting that the table features be updated from 'old' + * to 'new', where 'old' is the features currently in use as previously + * initialized by 'query_tables'. + * + * If this function is nonnull, then it should either update the table + * features or return an OpenFlow error. The update should be + * all-or-nothing. + * + * If this function is null, then only updates that eliminate table + * features will be allowed. Such updates have no actual effect. This + * implementation is acceptable because OpenFlow says that a table's + * features may be a superset of those requested. */ + enum ofperr (*modify_tables)(struct ofproto *ofproto, + const struct ofputil_table_features *old, + const struct ofputil_table_features *new); + /* Sets the current tables version the provider should use for classifier * lookups. This must be called with a new version number after each set * of flow table changes has been completed, so that datapath revalidation diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index c9d73c10c0ae..d65a3fea1559 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -85,7 +85,9 @@ const enum mf_field_id default_prefix_fields[2] = static void oftable_init(struct oftable *); static void oftable_destroy(struct oftable *); -static void oftable_set_name(struct oftable *, const char *name); +static void oftable_set_name(struct oftable *, const char *name, int level); +static bool oftable_may_set_name(const struct oftable *, + const char *name, int level); static enum ofperr evict_rules_from_table(struct oftable *) OVS_REQUIRES(ofproto_mutex); @@ -1476,7 +1478,7 @@ ofproto_configure_table(struct ofproto *ofproto, int table_id, ovs_assert(table_id >= 0 && table_id < ofproto->n_tables); table = &ofproto->tables[table_id]; - oftable_set_name(table, s->name); + oftable_set_name(table, s->name, 2); if (table->flags & OFTABLE_READONLY) { return; @@ -3234,6 +3236,8 @@ query_tables(struct ofproto *ofproto, atomic_read_relaxed(&ofproto->tables[i].miss_config, &f->miss_config); f->max_entries = 1000000; + f->any_properties = true; + bool more_tables = false; for (int j = i + 1; j < ofproto->n_tables; j++) { if (!(ofproto->tables[j].flags & OFTABLE_HIDDEN)) { @@ -3765,33 +3769,238 @@ handle_table_stats_request(struct ofconn *ofconn, return 0; } -static enum ofperr -handle_table_features_request(struct ofconn *ofconn, - const struct ofp_header *request) +/* OpenFlow 1.5.1 section 7.3.5.18.1 "Table Features request and reply" + * says: + * + * If a table feature included in the request has an empty list of + * properties, the list of properties for that flow table is unchanged and + * only the other features of that flow table are updated. + * + * This function copies the "list of properties" from '*src' to '*dst'. */ +static void +copy_properties(struct ofputil_table_features *dst, + const struct ofputil_table_features *src) +{ + dst->any_properties = src->any_properties; + if (src->any_properties) { + dst->nonmiss = src->nonmiss; + dst->miss = src->miss; + dst->match = src->match; + dst->mask = src->mask; + dst->wildcard = src->wildcard; + } + +} + +/* Attempts to change the table features of the ofproto backing 'ofconn' to + * those specified in the table features request in 'msgs', given that the + * features are currently those in 'old'. + * + * Returns 0 if successful, an OpenFlow error if the caller should send an + * error message for the request as a whole, or -1 if the function already sent + * an error message for some message in 'msgs'. */ +static int +handle_table_features_change(struct ofconn *ofconn, + const struct ovs_list *msgs, + const struct ofputil_table_features old[]) { struct ofproto *ofproto = ofconn_get_ofproto(ofconn); - struct ofpbuf msg = ofpbuf_const_initializer(request, - ntohs(request->length)); - ofpraw_pull_assert(&msg); - if (msg.size || ofpmp_more(request)) { + + enum ofp15_table_features_command command = OFPTFC15_REPLACE; + struct ofputil_table_features new[255]; + + unsigned long int seen[BITMAP_N_LONGS(255)]; + memset(seen, 0, sizeof seen); + + struct ofpbuf *msg; + int n = 0; + LIST_FOR_EACH (msg, list_node, msgs) { + for (;;) { + struct ofputil_table_features tf; + int retval = ofputil_decode_table_features(msg, &tf, NULL); + if (retval == EOF) { + break; + } else if (retval) { + ofconn_send_error(ofconn, msg->header, retval); + return -1; + } + + /* Get command from first request. */ + if (!n) { + command = tf.command; + } + n++; + + /* Avoid duplicate tables. */ + if (bitmap_is_set(seen, tf.table_id)) { + VLOG_INFO_RL(&rl, "duplicate table %"PRIu8, tf.table_id); + ofconn_send_error(ofconn, msg->header, + OFPERR_NXTFFC_DUP_TABLE); + return -1; + } + bitmap_set1(seen, tf.table_id); + + /* Save table. */ + new[tf.table_id] = tf; + } + } + + if (!n) { + return 0; + } + + for (size_t i = 0; i < ofproto->n_tables; i++) { + if (ofproto->tables[i].flags & OFTABLE_HIDDEN) { + if (bitmap_is_set(seen, i)) { + VLOG_INFO_RL(&rl, "can't modify hidden table %"PRIuSIZE, i); + return OFPERR_OFPTFFC_EPERM; + } + + new[i] = old[i]; + bitmap_set1(seen, i); + } + } + + switch (command) { + case OFPTFC15_REPLACE: + break; + + case OFPTFC15_MODIFY: + for (size_t i = 0; i < ofproto->n_tables; i++) { + if (!bitmap_is_set(seen, i)) { + new[i] = old[i]; + bitmap_set1(seen, i); + } else if (!new[i].any_properties) { + copy_properties(&new[i], &old[i]); + } + } + break; + + case OFPTFC15_ENABLE: + case OFPTFC15_DISABLE: + /* It really isn't clear what these commands are supposed to do in an + * Open vSwitch context. OVS doesn't have a concept of tables that + * exist but are not in the pipeline, and OVS table ids are always + * sequential from 0. */ return OFPERR_OFPTFFC_EPERM; } + /* Make sure that the new number of tables is the same as the old number, + * because we don't support changing the number of tables or disabling + * tables. */ + int n_tables = bitmap_scan(seen, 0, 0, 255); + bool skipped_tables = bitmap_scan(seen, 1, n_tables, 255) != 255; + if (n_tables != ofproto->n_tables || skipped_tables) { + if (skipped_tables) { + VLOG_INFO_RL(&rl, "can't disable table %d", n_tables); + } else { + VLOG_INFO_RL(&rl, "can't change number of tables from %d to %d", + ofproto->n_tables, n_tables); + } + return (n_tables > ofproto->n_tables + ? OFPERR_OFPTFFC_TOO_MANY + : OFPERR_OFPTFFC_EPERM); + } + + /* OpenFlow 1.5.1 section 7.3.5.18.1 "Table Features request and reply" + * says: + * + * "All fields in ofp_table_features may be requested to be changed by + * the controller with the exception of the max_entries field, this is + * read only and returned by the switch." + * + * so forbid the controller from attempting to change it. + * + * (This seems like a particularly arbitrary prohibition since OVS could + * easily implement such a feature. Whatever.) */ + for (size_t i = 0; i < n_tables; i++) { + if (old[i].max_entries != new[i].max_entries) { + VLOG_INFO_RL(&rl, "can't change max_entries"); + return OFPERR_OFPTFFC_EPERM; + } + } + + /* Check that we can set table names. */ + for (size_t i = 0; i < n_tables; i++) { + if (!oftable_may_set_name(&ofproto->tables[i], new[i].name, 1)) { + const char *name = ofproto->tables[i].name; + VLOG_INFO_RL(&rl, "can't change name of table %"PRIuSIZE" " + "to %s because it is already set to %s via OVSDB", + i, new[i].name, name ? name : "\"\""); + return OFPERR_OFPTFFC_EPERM; + } + } + + /* Ask the provider to update its features. + * + * If the provider can't update features, just make sure that the + * controller isn't asking to enable new features. OpenFlow says it's OK + * if a superset of the requested features are actually enabled. */ + if (ofproto->ofproto_class->modify_tables) { + enum ofperr error = ofproto->ofproto_class->modify_tables(ofproto, + old, new); + if (error) { + VLOG_INFO_RL(&rl, "can't change table features"); + return error; + } + } else { + for (size_t i = 0; i < n_tables; i++) { + if (!ofputil_table_features_are_superset(&old[i], &new[i])) { + VLOG_INFO_RL(&rl, "can't increase table features " + "for table %"PRIuSIZE, i); + return OFPERR_OFPTFFC_EPERM; + } + } + } + + /* Update table names. */ + for (size_t i = 0; i < n_tables; i++) { + oftable_set_name(&ofproto->tables[i], new[i].name, 1); + } + + return 0; +} + +static void +handle_table_features_request(struct ofconn *ofconn, + const struct ovs_list *msgs) +{ + struct ofproto *ofproto = ofconn_get_ofproto(ofconn); + + struct ofpbuf *msg = ofpbuf_from_list(ovs_list_back(msgs)); + const struct ofp_header *request = msg->data; + ofpraw_pull_assert(msg); + + /* Update the table features configuration, if requested. */ struct ofputil_table_features *features; query_tables(ofproto, &features, NULL); + if (!ovs_list_is_singleton(msgs) || msg->size) { + int retval = handle_table_features_change(ofconn, msgs, features); + if (retval) { + if (retval < 0) { + /* handle_table_features_change() already sent an error. */ + } else { + ofconn_send_error(ofconn, request, retval); + } + return; + } + + /* Features may have changed, re-query. */ + free(features); + query_tables(ofproto, &features, NULL); + } + /* Reply the controller with the table configuration. */ struct ovs_list replies; ofpmp_init(&replies, request); for (size_t i = 0; i < ofproto->n_tables; i++) { if (!(ofproto->tables[i].flags & OFTABLE_HIDDEN)) { - ofputil_append_table_features_reply(&features[i], &replies); + ofputil_append_table_features(&features[i], NULL, &replies); } } ofconn_send_replies(ofconn, &replies); free(features); - - return 0; } /* Returns the vacancy of 'oftable', a number that ranges from 0 (if the table @@ -8174,7 +8383,7 @@ handle_single_part_openflow(struct ofconn *ofconn, const struct ofp_header *oh, return handle_table_stats_request(ofconn, oh); case OFPTYPE_TABLE_FEATURES_STATS_REQUEST: - return handle_table_features_request(ofconn, oh); + OVS_NOT_REACHED(); case OFPTYPE_TABLE_DESC_REQUEST: return handle_table_desc_request(ofconn, oh); @@ -8285,7 +8494,9 @@ handle_openflow(struct ofconn *ofconn, const struct ovs_list *msgs) enum ofptype type; enum ofperr error = ofptype_decode(&type, msg->data); if (!error) { - if (!ovs_list_is_short(msgs)) { + if (type == OFPTYPE_TABLE_FEATURES_STATS_REQUEST) { + handle_table_features_request(ofconn, msgs); + } else if (!ovs_list_is_short(msgs)) { error = OFPERR_OFPBRC_BAD_STAT; } else { error = handle_single_part_openflow(ofconn, msg->data, type); @@ -8610,26 +8821,49 @@ oftable_destroy(struct oftable *table) free(table->name); } -/* Changes the name of 'table' to 'name'. If 'name' is NULL or the empty - * string, then 'table' will use its default name. +/* Changes the name of 'table' to 'name'. Null or empty string 'name' unsets + * the name. + * + * 'level' should be 1 if the name is being set via OpenFlow, or 2 if the name + * is being set via OVSDB. Higher levels get precedence. * * This only affects the name exposed for a table exposed through the OpenFlow * OFPST_TABLE (as printed by "ovs-ofctl dump-tables"). */ static void -oftable_set_name(struct oftable *table, const char *name) -{ - if (name && name[0]) { - int len = strnlen(name, OFP_MAX_TABLE_NAME_LEN); - if (!table->name || strncmp(name, table->name, len)) { +oftable_set_name(struct oftable *table, const char *name, int level) +{ + int len = name ? strnlen(name, OFP_MAX_TABLE_NAME_LEN) : 0; + if (level >= table->name_level) { + if (name) { + if (name[0]) { + if (!table->name || strncmp(name, table->name, len)) { + free(table->name); + table->name = xmemdup0(name, len); + } + } else { + free(table->name); + table->name = NULL; + } + table->name_level = level; + } else if (table->name_level == level) { free(table->name); - table->name = xmemdup0(name, len); + table->name = NULL; + table->name_level = 0; } - } else { - free(table->name); - table->name = NULL; } } +/* Returns true if oftable_set_name(table, name, level) would be effective, + * false otherwise. */ +static bool +oftable_may_set_name(const struct oftable *table, const char *name, int level) +{ + return (level >= table->name_level + || !name + || !strncmp(name, table->name, + strnlen(name, OFP_MAX_TABLE_NAME_LEN))); +} + /* oftables support a choice of two policies when adding a rule would cause the * number of flows in the table to exceed the configured maximum number: either * they can refuse to add the new flow or they can evict some existing flow. diff --git a/tests/ofproto.at b/tests/ofproto.at index 2291bfb93225..6fbbd6c32859 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -2668,6 +2668,19 @@ AT_CLEANUP AT_SETUP([ofproto - flow table names]) OVS_VSWITCHD_START add_of_ports br0 1 2 + +# Set a table name via OpenFlow 1.3 and one via OpenFlow 1.5. +AT_CHECK([ovs-ofctl -O OpenFlow13 mod-table br0 0 name:xyzzy]) +AT_CHECK([ovs-ofctl -O OpenFlow15 mod-table br0 1 name:quux]) +AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], + [0], [dnl + table 0 ("xyzzy"): + table 1 ("quux"): ditto + tables 2...252: ditto + table 253: +]) + +# Set some table names via OVSDB. AT_CHECK( [ovs-vsctl \ -- --id=@t0 create Flow_Table name=zero \ @@ -2679,6 +2692,16 @@ AT_CHECK( <1> <2> ]) +AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], + [0], [dnl + table 0 ("zero"): + table 1 ("one"): ditto + table 2 ("two"): ditto + tables 3...252: ditto + table 253: +]) + +# Check that flow table parsing and dumping uses the names. AT_DATA([flows.txt], [dnl table=zero in_port=p2 actions=p1,resubmit(,one) table=one,in_port=p1,ip,actions=ct(table=two) @@ -2695,6 +2718,73 @@ AT_CHECK([ovs-ofctl --no-names --no-stats dump-flows br0], [0], [dnl table=1, ip,in_port=1 actions=ct(table=2) table=1, arp,in_port=1 actions=resubmit(,2) ]) + +# Setting the same table names via OpenFlow 1.3 or OpenFlow 1.5 is a no-op. +AT_CHECK([ovs-ofctl -O OpenFlow13 mod-table br0 0 name:zero]) +AT_CHECK([ovs-ofctl -O OpenFlow15 mod-table br0 1 name:one]) +AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], + [0], [dnl + table 0 ("zero"): + table 1 ("one"): ditto + table 2 ("two"): ditto + tables 3...252: ditto + table 253: +]) + +# Setting different tables names via OpenFlow 1.3 or OpenFlow 1.5 yield errors. +AT_CHECK([ovs-ofctl -O OpenFlow13 mod-table br0 0 name:xyzzy], 1, [], [stderr]) +AT_CHECK([head -1 stderr], [0], [OFPT_ERROR (OF1.3) (xid=0x5): OFPTFFC_EPERM +]) +AT_CHECK([ovs-ofctl -O OpenFlow15 mod-table br0 1 name:quux], 1, [], [stderr]) +AT_CHECK([head -1 stderr], [0], [OFPT_ERROR (OF1.5) (xid=0x5): OFPTFFC_EPERM +]) + +# But we can still set table names for those not set via OVSDB. +AT_CHECK([ovs-ofctl -O OpenFlow13 mod-table br0 3 name:three]) +AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], + [0], [dnl + table 0 ("zero"): + table 1 ("one"): ditto + table 2 ("two"): ditto + table 3 ("three"): ditto + tables 4...252: ditto + table 253: +]) + +# Unsetting names via OVSDB then setting them via OpenFlow works too. +AT_CHECK([ovs-vsctl remove bridge br0 Flow_Table 2]) +AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], + [0], [dnl + table 0 ("zero"): + table 1 ("one"): ditto + table 2: ditto + table 3 ("three"): ditto + tables 4...252: ditto + table 253: +]) +AT_CHECK([ovs-ofctl -O OpenFlow13 mod-table br0 2 name:foobar]) +AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], + [0], [dnl + table 0 ("zero"): + table 1 ("one"): ditto + table 2 ("foobar"): ditto + table 3 ("three"): ditto + tables 4...252: ditto + table 253: +]) + +# We can clear names via OpenFlow, at least if they were set that way. +AT_CHECK([ovs-ofctl -O OpenFlow13 mod-table br0 2 name:]) +AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], + [0], [dnl + table 0 ("zero"): + table 1 ("one"): ditto + table 2: ditto + table 3 ("three"): ditto + tables 4...252: ditto + table 253: +]) + OVS_VSWITCHD_STOP AT_CLEANUP diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in index 4850f4a25504..968e1ec347af 100644 --- a/utilities/ovs-ofctl.8.in +++ b/utilities/ovs-ofctl.8.in @@ -88,6 +88,17 @@ Send to controller. (This is how an OpenFlow 1.0 switch always handles packets that do not match any flow in the last table.) .RE .IP +In OpenFlow 1.3 and later (which must be enabled with the \fB\-O\fR +option) and Open vSwitch 2.11 and later only, \fBmod\-table\fR can +change the name of a table: +.RS +.IP \fBname:\fInew-name\fR +Changes the name of the table to \fInew-name\fR. (This will be +ineffective if the name is set via the \fBname\fR column in the +\fBFlow_Table\fR table in the \fBOpen_vSwitch\fR database as described +in \fBovs\-vswitchd.conf.db\fR(5).) +.RE +.IP In OpenFlow 1.4 and later (which must be enabled with the \fB\-O\fR option) only, \fBmod\-table\fR configures the behavior when a controller attempts to add a flow to a flow table that is full. The diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index aa9a1291e60a..82b2b42a92cd 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -913,9 +913,9 @@ ofctl_dump_table_features(struct ovs_cmdl_context *ctx) done = !ofpmp_more(reply->data); for (;;) { struct ofputil_table_features tf; - int retval; - - retval = ofputil_decode_table_features(reply, &tf, true); + struct ofpbuf raw_properties; + int retval = ofputil_decode_table_features( + reply, &tf, &raw_properties); if (retval) { if (retval != EOF) { ovs_fatal(0, "decode error: %s", @@ -1207,6 +1207,7 @@ struct table_iterator { bool more; struct ofputil_table_features features; + struct ofpbuf raw_properties; }; /* Initializes 'ti' to prepare for iterating through all of the tables on the @@ -1248,7 +1249,7 @@ table_iterator_next(struct table_iterator *ti) ovs_assert(ti->variant == TI_FEATURES); retval = ofputil_decode_table_features(ti->reply, &ti->features, - true); + &ti->raw_properties); } if (!retval) { return &ti->features; @@ -2580,16 +2581,95 @@ fetch_table_desc(struct vconn *vconn, struct ofputil_table_mod *tm, } } +static void +change_table_name(struct vconn *vconn, uint8_t table_id, const char *new_name) +{ + /* Get all tables' features and properties. */ + struct table { + struct ofputil_table_features tf; + struct ofpbuf *raw_properties; + } *tables[256]; + memset(tables, 0, sizeof tables); + + struct table_iterator ti; + table_iterator_init(&ti, vconn); + while (table_iterator_next(&ti)) { + struct table *t = tables[ti.features.table_id] = xmalloc(sizeof *t); + t->tf = ti.features; + t->raw_properties = ofpbuf_clone(&ti.raw_properties); + } + table_iterator_destroy(&ti); + + /* Change the name for table 'table_id'. */ + struct table *t = tables[table_id]; + if (!t) { + ovs_fatal(0, "switch does not have table %"PRIu8, table_id); + } + ovs_strlcpy(t->tf.name, new_name, OFP_MAX_TABLE_NAME_LEN); + + /* Compose the transaction. */ + enum ofp_version version = vconn_get_version(vconn); + struct ovs_list requests = OVS_LIST_INITIALIZER(&requests); + struct ofpbuf *tfr = ofputil_encode_table_features_request(version); + ovs_list_push_back(&requests, &tfr->list_node); + if (version >= OFP15_VERSION) { + /* For OpenFlow 1.5, we can use a single OFPTFC15_MODIFY without any + * properties. */ + t->tf.command = OFPTFC15_MODIFY; + t->tf.any_properties = false; + ofputil_append_table_features(&t->tf, NULL, &requests); + } else { + /* For OpenFlow 1.3 and 1.4, we have to regurgitate all of the tables + * and their properties. */ + for (size_t i = 0; i < 256; i++) { + if (tables[i]) { + ofputil_append_table_features(&tables[i]->tf, + tables[i]->raw_properties, + &requests); + } + } + } + + /* Transact. + * + * The reply repeats the entire new configuration of the tables, so we + * don't bother printing it unless there's an error. */ + struct ovs_list replies; + struct ofpbuf *reply; + vconn_transact_multipart(vconn, &requests, &replies); + LIST_FOR_EACH (reply, list_node, &replies) { + enum ofptype type; + enum ofperr error = ofptype_decode(&type, reply->data); + if (error) { + ovs_fatal(0, "decode error: %s", ofperr_get_name(error)); + } else if (type == OFPTYPE_ERROR) { + ofp_print(stderr, reply->data, reply->size, NULL, NULL, + verbosity + 1); + exit(1); + } + } + ofpbuf_list_delete(&replies); + + /* Clean up. */ + for (size_t i = 0; i < ARRAY_SIZE(tables); i++) { + if (tables[i]) { + ofpbuf_delete(tables[i]->raw_properties); + free(tables[i]); + } + } +} + static void ofctl_mod_table(struct ovs_cmdl_context *ctx) { uint32_t usable_versions; struct ofputil_table_mod tm; + const char *name; struct vconn *vconn; char *error; int i; - error = parse_ofp_table_mod(&tm, ctx->argv[2], ctx->argv[3], + error = parse_ofp_table_mod(&tm, &name, ctx->argv[2], ctx->argv[3], tables_to_accept(ctx->argv[1]), &usable_versions); if (error) { @@ -2607,27 +2687,32 @@ ofctl_mod_table(struct ovs_cmdl_context *ctx) mask_allowed_ofp_versions(usable_versions); enum ofputil_protocol protocol = open_vconn(ctx->argv[1], &vconn); - /* For OpenFlow 1.4+, ovs-ofctl mod-table should not affect table-config - * properties that the user didn't ask to change, so it is necessary to - * restore the current configuration of table-config parameters using - * OFPMP14_TABLE_DESC request. */ - if ((allowed_versions & (1u << OFP14_VERSION)) || - (allowed_versions & (1u << OFP15_VERSION))) { - struct ofputil_table_desc td; - - if (tm.table_id == OFPTT_ALL) { - for (i = 0; i < OFPTT_MAX; i++) { - tm.table_id = i; + if (name) { + change_table_name(vconn, tm.table_id, name); + } else { + /* For OpenFlow 1.4+, ovs-ofctl mod-table should not affect + * table-config properties that the user didn't ask to change, so it is + * necessary to restore the current configuration of table-config + * parameters using OFPMP14_TABLE_DESC request. */ + if ((allowed_versions & (1u << OFP14_VERSION)) || + (allowed_versions & (1u << OFP15_VERSION))) { + struct ofputil_table_desc td; + + if (tm.table_id == OFPTT_ALL) { + for (i = 0; i < OFPTT_MAX; i++) { + tm.table_id = i; + fetch_table_desc(vconn, &tm, &td); + transact_noreply(vconn, + ofputil_encode_table_mod(&tm, protocol)); + } + } else { fetch_table_desc(vconn, &tm, &td); - transact_noreply(vconn, - ofputil_encode_table_mod(&tm, protocol)); + transact_noreply(vconn, ofputil_encode_table_mod(&tm, + protocol)); } } else { - fetch_table_desc(vconn, &tm, &td); transact_noreply(vconn, ofputil_encode_table_mod(&tm, protocol)); } - } else { - transact_noreply(vconn, ofputil_encode_table_mod(&tm, protocol)); } vconn_close(vconn); } From patchwork Thu Aug 30 20:00:56 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 964170 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 421YNX18SDz9s1x for ; Fri, 31 Aug 2018 06:04:12 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 107B2CE9; Thu, 30 Aug 2018 20:01:19 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 78D85CDB for ; Thu, 30 Aug 2018 20:01:18 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 696927DE for ; Thu, 30 Aug 2018 20:01:17 +0000 (UTC) X-Originating-IP: 173.228.112.177 Received: from sigabrt.gateway.sonic.net (173-228-112-177.dsl.dynamic.fusionbroadband.com [173.228.112.177]) (Authenticated sender: blp@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 2FE4120005; Thu, 30 Aug 2018 20:01:14 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 30 Aug 2018 13:00:56 -0700 Message-Id: <20180830200056.15484-8-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20180830200056.15484-1-blp@ovn.org> References: <20180830200056.15484-1-blp@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 8/8] ofproto: Handle flow monitor requests with multiple parts. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- ofproto/ofproto.c | 84 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 44 insertions(+), 40 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index d65a3fea1559..54f56d9f100e 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -6344,49 +6344,60 @@ flow_monitor_delete(struct ofconn *ofconn, uint32_t id) } static enum ofperr -handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh) +handle_flow_monitor_request(struct ofconn *ofconn, const struct ovs_list *msgs) OVS_EXCLUDED(ofproto_mutex) { struct ofproto *ofproto = ofconn_get_ofproto(ofconn); - struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length)); - struct ofmonitor **monitors = NULL; size_t allocated_monitors = 0; size_t n_monitors = 0; - enum ofperr error; - ovs_mutex_lock(&ofproto_mutex); - for (;;) { - struct ofputil_flow_monitor_request request; - struct ofmonitor *m; - int retval; + struct ofpbuf *b; + LIST_FOR_EACH (b, list_node, msgs) { + for (;;) { + enum ofperr error; - retval = ofputil_decode_flow_monitor_request(&request, &b); - if (retval == EOF) { - break; - } else if (retval) { - error = retval; - goto error; - } + struct ofputil_flow_monitor_request request; + int retval = ofputil_decode_flow_monitor_request(&request, b); + if (retval == EOF) { + break; + } else if (retval) { + error = retval; + goto error; + } - if (request.table_id != 0xff - && request.table_id >= ofproto->n_tables) { - error = OFPERR_OFPBRC_BAD_TABLE_ID; - goto error; - } + if (request.table_id != 0xff + && request.table_id >= ofproto->n_tables) { + error = OFPERR_OFPBRC_BAD_TABLE_ID; + goto error; + } - error = ofmonitor_create(&request, ofconn, &m); - if (error) { - goto error; - } + struct ofmonitor *m; + error = ofmonitor_create(&request, ofconn, &m); + if (error) { + goto error; + } - if (n_monitors >= allocated_monitors) { - monitors = x2nrealloc(monitors, &allocated_monitors, - sizeof *monitors); + if (n_monitors >= allocated_monitors) { + monitors = x2nrealloc(monitors, &allocated_monitors, + sizeof *monitors); + } + monitors[n_monitors++] = m; + continue; + + error: + ofconn_send_error(ofconn, b->data, error); + + for (size_t i = 0; i < n_monitors; i++) { + ofmonitor_destroy(monitors[i]); + } + free(monitors); + ovs_mutex_unlock(&ofproto_mutex); + + return error; } - monitors[n_monitors++] = m; } struct rule_collection rules; @@ -6396,7 +6407,7 @@ handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh) } struct ovs_list replies; - ofpmp_init(&replies, oh); + ofpmp_init(&replies, ofpbuf_from_list(ovs_list_back(msgs))->header); ofmonitor_compose_refresh_updates(&rules, &replies); ovs_mutex_unlock(&ofproto_mutex); @@ -6406,15 +6417,6 @@ handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh) free(monitors); return 0; - -error: - for (size_t i = 0; i < n_monitors; i++) { - ofmonitor_destroy(monitors[i]); - } - free(monitors); - ovs_mutex_unlock(&ofproto_mutex); - - return error; } static enum ofperr @@ -8398,7 +8400,7 @@ handle_single_part_openflow(struct ofconn *ofconn, const struct ofp_header *oh, return handle_port_desc_stats_request(ofconn, oh); case OFPTYPE_FLOW_MONITOR_STATS_REQUEST: - return handle_flow_monitor_request(ofconn, oh); + OVS_NOT_REACHED(); case OFPTYPE_METER_STATS_REQUEST: case OFPTYPE_METER_CONFIG_STATS_REQUEST: @@ -8496,6 +8498,8 @@ handle_openflow(struct ofconn *ofconn, const struct ovs_list *msgs) if (!error) { if (type == OFPTYPE_TABLE_FEATURES_STATS_REQUEST) { handle_table_features_request(ofconn, msgs); + } else if (type == OFPTYPE_FLOW_MONITOR_STATS_REQUEST) { + handle_flow_monitor_request(ofconn, msgs); } else if (!ovs_list_is_short(msgs)) { error = OFPERR_OFPBRC_BAD_STAT; } else {