[ovs-dev] [PATCH v3] ovsdb-idl: Fix *_is_new() IDL functions
Han Zhou
hzhou at ovn.org
Tue Oct 20 19:46:46 UTC 2020
On Tue, Oct 20, 2020 at 8:07 AM Mark Gray <mark.d.gray at 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 at 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 at 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 at ovn.org>
More information about the dev
mailing list