[PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Namhyung Kim]

Minchan reported that perf failed to load vmlinux if --symfs argument
doesn't end with '/' character.  So make sure that the symfs always
ends with the '/'.

Reported-by: Minchan Kim <minchan@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index eb06746b06b2..90723a12e947 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1829,7 +1829,7 @@ static bool symbol__read_kptr_restrict(void)
 
 int symbol__init(void)
 {
-	const char *symfs;
+	char *symfs;
 
 	if (symbol_conf.initialized)
 		return 0;
@@ -1862,23 +1862,40 @@ int symbol__init(void)
 		       symbol_conf.sym_list_str, "symbol") < 0)
 		goto out_free_comm_list;
 
-	/*
-	 * A path to symbols of "/" is identical to ""
-	 * reset here for simplicity.
-	 */
-	symfs = realpath(symbol_conf.symfs, NULL);
-	if (symfs == NULL)
-		symfs = symbol_conf.symfs;
-	if (strcmp(symfs, "/") == 0)
-		symbol_conf.symfs = "";
-	if (symfs != symbol_conf.symfs)
-		free((void *)symfs);
+	if (*symbol_conf.symfs) {
+		symfs = realpath(symbol_conf.symfs, NULL);
+		if (symfs == NULL)
+			symfs = (char *)symbol_conf.symfs;
+
+		/*
+		 * A path to symbols of "/" is identical to ""
+		 * reset here for simplicity.
+		 */
+		if (strcmp(symfs, "/") == 0)
+			symbol_conf.symfs = "";
+
+		/* ensure symfs ends with '/' */
+		if (symfs[strlen(symfs)-1] != '/') {
+			char *tmp = realloc(symfs, strlen(symfs) + 2);
+			if (tmp == NULL)
+				goto out_free;
+
+			tmp[strlen(tmp)+1] = '\0';
+			tmp[strlen(tmp)] = '/';
+
+			symbol_conf.symfs = tmp;
+		} else {
+			free(symfs);
+		}
+	}
 
 	symbol_conf.kptr_restrict = symbol__read_kptr_restrict();
 
 	symbol_conf.initialized = true;
 	return 0;
 
+out_free:
+	free(symfs);
 out_free_comm_list:
 	strlist__delete(symbol_conf.comm_list);
 out_free_dso_list:
-- 
2.0.0

[PATCH 2/2] perf tools: Check validity of --symfs value

[Namhyung Kim]

If a symfs is given by user, it needs to be checked before applying.
Currently, it just uses it even if realpath() failed.

I guess it's because it needed to handle empty symfs which is the
default value.  But now it's only called when it's not empty so it'd
be better to check before using it and warn users if failed.

Cc: Minchan Kim <minchan@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 90723a12e947..95186185c1d5 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1864,8 +1864,11 @@ int symbol__init(void)
 
 	if (*symbol_conf.symfs) {
 		symfs = realpath(symbol_conf.symfs, NULL);
-		if (symfs == NULL)
-			symfs = (char *)symbol_conf.symfs;
+		if (symfs == NULL) {
+			pr_err("cannot apply symfs: %s: %s\n",
+			       symbol_conf.symfs, strerror(errno));
+			return -1;
+		}
 
 		/*
 		 * A path to symbols of "/" is identical to ""
-- 
2.0.0

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Arnaldo Carvalho de Melo]

Em Fri, Jul 25, 2014 at 10:31:47AM +0900, Namhyung Kim escreveu:
> Minchan reported that perf failed to load vmlinux if --symfs argument
> doesn't end with '/' character.  So make sure that the symfs always
> ends with the '/'.

I don't think this is the right way of doing this, users of
symbol_conf.symfs should instead make sure that they do what they expect
to, i.e. if they want it to be a base directory, make sure that a path
separator is added between it and the relative path following it, if
not, do as they please.

- Arnaldo
 
> Reported-by: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/symbol.c | 41 +++++++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index eb06746b06b2..90723a12e947 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1829,7 +1829,7 @@ static bool symbol__read_kptr_restrict(void)
>  
>  int symbol__init(void)
>  {
> -	const char *symfs;
> +	char *symfs;
>  
>  	if (symbol_conf.initialized)
>  		return 0;
> @@ -1862,23 +1862,40 @@ int symbol__init(void)
>  		       symbol_conf.sym_list_str, "symbol") < 0)
>  		goto out_free_comm_list;
>  
> -	/*
> -	 * A path to symbols of "/" is identical to ""
> -	 * reset here for simplicity.
> -	 */
> -	symfs = realpath(symbol_conf.symfs, NULL);
> -	if (symfs == NULL)
> -		symfs = symbol_conf.symfs;
> -	if (strcmp(symfs, "/") == 0)
> -		symbol_conf.symfs = "";
> -	if (symfs != symbol_conf.symfs)
> -		free((void *)symfs);
> +	if (*symbol_conf.symfs) {
> +		symfs = realpath(symbol_conf.symfs, NULL);
> +		if (symfs == NULL)
> +			symfs = (char *)symbol_conf.symfs;
> +
> +		/*
> +		 * A path to symbols of "/" is identical to ""
> +		 * reset here for simplicity.
> +		 */
> +		if (strcmp(symfs, "/") == 0)
> +			symbol_conf.symfs = "";
> +
> +		/* ensure symfs ends with '/' */
> +		if (symfs[strlen(symfs)-1] != '/') {
> +			char *tmp = realloc(symfs, strlen(symfs) + 2);
> +			if (tmp == NULL)
> +				goto out_free;
> +
> +			tmp[strlen(tmp)+1] = '\0';
> +			tmp[strlen(tmp)] = '/';
> +
> +			symbol_conf.symfs = tmp;
> +		} else {
> +			free(symfs);
> +		}
> +	}
>  
>  	symbol_conf.kptr_restrict = symbol__read_kptr_restrict();
>  
>  	symbol_conf.initialized = true;
>  	return 0;
>  
> +out_free:
> +	free(symfs);
>  out_free_comm_list:
>  	strlist__delete(symbol_conf.comm_list);
>  out_free_dso_list:
> -- 
> 2.0.0

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Namhyung Kim]

Hi Arnaldo,

On Fri, 25 Jul 2014 10:15:21 -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 25, 2014 at 10:31:47AM +0900, Namhyung Kim escreveu:
>> Minchan reported that perf failed to load vmlinux if --symfs argument
>> doesn't end with '/' character.  So make sure that the symfs always
>> ends with the '/'.
>
> I don't think this is the right way of doing this, users of
> symbol_conf.symfs should instead make sure that they do what they expect
> to, i.e. if they want it to be a base directory, make sure that a path
> separator is added between it and the relative path following it, if
> not, do as they please.

Hmm.. what's the usecase of non-directory symfs?  I guess realpath(3)
already nullifies such case and defaults to no symfs..

Thanks,
Namhyung

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Minchan Kim]

On Fri, Jul 25, 2014 at 10:31:47AM +0900, Namhyung Kim wrote:
> Minchan reported that perf failed to load vmlinux if --symfs argument
> doesn't end with '/' character.  So make sure that the symfs always
> ends with the '/'.
> 
> Reported-by: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Both patches work and are handy to me.
Thanks Namhyung.


-- 
Kind regards,
Minchan Kim

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Arnaldo Carvalho de Melo]

Em Tue, Jul 29, 2014 at 02:02:46PM +0900, Minchan Kim escreveu:
> On Fri, Jul 25, 2014 at 10:31:47AM +0900, Namhyung Kim wrote:
> > Minchan reported that perf failed to load vmlinux if --symfs argument
> > doesn't end with '/' character.  So make sure that the symfs always
> > ends with the '/'.
> > 
> > Reported-by: Minchan Kim <minchan@kernel.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Both patches work and are handy to me.
> Thanks Namhyung.

I haven't said it is not :-) Just that it should be fixed in a different
way.

Can you please try the patch below instead?

David, was there any reason not to do it like done in this patch?

- Arnaldo

---
 annotate.c |    4 ++--
 dso.c      |    8 ++++----
 symbol.c   |    2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 809b4c50beae..e67ef4a2b356 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -900,7 +900,7 @@ int symbol__annotate(struct symbol *sym, struct map *map, size_t privsize)
 	bool delete_extract = false;
 
 	if (filename) {
-		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
+		snprintf(symfs_filename, sizeof(symfs_filename), "%s/%s",
 			 symbol_conf.symfs, filename);
 	}
 
@@ -922,7 +922,7 @@ fallback:
 		 * DSO is the same as when 'perf record' ran.
 		 */
 		filename = (char *)dso->long_name;
-		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
+		snprintf(symfs_filename, sizeof(symfs_filename), "%s/%s",
 			 symbol_conf.symfs, filename);
 		free_filename = false;
 	}
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 90d02c661dd4..f81550583429 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -79,7 +79,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
 		while (last_slash != dso->long_name && *last_slash != '/')
 			last_slash--;
 
-		len = scnprintf(filename, size, "%s", symbol_conf.symfs);
+		len = scnprintf(filename, size, "%s/", symbol_conf.symfs);
 		dir_size = last_slash - dso->long_name + 2;
 		if (dir_size > (size - len)) {
 			ret = -1;
@@ -108,17 +108,17 @@ int dso__read_binary_type_filename(const struct dso *dso,
 	case DSO_BINARY_TYPE__VMLINUX:
 	case DSO_BINARY_TYPE__GUEST_VMLINUX:
 	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
-		snprintf(filename, size, "%s%s",
+		snprintf(filename, size, "%s/%s",
 			 symbol_conf.symfs, dso->long_name);
 		break;
 
 	case DSO_BINARY_TYPE__GUEST_KMODULE:
-		snprintf(filename, size, "%s%s%s", symbol_conf.symfs,
+		snprintf(filename, size, "%s/%s/%s", symbol_conf.symfs,
 			 root_dir, dso->long_name);
 		break;
 
 	case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE:
-		snprintf(filename, size, "%s%s", symbol_conf.symfs,
+		snprintf(filename, size, "%s/%s", symbol_conf.symfs,
 			 dso->long_name);
 		break;
 
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index eb06746b06b2..c3549d5955ea 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1468,7 +1468,7 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
 	if (vmlinux[0] == '/')
 		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s", vmlinux);
 	else
-		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s%s",
+		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s/%s",
 			 symbol_conf.symfs, vmlinux);
 
 	if (dso->kernel == DSO_TYPE_GUEST_KERNEL)

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Minchan Kim]

Hello,

On Tue, Jul 29, 2014 at 09:33:05AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jul 29, 2014 at 02:02:46PM +0900, Minchan Kim escreveu:
> > On Fri, Jul 25, 2014 at 10:31:47AM +0900, Namhyung Kim wrote:
> > > Minchan reported that perf failed to load vmlinux if --symfs argument
> > > doesn't end with '/' character.  So make sure that the symfs always
> > > ends with the '/'.
> > > 
> > > Reported-by: Minchan Kim <minchan@kernel.org>
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > 
> > Both patches work and are handy to me.
> > Thanks Namhyung.
> 
> I haven't said it is not :-) Just that it should be fixed in a different
> way.

I just wanted to confirm Namhyung's patches work as reporter. :)

> 
> Can you please try the patch below instead?

Tested. It works.
And I'd like to say that [2/2] in patchset is handy to me.

Thanks.

> 
> David, was there any reason not to do it like done in this patch?
> 
> - Arnaldo
> 
> ---
>  annotate.c |    4 ++--
>  dso.c      |    8 ++++----
>  symbol.c   |    2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 809b4c50beae..e67ef4a2b356 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -900,7 +900,7 @@ int symbol__annotate(struct symbol *sym, struct map *map, size_t privsize)
>  	bool delete_extract = false;
>  
>  	if (filename) {
> -		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
> +		snprintf(symfs_filename, sizeof(symfs_filename), "%s/%s",
>  			 symbol_conf.symfs, filename);
>  	}
>  
> @@ -922,7 +922,7 @@ fallback:
>  		 * DSO is the same as when 'perf record' ran.
>  		 */
>  		filename = (char *)dso->long_name;
> -		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
> +		snprintf(symfs_filename, sizeof(symfs_filename), "%s/%s",
>  			 symbol_conf.symfs, filename);
>  		free_filename = false;
>  	}
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 90d02c661dd4..f81550583429 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -79,7 +79,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
>  		while (last_slash != dso->long_name && *last_slash != '/')
>  			last_slash--;
>  
> -		len = scnprintf(filename, size, "%s", symbol_conf.symfs);
> +		len = scnprintf(filename, size, "%s/", symbol_conf.symfs);
>  		dir_size = last_slash - dso->long_name + 2;
>  		if (dir_size > (size - len)) {
>  			ret = -1;
> @@ -108,17 +108,17 @@ int dso__read_binary_type_filename(const struct dso *dso,
>  	case DSO_BINARY_TYPE__VMLINUX:
>  	case DSO_BINARY_TYPE__GUEST_VMLINUX:
>  	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
> -		snprintf(filename, size, "%s%s",
> +		snprintf(filename, size, "%s/%s",
>  			 symbol_conf.symfs, dso->long_name);
>  		break;
>  
>  	case DSO_BINARY_TYPE__GUEST_KMODULE:
> -		snprintf(filename, size, "%s%s%s", symbol_conf.symfs,
> +		snprintf(filename, size, "%s/%s/%s", symbol_conf.symfs,
>  			 root_dir, dso->long_name);
>  		break;
>  
>  	case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE:
> -		snprintf(filename, size, "%s%s", symbol_conf.symfs,
> +		snprintf(filename, size, "%s/%s", symbol_conf.symfs,
>  			 dso->long_name);
>  		break;
>  
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index eb06746b06b2..c3549d5955ea 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1468,7 +1468,7 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
>  	if (vmlinux[0] == '/')
>  		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s", vmlinux);
>  	else
> -		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s%s",
> +		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s/%s",
>  			 symbol_conf.symfs, vmlinux);
>  
>  	if (dso->kernel == DSO_TYPE_GUEST_KERNEL)

-- 
Kind regards,
Minchan Kim

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Arnaldo Carvalho de Melo]

Em Tue, Jul 29, 2014 at 10:26:57PM +0900, Minchan Kim escreveu:
> Hello,
> 
> On Tue, Jul 29, 2014 at 09:33:05AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jul 29, 2014 at 02:02:46PM +0900, Minchan Kim escreveu:
> > > On Fri, Jul 25, 2014 at 10:31:47AM +0900, Namhyung Kim wrote:
> > > > Minchan reported that perf failed to load vmlinux if --symfs argument
> > > > doesn't end with '/' character.  So make sure that the symfs always
> > > > ends with the '/'.
> > > > 
> > > > Reported-by: Minchan Kim <minchan@kernel.org>
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > 
> > > Both patches work and are handy to me.
> > > Thanks Namhyung.
> > 
> > I haven't said it is not :-) Just that it should be fixed in a different
> > way.
> 
> I just wanted to confirm Namhyung's patches work as reporter. :)
> 
> > 
> > Can you please try the patch below instead?
> 
> Tested. It works.

Thanks, Adding a Tested-by: you, in addition to the Reported-by: you
already there.

> And I'd like to say that [2/2] in patchset is handy to me.

I'll check it.
 
> Thanks.
> 
> > 
> > David, was there any reason not to do it like done in this patch?
> > 
> > - Arnaldo
> > 
> > ---
> >  annotate.c |    4 ++--
> >  dso.c      |    8 ++++----
> >  symbol.c   |    2 +-
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 809b4c50beae..e67ef4a2b356 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -900,7 +900,7 @@ int symbol__annotate(struct symbol *sym, struct map *map, size_t privsize)
> >  	bool delete_extract = false;
> >  
> >  	if (filename) {
> > -		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
> > +		snprintf(symfs_filename, sizeof(symfs_filename), "%s/%s",
> >  			 symbol_conf.symfs, filename);
> >  	}
> >  
> > @@ -922,7 +922,7 @@ fallback:
> >  		 * DSO is the same as when 'perf record' ran.
> >  		 */
> >  		filename = (char *)dso->long_name;
> > -		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
> > +		snprintf(symfs_filename, sizeof(symfs_filename), "%s/%s",
> >  			 symbol_conf.symfs, filename);
> >  		free_filename = false;
> >  	}
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 90d02c661dd4..f81550583429 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -79,7 +79,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
> >  		while (last_slash != dso->long_name && *last_slash != '/')
> >  			last_slash--;
> >  
> > -		len = scnprintf(filename, size, "%s", symbol_conf.symfs);
> > +		len = scnprintf(filename, size, "%s/", symbol_conf.symfs);
> >  		dir_size = last_slash - dso->long_name + 2;
> >  		if (dir_size > (size - len)) {
> >  			ret = -1;
> > @@ -108,17 +108,17 @@ int dso__read_binary_type_filename(const struct dso *dso,
> >  	case DSO_BINARY_TYPE__VMLINUX:
> >  	case DSO_BINARY_TYPE__GUEST_VMLINUX:
> >  	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
> > -		snprintf(filename, size, "%s%s",
> > +		snprintf(filename, size, "%s/%s",
> >  			 symbol_conf.symfs, dso->long_name);
> >  		break;
> >  
> >  	case DSO_BINARY_TYPE__GUEST_KMODULE:
> > -		snprintf(filename, size, "%s%s%s", symbol_conf.symfs,
> > +		snprintf(filename, size, "%s/%s/%s", symbol_conf.symfs,
> >  			 root_dir, dso->long_name);
> >  		break;
> >  
> >  	case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE:
> > -		snprintf(filename, size, "%s%s", symbol_conf.symfs,
> > +		snprintf(filename, size, "%s/%s", symbol_conf.symfs,
> >  			 dso->long_name);
> >  		break;
> >  
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index eb06746b06b2..c3549d5955ea 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1468,7 +1468,7 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
> >  	if (vmlinux[0] == '/')
> >  		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s", vmlinux);
> >  	else
> > -		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s%s",
> > +		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s/%s",
> >  			 symbol_conf.symfs, vmlinux);
> >  
> >  	if (dso->kernel == DSO_TYPE_GUEST_KERNEL)
> 
> -- 
> Kind regards,
> Minchan Kim

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[David Ahern]

On 7/29/14, 6:33 AM, Arnaldo Carvalho de Melo wrote:
>
> David, was there any reason not to do it like done in this patch?

I feel like we did at one point.
<searching>
http://www.spinics.net/lists/linux-perf-users/msg00252.html

So my original rootfs patch did use '/' after the path.

Incarnation as symfs which got accepted:
    https://lkml.org/lkml/2010/12/9/391
has it removed.

I recall you running the patch through a number of scenarios. I do not 
recall the reason '/' got dropped.

David

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Arnaldo Carvalho de Melo]

Em Tue, Jul 29, 2014 at 10:26:57PM +0900, Minchan Kim escreveu:
> On Tue, Jul 29, 2014 at 09:33:05AM -0300, Arnaldo Carvalho de Melo wrote:
> > Can you please try the patch below instead?
> 
> Tested. It works.
> And I'd like to say that [2/2] in patchset is handy to me.

Ok, here it is, refreshed, will apply with Namhyung as author as the
change should be functionally equivalent to what he did.

Please check that it works as expected,

Thanks,

- Arnaldo

---

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index c3549d5955ea..223a7ffe0d93 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1862,13 +1862,21 @@ int symbol__init(void)
 		       symbol_conf.sym_list_str, "symbol") < 0)
 		goto out_free_comm_list;
 
+	if (symbol_conf.symfs[0] != '\0') {
+		symfs = realpath(symbol_conf.symfs, NULL);
+		if (symfs == NULL) {
+			pr_debug("Cannot apply symfs: %s: %s\n",
+				symbol_conf.symfs, strerror(errno));
+			return -1;
+		}
+	} else {
+		symfs = symbol_conf.symfs;
+	}
+
 	/*
 	 * A path to symbols of "/" is identical to ""
 	 * reset here for simplicity.
 	 */
-	symfs = realpath(symbol_conf.symfs, NULL);
-	if (symfs == NULL)
-		symfs = symbol_conf.symfs;
 	if (strcmp(symfs, "/") == 0)
 		symbol_conf.symfs = "";
 	if (symfs != symbol_conf.symfs)

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Namhyung Kim]

Hi Arnaldo,

On Tue, 29 Jul 2014 09:33:05 -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jul 29, 2014 at 02:02:46PM +0900, Minchan Kim escreveu:
>> On Fri, Jul 25, 2014 at 10:31:47AM +0900, Namhyung Kim wrote:
>> > Minchan reported that perf failed to load vmlinux if --symfs argument
>> > doesn't end with '/' character.  So make sure that the symfs always
>> > ends with the '/'.
>> > 
>> > Reported-by: Minchan Kim <minchan@kernel.org>
>> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> 
>> Both patches work and are handy to me.
>> Thanks Namhyung.
>
> I haven't said it is not :-) Just that it should be fixed in a different
> way.

I also thought about that way first but changed my mind to the current
approach because I don't want to change current behavior.

I worried about the common case which has empty symfs.  By your patch,
it makes a pathname absolute even with an empty symfs - I can see most
filenames are already absolute paths but I'm not 100% sure it's always
the case.

Thanks,
Namhyung


>
> Can you please try the patch below instead?
>
> David, was there any reason not to do it like done in this patch?
>
> - Arnaldo
>
> ---
>  annotate.c |    4 ++--
>  dso.c      |    8 ++++----
>  symbol.c   |    2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 809b4c50beae..e67ef4a2b356 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -900,7 +900,7 @@ int symbol__annotate(struct symbol *sym, struct map *map, size_t privsize)
>  	bool delete_extract = false;
>  
>  	if (filename) {
> -		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
> +		snprintf(symfs_filename, sizeof(symfs_filename), "%s/%s",
>  			 symbol_conf.symfs, filename);
>  	}
>  
> @@ -922,7 +922,7 @@ fallback:
>  		 * DSO is the same as when 'perf record' ran.
>  		 */
>  		filename = (char *)dso->long_name;
> -		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
> +		snprintf(symfs_filename, sizeof(symfs_filename), "%s/%s",
>  			 symbol_conf.symfs, filename);
>  		free_filename = false;
>  	}
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 90d02c661dd4..f81550583429 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -79,7 +79,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
>  		while (last_slash != dso->long_name && *last_slash != '/')
>  			last_slash--;
>  
> -		len = scnprintf(filename, size, "%s", symbol_conf.symfs);
> +		len = scnprintf(filename, size, "%s/", symbol_conf.symfs);
>  		dir_size = last_slash - dso->long_name + 2;
>  		if (dir_size > (size - len)) {
>  			ret = -1;
> @@ -108,17 +108,17 @@ int dso__read_binary_type_filename(const struct dso *dso,
>  	case DSO_BINARY_TYPE__VMLINUX:
>  	case DSO_BINARY_TYPE__GUEST_VMLINUX:
>  	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
> -		snprintf(filename, size, "%s%s",
> +		snprintf(filename, size, "%s/%s",
>  			 symbol_conf.symfs, dso->long_name);
>  		break;
>  
>  	case DSO_BINARY_TYPE__GUEST_KMODULE:
> -		snprintf(filename, size, "%s%s%s", symbol_conf.symfs,
> +		snprintf(filename, size, "%s/%s/%s", symbol_conf.symfs,
>  			 root_dir, dso->long_name);
>  		break;
>  
>  	case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE:
> -		snprintf(filename, size, "%s%s", symbol_conf.symfs,
> +		snprintf(filename, size, "%s/%s", symbol_conf.symfs,
>  			 dso->long_name);
>  		break;
>  
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index eb06746b06b2..c3549d5955ea 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1468,7 +1468,7 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
>  	if (vmlinux[0] == '/')
>  		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s", vmlinux);
>  	else
> -		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s%s",
> +		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s/%s",
>  			 symbol_conf.symfs, vmlinux);
>  
>  	if (dso->kernel == DSO_TYPE_GUEST_KERNEL)

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Arnaldo Carvalho de Melo]

Em Wed, Jul 30, 2014 at 08:52:36AM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Tue, 29 Jul 2014 09:33:05 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jul 29, 2014 at 02:02:46PM +0900, Minchan Kim escreveu:
> >> On Fri, Jul 25, 2014 at 10:31:47AM +0900, Namhyung Kim wrote:
> >> > Minchan reported that perf failed to load vmlinux if --symfs argument
> >> > doesn't end with '/' character.  So make sure that the symfs always
> >> > ends with the '/'.

> >> Both patches work and are handy to me.
> >> Thanks Namhyung.

> > I haven't said it is not :-) Just that it should be fixed in a different
> > way.
 
> I also thought about that way first but changed my mind to the current
> approach because I don't want to change current behavior.

> I worried about the common case which has empty symfs.  By your patch,
> it makes a pathname absolute even with an empty symfs - I can see most
> filenames are already absolute paths but I'm not 100% sure it's always
> the case.

Yeah, after doing some research on the tools/perf/ 'git log' I got your point,
we can't add the / after symfs usages when it is "", i.e. we need something
like:

	("%s%s%s, symfs, symfs[0] ? "/" : "", dso_name)

I.e. the equivalent of this:

[acme@zoo linux]$ python
Python 2.7.5 (default, Jun 25 2014, 10:19:55) 
[GCC 4.8.2 20131212 (Red Hat 4.8.2-7)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> symfs = ""
>>> os.path.join(symfs, "dso_path")
'dso_path'
>>> symfs = "/home/acme/embedded_device_dsos"
>>> os.path.join(symfs, "dso_path")
'/home/acme/embedded_device_dsos/dso_path'
>>> 

I'll try and get that in place.

- Arnaldo

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Arnaldo Carvalho de Melo]

Em Wed, Jul 30, 2014 at 12:19:32PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Jul 30, 2014 at 08:52:36AM +0900, Namhyung Kim escreveu:
> > I also thought about that way first but changed my mind to the current
> > approach because I don't want to change current behavior.
> 
> > I worried about the common case which has empty symfs.  By your patch,
> > it makes a pathname absolute even with an empty symfs - I can see most
> > filenames are already absolute paths but I'm not 100% sure it's always
> > the case.
> 
> Yeah, after doing some research on the tools/perf/ 'git log' I got your point,
> we can't add the / after symfs usages when it is "", i.e. we need something
> like:
> 
> 	("%s%s%s, symfs, symfs[0] ? "/" : "", dso_name)
> 
> I.e. the equivalent of this:
> 
> [acme@zoo linux]$ python
> >>> import os
> >>> symfs = ""
> >>> os.path.join(symfs, "dso_path")
> 'dso_path'
> >>> symfs = "/home/acme/embedded_device_dsos"
> >>> os.path.join(symfs, "dso_path")
> '/home/acme/embedded_device_dsos/dso_path'
> >>> 
> 
> I'll try and get that in place.

Ok, the patch below should implement it just like above, if Minchan
could please retest, I did just minimal testing, will do more later.

- Arnaldo

---

>From d0240769e7d844ee21c1e8fc5cc57627c666f155 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@kernel.org>
Date: Tue, 29 Jul 2014 10:21:58 -0300
Subject: [PATCH 1/1] perf symbols: Make sure --symfs usage includes the path
 separator

Minchan reported that perf failed to load vmlinux if --symfs argument
doesn't end with '/' character.

Fix it by making sure that the '/' path separator is used when composing
pathnames with a --symfs provided directory name.

Reported-by: Minchan Kim <minchan@kernel.org>
Tested-by: Minchan Kim <minchan@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/n/tip-8n4s6b6zvsez5ktanw006125@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/annotate.c |  9 +++------
 tools/perf/util/dso.c      | 28 +++++++++++++---------------
 tools/perf/util/symbol.c   |  3 +--
 tools/perf/util/symbol.h   |  9 +++++++++
 tools/perf/util/util.h     | 16 ++++++++++++++++
 5 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 809b4c50beae..7745fec01a6b 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -899,10 +899,8 @@ int symbol__annotate(struct symbol *sym, struct map *map, size_t privsize)
 	struct kcore_extract kce;
 	bool delete_extract = false;
 
-	if (filename) {
-		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
-			 symbol_conf.symfs, filename);
-	}
+	if (filename)
+		symbol__join_symfs(symfs_filename, filename);
 
 	if (filename == NULL) {
 		if (dso->has_build_id) {
@@ -922,8 +920,7 @@ fallback:
 		 * DSO is the same as when 'perf record' ran.
 		 */
 		filename = (char *)dso->long_name;
-		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
-			 symbol_conf.symfs, filename);
+		symbol__join_symfs(symfs_filename, filename);
 		free_filename = false;
 	}
 
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 90d02c661dd4..bdafd306fb52 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -37,6 +37,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
 {
 	char build_id_hex[BUILD_ID_SIZE * 2 + 1];
 	int ret = 0;
+	size_t len;
 
 	switch (type) {
 	case DSO_BINARY_TYPE__DEBUGLINK: {
@@ -60,26 +61,25 @@ int dso__read_binary_type_filename(const struct dso *dso,
 		break;
 
 	case DSO_BINARY_TYPE__FEDORA_DEBUGINFO:
-		snprintf(filename, size, "%s/usr/lib/debug%s.debug",
-			 symbol_conf.symfs, dso->long_name);
+		len = __symbol__join_symfs(filename, size, "/usr/lib/debug");
+		snprintf(filename + len, size - len, "%s.debug", dso->long_name);
 		break;
 
 	case DSO_BINARY_TYPE__UBUNTU_DEBUGINFO:
-		snprintf(filename, size, "%s/usr/lib/debug%s",
-			 symbol_conf.symfs, dso->long_name);
+		len = __symbol__join_symfs(filename, size, "/usr/lib/debug");
+		snprintf(filename + len, size - len, "%s", dso->long_name);
 		break;
 
 	case DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO:
 	{
 		const char *last_slash;
-		size_t len;
 		size_t dir_size;
 
 		last_slash = dso->long_name + dso->long_name_len;
 		while (last_slash != dso->long_name && *last_slash != '/')
 			last_slash--;
 
-		len = scnprintf(filename, size, "%s", symbol_conf.symfs);
+		len = __symbol__join_symfs(filename, size, "");
 		dir_size = last_slash - dso->long_name + 2;
 		if (dir_size > (size - len)) {
 			ret = -1;
@@ -100,26 +100,24 @@ int dso__read_binary_type_filename(const struct dso *dso,
 		build_id__sprintf(dso->build_id,
 				  sizeof(dso->build_id),
 				  build_id_hex);
-		snprintf(filename, size,
-			 "%s/usr/lib/debug/.build-id/%.2s/%s.debug",
-			 symbol_conf.symfs, build_id_hex, build_id_hex + 2);
+		len = __symbol__join_symfs(filename, size, "/usr/lib/debug/.build-id/");
+		snprintf(filename + len, size - len, "%.2s/%s.debug",
+			 build_id_hex, build_id_hex + 2);
 		break;
 
 	case DSO_BINARY_TYPE__VMLINUX:
 	case DSO_BINARY_TYPE__GUEST_VMLINUX:
 	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
-		snprintf(filename, size, "%s%s",
-			 symbol_conf.symfs, dso->long_name);
+		__symbol__join_symfs(filename, size, dso->long_name);
 		break;
 
 	case DSO_BINARY_TYPE__GUEST_KMODULE:
-		snprintf(filename, size, "%s%s%s", symbol_conf.symfs,
-			 root_dir, dso->long_name);
+		path__join3(filename, size, symbol_conf.symfs,
+			    root_dir, dso->long_name);
 		break;
 
 	case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE:
-		snprintf(filename, size, "%s%s", symbol_conf.symfs,
-			 dso->long_name);
+		__symbol__join_symfs(filename, size, dso->long_name);
 		break;
 
 	case DSO_BINARY_TYPE__KCORE:
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index eb06746b06b2..f134ec138934 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1468,8 +1468,7 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
 	if (vmlinux[0] == '/')
 		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s", vmlinux);
 	else
-		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s%s",
-			 symbol_conf.symfs, vmlinux);
+		symbol__join_symfs(symfs_vmlinux, vmlinux);
 
 	if (dso->kernel == DSO_TYPE_GUEST_KERNEL)
 		symtab_type = DSO_BINARY_TYPE__GUEST_VMLINUX;
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index e7295e93cff9..196b29104276 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -13,6 +13,7 @@
 #include <libgen.h>
 #include "build-id.h"
 #include "event.h"
+#include "util.h"
 
 #ifdef HAVE_LIBELF_SUPPORT
 #include <libelf.h>
@@ -143,6 +144,14 @@ struct symbol_conf {
 };
 
 extern struct symbol_conf symbol_conf;
+
+static inline int __symbol__join_symfs(char *bf, size_t size, const char *path)
+{
+	return path__join(bf, size, symbol_conf.symfs, path);
+}
+
+#define symbol__join_symfs(bf, path) __symbol__join_symfs(bf, sizeof(bf), path)
+
 extern int vmlinux_path__nr_entries;
 extern char **vmlinux_path;
 
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 66864364ccb4..38af77550c75 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -68,6 +68,7 @@
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <inttypes.h>
+#include <linux/kernel.h>
 #include <linux/magic.h>
 #include <linux/types.h>
 #include <sys/ttydefaults.h>
@@ -317,6 +318,21 @@ unsigned long parse_tag_value(const char *str, struct parse_tag *tags);
 
 #define SRCLINE_UNKNOWN  ((char *) "??:0")
 
+static inline int path__join(char *bf, size_t size,
+			     const char *path1, const char *path2)
+{
+	return scnprintf(bf, size, "%s%s%s", path1, path1[0] ? "/" : "", path2);
+}
+
+static inline int path__join3(char *bf, size_t size,
+			      const char *path1, const char *path2,
+			      const char *path3)
+{
+	return scnprintf(bf, size, "%s%s%s%s%s",
+			 path1, path1[0] ? "/" : "",
+			 path2, path2[0] ? "/" : "", path3);
+}
+
 struct dso;
 
 char *get_srcline(struct dso *dso, unsigned long addr);
-- 
1.8.5.rc2

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[David Ahern]

On 7/30/14, 2:55 PM, Arnaldo Carvalho de Melo wrote:
> @@ -143,6 +144,14 @@ struct symbol_conf {
>   };
>
>   extern struct symbol_conf symbol_conf;
> +
> +static inline int __symbol__join_symfs(char *bf, size_t size, const char *path)
> +{
> +	return path__join(bf, size, symbol_conf.symfs, path);
> +}
> +
> +#define symbol__join_symfs(bf, path) __symbol__join_symfs(bf, sizeof(bf), path)
> +
>   extern int vmlinux_path__nr_entries;
>   extern char **vmlinux_path;
>

I am not comfortable with the sizeof(bf) in the macro there. It is to a 
large degree hiding an important detail and sets up odd failures in the 
future. ie., It works fine for 'char bf[N];' but not for 'char *bf = 
"some string";'.

David

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Namhyung Kim]

Hi Arnaldo,

On Wed, 30 Jul 2014 17:55:21 -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jul 30, 2014 at 12:19:32PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Wed, Jul 30, 2014 at 08:52:36AM +0900, Namhyung Kim escreveu:
>> > I also thought about that way first but changed my mind to the current
>> > approach because I don't want to change current behavior.
>> 
>> > I worried about the common case which has empty symfs.  By your patch,
>> > it makes a pathname absolute even with an empty symfs - I can see most
>> > filenames are already absolute paths but I'm not 100% sure it's always
>> > the case.
>> 
>> Yeah, after doing some research on the tools/perf/ 'git log' I got your point,
>> we can't add the / after symfs usages when it is "", i.e. we need something
>> like:
>> 
>> 	("%s%s%s, symfs, symfs[0] ? "/" : "", dso_name)
>> 
>> I.e. the equivalent of this:
>> 
>> [acme@zoo linux]$ python
>> >>> import os
>> >>> symfs = ""
>> >>> os.path.join(symfs, "dso_path")
>> 'dso_path'
>> >>> symfs = "/home/acme/embedded_device_dsos"
>> >>> os.path.join(symfs, "dso_path")
>> '/home/acme/embedded_device_dsos/dso_path'
>> >>> 
>> 
>> I'll try and get that in place.
>
> Ok, the patch below should implement it just like above, if Minchan
> could please retest, I did just minimal testing, will do more later.

Are you still against my approach - adding '/' at the end of the symfs
string itself?  It seems that mine is simpler and shorter.

Thanks,
Namhyung


>
> - Arnaldo
>
> ---
>
> From d0240769e7d844ee21c1e8fc5cc57627c666f155 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@kernel.org>
> Date: Tue, 29 Jul 2014 10:21:58 -0300
> Subject: [PATCH 1/1] perf symbols: Make sure --symfs usage includes the path
>  separator
>
> Minchan reported that perf failed to load vmlinux if --symfs argument
> doesn't end with '/' character.
>
> Fix it by making sure that the '/' path separator is used when composing
> pathnames with a --symfs provided directory name.
>
> Reported-by: Minchan Kim <minchan@kernel.org>
> Tested-by: Minchan Kim <minchan@kernel.org>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Link: http://lkml.kernel.org/n/tip-8n4s6b6zvsez5ktanw006125@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/annotate.c |  9 +++------
>  tools/perf/util/dso.c      | 28 +++++++++++++---------------
>  tools/perf/util/symbol.c   |  3 +--
>  tools/perf/util/symbol.h   |  9 +++++++++
>  tools/perf/util/util.h     | 16 ++++++++++++++++
>  5 files changed, 42 insertions(+), 23 deletions(-)
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 809b4c50beae..7745fec01a6b 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -899,10 +899,8 @@ int symbol__annotate(struct symbol *sym, struct map *map, size_t privsize)
>  	struct kcore_extract kce;
>  	bool delete_extract = false;
>  
> -	if (filename) {
> -		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
> -			 symbol_conf.symfs, filename);
> -	}
> +	if (filename)
> +		symbol__join_symfs(symfs_filename, filename);
>  
>  	if (filename == NULL) {
>  		if (dso->has_build_id) {
> @@ -922,8 +920,7 @@ fallback:
>  		 * DSO is the same as when 'perf record' ran.
>  		 */
>  		filename = (char *)dso->long_name;
> -		snprintf(symfs_filename, sizeof(symfs_filename), "%s%s",
> -			 symbol_conf.symfs, filename);
> +		symbol__join_symfs(symfs_filename, filename);
>  		free_filename = false;
>  	}
>  
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 90d02c661dd4..bdafd306fb52 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -37,6 +37,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
>  {
>  	char build_id_hex[BUILD_ID_SIZE * 2 + 1];
>  	int ret = 0;
> +	size_t len;
>  
>  	switch (type) {
>  	case DSO_BINARY_TYPE__DEBUGLINK: {
> @@ -60,26 +61,25 @@ int dso__read_binary_type_filename(const struct dso *dso,
>  		break;
>  
>  	case DSO_BINARY_TYPE__FEDORA_DEBUGINFO:
> -		snprintf(filename, size, "%s/usr/lib/debug%s.debug",
> -			 symbol_conf.symfs, dso->long_name);
> +		len = __symbol__join_symfs(filename, size, "/usr/lib/debug");
> +		snprintf(filename + len, size - len, "%s.debug", dso->long_name);
>  		break;
>  
>  	case DSO_BINARY_TYPE__UBUNTU_DEBUGINFO:
> -		snprintf(filename, size, "%s/usr/lib/debug%s",
> -			 symbol_conf.symfs, dso->long_name);
> +		len = __symbol__join_symfs(filename, size, "/usr/lib/debug");
> +		snprintf(filename + len, size - len, "%s", dso->long_name);
>  		break;
>  
>  	case DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO:
>  	{
>  		const char *last_slash;
> -		size_t len;
>  		size_t dir_size;
>  
>  		last_slash = dso->long_name + dso->long_name_len;
>  		while (last_slash != dso->long_name && *last_slash != '/')
>  			last_slash--;
>  
> -		len = scnprintf(filename, size, "%s", symbol_conf.symfs);
> +		len = __symbol__join_symfs(filename, size, "");
>  		dir_size = last_slash - dso->long_name + 2;
>  		if (dir_size > (size - len)) {
>  			ret = -1;
> @@ -100,26 +100,24 @@ int dso__read_binary_type_filename(const struct dso *dso,
>  		build_id__sprintf(dso->build_id,
>  				  sizeof(dso->build_id),
>  				  build_id_hex);
> -		snprintf(filename, size,
> -			 "%s/usr/lib/debug/.build-id/%.2s/%s.debug",
> -			 symbol_conf.symfs, build_id_hex, build_id_hex + 2);
> +		len = __symbol__join_symfs(filename, size, "/usr/lib/debug/.build-id/");
> +		snprintf(filename + len, size - len, "%.2s/%s.debug",
> +			 build_id_hex, build_id_hex + 2);
>  		break;
>  
>  	case DSO_BINARY_TYPE__VMLINUX:
>  	case DSO_BINARY_TYPE__GUEST_VMLINUX:
>  	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
> -		snprintf(filename, size, "%s%s",
> -			 symbol_conf.symfs, dso->long_name);
> +		__symbol__join_symfs(filename, size, dso->long_name);
>  		break;
>  
>  	case DSO_BINARY_TYPE__GUEST_KMODULE:
> -		snprintf(filename, size, "%s%s%s", symbol_conf.symfs,
> -			 root_dir, dso->long_name);
> +		path__join3(filename, size, symbol_conf.symfs,
> +			    root_dir, dso->long_name);
>  		break;
>  
>  	case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE:
> -		snprintf(filename, size, "%s%s", symbol_conf.symfs,
> -			 dso->long_name);
> +		__symbol__join_symfs(filename, size, dso->long_name);
>  		break;
>  
>  	case DSO_BINARY_TYPE__KCORE:
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index eb06746b06b2..f134ec138934 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1468,8 +1468,7 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
>  	if (vmlinux[0] == '/')
>  		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s", vmlinux);
>  	else
> -		snprintf(symfs_vmlinux, sizeof(symfs_vmlinux), "%s%s",
> -			 symbol_conf.symfs, vmlinux);
> +		symbol__join_symfs(symfs_vmlinux, vmlinux);
>  
>  	if (dso->kernel == DSO_TYPE_GUEST_KERNEL)
>  		symtab_type = DSO_BINARY_TYPE__GUEST_VMLINUX;
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index e7295e93cff9..196b29104276 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -13,6 +13,7 @@
>  #include <libgen.h>
>  #include "build-id.h"
>  #include "event.h"
> +#include "util.h"
>  
>  #ifdef HAVE_LIBELF_SUPPORT
>  #include <libelf.h>
> @@ -143,6 +144,14 @@ struct symbol_conf {
>  };
>  
>  extern struct symbol_conf symbol_conf;
> +
> +static inline int __symbol__join_symfs(char *bf, size_t size, const char *path)
> +{
> +	return path__join(bf, size, symbol_conf.symfs, path);
> +}
> +
> +#define symbol__join_symfs(bf, path) __symbol__join_symfs(bf, sizeof(bf), path)
> +
>  extern int vmlinux_path__nr_entries;
>  extern char **vmlinux_path;
>  
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 66864364ccb4..38af77550c75 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -68,6 +68,7 @@
>  #include <sys/socket.h>
>  #include <sys/ioctl.h>
>  #include <inttypes.h>
> +#include <linux/kernel.h>
>  #include <linux/magic.h>
>  #include <linux/types.h>
>  #include <sys/ttydefaults.h>
> @@ -317,6 +318,21 @@ unsigned long parse_tag_value(const char *str, struct parse_tag *tags);
>  
>  #define SRCLINE_UNKNOWN  ((char *) "??:0")
>  
> +static inline int path__join(char *bf, size_t size,
> +			     const char *path1, const char *path2)
> +{
> +	return scnprintf(bf, size, "%s%s%s", path1, path1[0] ? "/" : "", path2);
> +}
> +
> +static inline int path__join3(char *bf, size_t size,
> +			      const char *path1, const char *path2,
> +			      const char *path3)
> +{
> +	return scnprintf(bf, size, "%s%s%s%s%s",
> +			 path1, path1[0] ? "/" : "",
> +			 path2, path2[0] ? "/" : "", path3);
> +}
> +
>  struct dso;
>  
>  char *get_srcline(struct dso *dso, unsigned long addr);

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Arnaldo Carvalho de Melo]

Em Thu, Jul 31, 2014 at 01:25:52PM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Wed, 30 Jul 2014 17:55:21 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Jul 30, 2014 at 12:19:32PM -0300, Arnaldo Carvalho de Melo escreveu:
> >> Em Wed, Jul 30, 2014 at 08:52:36AM +0900, Namhyung Kim escreveu:
> >> > I also thought about that way first but changed my mind to the current
> >> > approach because I don't want to change current behavior.
> >> 
> >> > I worried about the common case which has empty symfs.  By your patch,
> >> > it makes a pathname absolute even with an empty symfs - I can see most
> >> > filenames are already absolute paths but I'm not 100% sure it's always
> >> > the case.
> >> 
> >> Yeah, after doing some research on the tools/perf/ 'git log' I got your point,
> >> we can't add the / after symfs usages when it is "", i.e. we need something
> >> like:

> >> [acme@zoo linux]$ python
> >> >>> import os
> >> >>> symfs = ""
> >> >>> os.path.join(symfs, "dso_path")
> >> 'dso_path'
> >> >>> symfs = "/home/acme/embedded_device_dsos"
> >> >>> os.path.join(symfs, "dso_path")
> >> '/home/acme/embedded_device_dsos/dso_path'

> >> I'll try and get that in place.

> > Ok, the patch below should implement it just like above, if Minchan
> > could please retest, I did just minimal testing, will do more later.
 
> Are you still against my approach - adding '/' at the end of the symfs
> string itself?  It seems that mine is simpler and shorter.

Yes, I am.

We are not just concatenating two strings, we are joining two path
components.

I think it is more clear and elegant to do it as python os.path.join()
does.

- Arnaldo

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Namhyung Kim]

Hi Arnaldo,

On Thu, 31 Jul 2014 09:26:21 -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 31, 2014 at 01:25:52PM +0900, Namhyung Kim escreveu:
>> Are you still against my approach - adding '/' at the end of the symfs
>> string itself?  It seems that mine is simpler and shorter.
>
> Yes, I am.
>
> We are not just concatenating two strings, we are joining two path
> components.
>
> I think it is more clear and elegant to do it as python os.path.join()
> does.

Then I think you also need to care about trailing and leading '/' in the
components so that, say, joining '/home/' and '/namhyung/' can result in
'/home/namhyung/' not '/home///namhyung/'.

Btw, it seems like python's os.path.join() just use latter if it's an
absolute path.

  $ python
  Python 2.7.3 (default, Jul 24 2012, 10:05:38) 
  [GCC 4.7.0 20120507 (Red Hat 4.7.0-5)] on linux2
  Type "help", "copyright", "credits" or "license" for more information.
  >>> import os.path
  >>> os.path.join('/home/', '/namhyung/')
  '/namhyung/'
  >>> 


Thanks,
Namhyung

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Arnaldo Carvalho de Melo]

Em Fri, Aug 01, 2014 at 08:38:02AM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Thu, 31 Jul 2014 09:26:21 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jul 31, 2014 at 01:25:52PM +0900, Namhyung Kim escreveu:
> >> Are you still against my approach - adding '/' at the end of the symfs
> >> string itself?  It seems that mine is simpler and shorter.

> > Yes, I am.

> > We are not just concatenating two strings, we are joining two path
> > components.

> > I think it is more clear and elegant to do it as python os.path.join()
> > does.

> Then I think you also need to care about trailing and leading '/' in the
> components so that, say, joining '/home/' and '/namhyung/' can result in
> '/home/namhyung/' not '/home///namhyung/'.

Well, "/home/namhyung/" is the same as "/home///namhyung/" for POSIX:
http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266

So, when we can easily avoid it, no use to have a sequence of slashes,
but otherwise it is harmless.
 
> Btw, it seems like python's os.path.join() just use latter if it's an
> absolute path.
> 
>   $ python
>   Python 2.7.3 (default, Jul 24 2012, 10:05:38) 
>   [GCC 4.7.0 20120507 (Red Hat 4.7.0-5)] on linux2
>   Type "help", "copyright", "credits" or "license" for more information.
>   >>> import os.path
>   >>> os.path.join('/home/', '/namhyung/')
>   '/namhyung/'

Interesting, wonder what is the rationale for that or if this is a bug.

- Arnaldo

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Namhyung Kim]

Hi Arnaldo,

On Fri, 1 Aug 2014 17:15:38 -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Aug 01, 2014 at 08:38:02AM +0900, Namhyung Kim escreveu:
>> Hi Arnaldo,
>> 
>> On Thu, 31 Jul 2014 09:26:21 -0300, Arnaldo Carvalho de Melo wrote:
>> > Em Thu, Jul 31, 2014 at 01:25:52PM +0900, Namhyung Kim escreveu:
>> >> Are you still against my approach - adding '/' at the end of the symfs
>> >> string itself?  It seems that mine is simpler and shorter.
>
>> > Yes, I am.
>
>> > We are not just concatenating two strings, we are joining two path
>> > components.
>
>> > I think it is more clear and elegant to do it as python os.path.join()
>> > does.
>
>> Then I think you also need to care about trailing and leading '/' in the
>> components so that, say, joining '/home/' and '/namhyung/' can result in
>> '/home/namhyung/' not '/home///namhyung/'.
>
> Well, "/home/namhyung/" is the same as "/home///namhyung/" for POSIX:
> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266
>
> So, when we can easily avoid it, no use to have a sequence of slashes,
> but otherwise it is harmless.

Yes, I know it's supported.  But I think it'd be better off avoiding it
in order to be an elegant path joiner. :)


>  
>> Btw, it seems like python's os.path.join() just use latter if it's an
>> absolute path.
>> 
>>   $ python
>>   Python 2.7.3 (default, Jul 24 2012, 10:05:38) 
>>   [GCC 4.7.0 20120507 (Red Hat 4.7.0-5)] on linux2
>>   Type "help", "copyright", "credits" or "license" for more information.
>>   >>> import os.path
>>   >>> os.path.join('/home/', '/namhyung/')
>>   '/namhyung/'
>
> Interesting, wonder what is the rationale for that or if this is a bug.

Hmm.. pydoc os.path.join says below:

 os.path.join = join(a, *p)

    Join two or more pathname components, inserting '/' as needed.  If
    any component is an absolute path, all previous path components will
    be discarded.  An empty last part will result in a path that ends
    with a separator.


Thanks,
Namhyung

Re: [PATCH 1/2] perf tools: Ensure --symfs ends with '/'

[Arnaldo Carvalho de Melo]

Em Mon, Aug 11, 2014 at 04:38:17PM +0900, Namhyung Kim escreveu:
> On Fri, 1 Aug 2014 17:15:38 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Aug 01, 2014 at 08:38:02AM +0900, Namhyung Kim escreveu:
> >> On Thu, 31 Jul 2014 09:26:21 -0300, Arnaldo Carvalho de Melo wrote:
> >> > Em Thu, Jul 31, 2014 at 01:25:52PM +0900, Namhyung Kim escreveu:
> >> >> Are you still against my approach - adding '/' at the end of the symfs
> >> >> string itself?  It seems that mine is simpler and shorter.

> >> > Yes, I am.

> >> > We are not just concatenating two strings, we are joining two path
> >> > components.

> >> > I think it is more clear and elegant to do it as python os.path.join()
> >> > does.

> >> Then I think you also need to care about trailing and leading '/' in the
> >> components so that, say, joining '/home/' and '/namhyung/' can result in
> >> '/home/namhyung/' not '/home///namhyung/'.

> > Well, "/home/namhyung/" is the same as "/home///namhyung/" for POSIX:
> > http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266

> > So, when we can easily avoid it, no use to have a sequence of slashes,
> > but otherwise it is harmless.
 
> Yes, I know it's supported.  But I think it'd be better off avoiding it
> in order to be an elegant path joiner. :)

Yeah, when we can do such things with just one or two lines of code,
that is ok, but if not, hey, the kernel will do it for us. I.e. there
_is_ code already to make it sane, to understand the intent. And it is
_always_ in place, since this is a standard.
 
> >> Btw, it seems like python's os.path.join() just use latter if it's an
> >> absolute path.
> >> 
> >>   $ python
> >>   Python 2.7.3 (default, Jul 24 2012, 10:05:38) 
> >>   [GCC 4.7.0 20120507 (Red Hat 4.7.0-5)] on linux2
> >>   Type "help", "copyright", "credits" or "license" for more information.
> >>   >>> import os.path
> >>   >>> os.path.join('/home/', '/namhyung/')
> >>   '/namhyung/'

> > Interesting, wonder what is the rationale for that or if this is a bug.
 
> Hmm.. pydoc os.path.join says below:
 
>  os.path.join = join(a, *p)
> 
>     Join two or more pathname components, inserting '/' as needed.  If
>     any component is an absolute path, all previous path components will
>     be discarded.  An empty last part will result in a path that ends
>     with a separator.

It states the behaviour, but doesn't present the rationale, that left me
wondering :)

- Arnaldo