[ovs-dev] [PATCH v5 02/20] ovsdb-idl.c: Update conditions when handling change tracking list.

[Han Zhou]

There are places with the pattern:
    if (!ovs_list_is_empty(&row->track_node)) {
        ovs_list_remove(&row->track_node);
    }
    ovs_list_push_back(&row->table->track_list,
                       &row->track_node);

It seems to be trying to prevent double removing the node from a list,
but actually it doesn't, and may be misleading.

The function ovs_list_is_empty() returns true only if the node has been
initialized with ovs_list_init(). If a node is deleted but not
initialized, the check will return false and the above code will continue
deleting it again. But if a node is already initialized, calling
ovs_list_remove() is a no-op. So the check is not necessary and misleading.

In fact there is already a double removal resulted by this code: the function
ovsdb_idl_db_clear() removes the node but it then calls ovsdb_idl_row_destroy()
immediately which removes the node again. It should not result in any real
issue yet since the list was not changed so the second removal just
assigns the pointers with same value.

It is in fact not necessary to remove and then add back to the list, because
the purpose of the change tracking is just to tell if a row is changed
or not. So this patch removes the "check and remove" code before adding
the node to a list, but instead, adding it to the list only if it is
not in a list. This way it ensures the node is added to a list only once
in one idl loop. (ovsdb_idl_db_track_clear() will be called in each
iteration and the check ovs_list_is_empty() will return true at the first
check in next iteration).

Signed-off-by: Han Zhou <hzhou8 at ebay.com>
---
 lib/ovsdb-idl.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index b0ebe8c..4b2bc21 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -557,10 +557,6 @@ ovsdb_idl_db_clear(struct ovsdb_idl_db *db)
             /* No need to do anything with dst_arcs: some node has those arcs
              * as forward arcs and will destroy them itself. */
 
-            if (!ovs_list_is_empty(&row->track_node)) {
-                ovs_list_remove(&row->track_node);
-            }
-
             ovsdb_idl_row_destroy(row);
         }
     }
@@ -2366,11 +2362,10 @@ ovsdb_idl_row_change__(struct ovsdb_idl_row *row, const struct json *row_json,
                         = row->table->change_seqno[change]
                         = row->table->db->change_seqno + 1;
                     if (table->modes[column_idx] & OVSDB_IDL_TRACK) {
-                        if (!ovs_list_is_empty(&row->track_node)) {
-                            ovs_list_remove(&row->track_node);
+                        if (ovs_list_is_empty(&row->track_node)) {
+                            ovs_list_push_back(&row->table->track_list,
+                                               &row->track_node);
                         }
-                        ovs_list_push_back(&row->table->track_list,
-                                       &row->track_node);
                         if (!row->updated) {
                             row->updated = bitmap_allocate(class->n_columns);
                         }
@@ -2912,10 +2907,9 @@ ovsdb_idl_row_destroy(struct ovsdb_idl_row *row)
                 = row->table->change_seqno[OVSDB_IDL_CHANGE_DELETE]
                 = row->table->db->change_seqno + 1;
         }
-        if (!ovs_list_is_empty(&row->track_node)) {
-            ovs_list_remove(&row->track_node);
+        if (ovs_list_is_empty(&row->track_node)) {
+            ovs_list_push_back(&row->table->track_list, &row->track_node);
         }
-        ovs_list_push_back(&row->table->track_list, &row->track_node);
     }
 }
 
-- 
2.1.0