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.
> 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.
.
> + * 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
