ovsdb: Update _version more accurately in transaction commit.

The _version column in each OVSDB row is supposed to be updated whenever
any other column in the row changes.  However, the transaction code was
not careful to do this only when a row actually changed--there were other
cases where a row was considered at transaction commit time and _version
updated even though the row did not actually change.  For example,
ovsdb_txn_adjust_atom_refs() calls find_or_make_txn_row(), which calls
ovsdb_txn_row_modify(), which updates _version, but
ovsdb_txn_adjust_atom_refs() doesn't actually update any data.

One way to fix this would be to carefully consider and adjust all the code
that looks at transaction rows.  However, this seems somewhat error prone
and thus difficult to test.  This commit takes a different approach: it
drops the code that adjusts _version on the fly, instead replacing it by
a final pass over the database at the end of the commit process that checks
for each row whether any columns changed and updates _version at that point
if any did.  That seems pretty foolproof to me.

Reported-by: RishiRaj Maulick <rishi.raj2509@gmail.com>
Reported-at: http://openvswitch.org/pipermail/dev/2015-August/059439.html
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Andy Zhou <azhou@nicira.com>
Tested-by: RishiRaj Maulick <rishi.raj2509@gmail.com>

1 files changed, 22 insertions, 3 deletions

diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index 83ddaffd6..2c85fee11 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -534,7 +534,6 @@ assess_weak_refs(struct ovsdb_txn *txn, struct ovsdb_txn_row *txn_row)
         }
 
         if (datum->n != orig_n) {
-            bitmap_set1(txn_row->changed, OVSDB_COL_VERSION);
             bitmap_set1(txn_row->changed, column->index);
             ovsdb_datum_sort_assert(datum, column->type.key.type);
             if (datum->n < column->type.n_min) {
@@ -748,6 +747,21 @@ check_index_uniqueness(struct ovsdb_txn *txn OVS_UNUSED,
     return NULL;
 }
 
+static struct ovsdb_error * OVS_WARN_UNUSED_RESULT
+update_version(struct ovsdb_txn *txn OVS_UNUSED, struct ovsdb_txn_row *txn_row)
+{
+    struct ovsdb_table *table = txn_row->table;
+    size_t n_columns = shash_count(&table->schema->columns);
+
+    if (txn_row->old && txn_row->new
+        && !bitmap_is_all_zeros(txn_row->changed, n_columns)) {
+        bitmap_set1(txn_row->changed, OVSDB_COL_VERSION);
+        uuid_generate(ovsdb_row_get_version_rw(txn_row->new));
+    }
+
+    return NULL;
+}
+
 static struct ovsdb_error *
 ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
 {
@@ -801,6 +815,12 @@ ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
         return error;
     }
 
+    /* Update _version for rows that changed.  */
+    error = for_each_txn_row(txn, update_version);
+    if (error) {
+        return OVSDB_WRAP_BUG("can't happen", error);
+    }
+
     /* Send the commit to each replica. */
     LIST_FOR_EACH (replica, node, &txn->db->replicas) {
         error = (replica->class->commit)(replica, txn, durable);
@@ -915,7 +935,6 @@ ovsdb_txn_row_modify(struct ovsdb_txn *txn, const struct ovsdb_row *ro_row_)
 
         rw_row = ovsdb_row_clone(ro_row);
         rw_row->n_refs = ro_row->n_refs;
-        uuid_generate(ovsdb_row_get_version_rw(rw_row));
         ovsdb_txn_row_create(txn, table, ro_row, rw_row);
         hmap_replace(&table->rows, &ro_row->hmap_node, &rw_row->hmap_node);