Re: [PATCH perf/core v8 02/16] perf symbol: Cleanup the code flow of dso__find_kallsyms

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

Em Sun, May 15, 2016 at 12:19:50PM +0900, Masami Hiramatsu escreveu:
> Cleanup the code flow of dso__find_kallsyms() to remove
> redundant checking code and add some comment for readability.

Thanks for adding the comment, I'll wait for Hemant to review this, if
you respin it, please break this patch into multiple ones, for
instance, the first should do just the filename__readable() part, then
try to check if you could split it further, to ease reviewing.

I applied the first patch already.

- Arnaldo
 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  Changes in v8:
>   - Add comments why we can't use access(R_OK) for /proc/kcore access.
> ---
>  tools/perf/util/symbol.c |   68 ++++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 2252b54..abda8ba 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1641,6 +1641,20 @@ static int find_matching_kcore(struct map *map, char *dir, size_t dir_sz)
>  	return ret;
>  }
>  
> +/*
> + * Use open(O_RDONLY) to check readability directly instead of access(R_OK)
> + * since access(R_OK) only checks with real UID/GID but open() use effective
> + * UID/GID and actual capabilities (e.g. /proc/kcore requires CAP_SYS_RAWIO).
> + */
> +static bool filename__readable(const char *file)
> +{
> +	int fd = open(file, O_RDONLY);
> +	if (fd < 0)
> +		return false;
> +	close(fd);
> +	return true;
> +}
> +
>  static char *dso__find_kallsyms(struct dso *dso, struct map *map)
>  {
>  	u8 host_build_id[BUILD_ID_SIZE];
> @@ -1660,45 +1674,36 @@ 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 */
>  	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.
> +		 * To check readability of /proc/kcore, do not use access(R_OK)
> +		 * since /proc/kcore requires CAP_SYS_RAWIO to read and access
> +		 * can't check it.
>  		 */
> -		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);
>  
> @@ -1709,9 +1714,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,