ovsdb-idl: Transition to better interfaces for reading table columns.

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.

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 2537db7..f32fe0e 100644 (file)
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -908,6 +908,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);
+}
 \f
 /* Transactions. */
 
@@ -1409,24 +1458,6 @@ ovsdb_idl_txn_complete(struct ovsdb_idl_txn *txn,
     hmap_remove(&txn->idl->outstanding_txns, &txn->hmap_node);
 }
 
-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,

diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
index 7832e4e..c09ba9b 100644 (file)
--- a/lib/ovsdb-idl.h
+++ b/lib/ovsdb-idl.h
@@ -33,6 +33,7 @@
 #include <stdbool.h>
 #include <stdint.h>
 #include "compiler.h"
+#include "ovsdb-types.h"
 
 struct json;
 struct ovsdb_datum;
@@ -59,6 +60,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. */
@@ -90,9 +98,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 (file)
--- 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 0291fe8..b5ab253 100644 (file)
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -1989,23 +1989,21 @@ 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) {
+            name = ovsdb_idl_get(row, id->name_column,
+                                 OVSDB_TYPE_STRING, OVSDB_TYPE_VOID);
+            if (name->n == 1) {
                 unsigned int score;
 
                 score = (partial_match_ok
-                         ? score_partial_match(name.keys[0].string, record_id)
-                         : !strcmp(name.keys[0].string, record_id));
+                         ? score_partial_match(name->keys[0].string, record_id)
+                         : !strcmp(name->keys[0].string, record_id));
                 if (score > best_score) {
                     referrer = row;
                     best_score = score;
@@ -2013,7 +2011,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\"",
@@ -2026,17 +2023,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;
     }
@@ -2297,7 +2291,7 @@ 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;
 
         /* Special case for obtaining the UUID of a row.  We can't just do this
@@ -2313,7 +2307,7 @@ cmd_get(struct vsctl_context *ctx)
                                             &column, &key_string,
                                             NULL, NULL, 0, NULL));
 
-        ovsdb_idl_txn_read(row, column, &datum);
+        datum = ovsdb_idl_read(row, column);
         if (key_string) {
             union ovsdb_atom key;
             unsigned int idx;
@@ -2327,7 +2321,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) {
@@ -2336,15 +2330,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);
     }
@@ -2360,15 +2353,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);
     }
 }
 
@@ -2421,7 +2412,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",
@@ -2433,17 +2424,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;
 
@@ -2490,7 +2479,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;
@@ -2531,7 +2520,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;
@@ -2681,8 +2670,8 @@ is_condition_satified(const struct vsctl_table_class *table,
     };
 
     const struct ovsdb_idl_column *column;
+    const struct ovsdb_datum *have_datum;
     char *key_string, *value_string;
-    struct ovsdb_datum have_datum;
     const char *operator;
     unsigned int idx;
     char *error;
@@ -2696,7 +2685,7 @@ is_condition_satified(const struct vsctl_table_class *table,
         vsctl_fatal("%s: missing value", arg);
     }
 
-    ovsdb_idl_txn_read(row, column, &have_datum);
+    have_datum = ovsdb_idl_read(row, column);
     if (key_string) {
         union ovsdb_atom want_key, want_value;
 
@@ -2710,10 +2699,10 @@ is_condition_satified(const struct vsctl_table_class *table,
         die_if_error(ovsdb_atom_from_string(&want_value, &column->type.value,
                                             value_string, symtab));
 
-        idx = ovsdb_datum_find_key(&have_datum,
+        idx = ovsdb_datum_find_key(have_datum,
                                    &want_key, column->type.key.type);
         if (idx != UINT_MAX) {
-            cmp = ovsdb_atom_compare_3way(&have_datum.values[idx],
+            cmp = ovsdb_atom_compare_3way(&have_datum->values[idx],
                                           &want_value,
                                           column->type.value.type);
         }
@@ -2726,11 +2715,10 @@ is_condition_satified(const struct vsctl_table_class *table,
         die_if_error(ovsdb_datum_from_string(&want_datum, &column->type,
                                              value_string, symtab));
         idx = 0;
-        cmp = ovsdb_datum_compare_3way(&have_datum, &want_datum,
+        cmp = ovsdb_datum_compare_3way(have_datum, &want_datum,
                                        &column->type);
         ovsdb_datum_destroy(&want_datum, &column->type);
     }
-    ovsdb_datum_destroy(&have_datum, &column->type);
 
     free(key_string);
     free(value_string);