[ovs-dev] [PATCH] ovsdb-idl: Fix iteration over tracked rows with no actual data.

Ilya Maximets i.maximets at ovn.org
Mon Nov 23 08:37:47 UTC 2020


When idl removes orphan rows, those rows are inserted into the
'track_list'.  This allows iterators such as *_FOR_EACH_TRACKED () to
return orphan rows that never had any data to the IDL user.  In this
case, it is difficult for the user to understand whether it is a row
with no data (there was no "insert" / "modify" for this row) or it is
a row with zero data (columns were cleared by DB transaction).

The main problem with this condition is that rows without data will
have NULL pointers instead of references that should be there according
to the database schema.  For example, ovn-controller might crash:

 ERROR: AddressSanitizer: SEGV on unknown address 0x000000000100
       (pc 0x00000055e9b2 bp 0x7ffef6180880 sp 0x7ffef6180860 T0)
 The signal is caused by a READ memory access.
 Hint: address points to the zero page.
    #0 0x55e9b1 in handle_deleted_lport /controller/binding.c
    #1 0x55e903 in handle_deleted_vif_lport /controller/binding.c:2072:5
    #2 0x55e059 in binding_handle_port_binding_changes /controller/binding.c:2155:23
    #3 0x5a6395 in runtime_data_sb_port_binding_handler /controller/ovn-controller.c:1454:10
    #4 0x5e15b3 in engine_compute /lib/inc-proc-eng.c:306:18
    #5 0x5e0faf in engine_run_node /lib/inc-proc-eng.c:352:14
    #6 0x5e0e04 in engine_run /lib/inc-proc-eng.c:377:9
    #7 0x5a03de in main /controller/ovn-controller.c
    #8 0x7f4fd9c991a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
    #9 0x483f0d in _start (/controller/ovn-controller+0x483f0d)

It doesn't make much sense to return non-real rows to the user, so it's
best to exclude them from iteration.

Test included.  Without the fix, provided test will print empty orphan
rows that was never received by idl as tracked changes.

Fixes: 932104f483ef ("ovsdb-idl: Add support for change tracking.")
Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
---
 lib/ovsdb-idl.c         | 22 ++++++++++-----
 tests/idltest.ovsschema | 15 ++++++++++
 tests/ovsdb-idl.at      | 55 +++++++++++++++++++++++++++++++++++++
 tests/test-ovsdb.c      | 61 +++++++++++++++++++++++++++++++++++++----
 4 files changed, 140 insertions(+), 13 deletions(-)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 6334061b4..23648ff6b 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -1892,29 +1892,37 @@ ovsdb_idl_track_is_set(struct ovsdb_idl_table *table)
 }
 
 /* Returns the first tracked row in table with class 'table_class'
- * for the specified 'idl'. Returns NULL if there are no tracked rows */
+ * for the specified 'idl'. Returns NULL if there are no tracked rows.
+ * Pure orphan rows, i.e. rows that never had any datum, are skipped. */
 const struct ovsdb_idl_row *
 ovsdb_idl_track_get_first(const struct ovsdb_idl *idl,
                           const struct ovsdb_idl_table_class *table_class)
 {
     struct ovsdb_idl_table *table
         = ovsdb_idl_db_table_from_class(&idl->data, table_class);
+    struct ovsdb_idl_row *row;
 
-    if (!ovs_list_is_empty(&table->track_list)) {
-        return CONTAINER_OF(ovs_list_front(&table->track_list), struct ovsdb_idl_row, track_node);
+    LIST_FOR_EACH (row, track_node, &table->track_list) {
+        if (!ovsdb_idl_row_is_orphan(row) || row->tracked_old_datum) {
+            return row;
+        }
     }
     return NULL;
 }
 
 /* Returns the next tracked row in table after the specified 'row'
- * (in no particular order). Returns NULL if there are no tracked rows */
+ * (in no particular order). Returns NULL if there are no tracked rows.
+ * Pure orphan rows, i.e. rows that never had any datum, are skipped.*/
 const struct ovsdb_idl_row *
 ovsdb_idl_track_get_next(const struct ovsdb_idl_row *row)
 {
-    if (row->track_node.next != &row->table->track_list) {
-        return CONTAINER_OF(row->track_node.next, struct ovsdb_idl_row, track_node);
-    }
+    struct ovsdb_idl_table *table = row->table;
 
+    LIST_FOR_EACH_CONTINUE (row, track_node, &table->track_list) {
+        if (!ovsdb_idl_row_is_orphan(row) || row->tracked_old_datum) {
+            return row;
+        }
+    }
     return NULL;
 }
 
diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema
index e04755ea0..3ddb612b0 100644
--- a/tests/idltest.ovsschema
+++ b/tests/idltest.ovsschema
@@ -195,6 +195,21 @@
       },
       "isRoot": true
     },
+    "simple6": {
+      "columns" : {
+        "name": {"type": "string"},
+        "weak_ref": {
+          "type": {
+            "key": {"type": "uuid",
+                    "refTable": "simple",
+                    "refType": "weak"},
+            "min": 0,
+            "max": "unlimited"
+          }
+        }
+      },
+      "isRoot": true
+    },
     "singleton" : {
       "columns" : {
         "name" : {
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index cacc82d82..406a57627 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -967,6 +967,7 @@ AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl
 test-ovsdb|ovsdb_idl|idltest database lacks indexed table (database needs upgrade?)
 test-ovsdb|ovsdb_idl|idltest database lacks link2 table (database needs upgrade?)
 test-ovsdb|ovsdb_idl|idltest database lacks simple5 table (database needs upgrade?)
+test-ovsdb|ovsdb_idl|idltest database lacks simple6 table (database needs upgrade?)
 test-ovsdb|ovsdb_idl|idltest database lacks singleton table (database needs upgrade?)
 test-ovsdb|ovsdb_idl|link1 table in idltest database lacks l2 column (database needs upgrade?)
 ])
@@ -1171,6 +1172,59 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated],
 003: done
 ]])
 
+dnl This test creates database with weak references and checks that orphan
+dnl rows created for weak references are not available for iteration via
+dnl list of tracked changes.
+OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references],
+  [['["idltest",
+      {"op": "insert",
+       "table": "simple",
+       "row": {"s": "row0_s"},
+       "uuid-name": "weak_row0"},
+      {"op": "insert",
+       "table": "simple",
+       "row": {"s": "row1_s"},
+       "uuid-name": "weak_row1"},
+      {"op": "insert",
+       "table": "simple",
+       "row": {"s": "row2_s"},
+       "uuid-name": "weak_row2"},
+      {"op": "insert",
+       "table": "simple6",
+       "row": {"name": "first_row",
+               "weak_ref": ["set",
+                             [["named-uuid", "weak_row0"],
+                              ["named-uuid", "weak_row1"],
+                              ["named-uuid", "weak_row2"]]
+                           ]}}]']],
+  [['condition simple []' \
+    'condition simple [["s","==","row1_s"]]' \
+    '["idltest",
+      {"op": "update",
+      "table": "simple6",
+      "where": [],
+      "row": {"name": "new_name"}}]' \
+    '["idltest",
+      {"op": "delete",
+      "table": "simple6",
+      "where": []}]']],
+  [[000: change conditions
+001: inserted row: uuid=<0>
+001: name=first_row weak_ref=[] uuid=<0>
+001: updated columns: name weak_ref
+002: change conditions
+003: i=0 r=0 b=false s=row1_s u=<1> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<2>
+003: inserted row: uuid=<2>
+003: name=first_row weak_ref=[<2>] uuid=<0>
+003: updated columns: s
+004: {"error":null,"result":[{"count":1}]}
+005: name=new_name weak_ref=[<2>] uuid=<0>
+005: updated columns: name
+006: {"error":null,"result":[{"count":1}]}
+007: i=0 r=0 b=false s=row1_s u=<1> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<2>
+008: done
+]])
+
 OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops],
   [],
   [['["idltest",
@@ -1246,6 +1300,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops],
 010: updated columns: s
 011: {"error":null,"result":[{"count":1}]}
 012: deleted row: uuid=<1>
+012: i=0 r=123.5 b=true s=newstring u=<5> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 013: reconnect
 014: i=-1 r=125 b=false s=newstring u=<5> ia=[1] ra=[1.5] ba=[false] sa=[] ua=[] uuid=<6>
 014: i=1 r=123.5 b=true s=mystring u=<2> ia=[1 2 3] ra=[-0.5] ba=[true] sa=[abc def] ua=[<3> <4>] uuid=<0>
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index aade40f3f..31513c537 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -1904,6 +1904,26 @@ print_idl_row_updated_link2(const struct idltest_link2 *l2, int step)
     }
 }
 
+static void
+print_idl_row_updated_simple6(const struct idltest_simple6 *s6, int step)
+{
+    size_t i;
+    bool updated = false;
+
+    for (i = 0; i < IDLTEST_SIMPLE6_N_COLUMNS; i++) {
+        if (idltest_simple6_is_updated(s6, i)) {
+            if (!updated) {
+                printf("%03d: updated columns:", step);
+                updated = true;
+            }
+            printf(" %s", idltest_simple6_columns[i].name);
+        }
+    }
+    if (updated) {
+        printf("\n");
+    }
+}
+
 static void
 print_idl_row_updated_singleton(const struct idltest_singleton *sng, int step)
 {
@@ -1991,6 +2011,22 @@ print_idl_row_link2(const struct idltest_link2 *l2, int step)
     print_idl_row_updated_link2(l2, step);
 }
 
+static void
+print_idl_row_simple6(const struct idltest_simple6 *s6, int step)
+{
+    int i;
+
+    printf("%03d: name=%s ", step, s6->name);
+    printf("weak_ref=[");
+    for (i = 0; i < s6->n_weak_ref; i++) {
+        printf("%s"UUID_FMT, i ? " " : "",
+               UUID_ARGS(&s6->weak_ref[i]->header_.uuid));
+    }
+
+    printf("] uuid="UUID_FMT"\n", UUID_ARGS(&s6->header_.uuid));
+    print_idl_row_updated_simple6(s6, step);
+}
+
 static void
 print_idl_row_singleton(const struct idltest_singleton *sng, int step)
 {
@@ -2032,21 +2068,20 @@ print_idl(struct ovsdb_idl *idl, int step)
 static void
 print_idl_track(struct ovsdb_idl *idl, int step)
 {
+    const struct idltest_simple6 *s6;
     const struct idltest_simple *s;
     const struct idltest_link1 *l1;
     const struct idltest_link2 *l2;
     int n = 0;
 
     IDLTEST_SIMPLE_FOR_EACH_TRACKED (s, idl) {
+        print_idl_row_simple(s, step);
         if (idltest_simple_is_deleted(s)) {
             printf("%03d: deleted row: uuid="UUID_FMT"\n", step,
                    UUID_ARGS(&s->header_.uuid));
-        } else {
-            print_idl_row_simple(s, step);
-            if (idltest_simple_is_new(s)) {
-                printf("%03d: inserted row: uuid="UUID_FMT"\n", step,
-                       UUID_ARGS(&s->header_.uuid));
-            }
+        } else if (idltest_simple_is_new(s)) {
+            printf("%03d: inserted row: uuid="UUID_FMT"\n", step,
+                   UUID_ARGS(&s->header_.uuid));
         }
         n++;
     }
@@ -2077,6 +2112,18 @@ print_idl_track(struct ovsdb_idl *idl, int step)
         }
         n++;
     }
+    IDLTEST_SIMPLE6_FOR_EACH_TRACKED (s6, idl) {
+        print_idl_row_simple6(s6, step);
+        if (idltest_simple6_is_deleted(s6)) {
+            printf("%03d: deleted row: uuid="UUID_FMT"\n", step,
+                   UUID_ARGS(&s6->header_.uuid));
+        } else if (idltest_simple6_is_new(s6)) {
+            printf("%03d: inserted row: uuid="UUID_FMT"\n", step,
+                   UUID_ARGS(&s6->header_.uuid));
+        }
+        n++;
+    }
+
     if (!n) {
         printf("%03d: empty\n", step);
     }
@@ -2298,6 +2345,8 @@ find_table_class(const char *name)
         return &idltest_table_link1;
     } else if (!strcmp(name, "link2")) {
         return &idltest_table_link2;
+    } else if (!strcmp(name, "simple6")) {
+        return &idltest_table_simple6;
     }
     return NULL;
 }
-- 
2.25.4



More information about the dev mailing list