[ovs-dev] [PATCH] ovsdb-idl: Remove unnecessary code in track clear.

Ben Pfaff blp at ovn.org
Mon Jun 18 22:51:31 UTC 2018


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 at 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 at 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.


More information about the dev mailing list