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 <[email protected]> --- 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
