* [PATCH] perf python: Set error messages on call failure
@ 2023-05-02 20:31 Jinli Xiao
2023-05-02 20:37 ` Ian Rogers
0 siblings, 1 reply; 4+ messages in thread
From: Jinli Xiao @ 2023-05-02 20:31 UTC (permalink / raw)
Cc: Jinli Xiao, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Leo Yan, Suzuki Poulouse, James Clark,
linux-perf-users, linux-kernel
Updates the perf python binding to provide more informative error
messages to the user when there is a call failure. The changes include
setting error messages on several different scenarios when the package
needs to return -1 or NULL.
Signed-off-by: Jinli Xiao <jinli.xiao@nyu.edu>
---
tools/perf/util/python.c | 61 ++++++++++++++++++++++++++++++----------
1 file changed, 46 insertions(+), 15 deletions(-)
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 42e8b813d010..7e6b12e87744 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -635,12 +635,16 @@ static int pyrf_cpu_map__init(struct pyrf_cpu_map *pcpus,
char *cpustr = NULL;
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|s",
- kwlist, &cpustr))
+ kwlist, &cpustr)) {
+ PyErr_BadArgument();
return -1;
+ }
pcpus->cpus = perf_cpu_map__new(cpustr);
- if (pcpus->cpus == NULL)
+ if (pcpus->cpus == NULL) {
+ PyErr_SetFromErrno(PyExc_OSError);
return -1;
+ }
return 0;
}
@@ -704,12 +708,16 @@ static int pyrf_thread_map__init(struct pyrf_thread_map *pthreads,
int pid = -1, tid = -1, uid = UINT_MAX;
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|iii",
- kwlist, &pid, &tid, &uid))
+ kwlist, &pid, &tid, &uid)) {
+ PyErr_BadArgument();
return -1;
+ }
pthreads->threads = thread_map__new(pid, tid, uid);
- if (pthreads->threads == NULL)
+ if (pthreads->threads == NULL) {
+ PyErr_SetFromErrno(PyExc_OSError);
return -1;
+ }
return 0;
}
@@ -839,13 +847,18 @@ static int pyrf_evsel__init(struct pyrf_evsel *pevsel,
&enable_on_exec, &task, &watermark,
&precise_ip, &mmap_data, &sample_id_all,
&attr.wakeup_events, &attr.bp_type,
- &attr.bp_addr, &attr.bp_len, &idx))
+ &attr.bp_addr, &attr.bp_len, &idx)) {
+ PyErr_BadArgument();
return -1;
+ }
/* union... */
if (sample_period != 0) {
- if (attr.sample_freq != 0)
- return -1; /* FIXME: throw right exception */
+ if (attr.sample_freq != 0) {
+ PyErr_SetString(PyExc_ValueError,
+ "perf: sample_freq and sample_period are mutually exclusive");
+ return -1;
+ }
attr.sample_period = sample_period;
}
@@ -892,8 +905,10 @@ static PyObject *pyrf_evsel__open(struct pyrf_evsel *pevsel,
static char *kwlist[] = { "cpus", "threads", "group", "inherit", NULL };
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|OOii", kwlist,
- &pcpus, &pthreads, &group, &inherit))
+ &pcpus, &pthreads, &group, &inherit)) {
+ PyErr_BadArgument();
return NULL;
+ }
if (pthreads != NULL)
threads = ((struct pyrf_thread_map *)pthreads)->threads;
@@ -957,8 +972,10 @@ static int pyrf_evlist__init(struct pyrf_evlist *pevlist,
struct perf_cpu_map *cpus;
struct perf_thread_map *threads;
- if (!PyArg_ParseTuple(args, "OO", &pcpus, &pthreads))
+ if (!PyArg_ParseTuple(args, "OO", &pcpus, &pthreads)) {
+ PyErr_BadArgument();
return -1;
+ }
threads = ((struct pyrf_thread_map *)pthreads)->threads;
cpus = ((struct pyrf_cpu_map *)pcpus)->cpus;
@@ -980,8 +997,10 @@ static PyObject *pyrf_evlist__mmap(struct pyrf_evlist *pevlist,
int pages = 128, overwrite = false;
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|ii", kwlist,
- &pages, &overwrite))
+ &pages, &overwrite)) {
+ PyErr_BadArgument();
return NULL;
+ }
if (evlist__mmap(evlist, pages) < 0) {
PyErr_SetFromErrno(PyExc_OSError);
@@ -999,8 +1018,10 @@ static PyObject *pyrf_evlist__poll(struct pyrf_evlist *pevlist,
static char *kwlist[] = { "timeout", NULL };
int timeout = -1, n;
- if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|i", kwlist, &timeout))
+ if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|i", kwlist, &timeout)) {
+ PyErr_BadArgument();
return NULL;
+ }
n = evlist__poll(evlist, timeout);
if (n < 0) {
@@ -1057,8 +1078,10 @@ static PyObject *pyrf_evlist__add(struct pyrf_evlist *pevlist,
PyObject *pevsel;
struct evsel *evsel;
- if (!PyArg_ParseTuple(args, "O", &pevsel))
+ if (!PyArg_ParseTuple(args, "O", &pevsel)) {
+ PyErr_BadArgument();
return NULL;
+ }
Py_INCREF(pevsel);
evsel = &((struct pyrf_evsel *)pevsel)->evsel;
@@ -1093,12 +1116,16 @@ static PyObject *pyrf_evlist__read_on_cpu(struct pyrf_evlist *pevlist,
int err;
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "i|i", kwlist,
- &cpu, &sample_id_all))
+ &cpu, &sample_id_all)) {
+ PyErr_BadArgument();
return NULL;
+ }
md = get_md(evlist, cpu);
- if (!md)
+ if (!md) {
+ PyErr_SetFromErrno(PyExc_OSError);
return NULL;
+ }
if (perf_mmap__read_init(&md->core) < 0)
goto end;
@@ -1324,6 +1351,8 @@ static PyObject *pyrf__tracepoint(struct pyrf_evsel *pevsel,
PyObject *args, PyObject *kwargs)
{
#ifndef HAVE_LIBTRACEEVENT
+ PyErr_SetString(PyExc_OSError,
+ "perf: tracepoint support not compiled in");
return NULL;
#else
struct tep_event *tp_format;
@@ -1332,8 +1361,10 @@ static PyObject *pyrf__tracepoint(struct pyrf_evsel *pevsel,
char *name = NULL;
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|ss", kwlist,
- &sys, &name))
+ &sys, &name)) {
+ PyErr_BadArgument();
return NULL;
+ }
tp_format = trace_event__tp_format(sys, name);
if (IS_ERR(tp_format))
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] perf python: Set error messages on call failure
2023-05-02 20:31 [PATCH] perf python: Set error messages on call failure Jinli Xiao
@ 2023-05-02 20:37 ` Ian Rogers
2023-05-03 2:27 ` Jinli Xiao
0 siblings, 1 reply; 4+ messages in thread
From: Ian Rogers @ 2023-05-02 20:37 UTC (permalink / raw)
To: Jinli Xiao
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, Leo Yan, Suzuki Poulouse, James Clark,
linux-perf-users, linux-kernel
On Tue, May 2, 2023 at 1:31 PM Jinli Xiao <jinli.xiao@nyu.edu> wrote:
>
> Updates the perf python binding to provide more informative error
> messages to the user when there is a call failure. The changes include
> setting error messages on several different scenarios when the package
> needs to return -1 or NULL.
>
> Signed-off-by: Jinli Xiao <jinli.xiao@nyu.edu>
Nice! Would it be possible to test this? We could do a shell test:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/tests/shell
Thanks,
Ian
> ---
> tools/perf/util/python.c | 61 ++++++++++++++++++++++++++++++----------
> 1 file changed, 46 insertions(+), 15 deletions(-)
>
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 42e8b813d010..7e6b12e87744 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -635,12 +635,16 @@ static int pyrf_cpu_map__init(struct pyrf_cpu_map *pcpus,
> char *cpustr = NULL;
>
> if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|s",
> - kwlist, &cpustr))
> + kwlist, &cpustr)) {
> + PyErr_BadArgument();
> return -1;
> + }
>
> pcpus->cpus = perf_cpu_map__new(cpustr);
> - if (pcpus->cpus == NULL)
> + if (pcpus->cpus == NULL) {
> + PyErr_SetFromErrno(PyExc_OSError);
> return -1;
> + }
> return 0;
> }
>
> @@ -704,12 +708,16 @@ static int pyrf_thread_map__init(struct pyrf_thread_map *pthreads,
> int pid = -1, tid = -1, uid = UINT_MAX;
>
> if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|iii",
> - kwlist, &pid, &tid, &uid))
> + kwlist, &pid, &tid, &uid)) {
> + PyErr_BadArgument();
> return -1;
> + }
>
> pthreads->threads = thread_map__new(pid, tid, uid);
> - if (pthreads->threads == NULL)
> + if (pthreads->threads == NULL) {
> + PyErr_SetFromErrno(PyExc_OSError);
> return -1;
> + }
> return 0;
> }
>
> @@ -839,13 +847,18 @@ static int pyrf_evsel__init(struct pyrf_evsel *pevsel,
> &enable_on_exec, &task, &watermark,
> &precise_ip, &mmap_data, &sample_id_all,
> &attr.wakeup_events, &attr.bp_type,
> - &attr.bp_addr, &attr.bp_len, &idx))
> + &attr.bp_addr, &attr.bp_len, &idx)) {
> + PyErr_BadArgument();
> return -1;
> + }
>
> /* union... */
> if (sample_period != 0) {
> - if (attr.sample_freq != 0)
> - return -1; /* FIXME: throw right exception */
> + if (attr.sample_freq != 0) {
> + PyErr_SetString(PyExc_ValueError,
> + "perf: sample_freq and sample_period are mutually exclusive");
> + return -1;
> + }
> attr.sample_period = sample_period;
> }
>
> @@ -892,8 +905,10 @@ static PyObject *pyrf_evsel__open(struct pyrf_evsel *pevsel,
> static char *kwlist[] = { "cpus", "threads", "group", "inherit", NULL };
>
> if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|OOii", kwlist,
> - &pcpus, &pthreads, &group, &inherit))
> + &pcpus, &pthreads, &group, &inherit)) {
> + PyErr_BadArgument();
> return NULL;
> + }
>
> if (pthreads != NULL)
> threads = ((struct pyrf_thread_map *)pthreads)->threads;
> @@ -957,8 +972,10 @@ static int pyrf_evlist__init(struct pyrf_evlist *pevlist,
> struct perf_cpu_map *cpus;
> struct perf_thread_map *threads;
>
> - if (!PyArg_ParseTuple(args, "OO", &pcpus, &pthreads))
> + if (!PyArg_ParseTuple(args, "OO", &pcpus, &pthreads)) {
> + PyErr_BadArgument();
> return -1;
> + }
>
> threads = ((struct pyrf_thread_map *)pthreads)->threads;
> cpus = ((struct pyrf_cpu_map *)pcpus)->cpus;
> @@ -980,8 +997,10 @@ static PyObject *pyrf_evlist__mmap(struct pyrf_evlist *pevlist,
> int pages = 128, overwrite = false;
>
> if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|ii", kwlist,
> - &pages, &overwrite))
> + &pages, &overwrite)) {
> + PyErr_BadArgument();
> return NULL;
> + }
>
> if (evlist__mmap(evlist, pages) < 0) {
> PyErr_SetFromErrno(PyExc_OSError);
> @@ -999,8 +1018,10 @@ static PyObject *pyrf_evlist__poll(struct pyrf_evlist *pevlist,
> static char *kwlist[] = { "timeout", NULL };
> int timeout = -1, n;
>
> - if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|i", kwlist, &timeout))
> + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|i", kwlist, &timeout)) {
> + PyErr_BadArgument();
> return NULL;
> + }
>
> n = evlist__poll(evlist, timeout);
> if (n < 0) {
> @@ -1057,8 +1078,10 @@ static PyObject *pyrf_evlist__add(struct pyrf_evlist *pevlist,
> PyObject *pevsel;
> struct evsel *evsel;
>
> - if (!PyArg_ParseTuple(args, "O", &pevsel))
> + if (!PyArg_ParseTuple(args, "O", &pevsel)) {
> + PyErr_BadArgument();
> return NULL;
> + }
>
> Py_INCREF(pevsel);
> evsel = &((struct pyrf_evsel *)pevsel)->evsel;
> @@ -1093,12 +1116,16 @@ static PyObject *pyrf_evlist__read_on_cpu(struct pyrf_evlist *pevlist,
> int err;
>
> if (!PyArg_ParseTupleAndKeywords(args, kwargs, "i|i", kwlist,
> - &cpu, &sample_id_all))
> + &cpu, &sample_id_all)) {
> + PyErr_BadArgument();
> return NULL;
> + }
>
> md = get_md(evlist, cpu);
> - if (!md)
> + if (!md) {
> + PyErr_SetFromErrno(PyExc_OSError);
> return NULL;
> + }
>
> if (perf_mmap__read_init(&md->core) < 0)
> goto end;
> @@ -1324,6 +1351,8 @@ static PyObject *pyrf__tracepoint(struct pyrf_evsel *pevsel,
> PyObject *args, PyObject *kwargs)
> {
> #ifndef HAVE_LIBTRACEEVENT
> + PyErr_SetString(PyExc_OSError,
> + "perf: tracepoint support not compiled in");
> return NULL;
> #else
> struct tep_event *tp_format;
> @@ -1332,8 +1361,10 @@ static PyObject *pyrf__tracepoint(struct pyrf_evsel *pevsel,
> char *name = NULL;
>
> if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|ss", kwlist,
> - &sys, &name))
> + &sys, &name)) {
> + PyErr_BadArgument();
> return NULL;
> + }
>
> tp_format = trace_event__tp_format(sys, name);
> if (IS_ERR(tp_format))
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] perf python: Set error messages on call failure
2023-05-02 20:37 ` Ian Rogers
@ 2023-05-03 2:27 ` Jinli Xiao
2023-05-03 17:16 ` Ian Rogers
0 siblings, 1 reply; 4+ messages in thread
From: Jinli Xiao @ 2023-05-03 2:27 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, Leo Yan, Suzuki Poulouse, James Clark,
linux-perf-users, linux-kernel
Thanks for the prompt response!
On Tue, May 2, 2023 at 4:37 PM Ian Rogers <irogers@google.com> wrote:
>
> Nice! Would it be possible to test this? We could do a shell test
All my changes in this patch is just setting the error message that
can be interpreted by Python just before the bindings return.
I am not sure about shell test, but I can do something like
[root@nyu-lexis linux]# export PYTHONPATH=~jinli/linux/tools/perf/python/
[root@nyu-lexis linux]# python3
Python 3.11.2 (main, Feb 8 2023, 00:00:00) [GCC 12.2.1 20221121 (Red
Hat 12.2.1-4)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import perf
>>> thread_map = perf.thread_map(9999) # a non-existent pid in /proc
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory
whereas the original output is
>>> thread_map = perf.thread_map(9999)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
SystemError: <class 'perf.thread_map'> returned NULL without setting
an exception
Upon testing I did find some of the changes unnecessary. I will submit
a new patch to this.
This is my first time contributing so please let me know if I did
anything wrong :)
Best wishes,
Jinli
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] perf python: Set error messages on call failure
2023-05-03 2:27 ` Jinli Xiao
@ 2023-05-03 17:16 ` Ian Rogers
0 siblings, 0 replies; 4+ messages in thread
From: Ian Rogers @ 2023-05-03 17:16 UTC (permalink / raw)
To: Jinli Xiao
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, Leo Yan, Suzuki Poulouse, James Clark,
linux-perf-users, linux-kernel
On Tue, May 2, 2023 at 7:27 PM Jinli Xiao <jinli.xiao@nyu.edu> wrote:
>
> Thanks for the prompt response!
>
> On Tue, May 2, 2023 at 4:37 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Nice! Would it be possible to test this? We could do a shell test
>
> All my changes in this patch is just setting the error message that
> can be interpreted by Python just before the bindings return.
>
> I am not sure about shell test, but I can do something like
>
> [root@nyu-lexis linux]# export PYTHONPATH=~jinli/linux/tools/perf/python/
> [root@nyu-lexis linux]# python3
> Python 3.11.2 (main, Feb 8 2023, 00:00:00) [GCC 12.2.1 20221121 (Red
> Hat 12.2.1-4)] on linux
> Type "help", "copyright", "credits" or "license" for more information.
> >>> import perf
> >>> thread_map = perf.thread_map(9999) # a non-existent pid in /proc
> Traceback (most recent call last):
> File "<stdin>", line 1, in <module>
> FileNotFoundError: [Errno 2] No such file or directory
>
> whereas the original output is
>
> >>> thread_map = perf.thread_map(9999)
> Traceback (most recent call last):
> File "<stdin>", line 1, in <module>
> SystemError: <class 'perf.thread_map'> returned NULL without setting
> an exception
>
> Upon testing I did find some of the changes unnecessary. I will submit
> a new patch to this.
>
> This is my first time contributing so please let me know if I did
> anything wrong :)
Everything is good with your patch. I'm keen that we try to make sure
we have coverage with tests. We have other shell tests that run
python:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/shell/stat+json_output.sh?h=perf-tools-next
So I wonder we can start doing what you did in your example and then
just assert we get a FileNotFoundError in the python. The shell script
is really just a way to run the python and can follow the example in
the link. Sound good?
Thanks,
Ian
> Best wishes,
> Jinli
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-03 17:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-02 20:31 [PATCH] perf python: Set error messages on call failure Jinli Xiao
2023-05-02 20:37 ` Ian Rogers
2023-05-03 2:27 ` Jinli Xiao
2023-05-03 17:16 ` Ian Rogers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).