Re: [PATCH] libgpiod: bindings: python: Fix PyDict_Next contiguous assumption and pypy

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Dec 2, 2024 at 12:15 AM George Harker
<george@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> PyDict_Next does not guarantee pos is contiguous, and pypy increments
> past the end of the dict size.  Patch fixes reliance on pos for constructing
> args for gpiod call.
>

Just a couple nits below:

> As per discussion here https://github.com/pypy/pypy/issues/5142
>

Can you add your Signed-off-by: tag here?

> ---
>  bindings/python/gpiod/ext/request.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/bindings/python/gpiod/ext/request.c b/bindings/python/gpiod/ext/request.c
> index e1a2a42..4e49289 100644
> --- a/bindings/python/gpiod/ext/request.c
> +++ b/bindings/python/gpiod/ext/request.c
> @@ -206,6 +206,7 @@ static PyObject *request_set_values(request_object *self, PyObject *args)
>  {
>         PyObject *values, *key, *val, *val_stripped;
>         Py_ssize_t pos = 0;
> +       Py_ssize_t index = 0;

Please put this on the same line as pos.

>         int ret;
>
>         ret = PyArg_ParseTuple(args, "O", &values);
> @@ -214,8 +215,10 @@ static PyObject *request_set_values(request_object *self, PyObject *args)
>
>         clear_buffers(self);
>
> +       // Note: pos may not be contiguous and in pypy, is incremented
> +       // past the end of the dict size.

Please use /* */ style comments for consistency and don't mention pypy
- it's actually python spec and just by chance it worked fine in
cpython so far.

>         while (PyDict_Next(values, &pos, &key, &val)) {
> -               self->offsets[pos - 1] = Py_gpiod_PyLongAsUnsignedInt(key);
> +               self->offsets[index] = Py_gpiod_PyLongAsUnsignedInt(key);
>                 if (PyErr_Occurred())
>                         return NULL;
>
> @@ -223,15 +226,17 @@ static PyObject *request_set_values(request_object *self, PyObject *args)
>                 if (!val_stripped)
>                         return NULL;
>
> -               self->values[pos - 1] = PyLong_AsLong(val_stripped);
> +               self->values[index] = PyLong_AsLong(val_stripped);
>                 Py_DECREF(val_stripped);
>                 if (PyErr_Occurred())
>                         return NULL;
> +
> +               index += 1;

index++ ?

>         }
>
>         Py_BEGIN_ALLOW_THREADS;
>         ret = gpiod_line_request_set_values_subset(self->request,
> -                                                  pos,
> +                                                  index,
>                                                    self->offsets,
>                                                    self->values);
>         Py_END_ALLOW_THREADS;
> --
> 2.39.5
>
>

Thanks,
Bartosz