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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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)

?

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?

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...

- 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,