[ovs-dev,v3,11/16] ovsdb-idl: Tracking - preserve data for deleted rows.

Message ID 1527874926-40450-12-git-send-email-hzhou8@ebay.com
State New
Headers show
Series
  • ovn-controller incremental processing
Related show

Commit Message

Han Zhou June 1, 2018, 5:42 p.m.
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(-)

Comments

Ben Pfaff June 4, 2018, 10:48 p.m. | #1
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?
Han Zhou June 5, 2018, 12:25 a.m. | #2
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

Patch

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)) {