From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202408 header.b=dPLHJXb1; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id EBDE35A004F for ; Wed, 04 Sep 2024 04:25:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202408; t=1725416717; bh=9VNj+bnFmdxmgt486nBq22AvLf9jCmol+r1EzWfiGOI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=dPLHJXb1RYGjyq/H+yr+xNPx2IIN0eqVyJ7Aq3u/Es7iV00dX77GgtWXh2axLENGU FKZNlMKaYp82BXUszN5Y3K7UChFXwelcgMQTTrZsn5B0jrpDkN8Xf4pXXJcR5mMRVU Klyc7f+nHCJkzVoNVp5JPb27fw+n4LdbYbB9hiSj31he4n2YTF1hcFLZQ2RkHwNvfy eNFT7PARp5Rmam/px4wOGX35LGshzOMZ6eq7DRr1Us5dB5g8in+wQjMsJcvm91bS1+ lHDcUDbZzPtKa08TU8p9Fq6/tXKuMVtgxsFOobxCsCN6Rjxaxt5+y5Gm/xR8jBwh2Y aWuF00bhiCHzw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Wz5tj23wzz4xCW; Wed, 4 Sep 2024 12:25:17 +1000 (AEST) Date: Wed, 4 Sep 2024 11:33:13 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 3/6] tap: Restructure in tap_pasta_input() Message-ID: References: <20240903120235.1688429-1-david@gibson.dropbear.id.au> <20240903120235.1688429-4-david@gibson.dropbear.id.au> <20240903212542.4934f5a6@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="rDVM/ALT/8JMCeBk" Content-Disposition: inline In-Reply-To: <20240903212542.4934f5a6@elisabeth> Message-ID-Hash: RYSQN4N4VGZQ4TLXZYC6DBCMYPDPTCCW X-Message-ID-Hash: RYSQN4N4VGZQ4TLXZYC6DBCMYPDPTCCW X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --rDVM/ALT/8JMCeBk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 03, 2024 at 09:25:42PM +0200, Stefano Brivio wrote: > On Tue, 3 Sep 2024 22:02:32 +1000 > David Gibson wrote: >=20 > > tap_pasta_input() has a rather confusing structure, using two gotos. > > Remove these by restructuring the function to have the main loop condit= ion > > 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 contin= ue. > >=20 > > The outer 'redo' was triggered if we completely filled our buffer, to f= lush > > it and do another pass. This one is unnecessary since we don't (yet) u= se > > EPOLLET on the tap device: if there's still more data we'll get another > > event and re-enter the loop. > >=20 > > 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 > >=20 > > Signed-off-by: David Gibson > > --- > > tap.c | 42 +++++++++++++++++------------------------- > > 1 file changed, 17 insertions(+), 25 deletions(-) > >=20 > > 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 =3D 0; > > =20 > > tap_flush_pools(); > > -restart: > > - while ((len =3D read(c->fd_tap, pkt_buf + n, TAP_BUF_BYTES - n)) > 0)= { > > =20 > > - if (len < (ssize_t)sizeof(struct ethhdr) || > > - len > (ssize_t)ETH_MAX_MTU) { > > - n +=3D len; > > - continue; > > + for (n =3D 0; n < (ssize_t)TAP_BUF_BYTES; n +=3D len) { > > + len =3D read(c->fd_tap, pkt_buf + n, TAP_BUF_BYTES - n); > > + > > + if (len <=3D 0) { > > + if (errno =3D=3D EINTR) { >=20 > 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 =3D=3D 0 here. Good catch. I've re-organised to avoid that. >=20 > > + len =3D 0; > > + continue; > > + } > > + break; > > } > > =20 > > + /* Ignore frames of bad length */ > > + if (len < (ssize_t)sizeof(struct ethhdr) || > > + len > (ssize_t)ETH_MAX_MTU) > > + continue; > > =20 > > tap_add_packet(c, len, pkt_buf + n); > > - > > - if ((n +=3D len) =3D=3D TAP_BUF_BYTES) > > - break; > > } > > =20 > > - if (len < 0 && errno =3D=3D EINTR) > > - goto restart; > > - > > - ret =3D errno; > > + if (len < 0 && errno !=3D EAGAIN && errno !=3D EWOULDBLOCK) > > + die("Error on tap device, exiting"); > > + else if (len =3D=3D 0) > > + die("EOF on tap device, exiting"); > > =20 > > tap_handler(c, now); > > - > > - if (len > 0 || ret =3D=3D EAGAIN) > > - return; > > - > > - if (n =3D=3D TAP_BUF_BYTES) > > - goto redo; > > - > > - die("Error on tap device, exiting"); > > } > > =20 > > /** >=20 --=20 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 --rDVM/ALT/8JMCeBk Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmbXuNgACgkQzQJF27ox 2GdLKhAAnK9rEYgkf5XXGRXs76pyUw17ov50EU1UkMZ48LDLETKc2HhEXaWVZBE5 K8RRZ+gqeA5HD7DhkXOa3eEUT6cwjYn+0sfpSREiKHtLHpR8m1DnpZxDuVnqsR1O YVsMcsp5Gr0wodFPJpZrC11C9IRrPpLJYMcpancwGDGthwkMSYlUJzZ/3HRRrUrf L8d2AVlFxHZOMR4fsm/3BELuSMLMO3d9IeeuXVzIxmYKERJaAPO+a0Rg5/WpXR7v dCsIIs5lW88ArrE1l3HSmkeGrHIib+05W22NklGSFEWHW3ZNx+6h1viyJG3JhJTk 4dWrh0Yl0gD5+8r7o2FKyCedv2AdyYlBZ/LbHaUgY7eYlU0kF1MDDMudQuKMCYpF xsnRjGMltVADtKQk3C+TKmE3EZ8y8kZcNyHK+xs8TUvB9JT5SCdF8fb1rBuu7ZsQ Ag78fLQW8Yoo0252fwXWdE4L4+BXMGtKcBAyge3yVxaKbG6nKCEj9GxOFZ2PGY3X H6x6M3pGFdxts0IUZvpKEc422hgshXVgzVMPZ0dFugo/838GMIESmzWgtxQwRNdK Ukuv1TiZIS/ODD7stFqGYS9oC49kmsY2aSIFHUaUlU7yJRLSoD9c2CVuZAetqhyu uSPwqlQbDBy457Nz5X50lwPwt6AODE4ZAfwA/kpEAF/LWJWHkwY= =5ab3 -----END PGP SIGNATURE----- --rDVM/ALT/8JMCeBk--