From: Bartosz Golaszewski Date: Thu, 12 Apr 2018 10:28:18 +0000 (+0200) Subject: core: use reference counting for line file descriptors X-Git-Url: http://git.maquefel.me/?a=commitdiff_plain;h=5ab6ccbd4939ad3076df95f822953c885522c3d2;p=qemu-gpiodev%2Flibgpiod.git 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 --- diff --git a/src/lib/core.c b/src/lib/core.c index 5a7e9ff..8b89517 100644 --- 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 0c49219..1b73b86 100644 --- 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;