[ovs-dev] [PATCH] ovsdb-idl: fix index row setting with references.

Han Zhou zhouhan at gmail.com
Sun Oct 15 06:53:23 UTC 2017


IDL index should be able to be used without having to be in a
transaction. However, current implementation leads to crash if
a reference type column is being set in an index row for querying
purpose when it is not in a transaction. It is because of the
uninitialized arcs and unnecessary updates of the arcs. This patch
fixes it by telling the column parsing function whether it is for
index row or not, so that when parsing index row, the arcs are not
updated. A new test case is added to cover this scenario.

Signed-off-by: Han Zhou <zhouhan at gmail.com>
---
 lib/ovsdb-idl-provider.h |  6 ++--
 lib/ovsdb-idl.c          | 26 ++++++++++-----
 ovsdb/ovsdb-idlc.in      | 10 +++---
 tests/idltest.ovsschema  | 18 +++++++----
 tests/ovsdb-idl.at       | 26 +++++++++++++++
 tests/test-ovsdb.c       | 83 ++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 148 insertions(+), 21 deletions(-)

diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index a3eccb4..6e030bf 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -90,7 +90,8 @@ struct ovsdb_idl_column {
     char *name;
     struct ovsdb_type type;
     bool is_mutable;
-    void (*parse)(struct ovsdb_idl_row *, const struct ovsdb_datum *);
+    void (*parse)(struct ovsdb_idl_row *, const struct ovsdb_datum *,
+            bool is_index);
     void (*unparse)(struct ovsdb_idl_row *);
 };
 
@@ -154,7 +155,8 @@ struct ovsdb_idl_index {
 struct ovsdb_idl_row *ovsdb_idl_get_row_arc(
     struct ovsdb_idl_row *src,
     const struct ovsdb_idl_table_class *dst_table,
-    const struct uuid *dst_uuid);
+    const struct uuid *dst_uuid,
+    bool is_index);
 
 void ovsdb_idl_txn_verify(const struct ovsdb_idl_row *,
                           const struct ovsdb_idl_column *);
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index af1821b..8ecee93 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -2025,7 +2025,7 @@ ovsdb_idl_row_parse(struct ovsdb_idl_row *row)
 
     for (i = 0; i < class->n_columns; i++) {
         const struct ovsdb_idl_column *c = &class->columns[i];
-        (c->parse)(row, &row->old_datum[i]);
+        (c->parse)(row, &row->old_datum[i], false);
     }
 }
 
@@ -2295,7 +2295,7 @@ ovsdb_idl_index_write_(struct ovsdb_idl_row *const_row,
      }
     row->new_datum[column_idx] = *datum;
     (column->unparse)(row);
-    (column->parse)(row, &row->new_datum[column_idx]);
+    (column->parse)(row, &row->new_datum[column_idx], true);
 }
 
 /* Initializes a row for use in an indexed query.
@@ -2310,6 +2310,9 @@ ovsdb_idl_index_init_row(struct ovsdb_idl * idl,
     row->new_datum = xmalloc(class->n_columns * sizeof *row->new_datum);
     row->written = bitmap_allocate(class->n_columns);
     row->table = ovsdb_idl_table_from_class(idl, class);
+    /* arcs are not used for index row, but it doesn't harm to initialize */
+    ovs_list_init(&row->src_arcs);
+    ovs_list_init(&row->dst_arcs);
     return row;
 }
 
@@ -2325,6 +2328,8 @@ ovsdb_idl_index_destroy_row__(const struct ovsdb_idl_row *row_)
     const struct ovsdb_idl_column *c;
     size_t i;
 
+    ovs_assert(ovs_list_is_empty(&row_->src_arcs));
+    ovs_assert(ovs_list_is_empty(&row_->dst_arcs));
     BITMAP_FOR_EACH_1 (i, class->n_columns, row->written) {
         c = &class->columns[i];
         (c->unparse) (row);
@@ -2717,7 +2722,8 @@ ovsdb_idl_table_from_class(const struct ovsdb_idl *idl,
 struct ovsdb_idl_row *
 ovsdb_idl_get_row_arc(struct ovsdb_idl_row *src,
                       const struct ovsdb_idl_table_class *dst_table_class,
-                      const struct uuid *dst_uuid)
+                      const struct uuid *dst_uuid,
+                      bool is_index)
 {
     struct ovsdb_idl *idl = src->table->idl;
     struct ovsdb_idl_table *dst_table;
@@ -2726,13 +2732,17 @@ ovsdb_idl_get_row_arc(struct ovsdb_idl_row *src,
 
     dst_table = ovsdb_idl_table_from_class(idl, dst_table_class);
     dst = ovsdb_idl_get_row(dst_table, dst_uuid);
-    if (idl->txn) {
-        /* We're being called from ovsdb_idl_txn_write().  We must not update
+    if (idl->txn || is_index) {
+        /* There are two cases we should not udpate any arcs:
+         *
+         * 1. We're being called from ovsdb_idl_txn_write(). We must not update
          * any arcs, because the transaction will be backed out at commit or
          * abort time and we don't want our graph screwed up.
          *
-         * Just return the destination row, if there is one and it has not been
-         * deleted. */
+         * 2. The row is used as an index for querying purpose only.
+         *
+         * In these cases, just return the destination row, if there is one and
+         * it has not been deleted. */
         if (dst && (hmap_node_is_null(&dst->txn_node) || dst->new_datum)) {
             return dst;
         }
@@ -3826,7 +3836,7 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
         ovsdb_datum_clone(&row->new_datum[column_idx], datum, &column->type);
     }
     (column->unparse)(row);
-    (column->parse)(row, &row->new_datum[column_idx]);
+    (column->parse)(row, &row->new_datum[column_idx], false);
     return;
 
 discard_datum:
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 24e86b7..fd87f2b 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -354,7 +354,7 @@ static struct %(s)s *
         for columnName, column in sorted_columns(table):
             print('''
 static void
-%(s)s_parse_%(c)s(struct ovsdb_idl_row *row_, const struct ovsdb_datum *datum)
+%(s)s_parse_%(c)s(struct ovsdb_idl_row *row_, const struct ovsdb_datum *datum, bool is_index OVS_UNUSED)
 {
     struct %(s)s *row = %(s)s_cast(row_);''' % {'s': structName,
                                                 'c': columnName})
@@ -379,13 +379,13 @@ static void
                 if not type.key.ref_table:
                     print("        %s = datum->keys[0].%s;" % (keyVar, type.key.type.to_string()))
                 else:
-                    print("        %s = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->keys[0].uuid));" % (keyVar, prefix, type.key.ref_table.name.lower(), prefix, type.key.ref_table.name.lower()))
+                    print("        %s = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->keys[0].uuid, is_index));" % (keyVar, prefix, type.key.ref_table.name.lower(), prefix, type.key.ref_table.name.lower()))
 
                 if valueVar:
                     if not type.value.ref_table:
                         print("        %s = datum->values[0].%s;" % (valueVar, type.value.type.to_string()))
                     else:
-                        print("        %s = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->values[0].uuid));" % (valueVar, prefix, type.value.ref_table.name.lower(), prefix, type.value.ref_table.name.lower()))
+                        print("        %s = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->values[0].uuid, is_index));" % (valueVar, prefix, type.value.ref_table.name.lower(), prefix, type.value.ref_table.name.lower()))
                 print("    } else {")
                 print("        %s" % type.key.initCDefault(keyVar, type.n_min == 0))
                 if valueVar:
@@ -404,7 +404,7 @@ static void
                 print("    for (size_t i = 0; i < %s; i++) {" % nMax)
                 if type.key.ref_table:
                     print("""\
-        struct %s%s *keyRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->keys[i].uuid));
+        struct %s%s *keyRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->keys[i].uuid, is_index));
         if (!keyRow) {
             continue;
         }\
@@ -414,7 +414,7 @@ static void
                     keySrc = "datum->keys[i].%s" % type.key.type.to_string()
                 if type.value and type.value.ref_table:
                     print("""\
-        struct %s%s *valueRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->values[i].uuid));
+        struct %s%s *valueRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->values[i].uuid, is_index));
         if (!valueRow) {
             continue;
         }\
diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema
index 21d8118..57b6bde 100644
--- a/tests/idltest.ovsschema
+++ b/tests/idltest.ovsschema
@@ -34,7 +34,8 @@
             "min": 0
           }
         }
-      }
+      },
+      "isRoot" : true
     },
     "link2": {
       "columns": {
@@ -50,7 +51,8 @@
             "min": 0
           }
         }
-      }
+      },
+      "isRoot" : true
     },
     "simple": {
       "columns": {
@@ -104,7 +106,8 @@
             "min": 0
           }
         }
-      }
+      },
+      "isRoot" : true
     },
     "simple2" : {
       "columns" : {
@@ -133,7 +136,8 @@
             "max": "unlimited"
           }
         }
-      }
+      },
+      "isRoot" : true
     },
     "simple3" : {
       "columns" : {
@@ -156,14 +160,16 @@
              "max": "unlimited"
           }
         }
-      }
+      },
+      "isRoot" : true
     },
     "simple4" : {
       "columns" : {
         "name" : {
           "type": "string"
         }
-      }
+      },
+      "isRoot" : false
     }
   }
 }
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index a5f75fe..21745ea 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -1596,3 +1596,29 @@ OVSDB_CHECK_IDL_COMPOUND_INDEX_DOUBLE_COLUMN_C([Compound_index, double column te
 006: i=4 s=List001 b=True r=130.000000
 006: i=5 s=List005 b=True r=130.000000
 ])
+
+m4_define([OVSDB_CHECK_IDL_COMPOUND_INDEX_WITH_REF],
+  [AT_SETUP([$1 - C])
+   AT_KEYWORDS([ovsdb server idl compound_index compound_index_with_ref positive $5])
+   AT_CHECK([ovsdb_start_idltest])
+   m4_if([$2], [], [],
+     [AT_CHECK([ovsdb-client transact unix:socket $2], [0], [ignore], [ignore])])
+   AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 -c idl-compound-index-with-ref unix:socket $3],
+            [0], [stdout], [ignore])
+   AT_CHECK([sort stdout | ${PERL} $srcdir/uuidfilt.pl]m4_if([$6],,, [[| $6]]),
+            [0], [$4])
+   OVSDB_SERVER_SHUTDOWN
+   AT_CLEANUP])
+
+OVSDB_CHECK_IDL_COMPOUND_INDEX_WITH_REF([set, simple3 idl-compound-index-with-ref, initially populated],
+[],
+[],
+[[000: After add to other table + set of strong ref
+001: name= uset=[] uref=[[<0>]]
+002: check simple4: not empty
+003: Query using index with reference
+004: name= uset=[] uref=[[<0>]]
+005: After delete
+007: check simple4: empty
+008: End test
+]])
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index 1760268..3636a0e 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -2661,6 +2661,87 @@ do_idl_partial_update_set_column(struct ovs_cmdl_context *ctx)
     return;
 }
 
+static void
+do_idl_compound_index_with_ref(struct ovs_cmdl_context *ctx)
+{
+    struct ovsdb_idl *idl;
+    struct ovsdb_idl_txn *myTxn;
+    const struct idltest_simple3 *myRow;
+    struct idltest_simple4 *myRow2;
+    const struct ovsdb_datum *uset OVS_UNUSED;
+    const struct ovsdb_datum *uref OVS_UNUSED;
+    struct ovsdb_idl_index_cursor cursor;
+    int step = 0;
+
+    idl = ovsdb_idl_create(ctx->argv[1], &idltest_idl_class, false, true);
+    ovsdb_idl_add_table(idl, &idltest_table_simple3);
+    ovsdb_idl_add_column(idl, &idltest_simple3_col_name);
+    ovsdb_idl_add_column(idl, &idltest_simple3_col_uset);
+    ovsdb_idl_add_column(idl, &idltest_simple3_col_uref);
+    ovsdb_idl_add_table(idl, &idltest_table_simple4);
+    ovsdb_idl_add_column(idl, &idltest_simple4_col_name);
+
+    struct ovsdb_idl_index *index;
+    index = ovsdb_idl_create_index(idl, &idltest_table_simple3, "uref");
+    ovsdb_idl_index_add_column(index, &idltest_simple3_col_uref, OVSDB_INDEX_ASC,
+                               NULL);
+
+    ovsdb_idl_get_initial_snapshot(idl);
+
+    ovsdb_idl_initialize_cursor(idl, &idltest_table_simple3, "uref",
+                                &cursor);
+
+    setvbuf(stdout, NULL, _IONBF, 0);
+    ovsdb_idl_run(idl);
+
+    /* Adds to a table and update a strong reference in another table. */
+    myTxn = ovsdb_idl_txn_create(idl);
+    myRow = idltest_simple3_insert(myTxn);
+    myRow2 = idltest_simple4_insert(myTxn);
+    idltest_simple4_set_name(myRow2, "test");
+    idltest_simple3_update_uref_addvalue(myRow, myRow2);
+    ovsdb_idl_txn_commit_block(myTxn);
+    ovsdb_idl_txn_destroy(myTxn);
+    ovsdb_idl_get_initial_snapshot(idl);
+    printf("%03d: After add to other table + set of strong ref\n", step++);
+    dump_simple3(idl, myRow, step++);
+
+    myRow2 = (struct idltest_simple4*)idltest_simple4_first(idl);
+    printf("%03d: check simple4: %s\n", step++,
+           myRow2 ? "not empty" : "empty");
+
+    /* Use index to query the row with reference */
+
+    struct idltest_simple3 *equal;
+    equal = idltest_simple3_index_init_row(idl, &idltest_table_simple3);
+    myRow2 = (struct idltest_simple4*)idltest_simple4_first(idl);
+    idltest_simple3_index_set_uref(equal, &myRow2, 1);
+    printf("%03d: Query using index with reference\n", step++);
+    IDLTEST_SIMPLE3_FOR_EACH_EQUAL (myRow, &cursor, equal) {
+        print_idl_row_simple3(myRow, step++);
+    }
+    idltest_simple3_index_destroy_row(equal);
+
+    /* Delete the row with reference */
+    myTxn = ovsdb_idl_txn_create(idl);
+    myRow = idltest_simple3_first(idl);
+    idltest_simple3_delete(myRow);
+    ovsdb_idl_txn_commit_block(myTxn);
+    ovsdb_idl_txn_destroy(myTxn);
+    ovsdb_idl_get_initial_snapshot(idl);
+    printf("%03d: After delete\n", step++);
+    dump_simple3(idl, myRow, step++);
+
+    myRow2 = (struct idltest_simple4*)idltest_simple4_first(idl);
+    printf("%03d: check simple4: %s\n", step++,
+           myRow2 ? "not empty" : "empty");
+
+    ovsdb_idl_destroy(idl);
+    printf("%03d: End test\n", step);
+    return;
+}
+
+
 static int
 test_idl_compound_index_single_column(struct ovsdb_idl *idl,
         struct ovsdb_idl_index_cursor *sCursor,
@@ -2962,6 +3043,8 @@ static struct ovs_cmdl_command all_commands[] = {
     { "trigger", NULL, 2, INT_MAX, do_trigger, OVS_RO },
     { "idl", NULL, 1, INT_MAX, do_idl, OVS_RO },
     { "idl-compound-index", NULL, 2, 2, do_idl_compound_index, OVS_RW },
+    { "idl-compound-index-with-ref", NULL, 1, INT_MAX,
+        do_idl_compound_index_with_ref, OVS_RO },
     { "idl-partial-update-map-column", NULL, 1, INT_MAX,
         do_idl_partial_update_map_column, OVS_RO },
     { "idl-partial-update-set-column", NULL, 1, INT_MAX,
-- 
2.1.0



More information about the dev mailing list