linux-kernel - Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 22 Jan 2016 19:44:17 +0900
From:	Namhyung Kim <namhyung@...nel.org>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:	Jiri Olsa <jolsa@...hat.com>, Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Jiri Olsa <jolsa@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	David Ahern <dsahern@...il.com>,
	Stephane Eranian <eranian@...gle.com>,
	Andi Kleen <andi@...stfloor.org>,
	Wang Nan <wangnan0@...wei.com>
Subject: Re: [PATCH 01/17] perf hists: Basic support of hierarchical report
 view

Hi Arnaldo,

On Thu, Jan 21, 2016 at 11:02:09AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa escreveu:
> > On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:
> > 
> > SNIP
> > 
> > > > > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > > > > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > > > > +		if (new == NULL)
> > > > > +			break;
> 
> Also, can we rename 'new' to new_he? In the past I used 'self' and
> Thomas rightly told me that 'self' didn't convey any info, likewise for
> 'new' (that is even a keyword in C++ and may confuse some syntax
> highligting, etc).

OK


> 
> > > > so hierarchy_insert_entry can fail because of memory allocation
> > > > but the resort path does not cover any error path because it only
> > > > shuffles entries from in-tree into sorted tree
> 
> > > Yes, memory allocation can fail anywhere.  If it happens, there's not
> > > much thing we can do IMHO - just print warning and bail out.
> > > Currently it silently ignores the allocation error and try to proceed.
> > > But I guess it'll fail soon at other place anyway.
> > 
> > I thought the 'policy' is to handle all allocation failures
> 
> yup, if some place doesn't, we need to fix it, silently trowing away
> stuff is not good. At the very least count the number of failures and
> inform the user somewhere on the screen.

OK, will change.


>  
> > > AFAICS current code also can fail in callchain_merge()..
> 
> That needs to be fixed too, then.

OK

Thanks,
Namhyung