[ovs-dev,v12,5/8] file, monitor: Add null pointer assertions for old and new ovsdb_rows

This commit adds non-null pointer assertions in some code that performs
some decisions based on old and new input ovsdb_rows.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
 ovsdb/file.c    | 2 ++
 ovsdb/monitor.c | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:

> This commit adds non-null pointer assertions in some code that performs
> some decisions based on old and new input ovsdb_rows.
>
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>

What about error messages/argument checking instead of asserts here?

//Eelco

> ---
>  ovsdb/file.c    | 2 ++
>  ovsdb/monitor.c | 4 +++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/ovsdb/file.c b/ovsdb/file.c
> index 2d887e53e..b1d386e76 100644
> --- a/ovsdb/file.c
> +++ b/ovsdb/file.c
> @@ -522,7 +522,9 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
>      }
>
>      if (row) {
> +        ovs_assert(new || old);
>          struct ovsdb_table *table = new ? new->table : old->table;
> +        ovs_assert(table);
>          char uuid[UUID_LEN + 1];
>
>          if (table != ftxn->table) {
> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> index 4afaa89f4..c32af7b02 100644
> --- a/ovsdb/monitor.c
> +++ b/ovsdb/monitor.c
> @@ -1372,8 +1372,10 @@ ovsdb_monitor_changes_update(const struct ovsdb_row *old,
>                               const struct ovsdb_monitor_table *mt,
>                               struct ovsdb_monitor_change_set_for_table *mcst)
>  {
> +    ovs_assert(new || old);
>      const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old);
> -    struct ovsdb_monitor_row *change;
> +    ovs_assert(uuid);
> +    struct ovsdb_monitor_row *change = NULL;
>
>      change = ovsdb_monitor_changes_row_find(mcst, uuid);
>      if (!change) {
> -- 
> 2.40.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 7/11/23 12:17, Eelco Chaudron wrote:
> 
> 
> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> 
>> This commit adds non-null pointer assertions in some code that performs
>> some decisions based on old and new input ovsdb_rows.
>>
>> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> 
> What about error messages/argument checking instead of asserts here?

File transactions must have some data.  Every row must have at least
uuid and version.  New rows should have 'new' versions of the data,
deleted should have 'old', and modified should have both.  It's a
bug somewhere if we have a row without any data.

> 
> //Eelco
> 
>> ---
>>  ovsdb/file.c    | 2 ++
>>  ovsdb/monitor.c | 4 +++-
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/ovsdb/file.c b/ovsdb/file.c
>> index 2d887e53e..b1d386e76 100644
>> --- a/ovsdb/file.c
>> +++ b/ovsdb/file.c
>> @@ -522,7 +522,9 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
>>      }
>>
>>      if (row) {
>> +        ovs_assert(new || old);
>>          struct ovsdb_table *table = new ? new->table : old->table;
>> +        ovs_assert(table);
>>          char uuid[UUID_LEN + 1];
>>
>>          if (table != ftxn->table) {
>> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
>> index 4afaa89f4..c32af7b02 100644
>> --- a/ovsdb/monitor.c
>> +++ b/ovsdb/monitor.c
>> @@ -1372,8 +1372,10 @@ ovsdb_monitor_changes_update(const struct ovsdb_row *old,
>>                               const struct ovsdb_monitor_table *mt,
>>                               struct ovsdb_monitor_change_set_for_table *mcst)
>>  {
>> +    ovs_assert(new || old);
>>      const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old);
>> -    struct ovsdb_monitor_row *change;
>> +    ovs_assert(uuid);
>> +    struct ovsdb_monitor_row *change = NULL;
>>
>>      change = ovsdb_monitor_changes_row_find(mcst, uuid);
>>      if (!change) {
>> -- 
>> 2.40.1

On 11 Jul 2023, at 16:38, Ilya Maximets wrote:

> On 7/11/23 12:17, Eelco Chaudron wrote:
>>
>>
>> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
>>
>>> This commit adds non-null pointer assertions in some code that performs
>>> some decisions based on old and new input ovsdb_rows.
>>>
>>> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
>>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>>
>> What about error messages/argument checking instead of asserts here?
>
> File transactions must have some data.  Every row must have at least
> uuid and version.  New rows should have 'new' versions of the data,
> deleted should have 'old', and modified should have both.  It's a
> bug somewhere if we have a row without any data.


Thanks for confirming/clarification. With this;

Acked-by: Eelco Chaudron <echaudro@redhat.com>



>>
>> //Eelco
>>
>>> ---
>>>  ovsdb/file.c    | 2 ++
>>>  ovsdb/monitor.c | 4 +++-
>>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/ovsdb/file.c b/ovsdb/file.c
>>> index 2d887e53e..b1d386e76 100644
>>> --- a/ovsdb/file.c
>>> +++ b/ovsdb/file.c
>>> @@ -522,7 +522,9 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
>>>      }
>>>
>>>      if (row) {
>>> +        ovs_assert(new || old);
>>>          struct ovsdb_table *table = new ? new->table : old->table;
>>> +        ovs_assert(table);
>>>          char uuid[UUID_LEN + 1];
>>>
>>>          if (table != ftxn->table) {
>>> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
>>> index 4afaa89f4..c32af7b02 100644
>>> --- a/ovsdb/monitor.c
>>> +++ b/ovsdb/monitor.c
>>> @@ -1372,8 +1372,10 @@ ovsdb_monitor_changes_update(const struct ovsdb_row *old,
>>>                               const struct ovsdb_monitor_table *mt,
>>>                               struct ovsdb_monitor_change_set_for_table *mcst)
>>>  {
>>> +    ovs_assert(new || old);
>>>      const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old);
>>> -    struct ovsdb_monitor_row *change;
>>> +    ovs_assert(uuid);
>>> +    struct ovsdb_monitor_row *change = NULL;
>>>
>>>      change = ovsdb_monitor_changes_row_find(mcst, uuid);
>>>      if (!change) {
>>> -- 
>>> 2.40.1

On 7/11/23 17:08, Eelco Chaudron wrote:
> 
> 
> On 11 Jul 2023, at 16:38, Ilya Maximets wrote:
> 
>> On 7/11/23 12:17, Eelco Chaudron wrote:
>>>
>>>
>>> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
>>>
>>>> This commit adds non-null pointer assertions in some code that performs
>>>> some decisions based on old and new input ovsdb_rows.
>>>>
>>>> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
>>>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>>>
>>> What about error messages/argument checking instead of asserts here?
>>
>> File transactions must have some data.  Every row must have at least
>> uuid and version.  New rows should have 'new' versions of the data,
>> deleted should have 'old', and modified should have both.  It's a
>> bug somewhere if we have a row without any data.
> 
> 
> Thanks for confirming/clarification. With this;
> 
> Acked-by: Eelco Chaudron <echaudro@redhat.com>

Thanks, James, Simon and Eelco!

Applied this one.

Best regards, Ilya Maximets.