perf tools: Enhance parsing events tracepoint error output
Jiri Olsa
reviewerv2 cover perf tools: Enhance parsing events tracepoint error output
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
...
v2 changes:
- debugfs/tracefs changes went already in through separate patchset
- more commentary on err.h interface
- fixed callers of err.h enhanced functions
- added extra tags/cscope fix
Also available in:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/tp
thanks,
jirka
---
Jiri Olsa (5):
tools: Add err.h with ERR_PTR PTR_ERR interface perf tools: Add tools/include into tags directories 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 | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ tools/perf/Makefile.perf | 2 +- tools/perf/builtin-trace.c | 19 +++++++++++-------- tools/perf/tests/evsel-tp-sched.c | 10 ++++++++-- tools/perf/tests/openat-syscall-all-cpus.c | 3 ++- tools/perf/tests/openat-syscall-tp-fields.c | 3 ++- tools/perf/tests/openat-syscall.c | 3 ++- tools/perf/util/evlist.c | 3 ++- tools/perf/util/evsel.c | 11 +++++++++-- tools/perf/util/evsel.h | 3 +++ tools/perf/util/parse-events.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------- tools/perf/util/parse-events.h | 3 ++- tools/perf/util/parse-events.y | 16 +++++++++------- tools/perf/util/trace-event.c | 13 +++++++++++-- 14 files changed, 161 insertions(+), 43 deletions(-) create mode 100644 tools/include/linux/err.h
Jiri Olsa
reviewerPATCH v2 1/5 tools: Add err.h with ERR_PTR PTR_ERR interface
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.
--- tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 tools/include/linux/err.h
tools/include/linux/err.h new file +49 -0
@@ -0,0 +1,49 @@ +#ifndef __TOOLS_LINUX_ERR_H +#define __TOOLS_LINUX_ERR_H + +#include <linux/compiler.h> +#include <linux/types.h> + +#include <asm/errno.h> + +/* +* Original kernel header comment: +* +* 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. +* +* Userspace note: +* The same principle works for userspace, because 'error' pointers +* fall down to the unused hole far from user space, as described +* in Documentation/x86/x86_64/mm.txt for x86_64 arch: +* +* 0000000000000000 - 00007fffffffffff (=47 bits) user space, different per mm hole caused by [48:63] sign extension +* ffffffffffe00000 - ffffffffffffffff (=2 MB) unused hole +* +* It should be the same case for other architectures, because +* this code is used in generic kernel code. +*/ +#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 */
Signature · 1 line
2.4.3
Re: [PATCH 1/5] tools: Add err.h with ERR_PTR PTR_ERR interface
2015-09-07 4:38 GMT-04:00 Jiri Olsa <[email protected]>:
Quoted 71 lines
Adding part of the kernel's <
linux/err.h> interface:
inline void *__must_checkERR_PTR(long error);
inline long__must_checkPTR_ERR(__force const void *ptr);
inline bool__must_checkIS_ERR(__force const void *ptr);it will be used to propagate error through pointers in following patches.
Signed-off-by Jiri Olsa <[email protected]>--- tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 tools/include/linux/err.htools/include/linux/err.h new file +49
-1@@ -0,0 +1,49 @@ +#ifndef __TOOLS_LINUX_ERR_H +#define __TOOLS_LINUX_ERR_H + +#include <linux/compiler.h> +#include <linux/types.h> + +#include <asm/errno.h> + +/* +* Original kernel header comment: +* +* 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. +* +* Userspace note: +* The same principle works for userspace, because 'error' pointers +* fall down to the unused hole far from user space, as described +* in Documentation/x86/x86_64/mm.txt for x86_64 arch: +* +* 0000000000000000 - 00007fffffffffff (=47 bits) user space, different per mm hole caused by [48:63] sign extension +* ffffffffffe00000 - ffffffffffffffff (=2 MB) unused hole +* +* It should be the same case for other architectures, because +* this code is used in generic kernel code. +*/ +#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 */ --
Quoted 1 line
2.4.3
Perhaps a dumb question, but it seems the code is exactly the same as in linux/err.h besides the part of the comment you added. Why not using that file directly in the other patches then?
Re: [PATCH 1/5] tools: Add err.h with ERR_PTR PTR_ERR interface
2015-09-08 16:22 GMT-04:00 Raphaël Beamonte <[email protected]>:
Quoted 3 lines
Perhaps a dumb question, but it seems the code is exactly the same as in
linux/err.hbesides the part of the comment you added. Why not using that file directly in the other patches then?
I meant include/linux/err.h, from git root
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 1/5] tools: Add err.h with ERR_PTR PTR_ERR interface
In reply to Raphaël BeamonteEm Tue, Sep 08, 2015 at 04:22:39PM -0400, Raphaël Beamonte escreveu:
Quoted 69 lines
2015-09-07 4:38 GMT-04:00 Jiri Olsa <[email protected]>:
Quoted 68 lines
Adding part of the kernel's <
linux/err.h> interface:
inline void *__must_checkERR_PTR(long error);
inline long__must_checkPTR_ERR(__force const void *ptr);
inline bool__must_checkIS_ERR(__force const void *ptr);it will be used to propagate error through pointers in following patches.
Signed-off-by Jiri Olsa <[email protected]>--- tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 tools/include/linux/err.htools/include/linux/err.h new file +47
-0@@ -0,0 +1,49 @@ +#ifndef __TOOLS_LINUX_ERR_H +#define __TOOLS_LINUX_ERR_H + +#include <linux/compiler.h> +#include <linux/types.h> + +#include <asm/errno.h> + +/* +* Original kernel header comment: +* +* 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. +* +* Userspace note: +* The same principle works for userspace, because 'error' pointers +* fall down to the unused hole far from user space, as described +* in Documentation/x86/x86_64/mm.txt for x86_64 arch: +* +* 0000000000000000 - 00007fffffffffff (=47 bits) user space, different per mm hole caused by [48:63] sign extension +* ffffffffffe00000 - ffffffffffffffff (=2 MB) unused hole +* +* It should be the same case for other architectures, because +* this code is used in generic kernel code. +*/ +#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); +}
Quoted 8 lines
Quoted 4 lines
tools/include/linux/err.h new file +2
-1+ +#endif /* _LINUX_ERR_H */ --2.4.3
Perhaps a dumb question, but it seems the code is exactly the same as in
linux/err.hbesides the part of the comment you added. Why not using that file directly in the other patches then?
We can't do that.
Read:
commit 3f735377bfd6567d80815a6242c147211963680a
Author: Arnaldo Carvalho de Melo <[email protected]> Date: Sun Jul 5 22:48:21 2015 -0300
tools: Copy lib/rbtree.c to tools/lib/
So that we can remove kernel specific stuff we've been stubbing out via a tools/include/linux/export.h that gets removed in this patch and to avoid breakages in the future like the one fixed recently where rcupdate.h started being used in rbtree.h.
--------------------------
There are more copies like that, but the explanation above should be enough, no?
- Arnaldo
Re: [PATCH 1/5] tools: Add err.h with ERR_PTR PTR_ERR interface
2015-09-08 17:06 GMT-04:00 Arnaldo Carvalho de Melo <[email protected]>:
Quoted 95 lines
Em Tue, Sep 08, 2015 at 04:22:39PM -0400, Raphaël Beamonte escreveu:
Quoted 69 lines
2015-09-07 4:38 GMT-04:00 Jiri Olsa <[email protected]>:
Quoted 68 lines
Adding part of the kernel's <
linux/err.h> interface:
inline void *__must_checkERR_PTR(long error);
inline long__must_checkPTR_ERR(__force const void *ptr);
inline bool__must_checkIS_ERR(__force const void *ptr);it will be used to propagate error through pointers in following patches.
Signed-off-by Jiri Olsa <[email protected]>--- tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 tools/include/linux/err.htools/include/linux/err.h new file +47
-0@@ -0,0 +1,49 @@ +#ifndef __TOOLS_LINUX_ERR_H +#define __TOOLS_LINUX_ERR_H + +#include <linux/compiler.h> +#include <linux/types.h> + +#include <asm/errno.h> + +/* +* Original kernel header comment: +* +* 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. +* +* Userspace note: +* The same principle works for userspace, because 'error' pointers +* fall down to the unused hole far from user space, as described +* in Documentation/x86/x86_64/mm.txt for x86_64 arch: +* +* 0000000000000000 - 00007fffffffffff (=47 bits) user space, different per mm hole caused by [48:63] sign extension +* ffffffffffe00000 - ffffffffffffffff (=2 MB) unused hole +* +* It should be the same case for other architectures, because +* this code is used in generic kernel code. +*/ +#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); +}Quoted 8 lines
Quoted 4 lines
tools/include/linux/err.h new file +2
-1+ +#endif /* _LINUX_ERR_H */ --2.4.3
Perhaps a dumb question, but it seems the code is exactly the same as in
linux/err.hbesides the part of the comment you added. Why not using that file directly in the other patches then?We can't do that.
Read:
commit
3f735377bfd6567d80815a6242c147211963680aAuthor: Arnaldo Carvalho de Melo <[email protected]> Date: Sun Jul 5 22:48:21 2015 -0300tools: Copy
lib/rbtree.ctotools/lib/So that we can remove kernel specific stuff we've been stubbing out via a
tools/include/linux/export.hthat gets removed in this patch and to avoid breakages in the future like the one fixed recently wherercupdate.hstarted being used inrbtree.h.
Quoted 4 lines
--------------------------
There are more copies like that, but the explanation above should be enough, no?
Yes, I understand now! Thanks!
Quoted 3 lines
- Arnaldo
Re: [PATCH 1/5] tools: Add err.h with ERR_PTR PTR_ERR interface
In reply to Jiri Olsa2015-09-07 4:38 GMT-04:00 Jiri Olsa <[email protected]>:
Quoted 8 lines
Adding part of the kernel's <
linux/err.h> interface:
inline void *__must_checkERR_PTR(long error);
inline long__must_checkPTR_ERR(__force const void *ptr);
inline bool__must_checkIS_ERR(__force const void *ptr);it will be used to propagate error through pointers in following patches.
Quoted 6 lines
Signed-off-by Jiri Olsa <[email protected]>--- tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 tools/include/linux/err.h
Quoted 59 lines
tools/include/linux/err.h new file +49
-1@@ -0,0 +1,49 @@ +#ifndef __TOOLS_LINUX_ERR_H +#define __TOOLS_LINUX_ERR_H + +#include <linux/compiler.h> +#include <linux/types.h> + +#include <asm/errno.h> + +/* +* Original kernel header comment: +* +* 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. +* +* Userspace note: +* The same principle works for userspace, because 'error' pointers +* fall down to the unused hole far from user space, as described +* in Documentation/x86/x86_64/mm.txt for x86_64 arch: +* +* 0000000000000000 - 00007fffffffffff (=47 bits) user space, different per mm hole caused by [48:63] sign extension +* ffffffffffe00000 - ffffffffffffffff (=2 MB) unused hole +* +* It should be the same case for other architectures, because +* this code is used in generic kernel code. +*/ +#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
tip-bot for Jiri Olsa
merge bot[tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface
In reply to Jiri Olsatools: Add err.h with ERR_PTR PTR_ERR interface
Commit-ID: 01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7 Gitweb: http://git.kernel.org/tip/01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7 Author: Jiri Olsa <[email protected]> AuthorDate: Mon, 7 Sep 2015 10:38:03 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 15 Sep 2015 09:48:32 -0300
tools: Add err.h with ERR_PTR PTR_ERR interface
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.
--- tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
tools/include/linux/err.h new file +49 -0
@@ -0,0 +1,49 @@ +#ifndef __TOOLS_LINUX_ERR_H +#define __TOOLS_LINUX_ERR_H + +#include <linux/compiler.h> +#include <linux/types.h> + +#include <asm/errno.h> + +/* +* Original kernel header comment: +* +* 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. +* +* Userspace note: +* The same principle works for userspace, because 'error' pointers +* fall down to the unused hole far from user space, as described +* in Documentation/x86/x86_64/mm.txt for x86_64 arch: +* +* 0000000000000000 - 00007fffffffffff (=47 bits) user space, different per mm hole caused by [48:63] sign extension +* ffffffffffe00000 - ffffffffffffffff (=2 MB) unused hole +* +* It should be the same case for other architectures, because +* this code is used in generic kernel code. +*/ +#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 */
Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface
Quoted 18 lines · tip-bot for Jiri Olsa
Commit-ID: 01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7 Gitweb: http://git.kernel.org/tip/01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7 Author: Jiri Olsa <[email protected]> AuthorDate: Mon, 7 Sep 2015 10:38:03 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 15 Sep 2015 09:48:32 -0300tools: Add
err.hwithERR_PTRPTR_ERRinterfaceAdding 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.
Quoted 8 lines
Signed-off-by Jiri Olsa <[email protected]>Reviewed-by Raphael Beamonte <[email protected]>Cc David Ahern <[email protected]>Cc Matt Fleming <[email protected]>Cc Namhyung Kim <[email protected]>Cc Peter Zijlstra <[email protected]>Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
Hi.
This patch appears to have introduced a build error on CentOS 6.7 with GCC 4.4.
This build error occurs on next-20150921.
CC util/evlist.o
cc1: warnings being treated as errors
In file included from util/evlist.c:28:tools/include/linux/err.h: In function ‘ERR_PTR’:tools/include/linux/err.h:34: error: declaration of ‘error’ shadows a global declarationutil/util.h:135: error: shadowed declaration is here
$ gcc --version gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16) Copyright (C) 2010 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Cheers,
Vinson
Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface
Quoted 44 lines · Vinson Lee
Quoted 18 lines · tip-bot for Jiri Olsa
Commit-ID: 01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7 Gitweb: http://git.kernel.org/tip/01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7 Author: Jiri Olsa <[email protected]> AuthorDate: Mon, 7 Sep 2015 10:38:03 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 15 Sep 2015 09:48:32 -0300tools: Add
err.hwithERR_PTRPTR_ERRinterfaceAdding 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.
Quoted 8 lines
Signed-off-by Jiri Olsa <[email protected]>Reviewed-by Raphael Beamonte <[email protected]>Cc David Ahern <[email protected]>Cc Matt Fleming <[email protected]>Cc Namhyung Kim <[email protected]>Cc Peter Zijlstra <[email protected]>Signed-off-by Arnaldo Carvalho de Melo <[email protected]>Hi.
This patch appears to have introduced a build error on CentOS 6.7 with GCC 4.4.
This build error occurs on next-20150921.
CC
util/evlist.o
cc1: warnings being treated as errors
In file included fromutil/evlist.c:28:tools/include/linux/err.h: In function ‘ERR_PTR’:tools/include/linux/err.h:34: error: declaration of ‘error’ shadows a global declarationutil/util.h:135: error: shadowed declaration is here
Quoted 8 lines
$ gcc --version gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16) Copyright (C) 2010 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.Cheers,
Vinson
Hi.
This build error still occurs with next-20150929.
Cheers,
Vinson
Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface
Quoted 51 lines · Vinson Lee
Quoted 50 lines · Vinson Lee
Quoted 18 lines · tip-bot for Jiri Olsa
Commit-ID: 01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7 Gitweb: http://git.kernel.org/tip/01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7 Author: Jiri Olsa <[email protected]> AuthorDate: Mon, 7 Sep 2015 10:38:03 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 15 Sep 2015 09:48:32 -0300tools: Add
err.hwithERR_PTRPTR_ERRinterfaceAdding 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.
Quoted 8 lines
Signed-off-by Jiri Olsa <[email protected]>Reviewed-by Raphael Beamonte <[email protected]>Cc David Ahern <[email protected]>Cc Matt Fleming <[email protected]>Cc Namhyung Kim <[email protected]>Cc Peter Zijlstra <[email protected]>Signed-off-by Arnaldo Carvalho de Melo <[email protected]>Hi.
This patch appears to have introduced a build error on CentOS 6.7 with GCC 4.4.
This build error occurs on next-20150921.
CC
util/evlist.o
cc1: warnings being treated as errors
In file included fromutil/evlist.c:28:tools/include/linux/err.h: In function ‘ERR_PTR’:tools/include/linux/err.h:34: error: declaration of ‘error’ shadows a global declarationutil/util.h:135: error: shadowed declaration is here$ gcc --version gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16) Copyright (C) 2010 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Quoted 6 lines
Quoted 2 lines
Cheers,
VinsonHi.
This build error still occurs with next-20150929.
attached patch should fix it
FYI there's another instance of this bug in parse-events.c in Arnaldo's perf/core due to recent fixes, I'll send out fix shortly
thanks,
jirka
---
The error variable breaks build on CentOS 6.7, due to collision with global error symbol:
CC util/evlist.o
cc1: warnings being treated as errors
In file included from util/evlist.c:28:
tools/include/linux/err.h: In function ‘ERR_PTR’:
tools/include/linux/err.h:34: error: declaration of ‘error’ shadows a global declaration util/util.h:135: error: shadowed declaration is here
Using 'err' name instead to fix it.
--- tools/include/linux/err.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
tools/include/linux/err.h +2 -2
@@ -31,9 +31,9 @@#define IS_ERR_VALUE (x) unlikely((x) >= (unsigned long)-MAX_ERRNO) -static inline void *__must_check ERR_PTR (long error) +static inline void *__must_check ERR_PTR (long err) { -return (void *) error; +return (void *) err; }static inline long __must_check PTR_ERR (__force const void *ptr)
Signature · 1 line
2.4.3
Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface
On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
SNIP
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
$ gcc --version gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16) Copyright (C) 2010 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.Cheers,
Vinson
Quoted 8 lines
Quoted 3 lines
Hi.
This build error still occurs with next-20150929.
attached patch should fix it
FYI there's another instance of this bug in
parse-events.cin Arnaldo's perf/core due to recent fixes, I'll send out fix shortly
and here it is..
He Kuang,
this might collide with your recent fixes..
jirka
---
The error variable breaks build on CentOS 6.7, due to collision with global error symbol:
CC util/parse-events.o
cc1: warnings being treated as errors util/parse-events.c:419: error: declaration of ‘error’ shadows a global declaration
util/util.h:135: error: shadowed declaration is here util/parse-events.c: In function ‘add_tracepoint_multi_event’:
...
Using different argument names instead to fix it.
--- tools/perf/util/parse-events.c | 38 +++++++++++++++++++------------------- 1 files changed, 19 insertions(+), 19 deletions(-)
tools/perf/util/parse-events.c +19 -19
@@ -387,7 +387,7 @@ 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, +static void tracepoint_error (struct parse_events_error *e,int err,char *sys,char *name) {char help[BUFSIZ ];
@@ -400,29 +400,29 @@ static void tracepoint_error(struct parse_events_error *error, int err,switch (err) {case EACCES : - error->str =strdup ("can't access trace events" ); + e->str =strdup ("can't access trace events" );break ;case ENOENT : - error->str =strdup ("unknown tracepoint" ); + e->str =strdup ("unknown tracepoint" );break ;default : - error->str =strdup ("failed to add tracepoint" ); + e->str =strdup ("failed to add tracepoint" );break ; }tracing_path__strerror_open_tp (err, help,sizeof (help), sys, name); - error->help =strdup (help); + e->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 ) +struct parse_events_error *err__maybe_unused ) {struct perf_evsel *evsel; evsel =perf_evsel__newtp_idx (sys_name, evt_name, (*idx)++);if (IS_ERR (evsel)) { -tracepoint_error (error,PTR_ERR (evsel), sys_name, evt_name); +tracepoint_error (err,PTR_ERR (evsel), sys_name, evt_name);return PTR_ERR (evsel); }
@@ -432,7 +432,7 @@ 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, -struct parse_events_error *error) +struct parse_events_error *err) {char evt_path[MAXPATHLEN ];struct dirent *evt_ent;
@@ -442,7 +442,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) { -tracepoint_error (error, errno, sys_name, evt_name); +tracepoint_error (err, errno, sys_name, evt_name);return -1 ; }
@@ -456,7 +456,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, error); + ret =add_tracepoint (list, idx, sys_name, evt_ent->d_name, err); }closedir (evt_dir);
@@ -465,16 +465,16 @@ 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, -struct parse_events_error *error) +struct parse_events_error *err) {return strpbrk (evt_name,"*?" ) ? -add_tracepoint_multi_event (list, idx, sys_name, evt_name, error) : -add_tracepoint (list, idx, sys_name, evt_name, error); +add_tracepoint_multi_event (list, idx, sys_name, evt_name, err) : +add_tracepoint (list, idx, sys_name, evt_name, err); }static int add_tracepoint_multi_sys (struct list_head *list,int *idx,char *sys_name,char *evt_name, -struct parse_events_error *error) +struct parse_events_error *err) {struct dirent *events_ent;DIR *events_dir;
@@ -482,7 +482,7 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx, events_dir =opendir (tracing_events_path);if (!events_dir) { -tracepoint_error (error, errno, sys_name, evt_name); +tracepoint_error (err, errno, sys_name, evt_name);return -1 ; }
@@ -498,7 +498,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, error); + evt_name, err); }closedir (events_dir);
@@ -507,12 +507,12 @@ 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, -struct parse_events_error *error) +struct parse_events_error *err) {if (strpbrk (sys,"*?" )) -return add_tracepoint_multi_sys (list, idx, sys, event, error); +return add_tracepoint_multi_sys (list, idx, sys, event, err);else -return add_tracepoint_event (list, idx, sys, event, error); +return add_tracepoint_event (list, idx, sys, event, err); } static int
Signature · 1 line
1.7.1
Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface
hi, jirka
Quoted 19 lines · Jiri Olsa
On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
SNIP
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
$ gcc --version gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16) Copyright (C) 2010 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.Quoted 6 lines
Quoted 2 lines
Cheers,
VinsonHi.
This build error still occurs with next-20150929.
attached patch should fix it
Quoted 8 lines
Quoted 2 lines
FYI there's another instance of this bug in
parse-events.cin Arnaldo's perf/core due to recent fixes, I'll send out fix shortlyand here it is..
He Kuang,
this might collide with your recent fixes..
Do I need to send new ones based on your patch below now?
Quoted 156 lines
jirka
---
The error variable breaks build on CentOS 6.7, due to collision with global error symbol:CC
util/parse-events.o
cc1: warnings being treated as errorsutil/parse-events.c:419: error: declaration of ‘error’ shadows a global declaration
util/util.h:135: error: shadowed declaration is hereutil/parse-events.c: In function ‘add_tracepoint_multi_event’:...Using different argument names instead to fix it.
Reported-by Vinson Lee <[email protected]>Signed-off-by Jiri Olsa <[email protected]>--- tools/perf/util/parse-events.c | 38 +++++++++++++++++++------------------- 1 files changed, 19 insertions(+), 19 deletions(-)tools/perf/util/parse-events.c +19
-19@@ -387,7 +387,7 @@ 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, +static void tracepoint_error (struct parse_events_error *e,int err,char *sys,char *name) {char help[BUFSIZ ];@@ -400,29 +400,29 @@ static void tracepoint_error(struct parse_events_error *error, int err,switch (err) {case EACCES : - error->str =strdup ("can't access trace events" ); + e->str =strdup ("can't access trace events" );break ;case ENOENT : - error->str =strdup ("unknown tracepoint" ); + e->str =strdup ("unknown tracepoint" );break ;default : - error->str =strdup ("failed to add tracepoint" ); + e->str =strdup ("failed to add tracepoint" );break ; }tracing_path__strerror_open_tp (err, help,sizeof (help), sys, name); - error->help =strdup (help); + e->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 ) +struct parse_events_error *err__maybe_unused ) {struct perf_evsel *evsel; evsel =perf_evsel__newtp_idx (sys_name, evt_name, (*idx)++);if (IS_ERR (evsel)) { -tracepoint_error (error,PTR_ERR (evsel), sys_name, evt_name); +tracepoint_error (err,PTR_ERR (evsel), sys_name, evt_name);return PTR_ERR (evsel); }@@ -432,7 +432,7 @@ 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, -struct parse_events_error *error) +struct parse_events_error *err) {char evt_path[MAXPATHLEN ];struct dirent *evt_ent;@@ -442,7 +442,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) { -tracepoint_error (error, errno, sys_name, evt_name); +tracepoint_error (err, errno, sys_name, evt_name);return -1 ; }@@ -456,7 +456,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, error); + ret =add_tracepoint (list, idx, sys_name, evt_ent->d_name, err); }closedir (evt_dir);@@ -465,16 +465,16 @@ 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, -struct parse_events_error *error) +struct parse_events_error *err) {return strpbrk (evt_name,"*?" ) ? -add_tracepoint_multi_event (list, idx, sys_name, evt_name, error) : -add_tracepoint (list, idx, sys_name, evt_name, error); +add_tracepoint_multi_event (list, idx, sys_name, evt_name, err) : +add_tracepoint (list, idx, sys_name, evt_name, err); }static int add_tracepoint_multi_sys (struct list_head *list,int *idx,char *sys_name,char *evt_name, -struct parse_events_error *error) +struct parse_events_error *err) {struct dirent *events_ent;DIR *events_dir;@@ -482,7 +482,7 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx, events_dir =opendir (tracing_events_path);if (!events_dir) { -tracepoint_error (error, errno, sys_name, evt_name); +tracepoint_error (err, errno, sys_name, evt_name);return -1 ; }@@ -498,7 +498,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, error); + evt_name, err); }closedir (events_dir);@@ -507,12 +507,12 @@ 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, -struct parse_events_error *error) +struct parse_events_error *err) {if (strpbrk (sys,"*?" )) -return add_tracepoint_multi_sys (list, idx, sys, event, error); +return add_tracepoint_multi_sys (list, idx, sys, event, err);else -return add_tracepoint_event (list, idx, sys, event, error); +return add_tracepoint_event (list, idx, sys, event, err); } static int
Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface
Quoted 25 lines · He Kuang
hi, jirka
Quoted 22 lines · Jiri Olsa
On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
SNIP
Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
$ gcc --version gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16) Copyright (C) 2010 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.Quoted 6 lines
Quoted 2 lines
Cheers,
VinsonHi.
This build error still occurs with next-20150929.
attached patch should fix it
FYI there's another instance of this bug in
parse-events.cin Arnaldo's perf/core due to recent fixes, I'll send out fix shortly
Quoted 7 lines
Quoted 5 lines
and here it is..
He Kuang,
this might collide with your recent fixes..Do I need to send new ones based on your patch below now?
that'd be great.. also please make sure you don't reintroduce it
thanks,
jirka
Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface
Quoted 28 lines · Jiri Olsa
Quoted 27 lines · He Kuang
hi, jirka
Quoted 24 lines · Jiri Olsa
On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
SNIP
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
$ gcc --version gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16) Copyright (C) 2010 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.Cheers,
VinsonQuoted 8 lines
Quoted 3 lines
Hi.
This build error still occurs with next-20150929.
attached patch should fix it
FYI there's another instance of this bug in
parse-events.cin Arnaldo's perf/core due to recent fixes, I'll send out fix shortlyand here it is..
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
He Kuang,
this might collide with your recent fixes..Do I need to send new ones based on your patch below now?
that'd be great.. also please make sure you don't reintroduce it
I saw Ingo just accepted ACME's pull request which contains my patches, then resend may reintroduce or conflict things, I will wait for ACME's suggestion for my lack of experience in this.
Thank you.
Quoted 3 lines
thanks,
jirka
Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface
Quoted 35 lines · He Kuang
Quoted 32 lines · Jiri Olsa
Quoted 31 lines · He Kuang
hi, jirka
Quoted 27 lines · Jiri Olsa
On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
SNIP
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
$ gcc --version gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16) Copyright (C) 2010 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.Cheers,
VinsonQuoted 8 lines
Quoted 3 lines
Hi.
This build error still occurs with next-20150929.
attached patch should fix it
FYI there's another instance of this bug in
parse-events.cin Arnaldo's perf/core due to recent fixes, I'll send out fix shortlyand here it is..
He Kuang,
this might collide with your recent fixes..
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Do I need to send new ones based on your patch below now?
that'd be great.. also please make sure you don't reintroduce it
I saw Ingo just accepted ACME's pull request which contains my patches, then resend may reintroduce or conflict things, I will wait for ACME's suggestion for my lack of experience in this.
hum, I can see your changes in pull request to Ingo, but not in acme's perf/core.. strange.. let's wait for Arnaldo to sort it out ;-)
thanks,
jirka
Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface
Em Tue, Sep 29, 2015 at 12:41:47PM +0200, Jiri Olsa escreveu:
Quoted 41 lines · He Kuang
Quoted 40 lines · Jiri Olsa
Quoted 36 lines · He Kuang
Quoted 25 lines
hi, jirka
Quoted 22 lines · Jiri Olsa
On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
SNIP
Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
$ gcc --version gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-16) Copyright (C) 2010 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.Quoted 6 lines
Quoted 2 lines
Cheers,
VinsonHi.
This build error still occurs with next-20150929.
attached patch should fix it
FYI there's another instance of this bug in
parse-events.cin Arnaldo's perf/core due to recent fixes, I'll send out fix shortlyQuoted 7 lines
Quoted 5 lines
and here it is..
He Kuang,
this might collide with your recent fixes..Do I need to send new ones based on your patch below now?
that'd be great.. also please make sure you don't reintroduce it
Quoted 7 lines
Quoted 3 lines
I saw Ingo just accepted ACME's pull request which contains my patches, then resend may reintroduce or conflict things, I will wait for ACME's suggestion for my lack of experience in this.
hum, I can see your changes in pull request to Ingo, but not in acme's perf/core.. strange.. let's wait for Arnaldo to sort it out ;-)
Sorry, probably I didn't push perf/core yesterday, just the perf-core-for-mingo tag.
Anyway, please pull again, should have everything in there now plus some patches I processed today.
- Arnaldo
PATCH 1/1 perf tool: Fix shadowed declaration in parse-events.c
On Tue, Sep 29, 2015 at 11:52:59AM -0300, Arnaldo Carvalho de Melo wrote:
SNIP
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Do I need to send new ones based on your patch below now?
that'd be great.. also please make sure you don't reintroduce it
Quoted 7 lines
Quoted 3 lines
I saw Ingo just accepted ACME's pull request which contains my patches, then resend may reintroduce or conflict things, I will wait for ACME's suggestion for my lack of experience in this.
hum, I can see your changes in pull request to Ingo, but not in acme's perf/core.. strange.. let's wait for Arnaldo to sort it out ;-)
Quoted 5 lines
Sorry, probably I didn't push perf/core yesterday, just the perf-core-for-mingo tag.
Anyway, please pull again, should have everything in there now plus some patches I processed today.
np, resending the patch rebase on your latest perf/core
thanks,
jirka
---
The error variable breaks build on CentOS 6.7, due to collision with global error symbol:
CC util/parse-events.o
cc1: warnings being treated as errors util/parse-events.c:419: error: declaration of ‘error’ shadows a global declaration
util/util.h:135: error: shadowed declaration is here util/parse-events.c: In function ‘add_tracepoint_multi_event’:
...
Using different argument names instead to fix it.
--- tools/perf/util/parse-events.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
tools/perf/util/parse-events.c +16 -16
@@ -389,7 +389,7 @@ 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, +static void tracepoint_error (struct parse_events_error *e,int err,char *sys,char *name) {char help[BUFSIZ ];
@@ -402,30 +402,30 @@ static void tracepoint_error(struct parse_events_error *error, int err,switch (err) {case EACCES : - error->str =strdup ("can't access trace events" ); + e->str =strdup ("can't access trace events" );break ;case ENOENT : - error->str =strdup ("unknown tracepoint" ); + e->str =strdup ("unknown tracepoint" );break ;default : - error->str =strdup ("failed to add tracepoint" ); + e->str =strdup ("failed to add tracepoint" );break ; }tracing_path__strerror_open_tp (err, help,sizeof (help), sys, name); - error->help =strdup (help); + e->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 , +struct parse_events_error *err,struct list_head *head_config) {struct perf_evsel *evsel; evsel =perf_evsel__newtp_idx (sys_name, evt_name, (*idx)++);if (IS_ERR (evsel)) { -tracepoint_error (error,PTR_ERR (evsel), sys_name, evt_name); +tracepoint_error (err,PTR_ERR (evsel), sys_name, evt_name);return PTR_ERR (evsel); }
@@ -443,7 +443,7 @@ 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, -struct parse_events_error *error, +struct parse_events_error *err,struct list_head *head_config) {char evt_path[MAXPATHLEN ];
@@ -454,7 +454,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) { -tracepoint_error (error, errno, sys_name, evt_name); +tracepoint_error (err, errno, sys_name, evt_name);return -1 ; }
@@ -469,7 +469,7 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,continue ; ret =add_tracepoint (list, idx, sys_name, evt_ent->d_name, - error, head_config); + err, head_config); }closedir (evt_dir);
@@ -478,19 +478,19 @@ 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, -struct parse_events_error *error, +struct parse_events_error *err,struct list_head *head_config) {return strpbrk (evt_name,"*?" ) ?add_tracepoint_multi_event (list, idx, sys_name, evt_name, - error, head_config) : + err, head_config) :add_tracepoint (list, idx, sys_name, evt_name, - error, head_config); + err, head_config); }static int add_tracepoint_multi_sys (struct list_head *list,int *idx,char *sys_name,char *evt_name, -struct parse_events_error *error, +struct parse_events_error *err,struct list_head *head_config) {struct dirent *events_ent;
@@ -499,7 +499,7 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx, events_dir =opendir (tracing_events_path);if (!events_dir) { -tracepoint_error (error, errno, sys_name, evt_name); +tracepoint_error (err, errno, sys_name, evt_name);return -1 ; }
@@ -515,7 +515,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, error, head_config); + evt_name, err, head_config); }closedir (events_dir);
Signature · 1 line
2.4.3
tip-bot for Jiri Olsa
merge bot[tip:perf/core] perf tools: Fix shadowed declaration in parse-events.c
perf tools: Fix shadowed declaration in parse-events.c
Commit-ID: 272ed29a91aea4397c05157dea7210dd7e81557a Gitweb: http://git.kernel.org/tip/272ed29a91aea4397c05157dea7210dd7e81557a Author: Jiri Olsa <[email protected]> AuthorDate: Tue, 29 Sep 2015 17:05:31 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Wed, 30 Sep 2015 18:34:23 -0300
perf tools: Fix shadowed declaration in parse-events.c
The error variable breaks build on CentOS 6.7, due to a collision with a global error symbol:
CC util/parse-events.o
cc1: warnings being treated as errors util/parse-events.c:419: error: declaration of ‘error’ shadows a global declaration
util/util.h:135: error: shadowed declaration is here util/parse-events.c: In function ‘add_tracepoint_multi_event’:
...
Using different argument names instead to fix it.
--- tools/perf/util/parse-events.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)
tools/perf/util/parse-events.c +20 -20
@@ -389,7 +389,7 @@ 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, +static void tracepoint_error (struct parse_events_error *e,int err,char *sys,char *name) {char help[BUFSIZ ];
@@ -402,30 +402,30 @@ static void tracepoint_error(struct parse_events_error *error, int err,switch (err) {case EACCES : - error->str =strdup ("can't access trace events" ); + e->str =strdup ("can't access trace events" );break ;case ENOENT : - error->str =strdup ("unknown tracepoint" ); + e->str =strdup ("unknown tracepoint" );break ;default : - error->str =strdup ("failed to add tracepoint" ); + e->str =strdup ("failed to add tracepoint" );break ; }tracing_path__strerror_open_tp (err, help,sizeof (help), sys, name); - error->help =strdup (help); + e->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 , +struct parse_events_error *err,struct list_head *head_config) {struct perf_evsel *evsel; evsel =perf_evsel__newtp_idx (sys_name, evt_name, (*idx)++);if (IS_ERR (evsel)) { -tracepoint_error (error,PTR_ERR (evsel), sys_name, evt_name); +tracepoint_error (err,PTR_ERR (evsel), sys_name, evt_name);return PTR_ERR (evsel); }
@@ -443,7 +443,7 @@ 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, -struct parse_events_error *error, +struct parse_events_error *err,struct list_head *head_config) {char evt_path[MAXPATHLEN ];
@@ -454,7 +454,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) { -tracepoint_error (error, errno, sys_name, evt_name); +tracepoint_error (err, errno, sys_name, evt_name);return -1 ; }
@@ -469,7 +469,7 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,continue ; ret =add_tracepoint (list, idx, sys_name, evt_ent->d_name, - error, head_config); + err, head_config); }closedir (evt_dir);
@@ -478,19 +478,19 @@ 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, -struct parse_events_error *error, +struct parse_events_error *err,struct list_head *head_config) {return strpbrk (evt_name,"*?" ) ?add_tracepoint_multi_event (list, idx, sys_name, evt_name, - error, head_config) : + err, head_config) :add_tracepoint (list, idx, sys_name, evt_name, - error, head_config); + err, head_config); }static int add_tracepoint_multi_sys (struct list_head *list,int *idx,char *sys_name,char *evt_name, -struct parse_events_error *error, +struct parse_events_error *err,struct list_head *head_config) {struct dirent *events_ent;
@@ -499,7 +499,7 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx, events_dir =opendir (tracing_events_path);if (!events_dir) { -tracepoint_error (error, errno, sys_name, evt_name); +tracepoint_error (err, errno, sys_name, evt_name);return -1 ; }
@@ -515,7 +515,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, error, head_config); + evt_name, err, head_config); }closedir (events_dir);
@@ -767,23 +767,23 @@ do { \int parse_events_add_tracepoint (struct list_head *list,int *idx,char *sys,char *event, -struct parse_events_error *error, +struct parse_events_error *err,struct list_head *head_config) {if (head_config) {struct perf_event_attr attr; -if (config_attr (&attr, head_config, error, +if (config_attr (&attr, head_config, err, config_term_tracepoint))return -EINVAL ; }if (strpbrk (sys,"*?" ))return add_tracepoint_multi_sys (list, idx, sys, event, - error, head_config); + err, head_config);else return add_tracepoint_event (list, idx, sys, event, - error, head_config); + err, head_config); } int parse_events_add_numeric(struct parse_events_evlist *data,
Jiri Olsa
reviewerPATCH v2 2/5 perf tools: Add tools/include into tags directories
In reply to Jiri OlsaAdding tools/include into tags directories, to have include definitions reachable via tags/cscope.
--- tools/perf/Makefile.perf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/Makefile.perf +1 -1
@@ -459,7 +459,7 @@ INSTALL_DOC_TARGETS += quick-install-doc quick-install-man quick-install-html $(DOC_TARGETS): $(QUIET_SUBDIR0)Documentation $(QUIET_SUBDIR1) $(@:doc=all) -TAG_FOLDERS= . ../lib/traceevent ../lib/api ../lib/symbol +TAG_FOLDERS= . ../lib/traceevent ../lib/api ../lib/symbol ../include TAG_FILES= ../../include/uapi/linux/perf_event.h TAGS:
Signature · 1 line
2.4.3
tip-bot for Jiri Olsa
merge bot[tip:perf/core] perf tools: Add tools/ include into tags directories
perf tools: Add tools/include into tags directories
Commit-ID: b60114067c656be572f0ca4030c3a2c560e7255c Gitweb: http://git.kernel.org/tip/b60114067c656be572f0ca4030c3a2c560e7255c Author: Jiri Olsa <[email protected]> AuthorDate: Mon, 7 Sep 2015 10:38:04 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Mon, 14 Sep 2015 12:50:23 -0300
perf tools: Add tools/include into tags directories
Adding tools/include into tags directories, to have include definitions reachable via tags/cscope.
--- tools/perf/Makefile.perf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/Makefile.perf +1 -1
@@ -459,7 +459,7 @@ INSTALL_DOC_TARGETS += quick-install-doc quick-install-man quick-install-html $(DOC_TARGETS): $(QUIET_SUBDIR0)Documentation $(QUIET_SUBDIR1) $(@:doc=all) -TAG_FOLDERS= . ../lib/traceevent ../lib/api ../lib/symbol +TAG_FOLDERS= . ../lib/traceevent ../lib/api ../lib/symbol ../include TAG_FILES= ../../include/uapi/linux/perf_event.h TAGS:
Jiri Olsa
reviewerPATCH v2 3/5 perf tools: Propagate error info for the tracepoint parsing
In reply to Jiri OlsaPass 'struct parse_events_error *error' to the parse-event.c tracepoint adding path. It will be filled with error data in following patches.
--- 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(-)
tools/perf/util/parse-events.c +16 -11
@@ -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
tools/perf/util/parse-events.h +2 -1
@@ -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,
tools/perf/util/parse-events.y +2 -2
@@ -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) {
Signature · 1 line
2.4.3
Re: [PATCH 3/5] perf tools: Propagate error info for the tracepoint parsing
2015-09-07 4:38 GMT-04:00 Jiri Olsa <[email protected]>:
Quoted 130 lines
Pass 'struct
parse_events_error*error' to theparse-event.ctracepoint adding path. It will be filled with error data in following patches.Signed-off-by Jiri Olsa <[email protected]>--- 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(-)tools/perf/util/parse-events.c +16
-11@@ -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 inttools/perf/util/parse-events.h +2
-1@@ -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,tools/perf/util/parse-events.y +2
-3@@ -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) { --
Quoted 2 lines
2.4.3
Works for me.
I also made sure I could compile and run perf with that patch applied on top of the current linux master. Should I also propose my
Tested-by: tag? I didn't do thorough tests though.
Re: [PATCH 3/5] perf tools: Propagate error info for the tracepoint parsing
On Tue, Sep 08, 2015 at 05:42:25PM -0400, Raphaël Beamonte wrote:
SNIP
Quoted 14 lines
Quoted 10 lines
tools/perf/util/parse-events.y +1
-2@@ -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) { --Quoted 2 lines
2.4.3
Works for me.
Quoted 5 lines
Reviewed-by Raphaël Beamonte <[email protected]>I also made sure I could compile and run perf with that patch applied on top of the current linux master. Should I also propose my
Tested-by: tag? I didn't do thorough tests though.
I always base my changes over Arnaldo's perf/core, which gets eventually merged to Ingo's tip tree and then the Linus'es tree.. it should be enough to test it over my branch
thanks,
jirka
Re: [PATCH 3/5] perf tools: Propagate error info for the tracepoint parsing
In reply to Jiri OlsaQuoted 11 lines · Jiri Olsa
Pass 'struct
parse_events_error*error' to theparse-event.ctracepoint adding path. It will be filled with error data in following patches.Signed-off-by Jiri Olsa <[email protected]>--- 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(-)
Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
tip-bot for Jiri Olsa
merge bot[tip:perf/core] perf tools: Propagate error info for the tracepoint parsing
In reply to Jiri Olsaperf tools: Propagate error info for the tracepoint parsing
Commit-ID: e2f9f8ea6a54e252e3a94a5c2321f673b5b97360 Gitweb: http://git.kernel.org/tip/e2f9f8ea6a54e252e3a94a5c2321f673b5b97360 Author: Jiri Olsa <[email protected]> AuthorDate: Mon, 7 Sep 2015 10:38:05 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 15 Sep 2015 09:48:32 -0300
perf tools: Propagate error info for the tracepoint parsing
Pass 'struct parse_events_error *error' to the parse-event.c tracepoint adding path. It will be filled with error data in following patches.
--- 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(-)
tools/perf/util/parse-events.c +16 -11
@@ -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
tools/perf/util/parse-events.h +2 -1
@@ -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,
tools/perf/util/parse-events.y +2 -2
@@ -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) {
Jiri Olsa
reviewerPATCH v2 4/5 perf tools: Propagate error info from tp_format
In reply to Jiri OlsaPropagate error info from tp_format via ERR_PTR to get it all the way down to the parse-event.c tracepoint adding routines. Following functions now return pointer with encoded error:
- tp_format
- trace_event__tp_format
- perf_evsel__newtp_idx
- perf_evsel__newtp
This affects several other places in perf, that cannot use pointer check anymore, but must utilize the err.h interface, when getting error information from above functions list.
--- tools/perf/builtin-trace.c | 19 +++++++++++-------- tools/perf/tests/evsel-tp-sched.c | 10 ++++++++-- tools/perf/tests/openat-syscall-all-cpus.c | 3 ++- tools/perf/tests/openat-syscall-tp-fields.c | 3 ++- tools/perf/tests/openat-syscall.c | 3 ++- tools/perf/util/evlist.c | 3 ++- tools/perf/util/evsel.c | 11 +++++++++-- tools/perf/util/evsel.h | 3 +++ tools/perf/util/parse-events.c | 6 +++--- tools/perf/util/trace-event.c | 13 +++++++++++-- 10 files changed, 53 insertions(+), 21 deletions(-)
tools/perf/builtin-trace.c +11 -8
@@ -38,6 +38,7 @@#include <stdlib.h> #include <sys/mman.h> #include <linux/futex.h> +#include <linux/err.h> /* For older distros: */ #ifndef MAP_STACK
@@ -245,13 +246,14 @@ static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, voidstruct perf_evsel *evsel =perf_evsel__newtp ("raw_syscalls" , direction);/* older kernel (e.g., RHEL6) use syscalls:{enter,exit} */ -if (evsel ==NULL ) +if (IS_ERR (evsel)) evsel =perf_evsel__newtp ("syscalls" , direction); -if (evsel) { -if (perf_evsel__init_syscall_tp (evsel, handler)) - goto out_delete; - } +if (IS_ERR (evsel)) +return NULL ; + +if (perf_evsel__init_syscall_tp (evsel, handler)) + goto out_delete;return evsel;
@@ -1705,12 +1707,12 @@ static int trace__read_syscall_info(struct trace *trace, int id)snprintf (tp_name,sizeof (tp_name),"sys_enter_%s" , sc->name); sc->tp_format =trace_event__tp_format ("syscalls" , tp_name); -if (sc->tp_format ==NULL && sc->fmt && sc->fmt->alias) { +if (IS_ERR (sc->tp_format) && sc->fmt && sc->fmt->alias) {snprintf (tp_name,sizeof (tp_name),"sys_enter_%s" , sc->fmt->alias); sc->tp_format =trace_event__tp_format ("syscalls" , tp_name); } -if (sc->tp_format ==NULL ) +if (IS_ERR (sc->tp_format))return -1 ; sc->args = sc->tp_format->format.fields;
@@ -2390,7 +2392,8 @@ static size_t trace__fprintf_thread_summary(struct trace *trace, FILE *fp);static bool perf_evlist__add_vfs_getname (struct perf_evlist *evlist) {struct perf_evsel *evsel =perf_evsel__newtp ("probe" ,"vfs_getname" ); -if (evsel ==NULL ) + +if (IS_ERR (evsel))return false;if (perf_evsel__field (evsel,"pathname" ) ==NULL ) {
tools/perf/tests/evsel-tp-sched.c +8 -2
@@ -1,3 +1,4 @@ +#include <linux/err.h> #include <traceevent/event-parse.h> #include "evsel.h" #include "tests.h"
@@ -36,8 +37,8 @@ int test__perf_evsel__tp_sched_test(void)struct perf_evsel *evsel =perf_evsel__newtp ("sched" ,"sched_switch" );int ret =0 ; -if (evsel ==NULL ) { -pr_debug ("perf_evsel__new\n" ); +if (IS_ERR (evsel)) { +pr_debug ("perf_evsel__newtp failed with %ld\n" ,PTR_ERR (evsel));return -1 ; }
@@ -66,6 +67,11 @@ int test__perf_evsel__tp_sched_test(void) evsel =perf_evsel__newtp ("sched" ,"sched_wakeup" ); +if (IS_ERR (evsel)) { +pr_debug ("perf_evsel__newtp failed with %ld\n" ,PTR_ERR (evsel)); +return -1 ; + } +if (perf_evsel__test_field (evsel,"comm" ,16 , true)) ret =-1 ;
tools/perf/tests/openat-syscall-all-cpus.c +2 -1
@@ -1,4 +1,5 @@#include <api/fs/fs.h> +#include <linux/err.h> #include "evsel.h" #include "tests.h" #include "thread_map.h"
@@ -31,7 +32,7 @@ int test__openat_syscall_event_on_all_cpus(void)CPU_ZERO (&cpu_set); evsel =perf_evsel__newtp ("syscalls" ,"sys_enter_openat" ); -if (evsel ==NULL ) { +if (IS_ERR (evsel)) {tracing_path__strerror_open_tp (errno, errbuf,sizeof (errbuf),"syscalls" ,"sys_enter_openat" );pr_err ("%s\n" , errbuf); goto out_thread_map_delete;
tools/perf/tests/openat-syscall-tp-fields.c +2 -1
@@ -1,3 +1,4 @@ +#include <linux/err.h> #include "perf.h" #include "evlist.h" #include "evsel.h"
@@ -30,7 +31,7 @@ int test__syscall_openat_tp_fields(void) } evsel =perf_evsel__newtp ("syscalls" ,"sys_enter_openat" ); -if (evsel ==NULL ) { +if (IS_ERR (evsel)) {pr_debug ("%s: perf_evsel__newtp\n" , __func__); goto out_delete_evlist; }
tools/perf/tests/openat-syscall.c +2 -1
@@ -1,4 +1,5 @@#include <api/fs/tracing_path.h> +#include <linux/err.h> #include "thread_map.h" #include "evsel.h" #include "debug.h"
@@ -19,7 +20,7 @@ int test__openat_syscall_event(void) } evsel =perf_evsel__newtp ("syscalls" ,"sys_enter_openat" ); -if (evsel ==NULL ) { +if (IS_ERR (evsel)) {tracing_path__strerror_open_tp (errno, errbuf,sizeof (errbuf),"syscalls" ,"sys_enter_openat" );pr_err ("%s\n" , errbuf); goto out_thread_map_delete;
tools/perf/util/evlist.c +2 -1
@@ -25,6 +25,7 @@#include <linux/bitops.h> #include <linux/hash.h> #include <linux/log2.h> +#include <linux/err.h> static void perf_evlist__mmap_put (struct perf_evlist *evlist,int idx);static void __perf_evlist__munmap (struct perf_evlist *evlist,int idx);
@@ -265,7 +266,7 @@ int perf_evlist__add_newtp(struct perf_evlist *evlist, {struct perf_evsel *evsel =perf_evsel__newtp (sys, name); -if (evsel ==NULL ) +if (IS_ERR (evsel))return -1 ; evsel->handler = handler;
tools/perf/util/evsel.c +9 -2
@@ -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"
@@ -225,9 +226,13 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)return evsel; } +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ 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 = {
@@ -240,8 +245,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;
@@ -254,7 +261,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 ] = {
tools/perf/util/evsel.h +3 -0
@@ -161,6 +161,9 @@ static inline struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr)struct perf_evsel *perf_evsel__newtp_idx (const char *sys,const char *name,int idx); +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ static inline struct perf_evsel *perf_evsel__newtp (const char *sys,const char *name) {return perf_evsel__newtp_idx (sys, name,0 );
tools/perf/util/parse-events.c +3 -3
@@ -1,4 +1,5 @@#include <linux/hw_breakpoint.h> +#include <linux/err.h> #include "util.h" #include "../perf.h" #include "evlist.h"
@@ -393,11 +394,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 (!evsel) -return -ENOMEM ; +if (IS_ERR (evsel)) +return PTR_ERR (evsel);list_add_tail (&evsel->node, list); -return 0 ; }
tools/perf/util/trace-event.c +11 -2
@@ -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 <api/fs/tracing_path.h> #include "trace-event.h"
@@ -66,6 +67,9 @@ void trace_event__cleanup(struct trace_event *t)pevent_free (t->pevent); } +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ static struct event_format *tp_format (const char *sys,const char *name) {
@@ -74,12 +78,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);
@@ -87,6 +93,9 @@ tp_format(const char *sys, const char *name)return event; } +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ struct event_format *trace_event__tp_format (const char *sys,const char *name) {
Signature · 1 line
2.4.3
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 4/5] perf tools: Propagate error info from tp_format
Em Mon, Sep 07, 2015 at 10:38:06AM +0200, Jiri Olsa escreveu:
Quoted 9 lines
Propagate error info from
tp_formatviaERR_PTRto get it all the way down to theparse-event.ctracepoint adding routines. Following functions now return pointer with encoded error:
-tp_format
-trace_event__tp_format
-perf_evsel__newtp_idx
-perf_evsel__newtp
Quoted 3 lines
This affects several other places in perf, that cannot use pointer check anymore, but must utilize the
err.hinterface, when getting error information from above functions list.
Right, so this is tricky and we must be careful, see below...
Quoted 34 lines
Signed-off-by Jiri Olsa <[email protected]>--- tools/perf/builtin-trace.c | 19 +++++++++++-------- tools/perf/tests/evsel-tp-sched.c | 10 ++++++++-- tools/perf/tests/openat-syscall-all-cpus.c | 3 ++- tools/perf/tests/openat-syscall-tp-fields.c | 3 ++- tools/perf/tests/openat-syscall.c | 3 ++- tools/perf/util/evlist.c | 3 ++- tools/perf/util/evsel.c | 11 +++++++++-- tools/perf/util/evsel.h | 3 +++ tools/perf/util/parse-events.c | 6 +++--- tools/perf/util/trace-event.c | 13 +++++++++++-- 10 files changed, 53 insertions(+), 21 deletions(-)tools/perf/builtin-trace.c +2
-1@@ -38,6 +38,7 @@#include <stdlib.h> #include <sys/mman.h> #include <linux/futex.h> +#include <linux/err.h> /* For older distros: */ #ifndef MAP_STACK @@ -245,13 +246,14 @@ static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, voidstruct perf_evsel *evsel =perf_evsel__newtp ("raw_syscalls" , direction);/* older kernel (e.g., RHEL6) use syscalls:{enter,exit} */ -if (evsel ==NULL ) +if (IS_ERR (evsel)) evsel =perf_evsel__newtp ("syscalls" , direction);
tools/perf/builtin-trace.c static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, void · line 256
Quoted 10 lines
tools/perf/builtin-trace.c +5
-4-if (evsel) { -if (perf_evsel__init_syscall_tp (evsel, handler)) - goto out_delete; - } +if (IS_ERR (evsel)) +return NULL ; + +if (perf_evsel__init_syscall_tp (evsel, handler)) + goto out_delete;
This kind of stuff is ok, as evsel is a local variable and you kept the interface for perf_evsel__syscall_newtp(), i.e. it returns NULL if a new evsel can't be instantiated.
Ok, but that is a different interface than the one used by perf_evsel__newtp(), that also instantiates a new evsel.
So when one thinks about "foo__new()" we now need to check which one of the two interfaces it uses, if err.h or if the old NULL based failure reporting one.
Double tricky if it is foo__new() and foo__new_variant(), as perf_evsel__syscall_newtp() and perf_evsel__newtp(), i.e. both will return a "struct perf_evsel" instance, but one using err.h, the other use NULL.
Ok, you marked the ones using a comment, wonder if we couldn't use 'sparse' somehow here, is it used to check IS_ERR() usage in the kernel?
Ah, but what about this in trace__event_handler() in builtin-trace.c?
if (evsel->tp_format) {
event_format__fprintf(evsel->tp_format, sample->cpu,
sample->raw_data, sample->raw_size,
trace->output);
}Don't we have to use IS_ERR() here? Ok, no, because if setting up evsel->tp_format fails, then that evsel will be destroyed and perf_evsel__newtp() will return ERR_PTR(), so it is ok not no use ERR_PTR(evsel->tp_format) because it will only be != NULL when it was successfully set up.
But then, in perf_evsel__newtp_idx if zalloc() fails we will not return ERR_PTR(), but instead NULL, a-ha, this one seems to be a real bug, no?
/*
* Returns pointer with encoded error via <linux/err.h> interface.
*/
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 = {
.type = PERF_TYPE_TRACEPOINT,
.sample_type = (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME |
PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD),
};
if (asprintf(&evsel->name, "%s:%s", sys, name) < 0)
goto out_free;
evsel->tp_format = trace_event__tp_format(sys, name);
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;
attr.sample_period = 1;
perf_evsel__init(evsel, &attr, idx);
}
return evsel;out_free:
zfree(&evsel->name); free(evsel); return ERR_PTR(err); }
Quoted 273 lines
tools/perf/builtin-trace.c +4
-3return evsel;@@ -1705,12 +1707,12 @@ static int trace__read_syscall_info(struct trace *trace, int id)snprintf (tp_name,sizeof (tp_name),"sys_enter_%s" , sc->name); sc->tp_format =trace_event__tp_format ("syscalls" , tp_name); -if (sc->tp_format ==NULL && sc->fmt && sc->fmt->alias) { +if (IS_ERR (sc->tp_format) && sc->fmt && sc->fmt->alias) {snprintf (tp_name,sizeof (tp_name),"sys_enter_%s" , sc->fmt->alias); sc->tp_format =trace_event__tp_format ("syscalls" , tp_name); } -if (sc->tp_format ==NULL ) +if (IS_ERR (sc->tp_format))return -1 ; sc->args = sc->tp_format->format.fields;@@ -2390,7 +2392,8 @@ static size_t trace__fprintf_thread_summary(struct trace *trace, FILE *fp);static bool perf_evlist__add_vfs_getname (struct perf_evlist *evlist) {struct perf_evsel *evsel =perf_evsel__newtp ("probe" ,"vfs_getname" ); -if (evsel ==NULL ) + +if (IS_ERR (evsel))return false;if (perf_evsel__field (evsel,"pathname" ) ==NULL ) {tools/perf/tests/evsel-tp-sched.c +8
-2@@ -1,3 +1,4 @@ +#include <linux/err.h> #include <traceevent/event-parse.h> #include "evsel.h" #include "tests.h" @@ -36,8 +37,8 @@ int test__perf_evsel__tp_sched_test(void)struct perf_evsel *evsel =perf_evsel__newtp ("sched" ,"sched_switch" );int ret =0 ; -if (evsel ==NULL ) { -pr_debug ("perf_evsel__new\n" ); +if (IS_ERR (evsel)) { +pr_debug ("perf_evsel__newtp failed with %ld\n" ,PTR_ERR (evsel));return -1 ; }@@ -66,6 +67,11 @@ int test__perf_evsel__tp_sched_test(void) evsel =perf_evsel__newtp ("sched" ,"sched_wakeup" ); +if (IS_ERR (evsel)) { +pr_debug ("perf_evsel__newtp failed with %ld\n" ,PTR_ERR (evsel)); +return -1 ; + } +if (perf_evsel__test_field (evsel,"comm" ,16 , true)) ret =-1 ;tools/perf/tests/openat-syscall-all-cpus.c +2
-1@@ -1,4 +1,5 @@#include <api/fs/fs.h> +#include <linux/err.h> #include "evsel.h" #include "tests.h" #include "thread_map.h" @@ -31,7 +32,7 @@ int test__openat_syscall_event_on_all_cpus(void)CPU_ZERO (&cpu_set); evsel =perf_evsel__newtp ("syscalls" ,"sys_enter_openat" ); -if (evsel ==NULL ) { +if (IS_ERR (evsel)) {tracing_path__strerror_open_tp (errno, errbuf,sizeof (errbuf),"syscalls" ,"sys_enter_openat" );pr_err ("%s\n" , errbuf); goto out_thread_map_delete;tools/perf/tests/openat-syscall-tp-fields.c +2
-1@@ -1,3 +1,4 @@ +#include <linux/err.h> #include "perf.h" #include "evlist.h" #include "evsel.h" @@ -30,7 +31,7 @@ int test__syscall_openat_tp_fields(void) } evsel =perf_evsel__newtp ("syscalls" ,"sys_enter_openat" ); -if (evsel ==NULL ) { +if (IS_ERR (evsel)) {pr_debug ("%s: perf_evsel__newtp\n" , __func__); goto out_delete_evlist; }tools/perf/tests/openat-syscall.c +2
-1@@ -1,4 +1,5 @@#include <api/fs/tracing_path.h> +#include <linux/err.h> #include "thread_map.h" #include "evsel.h" #include "debug.h" @@ -19,7 +20,7 @@ int test__openat_syscall_event(void) } evsel =perf_evsel__newtp ("syscalls" ,"sys_enter_openat" ); -if (evsel ==NULL ) { +if (IS_ERR (evsel)) {tracing_path__strerror_open_tp (errno, errbuf,sizeof (errbuf),"syscalls" ,"sys_enter_openat" );pr_err ("%s\n" , errbuf); goto out_thread_map_delete;tools/perf/util/evlist.c +2
-1@@ -25,6 +25,7 @@#include <linux/bitops.h> #include <linux/hash.h> #include <linux/log2.h> +#include <linux/err.h> static void perf_evlist__mmap_put (struct perf_evlist *evlist,int idx);static void __perf_evlist__munmap (struct perf_evlist *evlist,int idx);@@ -265,7 +266,7 @@ int perf_evlist__add_newtp(struct perf_evlist *evlist, {struct perf_evsel *evsel =perf_evsel__newtp (sys, name); -if (evsel ==NULL ) +if (IS_ERR (evsel))return -1 ; evsel->handler = handler;tools/perf/util/evsel.c +9
-2@@ -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" @@ -225,9 +226,13 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)return evsel; } +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ 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 = {@@ -240,8 +245,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;@@ -254,7 +261,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 ] = {tools/perf/util/evsel.h +3
-0@@ -161,6 +161,9 @@ static inline struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr)struct perf_evsel *perf_evsel__newtp_idx (const char *sys,const char *name,int idx); +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ static inline struct perf_evsel *perf_evsel__newtp (const char *sys,const char *name) {return perf_evsel__newtp_idx (sys, name,0 );tools/perf/util/parse-events.c +3
-3@@ -1,4 +1,5 @@#include <linux/hw_breakpoint.h> +#include <linux/err.h> #include "util.h" #include "../perf.h" #include "evlist.h" @@ -393,11 +394,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 (!evsel) -return -ENOMEM ; +if (IS_ERR (evsel)) +return PTR_ERR (evsel);list_add_tail (&evsel->node, list); -return 0 ; }tools/perf/util/trace-event.c +11
-2@@ -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 <api/fs/tracing_path.h> #include "trace-event.h" @@ -66,6 +67,9 @@ void trace_event__cleanup(struct trace_event *t)pevent_free (t->pevent); } +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ static struct event_format *tp_format (const char *sys,const char *name) {@@ -74,12 +78,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);@@ -87,6 +93,9 @@ tp_format(const char *sys, const char *name)return event; } +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ struct event_format *trace_event__tp_format (const char *sys,const char *name) {Signature · 1 line
2.4.3
Re: [PATCH 4/5] perf tools: Propagate error info from tp_format
On Wed, Sep 09, 2015 at 05:58:13PM -0300, Arnaldo Carvalho de Melo wrote:
SNIP
Quoted 11 lines
This kind of stuff is ok, as evsel is a local variable and you kept the interface for
perf_evsel__syscall_newtp(), i.e. it returns NULL if a new evsel can't be instantiated.Ok, but that is a different interface than the one used by
perf_evsel__newtp(), that also instantiates a new evsel.So when one thinks about "
foo__new()" we now need to check which one of the two interfaces it uses, iferr.hor if the old NULL based failure reporting one.
Quoted 7 lines
Double tricky if it is
foo__new()andfoo__new_variant(), asperf_evsel__syscall_newtp()andperf_evsel__newtp(), i.e. both will return a "structperf_evsel" instance, but one usingerr.h, the other use NULL.Ok, you marked the ones using a comment, wonder if we couldn't use 'sparse' somehow here, is it used to check
IS_ERR()usage in the kernel?
hum, not sure.. will check ;-)
at least we could mark related functions with __must_check to force the return value check
Quoted 10 lines
Ah, but what about this in
trace__event_handler()inbuiltin-trace.c?if (evsel->tp_format) { event_format__fprintf(evsel->tp_format, sample->cpu, sample->raw_data, sample->raw_size, trace->output); }
Quoted 8 lines
Don't we have to use
IS_ERR()here? Ok, no, because if setting upevsel->tp_formatfails, then that evsel will be destroyed andperf_evsel__newtp()will returnERR_PTR(), so it is ok not no useERR_PTR(evsel->tp_format)because it will only be != NULL when it was successfully set up.But then, in
perf_evsel__newtp_idxifzalloc()fails we will not returnERR_PTR(), but instead NULL, a-ha, this one seems to be a real bug, no?
hate those allocations in declarations.. never do any good ;-)
yep, NULL is not an error, so it's real bug, attached patch should fix it
thanks,
jirka
---
tools/perf/util/evsel.c +1 -1
@@ -256,7 +256,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, intperf_evsel__init (evsel, &attr, idx); } -return evsel; +return evsel ?:ERR_PTR (err); out_free:zfree (&evsel->name);
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 4/5] perf tools: Propagate error info from tp_format
Em Thu, Sep 10, 2015 at 10:24:52AM +0200, Jiri Olsa escreveu:
Quoted 20 lines · Arnaldo Carvalho de Melo
SNIP
Quoted 16 lines
This kind of stuff is ok, as evsel is a local variable and you kept the interface for
perf_evsel__syscall_newtp(), i.e. it returns NULL if a new evsel can't be instantiated.Ok, but that is a different interface than the one used by
perf_evsel__newtp(), that also instantiates a new evsel.So when one thinks about "
foo__new()" we now need to check which one of the two interfaces it uses, iferr.hor if the old NULL based failure reporting one.Double tricky if it is
foo__new()andfoo__new_variant(), asperf_evsel__syscall_newtp()andperf_evsel__newtp(), i.e. both will return a "structperf_evsel" instance, but one usingerr.h, the other use NULL.
Quoted 7 lines
Quoted 2 lines
Ok, you marked the ones using a comment, wonder if we couldn't use 'sparse' somehow here, is it used to check
IS_ERR()usage in the kernel?hum, not sure.. will check ;-)
at least we could mark related functions with
__must_checkto force the return value check
Right, that helps a bit, but not when the test _is already there_, against NULL.
That is why I thought about sparse, if it was used in the kernel somehow to check for this, guess either it would notice ERR_PTR using routines and then auto-mark them for checking if they are being tested using IS_ERR() or plain NULL, will check, later...
- Arnaldo
Quoted 41 lines
Quoted 10 lines
Ah, but what about this in
trace__event_handler()inbuiltin-trace.c?if (evsel->tp_format) { event_format__fprintf(evsel->tp_format, sample->cpu, sample->raw_data, sample->raw_size, trace->output); }Quoted 8 lines
Don't we have to use
IS_ERR()here? Ok, no, because if setting upevsel->tp_formatfails, then that evsel will be destroyed andperf_evsel__newtp()will returnERR_PTR(), so it is ok not no useERR_PTR(evsel->tp_format)because it will only be != NULL when it was successfully set up.But then, in
perf_evsel__newtp_idxifzalloc()fails we will not returnERR_PTR(), but instead NULL, a-ha, this one seems to be a real bug, no?hate those allocations in declarations.. never do any good ;-)
yep, NULL is not an error, so it's real bug, attached patch should fix it
thanks,
jirka---
tools/perf/util/evsel.c +1
-1@@ -256,7 +256,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, intperf_evsel__init (evsel, &attr, idx); } -return evsel; +return evsel ?:ERR_PTR (err); out_free:zfree (&evsel->name);
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 4/5] perf tools: Propagate error info from tp_format
In reply to Jiri OlsaEm Thu, Sep 10, 2015 at 10:24:52AM +0200, Jiri Olsa escreveu:
Quoted 47 lines · Arnaldo Carvalho de Melo
SNIP
Quoted 11 lines
This kind of stuff is ok, as evsel is a local variable and you kept the interface for
perf_evsel__syscall_newtp(), i.e. it returns NULL if a new evsel can't be instantiated.Ok, but that is a different interface than the one used by
perf_evsel__newtp(), that also instantiates a new evsel.So when one thinks about "
foo__new()" we now need to check which one of the two interfaces it uses, iferr.hor if the old NULL based failure reporting one.Quoted 7 lines
Double tricky if it is
foo__new()andfoo__new_variant(), asperf_evsel__syscall_newtp()andperf_evsel__newtp(), i.e. both will return a "structperf_evsel" instance, but one usingerr.h, the other use NULL.Ok, you marked the ones using a comment, wonder if we couldn't use 'sparse' somehow here, is it used to check
IS_ERR()usage in the kernel?hum, not sure.. will check ;-)
at least we could mark related functions with
__must_checkto force the return value checkQuoted 10 lines
Ah, but what about this in
trace__event_handler()inbuiltin-trace.c?if (evsel->tp_format) { event_format__fprintf(evsel->tp_format, sample->cpu, sample->raw_data, sample->raw_size, trace->output); }Quoted 8 lines
Don't we have to use
IS_ERR()here? Ok, no, because if setting upevsel->tp_formatfails, then that evsel will be destroyed andperf_evsel__newtp()will returnERR_PTR(), so it is ok not no useERR_PTR(evsel->tp_format)because it will only be != NULL when it was successfully set up.But then, in
perf_evsel__newtp_idxifzalloc()fails we will not returnERR_PTR(), but instead NULL, a-ha, this one seems to be a real bug, no?
Quoted 6 lines
hate those allocations in declarations.. never do any good ;-)
yep, NULL is not an error, so it's real bug, attached patch should fix it
thanks,
jirka
Ok continuing, found two more problems in this patch, fixed as follows, merging.
- Arnaldo
tools/perf/tests/mmap-basic.c +1 -1
@@ -65,7 +65,7 @@ int test__basic_mmap(void)snprintf (name,sizeof (name),"sys_enter_%s" , syscall_names[i]); evsels[i] =perf_evsel__newtp ("syscalls" , name); -if (evsels[i] ==NULL ) { +if (IS_ERR (evsels[i]) ==NULL ) {pr_debug ("perf_evsel__new\n" ); goto out_delete_evlist; }
tools/perf/util/evsel.c +4 -1
@@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, intstruct perf_evsel *evsel =zalloc (perf_evsel__object.size);int err = -ENOMEM ; - if (evsel != NULL) { +if (evsel ==NULL ) { + goto out_err; + } else { struct perf_event_attr attr = { .type = PERF_TYPE_TRACEPOINT, .sample_type = (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME |
@@ -261,6 +263,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int out_free:zfree (&evsel->name);free (evsel); +out_err:return ERR_PTR (err); }
tools/perf/util/trace-event.c +1 -1
@@ -100,7 +100,7 @@ struct event_format* trace_event__tp_format(const char *sys, const char *name) {if (!tevent_initialized &&trace_event__init2 ()) -return NULL ; +return ERR_PTR (-ENOMEM );return tp_format (sys, name); }
Re: [PATCH 4/5] perf tools: Propagate error info from tp_format
2015-09-14 16:53 GMT-04:00 Arnaldo Carvalho de Melo <[email protected]>:
Quoted 79 lines
Em Thu, Sep 10, 2015 at 10:24:52AM +0200, Jiri Olsa escreveu:
Quoted 47 lines · Arnaldo Carvalho de Melo
SNIP
Quoted 11 lines
This kind of stuff is ok, as evsel is a local variable and you kept the interface for
perf_evsel__syscall_newtp(), i.e. it returns NULL if a new evsel can't be instantiated.Ok, but that is a different interface than the one used by
perf_evsel__newtp(), that also instantiates a new evsel.So when one thinks about "
foo__new()" we now need to check which one of the two interfaces it uses, iferr.hor if the old NULL based failure reporting one.Quoted 7 lines
Double tricky if it is
foo__new()andfoo__new_variant(), asperf_evsel__syscall_newtp()andperf_evsel__newtp(), i.e. both will return a "structperf_evsel" instance, but one usingerr.h, the other use NULL.Ok, you marked the ones using a comment, wonder if we couldn't use 'sparse' somehow here, is it used to check
IS_ERR()usage in the kernel?hum, not sure.. will check ;-)
at least we could mark related functions with
__must_checkto force the return value checkQuoted 10 lines
Ah, but what about this in
trace__event_handler()inbuiltin-trace.c?if (evsel->tp_format) { event_format__fprintf(evsel->tp_format, sample->cpu, sample->raw_data, sample->raw_size, trace->output); }Quoted 8 lines
Don't we have to use
IS_ERR()here? Ok, no, because if setting upevsel->tp_formatfails, then that evsel will be destroyed andperf_evsel__newtp()will returnERR_PTR(), so it is ok not no useERR_PTR(evsel->tp_format)because it will only be != NULL when it was successfully set up.But then, in
perf_evsel__newtp_idxifzalloc()fails we will not returnERR_PTR(), but instead NULL, a-ha, this one seems to be a real bug, no?Quoted 6 lines
hate those allocations in declarations.. never do any good ;-)
yep, NULL is not an error, so it's real bug, attached patch should fix it
thanks,
jirkaOk continuing, found two more problems in this patch, fixed as follows, merging.
- Arnaldo
tools/perf/tests/mmap-basic.c +1
-1@@ -65,7 +65,7 @@ int test__basic_mmap(void)snprintf (name,sizeof (name),"sys_enter_%s" , syscall_names[i]); evsels[i] =perf_evsel__newtp ("syscalls" , name); -if (evsels[i] ==NULL ) { +if (IS_ERR (evsels[i]) ==NULL ) {pr_debug ("perf_evsel__new\n" ); goto out_delete_evlist; }tools/perf/util/evsel.c +0
-0
tools/perf/util/evsel.c struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int · line 239
Quoted 7 lines
tools/perf/util/evsel.c +3
-1@@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, intstruct perf_evsel *evsel =zalloc (perf_evsel__object.size);int err = -ENOMEM ; -if (evsel !=NULL ) { +if (evsel ==NULL ) { + goto out_err; + }else {
Is the else really necessary after a goto?
Quoted 24 lines
tools/perf/util/evsel.c +1
-0struct perf_event_attr attr = { .type =PERF_TYPE_TRACEPOINT , .sample_type = (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME |@@ -261,6 +263,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int out_free:zfree (&evsel->name);free (evsel); +out_err:return ERR_PTR (err); }tools/perf/util/trace-event.c +1
-1@@ -100,7 +100,7 @@ struct event_format* trace_event__tp_format(const char *sys, const char *name) {if (!tevent_initialized &&trace_event__init2 ()) -return NULL ; +return ERR_PTR (-ENOMEM );return tp_format (sys, name); }
Re: [PATCH 4/5] perf tools: Propagate error info from tp_format
Em Mon, Sep 14, 2015 at 04:59:41PM -0400, Raphaël Beamonte escreveu:
Quoted 6 lines
2015-09-14 16:53 GMT-04:00 Arnaldo Carvalho de Melo <[email protected]>:
Quoted 5 lines
+++
b/tools/perf/util/evsel.ctools/perf/util/evsel.c +0
-0@@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, intstruct perf_evsel *evsel =zalloc (perf_evsel__object.size);int err = -ENOMEM ;
Quoted 6 lines
tools/perf/util/evsel.c struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int · line 239
Quoted 4 lines
tools/perf/util/evsel.c +3
-1-if (evsel !=NULL ) { +if (evsel ==NULL ) { + goto out_err; + }else {Is the else really necessary after a goto?
Not really, we can remove it and all would be equivalent (the code with/without should be the same), its just that I wanted to avoid touching the identation to reduce patch size and since we need o open a brace to declare that attr variable...
- Arnaldo
Quoted 24 lines
Quoted 24 lines
tools/perf/util/evsel.c +1
-0struct perf_event_attr attr = { .type =PERF_TYPE_TRACEPOINT , .sample_type = (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME |@@ -261,6 +263,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int out_free:zfree (&evsel->name);free (evsel); +out_err:return ERR_PTR (err); }tools/perf/util/trace-event.c +1
-1@@ -100,7 +100,7 @@ struct event_format* trace_event__tp_format(const char *sys, const char *name) {if (!tevent_initialized &&trace_event__init2 ()) -return NULL ; +return ERR_PTR (-ENOMEM );return tp_format (sys, name); }
Re: [PATCH 4/5] perf tools: Propagate error info from tp_format
2015-09-14 17:36 GMT-04:00 Arnaldo Carvalho de Melo <[email protected]>:
Quoted 14 lines
Em Mon, Sep 14, 2015 at 04:59:41PM -0400, Raphaël Beamonte escreveu:
Quoted 6 lines
2015-09-14 16:53 GMT-04:00 Arnaldo Carvalho de Melo <[email protected]>:
Quoted 5 lines
+++
b/tools/perf/util/evsel.ctools/perf/util/evsel.c +0
-0@@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, intstruct perf_evsel *evsel =zalloc (perf_evsel__object.size);int err = -ENOMEM ;Quoted 6 lines
tools/perf/util/evsel.c struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int · line 239
Quoted 4 lines
tools/perf/util/evsel.c +3
-1-if (evsel !=NULL ) { +if (evsel ==NULL ) { + goto out_err; + }else {Is the else really necessary after a goto?
Quoted 6 lines
Not really, we can remove it and all would be equivalent (the code with/without should be the same), its just that I wanted to avoid touching the identation to reduce patch size and since we need o open a brace to declare that attr variable...
- Arnaldo
Ok. Though, given the content of that function, we could probably declare attr in the first lines of the function and assign it its value after the if. But I understand the patch size argument! :o) On another subject, but on the same lines, shouldn't we use if (!evsel) instead of if (evsel == NULL)? (kernel code style if I'm not mistaken) Or is there something that prevents from using it here?
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
tools/perf/util/evsel.c +1
-0struct perf_event_attr attr = { .type =PERF_TYPE_TRACEPOINT , .sample_type = (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME |@@ -261,6 +263,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int out_free:zfree (&evsel->name);free (evsel); +out_err:return ERR_PTR (err); }tools/perf/util/trace-event.c +1
-1@@ -100,7 +100,7 @@ struct event_format* trace_event__tp_format(const char *sys, const char *name) {if (!tevent_initialized &&trace_event__init2 ()) -return NULL ; +return ERR_PTR (-ENOMEM );return tp_format (sys, name); }
Re: [PATCH 4/5] perf tools: Propagate error info from tp_format
Em Mon, Sep 14, 2015 at 06:05:38PM -0400, Raphaël Beamonte escreveu:
Quoted 15 lines
2015-09-14 17:36 GMT-04:00 Arnaldo Carvalho de Melo <[email protected]>:
Quoted 14 lines
Em Mon, Sep 14, 2015 at 04:59:41PM -0400, Raphaël Beamonte escreveu:
Quoted 12 lines
2015-09-14 16:53 GMT-04:00 Arnaldo Carvalho de Melo <[email protected]>:
tools/perf/util/evsel.c struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int · line 239
Quoted 9 lines
+++
b/tools/perf/util/evsel.ctools/perf/util/evsel.c +3
-1@@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, intstruct perf_evsel *evsel =zalloc (perf_evsel__object.size);int err = -ENOMEM ; -if (evsel !=NULL ) { +if (evsel ==NULL ) { + goto out_err; + }else {Is the else really necessary after a goto?
Quoted 8 lines
Quoted 4 lines
Not really, we can remove it and all would be equivalent (the code with/without should be the same), its just that I wanted to avoid touching the identation to reduce patch size and since we need o open a brace to declare that attr variable...
Ok. Though, given the content of that function, we could probably declare attr in the first lines of the function and assign it its value after the if. But I understand the patch size argument! :o)
Yeah, in other times I would have cleaned it all up, having that else is ugly, but review time is more expensive, I think, so we need to reduce its cost.
Cleaning it up makes it easier to read, but we get the info about when that particular code was written buried a bit below the reindentation cset.
Would be nice to have a 'git blame' that would just unwind things like that :-)
Quoted 3 lines
On another subject, but on the same lines, shouldn't we use if (!evsel) instead of if (evsel == NULL)? (kernel code style if I'm not mistaken) Or is there something that prevents from using it here?
I'd say both are ok. (evsel) is shorter than (evsel != NULL), but the later is more expressive, at a glance you know it is a pointer, etc.
- Arnaldo
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
tools/perf/util/evsel.c +1
-0struct perf_event_attr attr = { .type =PERF_TYPE_TRACEPOINT , .sample_type = (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME |@@ -261,6 +263,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int out_free:zfree (&evsel->name);free (evsel); +out_err:return ERR_PTR (err); }tools/perf/util/trace-event.c +1
-1@@ -100,7 +100,7 @@ struct event_format* trace_event__tp_format(const char *sys, const char *name) {if (!tevent_initialized &&trace_event__init2 ()) -return NULL ; +return ERR_PTR (-ENOMEM );return tp_format (sys, name); }
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 4/5] perf tools: Propagate error info from tp_format
In reply to Arnaldo Carvalho de MeloEm Mon, Sep 14, 2015 at 05:53:03PM -0300, Arnaldo Carvalho de Melo escreveu:
tools/perf/tests/mmap-basic.c int test__basic_mmap(void) · line 68
Quoted 12 lines
Ok continuing, found two more problems in this patch, fixed as follows, merging.
- Arnaldo
+++
b/tools/perf/tests/mmap-basic.ctools/perf/tests/mmap-basic.c +1
-1@@ -65,7 +65,7 @@ int test__basic_mmap(void)snprintf (name,sizeof (name),"sys_enter_%s" , syscall_names[i]); evsels[i] =perf_evsel__newtp ("syscalls" , name); -if (evsels[i] ==NULL ) { +if (IS_ERR (evsels[i]) ==NULL ) {
modulo the == NULL ;-)
Quoted 3 lines
tools/perf/tests/mmap-basic.c +0
-0pr_debug ("perf_evsel__new\n" ); goto out_delete_evlist; }
tip-bot for Jiri Olsa
merge bot[tip:perf/core] perf evsel: Propagate error info from tp_format
In reply to Jiri Olsaperf evsel: Propagate error info from tp_format
Commit-ID: 8dd2a1317eba2c207632dcb19adb7cb746861652 Gitweb: http://git.kernel.org/tip/8dd2a1317eba2c207632dcb19adb7cb746861652 Author: Jiri Olsa <[email protected]> AuthorDate: Mon, 7 Sep 2015 10:38:06 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 15 Sep 2015 09:48:33 -0300
perf evsel: Propagate error info from tp_format
Propagate error info from tp_format via ERR_PTR to get it all the way down to the parse-event.c tracepoint adding routines. Following functions now return pointer with encoded error:
- tp_format
- trace_event__tp_format
- perf_evsel__newtp_idx
- perf_evsel__newtp
This affects several other places in perf, that cannot use pointer check anymore, but must utilize the err.h interface, when getting error information from above functions list.
--- tools/perf/builtin-trace.c | 19 +++++++++++-------- tools/perf/tests/evsel-tp-sched.c | 10 ++++++++-- tools/perf/tests/mmap-basic.c | 3 ++- tools/perf/tests/openat-syscall-all-cpus.c | 3 ++- tools/perf/tests/openat-syscall-tp-fields.c | 3 ++- tools/perf/tests/openat-syscall.c | 3 ++- tools/perf/util/evlist.c | 3 ++- tools/perf/util/evsel.c | 16 +++++++++++++--- tools/perf/util/evsel.h | 3 +++ tools/perf/util/parse-events.c | 6 +++--- tools/perf/util/trace-event.c | 15 ++++++++++++--- 11 files changed, 60 insertions(+), 24 deletions(-)
tools/perf/builtin-trace.c +11 -8
@@ -38,6 +38,7 @@#include <stdlib.h> #include <sys/mman.h> #include <linux/futex.h> +#include <linux/err.h> /* For older distros: */ #ifndef MAP_STACK
@@ -245,13 +246,14 @@ static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, voidstruct perf_evsel *evsel =perf_evsel__newtp ("raw_syscalls" , direction);/* older kernel (e.g., RHEL6) use syscalls:{enter,exit} */ -if (evsel ==NULL ) +if (IS_ERR (evsel)) evsel =perf_evsel__newtp ("syscalls" , direction); -if (evsel) { -if (perf_evsel__init_syscall_tp (evsel, handler)) - goto out_delete; - } +if (IS_ERR (evsel)) +return NULL ; + +if (perf_evsel__init_syscall_tp (evsel, handler)) + goto out_delete;return evsel;
@@ -1705,12 +1707,12 @@ static int trace__read_syscall_info(struct trace *trace, int id)snprintf (tp_name,sizeof (tp_name),"sys_enter_%s" , sc->name); sc->tp_format =trace_event__tp_format ("syscalls" , tp_name); -if (sc->tp_format ==NULL && sc->fmt && sc->fmt->alias) { +if (IS_ERR (sc->tp_format) && sc->fmt && sc->fmt->alias) {snprintf (tp_name,sizeof (tp_name),"sys_enter_%s" , sc->fmt->alias); sc->tp_format =trace_event__tp_format ("syscalls" , tp_name); } -if (sc->tp_format ==NULL ) +if (IS_ERR (sc->tp_format))return -1 ; sc->args = sc->tp_format->format.fields;
@@ -2390,7 +2392,8 @@ static size_t trace__fprintf_thread_summary(struct trace *trace, FILE *fp);static bool perf_evlist__add_vfs_getname (struct perf_evlist *evlist) {struct perf_evsel *evsel =perf_evsel__newtp ("probe" ,"vfs_getname" ); -if (evsel ==NULL ) + +if (IS_ERR (evsel))return false;if (perf_evsel__field (evsel,"pathname" ) ==NULL ) {
tools/perf/tests/evsel-tp-sched.c +8 -2
@@ -1,3 +1,4 @@ +#include <linux/err.h> #include <traceevent/event-parse.h> #include "evsel.h" #include "tests.h"
@@ -36,8 +37,8 @@ int test__perf_evsel__tp_sched_test(void)struct perf_evsel *evsel =perf_evsel__newtp ("sched" ,"sched_switch" );int ret =0 ; -if (evsel ==NULL ) { -pr_debug ("perf_evsel__new\n" ); +if (IS_ERR (evsel)) { +pr_debug ("perf_evsel__newtp failed with %ld\n" ,PTR_ERR (evsel));return -1 ; }
@@ -66,6 +67,11 @@ int test__perf_evsel__tp_sched_test(void) evsel =perf_evsel__newtp ("sched" ,"sched_wakeup" ); +if (IS_ERR (evsel)) { +pr_debug ("perf_evsel__newtp failed with %ld\n" ,PTR_ERR (evsel)); +return -1 ; + } +if (perf_evsel__test_field (evsel,"comm" ,16 , true)) ret =-1 ;
tools/perf/tests/mmap-basic.c +2 -1
@@ -3,6 +3,7 @@#include "thread_map.h" #include "cpumap.h" #include "tests.h" +#include <linux/err.h> /* * This test will generate random numbers of calls to some getpid syscalls,
@@ -65,7 +66,7 @@ int test__basic_mmap(void)snprintf (name,sizeof (name),"sys_enter_%s" , syscall_names[i]); evsels[i] =perf_evsel__newtp ("syscalls" , name); -if (evsels[i] ==NULL ) { +if (IS_ERR (evsels[i])) {pr_debug ("perf_evsel__new\n" ); goto out_delete_evlist; }
tools/perf/tests/openat-syscall-all-cpus.c +2 -1
@@ -1,4 +1,5 @@#include <api/fs/fs.h> +#include <linux/err.h> #include "evsel.h" #include "tests.h" #include "thread_map.h"
@@ -31,7 +32,7 @@ int test__openat_syscall_event_on_all_cpus(void)CPU_ZERO (&cpu_set); evsel =perf_evsel__newtp ("syscalls" ,"sys_enter_openat" ); -if (evsel ==NULL ) { +if (IS_ERR (evsel)) {tracing_path__strerror_open_tp (errno, errbuf,sizeof (errbuf),"syscalls" ,"sys_enter_openat" );pr_err ("%s\n" , errbuf); goto out_thread_map_delete;
tools/perf/tests/openat-syscall-tp-fields.c +2 -1
@@ -1,3 +1,4 @@ +#include <linux/err.h> #include "perf.h" #include "evlist.h" #include "evsel.h"
@@ -30,7 +31,7 @@ int test__syscall_openat_tp_fields(void) } evsel =perf_evsel__newtp ("syscalls" ,"sys_enter_openat" ); -if (evsel ==NULL ) { +if (IS_ERR (evsel)) {pr_debug ("%s: perf_evsel__newtp\n" , __func__); goto out_delete_evlist; }
tools/perf/tests/openat-syscall.c +2 -1
@@ -1,4 +1,5 @@#include <api/fs/tracing_path.h> +#include <linux/err.h> #include "thread_map.h" #include "evsel.h" #include "debug.h"
@@ -19,7 +20,7 @@ int test__openat_syscall_event(void) } evsel =perf_evsel__newtp ("syscalls" ,"sys_enter_openat" ); -if (evsel ==NULL ) { +if (IS_ERR (evsel)) {tracing_path__strerror_open_tp (errno, errbuf,sizeof (errbuf),"syscalls" ,"sys_enter_openat" );pr_err ("%s\n" , errbuf); goto out_thread_map_delete;
tools/perf/util/evlist.c +2 -1
@@ -25,6 +25,7 @@#include <linux/bitops.h> #include <linux/hash.h> #include <linux/log2.h> +#include <linux/err.h> static void perf_evlist__mmap_put (struct perf_evlist *evlist,int idx);static void __perf_evlist__munmap (struct perf_evlist *evlist,int idx);
@@ -265,7 +266,7 @@ int perf_evlist__add_newtp(struct perf_evlist *evlist, {struct perf_evsel *evsel =perf_evsel__newtp (sys, name); -if (evsel ==NULL ) +if (IS_ERR (evsel))return -1 ; evsel->handler = handler;
tools/perf/util/evsel.c +13 -3
@@ -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"
@@ -225,11 +226,17 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)return evsel; } +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ 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) { + if (evsel == NULL) { + goto out_err; + } else { struct perf_event_attr attr = { .type = PERF_TYPE_TRACEPOINT, .sample_type = (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME |
@@ -240,8 +247,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;
@@ -254,7 +263,8 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int out_free:zfree (&evsel->name);free (evsel); -return NULL ; +out_err: +return ERR_PTR (err); }const char *perf_evsel__hw_names[PERF_COUNT_HW_MAX ] = {
tools/perf/util/evsel.h +3 -0
@@ -160,6 +160,9 @@ static inline struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr)struct perf_evsel *perf_evsel__newtp_idx (const char *sys,const char *name,int idx); +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ static inline struct perf_evsel *perf_evsel__newtp (const char *sys,const char *name) {return perf_evsel__newtp_idx (sys, name,0 );
tools/perf/util/parse-events.c +3 -3
@@ -1,4 +1,5 @@#include <linux/hw_breakpoint.h> +#include <linux/err.h> #include "util.h" #include "../perf.h" #include "evlist.h"
@@ -393,11 +394,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 (!evsel) -return -ENOMEM ; +if (IS_ERR (evsel)) +return PTR_ERR (evsel);list_add_tail (&evsel->node, list); -return 0 ; }
tools/perf/util/trace-event.c +12 -3
@@ -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 <api/fs/tracing_path.h> #include "trace-event.h"
@@ -66,6 +67,9 @@ void trace_event__cleanup(struct trace_event *t)pevent_free (t->pevent); } +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ static struct event_format *tp_format (const char *sys,const char *name) {
@@ -74,12 +78,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);
@@ -87,11 +93,14 @@ tp_format(const char *sys, const char *name)return event; } +/* +* Returns pointer with encoded error via <linux/err.h> interface. +*/ struct event_format *trace_event__tp_format (const char *sys,const char *name) {if (!tevent_initialized &&trace_event__init2 ()) -return NULL ; +return ERR_PTR (-ENOMEM );return tp_format (sys, name); }
Jiri Olsa
reviewerPATCH v2 5/5 perf tools: Enhance parsing events tracepoint error output
In reply to Jiri OlsaEnhancing 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
...
--- tools/perf/util/parse-events.c | 35 ++++++++++++++++++++++++++++++++--- tools/perf/util/parse-events.y | 16 +++++++++------- 2 files changed, 41 insertions(+), 10 deletions(-)
tools/perf/util/parse-events.c +32 -3
@@ -387,6 +387,33 @@ 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 ]; + +/* +* We get error directly from syscall errno ( > 0), +* or from encoded pointer's error ( < 0). +*/ + 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 +421,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 PTR_ERR (evsel); + }list_add_tail (&evsel->node, list);return 0 ;
@@ -413,7 +442,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 ; }
@@ -453,7 +482,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 ; }
tools/perf/util/parse-events.y +9 -7
@@ -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;
Signature · 1 line
2.4.3
Namhyung Kim
maintainerQuoted 14 lines · 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 tracepointError: File
/sys/kernel/debug/tracing//tracing/events/sched/sched_kravanot found.
Hint: Perhaps this kernel misses someCONFIG_setting to enable this feature?.Run 'perf list' for a list of valid events
...
Quoted 6 lines
$ perf record -e sched:sched_krava ls event syntax error: 'sched:sched_krava' \___ can't access trace eventsError: No permissions to read
/sys/kernel/debug/tracing//tracing/events/sched/sched_krava
Hint: Try 'sudo mount -o remount,mode=755/sys/kernel/debug'
What about tracefs? On my system, tracefs is mounted on /sys/kernel/debug/tracing thus I cannot access trace events after remounting debugfs with mode=755.
Also, IIRC tracepoint events adds PERF_SAMPLE_RAW bit automatically, and it requires perf_event_paranoid being -1 for non-root user, right?
Thanks,
Namhyung
Quoted 19 lines · Namhyung Kim
Quoted 18 lines · 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 tracepointError: File
/sys/kernel/debug/tracing//tracing/events/sched/sched_kravanot found.
Hint: Perhaps this kernel misses someCONFIG_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
Quoted 6 lines
Quoted 2 lines
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'What about tracefs? On my system, tracefs is mounted on
/sys/kernel/debug/tracingthus I cannot access trace events after remounting debugfs with mode=755.
right, patch below keeps the actual mount and
display proper info.. could you please try?
Quoted 3 lines
Also, IIRC tracepoint events adds
PERF_SAMPLE_RAWbit automatically, and it requiresperf_event_paranoidbeing -1 for non-root user, right?
there's related error message when you try to open the tracepoint, the whole session is like:
[jolsa@krava perf]$ ./perf record -e sched:sched_switch ls event syntax error: 'sched:sched_switch'
\___ can't access trace events
Error: No permissions to read /sys/kernel/debug/tracing/events/sched/sched_switch Hint: Try 'sudo mount -o remount,mode=755 /sys/kernel/debug/tracing'
Run 'perf list' for a list of valid events
usage: perf record [<options>] [<command>]
or: perf record [<options>] -- <command> [<options>]
-e, --event <event> event selector. use 'perf list' to list available events
[jolsa@krava perf]$ sudo mount -o remount,mode=755 /sys/kernel/debug/tracing [jolsa@krava perf]$ ./perf record -e sched:sched_switch ls Error:
You may not have permission to collect stats.
Consider tweaking /proc/sys/kernel/perf_event_paranoid:
-1 - Not paranoid at all
0 - Disallow raw tracepoint access for unpriv
1 - Disallow cpu events for unpriv
2 - Disallow kernel profiling for unpriv
thanks,
jirka
---
tools/lib/api/fs/tracing_path.c +3 -10
@@ -12,12 +12,14 @@#include "tracing_path.h" +char tracing_mnt[PATH_MAX +1 ] ="/sys/kernel/debug" ;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_mnt,sizeof (tracing_mnt),"%s" , mountpoint);snprintf (tracing_path,sizeof (tracing_path),"%s/%s" , mountpoint, tracing); snprintf(tracing_events_path, sizeof(tracing_events_path), "%s/%s%s",
@@ -109,19 +111,10 @@ static int strerror_open(int err, char *buf, size_t size, const char *filename) "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");break ;case EACCES : { -const char *mountpoint =debugfs__mountpoint (); - -if (!access (mountpoint,R_OK ) &&strncmp (filename,"tracing/" ,8 ) ==0 ) { -const char *tracefs_mntpoint =tracefs__mountpoint (); - -if (tracefs_mntpoint) - mountpoint =tracefs__mountpoint (); - } -snprintf (buf, size,"Error:\tNo permissions to read %s/%s\n" "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n" , - tracing_events_path, filename, mountpoint); + tracing_events_path, filename, tracing_mnt); }break ;default :
Namhyung Kim
maintainerHi Jiri,
Quoted 23 lines · Jiri Olsa
Quoted 22 lines · Namhyung Kim
Quoted 20 lines · 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 tracepointError: File
/sys/kernel/debug/tracing//tracing/events/sched/sched_kravanot found.
Hint: Perhaps this kernel misses someCONFIG_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 eventsError: No permissions to read
/sys/kernel/debug/tracing//tracing/events/sched/sched_krava
Hint: Try 'sudo mount -o remount,mode=755/sys/kernel/debug'
Quoted 6 lines
Quoted 3 lines
What about tracefs? On my system, tracefs is mounted on
/sys/kernel/debug/tracingthus I cannot access trace events after remounting debugfs with mode=755.right, patch below keeps the actual mount and
display proper info.. could you please try?
Sure, this patch displays the proper tracefs mountpoint. But it also has a problem - if tracefs is mounted under debugfs, the access mode of debugfs also affects, so in this case I had to change it both for debugfs and tracefs..
Quoted 7 lines
Quoted 4 lines
Also, IIRC tracepoint events adds
PERF_SAMPLE_RAWbit automatically, and it requiresperf_event_paranoidbeing -1 for non-root user, right?there's related error message when you try to open the tracepoint, the whole session is like:
Ah, great. :)
Thanks,
Namhyung
Quoted 70 lines
[jolsa@krava perf]$ ./perf record -e sched:sched_switch ls event syntax error: 'sched:sched_switch'
\___ can't access trace eventsError: No permissions to read /sys/kernel/debug/tracing/events/sched/sched_switch Hint: Try 'sudo mount -o remount,mode=755 /sys/kernel/debug/tracing'Run 'perf list' for a list of valid events
usage: perf record [<options>] [<command>]
or: perf record [<options>] -- <command> [<options>]-e, --event <event> event selector. use 'perf list' to list available events
[jolsa@krava perf]$ sudo mount -o remount,mode=755/sys/kernel/debug/tracing[jolsa@krava perf]$ ./perf record -e sched:sched_switch ls Error:
You may not have permission to collect stats.
Consider tweaking/proc/sys/kernel/perf_event_paranoid:
-1 - Not paranoid at all
0 - Disallow raw tracepoint access for unpriv
1 - Disallow cpu events for unpriv
2 - Disallow kernel profiling for unprivthanks,
jirka---
tools/lib/api/fs/tracing_path.c +3
-10@@ -12,12 +12,14 @@#include "tracing_path.h" +char tracing_mnt[PATH_MAX +1 ] ="/sys/kernel/debug" ;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_mnt,sizeof (tracing_mnt),"%s" , mountpoint);snprintf (tracing_path,sizeof (tracing_path),"%s/%s" , mountpoint, tracing); snprintf(tracing_events_path, sizeof(tracing_events_path), "%s/%s%s",@@ -109,19 +111,10 @@ static int strerror_open(int err, char *buf, size_t size, const char *filename) "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");break ;case EACCES : { -const char *mountpoint =debugfs__mountpoint (); - -if (!access (mountpoint,R_OK ) &&strncmp (filename,"tracing/" ,8 ) ==0 ) { -const char *tracefs_mntpoint =tracefs__mountpoint (); - -if (tracefs_mntpoint) - mountpoint =tracefs__mountpoint (); - } -snprintf (buf, size,"Error:\tNo permissions to read %s/%s\n" "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n" , - tracing_events_path, filename, mountpoint); + tracing_events_path, filename, tracing_mnt); }break ;default :
Quoted 26 lines · Namhyung Kim
Hi Jiri,
Quoted 23 lines · Jiri Olsa
Quoted 22 lines · Namhyung Kim
Quoted 20 lines · 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 tracepointError: File
/sys/kernel/debug/tracing//tracing/events/sched/sched_kravanot found.
Hint: Perhaps this kernel misses someCONFIG_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 eventsError: No permissions to read
/sys/kernel/debug/tracing//tracing/events/sched/sched_krava
Hint: Try 'sudo mount -o remount,mode=755/sys/kernel/debug'
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
What about tracefs? On my system, tracefs is mounted on
/sys/kernel/debug/tracingthus I cannot access trace events after remounting debugfs with mode=755.right, patch below keeps the actual mount and
display proper info.. could you please try?Sure, this patch displays the proper tracefs mountpoint. But it also
thanks
Quoted 3 lines
has a problem - if tracefs is mounted under debugfs, the access mode of debugfs also affects, so in this case I had to change it both for debugfs and tracefs..
hum, I wonder the error message needs to be that smart..
jirka
2015-09-11 12:16 GMT-04:00 Jiri Olsa <[email protected]>:
Quoted 1 line · Namhyung Kim
<SNIP>
Quoted 8 lines
Quoted 3 lines
has a problem - if tracefs is mounted under debugfs, the access mode of debugfs also affects, so in this case I had to change it both for debugfs and tracefs..
hum, I wonder the error message needs to be that smart..
jirka
Hmm... If tracefs is mounted under debugfs, wouldn't remounting debugfs do the trick, as it was done before?
If so, why couldn't we just check the paths with a basic strcmp to verify if tracefs starts by debugfs, and in that case offer to remount debugfs, else offer to remount tracefs?
Arnaldo Carvalho de Melo
maintainerEm Fri, Sep 11, 2015 at 01:50:02PM -0400, Raphaël Beamonte escreveu:
Quoted 11 lines
2015-09-11 12:16 GMT-04:00 Jiri Olsa <[email protected]>:
Quoted 1 line · Namhyung Kim
<SNIP>
Quoted 5 lines
Quoted 3 lines
has a problem - if tracefs is mounted under debugfs, the access mode of debugfs also affects, so in this case I had to change it both for debugfs and tracefs..
hum, I wonder the error message needs to be that smart..
Hmm... If tracefs is mounted under debugfs, wouldn't remounting debugfs do the trick, as it was done before?
Not necessarily, we may be able to access /a/ but not /a/b/, so, before we get to /a/b/ we need to solve access to /a/ to then realize that /a/b/ also need permission change so that we can access it.
Quoted 3 lines
If so, why couldn't we just check the paths with a basic strcmp to verify if tracefs starts by debugfs, and in that case offer to remount debugfs, else offer to remount tracefs?
say it is how it was before tracefs:
/sys/kernel/debug/tracing/
If we can't access "/sys/kernel/debug/tracing/" because we can't access "/sys/kernel/debug/" we need first to change (remount, chmod/grp/own, whatever is best in each hypotetical use case) /sys/kernel/debug/ to then do the same for /sys/kernel/debug/tracing/, no?
We could of course say something like "Something is wrong with tracefs and/or debugfs, figure it out and try again", but we can do better, right? 8-P
- Arnaldo
2015-09-11 14:55 GMT-04:00 Arnaldo Carvalho de Melo <[email protected]>:
Quoted 8 lines
Em Fri, Sep 11, 2015 at 01:50:02PM -0400, Raphaël Beamonte escreveu:
Quoted 6 lines
2015-09-11 12:16 GMT-04:00 Jiri Olsa <[email protected]>:
Quoted 1 line · Namhyung Kim
<SNIP>
Quoted 3 lines
Quoted 3 lines
has a problem - if tracefs is mounted under debugfs, the access mode of debugfs also affects, so in this case I had to change it both for debugfs and tracefs..
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
hum, I wonder the error message needs to be that smart..
Hmm... If tracefs is mounted under debugfs, wouldn't remounting debugfs do the trick, as it was done before?
Not necessarily, we may be able to access /a/ but not /a/b/, so, before we get to /a/b/ we need to solve access to /a/ to then realize that /a/b/ also need permission change so that we can access it.
Well, I kind of had in mind that if we can access /a/, it's that /a/ is not the problem, so why would we remount it? Remounting /a/b/ should do the trick, and there's no reason we couldn't do it if we can access /a/. Or perhaps I'm missing something?
Quoted 4 lines
Quoted 3 lines
If so, why couldn't we just check the paths with a basic strcmp to verify if tracefs starts by debugfs, and in that case offer to remount debugfs, else offer to remount tracefs?
Quoted 8 lines
say it is how it was before tracefs:
/sys/kernel/debug/tracing/If we can't access "
/sys/kernel/debug/tracing/" because we can't access "/sys/kernel/debug/" we need first to change (remount, chmod/grp/own, whatever is best in each hypotetical use case)/sys/kernel/debug/to then do the same for/sys/kernel/debug/tracing/, no?
In that case, I'm following: if we can't access /sys/kernel/debug, we have to remount it anyway, and there's no guarantee that /sys/kernel/debug/tracing will have the right permissions. In that case, ok, we perhaps need to remount both. Perhaps because: can we be sure that if /sys/kernel/debug is inaccessible, it will be the same for /sys/kernel/debug/tracing ?
Quoted 5 lines
We could of course say something like "Something is wrong with tracefs and/or debugfs, figure it out and try again", but we can do better, right? 8-P
- Arnaldo
I agree! We were talking in a previous conversation about making perf the user-friendliest possible. But I don't know if providing two different remount to the user at the same time is the best thing to do (even in that second case).
Why not checking first access to debugfs: if it doesn't work, give the remount line for debugfs then exit. The user will do it then run perf again. If it works, check tracefs: if it doesn't work, give the remount line for tracefs. That gives at most two fail-runs of perf before using it without any problem. It also allows not to give the user two different remount lines directly when we can't be sure the second one is useful (perhaps tracefs will be accessible directly?) Another possibility would be to have another perf command, sort of a "perf remountfs", to run with sudo and that would make itself both checks and remount accordingly the two fs.
Thoughts?
Em Fri, Sep 11, 2015 at 03:56:44PM -0400, Raphaël Beamonte escreveu:
Quoted 3 lines
Another possibility would be to have another perf command, sort of a "perf remountfs", to run with sudo and that would make itself both checks and remount accordingly the two fs.
This part maybe more interesting, so I'll focus on it, yeah, having a:
'perf fixperms' command (have a better name? :) ) may make sense, would need context tho, i.e. something like:
perf fixperms trace
Or:
perf fixperms top
But then, perhaps in that case:
$ trace ls Error: No permissions to read /sys/kernel/debug/tracing/events/raw_syscalls/sys_(enter|exit) Hint: Try 'sudo mount -o remount,mode=755 /sys/kernel/debug'
We could just add some more text saying that please check as well that the other parts of the path we're trying to access are available once the suggestion is followed.
- Arnaldo
2015-09-11 16:22 GMT-04:00 Arnaldo Carvalho de Melo <[email protected]>:
Quoted 17 lines
Em Fri, Sep 11, 2015 at 03:56:44PM -0400, Raphaël Beamonte escreveu:
Quoted 3 lines
Another possibility would be to have another perf command, sort of a "perf remountfs", to run with sudo and that would make itself both checks and remount accordingly the two fs.
This part maybe more interesting, so I'll focus on it, yeah, having a:
'perf fixperms' command (have a better name? :) ) may make sense, would need context tho, i.e. something like:perf fixperms trace
Or:
perf fixperms top
But then, perhaps in that case:
Quoted 8 lines
$ trace ls Error: No permissions to read /sys/kernel/debug/tracing/events/raw_syscalls/sys_(enter|exit) Hint: Try 'sudo mount -o remount,mode=755 /sys/kernel/debug'We could just add some more text saying that please check as well that the other parts of the path we're trying to access are available once the suggestion is followed.
Wouldn't it be almost the same as your "Something is wrong with tracefs and/or debugfs, figure it out and try again" ? ;o) I think that if the 'fixperms' need a context, we could as well give the 'perf fixperms' command to type directly in the Hint, such as:
$ trace ls Error: No permissions to read /sys/kernel/debug/tracing/events/raw_syscalls/sys_(enter|exit) Hint: Try 'sudo perf fixperms trace'
It would be the same as the current copy/paste, but one command that would fix the situation directly, instead of having to check the permissions of each level in the path. A little bit user-friendlier!
Arnaldo Carvalho de Melo
maintainerEm Thu, Sep 10, 2015 at 10:05:37AM +0200, Jiri Olsa escreveu:
Quoted 23 lines · Namhyung Kim
Quoted 22 lines · Jiri Olsa
Quoted 20 lines
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 tracepointError: File
/sys/kernel/debug/tracing//tracing/events/sched/sched_kravanot found.
Hint: Perhaps this kernel misses someCONFIG_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 eventsError: No permissions to read
/sys/kernel/debug/tracing//tracing/events/sched/sched_krava
Hint: Try 'sudo mount -o remount,mode=755/sys/kernel/debug'
Quoted 6 lines
Quoted 3 lines
What about tracefs? On my system, tracefs is mounted on
/sys/kernel/debug/tracingthus I cannot access trace events after remounting debugfs with mode=755.right, patch below keeps the actual mount and
display proper info.. could you please try?
I'll apply up to 5/5, will wait for this one to be resubmitted with S-o-B, ok?
- Arnaldo
Quoted 78 lines
Quoted 3 lines
Also, IIRC tracepoint events adds
PERF_SAMPLE_RAWbit automatically, and it requiresperf_event_paranoidbeing -1 for non-root user, right?there's related error message when you try to open the tracepoint, the whole session is like:
[jolsa@krava perf]$ ./perf record -e sched:sched_switch ls event syntax error: 'sched:sched_switch'
\___ can't access trace eventsError: No permissions to read /sys/kernel/debug/tracing/events/sched/sched_switch Hint: Try 'sudo mount -o remount,mode=755 /sys/kernel/debug/tracing'Run 'perf list' for a list of valid events
usage: perf record [<options>] [<command>]
or: perf record [<options>] -- <command> [<options>]-e, --event <event> event selector. use 'perf list' to list available events
[jolsa@krava perf]$ sudo mount -o remount,mode=755/sys/kernel/debug/tracing[jolsa@krava perf]$ ./perf record -e sched:sched_switch ls Error:
You may not have permission to collect stats.
Consider tweaking/proc/sys/kernel/perf_event_paranoid:
-1 - Not paranoid at all
0 - Disallow raw tracepoint access for unpriv
1 - Disallow cpu events for unpriv
2 - Disallow kernel profiling for unprivthanks,
jirka---
tools/lib/api/fs/tracing_path.c +3
-10@@ -12,12 +12,14 @@#include "tracing_path.h" +char tracing_mnt[PATH_MAX +1 ] ="/sys/kernel/debug" ;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_mnt,sizeof (tracing_mnt),"%s" , mountpoint);snprintf (tracing_path,sizeof (tracing_path),"%s/%s" , mountpoint, tracing); snprintf(tracing_events_path, sizeof(tracing_events_path), "%s/%s%s",@@ -109,19 +111,10 @@ static int strerror_open(int err, char *buf, size_t size, const char *filename) "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");break ;case EACCES : { -const char *mountpoint =debugfs__mountpoint (); - -if (!access (mountpoint,R_OK ) &&strncmp (filename,"tracing/" ,8 ) ==0 ) { -const char *tracefs_mntpoint =tracefs__mountpoint (); - -if (tracefs_mntpoint) - mountpoint =tracefs__mountpoint (); - } -snprintf (buf, size,"Error:\tNo permissions to read %s/%s\n" "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n" , - tracing_events_path, filename, mountpoint); + tracing_events_path, filename, tracing_mnt); }break ;default :
tip-bot for Jiri Olsa
merge botperf tools: Enhance parsing events tracepoint error output
Commit-ID: 196581717d85f59365dc9303685cd5b1cdf106a3 Gitweb: http://git.kernel.org/tip/196581717d85f59365dc9303685cd5b1cdf106a3 Author: Jiri Olsa <[email protected]> AuthorDate: Mon, 7 Sep 2015 10:38:07 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 15 Sep 2015 09:48:33 -0300
perf tools: Enhance parsing events tracepoint error output
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
...
--- tools/perf/util/parse-events.c | 35 ++++++++++++++++++++++++++++++++--- tools/perf/util/parse-events.y | 16 +++++++++------- 2 files changed, 41 insertions(+), 10 deletions(-)
tools/perf/util/parse-events.c +32 -3
@@ -387,6 +387,33 @@ 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 ]; + +/* +* We get error directly from syscall errno ( > 0), +* or from encoded pointer's error ( < 0). +*/ + 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 +421,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 PTR_ERR (evsel); + }list_add_tail (&evsel->node, list);return 0 ;
@@ -413,7 +442,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 ; }
@@ -453,7 +482,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 ; }
tools/perf/util/parse-events.y +9 -7
@@ -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;