Re: [PATCH perf/core v7 07/21] perf symbol: Cleanup the code flow of dso__find_kallsyms

[Masami Hiramatsu <mhiramat@kernel.org>]

On Wed, 11 May 2016 12:55:50 -0300
Arnaldo Carvalho de Melo <acme@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 <mhiramat@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 <mhiramat@kernel.org>