On Mon, Oct 6, 2025 at 9:31 PM Ilya Maximets <[email protected]> wrote:
> On 9/23/25 4:02 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. > > > > Acked-by: Dumitru Ceara <[email protected]> > > Signed-off-by: Ales Musil <[email protected]> > > --- > > v2: Add Dumitru's ack. > > Adjust the ovsdb_idl_txn_assert_read_only() comment. > > Address some small nits. > > --- > > lib/ovsdb-idl.c | 22 ++++++++++++++++++++++ > > lib/ovsdb-idl.h | 1 + > > tests/ovsdb-idl.at | 42 ++++++++++++++++++++++++++++++++++++++++++ > > tests/test-ovsdb.c | 11 +++++++++-- > > 4 files changed, 74 insertions(+), 2 deletions(-) > > Hi, Ales. Thanks for the update! > > The change looks good to me in general, see a few comments below. > > Hi Ilya, thank you for the review. All points should be addressed in v3. > > > > 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..a7a7f751b 100644 > > --- a/tests/ovsdb-idl.at > > +++ b/tests/ovsdb-idl.at > > @@ -3000,3 +3000,45 @@ 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]) > > I'd suggect to break the line with a '\' after the 'socket' and put the > transaction description at 2 spaces indentation, more similar to how the > OVSDB_CHECK_IDL tests are formatted. E.g.: > > 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]) > > This long line may also be wrapped like this: > > 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]) > > No need to have the [0], it's the default value for checks. > > Same formatting comments apply to all the other tests. > > The more important thing here, however, is that the tests are failing if we > actually build with NDEBUG. We can try to avoid that by doing something > like this: > > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c > index 7bfff7c01..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> > @@ -134,6 +139,9 @@ parse_options(int argc, char *argv[], struct > test_ovsdb_pvt_context *pvt) > 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; > > --- > > And then: > > AT_SKIP_IF([grep -q "Assertion tests are not available due to NDEBUG" > stderr]) > > This is still not bulletproof, as technically it's possible that tests and > the > libraries are built with different flags, but that is highly unlikely. > > > > Note: we need to undef the NDEBUG, as all other tests do, so we can't use > it > directly. The test-ovsdb didn't undef it for some reason, but it should. > > Note2: there is a couple of warnings when building with -DNDEBUG today > which > are unrelated to this patch. Build works fine without -Werror though. We > can > fix the warnings by re-defining ovs_assert like this: > > -#define ovs_assert(CONDITION) ((void) (CONDITION)) > +#define ovs_assert(CONDITION) (ovs_ignore(CONDITION)) > > And renaming and exporting ignore() as ovs_ignore(), but that's a separate > thing. > > Best regards, Ilya Maximets. > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
