linux-kernel - Re: [PATCH 07/10] perf script: Move printing of 'common' data from print

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] [day] [month] [year] [list]

Date:	Thu, 10 Mar 2011 01:50:03 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	David Ahern <daahern@...co.com>
Cc:	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org,
	Paul Mackerras <paulus@...ba.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH 07/10] perf script: Move printing of 'common' data from
 print_event and rename

On Wed, Mar 09, 2011 at 05:32:22PM -0700, David Ahern wrote:
> 
> 
> On 03/09/11 17:22, Frederic Weisbecker wrote:
> > On Wed, Mar 09, 2011 at 05:15:00PM -0700, David Ahern wrote:
> >>
> >>
> >> On 03/09/11 17:10, Frederic Weisbecker wrote:
> >>>
> >>> And if you actually keep those functions in place?
> >>
> >>     CC /tmp/build-perf/util/trace-event-read.o
> >> cc1: warnings being treated as errors
> >> util/trace-event-parse.c:2652:13: error: 'print_graph_cpu' defined but
> >> not used
> >> util/trace-event-parse.c:2681:13: error: 'print_graph_proc' defined but
> >> not used
> >> make: *** [/tmp/build-perf/util/trace-event-parse.o] Error 1
> >> make: *** Waiting for unfinished jobs....
> > 
> > All right, that's because you removed their calls in pretty_print_func_ent().
> > So either you remove the whole in a specific patch, pretty_print_func_ent()
> > included and other related functions, or you keep them.
> > 
> > But I prefer we don't do something halfway, and in particular not in a
> > semi-hidden way inside a patch that is not particularly focused on that
> > purpose.
> 
> You lost me on the halfway part.
> 
> So you want a separate patch that removes the code for an incomplete
> feature -- which means changing the references to the functions in that
> patch?

Yep.

> The intent being a patch that can be reverted later?

That can be reverted or something on top of which we can refer to
get that feature later.

In any case it is deadcode right now. If you remove it it should be
better done seperately. For all the reasons we always prefer to have
patches that do only one logic thing: easier to review, understand,
bisect, revert, find a bug, explain it, etc...

Ok, in fact you can actually keep it like you did: remove the cpu and
proc displays and let the rest of the function graph features. But
at least do it in an isolated patch because that should not be
an unnoticeable change, and for other reasons to make it a standalone
patch quoted above.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/