On 7/11/23 13:40, Eelco Chaudron wrote:
>> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
>> index 5361b3c76..a3ca48a7b 100644
>> --- a/ovsdb/jsonrpc-server.c
>> +++ b/ovsdb/jsonrpc-server.c
>> @@ -1131,6 +1131,8 @@ static void
>> ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *s, struct ovsdb
>> *db,
>> struct jsonrpc_msg *request)
>> {
>
> For the below ones, please see earlier comments on ovsdb related code, Ilya?
Some of these are fine, though I'm not a fan of constant checking of all the
input arguments. Some are strange, see inline.
>
>> + ovs_assert(db);
>> +
>> /* Check for duplicate ID. */
>> size_t hash = json_hash(request->id, 0);
>> struct ovsdb_jsonrpc_trigger *t
>> @@ -1391,6 +1393,8 @@ ovsdb_jsonrpc_monitor_create(struct
>> ovsdb_jsonrpc_session *s, struct ovsdb *db,
>> enum ovsdb_monitor_version version,
>> const struct json *request_id)
>> {
>> + ovs_assert(db);
>> +
>> struct ovsdb_jsonrpc_monitor *m = NULL;
>> struct ovsdb_monitor *dbmon = NULL;
>> struct json *monitor_id, *monitor_requests;
>> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
>> index b560b0745..2180f9e2d 100644
>> --- a/ovsdb/monitor.c
>> +++ b/ovsdb/monitor.c
>> @@ -1327,6 +1327,7 @@ ovsdb_monitor_table_add_select(struct ovsdb_monitor
>> *dbmon,
>> struct ovsdb_monitor_table * mt;
>>
>> mt = shash_find_data(&dbmon->tables, table->schema->name);
>> + ovs_assert(mt);
>> mt->select |= select;
>> }
>>
>> @@ -1710,6 +1711,8 @@ ovsdb_monitor_hash(const struct ovsdb_monitor *dbmon,
>> size_t basis)
>> for (i = 0; i < n; i++) {
>> struct ovsdb_monitor_table *mt = nodes[i]->data;
>>
>> + ovs_assert(mt);
>> +
>> basis = hash_pointer(mt->table, basis);
>> basis = hash_3words(mt->select, mt->n_columns, basis);
>>
>> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
>> index 9bad0c8dd..87da41f17 100644
>> --- a/ovsdb/ovsdb-server.c
>> +++ b/ovsdb/ovsdb-server.c
>> @@ -2229,6 +2229,8 @@ save_config(struct server_config *config)
>> static void
>> sset_from_json(struct sset *sset, const struct json *array)
>> {
>> + ovs_assert(array);
>> +
>> size_t i;
>>
>> sset_clear(sset);
>> diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
>> index f67b836d7..625b4ca68 100644
>> --- a/ovsdb/ovsdb.c
>> +++ b/ovsdb/ovsdb.c
>> @@ -40,6 +40,7 @@
>> #include "transaction-forward.h"
>> #include "trigger.h"
>> #include "unixctl.h"
>> +#include "util.h"
>>
>> #include "openvswitch/vlog.h"
>> VLOG_DEFINE_THIS_MODULE(ovsdb);
>> @@ -229,7 +230,7 @@ root_set_size(const struct ovsdb_schema *schema)
>> struct ovsdb_error *
>> ovsdb_schema_from_json(const struct json *json, struct ovsdb_schema
>> **schemap)
>> {
>> - struct ovsdb_schema *schema;
>> + struct ovsdb_schema *schema = NULL;
>> const struct json *name, *tables, *version_json, *cksum;
>> struct ovsdb_error *error;
>> struct shash_node *node;
>> @@ -249,6 +250,9 @@ ovsdb_schema_from_json(const struct json *json, struct
>> ovsdb_schema **schemap)
>> return error;
>> }
>>
>> + ovs_assert(name);
>> + ovs_assert(tables);
These are not optional for a json parser and ovsdb_parser_finish()
checks and fails if they do not exist. I don't think we should
check them here. If we can't trust the parser code, we'll need to
add this kind of checks in many other places as well to be consistent.
I'd like not to do that.
>> +
>> if (version_json) {
>> version = json_string(version_json);
>> if (!ovsdb_is_valid_version(version)) {
>> @@ -282,6 +286,8 @@ ovsdb_schema_from_json(const struct json *json, struct
>> ovsdb_schema **schemap)
>> shash_add(&schema->tables, table->name, table);
>> }
>>
>> + ovs_assert(schema);
If the schema didn't exist, we would crash in the loop above.
Also the ovsdb_schema_create() can't fail, so we should not
need to check it.
>> +
>> /* "isRoot" was not part of the original schema definition. Before it
>> was
>> * added, there was no support for garbage collection. So, for backward
>> * compatibility, if the root set is empty then assume that every table
>> is
>> diff --git a/ovsdb/query.c b/ovsdb/query.c
>> index eebe56412..29cc93093 100644
>> --- a/ovsdb/query.c
>> +++ b/ovsdb/query.c
>> @@ -21,6 +21,7 @@
>> #include "condition.h"
>> #include "row.h"
>> #include "table.h"
>> +#include "util.h"
>>
>> void
>> ovsdb_query(struct ovsdb_table *table, const struct ovsdb_condition *cnd,
>> @@ -91,6 +92,7 @@ ovsdb_query_distinct(struct ovsdb_table *table,
>> struct ovsdb_row_hash hash;
>>
>> ovsdb_row_hash_init(&hash, columns);
>> + ovs_assert(condition);
Here it's also not clear why we're checking in this branch and not
in the other. If we're checking condition, why not checking the
table, for example.
>> ovsdb_query(table, condition, query_distinct_cb, &hash);
>> HMAP_FOR_EACH (node, hmap_node, &hash.rows) {
>> ovsdb_row_set_add_row(results, node->row);
>> diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
>> index 7cf4a851a..4fdc5bcea 100644
>> --- a/ovsdb/transaction.c
>> +++ b/ovsdb/transaction.c
>> @@ -34,6 +34,7 @@
>> #include "storage.h"
>> #include "table.h"
>> #include "uuid.h"
>> +#include "util.h"
>>
>> VLOG_DEFINE_THIS_MODULE(transaction);
>>
>> @@ -576,6 +577,7 @@ ovsdb_txn_update_weak_refs(struct ovsdb_txn *txn
>> OVS_UNUSED,
>> dst_row = CONST_CAST(struct ovsdb_row *,
>> ovsdb_table_get_row(weak->dst_table, &weak->dst));
>>
>> + ovs_assert(dst_row);
>> ovs_assert(!ovsdb_row_find_weak_ref(dst_row, weak));
>> hmap_insert(&dst_row->dst_refs, &weak->dst_node,
>> ovsdb_weak_ref_hash(weak));
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
