diff mbox

[ovs-dev,1/1] ovsdb-idl: Retain column values of deleted rows until change-track is cleared.

Message ID 8CA204A851B7B14E86E75054661E417511EB1BE3@G4W3208.americas.hpqcorp.net
State Not Applicable
Headers show

Commit Message

Ansari, Shad March 31, 2016, 7:21 p.m. UTC 
(This is in reference to mail thread - http://openvswitch.org/pipermail/dev/2016-March/068331.html)

---

ovsdb-idl: Retain column values of deleted rows until change-track is cleared.

When change tracking is enabled, only the row uuid of deleted rows is available.
The column values are unparsed and the ovsdb_datum values are cleared so they
are not available for inspection. This change leaves the column values around
for inspection. The column values are cleared (unparsed) upon clearing the
change track.

Signed-off-by: Shad Ansari <shad.ansari@hpe.com>
---
 lib/ovsdb-idl.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

Comments

Ryan Moats March 31, 2016, 9:04 p.m. UTC | #1 
"dev" <dev-bounces@openvswitch.org> wrote on 03/31/2016 02:21:34 PM:

> From: "Ansari, Shad" <shad.ansari@hpe.com>
> To: "dev@openvswitch.org" <dev@openvswitch.org>
> Date: 03/31/2016 02:21 PM
> Subject: [ovs-dev] [PATCH 1/1] ovsdb-idl: Retain column values of
> deleted rows until change-track is cleared.
> Sent by: "dev" <dev-bounces@openvswitch.org>
>
> (This is in reference to mail thread - http://openvswitch.org/
> pipermail/dev/2016-March/068331.html)
>
> ---
>
> ovsdb-idl: Retain column values of deleted rows until change-track is
cleared.
>
> When change tracking is enabled, only the row uuid of deleted rows
> is available.
> The column values are unparsed and the ovsdb_datum values are cleared so
they
> are not available for inspection. This change leaves the column values
around
> for inspection. The column values are cleared (unparsed) upon clearing
the
> change track.
>
> Signed-off-by: Shad Ansari <shad.ansari@hpe.com>

I freely admit that I haven't tested this, but that my experience with
change
tracking while working on the incremental processing patches makes me
question
its usefulness.

My experience with trying to clear the change track and re-establish it
leads
to history being lost [this is likely from my not fully understanding what
happens with sequence numbers over turning tracking on and off] and so it
is
more efficient to just leave change tracking on.  Under this scenario, I'm
pretty convinced that this code would amount to a memory leak.

Ryan
Ansari, Shad March 31, 2016, 10:07 p.m. UTC | #2 
> My experience with trying to clear the change track and re-establish it

> leads

> to history being lost [this is likely from my not fully understanding what

> happens with sequence numbers over turning tracking on and off] and so it

> is

> more efficient to just leave change tracking on.  Under this scenario, I'm

> pretty convinced that this code would amount to a memory leak.

> 


Could you provide more exact description of the problem and the steps/code to reproduce.
Ansari, Shad March 31, 2016, 11:55 p.m. UTC | #3 
> > My experience with trying to clear the change track and re-establish it

> > leads

> > to history being lost [this is likely from my not fully understanding what

> > happens with sequence numbers over turning tracking on and off] and so it

> > is

> > more efficient to just leave change tracking on.  Under this scenario, I'm

> > pretty convinced that this code would amount to a memory leak.

> >

> 

> Could you provide more exact description of the problem and the steps/code to

> reproduce.


I ran valgrind on idl test cases with this change and they all came out clean:
make check-valgrind TESTSUITEFLAGS='-k idl'
find . -name "valgrind.*" | xargs cat
diff mbox

Patch

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 3ab05a3..be2cad2 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -181,6 +181,7 @@  static struct ovsdb_idl_row *ovsdb_idl_row_create(struct ovsdb_idl_table *,
                                                   const struct uuid *);
 static void ovsdb_idl_row_destroy(struct ovsdb_idl_row *);
 static void ovsdb_idl_row_destroy_postprocess(struct ovsdb_idl *);
+static void ovsdb_idl_row_free(struct ovsdb_idl_row *);
 
 static void ovsdb_idl_row_parse(struct ovsdb_idl_row *);
 static void ovsdb_idl_row_unparse(struct ovsdb_idl_row *);
@@ -318,19 +319,12 @@  ovsdb_idl_clear(struct ovsdb_idl *idl)
         HMAP_FOR_EACH_SAFE (row, next_row, hmap_node, &table->rows) {
             struct ovsdb_idl_arc *arc, *next_arc;
 
-            if (!ovsdb_idl_row_is_orphan(row)) {
-                ovsdb_idl_row_unparse(row);
-            }
             LIST_FOR_EACH_SAFE (arc, next_arc, src_node, &row->src_arcs) {
                 free(arc);
             }
             /* No need to do anything with dst_arcs: some node has those arcs
              * as forward arcs and will destroy them itself. */
 
-            if (!ovs_list_is_empty(&row->track_node)) {
-                ovs_list_remove(&row->track_node);
-            }
-
             ovsdb_idl_row_destroy(row);
         }
     }
@@ -856,8 +850,7 @@  ovsdb_idl_track_clear(const struct ovsdb_idl *idl)
                 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);
-                    free(row);
+                    ovsdb_idl_row_free(row);
                 }
             }
         }
@@ -1619,7 +1612,7 @@  ovsdb_idl_row_destroy_postprocess(struct ovsdb_idl *idl)
             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);
-                    free(row);
+                    ovsdb_idl_row_free(row);
                 }
             }
         }
@@ -1627,6 +1620,13 @@  ovsdb_idl_row_destroy_postprocess(struct ovsdb_idl *idl)
 }
 
 static void
+ovsdb_idl_row_free(struct ovsdb_idl_row *row)
+{
+    ovsdb_idl_row_unparse(row);
+    free(row);
+}
+
+static void
 ovsdb_idl_insert_row(struct ovsdb_idl_row *row, const struct json *row_json)
 {
     const struct ovsdb_idl_table_class *class = row->table->class;
@@ -1646,7 +1646,6 @@  ovsdb_idl_insert_row(struct ovsdb_idl_row *row, const struct json *row_json)
 static void
 ovsdb_idl_delete_row(struct ovsdb_idl_row *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)) {
@@ -2138,8 +2137,6 @@  ovsdb_idl_txn_disassemble(struct ovsdb_idl_txn *txn)
                 ovsdb_idl_row_clear_arcs(row, false);
                 ovsdb_idl_row_parse(row);
             }
-        } else {
-            ovsdb_idl_row_unparse(row);
         }
         ovsdb_idl_row_clear_new(row);
 
@@ -2153,7 +2150,7 @@  ovsdb_idl_txn_disassemble(struct ovsdb_idl_txn *txn)
         hmap_node_nullify(&row->txn_node);
         if (!row->old) {
             hmap_remove(&row->table->rows, &row->hmap_node);
-            free(row);
+            ovsdb_idl_row_free(row);
         }
     }
     hmap_destroy(&txn->txn_rows);
@@ -2720,12 +2717,11 @@  ovsdb_idl_txn_delete(const struct ovsdb_idl_row *row_)
 
     ovs_assert(row->new != NULL);
     if (!row->old) {
-        ovsdb_idl_row_unparse(row);
         ovsdb_idl_row_clear_new(row);
         ovs_assert(!row->prereqs);
         hmap_remove(&row->table->rows, &row->hmap_node);
         hmap_remove(&row->table->idl->txn->txn_rows, &row->txn_node);
-        free(row);
+        ovsdb_idl_row_free(row);
         return;
     }
     if (hmap_node_is_null(&row->txn_node)) {