| 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
| ||
|
Date: Fri, 9 Apr 2010 22:28:15 -0300
From: Arnaldo Carvalho de Melo <acme@...radead.org>
To: Masami Hiramatsu <mhiramat@...hat.com>
Cc: Ingo Molnar <mingo@...e.hu>, lkml <linux-kernel@...r.kernel.org>,
systemtap <systemtap@...rces.redhat.com>,
DLE <dle-develop@...ts.sourceforge.net>,
Paul Mackerras <paulus@...ba.org>,
Peter Zijlstra <peterz@...radead.org>,
Mike Galbraith <efault@....de>,
Frederic Weisbecker <fweisbec@...il.com>
Subject: Re: [PATCH -tip v2 7/8] perf probe: Remove die() from probe-finder
code
Em Fri, Apr 09, 2010 at 07:18:38PM -0400, Masami Hiramatsu escreveu:
> Hi Arnaldo,
>
> Has this code done what you suggested? :)
> I'd like to have your comment.
It improves the current situation, yes, but there are still cases there
where die() is called, I assume that is left for later, right?
More comments below.
> Thank you,
>
> Masami Hiramatsu wrote:
> > Remove die() and DIE_IF() code from util/probe-finder.c since
> > these 'sudden death' in utility functions make reusing it from
> > other code (especially tui/gui) difficult.
> >
> > Signed-off-by: Masami Hiramatsu <mhiramat@...hat.com>
> > Cc: Ingo Molnar <mingo@...e.hu>
> > Cc: Paul Mackerras <paulus@...ba.org>
> > Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Mike Galbraith <efault@....de>
> > Cc: Frederic Weisbecker <fweisbec@...il.com>
> > ---
> >
> > tools/perf/util/probe-event.c | 4
> > tools/perf/util/probe-finder.c | 517 +++++++++++++++++++++++++---------------
> > 2 files changed, 322 insertions(+), 199 deletions(-)
> >
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index bef2805..7893b32 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -151,10 +151,10 @@ static int try_to_find_kprobe_trace_events(struct perf_probe_event *pev,
> >
> > /* Error path */
> > if (need_dwarf) {
> > - if (ntevs == -ENOENT)
> > + if (ntevs == -EBADF)
> > pr_warning("No dwarf info found in the vmlinux - "
> > "please rebuild with CONFIG_DEBUG_INFO=y.\n");
> > - die("Could not analyze debuginfo.");
> > + die("Failed to analyze debuginfo.");
Like here, the TUI/GUI can try to add a probe but if it fails it can
still continue providing things like a "perf top" window, analysing
perf.data files, doing 'perf diffs', etc.
> > }
> > pr_debug("An error occurred in debuginfo analysis."
> > " Try to use symbols.\n");
> > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > index 1f45285..4a57504 100644
> > --- a/tools/perf/util/probe-finder.c
> > +++ b/tools/perf/util/probe-finder.c
> > @@ -196,19 +196,7 @@ static bool die_compare_name(Dwarf_Die *dw_die, const char *tname)
> > {
> > const char *name;
> > name = dwarf_diename(dw_die);
> > - DIE_IF(name == NULL);
> > - return strcmp(tname, name);
> > -}
> > -
> > -/* Get entry pc(or low pc, 1st entry of ranges) of the die */
> > -static Dwarf_Addr die_get_entrypc(Dwarf_Die *dw_die)
> > -{
> > - Dwarf_Addr epc;
> > - int ret;
> > -
> > - ret = dwarf_entrypc(dw_die, &epc);
> > - DIE_IF(ret == -1);
> > - return epc;
> > + return name ? strcmp(tname, name) : -1;
This change is correct, with the stated intent :-)
> > }
> >
> > /* Get type die, but skip qualifiers and typedef */
> > @@ -390,7 +378,7 @@ static Dwarf_Die *die_find_member(Dwarf_Die *st_die, const char *name,
> > */
> >
> > /* Show a location */
> > -static void convert_location(Dwarf_Op *op, struct probe_finder *pf)
> > +static int convert_location(Dwarf_Op *op, struct probe_finder *pf)
Yeah, there are still lots of places, in other areas that don't return a
status, just printing a warning and silently failing, like in the trace
parsing routines
> > {
> > unsigned int regn;
> > Dwarf_Word offs = 0;
> > @@ -400,8 +388,11 @@ static void convert_location(Dwarf_Op *op, struct probe_finder *pf)
> >
> > /* If this is based on frame buffer, set the offset */
> > if (op->atom == DW_OP_fbreg) {
> > - if (pf->fb_ops == NULL)
> > - die("The attribute of frame base is not supported.\n");
> > + if (pf->fb_ops == NULL) {
> > + pr_warning("The attribute of frame base is not "
> > + "supported.\n");
> > + return -ENOTSUP;
> > + }
Correct, if you look at tools/perf/util/debug.c, eprintf() looks if
we're in TUI mode, use_browser is true and either fprintfs(stderr) if
not or calls a routine to put it at the "status line" (bottom) in the
TUI.
Having a list with the last messages so that we can have a log window,
or keeping it in a log file that would then be browser is an enhancement
to be made.
> > ref = true;
> > offs = op->number;
> > op = &pf->fb_ops[0];
> > @@ -419,50 +410,63 @@ static void convert_location(Dwarf_Op *op, struct probe_finder *pf)
> > ref = true;
> > } else if (op->atom == DW_OP_regx) {
> > regn = op->number;
> > - } else
> > - die("DW_OP %d is not supported.", op->atom);
> > + } else {
> > + pr_warning("DW_OP %d is not supported.\n", op->atom);
> > + return -ENOTSUP;
> > + }
correct
> > regs = get_arch_regstr(regn);
> > - if (!regs)
> > - die("%u exceeds max register number.", regn);
> > + if (!regs) {
> > + pr_warning("%u exceeds max register number.\n", regn);
> > + return -ERANGE;
> > + }
> >
> > tvar->value = xstrdup(regs);
> > if (ref) {
> > tvar->ref = xzalloc(sizeof(struct kprobe_trace_arg_ref));
We have to kill those xzcalloc, etc, too they are die() in disguise :-)
- Arnaldo
--
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/
Powered by blists - more mailing lists