On Tue, Oct 7, 2025 at 2:32 PM Ilya Maximets <[email protected]> wrote:
> 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? > Yeah I will be sending fix for ovn about this, there were places where set was used instead of index_set. I've got confused in the process. So I guess we can still go with the v3? Thanks, Ales > > 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
