Messages in this thread Patch in this message | | | Date | Fri, 9 Jul 2021 14:57:23 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | [PATCH] libperf: Remove BUG_ON() from library code in get_group_fd(). was Re: [PATCH 6/7] libperF: Add group support to perf_evsel__open |
| |
Em Wed, Jul 07, 2021 at 08:15:38PM +0200, Jiri Olsa escreveu: > On Wed, Jul 07, 2021 at 02:43:07PM -0300, Arnaldo Carvalho de Melo wrote: > > Em Tue, Jul 06, 2021 at 05:17:03PM +0200, Jiri Olsa escreveu: > > > +static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread) > > > +{ > > > + struct perf_evsel *leader = evsel->leader; > > > + int fd; > > > + > > > + if (evsel == leader) > > > + return -1; > > > + > > > + /* > > > + * Leader must be already processed/open, > > > + * if not it's a bug. > > > + */ > > > + BUG_ON(!leader->fd);
> > Humm, having panics in library code looks ugly, why can't we just return > > some errno and let the whatever is using the library to fail gracefully?
> true, I took it from perf code, did not realize this, > I'll check what can we do in here
So, I've added this as a follow up patch:
commit 0ec138125eaea5f15157adcecc3e0def1ad2ed22 Author: Arnaldo Carvalho de Melo <acme@redhat.com> Date: Fri Jul 9 14:52:16 2021 -0300
libperf: Remove BUG_ON() from library code in get_group_fd()
We shouldn't just panic, return a value that doesn't clash with what perf_evsel__open() was already returning in case of error, i.e. errno when sys_perf_event_open() fails.
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Ian Rogers <irogers@google.com> Cc: Jiri Olsa <jolsa@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Michael Petlan <mpetlan@redhat.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Shunsuke Nakamura <nakamura.shun@fujitsu.com> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c index 9ebf7122d4766c5e..d8886720e83d8dfe 100644 --- a/tools/lib/perf/evsel.c +++ b/tools/lib/perf/evsel.c @@ -77,23 +77,30 @@ sys_perf_event_open(struct perf_event_attr *attr, return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd, flags); } -static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread) +static int get_group_fd(struct perf_evsel *evsel, int cpu, int thread, int *group_fd) { struct perf_evsel *leader = evsel->leader; int fd; - if (evsel == leader) - return -1; + if (evsel == leader) { + *group_fd = -1; + return 0; + } /* * Leader must be already processed/open, * if not it's a bug. */ - BUG_ON(!leader->fd); + if (!leader->fd) + return -ENOTCONN; fd = FD(leader, cpu, thread); - BUG_ON(fd == -1); - return fd; + if (fd == -1) + return -EBADF; + + *group_fd = fd; + + return 0; } int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *cpus, @@ -133,7 +140,9 @@ int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *cpus, for (thread = 0; thread < threads->nr; thread++) { int fd, group_fd; - group_fd = get_group_fd(evsel, cpu, thread); + err = get_group_fd(evsel, cpu, thread, &group_fd); + if (err < 0) + return err; fd = sys_perf_event_open(&evsel->attr, threads->map[thread].pid,
| |