| Message ID | 20230604173007.1005728-5-jamestiotio@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | treewide: Fix multiple Coverity defects | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Mon, Jun 05, 2023 at 01:30:03AM +0800, James Raphael Tiovalen wrote: > This commit adds a few null pointer assertions and checks to some return > values of `ovsdb_table_schema_get_column`. If a null pointer is > encountered in these blocks, either the assertion will fail or the > control flow will now be redirected to alternative paths which will > output the appropriate error messages. > > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> > Reviewed-by: Simon Horman <simon.horman@corigine.com> Hi James, unfortunately the CI is showing a number of failures with this patch applied. I haven't dug into them, but the first is the "ovsdb-server rbac" test for the "linux gcc test" run. Link: https://github.com/ovsrobot/ovs/actions/runs/5170413012/jobs/9313335549#step:11:5574
Hi Simon, On Mon, Jun 12, 2023 at 10:56 PM Simon Horman <simon.horman@corigine.com> wrote: > > On Mon, Jun 05, 2023 at 01:30:03AM +0800, James Raphael Tiovalen wrote: > > This commit adds a few null pointer assertions and checks to some return > > values of `ovsdb_table_schema_get_column`. If a null pointer is > > encountered in these blocks, either the assertion will fail or the > > control flow will now be redirected to alternative paths which will > > output the appropriate error messages. > > > > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > Hi James, > > unfortunately the CI is showing a number of failures with this patch > applied. I haven't dug into them, but the first is the "ovsdb-server rbac" > test for the "linux gcc test" run. > > Link: https://github.com/ovsrobot/ovs/actions/runs/5170413012/jobs/9313335549#step:11:5574 I have identified the cause of the test failures. The commit d56366bfa05b90e7b610716ebf9164bfd06e25f1 added a check for ovsdb-server logs for any warnings or errors. Since this patch adds an additional error message in the `ovsdb_util_write_string_string_column` function, which is unexpected from the perspective of some of those tests, they fail. To fix this, we can simply add the error messages as expected to the ALLOWLIST of the OVSDB_SERVER_SHUTDOWN statements within the tests. That said, I do have one question before I submit the next version of this patch. Should I change the log severity from an error to a warning instead since it seems that there are test cases that will print the log message? Or should I keep it as an error? Best regards, James Raphael Tiovalen
On Wed, Jun 14, 2023 at 12:09:42AM +0800, James R T wrote: > Hi Simon, > > On Mon, Jun 12, 2023 at 10:56 PM Simon Horman <simon.horman@corigine.com> wrote: > > > > On Mon, Jun 05, 2023 at 01:30:03AM +0800, James Raphael Tiovalen wrote: > > > This commit adds a few null pointer assertions and checks to some return > > > values of `ovsdb_table_schema_get_column`. If a null pointer is > > > encountered in these blocks, either the assertion will fail or the > > > control flow will now be redirected to alternative paths which will > > > output the appropriate error messages. > > > > > > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> > > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > > > Hi James, > > > > unfortunately the CI is showing a number of failures with this patch > > applied. I haven't dug into them, but the first is the "ovsdb-server rbac" > > test for the "linux gcc test" run. > > > > Link: https://github.com/ovsrobot/ovs/actions/runs/5170413012/jobs/9313335549#step:11:5574 > > I have identified the cause of the test failures. The commit > d56366bfa05b90e7b610716ebf9164bfd06e25f1 added a check for > ovsdb-server logs for any warnings or errors. Since this patch adds an > additional error message in the > `ovsdb_util_write_string_string_column` function, which is unexpected > from the perspective of some of those tests, they fail. > > To fix this, we can simply add the error messages as expected to the > ALLOWLIST of the OVSDB_SERVER_SHUTDOWN statements within the tests. Thanks, that sounds sensible to me. > That said, I do have one question before I submit the next version of > this patch. Should I change the log severity from an error to a > warning instead since it seems that there are test cases that will > print the log message? Or should I keep it as an error? I guess the distinction between an error and a warning is somewhat subjective. If the log is occurring during tests does it indicate that it's something that can occur during normal usage. And doesn't adversely affect the overall running of the system?. Then perhaps it is more of a warning than an error. But is just my opinion.
On Wed, Jun 14, 2023 at 1:38 AM Simon Horman <simon.horman@corigine.com> wrote: > > > That said, I do have one question before I submit the next version of > > this patch. Should I change the log severity from an error to a > > warning instead since it seems that there are test cases that will > > print the log message? Or should I keep it as an error? > > I guess the distinction between an error and a warning > is somewhat subjective. > > If the log is occurring during tests does it indicate that it's something > that can occur during normal usage. And doesn't adversely affect the > overall running of the system?. Then perhaps it is more of a warning than > an error. But is just my opinion. Understood Simon. Will change it to a warning for now since it should not adversely impact the system. If there are any further valid concerns in the future, we can always change it back to an error. Best regards, James Raphael Tiovalen
diff --git a/ovsdb/condition.c b/ovsdb/condition.c index d0016fa7f..a1a009bfc 100644 --- a/ovsdb/condition.c +++ b/ovsdb/condition.c @@ -47,7 +47,10 @@ 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"); + ovs_assert(uuid_column); + clause->column = uuid_column; clause->index = clause->column->index; ovsdb_datum_init_default(&clause->arg, &clause->column->type); return NULL; diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c index bae2c5f04..46484630d 100644 --- a/ovsdb/ovsdb-client.c +++ b/ovsdb/ovsdb-client.c @@ -1232,8 +1232,11 @@ 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"); + + ovs_assert(version_column); + add_column(server, version_column, columns, columns_json); } if (!initial || !insert || !delete || !modify) { diff --git a/ovsdb/ovsdb-util.c b/ovsdb/ovsdb-util.c index 303191dc8..d287a6be3 100644 --- a/ovsdb/ovsdb-util.c +++ b/ovsdb/ovsdb-util.c @@ -291,9 +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); + goto unwind; + } datum = ovsdb_util_get_datum(row, column_name, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING, UINT_MAX); if (!datum) { +unwind: for (i = 0; i < n; i++) { free(keys[i]); free(values[i]);