| Message ID | 20201020150707.877290-1-mark.d.gray@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v3] ovsdb-idl: Fix *_is_new() IDL functions | expand |
On Tue, Oct 20, 2020 at 8:07 AM Mark Gray <mark.d.gray@redhat.com> wrote: > > Currently all functions of the type *_is_new() always return > 'false'. This patch resolves this issue by using the > 'OVSDB_IDL_CHANGE_INSERT' 'change_seqno' instead of the > 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno' to determine if a row > is new and by resetting the 'OVSDB_IDL_CHANGE_INSERT' > 'change_seqno' on clear. > > Further to this, the code is also updated to match the following > behaviour: > > When a row is inserted, the 'OVSDB_IDL_CHANGE_INSERT' > 'change_seqno' is updated to match the new database > change_seqno. The 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno' > is not set for inserted rows (only for updated rows). > > At the end of a run, ovsdb_idl_db_track_clear() should be > called to clear all tracking information, this includes > resetting all row 'change_seqno' to zero. This will ensure > that subsequent runs will not see a previously 'new' row. > > add_tracked_change_for_references() is updated to only > track rows that reference the current row. > > Also, update unit tests in order to test the *_is_new(), > *_is_delete() functions. > > Suggested-by: Dumitru Ceara <dceara@redhat.com> > Reported-at: https://bugzilla.redhat.com/1883562 > Fixes: ca545a787ac0 ("ovsdb-idl.c: Increase seqno for change-tracking of table references.") > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > --- > v3: Update comments for _is_new(), is_deleted() and is_updated() functions > Removed change that modifies flags on local uncommitted changes > > lib/ovsdb-idl.c | 40 ++++++++++++++++++++++++++++------------ > ovsdb/ovsdb-idlc.in | 22 +++++++++++++++++++--- > tests/ovsdb-idl.at | 5 ++++- > tests/test-ovsdb.c | 32 ++++++++++++++++++++++++-------- > 4 files changed, 75 insertions(+), 24 deletions(-) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index d8f221ca6073..58468d283eef 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -1959,6 +1959,11 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db) > free(row->updated); > row->updated = NULL; > } > + > + row->change_seqno[OVSDB_IDL_CHANGE_INSERT] = > + row->change_seqno[OVSDB_IDL_CHANGE_MODIFY] = > + row->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0; > + > ovs_list_remove(&row->track_node); > ovs_list_init(&row->track_node); > if (ovsdb_idl_row_is_orphan(row) && row->tracked_old_datum) { > @@ -2684,24 +2689,27 @@ ovsdb_idl_process_update2(struct ovsdb_idl_table *table, > return OVSDB_IDL_UPDATE_DB_CHANGED; > } > > -/* Recursively add rows to tracked change lists for current row > - * and the rows that reference this row. */ > +/* Recursively add rows to tracked change lists for all rows that reference > + 'row'. */ > static void > add_tracked_change_for_references(struct ovsdb_idl_row *row) > { > - if (ovs_list_is_empty(&row->track_node) && > - ovsdb_idl_track_is_set(row->table)) { > - ovs_list_push_back(&row->table->track_list, > - &row->track_node); > - row->change_seqno[OVSDB_IDL_CHANGE_MODIFY] > - = row->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] > - = row->table->db->change_seqno + 1; > - > const struct ovsdb_idl_arc *arc; > LIST_FOR_EACH (arc, dst_node, &row->dst_arcs) { > - add_tracked_change_for_references(arc->src); > + struct ovsdb_idl_row *ref = arc->src; > + > + if (ovs_list_is_empty(&ref->track_node) && > + ovsdb_idl_track_is_set(ref->table)) { > + ovs_list_push_back(&ref->table->track_list, > + &ref->track_node); > + > + ref->change_seqno[OVSDB_IDL_CHANGE_MODIFY] > + = ref->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] > + = ref->table->db->change_seqno + 1; > + > + add_tracked_change_for_references(ref); > + } > } > - } > } > > > @@ -2767,7 +2775,14 @@ ovsdb_idl_row_change__(struct ovsdb_idl_row *row, const struct json *row_json, > row->change_seqno[change] > = row->table->change_seqno[change] > = row->table->db->change_seqno + 1; > + > if (table->modes[column_idx] & OVSDB_IDL_TRACK) { > + if (ovs_list_is_empty(&row->track_node) && > + ovsdb_idl_track_is_set(row->table)) { > + ovs_list_push_back(&row->table->track_list, > + &row->track_node); > + } > + > add_tracked_change_for_references(row); > if (!row->updated) { > row->updated = bitmap_allocate(class->n_columns); > @@ -4843,6 +4858,7 @@ ovsdb_idl_txn_insert(struct ovsdb_idl_txn *txn, > hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid)); > hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid)); > ovsdb_idl_add_to_indexes(row); > + > return row; > } > > diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in > index 698fe25f3095..a0b5a54bbba4 100755 > --- a/ovsdb/ovsdb-idlc.in > +++ b/ovsdb/ovsdb-idlc.in > @@ -279,13 +279,21 @@ const struct %(s)s *%(s)s_table_track_get_first(const struct %(s)s_table *); > (ROW) = %(s)s_track_get_next(ROW)) > > > -/* Returns true if 'row' was inserted since the last change tracking reset. */ > +/* Returns true if 'row' was inserted since the last change tracking reset. > + * > + * Note: This can only be used to test rows of tracked changes. This cannot be > + * used to test if an uncommitted row that has been added locally is new or it > + * may given unexpected results. */ > static inline bool %(s)s_is_new(const struct %(s)s *row) > { > - return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_MODIFY) == 0; > + return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_INSERT) > 0; > } > > -/* Returns true if 'row' was deleted since the last change tracking reset. */ > +/* Returns true if 'row' was deleted since the last change tracking reset. > + * > + * Note: This can only be used to test rows of tracked changes. This cannot be > + * used to test if an uncommitted row that has been added locally has been > + * deleted or it may given unexpected results. */ > static inline bool %(s)s_is_deleted(const struct %(s)s *row) > { > return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_DELETE) > 0; > @@ -333,6 +341,14 @@ struct %(s)s *%(s)s_cursor_data(struct ovsdb_idl_cursor *); > void %(s)s_init(struct %(s)s *); > void %(s)s_delete(const struct %(s)s *); > struct %(s)s *%(s)s_insert(struct ovsdb_idl_txn *); > + > +/* Returns true if the tracked column referenced by 'enum %(s)s_column_id' of > + * the row referenced by 'struct %(s)s *' was updated since the last change > + * tracking reset. > + * > + * Note: This can only be used to test rows of tracked changes. This cannot be > + * used to test if an uncommitted row that has been added locally has been > + * updated or it may given unexpected results. */ > bool %(s)s_is_updated(const struct %(s)s *, enum %(s)s_column_id); > ''' % {'s': structName, 'S': structName.upper()}) > > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > index b462585919a0..b95f18e3ccc0 100644 > --- a/tests/ovsdb-idl.at > +++ b/tests/ovsdb-idl.at > @@ -1162,6 +1162,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated], > "where": [], > "row": {"b": true}}]']], > [[000: i=1 r=2 b=true s=mystring u=<0> ia=[1 2 3] ra=[-0.5] ba=[true] sa=[abc def] ua=[<1> <2>] uuid=<3> > +000: inserted row: uuid=<3> > 000: updated columns: b ba i ia r ra s sa u ua > 001: {"error":null,"result":[{"count":2}]} > 002: i=0 r=0 b=true s= u=<4> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<5> > @@ -1224,6 +1225,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops], > [[000: empty > 001: {"error":null,"result":[{"uuid":["uuid","<0>"]},{"uuid":["uuid","<1>"]}]} > 002: i=1 r=2 b=true s=mystring u=<2> ia=[1 2 3] ra=[-0.5] ba=[true] sa=[abc def] ua=[<3> <4>] uuid=<0> > +002: inserted row: uuid=<0> > 002: updated columns: b ba i ia r ra s sa u ua > 003: {"error":null,"result":[{"count":2}]} > 004: i=0 r=0 b=true s= u=<5> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1> > @@ -1235,6 +1237,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops], > 006: updated columns: r > 007: {"error":null,"result":[{"uuid":["uuid","<6>"]}]} > 008: i=-1 r=125 b=false s= u=<5> ia=[1] ra=[1.5] ba=[false] sa=[] ua=[] uuid=<6> > +008: inserted row: uuid=<6> > 008: updated columns: ba i ia r ra > 009: {"error":null,"result":[{"count":2}]} > 010: i=-1 r=125 b=false s=newstring u=<5> ia=[1] ra=[1.5] ba=[false] sa=[] ua=[] uuid=<6> > @@ -1242,7 +1245,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops], > 010: updated columns: s > 010: updated columns: s > 011: {"error":null,"result":[{"count":1}]} > -012: ##deleted## uuid=<1> > +012: deleted row: 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> > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c > index b1a4be36bb1e..6dd476f75be0 100644 > --- a/tests/test-ovsdb.c > +++ b/tests/test-ovsdb.c > @@ -2030,7 +2030,7 @@ print_idl(struct ovsdb_idl *idl, int step) > } > > static void > -print_idl_track(struct ovsdb_idl *idl, int step, unsigned int seqno) > +print_idl_track(struct ovsdb_idl *idl, int step) > { > const struct idltest_simple *s; > const struct idltest_link1 *l1; > @@ -2038,26 +2038,42 @@ print_idl_track(struct ovsdb_idl *idl, int step, unsigned int seqno) > int n = 0; > > IDLTEST_SIMPLE_FOR_EACH_TRACKED (s, idl) { > - if (idltest_simple_row_get_seqno(s, OVSDB_IDL_CHANGE_DELETE) >= seqno) { > - printf("%03d: ##deleted## uuid="UUID_FMT"\n", step, UUID_ARGS(&s->header_.uuid)); > + 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)); > + } > } > n++; > } > IDLTEST_LINK1_FOR_EACH_TRACKED (l1, idl) { > - if (idltest_simple_row_get_seqno(s, OVSDB_IDL_CHANGE_DELETE) >= seqno) { > - printf("%03d: ##deleted## uuid="UUID_FMT"\n", step, UUID_ARGS(&s->header_.uuid)); > + if (idltest_link1_is_deleted(l1)) { > + printf("%03d: deleted row: uuid="UUID_FMT"\n", step, > + UUID_ARGS(&l1->header_.uuid)); > } else { > print_idl_row_link1(l1, step); > + if (idltest_link1_is_new(l1)) { > + printf("%03d: inserted row: uuid="UUID_FMT"\n", step, > + UUID_ARGS(&l1->header_.uuid)); > + } > } > n++; > } > IDLTEST_LINK2_FOR_EACH_TRACKED (l2, idl) { > - if (idltest_simple_row_get_seqno(s, OVSDB_IDL_CHANGE_DELETE) >= seqno) { > - printf("%03d: ##deleted## uuid="UUID_FMT"\n", step, UUID_ARGS(&s->header_.uuid)); > + if (idltest_link2_is_deleted(l2)) { > + printf("%03d: deleted row: uuid="UUID_FMT"\n", step, > + UUID_ARGS(&l2->header_.uuid)); > } else { > print_idl_row_link2(l2, step); > + if (idltest_link2_is_new(l2)) { > + printf("%03d: inserted row: uuid="UUID_FMT"\n", step, > + UUID_ARGS(&l2->header_.uuid)); > + } > + > } > n++; > } > @@ -2465,7 +2481,7 @@ do_idl(struct ovs_cmdl_context *ctx) > > /* Print update. */ > if (track) { > - print_idl_track(idl, step++, ovsdb_idl_get_seqno(idl)); > + print_idl_track(idl, step++); > ovsdb_idl_track_clear(idl); > } else { > print_idl(idl, step++); > -- > 2.26.2 > Thanks Mark. Acked-by: Han Zhou <hzhou@ovn.org>
On 10/20/20 9:46 PM, Han Zhou wrote: > On Tue, Oct 20, 2020 at 8:07 AM Mark Gray <mark.d.gray@redhat.com> wrote: >> >> Currently all functions of the type *_is_new() always return >> 'false'. This patch resolves this issue by using the >> 'OVSDB_IDL_CHANGE_INSERT' 'change_seqno' instead of the >> 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno' to determine if a row >> is new and by resetting the 'OVSDB_IDL_CHANGE_INSERT' >> 'change_seqno' on clear. >> >> Further to this, the code is also updated to match the following >> behaviour: >> >> When a row is inserted, the 'OVSDB_IDL_CHANGE_INSERT' >> 'change_seqno' is updated to match the new database >> change_seqno. The 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno' >> is not set for inserted rows (only for updated rows). >> >> At the end of a run, ovsdb_idl_db_track_clear() should be >> called to clear all tracking information, this includes >> resetting all row 'change_seqno' to zero. This will ensure >> that subsequent runs will not see a previously 'new' row. >> >> add_tracked_change_for_references() is updated to only >> track rows that reference the current row. >> >> Also, update unit tests in order to test the *_is_new(), >> *_is_delete() functions. >> >> Suggested-by: Dumitru Ceara <dceara@redhat.com> >> Reported-at: https://bugzilla.redhat.com/1883562 >> Fixes: ca545a787ac0 ("ovsdb-idl.c: Increase seqno for change-tracking of > table references.") >> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> >> --- >> v3: Update comments for _is_new(), is_deleted() and is_updated() functions >> Removed change that modifies flags on local uncommitted changes >> <snip> >> static void >> add_tracked_change_for_references(struct ovsdb_idl_row *row) >> { >> - if (ovs_list_is_empty(&row->track_node) && >> - ovsdb_idl_track_is_set(row->table)) { >> - ovs_list_push_back(&row->table->track_list, >> - &row->track_node); >> - row->change_seqno[OVSDB_IDL_CHANGE_MODIFY] >> - = row->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] >> - = row->table->db->change_seqno + 1; >> - >> const struct ovsdb_idl_arc *arc; >> LIST_FOR_EACH (arc, dst_node, &row->dst_arcs) { >> - add_tracked_change_for_references(arc->src); >> + struct ovsdb_idl_row *ref = arc->src; >> + >> + if (ovs_list_is_empty(&ref->track_node) && >> + ovsdb_idl_track_is_set(ref->table)) { >> + ovs_list_push_back(&ref->table->track_list, >> + &ref->track_node); >> + >> + ref->change_seqno[OVSDB_IDL_CHANGE_MODIFY] >> + = ref->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] >> + = ref->table->db->change_seqno + 1; >> + >> + add_tracked_change_for_references(ref); >> + } >> } >> - } I fixed indentation of this block before applying. >> } >> <snip> > > Thanks Mark. > Acked-by: Han Zhou <hzhou@ovn.org> Thanks! Applied to master and backported down to 2.11. Best regards, Ilya Maximets.
On 16/11/2020 19:33, Ilya Maximets wrote: > Applied to master and backported down to 2.11. > > Thanks! > > Best regards, Ilya Maximets. >
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index d8f221ca6073..58468d283eef 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -1959,6 +1959,11 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db) free(row->updated); row->updated = NULL; } + + row->change_seqno[OVSDB_IDL_CHANGE_INSERT] = + row->change_seqno[OVSDB_IDL_CHANGE_MODIFY] = + row->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0; + ovs_list_remove(&row->track_node); ovs_list_init(&row->track_node); if (ovsdb_idl_row_is_orphan(row) && row->tracked_old_datum) { @@ -2684,24 +2689,27 @@ ovsdb_idl_process_update2(struct ovsdb_idl_table *table, return OVSDB_IDL_UPDATE_DB_CHANGED; } -/* Recursively add rows to tracked change lists for current row - * and the rows that reference this row. */ +/* Recursively add rows to tracked change lists for all rows that reference + 'row'. */ static void add_tracked_change_for_references(struct ovsdb_idl_row *row) { - if (ovs_list_is_empty(&row->track_node) && - ovsdb_idl_track_is_set(row->table)) { - ovs_list_push_back(&row->table->track_list, - &row->track_node); - row->change_seqno[OVSDB_IDL_CHANGE_MODIFY] - = row->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] - = row->table->db->change_seqno + 1; - const struct ovsdb_idl_arc *arc; LIST_FOR_EACH (arc, dst_node, &row->dst_arcs) { - add_tracked_change_for_references(arc->src); + struct ovsdb_idl_row *ref = arc->src; + + if (ovs_list_is_empty(&ref->track_node) && + ovsdb_idl_track_is_set(ref->table)) { + ovs_list_push_back(&ref->table->track_list, + &ref->track_node); + + ref->change_seqno[OVSDB_IDL_CHANGE_MODIFY] + = ref->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY] + = ref->table->db->change_seqno + 1; + + add_tracked_change_for_references(ref); + } } - } } @@ -2767,7 +2775,14 @@ ovsdb_idl_row_change__(struct ovsdb_idl_row *row, const struct json *row_json, row->change_seqno[change] = row->table->change_seqno[change] = row->table->db->change_seqno + 1; + if (table->modes[column_idx] & OVSDB_IDL_TRACK) { + if (ovs_list_is_empty(&row->track_node) && + ovsdb_idl_track_is_set(row->table)) { + ovs_list_push_back(&row->table->track_list, + &row->track_node); + } + add_tracked_change_for_references(row); if (!row->updated) { row->updated = bitmap_allocate(class->n_columns); @@ -4843,6 +4858,7 @@ ovsdb_idl_txn_insert(struct ovsdb_idl_txn *txn, hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid)); hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid)); ovsdb_idl_add_to_indexes(row); + return row; } diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in index 698fe25f3095..a0b5a54bbba4 100755 --- a/ovsdb/ovsdb-idlc.in +++ b/ovsdb/ovsdb-idlc.in @@ -279,13 +279,21 @@ const struct %(s)s *%(s)s_table_track_get_first(const struct %(s)s_table *); (ROW) = %(s)s_track_get_next(ROW)) -/* Returns true if 'row' was inserted since the last change tracking reset. */ +/* Returns true if 'row' was inserted since the last change tracking reset. + * + * Note: This can only be used to test rows of tracked changes. This cannot be + * used to test if an uncommitted row that has been added locally is new or it + * may given unexpected results. */ static inline bool %(s)s_is_new(const struct %(s)s *row) { - return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_MODIFY) == 0; + return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_INSERT) > 0; } -/* Returns true if 'row' was deleted since the last change tracking reset. */ +/* Returns true if 'row' was deleted since the last change tracking reset. + * + * Note: This can only be used to test rows of tracked changes. This cannot be + * used to test if an uncommitted row that has been added locally has been + * deleted or it may given unexpected results. */ static inline bool %(s)s_is_deleted(const struct %(s)s *row) { return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_DELETE) > 0; @@ -333,6 +341,14 @@ struct %(s)s *%(s)s_cursor_data(struct ovsdb_idl_cursor *); void %(s)s_init(struct %(s)s *); void %(s)s_delete(const struct %(s)s *); struct %(s)s *%(s)s_insert(struct ovsdb_idl_txn *); + +/* Returns true if the tracked column referenced by 'enum %(s)s_column_id' of + * the row referenced by 'struct %(s)s *' was updated since the last change + * tracking reset. + * + * Note: This can only be used to test rows of tracked changes. This cannot be + * used to test if an uncommitted row that has been added locally has been + * updated or it may given unexpected results. */ bool %(s)s_is_updated(const struct %(s)s *, enum %(s)s_column_id); ''' % {'s': structName, 'S': structName.upper()}) diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index b462585919a0..b95f18e3ccc0 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -1162,6 +1162,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated], "where": [], "row": {"b": true}}]']], [[000: i=1 r=2 b=true s=mystring u=<0> ia=[1 2 3] ra=[-0.5] ba=[true] sa=[abc def] ua=[<1> <2>] uuid=<3> +000: inserted row: uuid=<3> 000: updated columns: b ba i ia r ra s sa u ua 001: {"error":null,"result":[{"count":2}]} 002: i=0 r=0 b=true s= u=<4> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<5> @@ -1224,6 +1225,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops], [[000: empty 001: {"error":null,"result":[{"uuid":["uuid","<0>"]},{"uuid":["uuid","<1>"]}]} 002: i=1 r=2 b=true s=mystring u=<2> ia=[1 2 3] ra=[-0.5] ba=[true] sa=[abc def] ua=[<3> <4>] uuid=<0> +002: inserted row: uuid=<0> 002: updated columns: b ba i ia r ra s sa u ua 003: {"error":null,"result":[{"count":2}]} 004: i=0 r=0 b=true s= u=<5> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1> @@ -1235,6 +1237,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops], 006: updated columns: r 007: {"error":null,"result":[{"uuid":["uuid","<6>"]}]} 008: i=-1 r=125 b=false s= u=<5> ia=[1] ra=[1.5] ba=[false] sa=[] ua=[] uuid=<6> +008: inserted row: uuid=<6> 008: updated columns: ba i ia r ra 009: {"error":null,"result":[{"count":2}]} 010: i=-1 r=125 b=false s=newstring u=<5> ia=[1] ra=[1.5] ba=[false] sa=[] ua=[] uuid=<6> @@ -1242,7 +1245,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops], 010: updated columns: s 010: updated columns: s 011: {"error":null,"result":[{"count":1}]} -012: ##deleted## uuid=<1> +012: deleted row: 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> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c index b1a4be36bb1e..6dd476f75be0 100644 --- a/tests/test-ovsdb.c +++ b/tests/test-ovsdb.c @@ -2030,7 +2030,7 @@ print_idl(struct ovsdb_idl *idl, int step) } static void -print_idl_track(struct ovsdb_idl *idl, int step, unsigned int seqno) +print_idl_track(struct ovsdb_idl *idl, int step) { const struct idltest_simple *s; const struct idltest_link1 *l1; @@ -2038,26 +2038,42 @@ print_idl_track(struct ovsdb_idl *idl, int step, unsigned int seqno) int n = 0; IDLTEST_SIMPLE_FOR_EACH_TRACKED (s, idl) { - if (idltest_simple_row_get_seqno(s, OVSDB_IDL_CHANGE_DELETE) >= seqno) { - printf("%03d: ##deleted## uuid="UUID_FMT"\n", step, UUID_ARGS(&s->header_.uuid)); + 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)); + } } n++; } IDLTEST_LINK1_FOR_EACH_TRACKED (l1, idl) { - if (idltest_simple_row_get_seqno(s, OVSDB_IDL_CHANGE_DELETE) >= seqno) { - printf("%03d: ##deleted## uuid="UUID_FMT"\n", step, UUID_ARGS(&s->header_.uuid)); + if (idltest_link1_is_deleted(l1)) { + printf("%03d: deleted row: uuid="UUID_FMT"\n", step, + UUID_ARGS(&l1->header_.uuid)); } else { print_idl_row_link1(l1, step); + if (idltest_link1_is_new(l1)) { + printf("%03d: inserted row: uuid="UUID_FMT"\n", step, + UUID_ARGS(&l1->header_.uuid)); + } } n++; } IDLTEST_LINK2_FOR_EACH_TRACKED (l2, idl) { - if (idltest_simple_row_get_seqno(s, OVSDB_IDL_CHANGE_DELETE) >= seqno) { - printf("%03d: ##deleted## uuid="UUID_FMT"\n", step, UUID_ARGS(&s->header_.uuid)); + if (idltest_link2_is_deleted(l2)) { + printf("%03d: deleted row: uuid="UUID_FMT"\n", step, + UUID_ARGS(&l2->header_.uuid)); } else { print_idl_row_link2(l2, step); + if (idltest_link2_is_new(l2)) { + printf("%03d: inserted row: uuid="UUID_FMT"\n", step, + UUID_ARGS(&l2->header_.uuid)); + } + } n++; } @@ -2465,7 +2481,7 @@ do_idl(struct ovs_cmdl_context *ctx) /* Print update. */ if (track) { - print_idl_track(idl, step++, ovsdb_idl_get_seqno(idl)); + print_idl_track(idl, step++); ovsdb_idl_track_clear(idl); } else { print_idl(idl, step++);
Currently all functions of the type *_is_new() always return 'false'. This patch resolves this issue by using the 'OVSDB_IDL_CHANGE_INSERT' 'change_seqno' instead of the 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno' to determine if a row is new and by resetting the 'OVSDB_IDL_CHANGE_INSERT' 'change_seqno' on clear. Further to this, the code is also updated to match the following behaviour: When a row is inserted, the 'OVSDB_IDL_CHANGE_INSERT' 'change_seqno' is updated to match the new database change_seqno. The 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno' is not set for inserted rows (only for updated rows). At the end of a run, ovsdb_idl_db_track_clear() should be called to clear all tracking information, this includes resetting all row 'change_seqno' to zero. This will ensure that subsequent runs will not see a previously 'new' row. add_tracked_change_for_references() is updated to only track rows that reference the current row. Also, update unit tests in order to test the *_is_new(), *_is_delete() functions. Suggested-by: Dumitru Ceara <dceara@redhat.com> Reported-at: https://bugzilla.redhat.com/1883562 Fixes: ca545a787ac0 ("ovsdb-idl.c: Increase seqno for change-tracking of table references.") Signed-off-by: Mark Gray <mark.d.gray@redhat.com> --- v3: Update comments for _is_new(), is_deleted() and is_updated() functions Removed change that modifies flags on local uncommitted changes lib/ovsdb-idl.c | 40 ++++++++++++++++++++++++++++------------ ovsdb/ovsdb-idlc.in | 22 +++++++++++++++++++--- tests/ovsdb-idl.at | 5 ++++- tests/test-ovsdb.c | 32 ++++++++++++++++++++++++-------- 4 files changed, 75 insertions(+), 24 deletions(-)