On 23/09/2021 01:13, Ilya Maximets wrote:
> 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.
>
Great!
> 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 <i.maxim...@ovn.org>
> ---
>
> 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';
perhaps a helper function would allow you to hide some of this from a
client. e.g.
char *ovsdb_atom_string_get(struct ovsdb_atom)
>
> 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);
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
