I ran into some mildly confusing stuff in the tap reset path while working on the epoll rework. Here's a couple of fixes for it. David Gibson (2): tap: Clean up tap reset path tap: More sensible behaviour for error on listening qemu socket tap.c | 57 ++++++++++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 25 deletions(-) -- 2.41.0
In tap_handler() if we get an error on the tap device or socket, we use tap_sock_init() to re-initialise it. However, what we actually need for this reset case has remarkably little in common with the case where we're initialising for the first time: * Re-initialising the packet pools is unnecessary * The case of a passed in fd (--fd) isn't relevant * We don't even call this for pasta mode * We will never re-call tap_sock_unix_init() because we never clear fd_tap_listen In fact the only thing we do in tap_sock_init() relevant to the reset case is to remove the fd from the epoll and close it... which isn't used in the first initialisation case. So make a new tap_sock_reset() function just for this case, and simplify tap_sock_init() slightly as being used only for the first time case. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- tap.c | 52 +++++++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/tap.c b/tap.c index a6a73d3..3a43263 100644 --- a/tap.c +++ b/tap.c @@ -1233,19 +1233,14 @@ void tap_sock_init(struct ctx *c) tap6_l4[i].p = PACKET_INIT(pool_l4, TAP_SEQS, pkt_buf, sz); } - if (c->fd_tap != -1) { - if (c->one_off) { /* Passed as --fd */ - struct epoll_event ev = { 0 }; - - ev.data.fd = c->fd_tap; - ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP; - epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); - return; - } + if (c->fd_tap != -1) { /* Passed as --fd */ + struct epoll_event ev = { 0 }; + ASSERT(c->one_off); - epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); - close(c->fd_tap); - c->fd_tap = -1; + ev.data.fd = c->fd_tap; + ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP; + epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); + return; } if (c->mode == MODE_PASST) { @@ -1256,6 +1251,26 @@ void tap_sock_init(struct ctx *c) } } +/** + * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket + * @c: Execution context + */ +static void tap_sock_reset(struct ctx *c) +{ + if (c->one_off) { + info("Client closed connection, exiting"); + exit(EXIT_SUCCESS); + } + + if (c->mode == MODE_PASTA) + die("Error on tap device, exiting"); + + /* Close the connected socket, wait for a new connection */ + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); + close(c->fd_tap); + c->fd_tap = -1; +} + /** * tap_handler() - Packet handler for AF_UNIX or tuntap file descriptor * @c: Execution context @@ -1273,15 +1288,6 @@ void tap_handler(struct ctx *c, int fd, uint32_t events, if ((c->mode == MODE_PASST && tap_handler_passt(c, now)) || (c->mode == MODE_PASTA && tap_handler_pasta(c, now)) || - (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) { - if (c->one_off) { - info("Client closed connection, exiting"); - exit(EXIT_SUCCESS); - } - - if (c->mode == MODE_PASTA) - die("Error on tap device, exiting"); - - tap_sock_init(c); - } + (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) + tap_sock_reset(c); } -- 2.41.0
We call tap_sock_unix_new() to handle a new connection to the qemu socket if we get an EPOLLIN event on c->fd_tap_listen. If we get an error event on c->fd_tap_listen instead, we'll fall through to the "tap reset" path. However, this won't do anything useful for an error on the listening socket, it will just close the already connected socket. If we wanted to handle errors on this socket, we'd need to do something different than for an error on the connected socket. So, change the logic to explicitly do nothing for any !EPOLLIN events on the listening socket. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- tap.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tap.c b/tap.c index 3a43263..a8e5ce6 100644 --- a/tap.c +++ b/tap.c @@ -1281,8 +1281,9 @@ static void tap_sock_reset(struct ctx *c) void tap_handler(struct ctx *c, int fd, uint32_t events, const struct timespec *now) { - if (fd == c->fd_tap_listen && events == EPOLLIN) { - tap_sock_unix_new(c); + if (fd == c->fd_tap_listen) { + if (events == EPOLLIN) + tap_sock_unix_new(c); return; } -- 2.41.0