On Wed, 11 May 2016 12:55:50 -0300
Arnaldo Carvalho de Melo <a...@kernel.org> wrote:
> Em Wed, May 11, 2016 at 10:52:27PM +0900, Masami Hiramatsu escreveu:
> > Cleanup the code flow of dso__find_kallsyms() to remove
> > redundant checking code and add some comment for readability.
> >
> > Signed-off-by: Masami Hiramatsu <mhira...@kernel.org>
> > ---
> > tools/perf/util/symbol.c | 60
> > +++++++++++++++++++++-------------------------
> > 1 file changed, 27 insertions(+), 33 deletions(-)
> >
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index e112bbd..b2b06b7 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1629,6 +1629,15 @@ static int find_matching_kcore(struct map *map, char
> > *dir, size_t dir_sz)
> > return ret;
> > }
> >
> > +static bool filename__readable(const char *file)
> > +{
> > + int fd = open(file, O_RDONLY);
> > + if (fd < 0)
> > + return false;
> > + close(fd);
> > + return true;
> > +}
>
> Do we really have to use this big hammer just for checking if a file is
> readable? What is wronte with:
>
> access(pathname, R_OK)
>
> ?
Yes, I actually had done that at prototyping. But as the manpage of access
said, access() checks accessibility by real uid/gid, but open() checks
by effective uid/gid. So, I considered this could cause unexpected
issue if we decided to use setuid(). I just wanted to check it was OK.
> I see, you're keeping the existing logic, but since you've gone to the
> trouble of introducing a seemingly generic function like
> filename__readable(), you could as well use the canonical way to check
> if a file is readable, namely access(R_OK), no?
I agreed in general :) If we don't can use access(R_OK) I'd like to use it.
>
> Adrian, is there something magical about /proc/kcore for this test to be
> done that way? If so, we better document not to waste our time next time
> we look at this...
Ah, right. At least I had to write a comment about it.
Thank you,
>
> - Arnaldo
>
> > +
> > static char *dso__find_kallsyms(struct dso *dso, struct map *map)
> > {
> > u8 host_build_id[BUILD_ID_SIZE];
> > @@ -1648,45 +1657,33 @@ static char *dso__find_kallsyms(struct dso *dso,
> > struct map *map)
> > sizeof(host_build_id)) == 0)
> > is_host = dso__build_id_equal(dso, host_build_id);
> >
> > - build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id);
> > -
> > - scnprintf(path, sizeof(path), "%s/%s/%s", buildid_dir,
> > - DSO__NAME_KCORE, sbuild_id);
> > -
> > - /* Use /proc/kallsyms if possible */
> > + /* Try a fast path for /proc/kallsyms if possible */
>
> How that improves the previous comment?
>
> > if (is_host) {
> > - DIR *d;
> > - int fd;
> > -
> > - /* If no cached kcore go with /proc/kallsyms */
> > - d = opendir(path);
> > - if (!d)
> > - goto proc_kallsyms;
> > - closedir(d);
> > -
> > /*
> > - * Do not check the build-id cache, until we know we cannot use
> > - * /proc/kcore.
> > + * Do not check the build-id cache, unless we know we cannot use
> > + * /proc/kcore or module maps don't match to /proc/kallsyms.
> > */
> > - fd = open("/proc/kcore", O_RDONLY);
> > - if (fd != -1) {
> > - close(fd);
> > - /* If module maps match go with /proc/kallsyms */
> > - if (!validate_kcore_addresses("/proc/kallsyms", map))
> > - goto proc_kallsyms;
> > - }
> > -
> > - /* Find kallsyms in build-id cache with kcore */
> > - if (!find_matching_kcore(map, path, sizeof(path)))
> > - return strdup(path);
> > -
> > - goto proc_kallsyms;
> > + if (filename__readable("/proc/kcore") &&
> > + !validate_kcore_addresses("/proc/kallsyms", map))
> > + goto proc_kallsyms;
> > }
> >
> > + build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id);
> > +
> > /* Find kallsyms in build-id cache with kcore */
> > + scnprintf(path, sizeof(path), "%s/%s/%s",
> > + buildid_dir, DSO__NAME_KCORE, sbuild_id);
> > +
> > if (!find_matching_kcore(map, path, sizeof(path)))
> > return strdup(path);
> >
> > + /* Use current /proc/kallsyms if possible */
> > + if (is_host) {
> > +proc_kallsyms:
> > + return strdup("/proc/kallsyms");
> > + }
> > +
> > + /* Finally, find a cache of kallsyms */
> > scnprintf(path, sizeof(path), "%s/%s/%s",
> > buildid_dir, DSO__NAME_KALLSYMS, sbuild_id);
> >
> > @@ -1697,9 +1694,6 @@ static char *dso__find_kallsyms(struct dso *dso,
> > struct map *map)
> > }
> >
> > return strdup(path);
> > -
> > -proc_kallsyms:
> > - return strdup("/proc/kallsyms");
> > }
> >
> > static int dso__load_kernel_sym(struct dso *dso, struct map *map,
--
Masami Hiramatsu <mhira...@kernel.org>
