On 9/23/25 4:02 PM, Ales Musil via dev wrote:
> Add a way to assert IDL txn as read only, that is useful in cases when
> we want to ensure that the program doesn't write anything into the
> txn. It is done via assert because this is considered a bug which
> should be fixed in the IDL caller.
>
> Acked-by: Dumitru Ceara <[email protected]>
> Signed-off-by: Ales Musil <[email protected]>
> ---
> v2: Add Dumitru's ack.
> Adjust the ovsdb_idl_txn_assert_read_only() comment.
> Address some small nits.
> ---
> lib/ovsdb-idl.c | 22 ++++++++++++++++++++++
> lib/ovsdb-idl.h | 1 +
> tests/ovsdb-idl.at | 42 ++++++++++++++++++++++++++++++++++++++++++
> tests/test-ovsdb.c | 11 +++++++++--
> 4 files changed, 74 insertions(+), 2 deletions(-)
Hi, Ales. Thanks for the update!
The change looks good to me in general, see a few comments below.
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index cd781f300..7f49e922d 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -111,6 +111,7 @@ struct ovsdb_idl_txn {
> enum ovsdb_idl_txn_status status;
> char *error;
> bool dry_run;
> + bool assert_read_only;
> struct ds comment;
>
> /* Increments. */
> @@ -2771,6 +2772,7 @@ ovsdb_idl_txn_create(struct ovsdb_idl *idl)
> txn->status = TXN_UNCOMMITTED;
> txn->error = NULL;
> txn->dry_run = false;
> + txn->assert_read_only = false;
> ds_init(&txn->comment);
>
> txn->inc_table = NULL;
> @@ -2839,6 +2841,7 @@ ovsdb_idl_txn_increment(struct ovsdb_idl_txn *txn,
> ovs_assert(!txn->inc_table);
> ovs_assert(column->type.key.type == OVSDB_TYPE_INTEGER);
> ovs_assert(column->type.value.type == OVSDB_TYPE_VOID);
> + ovs_assert(!txn->assert_read_only);
>
> txn->inc_table = row->table->class_->name;
> txn->inc_column = column->name;
> @@ -3560,6 +3563,18 @@ ovsdb_idl_txn_abort(struct ovsdb_idl_txn *txn)
> }
> }
>
> +/* If 'assert' is true, configures the IDL to generate an assertion
> + * failure when a write operation is attempted on the transaction.
> + * Otherwise, write operations are allowed on the transaction.
> + * The check will turn into no-op when building with NDEBUG. */
> +void
> +ovsdb_idl_txn_assert_read_only(struct ovsdb_idl_txn *txn, bool assert)
> +{
> + if (txn) {
> + txn->assert_read_only = assert;
> + }
> +}
> +
> /* Returns a string that reports the error status for 'txn'. The caller must
> * not modify or free the returned string. A call to ovsdb_idl_txn_destroy()
> * for 'txn' may free the returned string.
> @@ -3650,6 +3665,7 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
> ovs_assert(row->new_datum != NULL);
> ovs_assert(column_idx < class->n_columns);
> ovs_assert(row->old_datum == NULL || column_mode & OVSDB_IDL_MONITOR);
> + ovs_assert(!row->table->idl->txn->assert_read_only);
>
> if (row->table->idl->verify_write_only && !write_only) {
> VLOG_ERR("Bug: Attempt to write to a read/write column (%s:%s) when"
> @@ -3834,6 +3850,7 @@ ovsdb_idl_txn_delete(const struct ovsdb_idl_row *row_)
>
> ovs_assert(row->new_datum != NULL);
> ovs_assert(!is_index_row(row_));
> + ovs_assert(!row->table->idl->txn->assert_read_only);
> ovsdb_idl_remove_from_indexes(row_);
> if (!row->old_datum) {
> ovsdb_idl_row_unparse(row);
> @@ -3863,6 +3880,7 @@ ovsdb_idl_txn_insert__(struct ovsdb_idl_txn *txn,
> struct ovsdb_idl_row *row = ovsdb_idl_row_create__(class);
>
> ovs_assert(uuid || !persist_uuid);
> + ovs_assert(!txn->assert_read_only);
> if (uuid) {
> ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid));
> row->uuid = *uuid;
> @@ -4221,6 +4239,8 @@ ovsdb_idl_txn_add_map_op(struct ovsdb_idl_row *row,
> struct ovsdb_datum *datum,
> enum map_op_type op_type)
> {
> + ovs_assert(!row->table->idl->txn->assert_read_only);
> +
> const struct ovsdb_idl_table_class *class;
> size_t column_idx;
> struct map_op *map_op;
> @@ -4257,6 +4277,8 @@ ovsdb_idl_txn_add_set_op(struct ovsdb_idl_row *row,
> struct ovsdb_datum *datum,
> enum set_op_type op_type)
> {
> + ovs_assert(!row->table->idl->txn->assert_read_only);
> +
> const struct ovsdb_idl_table_class *class;
> size_t column_idx;
> struct set_op *set_op;
> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
> index 0d4864b6f..f6060e324 100644
> --- a/lib/ovsdb-idl.h
> +++ b/lib/ovsdb-idl.h
> @@ -351,6 +351,7 @@ void ovsdb_idl_txn_wait(const struct ovsdb_idl_txn *);
> enum ovsdb_idl_txn_status ovsdb_idl_txn_commit(struct ovsdb_idl_txn *);
> enum ovsdb_idl_txn_status ovsdb_idl_txn_commit_block(struct ovsdb_idl_txn *);
> void ovsdb_idl_txn_abort(struct ovsdb_idl_txn *);
> +void ovsdb_idl_txn_assert_read_only(struct ovsdb_idl_txn *, bool assert);
>
> const char *ovsdb_idl_txn_get_error(const struct ovsdb_idl_txn *);
>
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index 8dcc4c75f..a7a7f751b 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -3000,3 +3000,45 @@ m4_define([OVSDB_CHECK_IDL_CHANGE_AWARE],
>
> OVSDB_CHECK_IDL_CHANGE_AWARE([standalone])
> OVSDB_CHECK_IDL_CHANGE_AWARE([clustered])
> +
> +AT_SETUP([ovsdb-idl - read only insert - C])
> +AT_KEYWORDS([ovsdb server idl read only assert])
> +OVSDB_START_IDLTEST
> +
> +AT_CHECK([[ovsdb-client transact unix:socket '["idltest",
> + {"op": "insert",
> + "table": "simple",
> + "row": {}}]']], [0],
> [ignore], [ignore])
I'd suggect to break the line with a '\' after the 'socket' and put the
transaction description at 2 spaces indentation, more similar to how the
OVSDB_CHECK_IDL tests are formatted. E.g.:
AT_CHECK([[ovsdb-client transact unix:socket \
'["idltest",
{"op": "insert",
"table": "simple",
"row": {}}]']],
[0], [ignore], [ignore])
> +AT_CHECK([[test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10
> --assert-read-only idl unix:socket 'insert 1']], [ignore], [ignore], [stderr])
This long line may also be wrapped like this:
AT_CHECK([[test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 \
--assert-read-only idl unix:socket 'insert 1']],
[ignore], [ignore], [stderr])
> +AT_CHECK([grep -qE "assertion !.*txn->assert_read_only failed" stderr], [0])
No need to have the [0], it's the default value for checks.
Same formatting comments apply to all the other tests.
The more important thing here, however, is that the tests are failing if we
actually build with NDEBUG. We can try to avoid that by doing something
like this:
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index 7bfff7c01..95290ae78 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -16,6 +16,11 @@
#include <config.h>
+#ifdef NDEBUG
+#define TEST_OVSDB_SKIP_ASSERT 1
+#undef NDEBUG
+#endif
+
#include <fcntl.h>
#include <getopt.h>
#include <inttypes.h>
@@ -134,6 +139,9 @@ parse_options(int argc, char *argv[], struct
test_ovsdb_pvt_context *pvt)
break;
case 'r':
+#ifdef TEST_OVSDB_SKIP_ASSERT
+ ovs_fatal(0, "Assertion tests are not available due to NDEBUG");
+#endif
pvt->assert_read_only = true;
break;
---
And then:
AT_SKIP_IF([grep -q "Assertion tests are not available due to NDEBUG" stderr])
This is still not bulletproof, as technically it's possible that tests and the
libraries are built with different flags, but that is highly unlikely.
Note: we need to undef the NDEBUG, as all other tests do, so we can't use it
directly. The test-ovsdb didn't undef it for some reason, but it should.
Note2: there is a couple of warnings when building with -DNDEBUG today which
are unrelated to this patch. Build works fine without -Werror though. We can
fix the warnings by re-defining ovs_assert like this:
-#define ovs_assert(CONDITION) ((void) (CONDITION))
+#define ovs_assert(CONDITION) (ovs_ignore(CONDITION))
And renaming and exporting ignore() as ovs_ignore(), but that's a separate
thing.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
