| Message ID | 1527874926-40450-12-git-send-email-hzhou8@ebay.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ovn-controller incremental processing | expand |
On Fri, Jun 01, 2018 at 10:42:01AM -0700, Han Zhou wrote: > OVSDB IDL can track changes, but for deleted rows, the data is > destroyed and only uuid is tracked. In some cases we need to > check the data of the deleted rows. This patch preserves data > for deleted rows until track clear is called. > > Signed-off-by: Han Zhou <hzhou8@ebay.com> I like the idea of preserving data, but preserving it in the parsed form concerns me because the parsed data might contain pointers to other structures that don't exist anymore. How important is it to preserve it in parsed form?
On Mon, Jun 4, 2018 at 3:48 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Fri, Jun 01, 2018 at 10:42:01AM -0700, Han Zhou wrote: > > OVSDB IDL can track changes, but for deleted rows, the data is > > destroyed and only uuid is tracked. In some cases we need to > > check the data of the deleted rows. This patch preserves data > > for deleted rows until track clear is called. > > > > Signed-off-by: Han Zhou <hzhou8@ebay.com> > > I like the idea of preserving data, but preserving it in the parsed form > concerns me because the parsed data might contain pointers to other > structures that don't exist anymore. How important is it to preserve it > in parsed form? Thanks for pointing out. I understand your concern, but I also wonder how would it help if it is not in parsed form. My use case is actually simple: in patch https://patchwork.ozlabs.org/patch/924270/, I just need to access as->name: + SBREC_ADDRESS_SET_FOR_EACH_TRACKED (as, ctx->ovnsb_idl) { + if (sbrec_address_set_is_deleted(as)) { + expr_const_sets_remove(addr_sets, as->name); + sset_add(deleted, as->name); Without parsed form, I will have to access the row's header_.tracked_old_datum, and parse it myself like: name = tracked_old_datum->keys[0].string. Even if it works this way, it seems not a general approach for the tracking feature and the code might be broken if schema changes. Meanwhile, even if we unparse it, we can't prevent someone to access the fields of the generated data structure. It has always been a problem if a tracked but deleted row is accessed. For example, without this patch, we can't prevent someone to access as->name at this point, and it is just random data because the string has been freed already. This patch makes the access to the direct fields valid, but accessing referenced rows remains invalid. I think we can at least add some comment in the code warning the developers to avoid accessing references using tracked but deleted rows. A better approach might be preserving all the referenced and to-be-deleted rows, even if those rows are not being tracked, and destroy them also in the ovsdb_idl_db_track_clear(). I am not sure though, is there any known traps for this approach. If it is just about adding the referenced rows in a list and make sure they are checked and destroyed in the end, it shouldn't be a big change. Maybe I can keep this patch (adding some comments) for now since the next patch depends on it, and then work on preserving all referenced rows until ovsdb_idl_db_track_clear() in a separate patch. What do you think? Thanks, Han
diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h index 70bfde1..745000d 100644 --- a/lib/ovsdb-idl-provider.h +++ b/lib/ovsdb-idl-provider.h @@ -73,6 +73,7 @@ struct ovsdb_idl_row { struct ovs_list dst_arcs; /* Backward arcs (ovsdb_idl_arc.dst_node). */ struct ovsdb_idl_table *table; /* Containing table. */ struct ovsdb_datum *old_datum; /* Committed data (null if orphaned). */ + bool parsed; /* Whether the row is parsed. */ /* Transactional data. */ struct ovsdb_datum *new_datum; /* Modified data (null to delete row). */ @@ -88,6 +89,7 @@ struct ovsdb_idl_row { unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX]; struct ovs_list track_node; /* Rows modified/added/deleted by IDL */ unsigned long int *updated; /* Bitmap of columns updated by IDL */ + struct ovsdb_datum *tracked_old_datum; /* Old deleted data. */ }; struct ovsdb_idl_column { diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index c6ff78e..1e7d121 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -1709,8 +1709,16 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db) } ovs_list_remove(&row->track_node); ovs_list_init(&row->track_node); - if (ovsdb_idl_row_is_orphan(row)) { - ovsdb_idl_row_clear_old(row); + if (ovsdb_idl_row_is_orphan(row) && row->tracked_old_datum) { + ovsdb_idl_row_unparse(row); + const struct ovsdb_idl_table_class *class = + row->table->class_; + for (size_t c = 0; c < class->n_columns; c++) { + ovsdb_datum_destroy(&row->tracked_old_datum[c], + &class->columns[c].type); + } + free(row->tracked_old_datum); + row->tracked_old_datum = NULL; free(row); } } @@ -2437,10 +2445,14 @@ ovsdb_idl_row_parse(struct ovsdb_idl_row *row) const struct ovsdb_idl_table_class *class = row->table->class_; size_t i; + if (row->parsed) { + ovsdb_idl_row_unparse(row); + } for (i = 0; i < class->n_columns; i++) { const struct ovsdb_idl_column *c = &class->columns[i]; (c->parse)(row, &row->old_datum[i]); } + row->parsed = true; } static void @@ -2449,10 +2461,14 @@ ovsdb_idl_row_unparse(struct ovsdb_idl_row *row) const struct ovsdb_idl_table_class *class = row->table->class_; size_t i; + if (!row->parsed) { + return; + } for (i = 0; i < class->n_columns; i++) { const struct ovsdb_idl_column *c = &class->columns[i]; (c->unparse)(row); } + row->parsed = false; } /* The OVSDB-IDL Compound Indexes feature allows for the creation of custom @@ -2881,13 +2897,18 @@ ovsdb_idl_row_clear_old(struct ovsdb_idl_row *row) { ovs_assert(row->old_datum == row->new_datum); if (!ovsdb_idl_row_is_orphan(row)) { - const struct ovsdb_idl_table_class *class = row->table->class_; - size_t i; + if (ovsdb_idl_track_is_set(row->table)) { + row->tracked_old_datum = row->old_datum; + } else { + const struct ovsdb_idl_table_class *class = row->table->class_; + size_t i; - for (i = 0; i < class->n_columns; i++) { - ovsdb_datum_destroy(&row->old_datum[i], &class->columns[i].type); + for (i = 0; i < class->n_columns; i++) { + ovsdb_datum_destroy(&row->old_datum[i], + &class->columns[i].type); + } + free(row->old_datum); } - free(row->old_datum); row->old_datum = row->new_datum = NULL; } } @@ -3063,6 +3084,7 @@ ovsdb_idl_row_destroy_postprocess(struct ovsdb_idl_db *db) LIST_FOR_EACH_SAFE(row, next, track_node, &table->track_list) { if (!ovsdb_idl_track_is_set(row->table)) { ovs_list_remove(&row->track_node); + ovsdb_idl_row_unparse(row); free(row); } } @@ -3093,7 +3115,6 @@ static void ovsdb_idl_delete_row(struct ovsdb_idl_row *row) { ovsdb_idl_remove_from_indexes(row); - ovsdb_idl_row_unparse(row); ovsdb_idl_row_clear_arcs(row, true); ovsdb_idl_row_clear_old(row); if (ovs_list_is_empty(&row->dst_arcs)) {
OVSDB IDL can track changes, but for deleted rows, the data is destroyed and only uuid is tracked. In some cases we need to check the data of the deleted rows. This patch preserves data for deleted rows until track clear is called. Signed-off-by: Han Zhou <hzhou8@ebay.com> --- lib/ovsdb-idl-provider.h | 2 ++ lib/ovsdb-idl.c | 37 +++++++++++++++++++++++++++++-------- 2 files changed, 31 insertions(+), 8 deletions(-)