[RFC 00/11] perf tools: Enhance parsing events tracepoint error output

[RFC 00/11] perf tools: Enhance parsing events tracepoint error output

[Jiri Olsa]

hi,
enhancing parsing events tracepoint error output. Adding
more verbose output when the tracepoint is not found or
the tracing event path cannot be access.

  $ sudo perf record -e sched:sched_krava ls
  event syntax error: 'sched:sched_krava'
                       \___ unknown tracepoint

  Error:  File /sys/kernel/debug/tracing//tracing/events/sched/sched_krava not found.
  Hint:   Perhaps this kernel misses some CONFIG_ setting to enable this feature?.

  Run 'perf list' for a list of valid events
  ...

  $ perf record -e sched:sched_krava ls
  event syntax error: 'sched:sched_krava'
                       \___ can't access trace events

  Error:  No permissions to read /sys/kernel/debug/tracing//tracing/events/sched/sched_krava
  Hint:   Try 'sudo mount -o remount,mode=755 /sys/kernel/debug'

  Run 'perf list' for a list of valid events
  ...

I changed api/fs tracefs/debugfs related code and I'm not completely
sure what were the long term intentions with this code, so please
comment ;-)

jirka


---
Jiri Olsa (11):
      tools: Add err.h with ERR_PTR PTR_ERR interface
      perf tools: Add tracing_path and remove unneeded functions
      perf tools: Do not change lib/api/fs/debugfs directly
      perf tools: Move debugfs__strerror_open into util.c object
      perf tools: Move tracing_path stuff under same namespace
      perf tools: Move tracing_path interface into trace-event-path.c
      perf tools: Make tracing_path_strerror_open message generic
      perf tools: Do not export debugfs_mountpoint and tracefs_mountpoint
      perf tools: Propagate error info for the tracepoint parsing
      perf tools: Propagate error info from tp_format
      perf tools: Enhance parsing events tracepoint error output

 tools/include/linux/err.h                  |  28 +++++++++++++++++++
 tools/lib/api/fs/debugfs.c                 |  53 +-----------------------------------
 tools/lib/api/fs/debugfs.h                 |   5 ----
 tools/lib/api/fs/tracefs.c                 |   2 +-
 tools/lib/api/fs/tracefs.h                 |   2 --
 tools/perf/builtin-trace.c                 |   4 +--
 tools/perf/perf.c                          |  11 ++++----
 tools/perf/tests/openat-syscall-all-cpus.c |   2 ++
 tools/perf/tests/openat-syscall.c          |   2 ++
 tools/perf/util/Build                      |   1 +
 tools/perf/util/evsel.c                    |   8 ++++--
 tools/perf/util/parse-events.c             |  58 ++++++++++++++++++++++++++++++----------
 tools/perf/util/parse-events.h             |   3 ++-
 tools/perf/util/parse-events.y             |  16 ++++++-----
 tools/perf/util/trace-event-path.c         | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/trace-event.c              |   7 +++--
 tools/perf/util/trace-event.h              |  12 +++++++++
 tools/perf/util/util.c                     | 119 ---------------------------------------------------------------------------------
 tools/perf/util/util.h                     |   8 ------
 19 files changed, 250 insertions(+), 220 deletions(-)
 create mode 100644 tools/include/linux/err.h
 create mode 100644 tools/perf/util/trace-event-path.c

[PATCH 01/11] tools: Add err.h with ERR_PTR PTR_ERR interface

[Jiri Olsa]

Adding part of the kernel's <linux/err.h> interface:
  inline void * __must_check ERR_PTR(long error);
  inline long   __must_check PTR_ERR(__force const void *ptr);
  inline bool   __must_check IS_ERR(__force const void *ptr);

it will be used to propagate error through pointers
in following patches.

Link: http://lkml.kernel.org/n/tip-ufgnyf683uab69anmmrabgdf@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/include/linux/err.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 tools/include/linux/err.h

diff --git a/tools/include/linux/err.h b/tools/include/linux/err.h
new file mode 100644
index 000000000000..2df92df55cfe
--- /dev/null
+++ b/tools/include/linux/err.h
@@ -0,0 +1,28 @@
+#ifndef __TOOLS_LINUX_ERR_H
+#define __TOOLS_LINUX_ERR_H
+
+#include <linux/compiler.h>
+#include <linux/types.h>
+
+#include <asm/errno.h>
+
+#define MAX_ERRNO	4095
+
+#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
+
+static inline void * __must_check ERR_PTR(long error)
+{
+	return (void *) error;
+}
+
+static inline long __must_check PTR_ERR(__force const void *ptr)
+{
+	return (long) ptr;
+}
+
+static inline bool __must_check IS_ERR(__force const void *ptr)
+{
+	return IS_ERR_VALUE((unsigned long)ptr);
+}
+
+#endif /* _LINUX_ERR_H */
-- 
2.4.3

Re: [PATCH 01/11] tools: Add err.h with ERR_PTR PTR_ERR interface

[Arnaldo Carvalho de Melo]

Em Wed, Aug 26, 2015 at 03:46:43PM +0200, Jiri Olsa escreveu:
> Adding part of the kernel's <linux/err.h> interface:
>   inline void * __must_check ERR_PTR(long error);
>   inline long   __must_check PTR_ERR(__force const void *ptr);
>   inline bool   __must_check IS_ERR(__force const void *ptr);
> 
> it will be used to propagate error through pointers
> in following patches.
> 
> Link: http://lkml.kernel.org/n/tip-ufgnyf683uab69anmmrabgdf@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/include/linux/err.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 tools/include/linux/err.h
> 
> diff --git a/tools/include/linux/err.h b/tools/include/linux/err.h
> new file mode 100644
> index 000000000000..2df92df55cfe
> --- /dev/null
> +++ b/tools/include/linux/err.h
> @@ -0,0 +1,28 @@
> +#ifndef __TOOLS_LINUX_ERR_H
> +#define __TOOLS_LINUX_ERR_H
> +
> +#include <linux/compiler.h>
> +#include <linux/types.h>
> +
> +#include <asm/errno.h>
> +

You deleted the comment in the kernel sources at this point:

/*
 * Kernel pointers have redundant information, so we can use a
 * scheme where we can return either an error code or a normal
 * pointer with the same return value.
 *
 * This should be a per-architecture thing, to allow different
 * error and pointer decisions.
 */

> +#define MAX_ERRNO	4095

Now we're dealing with user pointers, are we completely sure we can use
this trick here?

- Arnaldo

> +#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +
> +static inline void * __must_check ERR_PTR(long error)
> +{
> +	return (void *) error;
> +}
> +
> +static inline long __must_check PTR_ERR(__force const void *ptr)
> +{
> +	return (long) ptr;
> +}
> +
> +static inline bool __must_check IS_ERR(__force const void *ptr)
> +{
> +	return IS_ERR_VALUE((unsigned long)ptr);
> +}
> +
> +#endif /* _LINUX_ERR_H */
> -- 
> 2.4.3

Re: [PATCH 01/11] tools: Add err.h with ERR_PTR PTR_ERR interface

[Jiri Olsa]

On Fri, Aug 28, 2015 at 01:21:39PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 26, 2015 at 03:46:43PM +0200, Jiri Olsa escreveu:
> > Adding part of the kernel's <linux/err.h> interface:
> >   inline void * __must_check ERR_PTR(long error);
> >   inline long   __must_check PTR_ERR(__force const void *ptr);
> >   inline bool   __must_check IS_ERR(__force const void *ptr);
> > 
> > it will be used to propagate error through pointers
> > in following patches.
> > 
> > Link: http://lkml.kernel.org/n/tip-ufgnyf683uab69anmmrabgdf@git.kernel.org
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/include/linux/err.h | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 tools/include/linux/err.h
> > 
> > diff --git a/tools/include/linux/err.h b/tools/include/linux/err.h
> > new file mode 100644
> > index 000000000000..2df92df55cfe
> > --- /dev/null
> > +++ b/tools/include/linux/err.h
> > @@ -0,0 +1,28 @@
> > +#ifndef __TOOLS_LINUX_ERR_H
> > +#define __TOOLS_LINUX_ERR_H
> > +
> > +#include <linux/compiler.h>
> > +#include <linux/types.h>
> > +
> > +#include <asm/errno.h>
> > +
> 
> You deleted the comment in the kernel sources at this point:

right.. I did not want to bring too much attention :-))

> 
> /*
>  * Kernel pointers have redundant information, so we can use a
>  * scheme where we can return either an error code or a normal
>  * pointer with the same return value.
>  *
>  * This should be a per-architecture thing, to allow different
>  * error and pointer decisions.
>  */
> 
> > +#define MAX_ERRNO	4095
> 
> Now we're dealing with user pointers, are we completely sure we can use
> this trick here?

it's safe for user as well, because 'error' pointers
fall down to the unused hole:

Documentation/x86/x86_64/mm.txt:
ffffffffffe00000 - ffffffffffffffff (=2 MB) unused hole

haven't checked for other archs, but since it's used
within generic code, it should be ok

I'll put the comment back with additional explanation
wrt user space in v2

jirka

Re: [PATCH 01/11] tools: Add err.h with ERR_PTR PTR_ERR interface

[Arnaldo Carvalho de Melo]

Em Mon, Aug 31, 2015 at 09:37:18AM +0200, Jiri Olsa escreveu:
> On Fri, Aug 28, 2015 at 01:21:39PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Aug 26, 2015 at 03:46:43PM +0200, Jiri Olsa escreveu:
> > > +++ b/tools/include/linux/err.h
> > > @@ -0,0 +1,28 @@
> > > +#ifndef __TOOLS_LINUX_ERR_H
> > > +#define __TOOLS_LINUX_ERR_H
> > > +
> > > +#include <linux/compiler.h>
> > > +#include <linux/types.h>
> > > +
> > > +#include <asm/errno.h>

> > You deleted the comment in the kernel sources at this point:
 
> right.. I did not want to bring too much attention :-))

:-)

Please get the explanation about why it is safe (the unused hole bits)
and merge it with the bits from the kernel comment that make sense,
well, I think we can just do a s/Kernel//g and explain why that is so.
 
> > /*
> >  * Kernel pointers have redundant information, so we can use a
> >  * scheme where we can return either an error code or a normal
> >  * pointer with the same return value.
> >  *
> >  * This should be a per-architecture thing, to allow different
> >  * error and pointer decisions.
> >  */

> > > +#define MAX_ERRNO	4095

> > Now we're dealing with user pointers, are we completely sure we can use
> > this trick here?

> it's safe for user as well, because 'error' pointers
> fall down to the unused hole:
> 
> Documentation/x86/x86_64/mm.txt:
> ffffffffffe00000 - ffffffffffffffff (=2 MB) unused hole
> 
> haven't checked for other archs, but since it's used
> within generic code, it should be ok
> 
> I'll put the comment back with additional explanation
> wrt user space in v2

Thanks!

- Arnaldo

[PATCH 02/11] perf tools: Add tracing_path and remove unneeded functions

[Jiri Olsa]

There's no need for find_tracing_dir, because perf already
searches for debugfs/tracefs mount on start and populate
tracing_events_path.

Adding tracing_path to carry tracing dir string to be used
in get_tracing_file instead of calling find_tracing_dir.

Link: http://lkml.kernel.org/n/tip-2ji655gnw2pspo9o8u8aghic@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/util.c | 56 ++++----------------------------------------------
 tools/perf/util/util.h |  2 +-
 2 files changed, 5 insertions(+), 53 deletions(-)

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index f7adf1203df1..d33c34196a5a 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -34,6 +34,7 @@ bool test_attr__enabled;
 bool perf_host  = true;
 bool perf_guest = false;
 
+char tracing_path[PATH_MAX + 1]        = "/sys/kernel/debug/tracing";
 char tracing_events_path[PATH_MAX + 1] = "/sys/kernel/debug/tracing/events";
 
 void event_attr_init(struct perf_event_attr *attr)
@@ -391,6 +392,8 @@ void set_term_quiet_input(struct termios *old)
 
 static void set_tracing_events_path(const char *tracing, const char *mountpoint)
 {
+	snprintf(tracing_path, sizeof(tracing_path), "%s/%s",
+		 mountpoint, tracing);
 	snprintf(tracing_events_path, sizeof(tracing_events_path), "%s/%s%s",
 		 mountpoint, tracing, "events");
 }
@@ -440,62 +443,11 @@ void perf_debugfs_set_path(const char *mntpt)
 	set_tracing_events_path("tracing/", mntpt);
 }
 
-static const char *find_tracefs(void)
-{
-	const char *path = __perf_tracefs_mount(NULL);
-
-	return path;
-}
-
-static const char *find_debugfs(void)
-{
-	const char *path = __perf_debugfs_mount(NULL);
-
-	if (!path)
-		fprintf(stderr, "Your kernel does not support the debugfs filesystem");
-
-	return path;
-}
-
-/*
- * Finds the path to the debugfs/tracing
- * Allocates the string and stores it.
- */
-const char *find_tracing_dir(void)
-{
-	const char *tracing_dir = "";
-	static char *tracing;
-	static int tracing_found;
-	const char *debugfs;
-
-	if (tracing_found)
-		return tracing;
-
-	debugfs = find_tracefs();
-	if (!debugfs) {
-		tracing_dir = "/tracing";
-		debugfs = find_debugfs();
-		if (!debugfs)
-			return NULL;
-	}
-
-	if (asprintf(&tracing, "%s%s", debugfs, tracing_dir) < 0)
-		return NULL;
-
-	tracing_found = 1;
-	return tracing;
-}
-
 char *get_tracing_file(const char *name)
 {
-	const char *tracing;
 	char *file;
 
-	tracing = find_tracing_dir();
-	if (!tracing)
-		return NULL;
-
-	if (asprintf(&file, "%s/%s", tracing, name) < 0)
+	if (asprintf(&file, "%s/%s", tracing_path, name) < 0)
 		return NULL;
 
 	return file;
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 88a891562a47..291be1d84bc3 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -83,10 +83,10 @@
 extern const char *graph_line;
 extern const char *graph_dotted_line;
 extern char buildid_dir[];
+extern char tracing_path[];
 extern char tracing_events_path[];
 extern void perf_debugfs_set_path(const char *mountpoint);
 const char *perf_debugfs_mount(const char *mountpoint);
-const char *find_tracing_dir(void);
 char *get_tracing_file(const char *name);
 void put_tracing_file(char *file);
 
-- 
2.4.3

Re: [PATCH 02/11] perf tools: Add tracing_path and remove unneeded functions

[Matt Fleming]

On Wed, 26 Aug, at 03:46:44PM, Jiri Olsa wrote:
> There's no need for find_tracing_dir, because perf already
> searches for debugfs/tracefs mount on start and populate
> tracing_events_path.
 
I'm getting a bit lost searching through the git history of these
functions. Why is it safe to delete these functions? Where did the old
user that required find_tracing_dir() to mount debugfs disappear to?

The code to do the mount of debugfs/tracefs when perf starts appears
to have been around for years. Is the mounting operation of
find_tracing_dir() just dead code?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 02/11] perf tools: Add tracing_path and remove unneeded functions

[Arnaldo Carvalho de Melo]

Em Thu, Aug 27, 2015 at 11:47:19PM +0100, Matt Fleming escreveu:
> On Wed, 26 Aug, at 03:46:44PM, Jiri Olsa wrote:
> > There's no need for find_tracing_dir, because perf already
> > searches for debugfs/tracefs mount on start and populate
> > tracing_events_path.
>  
> I'm getting a bit lost searching through the git history of these
> functions. Why is it safe to delete these functions? Where did the old
> user that required find_tracing_dir() to mount debugfs disappear to?
> 
> The code to do the mount of debugfs/tracefs when perf starts appears
> to have been around for years. Is the mounting operation of
> find_tracing_dir() just dead code?

It looks like, these things should come after we realize that
tracepoints were requested in record, so we already looked where things
are mounted, etc, without digging deeper I think Jiri's patch is ok.

- Arnaldo

Re: [PATCH 02/11] perf tools: Add tracing_path and remove unneeded functions

[Jiri Olsa]

On Fri, Aug 28, 2015 at 01:27:22PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Aug 27, 2015 at 11:47:19PM +0100, Matt Fleming escreveu:
> > On Wed, 26 Aug, at 03:46:44PM, Jiri Olsa wrote:
> > > There's no need for find_tracing_dir, because perf already
> > > searches for debugfs/tracefs mount on start and populate
> > > tracing_events_path.
> >  
> > I'm getting a bit lost searching through the git history of these
> > functions. Why is it safe to delete these functions? Where did the old
> > user that required find_tracing_dir() to mount debugfs disappear to?
> > 
> > The code to do the mount of debugfs/tracefs when perf starts appears
> > to have been around for years. Is the mounting operation of
> > find_tracing_dir() just dead code?

yep, I'll address that in v2.. found TODO in fs.c ;-)

jirka

[tip:perf/core] perf tools: Add tracing_path and remove unneeded functions

[tip-bot for Jiri Olsa]

Commit-ID:  9f44f0cc1c32f1542071447a9493652bbc03facb
Gitweb:     http://git.kernel.org/tip/9f44f0cc1c32f1542071447a9493652bbc03facb
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Wed, 26 Aug 2015 15:46:44 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 28 Aug 2015 14:53:51 -0300

perf tools: Add tracing_path and remove unneeded functions

There's no need for find_tracing_dir, because perf already searches for
debugfs/tracefs mount on start and populate tracing_events_path.

Adding tracing_path to carry tracing dir string to be used in
get_tracing_file instead of calling find_tracing_dir.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Raphael Beamonte <raphael.beamonte@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1440596813-12844-3-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/util.c | 56 ++++----------------------------------------------
 tools/perf/util/util.h |  2 +-
 2 files changed, 5 insertions(+), 53 deletions(-)

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index f7adf12..d33c341 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -34,6 +34,7 @@ bool test_attr__enabled;
 bool perf_host  = true;
 bool perf_guest = false;
 
+char tracing_path[PATH_MAX + 1]        = "/sys/kernel/debug/tracing";
 char tracing_events_path[PATH_MAX + 1] = "/sys/kernel/debug/tracing/events";
 
 void event_attr_init(struct perf_event_attr *attr)
@@ -391,6 +392,8 @@ void set_term_quiet_input(struct termios *old)
 
 static void set_tracing_events_path(const char *tracing, const char *mountpoint)
 {
+	snprintf(tracing_path, sizeof(tracing_path), "%s/%s",
+		 mountpoint, tracing);
 	snprintf(tracing_events_path, sizeof(tracing_events_path), "%s/%s%s",
 		 mountpoint, tracing, "events");
 }
@@ -440,62 +443,11 @@ void perf_debugfs_set_path(const char *mntpt)
 	set_tracing_events_path("tracing/", mntpt);
 }
 
-static const char *find_tracefs(void)
-{
-	const char *path = __perf_tracefs_mount(NULL);
-
-	return path;
-}
-
-static const char *find_debugfs(void)
-{
-	const char *path = __perf_debugfs_mount(NULL);
-
-	if (!path)
-		fprintf(stderr, "Your kernel does not support the debugfs filesystem");
-
-	return path;
-}
-
-/*
- * Finds the path to the debugfs/tracing
- * Allocates the string and stores it.
- */
-const char *find_tracing_dir(void)
-{
-	const char *tracing_dir = "";
-	static char *tracing;
-	static int tracing_found;
-	const char *debugfs;
-
-	if (tracing_found)
-		return tracing;
-
-	debugfs = find_tracefs();
-	if (!debugfs) {
-		tracing_dir = "/tracing";
-		debugfs = find_debugfs();
-		if (!debugfs)
-			return NULL;
-	}
-
-	if (asprintf(&tracing, "%s%s", debugfs, tracing_dir) < 0)
-		return NULL;
-
-	tracing_found = 1;
-	return tracing;
-}
-
 char *get_tracing_file(const char *name)
 {
-	const char *tracing;
 	char *file;
 
-	tracing = find_tracing_dir();
-	if (!tracing)
-		return NULL;
-
-	if (asprintf(&file, "%s/%s", tracing, name) < 0)
+	if (asprintf(&file, "%s/%s", tracing_path, name) < 0)
 		return NULL;
 
 	return file;
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 88a8915..291be1d 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -83,10 +83,10 @@
 extern const char *graph_line;
 extern const char *graph_dotted_line;
 extern char buildid_dir[];
+extern char tracing_path[];
 extern char tracing_events_path[];
 extern void perf_debugfs_set_path(const char *mountpoint);
 const char *perf_debugfs_mount(const char *mountpoint);
-const char *find_tracing_dir(void);
 char *get_tracing_file(const char *name);
 void put_tracing_file(char *file);
 

[PATCH 03/11] perf tools: Do not change lib/api/fs/debugfs directly

[Jiri Olsa]

The tracing_events_path is the variable we want to change
via --debugfs-dir option, not the debugfs_mountpoint.

Link: http://lkml.kernel.org/n/tip-pj9htv4foum6f6uk7wzi9etx@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/perf.c      | 2 +-
 tools/perf/util/util.c | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index b857fcbd00cf..07dbff5c0e60 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -231,7 +231,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			(*argc)--;
 		} else if (!prefixcmp(cmd, CMD_DEBUGFS_DIR)) {
 			perf_debugfs_set_path(cmd + strlen(CMD_DEBUGFS_DIR));
-			fprintf(stderr, "dir: %s\n", debugfs_mountpoint);
+			fprintf(stderr, "dir: %s\n", tracing_path);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--list-cmds")) {
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index d33c34196a5a..7acafb3c5592 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -439,7 +439,6 @@ const char *perf_debugfs_mount(const char *mountpoint)
 
 void perf_debugfs_set_path(const char *mntpt)
 {
-	snprintf(debugfs_mountpoint, strlen(debugfs_mountpoint), "%s", mntpt);
 	set_tracing_events_path("tracing/", mntpt);
 }
 
-- 
2.4.3

Re: [PATCH 03/11] perf tools: Do not change lib/api/fs/debugfs directly

[Jiri Olsa]

On Wed, Aug 26, 2015 at 10:06:45AM -0400, Raphaël Beamonte wrote:
> On Aug 26, 2015 9:47 AM, "Jiri Olsa" <jolsa@kernel.org> wrote:
> >
> > The tracing_events_path is the variable we want to change
> > via --debugfs-dir option, not the debugfs_mountpoint.
> 
> <SNIP>
> 
> >                         perf_debugfs_set_path(cmd +
> strlen(CMD_DEBUGFS_DIR));
> > -                       fprintf(stderr, "dir: %s\n", debugfs_mountpoint);
> > +                       fprintf(stderr, "dir: %s\n", tracing_path);
> >                         if (envchanged)
> >                                 *envchanged = 1;
> >                 } else if (!strcmp(cmd, "--list-cmds")) {
> > diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> > index d33c34196a5a..7acafb3c5592 100644
> > --- a/tools/perf/util/util.c
> > +++ b/tools/perf/util/util.c
> > @@ -439,7 +439,6 @@ const char *perf_debugfs_mount(const char *mountpoint)
> >
> >  void perf_debugfs_set_path(const char *mntpt)
> >  {
> > -       snprintf(debugfs_mountpoint, strlen(debugfs_mountpoint), "%s",
> mntpt);
> >         set_tracing_events_path("tracing/", mntpt) ;
> >  }
> 
> Why keep a function name with debugfs in it here if we're not touching
> debugfs anymore? Shouldn't we call directly set_tracing_events_path if

please check patch 05/11

> that's what we want to do? Also, the option name --debugfs-dir is not
> entirely relevant anymore?

probably, but it's out there and someone could be using it

jirka

Re: [PATCH 03/11] perf tools: Do not change lib/api/fs/debugfs directly

[Arnaldo Carvalho de Melo]

Em Wed, Aug 26, 2015 at 10:06:45AM -0400, Raphaël Beamonte escreveu:
> On Aug 26, 2015 9:47 AM, "Jiri Olsa" <jolsa@kernel.org> wrote:
> >
> > The tracing_events_path is the variable we want to change
> > via --debugfs-dir option, not the debugfs_mountpoint.
> 
> <SNIP>
> 
> >                         perf_debugfs_set_path(cmd +
> strlen(CMD_DEBUGFS_DIR));
> > -                       fprintf(stderr, "dir: %s\n", debugfs_mountpoint);
> > +                       fprintf(stderr, "dir: %s\n", tracing_path);
> >                         if (envchanged)
> >                                 *envchanged = 1;
> >                 } else if (!strcmp(cmd, "--list-cmds")) {
> > diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> > index d33c34196a5a..7acafb3c5592 100644
> > --- a/tools/perf/util/util.c
> > +++ b/tools/perf/util/util.c
> > @@ -439,7 +439,6 @@ const char *perf_debugfs_mount(const char *mountpoint)
> >
> >  void perf_debugfs_set_path(const char *mntpt)
> >  {
> > -       snprintf(debugfs_mountpoint, strlen(debugfs_mountpoint), "%s",
> mntpt);
> >         set_tracing_events_path("tracing/", mntpt) ;
> >  }
> 
> Why keep a function name with debugfs in it here if we're not touching
> debugfs anymore?

Right

> Shouldn't we call directly set_tracing_events_path if that's what we
> want to do? Also, the option name --debugfs-dir is not entirely
> relevant anymore?

Yeah, probably that was a really bad name to start with, i.e. it
should've been named perhaps --event-definitions-dir or something to
that degree.

"debugfs", "tracefs" and the next thing to come in this area are just
implementation details :-)

- Arnaldo

Re: [PATCH 03/11] perf tools: Do not change lib/api/fs/debugfs directly

[Matt Fleming]

On Wed, 26 Aug, at 11:27:58AM, Arnaldo Carvalho de Melo wrote:
> 
> Yeah, probably that was a really bad name to start with, i.e. it
> should've been named perhaps --event-definitions-dir or something to
> that degree.
> 
> "debugfs", "tracefs" and the next thing to come in this area are just
> implementation details :-)

Perhaps there should be an alias for --debugfs-dir that more
accurately reflects the "events definition dir" use?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 03/11] perf tools: Do not change lib/api/fs/debugfs directly

[Matt Fleming]

On Wed, 26 Aug, at 03:46:45PM, Jiri Olsa wrote:
> The tracing_events_path is the variable we want to change
> via --debugfs-dir option, not the debugfs_mountpoint.
> 
> Link: http://lkml.kernel.org/n/tip-pj9htv4foum6f6uk7wzi9etx@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/perf.c      | 2 +-
>  tools/perf/util/util.c | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Matt Fleming <matt.fleming@intel.com>

-- 
Matt Fleming, Intel Open Source Technology Center

[tip:perf/core] perf tools: Do not change lib/api/fs/ debugfs directly

[tip-bot for Jiri Olsa]

Commit-ID:  9f30fffc78ca35c862f74f34cc597c7fdddc8793
Gitweb:     http://git.kernel.org/tip/9f30fffc78ca35c862f74f34cc597c7fdddc8793
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Wed, 26 Aug 2015 15:46:45 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 28 Aug 2015 14:53:53 -0300

perf tools: Do not change lib/api/fs/debugfs directly

The tracing_events_path is the variable we want to change via
--debugfs-dir option, not the debugfs_mountpoint.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Raphael Beamonte <raphael.beamonte@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1440596813-12844-4-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/perf.c      | 2 +-
 tools/perf/util/util.c | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index b857fcb..07dbff5 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -231,7 +231,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			(*argc)--;
 		} else if (!prefixcmp(cmd, CMD_DEBUGFS_DIR)) {
 			perf_debugfs_set_path(cmd + strlen(CMD_DEBUGFS_DIR));
-			fprintf(stderr, "dir: %s\n", debugfs_mountpoint);
+			fprintf(stderr, "dir: %s\n", tracing_path);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--list-cmds")) {
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index d33c341..7acafb3 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -439,7 +439,6 @@ const char *perf_debugfs_mount(const char *mountpoint)
 
 void perf_debugfs_set_path(const char *mntpt)
 {
-	snprintf(debugfs_mountpoint, strlen(debugfs_mountpoint), "%s", mntpt);
 	set_tracing_events_path("tracing/", mntpt);
 }
 

[PATCH 04/11] perf tools: Move debugfs__strerror_open into util.c object

[Jiri Olsa]

Moving debugfs__strerror_open out of api/fs/debugfs.c,
because it's not debugfs specific. It'll be changed to
consider tracefs mount as well in following patches.

Link: http://lkml.kernel.org/n/tip-bq0f0l4r0bjvy0pjp4m759kv@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/api/fs/debugfs.c | 51 ---------------------------------------------
 tools/lib/api/fs/debugfs.h |  3 ---
 tools/perf/util/util.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/util.h     |  2 ++
 4 files changed, 54 insertions(+), 54 deletions(-)

diff --git a/tools/lib/api/fs/debugfs.c b/tools/lib/api/fs/debugfs.c
index eb7cf4d18f8a..896c3595c9df 100644
--- a/tools/lib/api/fs/debugfs.c
+++ b/tools/lib/api/fs/debugfs.c
@@ -76,54 +76,3 @@ out:
 	return debugfs_mountpoint;
 }
 
-int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename)
-{
-	char sbuf[128];
-
-	switch (err) {
-	case ENOENT:
-		if (debugfs_found) {
-			snprintf(buf, size,
-				 "Error:\tFile %s/%s not found.\n"
-				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
-				 debugfs_mountpoint, filename);
-			break;
-		}
-		snprintf(buf, size, "%s",
-			 "Error:\tUnable to find debugfs\n"
-			 "Hint:\tWas your kernel compiled with debugfs support?\n"
-			 "Hint:\tIs the debugfs filesystem mounted?\n"
-			 "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
-		break;
-	case EACCES: {
-		const char *mountpoint = debugfs_mountpoint;
-
-		if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
-			const char *tracefs_mntpoint = tracefs_find_mountpoint();
-
-			if (tracefs_mntpoint)
-				mountpoint = tracefs_mntpoint;
-		}
-
-		snprintf(buf, size,
-			 "Error:\tNo permissions to read %s/%s\n"
-			 "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
-			 debugfs_mountpoint, filename, mountpoint);
-	}
-		break;
-	default:
-		snprintf(buf, size, "%s", strerror_r(err, sbuf, sizeof(sbuf)));
-		break;
-	}
-
-	return 0;
-}
-
-int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
-{
-	char path[PATH_MAX];
-
-	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
-
-	return debugfs__strerror_open(err, buf, size, path);
-}
diff --git a/tools/lib/api/fs/debugfs.h b/tools/lib/api/fs/debugfs.h
index 455023698d2b..19a618e9dbc1 100644
--- a/tools/lib/api/fs/debugfs.h
+++ b/tools/lib/api/fs/debugfs.h
@@ -17,7 +17,4 @@ char *debugfs_mount(const char *mountpoint);
 
 extern char debugfs_mountpoint[];
 
-int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename);
-int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
-
 #endif /* __API_DEBUGFS_H__ */
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 7acafb3c5592..03d1d74a5ede 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -442,6 +442,58 @@ void perf_debugfs_set_path(const char *mntpt)
 	set_tracing_events_path("tracing/", mntpt);
 }
 
+int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename)
+{
+	char sbuf[128];
+
+	switch (err) {
+	case ENOENT:
+		if (debugfs_configured()) {
+			snprintf(buf, size,
+				 "Error:\tFile %s/%s not found.\n"
+				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
+				 debugfs_mountpoint, filename);
+			break;
+		}
+		snprintf(buf, size, "%s",
+			 "Error:\tUnable to find debugfs\n"
+			 "Hint:\tWas your kernel compiled with debugfs support?\n"
+			 "Hint:\tIs the debugfs filesystem mounted?\n"
+			 "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
+		break;
+	case EACCES: {
+		const char *mountpoint = debugfs_mountpoint;
+
+		if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
+			const char *tracefs_mntpoint = tracefs_find_mountpoint();
+
+			if (tracefs_mntpoint)
+				mountpoint = tracefs_mntpoint;
+		}
+
+		snprintf(buf, size,
+			 "Error:\tNo permissions to read %s/%s\n"
+			 "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
+			 debugfs_mountpoint, filename, mountpoint);
+	}
+		break;
+	default:
+		snprintf(buf, size, "%s", strerror_r(err, sbuf, sizeof(sbuf)));
+		break;
+	}
+
+	return 0;
+}
+
+int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
+{
+	char path[PATH_MAX];
+
+	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
+
+	return debugfs__strerror_open(err, buf, size, path);
+}
+
 char *get_tracing_file(const char *name)
 {
 	char *file;
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 291be1d84bc3..da48c00ab0db 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -87,6 +87,8 @@ extern char tracing_path[];
 extern char tracing_events_path[];
 extern void perf_debugfs_set_path(const char *mountpoint);
 const char *perf_debugfs_mount(const char *mountpoint);
+int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename);
+int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
 char *get_tracing_file(const char *name);
 void put_tracing_file(char *file);
 
-- 
2.4.3

Re: [PATCH 04/11] perf tools: Move debugfs__strerror_open into util.c object

[Arnaldo Carvalho de Melo]

Em Wed, Aug 26, 2015 at 03:46:46PM +0200, Jiri Olsa escreveu:
> Moving debugfs__strerror_open out of api/fs/debugfs.c,
> because it's not debugfs specific. It'll be changed to
> consider tracefs mount as well in following patches.

If it is "debugfs__" it should not deal with tracefs, right?

My feeling is that they should be decoupled more, not the other way
around :-\

- Arnaldo
 
> Link: http://lkml.kernel.org/n/tip-bq0f0l4r0bjvy0pjp4m759kv@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/lib/api/fs/debugfs.c | 51 ---------------------------------------------
>  tools/lib/api/fs/debugfs.h |  3 ---
>  tools/perf/util/util.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/util.h     |  2 ++
>  4 files changed, 54 insertions(+), 54 deletions(-)
> 
> diff --git a/tools/lib/api/fs/debugfs.c b/tools/lib/api/fs/debugfs.c
> index eb7cf4d18f8a..896c3595c9df 100644
> --- a/tools/lib/api/fs/debugfs.c
> +++ b/tools/lib/api/fs/debugfs.c
> @@ -76,54 +76,3 @@ out:
>  	return debugfs_mountpoint;
>  }
>  
> -int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename)
> -{
> -	char sbuf[128];
> -
> -	switch (err) {
> -	case ENOENT:
> -		if (debugfs_found) {
> -			snprintf(buf, size,
> -				 "Error:\tFile %s/%s not found.\n"
> -				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
> -				 debugfs_mountpoint, filename);
> -			break;
> -		}
> -		snprintf(buf, size, "%s",
> -			 "Error:\tUnable to find debugfs\n"
> -			 "Hint:\tWas your kernel compiled with debugfs support?\n"
> -			 "Hint:\tIs the debugfs filesystem mounted?\n"
> -			 "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
> -		break;
> -	case EACCES: {
> -		const char *mountpoint = debugfs_mountpoint;
> -
> -		if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
> -			const char *tracefs_mntpoint = tracefs_find_mountpoint();
> -
> -			if (tracefs_mntpoint)
> -				mountpoint = tracefs_mntpoint;
> -		}
> -
> -		snprintf(buf, size,
> -			 "Error:\tNo permissions to read %s/%s\n"
> -			 "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
> -			 debugfs_mountpoint, filename, mountpoint);
> -	}
> -		break;
> -	default:
> -		snprintf(buf, size, "%s", strerror_r(err, sbuf, sizeof(sbuf)));
> -		break;
> -	}
> -
> -	return 0;
> -}
> -
> -int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
> -{
> -	char path[PATH_MAX];
> -
> -	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
> -
> -	return debugfs__strerror_open(err, buf, size, path);
> -}
> diff --git a/tools/lib/api/fs/debugfs.h b/tools/lib/api/fs/debugfs.h
> index 455023698d2b..19a618e9dbc1 100644
> --- a/tools/lib/api/fs/debugfs.h
> +++ b/tools/lib/api/fs/debugfs.h
> @@ -17,7 +17,4 @@ char *debugfs_mount(const char *mountpoint);
>  
>  extern char debugfs_mountpoint[];
>  
> -int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename);
> -int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
> -
>  #endif /* __API_DEBUGFS_H__ */
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 7acafb3c5592..03d1d74a5ede 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -442,6 +442,58 @@ void perf_debugfs_set_path(const char *mntpt)
>  	set_tracing_events_path("tracing/", mntpt);
>  }
>  
> +int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename)
> +{
> +	char sbuf[128];
> +
> +	switch (err) {
> +	case ENOENT:
> +		if (debugfs_configured()) {
> +			snprintf(buf, size,
> +				 "Error:\tFile %s/%s not found.\n"
> +				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
> +				 debugfs_mountpoint, filename);
> +			break;
> +		}
> +		snprintf(buf, size, "%s",
> +			 "Error:\tUnable to find debugfs\n"
> +			 "Hint:\tWas your kernel compiled with debugfs support?\n"
> +			 "Hint:\tIs the debugfs filesystem mounted?\n"
> +			 "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
> +		break;
> +	case EACCES: {
> +		const char *mountpoint = debugfs_mountpoint;
> +
> +		if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
> +			const char *tracefs_mntpoint = tracefs_find_mountpoint();
> +
> +			if (tracefs_mntpoint)
> +				mountpoint = tracefs_mntpoint;
> +		}
> +
> +		snprintf(buf, size,
> +			 "Error:\tNo permissions to read %s/%s\n"
> +			 "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
> +			 debugfs_mountpoint, filename, mountpoint);
> +	}
> +		break;
> +	default:
> +		snprintf(buf, size, "%s", strerror_r(err, sbuf, sizeof(sbuf)));
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
> +{
> +	char path[PATH_MAX];
> +
> +	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
> +
> +	return debugfs__strerror_open(err, buf, size, path);
> +}
> +
>  char *get_tracing_file(const char *name)
>  {
>  	char *file;
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 291be1d84bc3..da48c00ab0db 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -87,6 +87,8 @@ extern char tracing_path[];
>  extern char tracing_events_path[];
>  extern void perf_debugfs_set_path(const char *mountpoint);
>  const char *perf_debugfs_mount(const char *mountpoint);
> +int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename);
> +int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
>  char *get_tracing_file(const char *name);
>  void put_tracing_file(char *file);
>  
> -- 
> 2.4.3

Re: [PATCH 04/11] perf tools: Move debugfs__strerror_open into util.c object

[Jiri Olsa]

On Wed, Aug 26, 2015 at 10:52:42AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 26, 2015 at 03:46:46PM +0200, Jiri Olsa escreveu:
> > Moving debugfs__strerror_open out of api/fs/debugfs.c,
> > because it's not debugfs specific. It'll be changed to
> > consider tracefs mount as well in following patches.
> 
> If it is "debugfs__" it should not deal with tracefs, right?
> 
> My feeling is that they should be decoupled more, not the other way
> around :-\

please check 5/11:
  perf tools: Move tracing_path stuff under same namespace

jirka

Re: [PATCH 04/11] perf tools: Move debugfs__strerror_open into util.c object

[Matt Fleming]

On Wed, 26 Aug, at 03:46:46PM, Jiri Olsa wrote:
> Moving debugfs__strerror_open out of api/fs/debugfs.c,
> because it's not debugfs specific. It'll be changed to
> consider tracefs mount as well in following patches.
> 
> Link: http://lkml.kernel.org/n/tip-bq0f0l4r0bjvy0pjp4m759kv@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/lib/api/fs/debugfs.c | 51 ---------------------------------------------
>  tools/lib/api/fs/debugfs.h |  3 ---
>  tools/perf/util/util.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/util.h     |  2 ++
>  4 files changed, 54 insertions(+), 54 deletions(-)

Reviewed-by: Matt Fleming <matt.fleming@intel.com>

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH 05/11] perf tools: Move tracing_path stuff under same namespace

[Jiri Olsa]

Renaming all functions touching tracing_path under same
namespace. New interface is:

  char tracing_path[];
  char tracing_events_path[];
  - tracing mount

  void perf_tracing_path_set(const char *mountpoint);
  - setting directly tracing_path(_events), used by --debugfs-dir option

  const char *perf_tracing_path_mount(const char *mountpoint);
  - initial setup of tracing_path(_events), called from perf.c

  int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename);
  int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
  - strerror functions to get error string when failing to open
    tracepoint files

  char *get_tracing_file(const char *name);
  void put_tracing_file(char *file);
  - get/put tracing file path

Link: http://lkml.kernel.org/n/tip-miy6kftxxryjf40b3qb6eknc@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-trace.c |  4 ++--
 tools/perf/perf.c          |  8 ++++----
 tools/perf/util/util.c     | 26 +++++++++++++-------------
 tools/perf/util/util.h     |  8 ++++----
 4 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 2f1162daa3c5..384ebe32698a 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -2667,11 +2667,11 @@ out_delete_evlist:
 	char errbuf[BUFSIZ];
 
 out_error_sched_stat_runtime:
-	debugfs__strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
+	tracing_path_strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
 	goto out_error;
 
 out_error_raw_syscalls:
-	debugfs__strerror_open_tp(errno, errbuf, sizeof(errbuf), "raw_syscalls", "sys_(enter|exit)");
+	tracing_path_strerror_open_tp(errno, errbuf, sizeof(errbuf), "raw_syscalls", "sys_(enter|exit)");
 	goto out_error;
 
 out_error_mmap:
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 07dbff5c0e60..c5acdadde347 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -214,7 +214,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				fprintf(stderr, "No directory given for --debugfs-dir.\n");
 				usage(perf_usage_string);
 			}
-			perf_debugfs_set_path((*argv)[1]);
+			perf_tracing_path_set((*argv)[1]);
 			if (envchanged)
 				*envchanged = 1;
 			(*argv)++;
@@ -230,7 +230,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			(*argv)++;
 			(*argc)--;
 		} else if (!prefixcmp(cmd, CMD_DEBUGFS_DIR)) {
-			perf_debugfs_set_path(cmd + strlen(CMD_DEBUGFS_DIR));
+			perf_tracing_path_set(cmd + strlen(CMD_DEBUGFS_DIR));
 			fprintf(stderr, "dir: %s\n", tracing_path);
 			if (envchanged)
 				*envchanged = 1;
@@ -517,8 +517,8 @@ int main(int argc, const char **argv)
 	cmd = perf_extract_argv0_path(argv[0]);
 	if (!cmd)
 		cmd = "perf-help";
-	/* get debugfs mount point from /proc/mounts */
-	perf_debugfs_mount(NULL);
+	/* get debugfs/tracefs mount point from /proc/mounts */
+	perf_tracing_path_mount(NULL);
 	/*
 	 * "perf-xxxx" is the same as "perf xxxx", but we obviously:
 	 *
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 03d1d74a5ede..675d112a887d 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -390,7 +390,7 @@ void set_term_quiet_input(struct termios *old)
 	tcsetattr(0, TCSANOW, &tc);
 }
 
-static void set_tracing_events_path(const char *tracing, const char *mountpoint)
+static void tracing_path_set(const char *tracing, const char *mountpoint)
 {
 	snprintf(tracing_path, sizeof(tracing_path), "%s/%s",
 		 mountpoint, tracing);
@@ -398,7 +398,7 @@ static void set_tracing_events_path(const char *tracing, const char *mountpoint)
 		 mountpoint, tracing, "events");
 }
 
-static const char *__perf_tracefs_mount(const char *mountpoint)
+static const char *tracing_path_tracefs_mount(const char *mountpoint)
 {
 	const char *mnt;
 
@@ -406,12 +406,12 @@ static const char *__perf_tracefs_mount(const char *mountpoint)
 	if (!mnt)
 		return NULL;
 
-	set_tracing_events_path("", mnt);
+	tracing_path_set("", mnt);
 
 	return mnt;
 }
 
-static const char *__perf_debugfs_mount(const char *mountpoint)
+static const char *tracing_path_debugfs_mount(const char *mountpoint)
 {
 	const char *mnt;
 
@@ -419,30 +419,30 @@ static const char *__perf_debugfs_mount(const char *mountpoint)
 	if (!mnt)
 		return NULL;
 
-	set_tracing_events_path("tracing/", mnt);
+	tracing_path_set("tracing/", mnt);
 
 	return mnt;
 }
 
-const char *perf_debugfs_mount(const char *mountpoint)
+const char *perf_tracing_path_mount(const char *mountpoint)
 {
 	const char *mnt;
 
-	mnt = __perf_tracefs_mount(mountpoint);
+	mnt = tracing_path_tracefs_mount(mountpoint);
 	if (mnt)
 		return mnt;
 
-	mnt = __perf_debugfs_mount(mountpoint);
+	mnt = tracing_path_debugfs_mount(mountpoint);
 
 	return mnt;
 }
 
-void perf_debugfs_set_path(const char *mntpt)
+void perf_tracing_path_set(const char *mntpt)
 {
-	set_tracing_events_path("tracing/", mntpt);
+	tracing_path_set("tracing/", mntpt);
 }
 
-int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename)
+int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename)
 {
 	char sbuf[128];
 
@@ -485,13 +485,13 @@ int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename
 	return 0;
 }
 
-int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
+int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
 {
 	char path[PATH_MAX];
 
 	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
 
-	return debugfs__strerror_open(err, buf, size, path);
+	return tracing_path_strerror_open(err, buf, size, path);
 }
 
 char *get_tracing_file(const char *name)
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index da48c00ab0db..34a68faf53fe 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -85,10 +85,10 @@ extern const char *graph_dotted_line;
 extern char buildid_dir[];
 extern char tracing_path[];
 extern char tracing_events_path[];
-extern void perf_debugfs_set_path(const char *mountpoint);
-const char *perf_debugfs_mount(const char *mountpoint);
-int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename);
-int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
+extern void perf_tracing_path_set(const char *mountpoint);
+const char *perf_tracing_path_mount(const char *mountpoint);
+int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename);
+int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
 char *get_tracing_file(const char *name);
 void put_tracing_file(char *file);
 
-- 
2.4.3

Re: [PATCH 05/11] perf tools: Move tracing_path stuff under same namespace

[Arnaldo Carvalho de Melo]

Em Wed, Aug 26, 2015 at 03:46:47PM +0200, Jiri Olsa escreveu:
> Renaming all functions touching tracing_path under same
> namespace. New interface is:

But we were trying to have debugfs stuff in tools/lib/api/fs/, so that
it could eventually be used by some other tools, etc, and now we're
going the other way around, de-librarifying, not good :-\

- Arnaldo
 
>   char tracing_path[];
>   char tracing_events_path[];
>   - tracing mount
> 
>   void perf_tracing_path_set(const char *mountpoint);
>   - setting directly tracing_path(_events), used by --debugfs-dir option
> 
>   const char *perf_tracing_path_mount(const char *mountpoint);
>   - initial setup of tracing_path(_events), called from perf.c
> 
>   int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename);
>   int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
>   - strerror functions to get error string when failing to open
>     tracepoint files
> 
>   char *get_tracing_file(const char *name);
>   void put_tracing_file(char *file);
>   - get/put tracing file path
> 
> Link: http://lkml.kernel.org/n/tip-miy6kftxxryjf40b3qb6eknc@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/builtin-trace.c |  4 ++--
>  tools/perf/perf.c          |  8 ++++----
>  tools/perf/util/util.c     | 26 +++++++++++++-------------
>  tools/perf/util/util.h     |  8 ++++----
>  4 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 2f1162daa3c5..384ebe32698a 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -2667,11 +2667,11 @@ out_delete_evlist:
>  	char errbuf[BUFSIZ];
>  
>  out_error_sched_stat_runtime:
> -	debugfs__strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
> +	tracing_path_strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
>  	goto out_error;
>  
>  out_error_raw_syscalls:
> -	debugfs__strerror_open_tp(errno, errbuf, sizeof(errbuf), "raw_syscalls", "sys_(enter|exit)");
> +	tracing_path_strerror_open_tp(errno, errbuf, sizeof(errbuf), "raw_syscalls", "sys_(enter|exit)");
>  	goto out_error;
>  
>  out_error_mmap:
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index 07dbff5c0e60..c5acdadde347 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -214,7 +214,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  				fprintf(stderr, "No directory given for --debugfs-dir.\n");
>  				usage(perf_usage_string);
>  			}
> -			perf_debugfs_set_path((*argv)[1]);
> +			perf_tracing_path_set((*argv)[1]);
>  			if (envchanged)
>  				*envchanged = 1;
>  			(*argv)++;
> @@ -230,7 +230,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			(*argv)++;
>  			(*argc)--;
>  		} else if (!prefixcmp(cmd, CMD_DEBUGFS_DIR)) {
> -			perf_debugfs_set_path(cmd + strlen(CMD_DEBUGFS_DIR));
> +			perf_tracing_path_set(cmd + strlen(CMD_DEBUGFS_DIR));
>  			fprintf(stderr, "dir: %s\n", tracing_path);
>  			if (envchanged)
>  				*envchanged = 1;
> @@ -517,8 +517,8 @@ int main(int argc, const char **argv)
>  	cmd = perf_extract_argv0_path(argv[0]);
>  	if (!cmd)
>  		cmd = "perf-help";
> -	/* get debugfs mount point from /proc/mounts */
> -	perf_debugfs_mount(NULL);
> +	/* get debugfs/tracefs mount point from /proc/mounts */
> +	perf_tracing_path_mount(NULL);
>  	/*
>  	 * "perf-xxxx" is the same as "perf xxxx", but we obviously:
>  	 *
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 03d1d74a5ede..675d112a887d 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -390,7 +390,7 @@ void set_term_quiet_input(struct termios *old)
>  	tcsetattr(0, TCSANOW, &tc);
>  }
>  
> -static void set_tracing_events_path(const char *tracing, const char *mountpoint)
> +static void tracing_path_set(const char *tracing, const char *mountpoint)
>  {
>  	snprintf(tracing_path, sizeof(tracing_path), "%s/%s",
>  		 mountpoint, tracing);
> @@ -398,7 +398,7 @@ static void set_tracing_events_path(const char *tracing, const char *mountpoint)
>  		 mountpoint, tracing, "events");
>  }
>  
> -static const char *__perf_tracefs_mount(const char *mountpoint)
> +static const char *tracing_path_tracefs_mount(const char *mountpoint)
>  {
>  	const char *mnt;
>  
> @@ -406,12 +406,12 @@ static const char *__perf_tracefs_mount(const char *mountpoint)
>  	if (!mnt)
>  		return NULL;
>  
> -	set_tracing_events_path("", mnt);
> +	tracing_path_set("", mnt);
>  
>  	return mnt;
>  }
>  
> -static const char *__perf_debugfs_mount(const char *mountpoint)
> +static const char *tracing_path_debugfs_mount(const char *mountpoint)
>  {
>  	const char *mnt;
>  
> @@ -419,30 +419,30 @@ static const char *__perf_debugfs_mount(const char *mountpoint)
>  	if (!mnt)
>  		return NULL;
>  
> -	set_tracing_events_path("tracing/", mnt);
> +	tracing_path_set("tracing/", mnt);
>  
>  	return mnt;
>  }
>  
> -const char *perf_debugfs_mount(const char *mountpoint)
> +const char *perf_tracing_path_mount(const char *mountpoint)
>  {
>  	const char *mnt;
>  
> -	mnt = __perf_tracefs_mount(mountpoint);
> +	mnt = tracing_path_tracefs_mount(mountpoint);
>  	if (mnt)
>  		return mnt;
>  
> -	mnt = __perf_debugfs_mount(mountpoint);
> +	mnt = tracing_path_debugfs_mount(mountpoint);
>  
>  	return mnt;
>  }
>  
> -void perf_debugfs_set_path(const char *mntpt)
> +void perf_tracing_path_set(const char *mntpt)
>  {
> -	set_tracing_events_path("tracing/", mntpt);
> +	tracing_path_set("tracing/", mntpt);
>  }
>  
> -int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename)
> +int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename)
>  {
>  	char sbuf[128];
>  
> @@ -485,13 +485,13 @@ int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename
>  	return 0;
>  }
>  
> -int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
> +int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
>  {
>  	char path[PATH_MAX];
>  
>  	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
>  
> -	return debugfs__strerror_open(err, buf, size, path);
> +	return tracing_path_strerror_open(err, buf, size, path);
>  }
>  
>  char *get_tracing_file(const char *name)
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index da48c00ab0db..34a68faf53fe 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -85,10 +85,10 @@ extern const char *graph_dotted_line;
>  extern char buildid_dir[];
>  extern char tracing_path[];
>  extern char tracing_events_path[];
> -extern void perf_debugfs_set_path(const char *mountpoint);
> -const char *perf_debugfs_mount(const char *mountpoint);
> -int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename);
> -int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
> +extern void perf_tracing_path_set(const char *mountpoint);
> +const char *perf_tracing_path_mount(const char *mountpoint);
> +int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename);
> +int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
>  char *get_tracing_file(const char *name);
>  void put_tracing_file(char *file);
>  
> -- 
> 2.4.3

Re: [PATCH 05/11] perf tools: Move tracing_path stuff under same namespace

[Jiri Olsa]

On Wed, Aug 26, 2015 at 11:42:11AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 26, 2015 at 03:46:47PM +0200, Jiri Olsa escreveu:
> > Renaming all functions touching tracing_path under same
> > namespace. New interface is:
> 
> But we were trying to have debugfs stuff in tools/lib/api/fs/, so that
> it could eventually be used by some other tools, etc, and now we're
> going the other way around, de-librarifying, not good :-\

well this gathers tracefs/debugfs together, the api/fs/{trace|debug}fs
are just building blocks

I only moved the debugfs__strerror_open_tp out of the debugfs object
because it needs to be one layer up, because it needs to touch tracefs
as well

jirka

> 
> - Arnaldo
>  
> >   char tracing_path[];
> >   char tracing_events_path[];
> >   - tracing mount
> > 
> >   void perf_tracing_path_set(const char *mountpoint);
> >   - setting directly tracing_path(_events), used by --debugfs-dir option
> > 
> >   const char *perf_tracing_path_mount(const char *mountpoint);
> >   - initial setup of tracing_path(_events), called from perf.c
> > 
> >   int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename);
> >   int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
> >   - strerror functions to get error string when failing to open
> >     tracepoint files
> > 
> >   char *get_tracing_file(const char *name);
> >   void put_tracing_file(char *file);
> >   - get/put tracing file path
> > 
> > Link: http://lkml.kernel.org/n/tip-miy6kftxxryjf40b3qb6eknc@git.kernel.org
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/builtin-trace.c |  4 ++--
> >  tools/perf/perf.c          |  8 ++++----
> >  tools/perf/util/util.c     | 26 +++++++++++++-------------
> >  tools/perf/util/util.h     |  8 ++++----
> >  4 files changed, 23 insertions(+), 23 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index 2f1162daa3c5..384ebe32698a 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -2667,11 +2667,11 @@ out_delete_evlist:
> >  	char errbuf[BUFSIZ];
> >  
> >  out_error_sched_stat_runtime:
> > -	debugfs__strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
> > +	tracing_path_strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
> >  	goto out_error;
> >  
> >  out_error_raw_syscalls:
> > -	debugfs__strerror_open_tp(errno, errbuf, sizeof(errbuf), "raw_syscalls", "sys_(enter|exit)");
> > +	tracing_path_strerror_open_tp(errno, errbuf, sizeof(errbuf), "raw_syscalls", "sys_(enter|exit)");
> >  	goto out_error;
> >  
> >  out_error_mmap:
> > diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> > index 07dbff5c0e60..c5acdadde347 100644
> > --- a/tools/perf/perf.c
> > +++ b/tools/perf/perf.c
> > @@ -214,7 +214,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> >  				fprintf(stderr, "No directory given for --debugfs-dir.\n");
> >  				usage(perf_usage_string);
> >  			}
> > -			perf_debugfs_set_path((*argv)[1]);
> > +			perf_tracing_path_set((*argv)[1]);
> >  			if (envchanged)
> >  				*envchanged = 1;
> >  			(*argv)++;
> > @@ -230,7 +230,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> >  			(*argv)++;
> >  			(*argc)--;
> >  		} else if (!prefixcmp(cmd, CMD_DEBUGFS_DIR)) {
> > -			perf_debugfs_set_path(cmd + strlen(CMD_DEBUGFS_DIR));
> > +			perf_tracing_path_set(cmd + strlen(CMD_DEBUGFS_DIR));
> >  			fprintf(stderr, "dir: %s\n", tracing_path);
> >  			if (envchanged)
> >  				*envchanged = 1;
> > @@ -517,8 +517,8 @@ int main(int argc, const char **argv)
> >  	cmd = perf_extract_argv0_path(argv[0]);
> >  	if (!cmd)
> >  		cmd = "perf-help";
> > -	/* get debugfs mount point from /proc/mounts */
> > -	perf_debugfs_mount(NULL);
> > +	/* get debugfs/tracefs mount point from /proc/mounts */
> > +	perf_tracing_path_mount(NULL);
> >  	/*
> >  	 * "perf-xxxx" is the same as "perf xxxx", but we obviously:
> >  	 *
> > diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> > index 03d1d74a5ede..675d112a887d 100644
> > --- a/tools/perf/util/util.c
> > +++ b/tools/perf/util/util.c
> > @@ -390,7 +390,7 @@ void set_term_quiet_input(struct termios *old)
> >  	tcsetattr(0, TCSANOW, &tc);
> >  }
> >  
> > -static void set_tracing_events_path(const char *tracing, const char *mountpoint)
> > +static void tracing_path_set(const char *tracing, const char *mountpoint)
> >  {
> >  	snprintf(tracing_path, sizeof(tracing_path), "%s/%s",
> >  		 mountpoint, tracing);
> > @@ -398,7 +398,7 @@ static void set_tracing_events_path(const char *tracing, const char *mountpoint)
> >  		 mountpoint, tracing, "events");
> >  }
> >  
> > -static const char *__perf_tracefs_mount(const char *mountpoint)
> > +static const char *tracing_path_tracefs_mount(const char *mountpoint)
> >  {
> >  	const char *mnt;
> >  
> > @@ -406,12 +406,12 @@ static const char *__perf_tracefs_mount(const char *mountpoint)
> >  	if (!mnt)
> >  		return NULL;
> >  
> > -	set_tracing_events_path("", mnt);
> > +	tracing_path_set("", mnt);
> >  
> >  	return mnt;
> >  }
> >  
> > -static const char *__perf_debugfs_mount(const char *mountpoint)
> > +static const char *tracing_path_debugfs_mount(const char *mountpoint)
> >  {
> >  	const char *mnt;
> >  
> > @@ -419,30 +419,30 @@ static const char *__perf_debugfs_mount(const char *mountpoint)
> >  	if (!mnt)
> >  		return NULL;
> >  
> > -	set_tracing_events_path("tracing/", mnt);
> > +	tracing_path_set("tracing/", mnt);
> >  
> >  	return mnt;
> >  }
> >  
> > -const char *perf_debugfs_mount(const char *mountpoint)
> > +const char *perf_tracing_path_mount(const char *mountpoint)
> >  {
> >  	const char *mnt;
> >  
> > -	mnt = __perf_tracefs_mount(mountpoint);
> > +	mnt = tracing_path_tracefs_mount(mountpoint);
> >  	if (mnt)
> >  		return mnt;
> >  
> > -	mnt = __perf_debugfs_mount(mountpoint);
> > +	mnt = tracing_path_debugfs_mount(mountpoint);
> >  
> >  	return mnt;
> >  }
> >  
> > -void perf_debugfs_set_path(const char *mntpt)
> > +void perf_tracing_path_set(const char *mntpt)
> >  {
> > -	set_tracing_events_path("tracing/", mntpt);
> > +	tracing_path_set("tracing/", mntpt);
> >  }
> >  
> > -int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename)
> > +int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename)
> >  {
> >  	char sbuf[128];
> >  
> > @@ -485,13 +485,13 @@ int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename
> >  	return 0;
> >  }
> >  
> > -int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
> > +int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
> >  {
> >  	char path[PATH_MAX];
> >  
> >  	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
> >  
> > -	return debugfs__strerror_open(err, buf, size, path);
> > +	return tracing_path_strerror_open(err, buf, size, path);
> >  }
> >  
> >  char *get_tracing_file(const char *name)
> > diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> > index da48c00ab0db..34a68faf53fe 100644
> > --- a/tools/perf/util/util.h
> > +++ b/tools/perf/util/util.h
> > @@ -85,10 +85,10 @@ extern const char *graph_dotted_line;
> >  extern char buildid_dir[];
> >  extern char tracing_path[];
> >  extern char tracing_events_path[];
> > -extern void perf_debugfs_set_path(const char *mountpoint);
> > -const char *perf_debugfs_mount(const char *mountpoint);
> > -int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename);
> > -int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
> > +extern void perf_tracing_path_set(const char *mountpoint);
> > +const char *perf_tracing_path_mount(const char *mountpoint);
> > +int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename);
> > +int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
> >  char *get_tracing_file(const char *name);
> >  void put_tracing_file(char *file);
> >  
> > -- 
> > 2.4.3

Re: [PATCH 05/11] perf tools: Move tracing_path stuff under same namespace

[Arnaldo Carvalho de Melo]

Em Wed, Aug 26, 2015 at 04:48:36PM +0200, Jiri Olsa escreveu:
> On Wed, Aug 26, 2015 at 11:42:11AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Aug 26, 2015 at 03:46:47PM +0200, Jiri Olsa escreveu:
> > > Renaming all functions touching tracing_path under same
> > > namespace. New interface is:
> > 
> > But we were trying to have debugfs stuff in tools/lib/api/fs/, so that
> > it could eventually be used by some other tools, etc, and now we're
> > going the other way around, de-librarifying, not good :-\
> 
> well this gathers tracefs/debugfs together, the api/fs/{trace|debug}fs
> are just building blocks
> 
> I only moved the debugfs__strerror_open_tp out of the debugfs object
> because it needs to be one layer up, because it needs to touch tracefs
> as well

Why not leave it there, since, for historical reasons, what is tracefs
now was something implemented inside debugfs, so having that special
case in debugfs__strerror_open_tp():

                if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {

Is ok, since tools will need to deal with older kernels, etc.

But ok, I see your point, we should rename it to something like
tracepoint__strerror_open_definition(), so as to detach this from
"debugfs" _and_ "tracefs", pseudo-filesystem based DWARF-like stuff
(event definitions), but then why not have that somewhere in
tools/lib/api/tracepoint/.

Or was it the difficulty in getting this nicely namespaced what made
you get it back to tools/perf/util/?

I can have some empathy for that, if that is the case ;-\

- Arnaldo
 
> > 
> > - Arnaldo
> >  
> > >   char tracing_path[];
> > >   char tracing_events_path[];
> > >   - tracing mount
> > > 
> > >   void perf_tracing_path_set(const char *mountpoint);
> > >   - setting directly tracing_path(_events), used by --debugfs-dir option
> > > 
> > >   const char *perf_tracing_path_mount(const char *mountpoint);
> > >   - initial setup of tracing_path(_events), called from perf.c
> > > 
> > >   int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename);
> > >   int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
> > >   - strerror functions to get error string when failing to open
> > >     tracepoint files
> > > 
> > >   char *get_tracing_file(const char *name);
> > >   void put_tracing_file(char *file);
> > >   - get/put tracing file path
> > > 
> > > Link: http://lkml.kernel.org/n/tip-miy6kftxxryjf40b3qb6eknc@git.kernel.org
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/perf/builtin-trace.c |  4 ++--
> > >  tools/perf/perf.c          |  8 ++++----
> > >  tools/perf/util/util.c     | 26 +++++++++++++-------------
> > >  tools/perf/util/util.h     |  8 ++++----
> > >  4 files changed, 23 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > > index 2f1162daa3c5..384ebe32698a 100644
> > > --- a/tools/perf/builtin-trace.c
> > > +++ b/tools/perf/builtin-trace.c
> > > @@ -2667,11 +2667,11 @@ out_delete_evlist:
> > >  	char errbuf[BUFSIZ];
> > >  
> > >  out_error_sched_stat_runtime:
> > > -	debugfs__strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
> > > +	tracing_path_strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
> > >  	goto out_error;
> > >  
> > >  out_error_raw_syscalls:
> > > -	debugfs__strerror_open_tp(errno, errbuf, sizeof(errbuf), "raw_syscalls", "sys_(enter|exit)");
> > > +	tracing_path_strerror_open_tp(errno, errbuf, sizeof(errbuf), "raw_syscalls", "sys_(enter|exit)");
> > >  	goto out_error;
> > >  
> > >  out_error_mmap:
> > > diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> > > index 07dbff5c0e60..c5acdadde347 100644
> > > --- a/tools/perf/perf.c
> > > +++ b/tools/perf/perf.c
> > > @@ -214,7 +214,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> > >  				fprintf(stderr, "No directory given for --debugfs-dir.\n");
> > >  				usage(perf_usage_string);
> > >  			}
> > > -			perf_debugfs_set_path((*argv)[1]);
> > > +			perf_tracing_path_set((*argv)[1]);
> > >  			if (envchanged)
> > >  				*envchanged = 1;
> > >  			(*argv)++;
> > > @@ -230,7 +230,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> > >  			(*argv)++;
> > >  			(*argc)--;
> > >  		} else if (!prefixcmp(cmd, CMD_DEBUGFS_DIR)) {
> > > -			perf_debugfs_set_path(cmd + strlen(CMD_DEBUGFS_DIR));
> > > +			perf_tracing_path_set(cmd + strlen(CMD_DEBUGFS_DIR));
> > >  			fprintf(stderr, "dir: %s\n", tracing_path);
> > >  			if (envchanged)
> > >  				*envchanged = 1;
> > > @@ -517,8 +517,8 @@ int main(int argc, const char **argv)
> > >  	cmd = perf_extract_argv0_path(argv[0]);
> > >  	if (!cmd)
> > >  		cmd = "perf-help";
> > > -	/* get debugfs mount point from /proc/mounts */
> > > -	perf_debugfs_mount(NULL);
> > > +	/* get debugfs/tracefs mount point from /proc/mounts */
> > > +	perf_tracing_path_mount(NULL);
> > >  	/*
> > >  	 * "perf-xxxx" is the same as "perf xxxx", but we obviously:
> > >  	 *
> > > diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> > > index 03d1d74a5ede..675d112a887d 100644
> > > --- a/tools/perf/util/util.c
> > > +++ b/tools/perf/util/util.c
> > > @@ -390,7 +390,7 @@ void set_term_quiet_input(struct termios *old)
> > >  	tcsetattr(0, TCSANOW, &tc);
> > >  }
> > >  
> > > -static void set_tracing_events_path(const char *tracing, const char *mountpoint)
> > > +static void tracing_path_set(const char *tracing, const char *mountpoint)
> > >  {
> > >  	snprintf(tracing_path, sizeof(tracing_path), "%s/%s",
> > >  		 mountpoint, tracing);
> > > @@ -398,7 +398,7 @@ static void set_tracing_events_path(const char *tracing, const char *mountpoint)
> > >  		 mountpoint, tracing, "events");
> > >  }
> > >  
> > > -static const char *__perf_tracefs_mount(const char *mountpoint)
> > > +static const char *tracing_path_tracefs_mount(const char *mountpoint)
> > >  {
> > >  	const char *mnt;
> > >  
> > > @@ -406,12 +406,12 @@ static const char *__perf_tracefs_mount(const char *mountpoint)
> > >  	if (!mnt)
> > >  		return NULL;
> > >  
> > > -	set_tracing_events_path("", mnt);
> > > +	tracing_path_set("", mnt);
> > >  
> > >  	return mnt;
> > >  }
> > >  
> > > -static const char *__perf_debugfs_mount(const char *mountpoint)
> > > +static const char *tracing_path_debugfs_mount(const char *mountpoint)
> > >  {
> > >  	const char *mnt;
> > >  
> > > @@ -419,30 +419,30 @@ static const char *__perf_debugfs_mount(const char *mountpoint)
> > >  	if (!mnt)
> > >  		return NULL;
> > >  
> > > -	set_tracing_events_path("tracing/", mnt);
> > > +	tracing_path_set("tracing/", mnt);
> > >  
> > >  	return mnt;
> > >  }
> > >  
> > > -const char *perf_debugfs_mount(const char *mountpoint)
> > > +const char *perf_tracing_path_mount(const char *mountpoint)
> > >  {
> > >  	const char *mnt;
> > >  
> > > -	mnt = __perf_tracefs_mount(mountpoint);
> > > +	mnt = tracing_path_tracefs_mount(mountpoint);
> > >  	if (mnt)
> > >  		return mnt;
> > >  
> > > -	mnt = __perf_debugfs_mount(mountpoint);
> > > +	mnt = tracing_path_debugfs_mount(mountpoint);
> > >  
> > >  	return mnt;
> > >  }
> > >  
> > > -void perf_debugfs_set_path(const char *mntpt)
> > > +void perf_tracing_path_set(const char *mntpt)
> > >  {
> > > -	set_tracing_events_path("tracing/", mntpt);
> > > +	tracing_path_set("tracing/", mntpt);
> > >  }
> > >  
> > > -int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename)
> > > +int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename)
> > >  {
> > >  	char sbuf[128];
> > >  
> > > @@ -485,13 +485,13 @@ int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename
> > >  	return 0;
> > >  }
> > >  
> > > -int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
> > > +int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
> > >  {
> > >  	char path[PATH_MAX];
> > >  
> > >  	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
> > >  
> > > -	return debugfs__strerror_open(err, buf, size, path);
> > > +	return tracing_path_strerror_open(err, buf, size, path);
> > >  }
> > >  
> > >  char *get_tracing_file(const char *name)
> > > diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> > > index da48c00ab0db..34a68faf53fe 100644
> > > --- a/tools/perf/util/util.h
> > > +++ b/tools/perf/util/util.h
> > > @@ -85,10 +85,10 @@ extern const char *graph_dotted_line;
> > >  extern char buildid_dir[];
> > >  extern char tracing_path[];
> > >  extern char tracing_events_path[];
> > > -extern void perf_debugfs_set_path(const char *mountpoint);
> > > -const char *perf_debugfs_mount(const char *mountpoint);
> > > -int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename);
> > > -int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
> > > +extern void perf_tracing_path_set(const char *mountpoint);
> > > +const char *perf_tracing_path_mount(const char *mountpoint);
> > > +int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename);
> > > +int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
> > >  char *get_tracing_file(const char *name);
> > >  void put_tracing_file(char *file);
> > >  
> > > -- 
> > > 2.4.3

Re: [PATCH 05/11] perf tools: Move tracing_path stuff under same namespace

[Jiri Olsa]

On Wed, Aug 26, 2015 at 11:58:55AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 26, 2015 at 04:48:36PM +0200, Jiri Olsa escreveu:
> > On Wed, Aug 26, 2015 at 11:42:11AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, Aug 26, 2015 at 03:46:47PM +0200, Jiri Olsa escreveu:
> > > > Renaming all functions touching tracing_path under same
> > > > namespace. New interface is:
> > > 
> > > But we were trying to have debugfs stuff in tools/lib/api/fs/, so that
> > > it could eventually be used by some other tools, etc, and now we're
> > > going the other way around, de-librarifying, not good :-\
> > 
> > well this gathers tracefs/debugfs together, the api/fs/{trace|debug}fs
> > are just building blocks
> > 
> > I only moved the debugfs__strerror_open_tp out of the debugfs object
> > because it needs to be one layer up, because it needs to touch tracefs
> > as well
> 
> Why not leave it there, since, for historical reasons, what is tracefs
> now was something implemented inside debugfs, so having that special
> case in debugfs__strerror_open_tp():
> 
>                 if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
> 
> Is ok, since tools will need to deal with older kernels, etc.
> 
> But ok, I see your point, we should rename it to something like
> tracepoint__strerror_open_definition(), so as to detach this from
> "debugfs" _and_ "tracefs", pseudo-filesystem based DWARF-like stuff
> (event definitions), but then why not have that somewhere in
> tools/lib/api/tracepoint/.

no problem with moving it over to the lib

how about having api/fs/tpfs
                 or     eventsfs
                 or     tracingfs


doing the same as tracing_path* stuff ATM

jirka

Re: [PATCH 05/11] perf tools: Move tracing_path stuff under same namespace

[Matt Fleming]

On Wed, 26 Aug, at 05:06:37PM, Jiri Olsa wrote:
> 
> no problem with moving it over to the lib
> 
> how about having api/fs/tpfs
>                  or     eventsfs
>                  or     tracingfs
> 
> 
> doing the same as tracing_path* stuff ATM

But we're talking about tracing path components here right and not a
file system? It'd make more sense to me to have this logically above
the file system layers.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 05/11] perf tools: Move tracing_path stuff under same namespace

[Jiri Olsa]

On Fri, Aug 28, 2015 at 02:06:50PM +0100, Matt Fleming wrote:
> On Wed, 26 Aug, at 05:06:37PM, Jiri Olsa wrote:
> > 
> > no problem with moving it over to the lib
> > 
> > how about having api/fs/tpfs
> >                  or     eventsfs
> >                  or     tracingfs
> > 
> > 
> > doing the same as tracing_path* stuff ATM
> 
> But we're talking about tracing path components here right and not a
> file system? It'd make more sense to me to have this logically above
> the file system layers.

right, on the other side it's fs related.. just holder
for preffered place to read tracepoints from

I dont have strong opinion on where to place it,
but I dont see better place.. fs/tracing_path.c ?

jirka

[PATCH 06/11] perf tools: Move tracing_path interface into trace-event-path.c

[Jiri Olsa]

Moving tracing_path interface into trace-event-path.c
out of util.c.

Link: http://lkml.kernel.org/n/tip-xqvrud2e3z4uynvnu3imlu2y@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/perf.c                          |   1 +
 tools/perf/tests/openat-syscall-all-cpus.c |   2 +
 tools/perf/tests/openat-syscall.c          |   2 +
 tools/perf/util/Build                      |   1 +
 tools/perf/util/trace-event-path.c         | 129 +++++++++++++++++++++++++++++
 tools/perf/util/trace-event.h              |  12 +++
 tools/perf/util/util.c                     | 122 ---------------------------
 tools/perf/util/util.h                     |  10 ---
 8 files changed, 147 insertions(+), 132 deletions(-)
 create mode 100644 tools/perf/util/trace-event-path.c

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index c5acdadde347..952a1becfd6c 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -15,6 +15,7 @@
 #include "util/parse-events.h"
 #include "util/parse-options.h"
 #include "util/debug.h"
+#include "util/trace-event.h"
 #include <api/fs/debugfs.h>
 #include <pthread.h>
 
diff --git a/tools/perf/tests/openat-syscall-all-cpus.c b/tools/perf/tests/openat-syscall-all-cpus.c
index a572f87e9c8d..caac16359b78 100644
--- a/tools/perf/tests/openat-syscall-all-cpus.c
+++ b/tools/perf/tests/openat-syscall-all-cpus.c
@@ -1,3 +1,5 @@
+#include <api/fs/debugfs.h>
+#include <api/fs/tracefs.h>
 #include "evsel.h"
 #include "tests.h"
 #include "thread_map.h"
diff --git a/tools/perf/tests/openat-syscall.c b/tools/perf/tests/openat-syscall.c
index c9a37bc6b33a..087c2b7b6296 100644
--- a/tools/perf/tests/openat-syscall.c
+++ b/tools/perf/tests/openat-syscall.c
@@ -1,3 +1,5 @@
+#include <api/fs/debugfs.h>
+#include <api/fs/tracefs.h>
 #include "thread_map.h"
 #include "evsel.h"
 #include "debug.h"
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index e912856cc4e5..9ac357016c56 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -56,6 +56,7 @@ libperf-y += pmu-bison.o
 libperf-y += trace-event-read.o
 libperf-y += trace-event-info.o
 libperf-y += trace-event-scripting.o
+libperf-y += trace-event-path.o
 libperf-y += trace-event.o
 libperf-y += svghelper.o
 libperf-y += sort.o
diff --git a/tools/perf/util/trace-event-path.c b/tools/perf/util/trace-event-path.c
new file mode 100644
index 000000000000..e557187dcfce
--- /dev/null
+++ b/tools/perf/util/trace-event-path.c
@@ -0,0 +1,129 @@
+#include <stdio.h>
+#include <unistd.h>
+#include <errno.h>
+#include "trace-event.h"
+#include <api/fs/debugfs.h>
+#include <api/fs/tracefs.h>
+
+char tracing_path[PATH_MAX + 1]        = "/sys/kernel/debug/tracing";
+char tracing_events_path[PATH_MAX + 1] = "/sys/kernel/debug/tracing/events";
+
+static void tracing_path_set(const char *tracing, const char *mountpoint)
+{
+	snprintf(tracing_path, sizeof(tracing_path), "%s/%s",
+		 mountpoint, tracing);
+	snprintf(tracing_events_path, sizeof(tracing_events_path), "%s/%s%s",
+		 mountpoint, tracing, "events");
+}
+
+static const char *tracing_path_tracefs_mount(const char *mountpoint)
+{
+	const char *mnt;
+
+	mnt = tracefs_mount(mountpoint);
+	if (!mnt)
+		return NULL;
+
+	tracing_path_set("", mnt);
+
+	return mnt;
+}
+
+static const char *tracing_path_debugfs_mount(const char *mountpoint)
+{
+	const char *mnt;
+
+	mnt = debugfs_mount(mountpoint);
+	if (!mnt)
+		return NULL;
+
+	tracing_path_set("tracing/", mnt);
+
+	return mnt;
+}
+
+const char *perf_tracing_path_mount(const char *mountpoint)
+{
+	const char *mnt;
+
+	mnt = tracing_path_tracefs_mount(mountpoint);
+	if (mnt)
+		return mnt;
+
+	mnt = tracing_path_debugfs_mount(mountpoint);
+
+	return mnt;
+}
+
+void perf_tracing_path_set(const char *mntpt)
+{
+	tracing_path_set("tracing/", mntpt);
+}
+
+int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename)
+{
+	char sbuf[128];
+
+	switch (err) {
+	case ENOENT:
+		if (debugfs_configured()) {
+			snprintf(buf, size,
+				 "Error:\tFile %s/%s not found.\n"
+				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
+				 debugfs_mountpoint, filename);
+			break;
+		}
+		snprintf(buf, size, "%s",
+			 "Error:\tUnable to find debugfs\n"
+			 "Hint:\tWas your kernel compiled with debugfs support?\n"
+			 "Hint:\tIs the debugfs filesystem mounted?\n"
+			 "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
+		break;
+	case EACCES: {
+		const char *mountpoint = debugfs_mountpoint;
+
+		if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
+			const char *tracefs_mntpoint = tracefs_find_mountpoint();
+
+			if (tracefs_mntpoint)
+				mountpoint = tracefs_mntpoint;
+		}
+
+		snprintf(buf, size,
+			 "Error:\tNo permissions to read %s/%s\n"
+			 "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
+			 debugfs_mountpoint, filename, mountpoint);
+	}
+		break;
+	default:
+		snprintf(buf, size, "%s", strerror_r(err, sbuf, sizeof(sbuf)));
+		break;
+	}
+
+	return 0;
+}
+
+int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
+{
+	char path[PATH_MAX];
+
+	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
+
+	return tracing_path_strerror_open(err, buf, size, path);
+}
+
+char *get_tracing_file(const char *name)
+{
+	char *file;
+
+	if (asprintf(&file, "%s/%s", tracing_path, name) < 0)
+		return NULL;
+
+	return file;
+}
+
+void put_tracing_file(char *file)
+{
+	free(file);
+}
+
diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
index da6cc4cc2a4f..59bd98a26b25 100644
--- a/tools/perf/util/trace-event.h
+++ b/tools/perf/util/trace-event.h
@@ -92,4 +92,16 @@ int common_pc(struct scripting_context *context);
 int common_flags(struct scripting_context *context);
 int common_lock_depth(struct scripting_context *context);
 
+/*
+ * The tracing_path interface from trace-event-path.c object.
+ */
+extern char tracing_path[];
+extern char tracing_events_path[];
+extern void perf_tracing_path_set(const char *mountpoint);
+const char *perf_tracing_path_mount(const char *mountpoint);
+int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename);
+int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
+char *get_tracing_file(const char *name);
+void put_tracing_file(char *file);
+
 #endif /* _PERF_UTIL_TRACE_EVENT_H */
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 675d112a887d..49a5c6ad55f5 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -34,9 +34,6 @@ bool test_attr__enabled;
 bool perf_host  = true;
 bool perf_guest = false;
 
-char tracing_path[PATH_MAX + 1]        = "/sys/kernel/debug/tracing";
-char tracing_events_path[PATH_MAX + 1] = "/sys/kernel/debug/tracing/events";
-
 void event_attr_init(struct perf_event_attr *attr)
 {
 	if (!perf_host)
@@ -390,125 +387,6 @@ void set_term_quiet_input(struct termios *old)
 	tcsetattr(0, TCSANOW, &tc);
 }
 
-static void tracing_path_set(const char *tracing, const char *mountpoint)
-{
-	snprintf(tracing_path, sizeof(tracing_path), "%s/%s",
-		 mountpoint, tracing);
-	snprintf(tracing_events_path, sizeof(tracing_events_path), "%s/%s%s",
-		 mountpoint, tracing, "events");
-}
-
-static const char *tracing_path_tracefs_mount(const char *mountpoint)
-{
-	const char *mnt;
-
-	mnt = tracefs_mount(mountpoint);
-	if (!mnt)
-		return NULL;
-
-	tracing_path_set("", mnt);
-
-	return mnt;
-}
-
-static const char *tracing_path_debugfs_mount(const char *mountpoint)
-{
-	const char *mnt;
-
-	mnt = debugfs_mount(mountpoint);
-	if (!mnt)
-		return NULL;
-
-	tracing_path_set("tracing/", mnt);
-
-	return mnt;
-}
-
-const char *perf_tracing_path_mount(const char *mountpoint)
-{
-	const char *mnt;
-
-	mnt = tracing_path_tracefs_mount(mountpoint);
-	if (mnt)
-		return mnt;
-
-	mnt = tracing_path_debugfs_mount(mountpoint);
-
-	return mnt;
-}
-
-void perf_tracing_path_set(const char *mntpt)
-{
-	tracing_path_set("tracing/", mntpt);
-}
-
-int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename)
-{
-	char sbuf[128];
-
-	switch (err) {
-	case ENOENT:
-		if (debugfs_configured()) {
-			snprintf(buf, size,
-				 "Error:\tFile %s/%s not found.\n"
-				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
-				 debugfs_mountpoint, filename);
-			break;
-		}
-		snprintf(buf, size, "%s",
-			 "Error:\tUnable to find debugfs\n"
-			 "Hint:\tWas your kernel compiled with debugfs support?\n"
-			 "Hint:\tIs the debugfs filesystem mounted?\n"
-			 "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
-		break;
-	case EACCES: {
-		const char *mountpoint = debugfs_mountpoint;
-
-		if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
-			const char *tracefs_mntpoint = tracefs_find_mountpoint();
-
-			if (tracefs_mntpoint)
-				mountpoint = tracefs_mntpoint;
-		}
-
-		snprintf(buf, size,
-			 "Error:\tNo permissions to read %s/%s\n"
-			 "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
-			 debugfs_mountpoint, filename, mountpoint);
-	}
-		break;
-	default:
-		snprintf(buf, size, "%s", strerror_r(err, sbuf, sizeof(sbuf)));
-		break;
-	}
-
-	return 0;
-}
-
-int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
-{
-	char path[PATH_MAX];
-
-	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
-
-	return tracing_path_strerror_open(err, buf, size, path);
-}
-
-char *get_tracing_file(const char *name)
-{
-	char *file;
-
-	if (asprintf(&file, "%s/%s", tracing_path, name) < 0)
-		return NULL;
-
-	return file;
-}
-
-void put_tracing_file(char *file)
-{
-	free(file);
-}
-
 int parse_nsec_time(const char *str, u64 *ptime)
 {
 	u64 time_sec, time_nsec;
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 34a68faf53fe..4c812f31557c 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -74,8 +74,6 @@
 #include <linux/magic.h>
 #include <linux/types.h>
 #include <sys/ttydefaults.h>
-#include <api/fs/debugfs.h>
-#include <api/fs/tracefs.h>
 #include <termios.h>
 #include <linux/bitops.h>
 #include <termios.h>
@@ -83,14 +81,6 @@
 extern const char *graph_line;
 extern const char *graph_dotted_line;
 extern char buildid_dir[];
-extern char tracing_path[];
-extern char tracing_events_path[];
-extern void perf_tracing_path_set(const char *mountpoint);
-const char *perf_tracing_path_mount(const char *mountpoint);
-int tracing_path_strerror_open(int err, char *buf, size_t size, const char *filename);
-int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
-char *get_tracing_file(const char *name);
-void put_tracing_file(char *file);
 
 /* On most systems <limits.h> would have given us this, but
  * not on some systems (e.g. GNU/Hurd).
-- 
2.4.3

[PATCH 07/11] perf tools: Make tracing_path_strerror_open message generic

[Jiri Olsa]

Making tracing_path_strerror_open message generic by using
both debugfs/tracefs words in error message plus the tracing_path
instead of debugfs_mountpoint.

Link: http://lkml.kernel.org/n/tip-5y7nboe2xe619hp649ry58z6@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/trace-event-path.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/trace-event-path.c b/tools/perf/util/trace-event-path.c
index e557187dcfce..72c9f66908ba 100644
--- a/tools/perf/util/trace-event-path.c
+++ b/tools/perf/util/trace-event-path.c
@@ -66,33 +66,33 @@ int tracing_path_strerror_open(int err, char *buf, size_t size, const char *file
 
 	switch (err) {
 	case ENOENT:
-		if (debugfs_configured()) {
+		if (debugfs_configured() || tracefs_configured()) {
 			snprintf(buf, size,
 				 "Error:\tFile %s/%s not found.\n"
 				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
-				 debugfs_mountpoint, filename);
+				 tracing_events_path, filename);
 			break;
 		}
 		snprintf(buf, size, "%s",
-			 "Error:\tUnable to find debugfs\n"
-			 "Hint:\tWas your kernel compiled with debugfs support?\n"
-			 "Hint:\tIs the debugfs filesystem mounted?\n"
+			 "Error:\tUnable to find debugfs/tracefs\n"
+			 "Hint:\tWas your kernel compiled with debugfs/tracefs support?\n"
+			 "Hint:\tIs the debugfs/tracefs filesystem mounted?\n"
 			 "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
 		break;
 	case EACCES: {
-		const char *mountpoint = debugfs_mountpoint;
+		const char *mountpoint = debugfs_find_mountpoint();
 
-		if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
+		if (!access(mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
 			const char *tracefs_mntpoint = tracefs_find_mountpoint();
 
 			if (tracefs_mntpoint)
-				mountpoint = tracefs_mntpoint;
+				mountpoint = tracefs_find_mountpoint();
 		}
 
 		snprintf(buf, size,
 			 "Error:\tNo permissions to read %s/%s\n"
 			 "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
-			 debugfs_mountpoint, filename, mountpoint);
+			 tracing_events_path, filename, mountpoint);
 	}
 		break;
 	default:
@@ -107,7 +107,7 @@ int tracing_path_strerror_open_tp(int err, char *buf, size_t size, const char *s
 {
 	char path[PATH_MAX];
 
-	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
+	snprintf(path, PATH_MAX, "%s/%s", sys, name ?: "*");
 
 	return tracing_path_strerror_open(err, buf, size, path);
 }
-- 
2.4.3

[PATCH 08/11] perf tools: Do not export debugfs_mountpoint and tracefs_mountpoint

[Jiri Olsa]

Both debugfs_mountpoint and tracefs_mountpoint should not be changed
externally.  Both mounpoints are returned by following interface:

  debugfs_find_mountpoint
  tracefs_find_mountpoint

Link: http://lkml.kernel.org/n/tip-ptrbv90c13ebticnfu1ysye7@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/api/fs/debugfs.c | 2 +-
 tools/lib/api/fs/debugfs.h | 2 --
 tools/lib/api/fs/tracefs.c | 2 +-
 tools/lib/api/fs/tracefs.h | 2 --
 4 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/lib/api/fs/debugfs.c b/tools/lib/api/fs/debugfs.c
index 896c3595c9df..9e7e039a99ae 100644
--- a/tools/lib/api/fs/debugfs.c
+++ b/tools/lib/api/fs/debugfs.c
@@ -18,7 +18,7 @@
 #define DEBUGFS_DEFAULT_PATH		"/sys/kernel/debug"
 #endif
 
-char debugfs_mountpoint[PATH_MAX + 1] = DEBUGFS_DEFAULT_PATH;
+static char debugfs_mountpoint[PATH_MAX + 1] = DEBUGFS_DEFAULT_PATH;
 
 static const char * const debugfs_known_mountpoints[] = {
 	DEBUGFS_DEFAULT_PATH,
diff --git a/tools/lib/api/fs/debugfs.h b/tools/lib/api/fs/debugfs.h
index 19a618e9dbc1..0eb57ecbbb5f 100644
--- a/tools/lib/api/fs/debugfs.h
+++ b/tools/lib/api/fs/debugfs.h
@@ -15,6 +15,4 @@ bool debugfs_configured(void);
 const char *debugfs_find_mountpoint(void);
 char *debugfs_mount(const char *mountpoint);
 
-extern char debugfs_mountpoint[];
-
 #endif /* __API_DEBUGFS_H__ */
diff --git a/tools/lib/api/fs/tracefs.c b/tools/lib/api/fs/tracefs.c
index e4aa9688b71e..b5d34ee3cddc 100644
--- a/tools/lib/api/fs/tracefs.c
+++ b/tools/lib/api/fs/tracefs.c
@@ -16,7 +16,7 @@
 #define TRACEFS_DEFAULT_PATH		"/sys/kernel/tracing"
 #endif
 
-char tracefs_mountpoint[PATH_MAX + 1] = TRACEFS_DEFAULT_PATH;
+static char tracefs_mountpoint[PATH_MAX + 1] = TRACEFS_DEFAULT_PATH;
 
 static const char * const tracefs_known_mountpoints[] = {
 	TRACEFS_DEFAULT_PATH,
diff --git a/tools/lib/api/fs/tracefs.h b/tools/lib/api/fs/tracefs.h
index da780ac49acb..be7d4c543de5 100644
--- a/tools/lib/api/fs/tracefs.h
+++ b/tools/lib/api/fs/tracefs.h
@@ -16,6 +16,4 @@ const char *tracefs_find_mountpoint(void);
 int tracefs_valid_mountpoint(const char *debugfs);
 char *tracefs_mount(const char *mountpoint);
 
-extern char tracefs_mountpoint[];
-
 #endif /* __API_DEBUGFS_H__ */
-- 
2.4.3

[PATCH 09/11] perf tools: Propagate error info for the tracepoint parsing

[Jiri Olsa]

Pass 'struct parse_events_error *error' to the parse-event.c
tracepoint adding path. It will be filled with error data
in following patches.

Link: http://lkml.kernel.org/n/tip-las1hm5zf58b0twd27h9895b@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/parse-events.c | 27 ++++++++++++++++-----------
 tools/perf/util/parse-events.h |  3 ++-
 tools/perf/util/parse-events.y |  4 ++--
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index d826e6f515db..5d17409039c8 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -387,7 +387,8 @@ int parse_events_add_cache(struct list_head *list, int *idx,
 }
 
 static int add_tracepoint(struct list_head *list, int *idx,
-			  char *sys_name, char *evt_name)
+			  char *sys_name, char *evt_name,
+			  struct parse_events_error *error __maybe_unused)
 {
 	struct perf_evsel *evsel;
 
@@ -401,7 +402,8 @@ static int add_tracepoint(struct list_head *list, int *idx,
 }
 
 static int add_tracepoint_multi_event(struct list_head *list, int *idx,
-				      char *sys_name, char *evt_name)
+				      char *sys_name, char *evt_name,
+				      struct parse_events_error *error)
 {
 	char evt_path[MAXPATHLEN];
 	struct dirent *evt_ent;
@@ -425,7 +427,7 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 		if (!strglobmatch(evt_ent->d_name, evt_name))
 			continue;
 
-		ret = add_tracepoint(list, idx, sys_name, evt_ent->d_name);
+		ret = add_tracepoint(list, idx, sys_name, evt_ent->d_name, error);
 	}
 
 	closedir(evt_dir);
@@ -433,15 +435,17 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 }
 
 static int add_tracepoint_event(struct list_head *list, int *idx,
-				char *sys_name, char *evt_name)
+				char *sys_name, char *evt_name,
+				struct parse_events_error *error)
 {
 	return strpbrk(evt_name, "*?") ?
-	       add_tracepoint_multi_event(list, idx, sys_name, evt_name) :
-	       add_tracepoint(list, idx, sys_name, evt_name);
+	       add_tracepoint_multi_event(list, idx, sys_name, evt_name, error) :
+	       add_tracepoint(list, idx, sys_name, evt_name, error);
 }
 
 static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
-				    char *sys_name, char *evt_name)
+				    char *sys_name, char *evt_name,
+				    struct parse_events_error *error)
 {
 	struct dirent *events_ent;
 	DIR *events_dir;
@@ -465,7 +469,7 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
 			continue;
 
 		ret = add_tracepoint_event(list, idx, events_ent->d_name,
-					   evt_name);
+					   evt_name, error);
 	}
 
 	closedir(events_dir);
@@ -473,12 +477,13 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
 }
 
 int parse_events_add_tracepoint(struct list_head *list, int *idx,
-				char *sys, char *event)
+				char *sys, char *event,
+				struct parse_events_error *error)
 {
 	if (strpbrk(sys, "*?"))
-		return add_tracepoint_multi_sys(list, idx, sys, event);
+		return add_tracepoint_multi_sys(list, idx, sys, event, error);
 	else
-		return add_tracepoint_event(list, idx, sys, event);
+		return add_tracepoint_event(list, idx, sys, event, error);
 }
 
 static int
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index a09b0e210997..ffee7ece75a6 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -118,7 +118,8 @@ int parse_events__modifier_event(struct list_head *list, char *str, bool add);
 int parse_events__modifier_group(struct list_head *list, char *event_mod);
 int parse_events_name(struct list_head *list, char *name);
 int parse_events_add_tracepoint(struct list_head *list, int *idx,
-				char *sys, char *event);
+				char *sys, char *event,
+				struct parse_events_error *error);
 int parse_events_add_numeric(struct parse_events_evlist *data,
 			     struct list_head *list,
 			     u32 type, u64 config,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 591905a02b92..742914746794 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -376,7 +376,7 @@ PE_NAME '-' PE_NAME ':' PE_NAME
 	snprintf(&sys_name, 128, "%s-%s", $1, $3);
 
 	ALLOC_LIST(list);
-	ABORT_ON(parse_events_add_tracepoint(list, &data->idx, &sys_name, $5));
+	ABORT_ON(parse_events_add_tracepoint(list, &data->idx, &sys_name, $5, data->error));
 	$$ = list;
 }
 |
@@ -386,7 +386,7 @@ PE_NAME ':' PE_NAME
 	struct list_head *list;
 
 	ALLOC_LIST(list);
-	if (parse_events_add_tracepoint(list, &data->idx, $1, $3)) {
+	if (parse_events_add_tracepoint(list, &data->idx, $1, $3, data->error)) {
 		struct parse_events_error *error = data->error;
 
 		if (error) {
-- 
2.4.3

Re: [PATCH 09/11] perf tools: Propagate error info for the tracepoint parsing

[Matt Fleming]

On Wed, 26 Aug, at 03:46:51PM, Jiri Olsa wrote:
> Pass 'struct parse_events_error *error' to the parse-event.c
> tracepoint adding path. It will be filled with error data
> in following patches.
> 
> Link: http://lkml.kernel.org/n/tip-las1hm5zf58b0twd27h9895b@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/parse-events.c | 27 ++++++++++++++++-----------
>  tools/perf/util/parse-events.h |  3 ++-
>  tools/perf/util/parse-events.y |  4 ++--
>  3 files changed, 20 insertions(+), 14 deletions(-)

Is there a reason you decided to go ahead with passing 'data->error'
directly to the parsing functions instead of adding some global state
for recording errors, like a globally accessible 'parse_events_error'?

Because I notice that if someone wanted to improve the breakpoint
parsing code, they'd basically need to make the same changes you've
made in this patch.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 09/11] perf tools: Propagate error info for the tracepoint parsing

[Jiri Olsa]

On Fri, Aug 28, 2015 at 02:20:52PM +0100, Matt Fleming wrote:
> On Wed, 26 Aug, at 03:46:51PM, Jiri Olsa wrote:
> > Pass 'struct parse_events_error *error' to the parse-event.c
> > tracepoint adding path. It will be filled with error data
> > in following patches.
> > 
> > Link: http://lkml.kernel.org/n/tip-las1hm5zf58b0twd27h9895b@git.kernel.org
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/parse-events.c | 27 ++++++++++++++++-----------
> >  tools/perf/util/parse-events.h |  3 ++-
> >  tools/perf/util/parse-events.y |  4 ++--
> >  3 files changed, 20 insertions(+), 14 deletions(-)
> 
> Is there a reason you decided to go ahead with passing 'data->error'
> directly to the parsing functions instead of adding some global state
> for recording errors, like a globally accessible 'parse_events_error'?
> 
> Because I notice that if someone wanted to improve the breakpoint
> parsing code, they'd basically need to make the same changes you've
> made in this patch.

well, I dont see much benefit other than loosing one extra arg
and worrying about concurent access in the future.. but I might
have missed some

jirka

[PATCH 10/11] perf tools: Propagate error info from tp_format

[Jiri Olsa]

Propagate error info from tp_format via ERR_PTR to get
it all the way down to the parse-event.c tracepoint adding
routines.

Link: http://lkml.kernel.org/n/tip-bzdckgv1zfp2y8up9l7ojt7y@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/evsel.c        | 8 ++++++--
 tools/perf/util/parse-events.c | 3 ++-
 tools/perf/util/trace-event.c  | 7 +++++--
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index b096ef7a240c..bbc18847278d 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -13,6 +13,7 @@
 #include <traceevent/event-parse.h>
 #include <linux/hw_breakpoint.h>
 #include <linux/perf_event.h>
+#include <linux/err.h>
 #include <sys/resource.h>
 #include "asm/bug.h"
 #include "callchain.h"
@@ -227,6 +228,7 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
 struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int idx)
 {
 	struct perf_evsel *evsel = zalloc(perf_evsel__object.size);
+	int err = -ENOMEM;
 
 	if (evsel != NULL) {
 		struct perf_event_attr attr = {
@@ -239,8 +241,10 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int
 			goto out_free;
 
 		evsel->tp_format = trace_event__tp_format(sys, name);
-		if (evsel->tp_format == NULL)
+		if (IS_ERR(evsel->tp_format)) {
+			err = PTR_ERR(evsel->tp_format);
 			goto out_free;
+		}
 
 		event_attr_init(&attr);
 		attr.config = evsel->tp_format->id;
@@ -253,7 +257,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int
 out_free:
 	zfree(&evsel->name);
 	free(evsel);
-	return NULL;
+	return ERR_PTR(err);
 }
 
 const char *perf_evsel__hw_names[PERF_COUNT_HW_MAX] = {
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5d17409039c8..ff8a28ac31b7 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1,4 +1,5 @@
 #include <linux/hw_breakpoint.h>
+#include <linux/err.h>
 #include "util.h"
 #include "../perf.h"
 #include "evlist.h"
@@ -393,7 +394,7 @@ static int add_tracepoint(struct list_head *list, int *idx,
 	struct perf_evsel *evsel;
 
 	evsel = perf_evsel__newtp_idx(sys_name, evt_name, (*idx)++);
-	if (!evsel)
+	if (IS_ERR(evsel))
 		return -ENOMEM;
 
 	list_add_tail(&evsel->node, list);
diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index b90e646c7a91..867292f2c4ef 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -7,6 +7,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <traceevent/event-parse.h>
 #include "trace-event.h"
 #include "machine.h"
@@ -73,12 +74,14 @@ tp_format(const char *sys, const char *name)
 	char path[PATH_MAX];
 	size_t size;
 	char *data;
+	int err;
 
 	scnprintf(path, PATH_MAX, "%s/%s/%s/format",
 		  tracing_events_path, sys, name);
 
-	if (filename__read_str(path, &data, &size))
-		return NULL;
+	err = filename__read_str(path, &data, &size);
+	if (err)
+		return ERR_PTR(err);
 
 	pevent_parse_format(pevent, &event, data, size, sys);
 
-- 
2.4.3

[PATCH 11/11] perf tools: Enhance parsing events tracepoint error output

[Jiri Olsa]

Enhancing parsing events tracepoint error output. Adding
more verbose output when the tracepoint is not found or
the tracing event path cannot be access.

  $ sudo perf record -e sched:sched_krava ls
  event syntax error: 'sched:sched_krava'
                       \___ unknown tracepoint

  Error:  File /sys/kernel/debug/tracing//tracing/events/sched/sched_krava not found.
  Hint:   Perhaps this kernel misses some CONFIG_ setting to enable this feature?.

  Run 'perf list' for a list of valid events
  ...

  $ perf record -e sched:sched_krava ls
  event syntax error: 'sched:sched_krava'
                       \___ can't access trace events

  Error:  No permissions to read /sys/kernel/debug/tracing//tracing/events/sched/sched_krava
  Hint:   Try 'sudo mount -o remount,mode=755 /sys/kernel/debug'

  Run 'perf list' for a list of valid events
  ...

Link: http://lkml.kernel.org/n/tip-l0lu26995rir1h6v0e7kqyzz@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/parse-events.c | 30 +++++++++++++++++++++++++++---
 tools/perf/util/parse-events.y | 16 +++++++++-------
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index ff8a28ac31b7..1a6d8c80388d 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -387,6 +387,28 @@ int parse_events_add_cache(struct list_head *list, int *idx,
 	return add_event(list, idx, &attr, name, NULL);
 }
 
+static void tracepoint_error(struct parse_events_error *error, int err,
+			     char *sys, char *name)
+{
+	char help[BUFSIZ];
+	err = abs(err);
+
+	switch (err) {
+	case EACCES:
+		error->str = strdup("can't access trace events");
+		break;
+	case ENOENT:
+		error->str = strdup("unknown tracepoint");
+		break;
+	default:
+		error->str = strdup("failed to add tracepoint");
+		break;
+	}
+
+	tracing_path_strerror_open_tp(err, help, sizeof(help), sys, name);
+	error->help = strdup(help);
+}
+
 static int add_tracepoint(struct list_head *list, int *idx,
 			  char *sys_name, char *evt_name,
 			  struct parse_events_error *error __maybe_unused)
@@ -394,8 +416,10 @@ static int add_tracepoint(struct list_head *list, int *idx,
 	struct perf_evsel *evsel;
 
 	evsel = perf_evsel__newtp_idx(sys_name, evt_name, (*idx)++);
-	if (IS_ERR(evsel))
+	if (IS_ERR(evsel)) {
+		tracepoint_error(error, PTR_ERR(evsel), sys_name, evt_name);
 		return -ENOMEM;
+	}
 
 	list_add_tail(&evsel->node, list);
 
@@ -414,7 +438,7 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 	snprintf(evt_path, MAXPATHLEN, "%s/%s", tracing_events_path, sys_name);
 	evt_dir = opendir(evt_path);
 	if (!evt_dir) {
-		perror("Can't open event dir");
+		tracepoint_error(error, errno, sys_name, evt_name);
 		return -1;
 	}
 
@@ -454,7 +478,7 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
 
 	events_dir = opendir(tracing_events_path);
 	if (!events_dir) {
-		perror("Can't open event dir");
+		tracepoint_error(error, errno, sys_name, evt_name);
 		return -1;
 	}
 
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 742914746794..2ccc6911dc5e 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -371,28 +371,30 @@ event_legacy_tracepoint:
 PE_NAME '-' PE_NAME ':' PE_NAME
 {
 	struct parse_events_evlist *data = _data;
+	struct parse_events_error *error = data->error;
 	struct list_head *list;
 	char sys_name[128];
 	snprintf(&sys_name, 128, "%s-%s", $1, $3);
 
 	ALLOC_LIST(list);
-	ABORT_ON(parse_events_add_tracepoint(list, &data->idx, &sys_name, $5, data->error));
+	if (parse_events_add_tracepoint(list, &data->idx, &sys_name, $5, error)) {
+		if (error)
+			error->idx = @1.first_column;
+		return -1;
+	}
 	$$ = list;
 }
 |
 PE_NAME ':' PE_NAME
 {
 	struct parse_events_evlist *data = _data;
+	struct parse_events_error *error = data->error;
 	struct list_head *list;
 
 	ALLOC_LIST(list);
-	if (parse_events_add_tracepoint(list, &data->idx, $1, $3, data->error)) {
-		struct parse_events_error *error = data->error;
-
-		if (error) {
+	if (parse_events_add_tracepoint(list, &data->idx, $1, $3, error)) {
+		if (error)
 			error->idx = @1.first_column;
-			error->str = strdup("unknown tracepoint");
-		}
 		return -1;
 	}
 	$$ = list;
-- 
2.4.3