[ovs-dev] [PATCH v2 4/9] ovsdb: row: Add support for xor-based row updates.

[Ilya Maximets]

On 6/19/21 8:01 PM, Mark Gray wrote:
> On 12/06/2021 03:00, Ilya Maximets wrote:
>> This will be used to apply update3 type updates to ovsdb tables
>> while processing updates for future ovsdb 'relay' service model.
>>
>> 'ovsdb_datum_apply_diff' is allowed to fail, so adding support
>> to return this error.
>>
>> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
>> ---
>>  ovsdb/execution.c   |  5 +++--
>>  ovsdb/replication.c |  2 +-
>>  ovsdb/row.c         | 30 +++++++++++++++++++++++++-----
>>  ovsdb/row.h         |  6 ++++--
>>  ovsdb/table.c       |  9 +++++----
>>  ovsdb/table.h       |  2 +-
>>  6 files changed, 39 insertions(+), 15 deletions(-)
>>
>> diff --git a/ovsdb/execution.c b/ovsdb/execution.c
>> index 3a0dad5d0..f6150e944 100644
>> --- a/ovsdb/execution.c
>> +++ b/ovsdb/execution.c
>> @@ -483,8 +483,9 @@ update_row_cb(const struct ovsdb_row *row, void *ur_)
>>  
>>      ur->n_matches++;
>>      if (!ovsdb_row_equal_columns(row, ur->row, ur->columns)) {
>> -        ovsdb_row_update_columns(ovsdb_txn_row_modify(ur->txn, row),
>> -                                 ur->row, ur->columns);
>> +        ovsdb_error_assert(ovsdb_row_update_columns(
>> +                               ovsdb_txn_row_modify(ur->txn, row),
>> +                               ur->row, ur->columns, false));
>>      }
>>  
>>      return true;
>> diff --git a/ovsdb/replication.c b/ovsdb/replication.c
>> index b755976b0..d8b56d813 100644
>> --- a/ovsdb/replication.c
>> +++ b/ovsdb/replication.c
>> @@ -677,7 +677,7 @@ process_table_update(struct json *table_update, const char *table_name,
>>          struct ovsdb_error *error;
>>          error = (!new ? ovsdb_table_execute_delete(txn, &uuid, table)
>>                   : !old ? ovsdb_table_execute_insert(txn, &uuid, table, new)
>> -                 : ovsdb_table_execute_update(txn, &uuid, table, new));
>> +                 : ovsdb_table_execute_update(txn, &uuid, table, new, false));
>>          if (error) {
>>              if (!strcmp(ovsdb_error_get_tag(error), "consistency violation")) {
>>                  ovsdb_error_assert(error);
>> diff --git a/ovsdb/row.c b/ovsdb/row.c
>> index 755ab91a8..65a054621 100644
>> --- a/ovsdb/row.c
>> +++ b/ovsdb/row.c
>> @@ -163,20 +163,40 @@ ovsdb_row_equal_columns(const struct ovsdb_row *a,
>>      return true;
>>  }
>>  
>> -void
>> +struct ovsdb_error *
>>  ovsdb_row_update_columns(struct ovsdb_row *dst,
>>                           const struct ovsdb_row *src,
>> -                         const struct ovsdb_column_set *columns)
>> +                         const struct ovsdb_column_set *columns,
>> +                         bool xor)
>>  {
>>      size_t i;
>>  
>>      for (i = 0; i < columns->n_columns; i++) {
>>          const struct ovsdb_column *column = columns->columns[i];
>> +        struct ovsdb_datum xor_datum;
>> +        struct ovsdb_error *error;
>> +
>> +        if (xor) {
>> +            error = ovsdb_datum_apply_diff(&xor_datum,
>> +                                           &dst->fields[column->index],
>> +                                           &src->fields[column->index],
>> +                                           &column->type);
>> +            if (error) {
>> +                return error;
>> +            }
>> +        }
>> +
>>          ovsdb_datum_destroy(&dst->fields[column->index], &column->type);
>> -        ovsdb_datum_clone(&dst->fields[column->index],
>> -                          &src->fields[column->index],
>> -                          &column->type);
> 
> Could you move ovsdb_datum_destroy(&dst->fields[column->index],
> &column->type) into the "else" clause below and then merge the "if"
> clause below into the "if" clause above?

We still need to destroy for both branches, so what I can do is
something like this:

@@ -184,13 +185,11 @@ ovsdb_row_update_columns(struct ovsdb_row *dst,
             if (error) {
                 return error;
             }
-        }
 
-        ovsdb_datum_destroy(&dst->fields[column->index], &column->type);
-
-        if (xor) {
+            ovsdb_datum_destroy(&dst->fields[column->index], &column->type);
             ovsdb_datum_swap(&dst->fields[column->index], &xor_datum);
         } else {
+            ovsdb_datum_destroy(&dst->fields[column->index], &column->type);
             ovsdb_datum_clone(&dst->fields[column->index],
                               &src->fields[column->index],
                               &column->type);
---

i.e. copy the ovsdb_datum_destroy() to both branches and merge the "if"s,
but I found this harder to read.

I'll keep as is for now, but if you think that above version will be better,
I can use it.

Best regards, Ilya Maximets.