[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