On Thu, Aug 10, 2023 at 09:50:55AM +0200, Stefano Brivio wrote:On Thu, 10 Aug 2023 11:08:19 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Ah, right, I missed that. Since I have a few other minor reasons to do a respin anyway, I've removed EPOLLERR for consistency with the other places, and will send that in v3. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibsonOn Wed, Aug 09, 2023 at 09:59:27PM +0200, Stefano Brivio wrote:There's no need to subscribe to EPOLLERR (and EPOLLHUP): both types of events are always reported. I guess it's also fine to indicate it explicitly in 2/13 v2 as you did, to remark that we handle it, but if you look around we never add EPOLLERR or EPOLLHUP (except for a stray occurrence in udp_splice_new()) to the set of events.On Mon, 7 Aug 2023 23:46:30 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:So, I don't think we'll spin, because we set EPOLLET (edge triggered). That said, EPOLLRDHUP makes no sense on a listening socket, and we're not subscribed to EPOLLERRtap_handler() actually handles events on three different types of object: the /dev/tap character device (pasta), a connected Unix domain socket (passt) or a listening Unix domain socket (passt). The last, in particular, really has no handling in common with the others, so split it into its own epoll type and directly dispatch to the relevant handler from the top level. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- passt.c | 6 +++++- passt.h | 6 ++++-- tap.c | 20 ++++++++------------ tap.h | 4 ++-- 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/passt.c b/passt.c index c32981d..c60f346 100644 --- a/passt.c +++ b/passt.c @@ -64,6 +64,7 @@ char *epoll_type_str[EPOLL_TYPE_MAX+1] = { [EPOLL_TYPE_ICMPV6] = "ICMPv6 socket", [EPOLL_TYPE_NSQUIT] = "namespace inotify", [EPOLL_TYPE_TAP] = "tap device", + [EPOLL_TYPE_TAP_LISTEN] = "listening qemu socket", }; /** @@ -317,7 +318,10 @@ loop: switch (ref.type) { case EPOLL_TYPE_TAP: - tap_handler(&c, ref.fd, events[i].events, &now); + tap_handler(&c, events[i].events, &now); + break; + case EPOLL_TYPE_TAP_LISTEN: + tap_listen_handler(&c, eventmask); break; case EPOLL_TYPE_NSQUIT: pasta_netns_quit_handler(&c, quit_fd); diff --git a/passt.h b/passt.h index 176bc85..7dae08b 100644 --- a/passt.h +++ b/passt.h @@ -61,10 +61,12 @@ enum epoll_type { EPOLL_TYPE_ICMPV6, /* inotify fd watching for end of netns (pasta) */ EPOLL_TYPE_NSQUIT, - /* tap char device, or qemu socket fd */ + /* tap char device, or connected qemu socket fd */ EPOLL_TYPE_TAP, + /* socket listening for qemu socket connections */ + EPOLL_TYPE_TAP_LISTEN, - EPOLL_TYPE_MAX = EPOLL_TYPE_TAP, + EPOLL_TYPE_MAX = EPOLL_TYPE_TAP_LISTEN, }; /** diff --git a/tap.c b/tap.c index ad0decf..8468f86 100644 --- a/tap.c +++ b/tap.c @@ -1076,7 +1076,7 @@ restart: static void tap_sock_unix_init(struct ctx *c) { int fd = socket(AF_UNIX, SOCK_STREAM, 0); - union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; + union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN }; struct epoll_event ev = { 0 }; struct sockaddr_un addr = { .sun_family = AF_UNIX, @@ -1142,10 +1142,11 @@ static void tap_sock_unix_init(struct ctx *c) } /** - * tap_sock_unix_new() - Handle new connection on listening socket + * tap_listen_handler() - Handle new connection on listening socket * @c: Execution context + * @events: epoll events on the socket */ -static void tap_sock_unix_new(struct ctx *c) +void tap_listen_handler(struct ctx *c, uint32_t events) { union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; struct epoll_event ev = { 0 }; @@ -1153,6 +1154,9 @@ static void tap_sock_unix_new(struct ctx *c) struct ucred ucred; socklen_t len; + if (events != EPOLLIN) + return; +This comment actually belongs to 2/4 of the tap reset clean-up series, but posting it here for clarity: ...wouldn't it be appropriate to handle errors while at it? Otherwise I guess we'll just spin on EPOLLERR or EPOLLRDHUP.