console-server: refactor pollfd management
Instead of the previous memory management of pollfd array,
with a fixed and flexible part and hardcoded indices,
provide two functions to request and release pollfds.
- console_server_request_pollfd
- console_server_release_pollfd
The pollfds are still in the same array but can be requested and
released by these functions now. struct console_server and
struct console now contain indices into that array of pollfds.
The benefit of this contribution is that the new interface provides a
clean allocator-like abstraction for requesting and releasing pollfds,
which will scale to multiple consoles and can be refactored or
unit-tested more easily in the future.
The previous implementation was tightly coupled to the single-console
use-case and the pollfds stored at hardcoded indices.
Change-Id: I93226699618130b175bffbeb4f71c20c91a7083a
Signed-off-by: Alexander Hansen <alexander.hansen@9elements.com>
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
diff --git a/console-dbus.c b/console-dbus.c
index 220d3e0..ba8356d 100644
--- a/console-dbus.c
+++ b/console-dbus.c
@@ -166,7 +166,6 @@
{
char obj_name[dbus_obj_path_len];
char dbus_name[dbus_obj_path_len];
- int dbus_poller = 0;
int fd;
int r;
size_t bytes;
@@ -234,10 +233,13 @@
return -1;
}
- dbus_poller = POLLFD_DBUS;
+ const ssize_t index =
+ console_server_request_pollfd(console->server, fd, POLLIN);
+ if (index < 0) {
+ fprintf(stderr, "Error: failed to allocate pollfd\n");
+ return -1;
+ }
- console->pollfds[dbus_poller].fd = fd;
- console->pollfds[dbus_poller].events = POLLIN;
-
+ console->dbus_pollfd_index = index;
return 0;
}
diff --git a/console-server.c b/console-server.c
index fa61e71..b7a046b 100644
--- a/console-server.c
+++ b/console-server.c
@@ -61,6 +61,76 @@
progname);
}
+static bool console_server_pollfd_reclaimable(struct pollfd *p)
+{
+ return p->fd == -1 && p->events == 0 && p->revents == ~0;
+}
+
+static ssize_t
+console_server_find_released_pollfd(struct console_server *server)
+{
+ for (size_t i = 0; i < server->capacity_pollfds; i++) {
+ struct pollfd *p = &server->pollfds[i];
+ if (console_server_pollfd_reclaimable(p)) {
+ return (ssize_t)i;
+ }
+ }
+ return -1;
+}
+
+// returns the index of that pollfd in server->pollfds
+// we cannot return a pointer because 'realloc' may move server->pollfds
+ssize_t console_server_request_pollfd(struct console_server *server, int fd,
+ short int events)
+{
+ ssize_t index;
+ struct pollfd *pollfd;
+
+ index = console_server_find_released_pollfd(server);
+
+ if (index < 0) {
+ const size_t newcap = server->capacity_pollfds + 1;
+
+ struct pollfd *newarr = reallocarray(server->pollfds, newcap,
+ sizeof(struct pollfd));
+ if (newarr == NULL) {
+ return -1;
+ }
+ server->pollfds = newarr;
+
+ index = (ssize_t)server->capacity_pollfds;
+
+ server->capacity_pollfds = newcap;
+ }
+
+ pollfd = &server->pollfds[index];
+ pollfd->fd = fd;
+ pollfd->events = events;
+ pollfd->revents = 0;
+
+ return index;
+}
+
+int console_server_release_pollfd(struct console_server *server,
+ size_t pollfd_index)
+{
+ if (pollfd_index >= server->capacity_pollfds) {
+ return -1;
+ }
+
+ struct pollfd *pfd = &server->pollfds[pollfd_index];
+
+ // mark pollfd as reclaimable
+
+ // ignore this file descriptor when calling 'poll'
+ // https://www.man7.org/linux/man-pages/man2/poll.2.html
+ pfd->fd = -1;
+ pfd->events = 0;
+ pfd->revents = ~0;
+
+ return 0;
+}
+
/* populates server->tty.dev and server->tty.sysfs_devnode, using the tty kernel name */
static int tty_find_device(struct console_server *server)
{
@@ -285,8 +355,14 @@
tty_init_termios(server);
- server->active->pollfds[server->active->n_pollers].fd = server->tty.fd;
- server->active->pollfds[server->active->n_pollers].events = POLLIN;
+ ssize_t index =
+ console_server_request_pollfd(server, server->tty.fd, POLLIN);
+
+ if (index < 0) {
+ return -1;
+ }
+
+ server->tty_pollfd_index = (size_t)index;
return 0;
}
@@ -393,9 +469,15 @@
static void tty_fini(struct console_server *server)
{
+ if (server->tty_pollfd_index < server->capacity_pollfds) {
+ console_server_release_pollfd(server, server->tty_pollfd_index);
+ server->tty_pollfd_index = SIZE_MAX;
+ }
+
if (server->tty.type == TTY_DEVICE_VUART) {
free(server->tty.vuart.sysfs_devnode);
}
+
free(server->tty.dev);
}
@@ -629,13 +711,22 @@
struct poller *poller;
long n;
+ const ssize_t index = console_server_request_pollfd(
+ console->server, fd, (short)(events & 0x7fff));
+ if (index < 0) {
+ fprintf(stderr, "Error requesting pollfd\n");
+ return NULL;
+ }
+
poller = malloc(sizeof(*poller));
+ // TODO: check for error case of malloc here and release previously requested pollfd
poller->remove = false;
poller->handler = handler;
poller->event_fn = poller_fn;
poller->timeout_fn = timeout_fn;
timerclear(&poller->timeout);
poller->data = data;
+ poller->pollfd_index = index;
/* add one to our pollers array */
n = console->n_pollers++;
@@ -646,22 +737,11 @@
/* NOLINTBEGIN(bugprone-sizeof-expression) */
console->pollers = reallocarray(console->pollers, console->n_pollers,
sizeof(*console->pollers));
+ // TODO: check for the error case of reallocarray and release previously requested pollfd
/* NOLINTEND(bugprone-sizeof-expression) */
console->pollers[n] = poller;
- /* increase pollfds array too */
- console->pollfds = reallocarray(
- console->pollfds, (MAX_INTERNAL_POLLFD + console->n_pollers),
- sizeof(*console->pollfds));
-
- /* shift the end pollfds up by one */
- memcpy(&console->pollfds[n + 1], &console->pollfds[n],
- sizeof(*console->pollfds) * MAX_INTERNAL_POLLFD);
-
- console->pollfds[n].fd = fd;
- console->pollfds[n].events = (short)(events & 0x7fff);
-
return poller;
}
@@ -700,14 +780,7 @@
}
/* NOLINTEND(bugprone-sizeof-expression) */
- /* ... and the pollfds array */
- memmove(&console->pollfds[i], &console->pollfds[i + 1],
- sizeof(*console->pollfds) *
- (MAX_INTERNAL_POLLFD + console->n_pollers - i));
-
- console->pollfds = reallocarray(
- console->pollfds, (MAX_INTERNAL_POLLFD + console->n_pollers),
- sizeof(*console->pollfds));
+ console_server_release_pollfd(console->server, poller->pollfd_index);
free(poller);
}
@@ -715,16 +788,8 @@
void console_poller_set_events(struct console *console, struct poller *poller,
int events)
{
- int i;
-
- /* find the entry in our pollers array */
- for (i = 0; i < console->n_pollers; i++) {
- if (console->pollers[i] == poller) {
- break;
- }
- }
-
- console->pollfds[i].events = (short)(events & 0x7fff);
+ console->server->pollfds[poller->pollfd_index].events =
+ (short)(events & 0x7fff);
}
void console_poller_set_timeout(struct console *console __attribute__((unused)),
@@ -792,7 +857,12 @@
*/
for (i = 0; i < console->n_pollers; i++) {
poller = console->pollers[i];
- pollfd = &console->pollfds[i];
+ pollfd = &console->server->pollfds[poller->pollfd_index];
+ if (pollfd->fd < 0) {
+ // pollfd has already been released
+ continue;
+ }
+
prc = POLLER_OK;
/* process pending events... */
@@ -879,7 +949,7 @@
timeout = get_poll_timeout(console, &tv);
- rc = poll(console->pollfds, console->n_pollers + MAX_INTERNAL_POLLFD,
+ rc = poll(console->server->pollfds, console->server->capacity_pollfds,
(int)timeout);
if (rc < 0) {
@@ -891,7 +961,7 @@
}
/* process internal fd first */
- if (console->pollfds[console->n_pollers].revents) {
+ if (console->server->pollfds[console->server->tty_pollfd_index].revents) {
rc = read(console->server->tty.fd, buf, sizeof(buf));
if (rc <= 0) {
warn("Error reading from tty device");
@@ -903,7 +973,7 @@
}
}
- if (console->pollfds[console->n_pollers + 1].revents) {
+ if (console->server->pollfds[console->dbus_pollfd_index].revents) {
sd_bus_process(console->bus, NULL);
}
@@ -993,8 +1063,8 @@
server.active = console;
console->server = &server;
- console->pollfds =
- calloc(MAX_INTERNAL_POLLFD, sizeof(*console->pollfds));
+ server.pollfds = NULL;
+ server.capacity_pollfds = 0;
buffer_size_str = config_get_value(config, "ringbuffer-size");
if (buffer_size_str) {
@@ -1041,7 +1111,7 @@
out_console_fini:
free(console->pollers);
- free(console->pollfds);
+ free(server.pollfds);
free(console);
out_config_fini:
diff --git a/console-server.h b/console-server.h
index 1b7c15c..5213dd0 100644
--- a/console-server.h
+++ b/console-server.h
@@ -115,6 +115,15 @@
};
} tty;
+ // All the pollfds are stored here,
+ // so 'poll' can operate on them.
+ // The other 'pollfd*' are just pointers to this array.
+ struct pollfd *pollfds;
+ size_t capacity_pollfds;
+
+ // index into pollfds
+ size_t tty_pollfd_index;
+
struct console *active;
};
@@ -137,7 +146,9 @@
struct poller **pollers;
long n_pollers;
- struct pollfd *pollfds;
+ // index into (struct console_server)->pollfds
+ size_t dbus_pollfd_index;
+
struct sd_bus *bus;
};
@@ -149,13 +160,9 @@
poller_timeout_fn_t timeout_fn;
struct timeval timeout;
bool remove;
-};
-/* we have two extra entry in the pollfds array for the VUART tty */
-enum internal_pollfds {
- POLLFD_HOSTTTY = 0,
- POLLFD_DBUS = 1,
- MAX_INTERNAL_POLLFD = 2,
+ // index into (struct console_server)->pollfds
+ size_t pollfd_index;
};
struct poller *console_poller_register(struct console *console,
@@ -253,3 +260,11 @@
} while (0)
#define BUILD_ASSERT_OR_ZERO(c) (sizeof(char[(c) ? 1 : -1]) - 1)
+
+// returns the index of that pollfd in server->pollfds
+// we cannot return a pointer because 'realloc' may move server->pollfds
+ssize_t console_server_request_pollfd(struct console_server *server, int fd,
+ short int events);
+
+int console_server_release_pollfd(struct console_server *server,
+ size_t pollfd_index);