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

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.
Related show

Commit Message

Han Zhou May 30, 2018, 5:08 p.m.
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(-)

Comments

Ben Pfaff June 15, 2018, 8:03 p.m. | #1
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.
Han Zhou June 15, 2018, 8:38 p.m. | #2
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
Ben Pfaff June 18, 2018, 10:51 p.m. | #3
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.

Patch

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);
                 }
             }