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