[PATCHv2 0/5] perf tools: Enhance parsing events tracepoint error output

[PATCHv2 0/5] perf tools: Enhance parsing events tracepoint error output

[Jiri Olsa]

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

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

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

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

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

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

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

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

[PATCH 1/5] tools: Add err.h with ERR_PTR PTR_ERR interface

[Jiri Olsa]

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

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

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

diff --git a/tools/include/linux/err.h b/tools/include/linux/err.h
new file mode 100644
index 000000000000..c9ada48f5156
--- /dev/null
+++ b/tools/include/linux/err.h
@@ -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

Re: [PATCH 1/5] tools: Add err.h with ERR_PTR PTR_ERR interface

[Raphaël Beamonte]

2015-09-07 4:38 GMT-04:00 Jiri Olsa <jolsa@kernel.org>:
> Adding part of the kernel's <linux/err.h> interface:
>   inline void * __must_check ERR_PTR(long error);
>   inline long   __must_check PTR_ERR(__force const void *ptr);
>   inline bool   __must_check IS_ERR(__force const void *ptr);
>
> it will be used to propagate error through pointers
> in following patches.
>
> Link: http://lkml.kernel.org/n/tip-ufgnyf683uab69anmmrabgdf@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 tools/include/linux/err.h
>
> diff --git a/tools/include/linux/err.h b/tools/include/linux/err.h
> new file mode 100644
> index 000000000000..c9ada48f5156
> --- /dev/null
> +++ b/tools/include/linux/err.h
> @@ -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

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

[Raphaël Beamonte]

2015-09-08 16:22 GMT-04:00 Raphaël Beamonte <raphael.beamonte@gmail.com>:
> 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?

I meant include/linux/err.h, from git root

Re: [PATCH 1/5] tools: Add err.h with ERR_PTR PTR_ERR interface

[Arnaldo Carvalho de Melo]

Em Tue, Sep 08, 2015 at 04:22:39PM -0400, Raphaël Beamonte escreveu:
> 2015-09-07 4:38 GMT-04:00 Jiri Olsa <jolsa@kernel.org>:
> > Adding part of the kernel's <linux/err.h> interface:
> >   inline void * __must_check ERR_PTR(long error);
> >   inline long   __must_check PTR_ERR(__force const void *ptr);
> >   inline bool   __must_check IS_ERR(__force const void *ptr);
> >
> > it will be used to propagate error through pointers
> > in following patches.
> >
> > Link: http://lkml.kernel.org/n/tip-ufgnyf683uab69anmmrabgdf@git.kernel.org
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 tools/include/linux/err.h
> >
> > diff --git a/tools/include/linux/err.h b/tools/include/linux/err.h
> > new file mode 100644
> > index 000000000000..c9ada48f5156
> > --- /dev/null
> > +++ b/tools/include/linux/err.h
> > @@ -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
> 
> 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?

We can't do that.

Read:

commit 3f735377bfd6567d80815a6242c147211963680a
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
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

[Raphaël Beamonte]

2015-09-08 17:06 GMT-04:00 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>:
> Em Tue, Sep 08, 2015 at 04:22:39PM -0400, Raphaël Beamonte escreveu:
>> 2015-09-07 4:38 GMT-04:00 Jiri Olsa <jolsa@kernel.org>:
>> > Adding part of the kernel's <linux/err.h> interface:
>> >   inline void * __must_check ERR_PTR(long error);
>> >   inline long   __must_check PTR_ERR(__force const void *ptr);
>> >   inline bool   __must_check IS_ERR(__force const void *ptr);
>> >
>> > it will be used to propagate error through pointers
>> > in following patches.
>> >
>> > Link: http://lkml.kernel.org/n/tip-ufgnyf683uab69anmmrabgdf@git.kernel.org
>> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> > ---
>> >  tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 49 insertions(+)
>> >  create mode 100644 tools/include/linux/err.h
>> >
>> > diff --git a/tools/include/linux/err.h b/tools/include/linux/err.h
>> > new file mode 100644
>> > index 000000000000..c9ada48f5156
>> > --- /dev/null
>> > +++ b/tools/include/linux/err.h
>> > @@ -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
>>
>> 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?
>
> We can't do that.
>
> Read:
>
> commit 3f735377bfd6567d80815a6242c147211963680a
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> 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?

Yes, I understand now! Thanks!

>
>
> - Arnaldo

Re: [PATCH 1/5] tools: Add err.h with ERR_PTR PTR_ERR interface

[Raphaël Beamonte]

2015-09-07 4:38 GMT-04:00 Jiri Olsa <jolsa@kernel.org>:
> Adding part of the kernel's <linux/err.h> interface:
>   inline void * __must_check ERR_PTR(long error);
>   inline long   __must_check PTR_ERR(__force const void *ptr);
>   inline bool   __must_check IS_ERR(__force const void *ptr);
>
> it will be used to propagate error through pointers
> in following patches.
>
> Link: http://lkml.kernel.org/n/tip-ufgnyf683uab69anmmrabgdf@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 tools/include/linux/err.h

Reviewed-by: Raphaël Beamonte <raphael.beamonte@gmail.com>

>
> diff --git a/tools/include/linux/err.h b/tools/include/linux/err.h
> new file mode 100644
> index 000000000000..c9ada48f5156
> --- /dev/null
> +++ b/tools/include/linux/err.h
> @@ -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:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface

[tip-bot for Jiri Olsa]

Commit-ID:  01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7
Gitweb:     http://git.kernel.org/tip/01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Mon, 7 Sep 2015 10:38:03 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Reviewed-by: Raphael Beamonte <raphael.beamonte@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1441615087-13886-2-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/include/linux/err.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/tools/include/linux/err.h b/tools/include/linux/err.h
new file mode 100644
index 0000000..c9ada48
--- /dev/null
+++ b/tools/include/linux/err.h
@@ -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

[Vinson Lee]

On Wed, Sep 16, 2015 at 12:28 AM, tip-bot for Jiri Olsa
<tipbot@zytor.com> wrote:
> Commit-ID:  01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7
> Gitweb:     http://git.kernel.org/tip/01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7
> Author:     Jiri Olsa <jolsa@kernel.org>
> AuthorDate: Mon, 7 Sep 2015 10:38:03 +0200
> Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
> 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.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> Reviewed-by: Raphael Beamonte <raphael.beamonte@gmail.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Link: http://lkml.kernel.org/r/1441615087-13886-2-git-send-email-jolsa@kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>


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 declaration
util/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

[Vinson Lee]

On Mon, Sep 21, 2015 at 4:41 PM, Vinson Lee <vlee@twopensource.com> wrote:
> On Wed, Sep 16, 2015 at 12:28 AM, tip-bot for Jiri Olsa
> <tipbot@zytor.com> wrote:
>> Commit-ID:  01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7
>> Gitweb:     http://git.kernel.org/tip/01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7
>> Author:     Jiri Olsa <jolsa@kernel.org>
>> AuthorDate: Mon, 7 Sep 2015 10:38:03 +0200
>> Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
>> 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.
>>
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> Reviewed-by: Raphael Beamonte <raphael.beamonte@gmail.com>
>> Cc: David Ahern <dsahern@gmail.com>
>> Cc: Matt Fleming <matt@codeblueprint.co.uk>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> Link: http://lkml.kernel.org/r/1441615087-13886-2-git-send-email-jolsa@kernel.org
>> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
>
> 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 declaration
> util/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

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

[Jiri Olsa]

On Mon, Sep 28, 2015 at 11:35:45PM -0700, Vinson Lee wrote:
> On Mon, Sep 21, 2015 at 4:41 PM, Vinson Lee <vlee@twopensource.com> wrote:
> > On Wed, Sep 16, 2015 at 12:28 AM, tip-bot for Jiri Olsa
> > <tipbot@zytor.com> wrote:
> >> Commit-ID:  01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7
> >> Gitweb:     http://git.kernel.org/tip/01ca9fd41d6f2ad796a6b109b5253e06b6ae6dc7
> >> Author:     Jiri Olsa <jolsa@kernel.org>
> >> AuthorDate: Mon, 7 Sep 2015 10:38:03 +0200
> >> Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
> >> 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.
> >>
> >> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >> Reviewed-by: Raphael Beamonte <raphael.beamonte@gmail.com>
> >> Cc: David Ahern <dsahern@gmail.com>
> >> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> >> Cc: Namhyung Kim <namhyung@kernel.org>
> >> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> >> Link: http://lkml.kernel.org/r/1441615087-13886-2-git-send-email-jolsa@kernel.org
> >> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> >
> >
> > 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 declaration
> > util/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
> 
> 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.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.

Reported-by: Vinson Lee <vlee@twopensource.com>
Link: http://lkml.kernel.org/n/tip-i9mdgdbrgauy3fe76s9rd125@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/include/linux/err.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/include/linux/err.h b/tools/include/linux/err.h
index c9ada48f5156..1156cd20e0b7 100644
--- a/tools/include/linux/err.h
+++ b/tools/include/linux/err.h
@@ -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)
-- 
2.4.3

Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface

[Jiri Olsa]

On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:

SNIP

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

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.

Reported-by: Vinson Lee <vlee@twopensource.com>
Link: http://lkml.kernel.org/n/tip-ujp5f63wvy70jlzeh3rt5f98@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/parse-events.c |   38 +++++++++++++++++++-------------------
 1 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 61c2bc2..626bf85 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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
-- 
1.7.1

Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface

[He Kuang]

hi, jirka

On 2015/9/29 15:20, Jiri Olsa wrote:
> On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
>
> SNIP
>
>>>> $ 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.
>>
>> 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
>
> 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?

> 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.
>
> Reported-by: Vinson Lee <vlee@twopensource.com>
> Link: http://lkml.kernel.org/n/tip-ujp5f63wvy70jlzeh3rt5f98@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   tools/perf/util/parse-events.c |   38 +++++++++++++++++++-------------------
>   1 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 61c2bc2..626bf85 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -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

[Jiri Olsa]

On Tue, Sep 29, 2015 at 03:52:09PM +0800, He Kuang wrote:
> hi, jirka
> 
> On 2015/9/29 15:20, Jiri Olsa wrote:
> >On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
> >
> >SNIP
> >
> >>>>$ 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.
> >>
> >>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
> >
> >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

[He Kuang]

On 2015/9/29 15:57, Jiri Olsa wrote:
> On Tue, Sep 29, 2015 at 03:52:09PM +0800, He Kuang wrote:
>> hi, jirka
>>
>> On 2015/9/29 15:20, Jiri Olsa wrote:
>>> On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
>>>
>>> SNIP
>>>
>>>>>> $ 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.
>>>>
>>>> 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
>>>
>>> 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
>

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.

> thanks,
> jirka
>

Re: [tip:perf/core] tools: Add err.h with ERR_PTR PTR_ERR interface

[Jiri Olsa]

On Tue, Sep 29, 2015 at 04:15:39PM +0800, He Kuang wrote:
> 
> 
> On 2015/9/29 15:57, Jiri Olsa wrote:
> >On Tue, Sep 29, 2015 at 03:52:09PM +0800, He Kuang wrote:
> >>hi, jirka
> >>
> >>On 2015/9/29 15:20, Jiri Olsa wrote:
> >>>On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
> >>>
> >>>SNIP
> >>>
> >>>>>>$ 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.
> >>>>
> >>>>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
> >>>
> >>>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
> >
> 
> 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

[Arnaldo Carvalho de Melo]

Em Tue, Sep 29, 2015 at 12:41:47PM +0200, Jiri Olsa escreveu:
> On Tue, Sep 29, 2015 at 04:15:39PM +0800, He Kuang wrote:
> > 
> > 
> > On 2015/9/29 15:57, Jiri Olsa wrote:
> > >On Tue, Sep 29, 2015 at 03:52:09PM +0800, He Kuang wrote:
> > >>hi, jirka
> > >>
> > >>On 2015/9/29 15:20, Jiri Olsa wrote:
> > >>>On Tue, Sep 29, 2015 at 09:14:10AM +0200, Jiri Olsa wrote:
> > >>>
> > >>>SNIP
> > >>>
> > >>>>>>$ 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.
> > >>>>
> > >>>>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
> > >>>
> > >>>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
> > >
> > 
> > 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] perf tool: Fix shadowed declaration in parse-events.c

[Jiri Olsa]

On Tue, Sep 29, 2015 at 11:52:59AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

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

Reported-by: Vinson Lee <vlee@twopensource.com>
Link: http://lkml.kernel.org/n/tip-i9mdgdbrgauy3fe76s9rd125@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/parse-events.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5ffb356cbcc6..c01a44b18faf 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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);
-- 
2.4.3

[tip:perf/core] perf tools: Fix shadowed declaration in parse-events.c

[tip-bot for Jiri Olsa]

Commit-ID:  272ed29a91aea4397c05157dea7210dd7e81557a
Gitweb:     http://git.kernel.org/tip/272ed29a91aea4397c05157dea7210dd7e81557a
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Tue, 29 Sep 2015 17:05:31 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Reported-by: Vinson Lee <vlee@twopensource.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: linux-tip-commits@vger.kernel.org
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Raphael Beamonte <raphael.beamonte@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20150929150531.GI27383@krava.redhat.com
[ Fix one more case, at line 770 ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-events.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5ffb356..c1c64fb 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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,

[PATCH 2/5] perf tools: Add tools/include into tags directories

[Jiri Olsa]

Adding tools/include into tags directories, to have include
definitions reachable via tags/cscope.

Link: http://lkml.kernel.org/n/tip-jihvx6xbdoz1xqtxce7cuuja@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/Makefile.perf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index d9863cb96f59..6c5c699002cb 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -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:
-- 
2.4.3

[tip:perf/core] perf tools: Add tools/ include into tags directories

[tip-bot for Jiri Olsa]

Commit-ID:  b60114067c656be572f0ca4030c3a2c560e7255c
Gitweb:     http://git.kernel.org/tip/b60114067c656be572f0ca4030c3a2c560e7255c
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Mon, 7 Sep 2015 10:38:04 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

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

diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index d9863cb..6c5c699 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -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:

[PATCH 3/5] perf tools: Propagate error info for the tracepoint parsing

[Jiri Olsa]

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

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

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

Re: [PATCH 3/5] perf tools: Propagate error info for the tracepoint parsing

[Raphaël Beamonte]

2015-09-07 4:38 GMT-04:00 Jiri Olsa <jolsa@kernel.org>:
> Pass 'struct parse_events_error *error' to the parse-event.c
> tracepoint adding path. It will be filled with error data
> in following patches.
>
> Link: http://lkml.kernel.org/n/tip-las1hm5zf58b0twd27h9895b@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/parse-events.c | 27 ++++++++++++++++-----------
>  tools/perf/util/parse-events.h |  3 ++-
>  tools/perf/util/parse-events.y |  4 ++--
>  3 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 3840176642f8..1b284b8ad243 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -387,7 +387,8 @@ int parse_events_add_cache(struct list_head *list, int *idx,
>  }
>
>  static int add_tracepoint(struct list_head *list, int *idx,
> -                         char *sys_name, char *evt_name)
> +                         char *sys_name, char *evt_name,
> +                         struct parse_events_error *error __maybe_unused)
>  {
>         struct perf_evsel *evsel;
>
> @@ -401,7 +402,8 @@ static int add_tracepoint(struct list_head *list, int *idx,
>  }
>
>  static int add_tracepoint_multi_event(struct list_head *list, int *idx,
> -                                     char *sys_name, char *evt_name)
> +                                     char *sys_name, char *evt_name,
> +                                     struct parse_events_error *error)
>  {
>         char evt_path[MAXPATHLEN];
>         struct dirent *evt_ent;
> @@ -425,7 +427,7 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
>                 if (!strglobmatch(evt_ent->d_name, evt_name))
>                         continue;
>
> -               ret = add_tracepoint(list, idx, sys_name, evt_ent->d_name);
> +               ret = add_tracepoint(list, idx, sys_name, evt_ent->d_name, error);
>         }
>
>         closedir(evt_dir);
> @@ -433,15 +435,17 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
>  }
>
>  static int add_tracepoint_event(struct list_head *list, int *idx,
> -                               char *sys_name, char *evt_name)
> +                               char *sys_name, char *evt_name,
> +                               struct parse_events_error *error)
>  {
>         return strpbrk(evt_name, "*?") ?
> -              add_tracepoint_multi_event(list, idx, sys_name, evt_name) :
> -              add_tracepoint(list, idx, sys_name, evt_name);
> +              add_tracepoint_multi_event(list, idx, sys_name, evt_name, error) :
> +              add_tracepoint(list, idx, sys_name, evt_name, error);
>  }
>
>  static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
> -                                   char *sys_name, char *evt_name)
> +                                   char *sys_name, char *evt_name,
> +                                   struct parse_events_error *error)
>  {
>         struct dirent *events_ent;
>         DIR *events_dir;
> @@ -465,7 +469,7 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
>                         continue;
>
>                 ret = add_tracepoint_event(list, idx, events_ent->d_name,
> -                                          evt_name);
> +                                          evt_name, error);
>         }
>
>         closedir(events_dir);
> @@ -473,12 +477,13 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
>  }
>
>  int parse_events_add_tracepoint(struct list_head *list, int *idx,
> -                               char *sys, char *event)
> +                               char *sys, char *event,
> +                               struct parse_events_error *error)
>  {
>         if (strpbrk(sys, "*?"))
> -               return add_tracepoint_multi_sys(list, idx, sys, event);
> +               return add_tracepoint_multi_sys(list, idx, sys, event, error);
>         else
> -               return add_tracepoint_event(list, idx, sys, event);
> +               return add_tracepoint_event(list, idx, sys, event, error);
>  }
>
>  static int
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index a09b0e210997..ffee7ece75a6 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -118,7 +118,8 @@ int parse_events__modifier_event(struct list_head *list, char *str, bool add);
>  int parse_events__modifier_group(struct list_head *list, char *event_mod);
>  int parse_events_name(struct list_head *list, char *name);
>  int parse_events_add_tracepoint(struct list_head *list, int *idx,
> -                               char *sys, char *event);
> +                               char *sys, char *event,
> +                               struct parse_events_error *error);
>  int parse_events_add_numeric(struct parse_events_evlist *data,
>                              struct list_head *list,
>                              u32 type, u64 config,
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 9cd70819c795..54a3004a8192 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -376,7 +376,7 @@ PE_NAME '-' PE_NAME ':' PE_NAME
>         snprintf(&sys_name, 128, "%s-%s", $1, $3);
>
>         ALLOC_LIST(list);
> -       ABORT_ON(parse_events_add_tracepoint(list, &data->idx, &sys_name, $5));
> +       ABORT_ON(parse_events_add_tracepoint(list, &data->idx, &sys_name, $5, data->error));
>         $$ = list;
>  }
>  |
> @@ -386,7 +386,7 @@ PE_NAME ':' PE_NAME
>         struct list_head *list;
>
>         ALLOC_LIST(list);
> -       if (parse_events_add_tracepoint(list, &data->idx, $1, $3)) {
> +       if (parse_events_add_tracepoint(list, &data->idx, $1, $3, data->error)) {
>                 struct parse_events_error *error = data->error;
>
>                 if (error) {
> --
> 2.4.3
>

Works for me.
Reviewed-by: Raphaël Beamonte <raphael.beamonte@gmail.com>

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

[Jiri Olsa]

On Tue, Sep 08, 2015 at 05:42:25PM -0400, Raphaël Beamonte wrote:

SNIP

> > @@ -386,7 +386,7 @@ PE_NAME ':' PE_NAME
> >         struct list_head *list;
> >
> >         ALLOC_LIST(list);
> > -       if (parse_events_add_tracepoint(list, &data->idx, $1, $3)) {
> > +       if (parse_events_add_tracepoint(list, &data->idx, $1, $3, data->error)) {
> >                 struct parse_events_error *error = data->error;
> >
> >                 if (error) {
> > --
> > 2.4.3
> >
> 
> Works for me.
> Reviewed-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> 
> 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

[Matt Fleming]

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

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

-- 
Matt Fleming, Intel Open Source Technology Center

[tip:perf/core] perf tools: Propagate error info for the tracepoint parsing

[tip-bot for Jiri Olsa]

Commit-ID:  e2f9f8ea6a54e252e3a94a5c2321f673b5b97360
Gitweb:     http://git.kernel.org/tip/e2f9f8ea6a54e252e3a94a5c2321f673b5b97360
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Mon, 7 Sep 2015 10:38:05 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

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

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

[PATCH 4/5] perf tools: Propagate error info from tp_format

[Jiri Olsa]

Propagate error info from tp_format via ERR_PTR to get
it all the way down to the parse-event.c tracepoint adding
routines. 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.

Link: http://lkml.kernel.org/n/tip-bzdckgv1zfp2y8up9l7ojt7y@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 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(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 215653274102..93b80f12f35e 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -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, void
 	struct 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) {
diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
index 52162425c969..790e413d9a1f 100644
--- a/tools/perf/tests/evsel-tp-sched.c
+++ b/tools/perf/tests/evsel-tp-sched.c
@@ -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;
 
diff --git a/tools/perf/tests/openat-syscall-all-cpus.c b/tools/perf/tests/openat-syscall-all-cpus.c
index 495d8126b722..9e104a2e973d 100644
--- a/tools/perf/tests/openat-syscall-all-cpus.c
+++ b/tools/perf/tests/openat-syscall-all-cpus.c
@@ -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;
diff --git a/tools/perf/tests/openat-syscall-tp-fields.c b/tools/perf/tests/openat-syscall-tp-fields.c
index 01a19626c846..473d3869727e 100644
--- a/tools/perf/tests/openat-syscall-tp-fields.c
+++ b/tools/perf/tests/openat-syscall-tp-fields.c
@@ -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;
 	}
diff --git a/tools/perf/tests/openat-syscall.c b/tools/perf/tests/openat-syscall.c
index 08ac9d94a050..7b1db8306098 100644
--- a/tools/perf/tests/openat-syscall.c
+++ b/tools/perf/tests/openat-syscall.c
@@ -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;
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d51a5200c8af..3cb2bf9bd4bd 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -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;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 771ade4d5966..08c20ee4e27d 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -13,6 +13,7 @@
 #include <traceevent/event-parse.h>
 #include <linux/hw_breakpoint.h>
 #include <linux/perf_event.h>
+#include <linux/err.h>
 #include <sys/resource.h>
 #include "asm/bug.h"
 #include "callchain.h"
@@ -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] = {
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 298e6bbca200..b6e8ff876f17 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -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);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1b284b8ad243..c47831c47220 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1,4 +1,5 @@
 #include <linux/hw_breakpoint.h>
+#include <linux/err.h>
 #include "util.h"
 #include "../perf.h"
 #include "evlist.h"
@@ -393,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;
 }
 
diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index 2f4996ab313d..8e3a60e3e15f 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -7,6 +7,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <traceevent/event-parse.h>
 #include <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)
 {
-- 
2.4.3

Re: [PATCH 4/5] perf tools: Propagate error info from tp_format

[Arnaldo Carvalho de Melo]

Em Mon, Sep 07, 2015 at 10:38:06AM +0200, Jiri Olsa escreveu:
> 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.

Right, so this is tricky and we must be careful, see below...
 
> Link: http://lkml.kernel.org/n/tip-bzdckgv1zfp2y8up9l7ojt7y@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  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(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 215653274102..93b80f12f35e 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -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, void
>  	struct 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;

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);
}

>  
>  	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) {
> diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
> index 52162425c969..790e413d9a1f 100644
> --- a/tools/perf/tests/evsel-tp-sched.c
> +++ b/tools/perf/tests/evsel-tp-sched.c
> @@ -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;
>  
> diff --git a/tools/perf/tests/openat-syscall-all-cpus.c b/tools/perf/tests/openat-syscall-all-cpus.c
> index 495d8126b722..9e104a2e973d 100644
> --- a/tools/perf/tests/openat-syscall-all-cpus.c
> +++ b/tools/perf/tests/openat-syscall-all-cpus.c
> @@ -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;
> diff --git a/tools/perf/tests/openat-syscall-tp-fields.c b/tools/perf/tests/openat-syscall-tp-fields.c
> index 01a19626c846..473d3869727e 100644
> --- a/tools/perf/tests/openat-syscall-tp-fields.c
> +++ b/tools/perf/tests/openat-syscall-tp-fields.c
> @@ -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;
>  	}
> diff --git a/tools/perf/tests/openat-syscall.c b/tools/perf/tests/openat-syscall.c
> index 08ac9d94a050..7b1db8306098 100644
> --- a/tools/perf/tests/openat-syscall.c
> +++ b/tools/perf/tests/openat-syscall.c
> @@ -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;
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index d51a5200c8af..3cb2bf9bd4bd 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -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;
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 771ade4d5966..08c20ee4e27d 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -13,6 +13,7 @@
>  #include <traceevent/event-parse.h>
>  #include <linux/hw_breakpoint.h>
>  #include <linux/perf_event.h>
> +#include <linux/err.h>
>  #include <sys/resource.h>
>  #include "asm/bug.h"
>  #include "callchain.h"
> @@ -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] = {
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 298e6bbca200..b6e8ff876f17 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -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);
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 1b284b8ad243..c47831c47220 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1,4 +1,5 @@
>  #include <linux/hw_breakpoint.h>
> +#include <linux/err.h>
>  #include "util.h"
>  #include "../perf.h"
>  #include "evlist.h"
> @@ -393,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;
>  }
>  
> diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
> index 2f4996ab313d..8e3a60e3e15f 100644
> --- a/tools/perf/util/trace-event.c
> +++ b/tools/perf/util/trace-event.c
> @@ -7,6 +7,7 @@
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  #include <linux/kernel.h>
> +#include <linux/err.h>
>  #include <traceevent/event-parse.h>
>  #include <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)
>  {
> -- 
> 2.4.3

Re: [PATCH 4/5] perf tools: Propagate error info from tp_format

[Jiri Olsa]

On Wed, Sep 09, 2015 at 05:58:13PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

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

hum, not sure.. will check ;-)

at least we could mark related functions with __must_check
to force the return value check

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

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


---
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 08c20ee4e27d..162973bec713 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -256,7 +256,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int
 		perf_evsel__init(evsel, &attr, idx);
 	}
 
-	return evsel;
+	return evsel ?: ERR_PTR(err);
 
 out_free:
 	zfree(&evsel->name);

Re: [PATCH 4/5] perf tools: Propagate error info from tp_format

[Arnaldo Carvalho de Melo]

Em Thu, Sep 10, 2015 at 10:24:52AM +0200, Jiri Olsa escreveu:
> On Wed, Sep 09, 2015 at 05:58:13PM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > 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?
> 
> hum, not sure.. will check ;-)
> 
> at least we could mark related functions with __must_check
> to 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
 
> > 
> > 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?
> 
> 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
> 
> 
> ---
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 08c20ee4e27d..162973bec713 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -256,7 +256,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int
>  		perf_evsel__init(evsel, &attr, idx);
>  	}
>  
> -	return evsel;
> +	return evsel ?: ERR_PTR(err);
>  
>  out_free:
>  	zfree(&evsel->name);

Re: [PATCH 4/5] perf tools: Propagate error info from tp_format

[Arnaldo Carvalho de Melo]

Em Thu, Sep 10, 2015 at 10:24:52AM +0200, Jiri Olsa escreveu:
> On Wed, Sep 09, 2015 at 05:58:13PM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > 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?
> 
> hum, not sure.. will check ;-)
> 
> at least we could mark related functions with __must_check
> to force the return value check
> 
> > 
> > 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?
> 
> 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

diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
index 666b67a4df9d..4bb0c5d2059d 100644
--- a/tools/perf/tests/mmap-basic.c
+++ b/tools/perf/tests/mmap-basic.c
@@ -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;
 		}
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 08c20ee4e27d..6b5d1b509148 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int
 	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 |
@@ -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);
 }
 
diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index 8e3a60e3e15f..802bb868d446 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -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

[Raphaël Beamonte]

2015-09-14 16:53 GMT-04:00 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>:
> Em Thu, Sep 10, 2015 at 10:24:52AM +0200, Jiri Olsa escreveu:
>> On Wed, Sep 09, 2015 at 05:58:13PM -0300, Arnaldo Carvalho de Melo wrote:
>>
>> SNIP
>>
>> > 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?
>>
>> hum, not sure.. will check ;-)
>>
>> at least we could mark related functions with __must_check
>> to force the return value check
>>
>> >
>> > 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?
>>
>> 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
>
> diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
> index 666b67a4df9d..4bb0c5d2059d 100644
> --- a/tools/perf/tests/mmap-basic.c
> +++ b/tools/perf/tests/mmap-basic.c
> @@ -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;
>                 }
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 08c20ee4e27d..6b5d1b509148 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int
>         struct 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?

>                 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);
>  }
>
> diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
> index 8e3a60e3e15f..802bb868d446 100644
> --- a/tools/perf/util/trace-event.c
> +++ b/tools/perf/util/trace-event.c
> @@ -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

[Arnaldo Carvalho de Melo]

Em Mon, Sep 14, 2015 at 04:59:41PM -0400, Raphaël Beamonte escreveu:
> 2015-09-14 16:53 GMT-04:00 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>:
> > +++ b/tools/perf/util/evsel.c
> > @@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int
> >         struct 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?

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
 
> >                 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);
> >  }
> >
> > diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
> > index 8e3a60e3e15f..802bb868d446 100644
> > --- a/tools/perf/util/trace-event.c
> > +++ b/tools/perf/util/trace-event.c
> > @@ -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

[Raphaël Beamonte]

2015-09-14 17:36 GMT-04:00 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>:
> Em Mon, Sep 14, 2015 at 04:59:41PM -0400, Raphaël Beamonte escreveu:
>> 2015-09-14 16:53 GMT-04:00 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>:
>> > +++ b/tools/perf/util/evsel.c
>> > @@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int
>> >         struct 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?
>
> 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?

>> >                 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);
>> >  }
>> >
>> > diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
>> > index 8e3a60e3e15f..802bb868d446 100644
>> > --- a/tools/perf/util/trace-event.c
>> > +++ b/tools/perf/util/trace-event.c
>> > @@ -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

[Arnaldo Carvalho de Melo]

Em Mon, Sep 14, 2015 at 06:05:38PM -0400, Raphaël Beamonte escreveu:
> 2015-09-14 17:36 GMT-04:00 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>:
> > Em Mon, Sep 14, 2015 at 04:59:41PM -0400, Raphaël Beamonte escreveu:
> >> 2015-09-14 16:53 GMT-04:00 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>:
> >> > +++ b/tools/perf/util/evsel.c
> >> > @@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int
> >> >         struct 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?
> >
> > 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 :-)

> 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
 
> >> >                 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);
> >> >  }
> >> >
> >> > diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
> >> > index 8e3a60e3e15f..802bb868d446 100644
> >> > --- a/tools/perf/util/trace-event.c
> >> > +++ b/tools/perf/util/trace-event.c
> >> > @@ -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

[Arnaldo Carvalho de Melo]

Em Mon, Sep 14, 2015 at 05:53:03PM -0300, Arnaldo Carvalho de Melo escreveu:
> Ok continuing, found two more problems in this patch, fixed as follows,
> merging.
 
> - Arnaldo
 
> +++ b/tools/perf/tests/mmap-basic.c
> @@ -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 ;-)

>  			pr_debug("perf_evsel__new\n");
>  			goto out_delete_evlist;
>  		}

[tip:perf/core] perf evsel: Propagate error info from tp_format

[tip-bot for Jiri Olsa]

Commit-ID:  8dd2a1317eba2c207632dcb19adb7cb746861652
Gitweb:     http://git.kernel.org/tip/8dd2a1317eba2c207632dcb19adb7cb746861652
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Mon, 7 Sep 2015 10:38:06 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Raphael Beamonte <raphael.beamonte@gmail.com>
Link: http://lkml.kernel.org/r/1441615087-13886-5-git-send-email-jolsa@kernel.org
[ Add two missing ERR_PTR() and one IS_ERR() ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 2156532..93b80f1 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -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, void
 	struct 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) {
diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
index 5216242..790e413 100644
--- a/tools/perf/tests/evsel-tp-sched.c
+++ b/tools/perf/tests/evsel-tp-sched.c
@@ -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;
 
diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
index 666b67a..4495493 100644
--- a/tools/perf/tests/mmap-basic.c
+++ b/tools/perf/tests/mmap-basic.c
@@ -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;
 		}
diff --git a/tools/perf/tests/openat-syscall-all-cpus.c b/tools/perf/tests/openat-syscall-all-cpus.c
index 495d812..9e104a2 100644
--- a/tools/perf/tests/openat-syscall-all-cpus.c
+++ b/tools/perf/tests/openat-syscall-all-cpus.c
@@ -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;
diff --git a/tools/perf/tests/openat-syscall-tp-fields.c b/tools/perf/tests/openat-syscall-tp-fields.c
index 01a1962..473d386 100644
--- a/tools/perf/tests/openat-syscall-tp-fields.c
+++ b/tools/perf/tests/openat-syscall-tp-fields.c
@@ -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;
 	}
diff --git a/tools/perf/tests/openat-syscall.c b/tools/perf/tests/openat-syscall.c
index 08ac9d9..7b1db83 100644
--- a/tools/perf/tests/openat-syscall.c
+++ b/tools/perf/tests/openat-syscall.c
@@ -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;
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d51a520..3cb2bf9 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -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;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 771ade4..6b5d1b5 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -13,6 +13,7 @@
 #include <traceevent/event-parse.h>
 #include <linux/hw_breakpoint.h>
 #include <linux/perf_event.h>
+#include <linux/err.h>
 #include <sys/resource.h>
 #include "asm/bug.h"
 #include "callchain.h"
@@ -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] = {
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index f164a14..62ab307 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -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);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1b284b8..c47831c 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1,4 +1,5 @@
 #include <linux/hw_breakpoint.h>
+#include <linux/err.h>
 #include "util.h"
 #include "../perf.h"
 #include "evlist.h"
@@ -393,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;
 }
 
diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index 2f4996a..802bb86 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -7,6 +7,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <traceevent/event-parse.h>
 #include <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);
 }

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

[Jiri Olsa]

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

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

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

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

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

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

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

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

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c47831c47220..d3fb90be6216 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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;
 	}
 
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 54a3004a8192..8bcc45868457 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -371,28 +371,30 @@ event_legacy_tracepoint:
 PE_NAME '-' PE_NAME ':' PE_NAME
 {
 	struct parse_events_evlist *data = _data;
+	struct parse_events_error *error = data->error;
 	struct list_head *list;
 	char sys_name[128];
 	snprintf(&sys_name, 128, "%s-%s", $1, $3);
 
 	ALLOC_LIST(list);
-	ABORT_ON(parse_events_add_tracepoint(list, &data->idx, &sys_name, $5, data->error));
+	if (parse_events_add_tracepoint(list, &data->idx, &sys_name, $5, error)) {
+		if (error)
+			error->idx = @1.first_column;
+		return -1;
+	}
 	$$ = list;
 }
 |
 PE_NAME ':' PE_NAME
 {
 	struct parse_events_evlist *data = _data;
+	struct parse_events_error *error = data->error;
 	struct list_head *list;
 
 	ALLOC_LIST(list);
-	if (parse_events_add_tracepoint(list, &data->idx, $1, $3, data->error)) {
-		struct parse_events_error *error = data->error;
-
-		if (error) {
+	if (parse_events_add_tracepoint(list, &data->idx, $1, $3, error)) {
+		if (error)
 			error->idx = @1.first_column;
-			error->str = strdup("unknown tracepoint");
-		}
 		return -1;
 	}
 	$$ = list;
-- 
2.4.3

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

[Namhyung Kim]

On Mon, Sep 07, 2015 at 10:38:07AM +0200, Jiri Olsa wrote:
> 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'

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

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

[Jiri Olsa]

On Thu, Sep 10, 2015 at 04:00:30PM +0900, Namhyung Kim wrote:
> On Mon, Sep 07, 2015 at 10:38:07AM +0200, Jiri Olsa wrote:
> > 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'
> 
> 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.

right, patch below keeps the actual mount and
display proper info.. could you please try?

> 
> Also, IIRC tracepoint events adds PERF_SAMPLE_RAW bit automatically,
> and it requires perf_event_paranoid being -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


---
diff --git a/tools/lib/api/fs/tracing_path.c b/tools/lib/api/fs/tracing_path.c
index 38aca2dd1946..0406a7d5c891 100644
--- a/tools/lib/api/fs/tracing_path.c
+++ b/tools/lib/api/fs/tracing_path.c
@@ -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:

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

[Namhyung Kim]

Hi Jiri,

On Thu, Sep 10, 2015 at 10:05:37AM +0200, Jiri Olsa wrote:
> On Thu, Sep 10, 2015 at 04:00:30PM +0900, Namhyung Kim wrote:
> > On Mon, Sep 07, 2015 at 10:38:07AM +0200, Jiri Olsa wrote:
> > > 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'
> > 
> > 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.
> 
> 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..


> 
> > 
> > Also, IIRC tracepoint events adds PERF_SAMPLE_RAW bit automatically,
> > and it requires perf_event_paranoid being -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


> [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
> 
> 
> ---
> diff --git a/tools/lib/api/fs/tracing_path.c b/tools/lib/api/fs/tracing_path.c
> index 38aca2dd1946..0406a7d5c891 100644
> --- a/tools/lib/api/fs/tracing_path.c
> +++ b/tools/lib/api/fs/tracing_path.c
> @@ -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:

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

[Jiri Olsa]

On Sat, Sep 12, 2015 at 01:09:31AM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Thu, Sep 10, 2015 at 10:05:37AM +0200, Jiri Olsa wrote:
> > On Thu, Sep 10, 2015 at 04:00:30PM +0900, Namhyung Kim wrote:
> > > On Mon, Sep 07, 2015 at 10:38:07AM +0200, Jiri Olsa wrote:
> > > > 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'
> > > 
> > > 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.
> > 
> > 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

> 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

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

[Raphaël Beamonte]

2015-09-11 12:16 GMT-04:00 Jiri Olsa <jolsa@redhat.com>:
> On Sat, Sep 12, 2015 at 01:09:31AM +0900, Namhyung Kim wrote:
<SNIP>
>> 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?

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

[Arnaldo Carvalho de Melo]

Em Fri, Sep 11, 2015 at 01:50:02PM -0400, Raphaël Beamonte escreveu:
> 2015-09-11 12:16 GMT-04:00 Jiri Olsa <jolsa@redhat.com>:
> > On Sat, Sep 12, 2015 at 01:09:31AM +0900, Namhyung Kim wrote:
> <SNIP>
> >> 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.

> 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

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

[Raphaël Beamonte]

2015-09-11 14:55 GMT-04:00 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>:
> Em Fri, Sep 11, 2015 at 01:50:02PM -0400, Raphaël Beamonte escreveu:
>> 2015-09-11 12:16 GMT-04:00 Jiri Olsa <jolsa@redhat.com>:
>> > On Sat, Sep 12, 2015 at 01:09:31AM +0900, Namhyung Kim wrote:
>> <SNIP>
>> >> 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.

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?

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

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 ?

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

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

[Arnaldo Carvalho de Melo]

Em Fri, Sep 11, 2015 at 03:56:44PM -0400, Raphaël Beamonte escreveu:
> 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

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

[Raphaël Beamonte]

2015-09-11 16:22 GMT-04:00 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>:
> Em Fri, Sep 11, 2015 at 03:56:44PM -0400, Raphaël Beamonte escreveu:
>> 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.

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!

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

[Arnaldo Carvalho de Melo]

Em Thu, Sep 10, 2015 at 10:05:37AM +0200, Jiri Olsa escreveu:
> On Thu, Sep 10, 2015 at 04:00:30PM +0900, Namhyung Kim wrote:
> > On Mon, Sep 07, 2015 at 10:38:07AM +0200, Jiri Olsa wrote:
> > > 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'
> > 
> > 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.
> 
> 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
 
> > 
> > Also, IIRC tracepoint events adds PERF_SAMPLE_RAW bit automatically,
> > and it requires perf_event_paranoid being -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
> 
> 
> ---
> diff --git a/tools/lib/api/fs/tracing_path.c b/tools/lib/api/fs/tracing_path.c
> index 38aca2dd1946..0406a7d5c891 100644
> --- a/tools/lib/api/fs/tracing_path.c
> +++ b/tools/lib/api/fs/tracing_path.c
> @@ -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:perf/core] perf tools: Enhance parsing events tracepoint error output

[tip-bot for Jiri Olsa]

Commit-ID:  196581717d85f59365dc9303685cd5b1cdf106a3
Gitweb:     http://git.kernel.org/tip/196581717d85f59365dc9303685cd5b1cdf106a3
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Mon, 7 Sep 2015 10:38:07 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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
  ...

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Raphael Beamonte <raphael.beamonte@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1441615087-13886-6-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-events.c | 35 ++++++++++++++++++++++++++++++++---
 tools/perf/util/parse-events.y | 16 +++++++++-------
 2 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c47831c..d3fb90b 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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;
 	}
 
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 54a3004..8bcc458 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -371,28 +371,30 @@ event_legacy_tracepoint:
 PE_NAME '-' PE_NAME ':' PE_NAME
 {
 	struct parse_events_evlist *data = _data;
+	struct parse_events_error *error = data->error;
 	struct list_head *list;
 	char sys_name[128];
 	snprintf(&sys_name, 128, "%s-%s", $1, $3);
 
 	ALLOC_LIST(list);
-	ABORT_ON(parse_events_add_tracepoint(list, &data->idx, &sys_name, $5, data->error));
+	if (parse_events_add_tracepoint(list, &data->idx, &sys_name, $5, error)) {
+		if (error)
+			error->idx = @1.first_column;
+		return -1;
+	}
 	$$ = list;
 }
 |
 PE_NAME ':' PE_NAME
 {
 	struct parse_events_evlist *data = _data;
+	struct parse_events_error *error = data->error;
 	struct list_head *list;
 
 	ALLOC_LIST(list);
-	if (parse_events_add_tracepoint(list, &data->idx, $1, $3, data->error)) {
-		struct parse_events_error *error = data->error;
-
-		if (error) {
+	if (parse_events_add_tracepoint(list, &data->idx, $1, $3, error)) {
+		if (error)
 			error->idx = @1.first_column;
-			error->str = strdup("unknown tracepoint");
-		}
 		return -1;
 	}
 	$$ = list;