| Message ID | 1527700106-82991-1-git-send-email-hzhou8@ebay.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovsdb-idl: Remove unnecessary code in track clear. | expand |
On Wed, May 30, 2018 at 10:08:26AM -0700, Han Zhou wrote: > In ovsdb_idl_db_track_clear(), it needs to free the deleted row. > However, it unnecessary to call ovsdb_idl_row_clear_old(), because > this has been called in ovsdb_idl_row_destroy(). It is also confusing > because it is called only if: > if (ovsdb_idl_row_is_orphan(row)) > This is contradict with the check in ovsdb_idl_row_clear_old(): > if (!ovsdb_idl_row_is_orphan(row)) > > Signed-off-by: Han Zhou <hzhou8@ebay.com> > --- > lib/ovsdb-idl.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index c6ff78e..958f924 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -1710,7 +1710,6 @@ 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); > free(row); > } > } Thanks for looking at this code. This code is definitely useless since, as you say, ovsdb_idl_row_clear_old() does nothing for orphan rows. I understand the tracking feature very poorly. This change implies that a tracked row never has data, since it never gets freed here. Or maybe it implies that any data in a tracked row is getting leaked. Do you understand which or those is true? Thanks, Ben.
On Fri, Jun 15, 2018 at 1:03 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, May 30, 2018 at 10:08:26AM -0700, Han Zhou wrote: > > In ovsdb_idl_db_track_clear(), it needs to free the deleted row. > > However, it unnecessary to call ovsdb_idl_row_clear_old(), because > > this has been called in ovsdb_idl_row_destroy(). It is also confusing > > because it is called only if: > > if (ovsdb_idl_row_is_orphan(row)) > > This is contradict with the check in ovsdb_idl_row_clear_old(): > > if (!ovsdb_idl_row_is_orphan(row)) > > > > Signed-off-by: Han Zhou <hzhou8@ebay.com> > > --- > > lib/ovsdb-idl.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > > index c6ff78e..958f924 100644 > > --- a/lib/ovsdb-idl.c > > +++ b/lib/ovsdb-idl.c > > @@ -1710,7 +1710,6 @@ 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); > > free(row); > > } > > } > > Thanks for looking at this code. > > This code is definitely useless since, as you say, > ovsdb_idl_row_clear_old() does nothing for orphan rows. > > I understand the tracking feature very poorly. This change implies that > a tracked row never has data, since it never gets freed here. Or maybe > it implies that any data in a tracked row is getting leaked. Do you > understand which or those is true? Hi Ben, Currently the tracked row doesn't maintain any data, so there is no leak. Here is the later patch that tries to keep the data until track_clear: https://patchwork.ozlabs.org/patch/924268/, but it is independent of this change. Thanks, Han
On Fri, Jun 15, 2018 at 01:38:02PM -0700, Han Zhou wrote: > On Fri, Jun 15, 2018 at 1:03 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Wed, May 30, 2018 at 10:08:26AM -0700, Han Zhou wrote: > > > In ovsdb_idl_db_track_clear(), it needs to free the deleted row. > > > However, it unnecessary to call ovsdb_idl_row_clear_old(), because > > > this has been called in ovsdb_idl_row_destroy(). It is also confusing > > > because it is called only if: > > > if (ovsdb_idl_row_is_orphan(row)) > > > This is contradict with the check in ovsdb_idl_row_clear_old(): > > > if (!ovsdb_idl_row_is_orphan(row)) > > > > > > Signed-off-by: Han Zhou <hzhou8@ebay.com> > > > --- > > > lib/ovsdb-idl.c | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > > > index c6ff78e..958f924 100644 > > > --- a/lib/ovsdb-idl.c > > > +++ b/lib/ovsdb-idl.c > > > @@ -1710,7 +1710,6 @@ 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); > > > free(row); > > > } > > > } > > > > Thanks for looking at this code. > > > > This code is definitely useless since, as you say, > > ovsdb_idl_row_clear_old() does nothing for orphan rows. > > > > I understand the tracking feature very poorly. This change implies that > > a tracked row never has data, since it never gets freed here. Or maybe > > it implies that any data in a tracked row is getting leaked. Do you > > understand which or those is true? > > Hi Ben, > > Currently the tracked row doesn't maintain any data, so there is no leak. > Here is the later patch that tries to keep the data until track_clear: > https://patchwork.ozlabs.org/patch/924268/, but it is independent of this > change. Thanks for the info. I added that info to the commit message and applied this to master.
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index c6ff78e..958f924 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -1710,7 +1710,6 @@ 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); free(row); } }
In ovsdb_idl_db_track_clear(), it needs to free the deleted row. However, it unnecessary to call ovsdb_idl_row_clear_old(), because this has been called in ovsdb_idl_row_destroy(). It is also confusing because it is called only if: if (ovsdb_idl_row_is_orphan(row)) This is contradict with the check in ovsdb_idl_row_clear_old(): if (!ovsdb_idl_row_is_orphan(row)) Signed-off-by: Han Zhou <hzhou8@ebay.com> --- lib/ovsdb-idl.c | 1 - 1 file changed, 1 deletion(-)