On Tue, Sep 03, 2024 at 09:25:42PM +0200, Stefano Brivio wrote:On Tue, 3 Sep 2024 22:02:32 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Good catch. I've re-organised to avoid that.tap_pasta_input() has a rather confusing structure, using two gotos. Remove these by restructuring the function to have the main loop condition based on filling our buffer space, with errors or running out of data treated as the exception, rather than the other way around. This allows us to handle the EINTR which triggered the 'restart' goto with a continue. The outer 'redo' was triggered if we completely filled our buffer, to flush it and do another pass. This one is unnecessary since we don't (yet) use EPOLLET on the tap device: if there's still more data we'll get another event and re-enter the loop. Along the way handle a couple of extra edge cases: - Check for EWOULDBLOCK as well as EAGAIN for the benefit of any future ports where those might not have the same value - Detect EOF on the tap device and exit in that case Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- tap.c | 42 +++++++++++++++++------------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/tap.c b/tap.c index 9ee59faa..71742748 100644 --- a/tap.c +++ b/tap.c @@ -1073,42 +1073,34 @@ void tap_handler_passt(struct ctx *c, uint32_t events, static void tap_pasta_input(struct ctx *c, const struct timespec *now) { ssize_t n, len; - int ret; - -redo: - n = 0; tap_flush_pools(); -restart: - while ((len = read(c->fd_tap, pkt_buf + n, TAP_BUF_BYTES - n)) > 0) { - if (len < (ssize_t)sizeof(struct ethhdr) || - len > (ssize_t)ETH_MAX_MTU) { - n += len; - continue; + for (n = 0; n < (ssize_t)TAP_BUF_BYTES; n += len) { + len = read(c->fd_tap, pkt_buf + n, TAP_BUF_BYTES - n); + + if (len <= 0) { + if (errno == EINTR) {You might be checking errno when read() returns 0, in which case it's not set. I guess you should zero out errno before read() if you want to keep this, or, alternatively, directly handle len == 0 here.-- David Gibson (he or they) | 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/~dgibson+ len = 0; + continue; + } + break; } + /* Ignore frames of bad length */ + if (len < (ssize_t)sizeof(struct ethhdr) || + len > (ssize_t)ETH_MAX_MTU) + continue; tap_add_packet(c, len, pkt_buf + n); - - if ((n += len) == TAP_BUF_BYTES) - break; } - if (len < 0 && errno == EINTR) - goto restart; - - ret = errno; + if (len < 0 && errno != EAGAIN && errno != EWOULDBLOCK) + die("Error on tap device, exiting"); + else if (len == 0) + die("EOF on tap device, exiting"); tap_handler(c, now); - - if (len > 0 || ret == EAGAIN) - return; - - if (n == TAP_BUF_BYTES) - goto redo; - - die("Error on tap device, exiting"); } /**