core: use reference counting for line file descriptors

In v0.x series we were using a separate, refcounted structure for
storing the file descriptor returned by the linehandle request ioctl().
We also had a separate mechanism for event file descriptors. This was
quite complicated and was replaced by a simpler structure in v1.x
series.

It turned out however that we're now calling close() on already closed
descriptors when releasing a set of simultaneously requested lines.

In order to fix that we need to go back to having a refcounted file
descriptor handle. This time we're using the same structure for storing
both values and events file descriptors (even though there can't be
more than one line per descriptor requested for events) for simplicity.

This patch also adds a test case that verifies the reference counting
works as expected.

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

diff --git a/src/lib/core.c b/src/lib/core.c
index 5a7e9ff8bf36f289c8bec35962e03a870d3ff7e4..8b89517f166d277ad374053d248d0510f9535ef5 100644 (file)
--- a/src/lib/core.c
+++ b/src/lib/core.c
@@ -26,6 +26,11 @@ enum {
        LINE_REQUESTED_EVENTS,
 };
 
+struct line_fd_handle {
+       int fd;
+       int refcount;
+};
+
 struct gpiod_line {
        unsigned int offset;
        int direction;
@@ -38,7 +43,7 @@ struct gpiod_line {
        bool up_to_date;
 
        struct gpiod_chip *chip;
-       int fd;
+       struct line_fd_handle *fd_handle;
 
        char name[32];
        char consumer[32];
@@ -164,7 +169,7 @@ gpiod_chip_get_line(struct gpiod_chip *chip, unsigned int offset)
                        return NULL;
 
                memset(line, 0, sizeof(*line));
-               line->fd = -1;
+               line->fd_handle = NULL;
 
                line->offset = offset;
                line->chip = chip;
@@ -181,6 +186,49 @@ gpiod_chip_get_line(struct gpiod_chip *chip, unsigned int offset)
        return line;
 }
 
+static struct line_fd_handle *line_make_fd_handle(int fd)
+{
+       struct line_fd_handle *handle;
+
+       handle = malloc(sizeof(*handle));
+       if (!handle)
+               return NULL;
+
+       handle->fd = fd;
+       handle->refcount = 0;
+
+       return handle;
+}
+
+static void line_fd_incref(struct gpiod_line *line)
+{
+       line->fd_handle->refcount++;
+}
+
+static void line_fd_decref(struct gpiod_line *line)
+{
+       struct line_fd_handle *handle = line->fd_handle;
+
+       handle->refcount--;
+
+       if (handle->refcount == 0) {
+               close(handle->fd);
+               free(handle);
+               line->fd_handle = NULL;
+       }
+}
+
+static void line_set_fd(struct gpiod_line *line, struct line_fd_handle *handle)
+{
+       line->fd_handle = handle;
+       line_fd_incref(line);
+}
+
+static int line_get_fd(struct gpiod_line *line)
+{
+       return line->fd_handle->fd;
+}
+
 static void line_maybe_update(struct gpiod_line *line)
 {
        int status;
@@ -329,6 +377,7 @@ static int line_request_values(struct gpiod_line_bulk *bulk,
                               const int *default_vals)
 {
        struct gpiod_line *line, **lineptr;
+       struct line_fd_handle *line_fd;
        struct gpiohandle_request req;
        unsigned int i;
        int rv, fd;
@@ -381,9 +430,13 @@ static int line_request_values(struct gpiod_line_bulk *bulk,
        if (rv < 0)
                return -1;
 
+       line_fd = line_make_fd_handle(req.fd);
+       if (!line_fd)
+               return -1;
+
        gpiod_line_bulk_foreach_line(bulk, line, lineptr) {
                line->state = LINE_REQUESTED_VALUES;
-               line->fd = req.fd;
+               line_set_fd(line, line_fd);
                line_maybe_update(line);
        }
 
@@ -393,6 +446,7 @@ static int line_request_values(struct gpiod_line_bulk *bulk,
 static int line_request_event_single(struct gpiod_line *line,
                        const struct gpiod_line_request_config *config)
 {
+       struct line_fd_handle *line_fd;
        struct gpioevent_request req;
        int rv;
 
@@ -423,8 +477,12 @@ static int line_request_event_single(struct gpiod_line *line,
        if (rv < 0)
                return -1;
 
+       line_fd = line_make_fd_handle(req.fd);
+       if (!line_fd)
+               return -1;
+
        line->state = LINE_REQUESTED_EVENTS;
-       line->fd = req.fd;
+       line_set_fd(line, line_fd);
        line_maybe_update(line);
 
        return 0;
@@ -510,7 +568,7 @@ void gpiod_line_release_bulk(struct gpiod_line_bulk *bulk)
 
        gpiod_line_bulk_foreach_line(bulk, line, lineptr) {
                if (line->state != LINE_FREE) {
-                       close(line->fd);
+                       line_fd_decref(line);
                        line->state = LINE_FREE;
                }
        }
@@ -556,7 +614,7 @@ int gpiod_line_get_value_bulk(struct gpiod_line_bulk *bulk, int *values)
 
        memset(&data, 0, sizeof(data));
 
-       fd = first->fd;
+       fd = line_get_fd(first);
 
        status = ioctl(fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
        if (status < 0)
@@ -583,7 +641,7 @@ int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk, const int *values)
        struct gpiohandle_data data;
        struct gpiod_line *line;
        unsigned int i;
-       int status;
+       int status, fd;
 
        if (!line_bulk_same_chip(bulk) || !line_bulk_all_requested(bulk))
                return -1;
@@ -594,7 +652,9 @@ int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk, const int *values)
                data.values[i] = (uint8_t)!!values[i];
 
        line = gpiod_line_bulk_get_line(bulk, 0);
-       status = ioctl(line->fd, GPIOHANDLE_SET_LINE_VALUES_IOCTL, &data);
+       fd = line_get_fd(line);
+
+       status = ioctl(fd, GPIOHANDLE_SET_LINE_VALUES_IOCTL, &data);
        if (status < 0)
                return -1;
 
@@ -628,7 +688,7 @@ int gpiod_line_event_wait_bulk(struct gpiod_line_bulk *bulk,
        num_lines = gpiod_line_bulk_num_lines(bulk);
 
        gpiod_line_bulk_foreach_line_off(bulk, line, off) {
-               fds[off].fd = line->fd;
+               fds[off].fd = line_get_fd(line);
                fds[off].events = POLLIN | POLLPRI;
        }
 
@@ -657,12 +717,16 @@ int gpiod_line_event_wait_bulk(struct gpiod_line_bulk *bulk,
 int gpiod_line_event_read(struct gpiod_line *line,
                          struct gpiod_line_event *event)
 {
+       int fd;
+
        if (line->state != LINE_REQUESTED_EVENTS) {
                errno = EPERM;
                return -1;
        }
 
-       return gpiod_line_event_read_fd(line->fd, event);
+       fd = line_get_fd(line);
+
+       return gpiod_line_event_read_fd(fd, event);
 }
 
 int gpiod_line_event_get_fd(struct gpiod_line *line)
@@ -672,7 +736,7 @@ int gpiod_line_event_get_fd(struct gpiod_line *line)
                return -1;
        }
 
-       return line->fd;
+       return line_get_fd(line);
 }
 
 int gpiod_line_event_read_fd(int fd, struct gpiod_line_event *event)

diff --git a/tests/tests-line.c b/tests/tests-line.c
index 0c492190f217ea8e5fe8c3d4b5a78f6006116d36..1b73b86d09b0c9fbd751a1933c0ce1e86423ae4a 100644 (file)
--- a/tests/tests-line.c
+++ b/tests/tests-line.c
@@ -570,6 +570,45 @@ TEST_DEFINE(line_open_source_open_drain_simultaneously,
            "gpiod_line - open-source & open-drain flags simultaneously",
            0, { 8 });
 
+/* Verify that the reference counting of the line fd handle works correctly. */
+static void line_release_one_use_another(void)
+{
+       TEST_CLEANUP_CHIP struct gpiod_chip *chip = NULL;
+       struct gpiod_line_bulk bulk;
+       struct gpiod_line *line1;
+       struct gpiod_line *line2;
+       int rv, vals[2];
+
+       chip = gpiod_chip_open(test_chip_path(0));
+       TEST_ASSERT_NOT_NULL(chip);
+
+       line1 = gpiod_chip_get_line(chip, 2);
+       TEST_ASSERT_NOT_NULL(line1);
+       line2 = gpiod_chip_get_line(chip, 3);
+       TEST_ASSERT_NOT_NULL(line2);
+
+       gpiod_line_bulk_init(&bulk);
+       gpiod_line_bulk_add(&bulk, line1);
+       gpiod_line_bulk_add(&bulk, line2);
+
+       vals[0] = vals[1] = 1;
+
+       rv = gpiod_line_request_bulk_output(&bulk, TEST_CONSUMER, vals);
+       TEST_ASSERT_RET_OK(rv);
+
+       gpiod_line_release(line1);
+
+       rv = gpiod_line_get_value(line1);
+       TEST_ASSERT_EQ(rv, -1);
+       TEST_ASSERT_ERRNO_IS(EPERM);
+
+       rv = gpiod_line_get_value(line2);
+       TEST_ASSERT_EQ(rv, 1);
+}
+TEST_DEFINE(line_release_one_use_another,
+           "gpiod_line - request two, release one, use the other one",
+           0, { 8 });
+
 static void line_null_consumer(void)
 {
        TEST_CLEANUP_CHIP struct gpiod_chip *chip = NULL;