| Message ID | 20231218020249.3447973-2-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | d51d4f42d3e880dce7c13b1437c8d5d1312ecbd5 |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | ovsdb: transaction: Bug Fixes + Reuse of column diffs. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > Datum of UUID and _version columns is accessed directly via > ovsdb_row_get_uuid_rw() and ovsdb_row_get_version_rw() functions > instead of ovsdb_data_* functions. Meaning, the data will be > directly modified even if it is shared between rows. > > Fix that by unsharing the data whenever RW pointer is taken. > > The issue was mostly hidden because weak reference assessment > code always called ovsdb_datum_subtract() even if not needed. > This way all the new transaction rows were always implicitly > unshared. > > Also making ovsdb_datum_subtract() call conditional, so the > issue can be hit by existing unit tests. > > Fixes: 485ac63d10f8 ("ovsdb: Add lazy-copy support for ovsdb_datum objects.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> This looks correct to me. Acked-by: Mike Pattrick <mkp@redhat.com>
diff --git a/ovsdb/row.h b/ovsdb/row.h index 59f498a20..6f5e58acb 100644 --- a/ovsdb/row.h +++ b/ovsdb/row.h @@ -130,6 +130,7 @@ ovsdb_row_get_uuid(const struct ovsdb_row *row) static inline struct uuid * ovsdb_row_get_uuid_rw(struct ovsdb_row *row) { + ovsdb_datum_unshare(&row->fields[OVSDB_COL_UUID], &ovsdb_type_uuid); return &row->fields[OVSDB_COL_UUID].keys[0].uuid; } @@ -142,6 +143,7 @@ ovsdb_row_get_version(const struct ovsdb_row *row) static inline struct uuid * ovsdb_row_get_version_rw(struct ovsdb_row *row) { + ovsdb_datum_unshare(&row->fields[OVSDB_COL_VERSION], &ovsdb_type_uuid); return &row->fields[OVSDB_COL_VERSION].keys[0].uuid; } diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c index 4fdc5bcea..f43533a8c 100644 --- a/ovsdb/transaction.c +++ b/ovsdb/transaction.c @@ -733,8 +733,10 @@ assess_weak_refs(struct ovsdb_txn *txn, struct ovsdb_txn_row *txn_row) ovsdb_datum_sort_unique(&deleted_refs, &column->type); /* Removing elements that references deleted rows. */ - ovsdb_datum_subtract(datum, &column->type, - &deleted_refs, &column->type); + if (deleted_refs.n) { + ovsdb_datum_subtract(datum, &column->type, + &deleted_refs, &column->type); + } ovsdb_datum_destroy(&deleted_refs, &column->type); /* Generating the difference between old and new data. */
Datum of UUID and _version columns is accessed directly via ovsdb_row_get_uuid_rw() and ovsdb_row_get_version_rw() functions instead of ovsdb_data_* functions. Meaning, the data will be directly modified even if it is shared between rows. Fix that by unsharing the data whenever RW pointer is taken. The issue was mostly hidden because weak reference assessment code always called ovsdb_datum_subtract() even if not needed. This way all the new transaction rows were always implicitly unshared. Also making ovsdb_datum_subtract() call conditional, so the issue can be hit by existing unit tests. Fixes: 485ac63d10f8 ("ovsdb: Add lazy-copy support for ovsdb_datum objects.") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- ovsdb/row.h | 2 ++ ovsdb/transaction.c | 6 ++++-- 2 files changed, 6 insertions(+), 2 deletions(-)