bindings: python: provide better resource control

So far we rely on cpython's reference counting for closing the GPIO
chips and freeing the underlying resources. This is however wrong.
Python doesn't guarantee anything regarding the lifetime of an object.

Provide a method for closing the underlying chip handle and add
relevant checks to all methods using the resources associated with
a GPIO chip, so that we raise an error should the user use a chip
after closing it.

Signed-off-by: Bartosz Golaszewski <bartekgola@gmail.com>

diff --git a/bindings/python/examples/gpiod_tests.py b/bindings/python/examples/gpiod_tests.py
index dfc3f14d88f53279f2bf75ba23a95fad63a94744..3fd3cb2babf31d74d0ce448ff18f9a985c669336 100755 (executable)
--- a/bindings/python/examples/gpiod_tests.py
+++ b/bindings/python/examples/gpiod_tests.py
@@ -40,14 +40,22 @@ def chip_open_default_lookup():
     by_label = gpiod.Chip('gpio-mockup-A')
     by_number = gpiod.Chip('0')
     print('All good')
+    by_name.close()
+    by_path.close()
+    by_label.close()
+    by_number.close()
 
 add_test('Open a GPIO chip using different lookup modes', chip_open_default_lookup)
 
 def chip_open_different_modes():
     chip = gpiod.Chip('/dev/gpiochip0', gpiod.Chip.OPEN_BY_PATH)
+    chip.close()
     chip = gpiod.Chip('gpiochip0', gpiod.Chip.OPEN_BY_NAME)
+    chip.close()
     chip = gpiod.Chip('gpio-mockup-A', gpiod.Chip.OPEN_BY_LABEL)
+    chip.close()
     chip = gpiod.Chip('0', gpiod.Chip.OPEN_BY_NUMBER)
+    chip.close()
     print('All good')
 
 add_test('Open a GPIO chip using different modes', chip_open_different_modes)
@@ -74,17 +82,39 @@ def chip_open_no_args():
 
 add_test('Open a GPIO chip without arguments', chip_open_no_args)
 
+def chip_use_after_close():
+    chip = gpiod.Chip('gpiochip0')
+    line = chip.get_line(2)
+    chip.close()
+
+    try:
+        chip.name()
+    except ValueError as ex:
+        print('Error as expected: {}'.format(ex))
+
+    try:
+        line = chip.get_line(3)
+    except ValueError as ex:
+        print('Error as expected: {}'.format(ex))
+        return
+
+    assert False, 'ValueError expected'
+
+add_test('Use a GPIO chip after closing it', chip_use_after_close)
+
 def chip_info():
     chip = gpiod.Chip('gpiochip0')
     print('name: {}'.format(chip.name()))
     print('label: {}'.format(chip.label()))
     print('lines: {}'.format(chip.num_lines()))
+    chip.close()
 
 add_test('Print chip info', chip_info)
 
 def print_chip():
     chip = gpiod.Chip('/dev/gpiochip0')
     print(chip)
+    chip.close()
 
 add_test('Print chip object', print_chip)
 
@@ -114,12 +144,14 @@ def print_line():
     chip = gpiod.Chip('gpio-mockup-A')
     line = chip.get_line(3)
     print(line)
+    chip.close()
 
 add_test('Print line object', print_line)
 
 def find_line():
     line = gpiod.find_line('gpio-mockup-A-4')
     print('found line - offset: {}'.format(line.offset()))
+    line.owner().close()
 
 add_test('Find line globally', find_line)
 
@@ -144,6 +176,8 @@ def get_lines():
     for line in lines:
         print(line)
 
+    chip.close()
+
 add_test('Get lines from chip', get_lines)
 
 def get_all_lines():
@@ -156,6 +190,8 @@ def get_all_lines():
     for line in lines:
         print(line)
 
+    chip.close()
+
 add_test('Get all lines from chip', get_all_lines)
 
 def find_lines():
@@ -168,6 +204,8 @@ def find_lines():
     for line in lines:
         print(line)
 
+    chip.close()
+
 add_test('Find multiple lines by name', find_lines)
 
 def create_line_bulk_from_lines():
@@ -178,6 +216,7 @@ def create_line_bulk_from_lines():
     lines = gpiod.LineBulk([line1, line2, line3])
     print('Created LineBulk:')
     print(lines)
+    chip.close()
 
 add_test('Create a LineBulk from a list of lines', create_line_bulk_from_lines)
 
@@ -185,6 +224,7 @@ def line_bulk_to_list():
     chip = gpiod.Chip('gpio-mockup-A')
     lines = chip.get_lines((1, 2, 3))
     print(lines.to_list())
+    chip.close()
 
 add_test('Convert a LineBulk to a list', line_bulk_to_list)
 
@@ -206,6 +246,8 @@ def line_flags():
     print('line is active-low: {}'.format(
             "True" if line.active_state() == gpiod.Line.ACTIVE_LOW else "False"))
 
+    chip.close()
+
 add_test('Check various line flags', line_flags)
 
 def get_value_single_line():
@@ -213,6 +255,7 @@ def get_value_single_line():
     line = chip.get_line(2)
     line.request(consumer=sys.argv[0], type=gpiod.LINE_REQ_DIR_IN)
     print('line value: {}'.format(line.get_value()))
+    chip.close()
 
 add_test('Get value - single line', get_value_single_line)
 
@@ -229,6 +272,8 @@ def set_value_single_line():
     line.request(consumer=sys.argv[0], type=gpiod.LINE_REQ_DIR_IN)
     print('line value after: {}'.format(line.get_value()))
 
+    chip.close()
+
 add_test('Set value - single line', set_value_single_line)
 
 def line_event_single_line():
@@ -246,6 +291,8 @@ def line_event_single_line():
     event = line.event_read()
     print_event(event)
 
+    chip.close()
+
 add_test('Monitor a single line for events', line_event_single_line)
 
 def line_event_multiple_lines():
@@ -267,6 +314,8 @@ def line_event_multiple_lines():
         event = line.event_read()
         print_event(event)
 
+    chip.close()
+
 add_test('Monitor multiple lines for events', line_event_multiple_lines)
 
 def line_event_poll_fd():
@@ -296,6 +345,8 @@ def line_event_poll_fd():
         event = line.event_read()
         print_event(event)
 
+    chip.close()
+
 add_test('Monitor multiple lines using their file descriptors', line_event_poll_fd)
 
 print('API version is {}'.format(gpiod.version_string()))

diff --git a/bindings/python/gpiodmodule.c b/bindings/python/gpiodmodule.c
index 2a5db5f61db98d81e5090cd4c26494cab66bf5f8..1bf8de7befab5ac830b900cd58de0ade5373797e 100644 (file)
--- a/bindings/python/gpiodmodule.c
+++ b/bindings/python/gpiodmodule.c
@@ -56,6 +56,7 @@ static PyObject *gpiod_LineBulk_release(gpiod_LineBulkObject *self);
 static gpiod_LineObject *gpiod_MakeLineObject(gpiod_ChipObject *owner,
                                              struct gpiod_line *line);
 static PyObject *gpiod_Line_repr(gpiod_LineObject *self);
+static bool gpiod_ChipIsClosed(gpiod_ChipObject *chip);
 
 enum {
        gpiod_LINE_REQ_DIR_AS_IS = 1,
@@ -227,6 +228,9 @@ PyDoc_STRVAR(gpiod_Line_offset_doc,
 
 static PyObject *gpiod_Line_offset(gpiod_LineObject *self)
 {
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
        return Py_BuildValue("I", gpiod_line_offset(self->line));
 }
 
@@ -235,8 +239,12 @@ PyDoc_STRVAR(gpiod_Line_name_doc,
 
 static PyObject *gpiod_Line_name(gpiod_LineObject *self)
 {
-       const char *name = gpiod_line_name(self->line);
+       const char *name;
+
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
 
+       name = gpiod_line_name(self->line);
        if (name)
                return PyUnicode_FromFormat("%s", name);
 
@@ -248,8 +256,12 @@ PyDoc_STRVAR(gpiod_Line_consumer_doc,
 
 static PyObject *gpiod_Line_consumer(gpiod_LineObject *self)
 {
-       const char *consumer = gpiod_line_consumer(self->line);
+       const char *consumer;
 
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
+       consumer = gpiod_line_consumer(self->line);
        if (consumer)
                return PyUnicode_FromFormat("%s", consumer);
 
@@ -264,6 +276,9 @@ static PyObject *gpiod_Line_direction(gpiod_LineObject *self)
        PyObject *ret;
        int dir;
 
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
        dir = gpiod_line_direction(self->line);
 
        if (dir == GPIOD_LINE_DIRECTION_INPUT)
@@ -282,6 +297,9 @@ static PyObject *gpiod_Line_active_state(gpiod_LineObject *self)
        PyObject *ret;
        int active;
 
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
        active = gpiod_line_active_state(self->line);
 
        if (active == GPIOD_LINE_ACTIVE_STATE_HIGH)
@@ -297,6 +315,9 @@ PyDoc_STRVAR(gpiod_Line_is_used_doc,
 
 static PyObject *gpiod_Line_is_used(gpiod_LineObject *self)
 {
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
        if (gpiod_line_is_used(self->line))
                Py_RETURN_TRUE;
 
@@ -308,6 +329,9 @@ PyDoc_STRVAR(gpiod_Line_is_open_drain_doc,
 
 static PyObject *gpiod_Line_is_open_drain(gpiod_LineObject *self)
 {
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
        if (gpiod_line_is_open_drain(self->line))
                Py_RETURN_TRUE;
 
@@ -319,6 +343,9 @@ PyDoc_STRVAR(gpiod_Line_is_open_source_doc,
 
 static PyObject *gpiod_Line_is_open_source(gpiod_LineObject *self)
 {
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
        if (gpiod_line_is_open_source(self->line))
                Py_RETURN_TRUE;
 
@@ -349,6 +376,9 @@ PyDoc_STRVAR(gpiod_Line_is_requested_doc,
 
 static PyObject *gpiod_Line_is_requested(gpiod_LineObject *self)
 {
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
        if (gpiod_line_is_requested(self->line))
                Py_RETURN_TRUE;
 
@@ -461,6 +491,9 @@ static gpiod_LineEventObject *gpiod_Line_event_read(gpiod_LineObject *self)
        gpiod_LineEventObject *ret;
        int rv;
 
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
        ret = PyObject_New(gpiod_LineEventObject, &gpiod_LineEventType);
        if (!ret)
                return NULL;
@@ -489,6 +522,9 @@ static PyObject *gpiod_Line_event_get_fd(gpiod_LineObject *self)
 {
        int fd;
 
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
        fd = gpiod_line_event_get_fd(self->line);
        if (fd < 0) {
                PyErr_SetFromErrno(PyExc_OSError);
@@ -503,6 +539,9 @@ static PyObject *gpiod_Line_repr(gpiod_LineObject *self)
        PyObject *chip_name, *ret;
        const char *line_name;
 
+       if (gpiod_ChipIsClosed(self->owner))
+               return NULL;
+
        chip_name = gpiod_Chip_name(self->owner);
        if (!chip_name)
                return NULL;
@@ -638,6 +677,13 @@ static PyTypeObject gpiod_LineType = {
        .tp_methods = gpiod_Line_methods,
 };
 
+static bool gpiod_LineBulkOwnerIsClosed(gpiod_LineBulkObject *self)
+{
+       gpiod_LineObject *line = (gpiod_LineObject *)self->lines[0];
+
+       return gpiod_ChipIsClosed(line->owner);
+}
+
 static int gpiod_LineBulk_init(gpiod_LineBulkObject *self, PyObject *args)
 {
        PyObject *lines, *iter, *next;
@@ -830,6 +876,9 @@ static PyObject *gpiod_LineBulk_request(gpiod_LineBulkObject *self,
        char *consumer = NULL;
        Py_ssize_t i;
 
+       if (gpiod_LineBulkOwnerIsClosed(self))
+               return NULL;
+
        rv = PyArg_ParseTupleAndKeywords(args, kwds, "s|iiO", kwlist,
                                         &consumer, &type,
                                         &flags, &default_vals);
@@ -892,6 +941,9 @@ static PyObject *gpiod_LineBulk_get_values(gpiod_LineBulkObject *self)
        PyObject *val_list, *val;
        Py_ssize_t i;
 
+       if (gpiod_LineBulkOwnerIsClosed(self))
+               return NULL;
+
        gpiod_LineBulkObjToCLineBulk(self, &bulk);
 
        memset(vals, 0, sizeof(vals));
@@ -936,6 +988,9 @@ static PyObject *gpiod_LineBulk_set_values(gpiod_LineBulkObject *self,
        struct gpiod_line_bulk bulk;
        Py_ssize_t num_vals, i;
 
+       if (gpiod_LineBulkOwnerIsClosed(self))
+               return NULL;
+
        gpiod_LineBulkObjToCLineBulk(self, &bulk);
        memset(vals, 0, sizeof(vals));
 
@@ -987,6 +1042,9 @@ static PyObject *gpiod_LineBulk_release(gpiod_LineBulkObject *self)
 {
        struct gpiod_line_bulk bulk;
 
+       if (gpiod_LineBulkOwnerIsClosed(self))
+               return NULL;
+
        gpiod_LineBulkObjToCLineBulk(self, &bulk);
        gpiod_line_release_bulk(&bulk);
 
@@ -1011,6 +1069,9 @@ static PyObject *gpiod_LineBulk_event_wait(gpiod_LineBulkObject *self,
        Py_ssize_t i;
        int rv;
 
+       if (gpiod_LineBulkOwnerIsClosed(self))
+               return NULL;
+
        rv = PyArg_ParseTupleAndKeywords(args, kwds,
                                         "|ll", kwlist, &sec, &nsec);
        if (!rv)
@@ -1061,6 +1122,9 @@ static PyObject *gpiod_LineBulk_repr(gpiod_LineBulkObject *self)
        gpiod_LineObject *line;
        gpiod_ChipObject *chip;
 
+       if (gpiod_LineBulkOwnerIsClosed(self))
+               return NULL;
+
        list = gpiod_LineBulk_to_list(self);
        if (!list)
                return NULL;
@@ -1228,17 +1292,48 @@ static void gpiod_Chip_dealloc(gpiod_ChipObject *self)
 
 static PyObject *gpiod_Chip_repr(gpiod_ChipObject *self)
 {
+       if (gpiod_ChipIsClosed(self))
+               return NULL;
+
        return PyUnicode_FromFormat("'%s /%s/ %u lines'",
                                    gpiod_chip_name(self->chip),
                                    gpiod_chip_label(self->chip),
                                    gpiod_chip_num_lines(self->chip));
 }
 
+PyDoc_STRVAR(gpiod_Chip_close_doc,
+"Close the associated gpiochip descriptor.");
+
+static PyObject *gpiod_Chip_close(gpiod_ChipObject *self)
+{
+       if (gpiod_ChipIsClosed(self))
+               return NULL;
+
+       gpiod_chip_close(self->chip);
+       self->chip = NULL;
+
+       Py_RETURN_NONE;
+}
+
+static bool gpiod_ChipIsClosed(gpiod_ChipObject *chip)
+{
+       if (!chip->chip) {
+               PyErr_SetString(PyExc_ValueError,
+                               "I/O operation on closed file");
+               return true;
+       }
+
+       return false;
+}
+
 PyDoc_STRVAR(gpiod_Chip_name_doc,
 "Get the name of the GPIO chip");
 
 static PyObject *gpiod_Chip_name(gpiod_ChipObject *self)
 {
+       if (gpiod_ChipIsClosed(self))
+               return NULL;
+
        return PyUnicode_FromFormat("%s", gpiod_chip_name(self->chip));
 }
 
@@ -1247,6 +1342,9 @@ PyDoc_STRVAR(gpiod_Chip_label_doc,
 
 static PyObject *gpiod_Chip_label(gpiod_ChipObject *self)
 {
+       if (gpiod_ChipIsClosed(self))
+               return NULL;
+
        return PyUnicode_FromFormat("%s", gpiod_chip_label(self->chip));
 }
 
@@ -1255,6 +1353,9 @@ PyDoc_STRVAR(gpiod_Chip_num_lines_doc,
 
 static PyObject *gpiod_Chip_num_lines(gpiod_ChipObject *self)
 {
+       if (gpiod_ChipIsClosed(self))
+               return NULL;
+
        return Py_BuildValue("I", gpiod_chip_num_lines(self->chip));
 }
 
@@ -1284,6 +1385,9 @@ gpiod_Chip_get_line(gpiod_ChipObject *self, PyObject *args)
        unsigned int offset;
        int rv;
 
+       if (gpiod_ChipIsClosed(self))
+               return NULL;
+
        rv = PyArg_ParseTuple(args, "I", &offset);
        if (!rv)
                return NULL;
@@ -1309,6 +1413,9 @@ gpiod_Chip_find_line(gpiod_ChipObject *self, PyObject *args)
        const char *name;
        int rv;
 
+       if (gpiod_ChipIsClosed(self))
+               return NULL;
+
        rv = PyArg_ParseTuple(args, "s", &name);
        if (!rv)
                return NULL;
@@ -1442,6 +1549,9 @@ gpiod_Chip_get_all_lines(gpiod_ChipObject *self)
        PyObject *list;
        int rv;
 
+       if (gpiod_ChipIsClosed(self))
+               return NULL;
+
        rv = gpiod_chip_get_all_lines(self->chip, &bulk);
        if (rv) {
                PyErr_SetFromErrno(PyExc_OSError);
@@ -1548,6 +1658,12 @@ gpiod_Chip_find_lines(gpiod_ChipObject *self, PyObject *args)
 }
 
 static PyMethodDef gpiod_Chip_methods[] = {
+       {
+               .ml_name = "close",
+               .ml_meth = (PyCFunction)gpiod_Chip_close,
+               .ml_flags = METH_NOARGS,
+               .ml_doc = gpiod_Chip_close_doc,
+       },
        {
                .ml_name = "name",
                .ml_meth = (PyCFunction)gpiod_Chip_name,
@@ -1680,6 +1796,9 @@ static int gpiod_LineIter_init(gpiod_LineIterObject *self, PyObject *args)
        if (!rv)
                return -1;
 
+       if (gpiod_ChipIsClosed(chip_obj))
+               return -1;
+
        Py_BEGIN_ALLOW_THREADS;
        self->iter = gpiod_line_iter_new(chip_obj->chip);
        Py_END_ALLOW_THREADS;