On 3 Dec 2025, at 12:01, Ilya Maximets wrote:
> The message about conflicting rows is trying to order the two rows
> in a consistent manner, so the log messages do not change in tests.
> But it fails to do so, because the order of columns in the column
> set depends on the order of columns inside the hash map, which
> depends on the hash function and the internal implementation details
> of the hash map. This results in random test failures, when two
> rows end up in the opposite order.
>
> Uncovered while testing a different hash map implementation, but the
> failure is technically possible even without any changes in the code,
> e.g., by running on a different CPU architecture or with different
> compiler flags.
>
> Fix that by introducing a new function that constructs the column
> set with columns in a predictable order and without UUID columns that
> have random values in most cases and so not actually comparable.
Thanks, Ilya.
The patch looks good to me. I reviewed the code, and OVSDB_TYPE_UUID is not
used for indexing.
Acked-by: Eelco Chaudron [email protected]
>
> Fixes: 6910a6e6f25f ("ovsdb: Implement table uniqueness constraints
> ("indexes").")
> Reported-by: Rosemarie O'Riorden <[email protected]>
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
> ovsdb/column.c | 19 +++++++++++++++++++
> ovsdb/column.h | 2 ++
> ovsdb/transaction.c | 2 +-
> 3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/ovsdb/column.c b/ovsdb/column.c
> index f4dc5bea6..a261cb61a 100644
> --- a/ovsdb/column.c
> +++ b/ovsdb/column.c
> @@ -257,6 +257,25 @@ ovsdb_column_set_add_all(struct ovsdb_column_set *set,
> }
> }
>
> +void
> +ovsdb_column_set_add_all_comparable(struct ovsdb_column_set *set,
> + const struct ovsdb_table *table)
> +{
> + const struct shash_node **nodes = shash_sort(&table->schema->columns);
> + size_t i, n = shash_count(&table->schema->columns);
> +
> + for (i = 0; i < n; i++) {
> + const struct ovsdb_column *column = nodes[i]->data;
> +
> + /* UUIDs would cause a random order if used for comparison. */
> + if (column->type.key.type != OVSDB_TYPE_UUID
> + && column->type.value.type != OVSDB_TYPE_UUID) {
> + ovsdb_column_set_add(set, column);
> + }
> + }
> + free(nodes);
> +}
> +
> bool
> ovsdb_column_set_contains(const struct ovsdb_column_set *set,
> unsigned int column_index)
> diff --git a/ovsdb/column.h b/ovsdb/column.h
> index f75a1076d..bae62567f 100644
> --- a/ovsdb/column.h
> +++ b/ovsdb/column.h
> @@ -79,6 +79,8 @@ void ovsdb_column_set_add(struct ovsdb_column_set *,
> const struct ovsdb_column *);
> void ovsdb_column_set_add_all(struct ovsdb_column_set *,
> const struct ovsdb_table *);
> +void ovsdb_column_set_add_all_comparable(struct ovsdb_column_set *,
> + const struct ovsdb_table *);
> bool ovsdb_column_set_contains(const struct ovsdb_column_set *,
> unsigned int column_index);
> bool ovsdb_column_set_equals(const struct ovsdb_column_set *,
> diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
> index d16f4daa3..0d0d27b61 100644
> --- a/ovsdb/transaction.c
> +++ b/ovsdb/transaction.c
> @@ -924,7 +924,7 @@ duplicate_index_row(const struct ovsdb_column_set *index,
> /* Put 'a' and 'b' in a predictable order to make error messages
> * reproducible for testing. */
> ovsdb_column_set_init(&all_columns);
> - ovsdb_column_set_add_all(&all_columns, a->table);
> + ovsdb_column_set_add_all_comparable(&all_columns, a->table);
> if (ovsdb_row_compare_columns_3way(a, b, &all_columns) < 0) {
> const struct ovsdb_row *tmp = a;
> a = b;
> --
> 2.51.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
