Acked-by: Andy Zhou <az...@nicira.com>
On Mon, Aug 31, 2015 at 9:52 AM, Ben Pfaff <b...@nicira.com> wrote: > 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.raj2...@gmail.com> > Reported-at: http://openvswitch.org/pipermail/dev/2015-August/059439.html > Signed-off-by: Ben Pfaff <b...@nicira.com> > --- > ovsdb/transaction.c | 25 ++++++++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c > index 83ddaff..2c85fee 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); > > -- > 2.1.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
