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]

Message-ID: <20160121140209.GA2312@kernel.org>
Date:	Thu, 21 Jan 2016 11:02:09 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Jiri Olsa <jolsa@...hat.com>
Cc:	Namhyung Kim <namhyung@...nel.org>, 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

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

> > > 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.
 
> > AFAICS current code also can fail in callchain_merge()..

That needs to be fixed too, then.

- Arnaldo