Re: [PATCH] perf python: Set error messages on call failure

[Ian Rogers]

On Tue, May 2, 2023 at 1:31 PM Jinli Xiao <jinli.xiao@xxxxxxx> 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@xxxxxxx>

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
>