[ovs-dev] [mirror names 05/10] ovsdb-idl: Transition to better interfaces for reading table columns.
Justin Pettit
jpettit at nicira.com
Fri Jul 2 23:24:32 UTC 2010
Looks good.
--Justin
On Jun 17, 2010, at 3:57 PM, Ben Pfaff wrote:
> The existing ovsdb_idl_txn_read() was somewhat difficult and expensive to
> use, because it always made a copy of the data in the column. This was
> necessary at the time it was introduced, because there was no way for it
> to return a "default" value for columns that had not yet been populated
> without allocating data and hence requiring the caller to free it.
>
> Now that ovsdb_datum_default() exists, this is no longer required. This
> commit introduces a pair of new functions, ovsdb_idl_read() and
> ovsdb_idl_get(), that return a pointer to existing data and do not do any
> copying. It also transitions all of ovsdb_idl_txn_read()'s callers to
> the new interfaces.
> ---
> lib/ovsdb-idl.c | 67 +++++++++++++++++++++++++++++++++++-------------
> lib/ovsdb-idl.h | 11 ++++++--
> ovsdb/OVSDB.py | 3 ++
> utilities/ovs-vsctl.c | 63 +++++++++++++++++++---------------------------
> 4 files changed, 86 insertions(+), 58 deletions(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index ca77cc2..6ad8086 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -865,6 +865,55 @@ ovsdb_idl_next_row(const struct ovsdb_idl_row *row)
>
> return next_real_row(table, hmap_next(&table->rows, &row->hmap_node));
> }
> +
> +/* Reads and returns the value of 'column' within 'row'. If an ongoing
> + * transaction has changed 'column''s value, the modified value is returned.
> + *
> + * The caller must not modify or free the returned value.
> + *
> + * Various kinds of changes can invalidate the returned value: writing to the
> + * same 'column' in 'row' (e.g. with ovsdb_idl_txn_write()), deleting 'row'
> + * (e.g. with ovsdb_idl_txn_delete()), or completing an ongoing transaction
> + * (e.g. with ovsdb_idl_txn_commit() or ovsdb_idl_txn_abort()). If the
> + * returned value is needed for a long time, it is best to make a copy of it
> + * with ovsdb_datum_clone(). */
> +const struct ovsdb_datum *
> +ovsdb_idl_read(const struct ovsdb_idl_row *row,
> + const struct ovsdb_idl_column *column)
> +{
> + const struct ovsdb_idl_table_class *class = row->table->class;
> + size_t column_idx = column - class->columns;
> +
> + assert(row->new != NULL);
> + assert(column_idx < class->n_columns);
> +
> + if (row->written && bitmap_is_set(row->written, column_idx)) {
> + return &row->new[column_idx];
> + } else if (row->old) {
> + return &row->old[column_idx];
> + } else {
> + return ovsdb_datum_default(&column->type);
> + }
> +}
> +
> +/* Same as ovsdb_idl_read(), except that it also asserts that 'column' has key
> + * type 'key_type' and value type 'value_type'. (Scalar and set types will
> + * have a value type of OVSDB_TYPE_VOID.)
> + *
> + * This is useful in code that "knows" that a particular column has a given
> + * type, so that it will abort if someone changes the column's type without
> + * updating the code that uses it. */
> +const struct ovsdb_datum *
> +ovsdb_idl_get(const struct ovsdb_idl_row *row,
> + const struct ovsdb_idl_column *column,
> + enum ovsdb_atomic_type key_type OVS_UNUSED,
> + enum ovsdb_atomic_type value_type OVS_UNUSED)
> +{
> + assert(column->type.key.type == key_type);
> + assert(column->type.value.type == value_type);
> +
> + return ovsdb_idl_read(row, column);
> +}
>
> /* Transactions. */
>
> @@ -1363,24 +1412,6 @@ ovsdb_idl_txn_complete(struct ovsdb_idl_txn *txn,
> }
>
> void
> -ovsdb_idl_txn_read(const struct ovsdb_idl_row *row,
> - const struct ovsdb_idl_column *column,
> - struct ovsdb_datum *datum)
> -{
> - const struct ovsdb_idl_table_class *class = row->table->class;
> - size_t column_idx = column - class->columns;
> -
> - assert(row->new != NULL);
> - if (row->written && bitmap_is_set(row->written, column_idx)) {
> - ovsdb_datum_clone(datum, &row->new[column_idx], &column->type);
> - } else if (row->old) {
> - ovsdb_datum_clone(datum, &row->old[column_idx], &column->type);
> - } else {
> - ovsdb_datum_init_default(datum, &column->type);
> - }
> -}
> -
> -void
> ovsdb_idl_txn_write(const struct ovsdb_idl_row *row_,
> const struct ovsdb_idl_column *column,
> struct ovsdb_datum *datum)
> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
> index 31fae98..f38d42c 100644
> --- a/lib/ovsdb-idl.h
> +++ b/lib/ovsdb-idl.h
> @@ -19,6 +19,7 @@
> #include <stdbool.h>
> #include <stdint.h>
> #include "compiler.h"
> +#include "ovsdb-types.h"
>
> struct json;
> struct ovsdb_datum;
> @@ -45,6 +46,13 @@ const struct ovsdb_idl_row *ovsdb_idl_first_row(
> const struct ovsdb_idl *, const struct ovsdb_idl_table_class *);
> const struct ovsdb_idl_row *ovsdb_idl_next_row(const struct ovsdb_idl_row *);
>
> +const struct ovsdb_datum *ovsdb_idl_read(const struct ovsdb_idl_row *,
> + const struct ovsdb_idl_column *);
> +const struct ovsdb_datum *ovsdb_idl_get(const struct ovsdb_idl_row *,
> + const struct ovsdb_idl_column *,
> + enum ovsdb_atomic_type key_type,
> + enum ovsdb_atomic_type value_type);
> +
> enum ovsdb_idl_txn_status {
> TXN_UNCHANGED, /* Transaction didn't include any changes. */
> TXN_INCOMPLETE, /* Commit in progress, please wait. */
> @@ -76,9 +84,6 @@ int64_t ovsdb_idl_txn_get_increment_new_value(const struct ovsdb_idl_txn *);
> const struct uuid *ovsdb_idl_txn_get_insert_uuid(const struct ovsdb_idl_txn *,
> const struct uuid *);
>
> -void ovsdb_idl_txn_read(const struct ovsdb_idl_row *,
> - const struct ovsdb_idl_column *,
> - struct ovsdb_datum *);
> void ovsdb_idl_txn_write(const struct ovsdb_idl_row *,
> const struct ovsdb_idl_column *,
> struct ovsdb_datum *);
> diff --git a/ovsdb/OVSDB.py b/ovsdb/OVSDB.py
> index 0cd416e..3acc7b8 100644
> --- a/ovsdb/OVSDB.py
> +++ b/ovsdb/OVSDB.py
> @@ -298,6 +298,9 @@ class BaseType:
> 'boolean': 'bool ',
> 'string': 'char *'}[self.type]
>
> + def toAtomicType(self):
> + return "OVSDB_TYPE_%s" % self.type.upper()
> +
> def copyCValue(self, dst, src):
> args = {'dst': dst, 'src': src}
> if self.refTable:
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index aea317c..a2b481c 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -2004,19 +2004,17 @@ get_row_by_id(struct vsctl_context *ctx, const struct vsctl_table_class *table,
> const struct ovsdb_idl_row *row;
> unsigned int best_score = 0;
>
> - /* It might make sense to relax this assertion. */
> - assert(id->name_column->type.key.type == OVSDB_TYPE_STRING);
> -
> referrer = NULL;
> for (row = ovsdb_idl_first_row(ctx->idl, id->table);
> row != NULL && best_score != UINT_MAX;
> row = ovsdb_idl_next_row(row))
> {
> - struct ovsdb_datum name;
> + const struct ovsdb_datum *name;
>
> - ovsdb_idl_txn_read(row, id->name_column, &name);
> - if (name.n == 1) {
> - unsigned int score = score_partial_match(name.keys[0].string,
> + name = ovsdb_idl_get(row, id->name_column,
> + OVSDB_TYPE_STRING, OVSDB_TYPE_VOID);
> + if (name->n == 1) {
> + unsigned int score = score_partial_match(name->keys[0].string,
> record_id);
> if (score > best_score) {
> referrer = row;
> @@ -2025,7 +2023,6 @@ get_row_by_id(struct vsctl_context *ctx, const struct vsctl_table_class *table,
> referrer = NULL;
> }
> }
> - ovsdb_datum_destroy(&name, &id->name_column->type);
> }
> if (best_score && !referrer) {
> vsctl_fatal("multiple rows in %s match \"%s\"",
> @@ -2038,17 +2035,14 @@ get_row_by_id(struct vsctl_context *ctx, const struct vsctl_table_class *table,
>
> final = NULL;
> if (id->uuid_column) {
> - struct ovsdb_datum uuid;
> -
> - assert(id->uuid_column->type.key.type == OVSDB_TYPE_UUID);
> - assert(id->uuid_column->type.value.type == OVSDB_TYPE_VOID);
> + const struct ovsdb_datum *uuid;
>
> - ovsdb_idl_txn_read(referrer, id->uuid_column, &uuid);
> - if (uuid.n == 1) {
> + uuid = ovsdb_idl_get(referrer, id->uuid_column,
> + OVSDB_TYPE_UUID, OVSDB_TYPE_VOID);
> + if (uuid->n == 1) {
> final = ovsdb_idl_get_row_for_uuid(ctx->idl, table->class,
> - &uuid.keys[0].uuid);
> + &uuid->keys[0].uuid);
> }
> - ovsdb_datum_destroy(&uuid, &id->uuid_column->type);
> } else {
> final = referrer;
> }
> @@ -2222,13 +2216,13 @@ cmd_get(struct vsctl_context *ctx)
> row = must_get_row(ctx, table, record_id);
> for (i = 3; i < ctx->argc; i++) {
> const struct ovsdb_idl_column *column;
> - struct ovsdb_datum datum;
> + const struct ovsdb_datum *datum;
> char *key_string;
>
> die_if_error(parse_column_key_value(ctx->argv[i], table,
> &column, &key_string, NULL));
>
> - ovsdb_idl_txn_read(row, column, &datum);
> + datum = ovsdb_idl_read(row, column);
> if (key_string) {
> union ovsdb_atom key;
> unsigned int idx;
> @@ -2242,7 +2236,7 @@ cmd_get(struct vsctl_context *ctx)
> &column->type.key,
> key_string, ctx->symtab));
>
> - idx = ovsdb_datum_find_key(&datum, &key,
> + idx = ovsdb_datum_find_key(datum, &key,
> column->type.key.type);
> if (idx == UINT_MAX) {
> if (!if_exists) {
> @@ -2251,15 +2245,14 @@ cmd_get(struct vsctl_context *ctx)
> column->name);
> }
> } else {
> - ovsdb_atom_to_string(&datum.values[idx],
> + ovsdb_atom_to_string(&datum->values[idx],
> column->type.value.type, out);
> }
> ovsdb_atom_destroy(&key, column->type.key.type);
> } else {
> - ovsdb_datum_to_string(&datum, &column->type, out);
> + ovsdb_datum_to_string(datum, &column->type, out);
> }
> ds_put_char(out, '\n');
> - ovsdb_datum_destroy(&datum, &column->type);
>
> free(key_string);
> }
> @@ -2275,15 +2268,13 @@ list_record(const struct vsctl_table_class *table,
> UUID_ARGS(&row->uuid));
> for (i = 0; i < table->class->n_columns; i++) {
> const struct ovsdb_idl_column *column = &table->class->columns[i];
> - struct ovsdb_datum datum;
> + const struct ovsdb_datum *datum;
>
> - ovsdb_idl_txn_read(row, column, &datum);
> + datum = ovsdb_idl_read(row, column);
>
> ds_put_format(out, "%-20s: ", column->name);
> - ovsdb_datum_to_string(&datum, &column->type, out);
> + ovsdb_datum_to_string(datum, &column->type, out);
> ds_put_char(out, '\n');
> -
> - ovsdb_datum_destroy(&datum, &column->type);
> }
> }
>
> @@ -2336,7 +2327,7 @@ set_column(const struct vsctl_table_class *table,
>
> if (key_string) {
> union ovsdb_atom key, value;
> - struct ovsdb_datum old, new;
> + struct ovsdb_datum datum;
>
> if (column->type.value.type == OVSDB_TYPE_VOID) {
> vsctl_fatal("cannot specify key to set for non-map column %s",
> @@ -2348,17 +2339,15 @@ set_column(const struct vsctl_table_class *table,
> die_if_error(ovsdb_atom_from_string(&value, &column->type.value,
> value_string, symtab));
>
> - ovsdb_datum_init_empty(&new);
> - ovsdb_datum_add_unsafe(&new, &key, &value, &column->type);
> + ovsdb_datum_init_empty(&datum);
> + ovsdb_datum_add_unsafe(&datum, &key, &value, &column->type);
>
> ovsdb_atom_destroy(&key, column->type.key.type);
> ovsdb_atom_destroy(&value, column->type.value.type);
>
> - ovsdb_idl_txn_read(row, column, &old);
> - ovsdb_datum_union(&old, &new, &column->type, true);
> - ovsdb_idl_txn_write(row, column, &old);
> -
> - ovsdb_datum_destroy(&new, &column->type);
> + ovsdb_datum_union(&datum, ovsdb_idl_read(row, column),
> + &column->type, false);
> + ovsdb_idl_txn_write(row, column, &datum);
> } else {
> struct ovsdb_datum datum;
>
> @@ -2405,7 +2394,7 @@ cmd_add(struct vsctl_context *ctx)
> die_if_error(get_column(table, column_name, &column));
>
> type = &column->type;
> - ovsdb_idl_txn_read(row, column, &old);
> + ovsdb_datum_clone(&old, ovsdb_idl_read(row, column), &column->type);
> for (i = 4; i < ctx->argc; i++) {
> struct ovsdb_type add_type;
> struct ovsdb_datum add;
> @@ -2446,7 +2435,7 @@ cmd_remove(struct vsctl_context *ctx)
> die_if_error(get_column(table, column_name, &column));
>
> type = &column->type;
> - ovsdb_idl_txn_read(row, column, &old);
> + ovsdb_datum_clone(&old, ovsdb_idl_read(row, column), &column->type);
> for (i = 4; i < ctx->argc; i++) {
> struct ovsdb_type rm_type;
> struct ovsdb_datum rm;
> --
> 1.7.1
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
More information about the dev
mailing list