Returning NULL will cause the python interpreter to fail but not
report an error. If none wants to be returned then Py_None needs
returning. Set the error for the cases returning NULL so that more
meaningful interpreter behavior is had.

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/python.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 2f28f71325a8..3affde0ad15a 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -475,13 +475,19 @@ static PyObject *pyrf_event__new(const union perf_event *event)
 	if ((event->header.type < PERF_RECORD_MMAP ||
 	     event->header.type > PERF_RECORD_SAMPLE) &&
 	    !(event->header.type == PERF_RECORD_SWITCH ||
-	      event->header.type == PERF_RECORD_SWITCH_CPU_WIDE))
+	      event->header.type == PERF_RECORD_SWITCH_CPU_WIDE)) {
+		PyErr_Format(PyExc_TypeError, "Unexpected header type %u",
+			     event->header.type);
 		return NULL;
+	}
 
 	// FIXME this better be dynamic or we need to parse everything
 	// before calling perf_mmap__consume(), including tracepoint fields.
-	if (sizeof(pevent->event) < event->header.size)
+	if (sizeof(pevent->event) < event->header.size) {
+		PyErr_Format(PyExc_TypeError, "Unexpected event size: %zd < %u",
+			     sizeof(pevent->event), event->header.size);
 		return NULL;
+	}
 
 	ptype = pyrf_event__type[event->header.type];
 	pevent = PyObject_New(struct pyrf_event, ptype);
@@ -1199,8 +1205,10 @@ static PyObject *pyrf_evlist__read_on_cpu(struct pyrf_evlist *pevlist,
 		return NULL;
 
 	md = get_md(evlist, cpu);
-	if (!md)
+	if (!md) {
+		PyErr_Format(PyExc_TypeError, "Unknown CPU '%d'", cpu);
 		return NULL;
+	}
 
 	if (perf_mmap__read_init(&md->core) < 0)
 		goto end;
-- 
2.50.0.727.gbf7dc18ff4-goog

On Wed, Jul 23, 2025 at 04:22:02PM -0700, Ian Rogers wrote:
> Returning NULL will cause the python interpreter to fail but not
> report an error. If none wants to be returned then Py_None needs
> returning. Set the error for the cases returning NULL so that more
> meaningful interpreter behavior is had.

Nit: I still don't get what "add more exceptions" means.  What I'm
seeing is adding more error messages.  Also returning NULL from this
function won't pass it to the interpreter as the convert checks the
return value.

But anyway, looks good to me.

Thanks,
Namhyung

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/python.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 2f28f71325a8..3affde0ad15a 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -475,13 +475,19 @@ static PyObject *pyrf_event__new(const union perf_event *event)
>  	if ((event->header.type < PERF_RECORD_MMAP ||
>  	     event->header.type > PERF_RECORD_SAMPLE) &&
>  	    !(event->header.type == PERF_RECORD_SWITCH ||
> -	      event->header.type == PERF_RECORD_SWITCH_CPU_WIDE))
> +	      event->header.type == PERF_RECORD_SWITCH_CPU_WIDE)) {
> +		PyErr_Format(PyExc_TypeError, "Unexpected header type %u",
> +			     event->header.type);
>  		return NULL;
> +	}
>  
>  	// FIXME this better be dynamic or we need to parse everything
>  	// before calling perf_mmap__consume(), including tracepoint fields.
> -	if (sizeof(pevent->event) < event->header.size)
> +	if (sizeof(pevent->event) < event->header.size) {
> +		PyErr_Format(PyExc_TypeError, "Unexpected event size: %zd < %u",
> +			     sizeof(pevent->event), event->header.size);
>  		return NULL;
> +	}
>  
>  	ptype = pyrf_event__type[event->header.type];
>  	pevent = PyObject_New(struct pyrf_event, ptype);
> @@ -1199,8 +1205,10 @@ static PyObject *pyrf_evlist__read_on_cpu(struct pyrf_evlist *pevlist,
>  		return NULL;
>  
>  	md = get_md(evlist, cpu);
> -	if (!md)
> +	if (!md) {
> +		PyErr_Format(PyExc_TypeError, "Unknown CPU '%d'", cpu);
>  		return NULL;
> +	}
>  
>  	if (perf_mmap__read_init(&md->core) < 0)
>  		goto end;
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
>

On Wed, Jul 23, 2025 at 5:40 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Jul 23, 2025 at 04:22:02PM -0700, Ian Rogers wrote:
> > Returning NULL will cause the python interpreter to fail but not
> > report an error. If none wants to be returned then Py_None needs
> > returning. Set the error for the cases returning NULL so that more
> > meaningful interpreter behavior is had.
>
> Nit: I still don't get what "add more exceptions" means.  What I'm
> seeing is adding more error messages.  Also returning NULL from this
> function won't pass it to the interpreter as the convert checks the
> return value.

So it isn't error messages, calling for example:

  PyErr_Format(PyExc_TypeError, "Unexpected header type %u",
event->header.type);

is like setting errno as in there is a global/thread-local current
exception object. When this is set/pending, returning any value will
cause the interpreter to throw an exception on return, so NULL is an
okay return value - normally for returning NULL you need to return
Py_NONE. Anyway, the old code was returning NULL but without the errno
like exception object set and so the interpreter was crashing/failing.

> But anyway, looks good to me.

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>