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); }