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 <[email protected]> > > > Signed-off-by: Namhyung Kim <[email protected]> > > > --- > > > 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
