| Message ID | 20241201231313.42935-1-george@george-graphics.co.uk |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libgpiod: bindings: python: Fix PyDict_Next contiguous assumption and pypy | expand |
On Mon, Dec 2, 2024 at 12:15 AM George Harker <george@george-graphics.co.uk> 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. > > As per discussion here https://github.com/pypy/pypy/issues/5142 > Ah, it's right there in the documentation too[1]: "Its value represents offsets within the internal dictionary structure, and since the structure is sparse, the offsets are not consecutive." Thanks for the catch. I wasn't aware pypy actually reimplements the entire C API of cpython. How would I go about building libgpiod python bindings with pypy? Bart [1] https://docs.python.org/3/c-api/dict.html#c.PyDict_Next
On Mon, Dec 2, 2024 at 12:15 AM George Harker <george@george-graphics.co.uk> 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
On Mon, Dec 2, 2024 at 9:33 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Mon, Dec 2, 2024 at 12:15 AM George Harker > <george@george-graphics.co.uk> 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: > One more thing: the commit title should look like this: [libgpiod][PATCH] bindings: python: Fix PyDict_Next contiguous assumption and pypy The [libgpiod] bit should be a mailing list tag. Bart
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; 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. 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; } Py_BEGIN_ALLOW_THREADS; ret = gpiod_line_request_set_values_subset(self->request, - pos, + index, self->offsets, self->values); Py_END_ALLOW_THREADS;