lkml.org 
[lkml]   [2010]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] perf tools: Add symfs option for off-box analysis using specified tree
Em Wed, Dec 08, 2010 at 03:22:37PM -0700, David S. Ahern escreveu:
> On 12/08/10 13:56, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Dec 07, 2010 at 07:42:54PM -0700, David Ahern escreveu:
> >> + if ((strlen(symbol_conf.symfs) != 0) ||
> >
> > I suggest you use:
> >
> > if (symbol_conf.symfs[1] ||
> >
> > cheaper :)
>
> Changed. I presume you meant: symbol_conf.symfs[0] -- ie., element 0
> has something other than '\0' which means strlen > 0

Right, off-by-one mistake by me, sorry

> >> @@ -1778,17 +1786,20 @@ static int dso__load_vmlinux(struct dso *self, struct map *map,
> >> const char *vmlinux, symbol_filter_t filter)
> >> {
> >> int err = -1, fd;
> >> + char symfs_vmlinux[PATH_MAX];
> >> - fd = open(vmlinux, O_RDONLY);
> >> + snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s/%s",
> >> + symbol_conf.symfs, vmlinux);
> >
> > Its userspace, so stack is plenty, but I guess if using asprintf
> > wouldn't be better...
> >
> > I.e. if we were programming some kernel module, creating a 4K stack
> > variable would be considered bad practice, so I think it is here as
> > well.
>
> Declaring symfs_vmlinux on the stack is consistent with other PATH_MAX
> declarations within perf, and especially within util/symbol.c.

Ok, so keep it like that and I'll then work on a follow on patch to
remove these stack abuses.

> >> + if (strlen(symbol_conf.symfs) == 0) {
> >
> > Do it as:
> >
> > if (!symbol_conf.symfs[1])
> > return 0;
> >
> > And then the patch can be made smaller.
>
> Done.
>
>
> I also added the symfs option to the Documentation files. Will send an
> updated patch.

Thanks a lot!

- Arnaldo


\
 
 \ /
  Last update: 2010-12-09 01:33    [W:0.106 / U:0.412 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site