On 4/24/23 13:53, Simon Horman wrote:
> On Sat, Apr 15, 2023 at 11:21:51PM +0800, James Raphael Tiovalen wrote:
>> This commit adds more verbose logging in null-pointer check blocks by
>> using `ovsdb_syntax_error` and `VLOG_ERR`. A few additional null pointer
>> checks are also performed to accompany the additional logging. If a null
>> pointer is encountered in these blocks, the control flow will now be
>> redirected to alternative paths which will output the appropriate error
>> messages.
>>
>> Signed-off-by: James Raphael Tiovalen <[email protected]>
>> ---
>> ovsdb/condition.c | 10 +++++++++-
>> ovsdb/monitor.c | 3 ++-
>> ovsdb/ovsdb-client.c | 10 ++++++++--
>> ovsdb/ovsdb-util.c | 9 +++++++++
>> ovsdb/table.c | 2 +-
>> 5 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/ovsdb/condition.c b/ovsdb/condition.c
>> index d0016fa7f..ada8f50dc 100644
>> --- a/ovsdb/condition.c
>> +++ b/ovsdb/condition.c
>> @@ -47,7 +47,15 @@ ovsdb_clause_from_json(const struct ovsdb_table_schema
>> *ts,
>>
>> /* Column and arg fields are not being used with boolean functions.
>> * Use dummy values */
>> - clause->column = ovsdb_table_schema_get_column(ts, "_uuid");
>> + const struct ovsdb_column *uuid_column =
>> + ovsdb_table_schema_get_column(ts,
>> "_uuid");
>> + if (uuid_column) {
>> + clause->column = uuid_column;
>> + } else {
>> + return ovsdb_syntax_error(json,
>> + "unknown column",
>> + "No column _uuid in table schema.");
>> + }
>
> Hi James,
>
> FWIIW, I would lean towards writing this as:
>
> if (!uuid_column) {
> return ovsdb_syntax_error(json,
> "unknown column",
> "No column _uuid in table schema.");
> }
> clause->column = uuid_column;
Every table must have a _uuid column, it's internal and should always be there.
So, I'd replace this with just an assertion.
>
>> clause->index = clause->column->index;
>> ovsdb_datum_init_default(&clause->arg, &clause->column->type);
>> return NULL;
>> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
>> index 191befcae..4a0d6512f 100644
>> --- a/ovsdb/monitor.c
>> +++ b/ovsdb/monitor.c
>> @@ -781,7 +781,8 @@ ovsdb_monitor_table_condition_update(
>> const struct json *cond_json)
>> {
>> if (!condition) {
>> - return NULL;
>> + return ovsdb_syntax_error(cond_json, NULL,
>> + "Parse error, condition empty.");
This changes behavior of ovsdb-server when user is sending cond_change
request without previously setting any conditions, i.e. the monitor v1
is in use. Documentation doesn't say what should be done in this case.
Right now the server will return a success reply without doing anything.
It's arguable if it is good or bad, but a) we probably need a better
error message for the user, b) its unclear why that would be flagged by
Coverity as an issue as it doesn't look like there could be any potential
NULL dereference of something like that.
>> }
>>
>> struct ovsdb_monitor_table_condition *mtc =
>> diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
>> index bae2c5f04..41dfeb3b6 100644
>> --- a/ovsdb/ovsdb-client.c
>> +++ b/ovsdb/ovsdb-client.c
>> @@ -1232,8 +1232,14 @@ parse_monitor_columns(char *arg, const char *server,
>> const char *database,
>> }
>> free(nodes);
>>
>> - add_column(server, ovsdb_table_schema_get_column(table, "_version"),
>> - columns, columns_json);
>> + const struct ovsdb_column *version_column =
>> + ovsdb_table_schema_get_column(table,
>> "_version");
>> +
>> + if (version_column) {
>> + add_column(server, version_column, columns, columns_json);
>> + } else {
>> + VLOG_ERR("Table does not contain _version column.");
>> + }
Same here. '_version' is an internal column that should always be there.
>> }
>>
>> if (!initial || !insert || !delete || !modify) {
>> diff --git a/ovsdb/ovsdb-util.c b/ovsdb/ovsdb-util.c
>> index 303191dc8..148230da8 100644
>> --- a/ovsdb/ovsdb-util.c
>> +++ b/ovsdb/ovsdb-util.c
>> @@ -291,6 +291,15 @@ ovsdb_util_write_string_string_column(struct ovsdb_row
>> *row,
>> size_t i;
>>
>> column = ovsdb_table_schema_get_column(row->table->schema, column_name);
>> + if (!column) {
>> + VLOG_ERR("No %s column present in the %s table",
>> + column_name, row->table->schema->name);
>
> The unwind logic below seems to already exist in this function.
> So rather than duplicating it, could it be moved to a goto label?
>
>> + for (i = 0; i < n; i++) {
>> + free(keys[i]);
>> + free(values[i]);
>> + }
>> + return;
>> + }
>> datum = ovsdb_util_get_datum(row, column_name, OVSDB_TYPE_STRING,
>> OVSDB_TYPE_STRING, UINT_MAX);
>> if (!datum) {
>> diff --git a/ovsdb/table.c b/ovsdb/table.c
>> index 66071ce2f..990d49ea4 100644
>> --- a/ovsdb/table.c
>> +++ b/ovsdb/table.c
>> @@ -158,7 +158,7 @@ ovsdb_table_schema_from_json(const struct json *json,
>> const char *name,
>> n_max_rows = UINT_MAX;
>> }
>>
>> - if (shash_is_empty(json_object(columns))) {
>> + if (!columns || shash_is_empty(json_object(columns))) {
'columns' is not an optional member. Parsing should fail if it
is not present, i.e. we should not be able to get to this line
if columns is NULL. It looks strange that we need to check the
pointers that parser did already check.
>> return ovsdb_syntax_error(json, NULL,
>> "table must have at least one column");
>> }
>> --
>> 2.40.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
