Skip to content

Commit

Permalink
ovsdb-idl: Fix iteration over tracked rows with no actual data.
Browse files Browse the repository at this point in the history 
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
    openvswitch#1 0x55e903 in handle_deleted_vif_lport /controller/binding.c:2072:5
    openvswitch#2 0x55e059 in binding_handle_port_binding_changes /controller/binding.c:2155:23
    openvswitch#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
    openvswitch#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: 932104f ("ovsdb-idl: Add support for change tracking.")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: 0-day Robot <robot@bytheb.org>
  • Loading branch information
@igsilya @ovsrobot
igsilya authored and ovsrobot committed Nov 23, 2020
1 parent 7bfb195 commit 36e6b81
Show file tree
Hide file tree
Showing 4 changed files with 140 additions and 13 deletions.
22 changes: 15 additions & 7 deletions lib/ovsdb-idl.c
View file
Expand Up @@ -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;
}

Expand Down
15 changes: 15 additions & 0 deletions tests/idltest.ovsschema
View file
Expand Up @@ -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" : {
Expand Down
55 changes: 55 additions & 0 deletions tests/ovsdb-idl.at
View file
Expand Up @@ -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?)
])
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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>
Expand Down
61 changes: 55 additions & 6 deletions tests/test-ovsdb.c
View file
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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++;
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
}
Expand Down

0 comments on commit 36e6b81

Please sign in to comment.