From patchwork Thu Sep 23 00:13:53 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1531478 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.133; helo=smtp2.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4HFFyX6nXTz9sW4 for ; Thu, 23 Sep 2021 10:14:11 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id A341D40603; Thu, 23 Sep 2021 00:14:07 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id sCj3uhtMPfd2; Thu, 23 Sep 2021 00:14:05 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp2.osuosl.org (Postfix) with ESMTPS id 1159B40138; Thu, 23 Sep 2021 00:14:04 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id C982EC0011; Thu, 23 Sep 2021 00:14:03 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [IPv6:2605:bc80:3010::133]) by lists.linuxfoundation.org (Postfix) with ESMTP id CFAADC000D for ; Thu, 23 Sep 2021 00:14:02 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id A737B404C7 for ; Thu, 23 Sep 2021 00:14:02 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zvLJzKBSxzyj for ; Thu, 23 Sep 2021 00:13:59 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by smtp2.osuosl.org (Postfix) with ESMTPS id 24E4340138 for ; Thu, 23 Sep 2021 00:13:58 +0000 (UTC) Received: (Authenticated sender: i.maximets@ovn.org) by relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 16118240005; Thu, 23 Sep 2021 00:13:54 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Thu, 23 Sep 2021 02:13:53 +0200 Message-Id: <20210923001353.4072533-1-i.maximets@ovn.org> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 Cc: Ilya Maximets , Dumitru Ceara Subject: [ovs-dev] [PATCH] ovsdb-data: Deduplicate string atoms. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" ovsdb-server spends a lot of time cloning atoms for various reasons, e.g. to create a diff of two rows or to clone a row to the transaction. All atoms, except for strings, contains a simple value that could be copied in efficient way, but duplicating strings every time has a significant performance impact. Introducing a new reference-counted structure 'ovsdb_atom_string' that allows to not copy strings every time, but just increase a reference counter. Changes in ovsdb/ovsdb-idlc.in are a bit brute-force-like, but I didn't manage to write anything better. And this part is very hard to understand and debug, so maybe it's better to keep it in this more or less understandable shape. This change allows to increase transaction throughput in benchmarks up to 2x for standalone databases and 3x for clustered databases, i.e. number of transactions that ovsdb-server can handle per second. It also noticeably reduces memory consumption of ovsdb-server. Next step will be to consolidate this structure with json strings, so we will not need to duplicate strings while converting database objects to json and back. Signed-off-by: Ilya Maximets Acked-by: Dumitru Ceara Acked-by: Mark D. Gray --- There is a small conflict in lib/db-ctl-base.c with the 'set optimizations' patch-set, but it can be easily resolved if someone wants to try these patches together. lib/db-ctl-base.c | 11 +-- lib/ovsdb-cs.c | 2 +- lib/ovsdb-data.c | 45 ++++++----- lib/ovsdb-data.h | 29 ++++++- lib/ovsdb-idl.c | 3 +- ovsdb/ovsdb-idlc.in | 179 ++++++++++++++++++++++++++++------------- ovsdb/ovsdb-server.c | 6 +- ovsdb/ovsdb-util.c | 16 ++-- ovsdb/rbac.c | 8 +- python/ovs/db/data.py | 43 +++++++--- python/ovs/db/types.py | 7 +- tests/test-ovsdb.c | 14 ++-- 12 files changed, 244 insertions(+), 119 deletions(-) diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c index 77cc76a9f..713487797 100644 --- a/lib/db-ctl-base.c +++ b/lib/db-ctl-base.c @@ -247,15 +247,15 @@ record_id_equals(const union ovsdb_atom *name, enum ovsdb_atomic_type type, const char *record_id) { if (type == OVSDB_TYPE_STRING) { - if (!strcmp(name->string, record_id)) { + if (!strcmp(name->s->string, record_id)) { return true; } struct uuid uuid; size_t len = strlen(record_id); if (len >= 4 - && uuid_from_string(&uuid, name->string) - && !strncmp(name->string, record_id, len)) { + && uuid_from_string(&uuid, name->s->string) + && !strncmp(name->s->string, record_id, len)) { return true; } @@ -318,11 +318,12 @@ get_row_by_id(struct ctl_context *ctx, if (!id->key) { name = datum->n == 1 ? &datum->keys[0] : NULL; } else { - const union ovsdb_atom key_atom - = { .string = CONST_CAST(char *, id->key) }; + union ovsdb_atom key_atom = { + .s = ovsdb_atom_string_create(CONST_CAST(char *, id->key)) }; unsigned int i = ovsdb_datum_find_key(datum, &key_atom, OVSDB_TYPE_STRING); name = i == UINT_MAX ? NULL : &datum->values[i]; + ovsdb_atom_destroy(&key_atom, OVSDB_TYPE_STRING); } if (!name) { continue; diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c index 659d49dbf..fcb6fe1b3 100644 --- a/lib/ovsdb-cs.c +++ b/lib/ovsdb-cs.c @@ -1833,7 +1833,7 @@ server_column_get_string(const struct server_row *row, { ovs_assert(server_columns[index].type.key.type == OVSDB_TYPE_STRING); const struct ovsdb_datum *d = &row->data[index]; - return d->n == 1 ? d->keys[0].string : default_value; + return d->n == 1 ? d->keys[0].s->string : default_value; } static bool diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c index 1f491a98b..0ed8f01b6 100644 --- a/lib/ovsdb-data.c +++ b/lib/ovsdb-data.c @@ -74,7 +74,7 @@ ovsdb_atom_init_default(union ovsdb_atom *atom, enum ovsdb_atomic_type type) break; case OVSDB_TYPE_STRING: - atom->string = xmemdup("", 1); + atom->s = ovsdb_atom_string_create_nocopy(xmemdup("", 1)); break; case OVSDB_TYPE_UUID: @@ -136,7 +136,7 @@ ovsdb_atom_is_default(const union ovsdb_atom *atom, return atom->boolean == false; case OVSDB_TYPE_STRING: - return atom->string[0] == '\0'; + return atom->s->string[0] == '\0'; case OVSDB_TYPE_UUID: return uuid_is_zero(&atom->uuid); @@ -172,7 +172,8 @@ ovsdb_atom_clone(union ovsdb_atom *new, const union ovsdb_atom *old, break; case OVSDB_TYPE_STRING: - new->string = xstrdup(old->string); + new->s = old->s; + new->s->n_refs++; break; case OVSDB_TYPE_UUID: @@ -214,7 +215,7 @@ ovsdb_atom_hash(const union ovsdb_atom *atom, enum ovsdb_atomic_type type, return hash_boolean(atom->boolean, basis); case OVSDB_TYPE_STRING: - return hash_string(atom->string, basis); + return hash_string(atom->s->string, basis); case OVSDB_TYPE_UUID: return hash_int(uuid_hash(&atom->uuid), basis); @@ -246,7 +247,7 @@ ovsdb_atom_compare_3way(const union ovsdb_atom *a, return a->boolean - b->boolean; case OVSDB_TYPE_STRING: - return strcmp(a->string, b->string); + return a->s == b->s ? 0 : strcmp(a->s->string, b->s->string); case OVSDB_TYPE_UUID: return uuid_compare_3way(&a->uuid, &b->uuid); @@ -404,7 +405,7 @@ ovsdb_atom_from_json__(union ovsdb_atom *atom, case OVSDB_TYPE_STRING: if (json->type == JSON_STRING) { - atom->string = xstrdup(json->string); + atom->s = ovsdb_atom_string_create(json->string); return NULL; } break; @@ -473,7 +474,7 @@ ovsdb_atom_to_json(const union ovsdb_atom *atom, enum ovsdb_atomic_type type) return json_boolean_create(atom->boolean); case OVSDB_TYPE_STRING: - return json_string_create(atom->string); + return json_string_create(atom->s->string); case OVSDB_TYPE_UUID: return wrap_json("uuid", json_string_create_nocopy( @@ -551,14 +552,18 @@ ovsdb_atom_from_string__(union ovsdb_atom *atom, if (s_len < 2 || s[s_len - 1] != '"') { return xasprintf("%s: missing quote at end of " "quoted string", s); - } else if (!json_string_unescape(s + 1, s_len - 2, - &atom->string)) { - char *error = xasprintf("%s: %s", s, atom->string); - free(atom->string); - return error; + } else { + char *res; + if (json_string_unescape(s + 1, s_len - 2, &res)) { + atom->s = ovsdb_atom_string_create_nocopy(res); + } else { + char *error = xasprintf("%s: %s", s, res); + free(res); + return error; + } } } else { - atom->string = xstrdup(s); + atom->s = ovsdb_atom_string_create(s); } break; @@ -721,14 +726,14 @@ ovsdb_atom_to_string(const union ovsdb_atom *atom, enum ovsdb_atomic_type type, break; case OVSDB_TYPE_STRING: - if (string_needs_quotes(atom->string)) { + if (string_needs_quotes(atom->s->string)) { struct json json; json.type = JSON_STRING; - json.string = atom->string; + json.string = atom->s->string; json_to_ds(&json, 0, out); } else { - ds_put_cstr(out, atom->string); + ds_put_cstr(out, atom->s->string); } break; @@ -750,7 +755,7 @@ ovsdb_atom_to_bare(const union ovsdb_atom *atom, enum ovsdb_atomic_type type, struct ds *out) { if (type == OVSDB_TYPE_STRING) { - ds_put_cstr(out, atom->string); + ds_put_cstr(out, atom->s->string); } else { ovsdb_atom_to_string(atom, type, out); } @@ -877,7 +882,7 @@ ovsdb_atom_check_constraints(const union ovsdb_atom *atom, return NULL; case OVSDB_TYPE_STRING: - return check_string_constraints(atom->string, &base->string); + return check_string_constraints(atom->s->string, &base->string); case OVSDB_TYPE_UUID: return NULL; @@ -1691,8 +1696,8 @@ ovsdb_datum_from_smap(struct ovsdb_datum *datum, const struct smap *smap) struct smap_node *node; size_t i = 0; SMAP_FOR_EACH (node, smap) { - datum->keys[i].string = xstrdup(node->key); - datum->values[i].string = xstrdup(node->value); + datum->keys[i].s = ovsdb_atom_string_create(node->key); + datum->values[i].s = ovsdb_atom_string_create(node->value); i++; } ovs_assert(i == datum->n); diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h index aa035ebad..fa7e0d2fc 100644 --- a/lib/ovsdb-data.h +++ b/lib/ovsdb-data.h @@ -20,6 +20,7 @@ #include "compiler.h" #include "ovsdb-types.h" #include "openvswitch/shash.h" +#include "util.h" #ifdef __cplusplus extern "C" { @@ -31,12 +32,33 @@ struct ds; struct ovsdb_symbol_table; struct smap; +struct ovsdb_atom_string { + char *string; + int n_refs; +}; + +static inline struct ovsdb_atom_string * +ovsdb_atom_string_create_nocopy(char *str) +{ + struct ovsdb_atom_string *s = xzalloc(sizeof *s); + + s->string = str; + s->n_refs = 1; + return s; +} + +static inline struct ovsdb_atom_string * +ovsdb_atom_string_create(const char *str) +{ + return ovsdb_atom_string_create_nocopy(xstrdup(str)); +} + /* One value of an atomic type (given by enum ovs_atomic_type). */ union ovsdb_atom { int64_t integer; double real; bool boolean; - char *string; + struct ovsdb_atom_string *s; struct uuid uuid; }; @@ -66,8 +88,9 @@ ovsdb_atom_needs_destruction(enum ovsdb_atomic_type type) static inline void ovsdb_atom_destroy(union ovsdb_atom *atom, enum ovsdb_atomic_type type) { - if (type == OVSDB_TYPE_STRING) { - free(atom->string); + if (type == OVSDB_TYPE_STRING && !--atom->s->n_refs) { + free(atom->s->string); + free(atom->s); } } diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index d3caccec8..40125f234 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -1951,8 +1951,7 @@ ovsdb_idl_index_destroy_row(const struct ovsdb_idl_row *row_) BITMAP_FOR_EACH_1 (i, class->n_columns, row->written) { c = &class->columns[i]; (c->unparse) (row); - free(row->new_datum[i].values); - free(row->new_datum[i].keys); + ovsdb_datum_destroy(&row->new_datum[i], &c->type); } free(row->new_datum); free(row->written); diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in index f19e92915..877f903cb 100755 --- a/ovsdb/ovsdb-idlc.in +++ b/ovsdb/ovsdb-idlc.in @@ -577,20 +577,20 @@ static void print(" smap_init(&row->%s);" % columnName) print(" for (size_t i = 0; i < datum->n; i++) {") print(" smap_add(&row->%s," % columnName) - print(" datum->keys[i].string,") - print(" datum->values[i].string);") + print(" datum->keys[i].s->string,") + print(" datum->values[i].s->string);") print(" }") elif (type.n_min == 1 and type.n_max == 1) or type.is_optional_pointer(): print("") print(" if (datum->n >= 1) {") if not type.key.ref_table: - print(" %s = datum->keys[0].%s;" % (keyVar, type.key.type.to_string())) + print(" %s = datum->keys[0].%s;" % (keyVar, type.key.type.to_Cvalue_string())) else: print(" %s = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->keys[0].uuid));" % (keyVar, prefix, type.key.ref_table.name.lower(), prefix, type.key.ref_table.name.lower())) if valueVar: if not type.value.ref_table: - print(" %s = datum->values[0].%s;" % (valueVar, type.value.type.to_string())) + print(" %s = datum->values[0].%s;" % (valueVar, type.value.type.to_Cvalue_string())) else: print(" %s = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->values[0].uuid));" % (valueVar, prefix, type.value.ref_table.name.lower(), prefix, type.value.ref_table.name.lower())) print(" } else {") @@ -618,7 +618,7 @@ static void """ % (prefix, type.key.ref_table.name.lower(), prefix, type.key.ref_table.name.lower(), prefix, type.key.ref_table.name.lower())) keySrc = "keyRow" else: - keySrc = "datum->keys[i].%s" % type.key.type.to_string() + keySrc = "datum->keys[i].%s" % type.key.type.to_Cvalue_string() if type.value and type.value.ref_table: print("""\ struct %s%s *valueRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->values[i].uuid)); @@ -628,7 +628,7 @@ static void """ % (prefix, type.value.ref_table.name.lower(), prefix, type.value.ref_table.name.lower(), prefix, type.value.ref_table.name.lower())) valueSrc = "valueRow" elif valueVar: - valueSrc = "datum->values[i].%s" % type.value.type.to_string() + valueSrc = "datum->values[i].%s" % type.value.type.to_Cvalue_string() print(" if (!row->n_%s) {" % (columnName)) print(" %s = xmalloc(%s * sizeof *%s);" % ( @@ -936,45 +936,57 @@ void 'args': ', '.join(['%(type)s%(name)s' % m for m in members])}) if type.n_min == 1 and type.n_max == 1: - print(" union ovsdb_atom key;") + print(" union ovsdb_atom *key = xmalloc(sizeof *key);") if type.value: - print(" union ovsdb_atom value;") + print(" union ovsdb_atom *value = xmalloc(sizeof *value);") print("") print(" datum.n = 1;") - print(" datum.keys = &key;") - print(" " + type.key.assign_c_value_casting_away_const("key.%s" % type.key.type.to_string(), keyVar)) + print(" datum.keys = key;") + if type.key.type == StringType: + print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") + else: + print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar)) if type.value: - print(" datum.values = &value;") - print(" "+ type.value.assign_c_value_casting_away_const("value.%s" % type.value.type.to_string(), valueVar)) + print(" datum.values = value;") + if type.value.type == StringType: + print(" value->s = ovsdb_atom_string_create(" + valueVar + ");") + else: + print(" "+ type.value.assign_c_value_casting_away_const("value->%s" % type.value.type.to_string(), valueVar)) else: print(" datum.values = NULL;") - txn_write_func = "ovsdb_idl_txn_write_clone" + txn_write_func = "ovsdb_idl_txn_write" elif type.is_optional_pointer(): - print(" union ovsdb_atom key;") print("") print(" if (%s) {" % keyVar) + print(" union ovsdb_atom *key = xmalloc(sizeof *key);") print(" datum.n = 1;") - print(" datum.keys = &key;") - print(" " + type.key.assign_c_value_casting_away_const("key.%s" % type.key.type.to_string(), keyVar)) + print(" datum.keys = key;") + if type.key.type == StringType: + print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") + else: + print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") print(" }") print(" datum.values = NULL;") - txn_write_func = "ovsdb_idl_txn_write_clone" + txn_write_func = "ovsdb_idl_txn_write" elif type.n_max == 1: - print(" union ovsdb_atom key;") print("") print(" if (%s) {" % nVar) + print(" union ovsdb_atom *key = xmalloc(sizeof *key);") print(" datum.n = 1;") - print(" datum.keys = &key;") - print(" " + type.key.assign_c_value_casting_away_const("key.%s" % type.key.type.to_string(), "*" + keyVar)) + print(" datum.keys = key;") + if type.key.type == StringType: + print(" key->s = ovsdb_atom_string_create(*" + keyVar + ");") + else: + print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), "*" + keyVar)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") print(" }") print(" datum.values = NULL;") - txn_write_func = "ovsdb_idl_txn_write_clone" + txn_write_func = "ovsdb_idl_txn_write" else: print("") print(" datum.n = %s;" % nVar) @@ -984,9 +996,15 @@ void else: print(" datum.values = NULL;") print(" for (size_t i = 0; i < %s; i++) {" % nVar) - print(" " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar)) + if type.key.type == StringType: + print(" datum.keys[i].s = ovsdb_atom_string_create(" + keyVar + "[i]);") + else: + print(" " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar)) if type.value: - print(" " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar)) + if type.value.type == StringType: + print(" datum.values[i].s = ovsdb_atom_string_create(" + valueVar + "[i]);") + else: + print(" " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar)) print(" }") if type.value: valueType = type.value.toAtomicType() @@ -1022,9 +1040,14 @@ void ''' % {'s': structName, 'c': columnName,'coltype':column.type.key.to_const_c_type(prefix), 'valtype':column.type.value.to_const_c_type(prefix), 'S': structName.upper(), 'C': columnName.upper(), 't': tableName}) - - print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "new_key")) - print(" "+ type.value.copyCValue("datum->values[0].%s" % type.value.type.to_string(), "new_value")) + if type.key.type == StringType: + print(" datum->keys[0].s = ovsdb_atom_string_create(new_key);") + else: + print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "new_key")) + if type.value.type == StringType: + print(" datum->values[0].s = ovsdb_atom_string_create(new_value);") + else: + print(" "+ type.value.copyCValue("datum->values[0].%s" % type.value.type.to_string(), "new_value")) print(''' ovsdb_idl_txn_write_partial_map(&row->header_, &%(s)s_col_%(c)s, @@ -1048,8 +1071,10 @@ void ''' % {'s': structName, 'c': columnName,'coltype':column.type.key.to_const_c_type(prefix), 'valtype':column.type.value.to_const_c_type(prefix), 'S': structName.upper(), 'C': columnName.upper(), 't': tableName}) - - print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "delete_key")) + if type.key.type == StringType: + print(" datum->keys[0].s = ovsdb_atom_string_create(delete_key);") + else: + print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "delete_key")) print(''' ovsdb_idl_txn_delete_partial_map(&row->header_, &%(s)s_col_%(c)s, @@ -1075,8 +1100,10 @@ void datum->values = NULL; ''' % {'s': structName, 'c': columnName, 'valtype':column.type.key.to_const_c_type(prefix), 't': tableName}) - - print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "new_value")) + if type.key.type == StringType: + print(" datum->keys[0].s = ovsdb_atom_string_create(new_value);") + else: + print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "new_value")) print(''' ovsdb_idl_txn_write_partial_set(&row->header_, &%(s)s_col_%(c)s, @@ -1100,8 +1127,10 @@ void ''' % {'s': structName, 'c': columnName,'coltype':column.type.key.to_const_c_type(prefix), 'valtype':column.type.key.to_const_c_type(prefix), 'S': structName.upper(), 'C': columnName.upper(), 't': tableName}) - - print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "delete_value")) + if type.key.type == StringType: + print(" datum->keys[0].s = ovsdb_atom_string_create(delete_value);") + else: + print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "delete_value")) print(''' ovsdb_idl_txn_delete_partial_set(&row->header_, &%(s)s_col_%(c)s, @@ -1169,37 +1198,49 @@ void print(" struct ovsdb_datum datum;") free = [] if type.n_min == 1 and type.n_max == 1: - print(" union ovsdb_atom key;") + print(" union ovsdb_atom *key = xmalloc(sizeof *key);") if type.value: - print(" union ovsdb_atom value;") + print(" union ovsdb_atom *value = xmalloc(sizeof *value);") print("") print(" datum.n = 1;") - print(" datum.keys = &key;") - print(" " + type.key.assign_c_value_casting_away_const("key.%s" % type.key.type.to_string(), keyVar, refTable=False)) + print(" datum.keys = key;") + if type.key.type == StringType: + print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") + else: + print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar, refTable=False)) if type.value: - print(" datum.values = &value;") - print(" "+ type.value.assign_c_value_casting_away_const("value.%s" % type.value.type.to_string(), valueVar, refTable=False)) + print(" datum.values = value;") + if type.value.type == StringType: + print(" value->s = ovsdb_atom_string_create(" + valueVar + ");") + else: + print(" "+ type.value.assign_c_value_casting_away_const("value.%s" % type.value.type.to_string(), valueVar, refTable=False)) else: print(" datum.values = NULL;") elif type.is_optional_pointer(): - print(" union ovsdb_atom key;") print("") print(" if (%s) {" % keyVar) + print(" union ovsdb_atom *key = xmalloc(sizeof *key);") print(" datum.n = 1;") - print(" datum.keys = &key;") - print(" " + type.key.assign_c_value_casting_away_const("key.%s" % type.key.type.to_string(), keyVar, refTable=False)) + print(" datum.keys = key;") + if type.key.type == StringType: + print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") + else: + print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar, refTable=False)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") print(" }") print(" datum.values = NULL;") elif type.n_max == 1: - print(" union ovsdb_atom key;") print("") print(" if (%s) {" % nVar) + print(" union ovsdb_atom *key = xmalloc(sizeof *key);") print(" datum.n = 1;") - print(" datum.keys = &key;") - print(" " + type.key.assign_c_value_casting_away_const("key.%s" % type.key.type.to_string(), "*" + keyVar, refTable=False)) + print(" datum.keys = key;") + if type.key.type == StringType: + print(" key->s = ovsdb_atom_string_create(*" + keyVar + ");") + else: + print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), "*" + keyVar, refTable=False)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") @@ -1208,16 +1249,20 @@ void else: print(" datum.n = %s;" % nVar) print(" datum.keys = %s ? xmalloc(%s * sizeof *datum.keys) : NULL;" % (nVar, nVar)) - free += ['datum.keys'] if type.value: print(" datum.values = xmalloc(%s * sizeof *datum.values);" % nVar) - free += ['datum.values'] else: print(" datum.values = NULL;") print(" for (size_t i = 0; i < %s; i++) {" % nVar) - print(" " + type.key.assign_c_value_casting_away_const("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar, refTable=False)) + if type.key.type == StringType: + print(" datum.keys[i].s = ovsdb_atom_string_create(" + keyVar + "[i]);") + else: + print(" " + type.key.assign_c_value_casting_away_const("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar, refTable=False)) if type.value: - print(" " + type.value.assign_c_value_casting_away_const("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar, refTable=False)) + if type.value.type == StringType: + print(" datum.values[i].s = ovsdb_atom_string_create(" + valueVar + "[i]);") + else: + print(" " + type.value.assign_c_value_casting_away_const("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar, refTable=False)) print(" }") if type.value: valueType = type.value.toAtomicType() @@ -1237,8 +1282,8 @@ void 's': structName, 'S': structName.upper(), 'c': columnName}) - for var in free: - print(" free(%s);" % var) + print(" ovsdb_datum_destroy(&datum, &%(s)s_col_%(c)s.type);" \ + % {'s': structName, 'c': columnName}) print("}") # Index table related functions @@ -1335,8 +1380,8 @@ struct %(s)s * i = 0; SMAP_FOR_EACH (node, %(c)s) { - datum->keys[i].string = node->key; - datum->values[i].string = node->value; + datum->keys[i].s = ovsdb_atom_string_create(node->key); + datum->values[i].s = ovsdb_atom_string_create(node->value); i++; } ovsdb_datum_sort_unique(datum, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING); @@ -1385,10 +1430,16 @@ struct %(s)s * print() print(" datum.n = 1;") print(" datum.keys = key;") - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar)) + if type.key.type == StringType: + print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") + else: + print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar)) if type.value: print(" datum.values = value;") - print(" "+ type.value.assign_c_value_casting_away_const("value->%s" % type.value.type.to_string(), valueVar)) + if type.value.type == StringType: + print(" value->s = ovsdb_atom_string_create(" + valueVar + ");") + else: + print(" " + type.value.assign_c_value_casting_away_const("value->%s" % type.value.type.to_string(), valueVar)) else: print(" datum.values = NULL;") txn_write_func = "ovsdb_idl_index_write" @@ -1399,7 +1450,10 @@ struct %(s)s * print(" key = xmalloc(sizeof (union ovsdb_atom));") print(" datum.n = 1;") print(" datum.keys = key;") - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar)) + if type.key.type == StringType: + print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") + else: + print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") @@ -1413,7 +1467,10 @@ struct %(s)s * print(" key = xmalloc(sizeof(union ovsdb_atom));") print(" datum.n = 1;") print(" datum.keys = key;") - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), "*" + keyVar)) + if type.key.type == StringType: + print(" key->s = ovsdb_atom_string_create(*" + keyVar + ");") + else: + print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), "*" + keyVar)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") @@ -1430,9 +1487,15 @@ struct %(s)s * else: print(" datum.values = NULL;") print(" for (i = 0; i < %s; i++) {" % nVar) - print(" " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar)) + if type.key.type == StringType: + print(" datum.keys[i].s = ovsdb_atom_string_create(" + keyVar + "[i]);") + else: + print(" " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar)) if type.value: - print(" " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar)) + if type.value.type == StringType: + print(" datum.values[i].s = ovsdb_atom_string_create(" + valueVar + "[i]);") + else: + print(" " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar)) print(" }") if type.value: valueType = type.value.toAtomicType() diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c index 0b3d2bb71..b34d97e29 100644 --- a/ovsdb/ovsdb-server.c +++ b/ovsdb/ovsdb-server.c @@ -904,8 +904,8 @@ query_db_string(const struct shash *all_dbs, const char *name, datum = &row->fields[column->index]; for (i = 0; i < datum->n; i++) { - if (datum->keys[i].string[0]) { - return datum->keys[i].string; + if (datum->keys[i].s->string[0]) { + return datum->keys[i].s->string; } } } @@ -1018,7 +1018,7 @@ query_db_remotes(const char *name, const struct shash *all_dbs, datum = &row->fields[column->index]; for (i = 0; i < datum->n; i++) { - add_remote(remotes, datum->keys[i].string); + add_remote(remotes, datum->keys[i].s->string); } } } else if (column->type.key.type == OVSDB_TYPE_UUID diff --git a/ovsdb/ovsdb-util.c b/ovsdb/ovsdb-util.c index c4075cdae..6d7be066b 100644 --- a/ovsdb/ovsdb-util.c +++ b/ovsdb/ovsdb-util.c @@ -111,13 +111,13 @@ ovsdb_util_read_map_string_column(const struct ovsdb_row *row, for (i = 0; i < datum->n; i++) { atom_key = &datum->keys[i]; - if (!strcmp(atom_key->string, key)) { + if (!strcmp(atom_key->s->string, key)) { atom_value = &datum->values[i]; break; } } - return atom_value ? atom_value->string : NULL; + return atom_value ? atom_value->s->string : NULL; } /* Read string-uuid key-values from a map. Returns the row associated with @@ -143,7 +143,7 @@ ovsdb_util_read_map_string_uuid_column(const struct ovsdb_row *row, const struct ovsdb_datum *datum = &row->fields[column->index]; for (size_t i = 0; i < datum->n; i++) { union ovsdb_atom *atom_key = &datum->keys[i]; - if (!strcmp(atom_key->string, key)) { + if (!strcmp(atom_key->s->string, key)) { const union ovsdb_atom *atom_value = &datum->values[i]; return ovsdb_table_get_row(ref_table, &atom_value->uuid); } @@ -181,7 +181,7 @@ ovsdb_util_read_string_column(const struct ovsdb_row *row, const union ovsdb_atom *atom; atom = ovsdb_util_read_column(row, column_name, OVSDB_TYPE_STRING); - *stringp = atom ? atom->string : NULL; + *stringp = atom ? atom->s->string : NULL; return atom != NULL; } @@ -269,8 +269,10 @@ ovsdb_util_write_string_column(struct ovsdb_row *row, const char *column_name, const char *string) { if (string) { - const union ovsdb_atom atom = { .string = CONST_CAST(char *, string) }; + union ovsdb_atom atom = { + .s = ovsdb_atom_string_create(CONST_CAST(char *, string)) }; ovsdb_util_write_singleton(row, column_name, &atom, OVSDB_TYPE_STRING); + ovsdb_atom_destroy(&atom, OVSDB_TYPE_STRING); } else { ovsdb_util_clear_column(row, column_name); } @@ -305,8 +307,8 @@ ovsdb_util_write_string_string_column(struct ovsdb_row *row, datum->values = xmalloc(n * sizeof *datum->values); for (i = 0; i < n; ++i) { - datum->keys[i].string = keys[i]; - datum->values[i].string = values[i]; + datum->keys[i].s = ovsdb_atom_string_create_nocopy(keys[i]); + datum->values[i].s = ovsdb_atom_string_create_nocopy(values[i]); } /* Sort and check constraints. */ diff --git a/ovsdb/rbac.c b/ovsdb/rbac.c index 2986027c9..ff411675f 100644 --- a/ovsdb/rbac.c +++ b/ovsdb/rbac.c @@ -53,8 +53,8 @@ ovsdb_find_row_by_string_key(const struct ovsdb_table *table, HMAP_FOR_EACH (row, hmap_node, &table->rows) { const struct ovsdb_datum *datum = &row->fields[column->index]; for (size_t i = 0; i < datum->n; i++) { - if (datum->keys[i].string[0] && - !strcmp(key, datum->keys[i].string)) { + if (datum->keys[i].s->string[0] && + !strcmp(key, datum->keys[i].s->string)) { return row; } } @@ -113,7 +113,7 @@ ovsdb_rbac_authorized(const struct ovsdb_row *perms, } for (i = 0; i < datum->n; i++) { - const char *name = datum->keys[i].string; + const char *name = datum->keys[i].s->string; const char *value = NULL; bool is_map; @@ -271,7 +271,7 @@ rbac_column_modification_permitted(const struct ovsdb_column *column, size_t i; for (i = 0; i < modifiable->n; i++) { - char *name = modifiable->keys[i].string; + char *name = modifiable->keys[i].s->string; if (!strcmp(name, column->name)) { return true; diff --git a/python/ovs/db/data.py b/python/ovs/db/data.py index 2a2102d6b..99bf80ed6 100644 --- a/python/ovs/db/data.py +++ b/python/ovs/db/data.py @@ -204,7 +204,7 @@ class Atom(object): else: return '.boolean = false' elif self.type == ovs.db.types.StringType: - return '.string = "%s"' % escapeCString(self.value) + return '.s = %s' % escapeCString(self.value) elif self.type == ovs.db.types.UuidType: return '.uuid = %s' % ovs.ovsuuid.to_c_assignment(self.value) @@ -563,16 +563,41 @@ class Datum(object): if n == 0: return ["static struct ovsdb_datum %s = { .n = 0 };"] - s = ["static union ovsdb_atom %s_keys[%d] = {" % (name, n)] - for key in sorted(self.values): - s += [" { %s }," % key.cInitAtom(key)] - s += ["};"] + s = [] + if self.type.key.type == ovs.db.types.StringType: + s += ["static struct ovsdb_atom_string %s_key_strings[%d] = {" + % (name, n)] + for key in sorted(self.values): + s += [' { .string = "%s", .n_refs = 2 },' + % escapeCString(key.value)] + s += ["};"] + s += ["static union ovsdb_atom %s_keys[%d] = {" % (name, n)] + for i in range(n): + s += [" { .s = &%s_key_strings[%d] }," % (name, i)] + s += ["};"] + else: + s = ["static union ovsdb_atom %s_keys[%d] = {" % (name, n)] + for key in sorted(self.values): + s += [" { %s }," % key.cInitAtom(key)] + s += ["};"] if self.type.value: - s = ["static union ovsdb_atom %s_values[%d] = {" % (name, n)] - for k, v in sorted(self.values.items()): - s += [" { %s }," % v.cInitAtom(v)] - s += ["};"] + if self.type.value.type == ovs.db.types.StringType: + s += ["static struct ovsdb_atom_string %s_val_strings[%d] = {" + % (name, n)] + for k, v in sorted(self.values): + s += [' { .string = "%s", .n_refs = 2 },' + % escapeCString(v.value)] + s += ["};"] + s += ["static union ovsdb_atom %s_values[%d] = {" % (name, n)] + for i in range(n): + s += [" { .s = &%s_val_strings[%d] }," % (name, i)] + s += ["};"] + else: + s = ["static union ovsdb_atom %s_values[%d] = {" % (name, n)] + for k, v in sorted(self.values.items()): + s += [" { %s }," % v.cInitAtom(v)] + s += ["};"] s += ["static struct ovsdb_datum %s = {" % name] s += [" .n = %d," % n] diff --git a/python/ovs/db/types.py b/python/ovs/db/types.py index 626ae8fc4..18485f701 100644 --- a/python/ovs/db/types.py +++ b/python/ovs/db/types.py @@ -48,6 +48,11 @@ class AtomicType(object): def to_string(self): return self.name + def to_Cvalue_string(self): + if self == StringType: + return 's->' + self.name + return self.name + def to_json(self): return self.name @@ -373,7 +378,7 @@ class BaseType(object): return "%(dst)s = *%(src)s;" % args return ("%(dst)s = %(src)s->header_.uuid;") % args elif self.type == StringType: - return "%(dst)s = xstrdup(%(src)s);" % args + return "%(dst)s = ovsdb_atom_string_create(%(src)s);" % args else: return "%(dst)s = %(src)s;" % args diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c index 02485b136..c3ce4f361 100644 --- a/tests/test-ovsdb.c +++ b/tests/test-ovsdb.c @@ -2727,13 +2727,15 @@ print_idl_row_simple2(const struct idltest_simple2 *s, int step) printf("%03d: name=%s smap=[", step, s->name); for (i = 0; i < smap->n; i++) { - printf("[%s : %s]%s", smap->keys[i].string, smap->values[i].string, - i < smap->n-1? ",": ""); + printf("[%s : %s]%s", + smap->keys[i].s->string, smap->values[i].s->string, + i < smap->n - 1 ? "," : ""); } printf("] imap=["); for (i = 0; i < imap->n; i++) { - printf("[%"PRId64" : %s]%s", imap->keys[i].integer, imap->values[i].string, - i < imap->n-1? ",":""); + printf("[%"PRId64" : %s]%s", + imap->keys[i].integer, imap->values[i].s->string, + i < imap->n - 1 ? "," : ""); } printf("]\n"); } @@ -2802,8 +2804,8 @@ do_idl_partial_update_map_column(struct ovs_cmdl_context *ctx) myTxn = ovsdb_idl_txn_create(idl); smap = idltest_simple2_get_smap(myRow, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING); - strcpy(key_to_delete, smap->keys[0].string); - idltest_simple2_update_smap_delkey(myRow, smap->keys[0].string); + ovs_strlcpy(key_to_delete, smap->keys[0].s->string, sizeof key_to_delete); + idltest_simple2_update_smap_delkey(myRow, smap->keys[0].s->string); ovsdb_idl_txn_commit_block(myTxn); ovsdb_idl_txn_destroy(myTxn); ovsdb_idl_get_initial_snapshot(idl);