From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759575AbcAUOCX (ORCPT ); Thu, 21 Jan 2016 09:02:23 -0500 Received: from mail.kernel.org ([198.145.29.136]:37770 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759517AbcAUOCU (ORCPT ); Thu, 21 Jan 2016 09:02:20 -0500 Date: Thu, 21 Jan 2016 11:02:09 -0300 From: Arnaldo Carvalho de Melo To: Jiri Olsa Cc: Namhyung Kim , Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern , Stephane Eranian , Andi Kleen , Wang Nan Subject: Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view Message-ID: <20160121140209.GA2312@kernel.org> References: <1452960197-5323-1-git-send-email-namhyung@kernel.org> <1452960197-5323-2-git-send-email-namhyung@kernel.org> <20160121104330.GD13547@krava.brq.redhat.com> <20160121125552.GA13893@danjae.kornet> <20160121133558.GC11238@krava.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160121133558.GC11238@krava.brq.redhat.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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