Messages in this thread | | | Date | Wed, 8 Dec 2010 22:29:42 -0200 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [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
| |