From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id B36A55A004F for ; Wed, 12 Jun 2024 08:37:31 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1718174247; bh=zLKtvQFDJtupQx2o39e5/dj2LuQF+auF9OlPAQtFo/U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=MjS4SiHRcYdxUpY5H5LusFN6rcqw+I4+Ri/lI9rzVsRVQxV2Q6KpPwq5pPcJiXITD YrJvoFszUJzrZdnkJZrUJkY6HdTl5spiXMHBuQxXpn86Fsp8oW4Yg2SXSDseIqMKA0 YYWEysQwGGUQovo6kjN3OBGAWAoCLV8Q9zJpoFdn4DvQe5eRI/0A0sc7io+XdUEhMR 4gn3jOFy96Wl/o8EM+hjRgZUc+M24kOLdxMVU61ebTSCnipYMsd80G+2rsqnczGgPi Dvn+GWV8zKBPMv+Q4izv/Vp2cyBWEAsE6xoUc1gr7MNKfGDf1z+kF5+9RLCAFepCXx VQVZ806h31ffw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4VzbSR2K5Gz4wcp; Wed, 12 Jun 2024 16:37:27 +1000 (AEST) Date: Wed, 12 Jun 2024 16:37:18 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v5 3/8] tap: refactor packets handling functions Message-ID: References: <20240605152129.1641658-1-lvivier@redhat.com> <20240605152129.1641658-4-lvivier@redhat.com> <20240612000950.6e8ad4b5@elisabeth> <20240612083408.43fffc4f@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="MD8BYtAVrB6QepAt" Content-Disposition: inline In-Reply-To: <20240612083408.43fffc4f@elisabeth> Message-ID-Hash: X54MWC5FEPZGXCFB7LLSQYMNS7CZDXMS X-Message-ID-Hash: X54MWC5FEPZGXCFB7LLSQYMNS7CZDXMS 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: Laurent Vivier , 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: --MD8BYtAVrB6QepAt Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 12, 2024 at 08:34:21AM +0200, Stefano Brivio wrote: > On Wed, 12 Jun 2024 16:18:59 +1000 > David Gibson wrote: >=20 > > On Wed, Jun 12, 2024 at 12:09:50AM +0200, Stefano Brivio wrote: > > > On Wed, 5 Jun 2024 17:21:24 +0200 > > > Laurent Vivier wrote: > > > =20 > > > > Consolidate pool_tap4() and pool_tap6() into pool_flush_all(), > > > > and tap4_handler() and tap6_handler() into tap_handler_all(). > > > > Create a generic packet_add_all() to consolidate packet > > > > addition logic and reduce code duplication. > > > >=20 > > > > The purpose is to ease the export of these functions to use > > > > them with the vhost-user backend. > > > >=20 > > > > Signed-off-by: Laurent Vivier > > > > --- > > > > tap.c | 113 +++++++++++++++++++++++++++++++++---------------------= ---- > > > > tap.h | 7 ++++ > > > > 2 files changed, 71 insertions(+), 49 deletions(-) > > > >=20 > > > > diff --git a/tap.c b/tap.c > > > > index 2ea08491a51f..5fb3cb83f3d2 100644 > > > > --- a/tap.c > > > > +++ b/tap.c > > > > @@ -920,6 +920,61 @@ append: > > > > return in->count; > > > > } > > > > =20 > > > > +/** > > > > + * pool_flush() - Flush both IPv4 and IPv6 packet pools > > > > + */ > > > > +void pool_flush_all(void) > > > > +{ > > > > + pool_flush(pool_tap4); > > > > + pool_flush(pool_tap6); > > > > +} > > > > + > > > > +/** > > > > + * tap_handler_all() - IPv4/IPv4 and ARP packet handler for tap fi= le descriptor =20 > > >=20 > > > IPv4/IPv6 > > > =20 > > > > + * @c: Execution context > > > > + * @now: Current timestamp > > > > + */ > > > > +void tap_handler_all(struct ctx *c, const struct timespec *now) = =20 > > >=20 > > > I wonder if this shouldn't be named tap_handler() instead. As we > > > already have tap_handler_passt() and tap_handler_pasta(), it's not > > > immediately clear what "all" refers to. =20 > >=20 > > I concur, I think tap_handler() is a better name. > >=20 > > > > +{ > > > > + tap4_handler(c, pool_tap4, now); > > > > + tap6_handler(c, pool_tap6, now); > > > > +} > > > > + > > > > +/** > > > > + * packet_add_all_do() - Add a packet to the appropriate TAP pool = =20 > > >=20 > > > A couple of remarks here: > > >=20 > > > - it's a bit unexpected that this is still in tap.c (it adds packets = to > > > a pool, it should be in packet.c judging by this name/description). > > > If we call it tap_queue_packet(), then it probably makes more sense? > > >=20 > > > - this does more than adding a packet to a pool. It's probably useless > > > to describe in detail what this does, as the function body is anyway > > > rather short and clear, but the current description could be a bit > > > misleading. What about "Queue/capture packet, update notion of > > > guest MAC address"? =20 > >=20 > > So, given this, I think it does make more sense for this to be in > > tap.c than packet.c. How about calling it tap_add_packets(). >=20 > It's a single packet it adds, so perhaps tap_add_packet()? Good point. > > > - what happens if you just call packet_add() from here, without deali= ng > > > with 'func' and 'line'? I think it's fine to print in tracing output > > > name and lines from this function, instead of the ones from the > > > caller. It's obvious who the caller is =20 > >=20 > > It is as of this patch, but I believe the idea is this will also be > > called from VU code down the line. >=20 > Okay, but even then, it would be obvious from previous tracing output > who the caller is. What's relevant here is to log that something went > wrong while adding a packet to an IPv4 or IPv6 pool. I don't think we > should bother passing around function name and line to log anything > else. Yeah, fair enough. --=20 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/~dgibson --MD8BYtAVrB6QepAt Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZpQh0ACgkQzQJF27ox 2GcP9w/9H9uAD7djp/l/j0PCCah5JWvF8E1Y9D5GotyYjl7aIXEd2KCj6wgTTV+n Fg0ywW6Eab8NkmJ4zFKTbxDult8gB9zx8XYoK6Ch28IU5Sn0M8Hl1HgvRcbHCUrh S4IMJ88hW8wCRjttysPtMDGPMOYA8MVin3vVwq1qQJ5YoD8ppNqA94OE8PKiNt6R jEHolHzKGJK/WF4vWVMnk6vC/jm2KyAgrwOpMi1mJfdeq0fI6DL9E1cEU51lkBL/ T69ILm80knJxtQf9by/9usEfhur7pAzMFNQNuugzO0sPo7g5q/W23EZ7aBhgzO0k CC6WGabEsLhbaR+TlHCg5phktW8h5gFUXxrLQfLEaOsi8gDKZONUiXwMkFhBk7tr VSPlTpc4MWjxrCBdtP0vx11Ipcc3dlj9XxuLQ4/EV9ZGJnKTgftmTYg1TQ0kzxph JL0m0w7NdeZKwVpAwFOrZF53TlGJWn19xDjDUVoxO/8nGflXGgdU5/BDhmk1cSyz To6L5doxfs5zcVa7HDvStkaWt7foe5kk9c0oInlGEciaViaKAp1RG3HqbpclhcDm aSXTi/wFKlIFd1AgmUsY2UuWZFfFuOKlzDAgkvxBrx5gPDdu0oTMfSnSLdJhK48B 02GfczCLYidu+UVTeYFlaVKX3eJxFktOrvhN9Tf/Q5VuuYhvJJk= =hzto -----END PGP SIGNATURE----- --MD8BYtAVrB6QepAt--