On Thu, Oct 26, 2017 at 2:28 PM, Ben Pfaff <[email protected]> wrote:
>
> On Sat, Oct 14, 2017 at 11:53:23PM -0700, Han Zhou wrote:
> > 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]>
>
> Thank you for the bug fix. I agree that this fix should go in.
>
> I see at least one place where "update" is misspelled "udpate".
>
> Did you consider whether there is a way to distinguish an index row from
> other rows, without adding a new member? It might be possible to simply
> consider the UUID, since an index row has UUID zero and other rows do
> not. If this is possible, then I think I'd prefer that approach.
Thanks Ben. Yes, this is a better idea. I didn't want to add a new field in
the
row structure, but didn't thought about playing around with uuid. I am not
sure
if there is/will be any cases that a row that is not an index row could get
parsed
before the uuid is generated, so I think it is safer to use a magic UUID to
distinguish
index rows from others. I just submitted V2 for this.
>
> The following incremental fixes some style violations identified by
> "checkpatch".
>
> --8<--------------------------cut here-------------------------->8--
>
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index 3636a0e09e0e..451172cdcc34 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -2683,8 +2683,8 @@ do_idl_compound_index_with_ref(struct
ovs_cmdl_context *ctx)
>
> 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_index_add_column(index, &idltest_simple3_col_uref,
> + OVSDB_INDEX_ASC, NULL);
>
> ovsdb_idl_get_initial_snapshot(idl);
>
> @@ -2706,7 +2706,7 @@ do_idl_compound_index_with_ref(struct
ovs_cmdl_context *ctx)
> 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);
> + myRow2 = (struct idltest_simple4 *) idltest_simple4_first(idl);
> printf("%03d: check simple4: %s\n", step++,
> myRow2 ? "not empty" : "empty");
>
> @@ -2714,7 +2714,7 @@ do_idl_compound_index_with_ref(struct
ovs_cmdl_context *ctx)
>
> struct idltest_simple3 *equal;
> equal = idltest_simple3_index_init_row(idl, &idltest_table_simple3);
> - myRow2 = (struct idltest_simple4*)idltest_simple4_first(idl);
> + 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) {
> @@ -2732,7 +2732,7 @@ do_idl_compound_index_with_ref(struct
ovs_cmdl_context *ctx)
> printf("%03d: After delete\n", step++);
> dump_simple3(idl, myRow, step++);
>
> - myRow2 = (struct idltest_simple4*)idltest_simple4_first(idl);
> + myRow2 = (struct idltest_simple4 *) idltest_simple4_first(idl);
> printf("%03d: check simple4: %s\n", step++,
> myRow2 ? "not empty" : "empty");
>
Now I know how to use checkpatch to make my life easier. Thanks!
Han
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
