On Tue, Sep 23, 2025 at 3:09 PM Dumitru Ceara <[email protected]> wrote:
> On 9/11/25 8:22 AM, Ales Musil 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]> > > --- > > Hi Ales, > > Thanks for the patch! The changes look correct to me and will achieve > what's needed in ovn-controller (as an IDL client). I have some minor > comments below. > Hi Dumitru, thank you for the review. > > > lib/ovsdb-idl.c | 21 +++++++++++++++++++++ > > lib/ovsdb-idl.h | 1 + > > tests/ovsdb-idl.at | 43 +++++++++++++++++++++++++++++++++++++++++++ > > tests/test-ovsdb.c | 11 +++++++++-- > > 4 files changed, 74 insertions(+), 2 deletions(-) > > > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > > index cd781f300..ed98c011e 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; > > @@ -3650,6 +3653,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 +3838,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 +3868,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 +4227,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 +4265,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; > > @@ -4566,3 +4576,14 @@ ovsdb_idl_loop_commit_and_wait(struct > ovsdb_idl_loop *loop) > > > > return retval; > > } > > + > > +/* Set the IDL txn to be read only or read/write. When the txn is set to > > + * read only any write operation attempted on the txn will fail > > Nit: the transaction itself is not necessarily read only or read/write. > Knowing the context of how this plans to be used, it's possible that > ovn-controller will add some write operations then call > ovsdb_idl_txn_assert_read_only(txn, true) then do some more processing > that should not change the database, then call > ovsdb_idl_txn_assert_read_only(txn, false). I think I'd rephrase this to: > > /* 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. > You are right that it makes more sense. Addressed in v2. > . > > + * 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; > > + } > > +} > > Tiny nit: I think I'd move this above, just after the definition of > ovsdb_idl_txn_abort() as it's more about the transaction as a whole. > But I have no strong opinion about it in the end. > > > diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h > > index 0d4864b6f..18e64456e 100644 > > --- a/lib/ovsdb-idl.h > > +++ b/lib/ovsdb-idl.h > > @@ -385,6 +385,7 @@ const struct ovsdb_idl_row > *ovsdb_idl_txn_insert_persist_uuid( > > const struct uuid *uuid); > > > > struct ovsdb_idl *ovsdb_idl_txn_get_idl (struct ovsdb_idl_txn *); > > +void ovsdb_idl_txn_assert_read_only(struct ovsdb_idl_txn *txn, bool > assert); > > If we move the implementation as suggested above then I'd move this one > too just after ovsdb_idl_txn_abort(). > > > void ovsdb_idl_get_initial_snapshot(struct ovsdb_idl *); > > > > > > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > > index 8dcc4c75f..69836c416 100644 > > --- a/tests/ovsdb-idl.at > > +++ b/tests/ovsdb-idl.at > > @@ -3000,3 +3000,46 @@ m4_define([OVSDB_CHECK_IDL_CHANGE_AWARE], > > > > OVSDB_CHECK_IDL_CHANGE_AWARE([standalone]) > > OVSDB_CHECK_IDL_CHANGE_AWARE([clustered]) > > + > > + > > Nit: one empty line too many. > > > +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_CHECK([grep -qE "assertion !.*txn->assert_read_only failed" stderr], > [0]) > > + > > +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_CHECK([grep -qE "assertion !.*txn->assert_read_only failed" stderr], > [0]) > > + > > +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_CHECK([grep -qE "assertion !.*txn->assert_read_only failed" stderr], > [0]) > > + > > +OVSDB_SERVER_SHUTDOWN > > +AT_CLEANUP > > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c > > index d3bb199ef..4cd454892 100644 > > --- a/tests/test-ovsdb.c > > +++ b/tests/test-ovsdb.c > > @@ -58,6 +58,7 @@ VLOG_DEFINE_THIS_MODULE(test_ovsdb); > > struct test_ovsdb_pvt_context { > > bool write_changed_only; > > bool track; > > + bool aseert_read_only; > > Typo: "aseert". > > Nit: I'd move this one line above for reverse xmas tree. > > > }; > > > > /* Magic to pass to ovsdb_log_open(). */ > > @@ -93,6 +94,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 +133,10 @@ parse_options(int argc, char *argv[], struct > test_ovsdb_pvt_context *pvt) > > pvt->write_changed_only = true; > > break; > > > > + case 'r': > > + pvt->aseert_read_only = true; > > + break; > > + > > case OPT_MAGIC: > > magic = optarg; > > break; > > @@ -2459,7 +2465,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 +2478,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 +2918,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->aseert_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' > > If these are the only changes in v2 please feel free to add my ack: > > Acked-by: Dumitru Ceara <[email protected]> > > Regards, > Dumitru > > All the other nits are addressed in v2 as well. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
