Re: [PATCH 06/21] perf hists: Return error from hists__collapse_resort()

[Namhyung Kim]

Hi Arnaldo,

On Tue, Feb 02, 2016 at 01:56:34PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Feb 02, 2016 at 01:52:05PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Feb 02, 2016 at 11:39:48PM +0900, Namhyung Kim escreveu:
> > > Currently hists__collapse_resort() and hists__collapse_insert_entry()
> > > don't return error code. Now callchain_merge() can check error case,
> > > abort and pass the error to the user.  Later patch can add more work
> > > which can be failed too.
> > > 
> > > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > > ---
> > >  tools/perf/util/hist.c | 27 +++++++++++++++++----------
> > >  tools/perf/util/hist.h |  4 ++--
> > >  2 files changed, 19 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > index 098310bc4489..b476b599e415 100644
> > > --- a/tools/perf/util/hist.c
> > > +++ b/tools/perf/util/hist.c
> > > @@ -1016,8 +1016,8 @@ void hist_entry__delete(struct hist_entry *he)
> > >   * collapse the histogram
> > >   */
> > >  
> > > -bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
> > > -				  struct rb_root *root, struct hist_entry *he)
> > > +int hists__collapse_insert_entry(struct hists *hists, struct rb_root *root,
> > > +				 struct hist_entry *he)
> > >  {
> > >  	struct rb_node **p = &root->rb_node;
> > >  	struct rb_node *parent = NULL;
> > > @@ -1037,12 +1037,13 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
> > >  
> > >  			if (symbol_conf.use_callchain) {
> > >  				callchain_cursor_reset(&callchain_cursor);
> > > -				callchain_merge(&callchain_cursor,
> > > -						iter->callchain,
> > > -						he->callchain);
> > > +				if (callchain_merge(&callchain_cursor,
> > > +						    iter->callchain,
> > > +						    he->callchain) < 0)
> > > +					return -1;
> > 
> > So now we're exiting on error here, whereas before we would
> > unconditionally call hist_entry__delete() right after this branch... Is
> > that right?
> 
> > >  			}
> > >  			hist_entry__delete(he);
> > > -			return false;
> > > +			return 0;
> > >  		}
> > 
> 
> > Looking at it...
> 
> Below it does:
> 
>  		rb_erase(&n->rb_node_in, root);
> -		if (hists__collapse_insert_entry(hists, &hists->entries_collapsed, n)) {
> +		ret = hists__collapse_insert_entry(hists, &hists->entries_collapsed, n);
> +		if (ret < 0)
> +			return -1;
> 
> So, we remove it from the rb tree it is in, and previously we expected
> that hists__collapse_insert_entry would hist_entry__delete(n), while
> now, if callchain_merge() fails, we leak it, no?

You're right, will fix..

Thanks,
Namhyung