On Tue, Oct 7, 2025 at 8:27 AM Ales Musil <[email protected]> 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]> > --- > 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. > --- > I have realized that this version will also crash on indexed rows which shouldn't happen, I'll send v4 that will fix that. Thanks, Ales > lib/ovsdb-idl.c | 22 ++++++++++++++++++ > lib/ovsdb-idl.h | 1 + > tests/ovsdb-idl.at | 57 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/test-ovsdb.c | 19 ++++++++++++++-- > 4 files changed, 97 insertions(+), 2 deletions(-) > > 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..ae097887d 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' > -- > 2.51.0 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
