On 10/7/25 1:57 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.
>
> Signed-off-by: Ales Musil <[email protected]>
> ---
> v4: Remove acks.
> Fix write to index row.
I'm a little confused by this. ovsdb_idl_txn_write__() should not be
used for writing into index rows. Index rows supposed to be updated
via ovsdb_idl_index_write() instead. What am I missing here?
Best regards, Ilya Maximets.
>
> v3: Add Mike's ack.
> Address comments from Ilya:
> - Adjust the line wraps and indentation of tests.
> - Make sure we skip the test if NDEBUG is defined.
>
> v2: Add Dumitru's ack.
> Adjust the ovsdb_idl_txn_assert_read_only() comment.
> Address some small nits.
> ---
> lib/ovsdb-idl.c | 25 ++++++++++++++++++--
> lib/ovsdb-idl.h | 1 +
> tests/ovsdb-idl.at | 57 ++++++++++++++++++++++++++++++++++++++++++++++
> tests/test-ovsdb.c | 19 ++++++++++++++--
> 4 files changed, 98 insertions(+), 4 deletions(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index cd781f300..1d7794a3f 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.
> @@ -3645,11 +3660,12 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row
> *row_,
> write_only = column_mode == OVSDB_IDL_MONITOR;
> optimize_rewritten =
> write_only || (column_mode & OVSDB_IDL_WRITE_CHANGED_ONLY);
> -
> + bool index_row = is_index_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(index_row || !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"
> @@ -3677,7 +3693,6 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
> goto discard_datum;
> }
>
> - bool index_row = is_index_row(row);
> if (!index_row) {
> ovsdb_idl_remove_from_indexes(row);
> }
> @@ -3834,6 +3849,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 +3879,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 +4238,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 +4276,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..e658737b6 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -3000,3 +3000,60 @@ 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])
> +AT_CHECK([[test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 \
> + --assert-read-only idl unix:socket 'insert 1']],
> + [ignore], [ignore], [stderr])
> +AT_SKIP_IF([grep -q "Assertion tests are not available due to NDEBUG"
> stderr])
> +AT_CHECK([grep -qE "assertion .*!.*txn->assert_read_only failed" stderr])
> +
> +OVSDB_SERVER_SHUTDOWN
> +AT_CLEANUP
> +
> +AT_SETUP([ovsdb-idl - read only set - 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])
> +AT_CHECK([[test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 \
> + --assert-read-only idl unix:socket 'set 0 r 2.0']],
> + [ignore], [ignore], [stderr])
> +AT_SKIP_IF([grep -q "Assertion tests are not available due to NDEBUG"
> stderr])
> +AT_CHECK([grep -qE "assertion .*!.*txn->assert_read_only failed" stderr])
> +
> +OVSDB_SERVER_SHUTDOWN
> +AT_CLEANUP
> +
> +AT_SETUP([ovsdb-idl - read only delete - 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": {"i": 1}}]']],
> + [0], [ignore], [ignore])
> +AT_CHECK([[test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 \
> + --assert-read-only idl unix:socket 'delete 1']],
> + [ignore], [ignore], [stderr])
> +AT_SKIP_IF([grep -q "Assertion tests are not available due to NDEBUG"
> stderr])
> +AT_CHECK([grep -qE "assertion .*!.*txn->assert_read_only failed" stderr])
> +
> +OVSDB_SERVER_SHUTDOWN
> +AT_CLEANUP
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index d3bb199ef..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>
> @@ -57,6 +62,7 @@ VLOG_DEFINE_THIS_MODULE(test_ovsdb);
>
> struct test_ovsdb_pvt_context {
> bool write_changed_only;
> + bool assert_read_only;
> bool track;
> };
>
> @@ -93,6 +99,7 @@ parse_options(int argc, char *argv[], struct
> test_ovsdb_pvt_context *pvt)
> {"verbose", optional_argument, NULL, 'v'},
> {"change-track", optional_argument, NULL, 'c'},
> {"write-changed-only", optional_argument, NULL, 'w'},
> + {"assert-read-only", optional_argument, NULL, 'r'},
> {"magic", required_argument, NULL, OPT_MAGIC},
> {"no-rename-open-files", no_argument, NULL,
> OPT_NO_RENAME_OPEN_FILES},
> {"help", no_argument, NULL, 'h'},
> @@ -131,6 +138,13 @@ parse_options(int argc, char *argv[], struct
> test_ovsdb_pvt_context *pvt)
> pvt->write_changed_only = true;
> 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;
> +
> case OPT_MAGIC:
> magic = optarg;
> break;
> @@ -2459,7 +2473,7 @@ idltest_find_simple(struct ovsdb_idl *idl, int i)
> }
>
> static bool
> -idl_set(struct ovsdb_idl *idl, char *commands, int step)
> +idl_set(struct ovsdb_idl *idl, char *commands, int step, bool
> assert_read_only)
> {
> char *cmd, *save_ptr1 = NULL;
> struct ovsdb_idl_txn *txn;
> @@ -2472,6 +2486,7 @@ idl_set(struct ovsdb_idl *idl, char *commands, int step)
>
> txn = ovsdb_idl_txn_create(idl);
> ovsdb_idl_check_consistency(idl);
> + ovsdb_idl_txn_assert_read_only(txn, assert_read_only);
> for (cmd = strtok_r(commands, ",", &save_ptr1); cmd;
> cmd = strtok_r(NULL, ",", &save_ptr1)) {
> char *save_ptr2 = NULL;
> @@ -2911,7 +2926,7 @@ do_idl(struct ovs_cmdl_context *ctx)
> next_cond_seqno = update_conditions(idl, arg + strlen(cond_s),
> step++);
> } else if (arg[0] != '[') {
> - if (!idl_set(idl, arg, step++)) {
> + if (!idl_set(idl, arg, step++, pvt->assert_read_only)) {
> /* If idl_set() returns false, then no transaction
> * was sent to the server and most likely 'seqno'
> * would remain the same. And the above 'Wait for update'
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
