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

Han Zhou zhouhan at gmail.com
Sat Oct 28 00:52:09 UTC 2017


On Thu, Oct 26, 2017 at 2:28 PM, Ben Pfaff <blp at ovn.org> 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 <zhouhan at gmail.com>
>
> 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


More information about the dev mailing list