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=202510 header.b=q5/3KSYc; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id D7EBB5A061E for ; Fri, 14 Nov 2025 01:26:27 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1763079985; bh=DbFmqqxAk7dkCw0+2iQcB+C8dRVV7y/GCqGOo4nIAII=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=q5/3KSYcMpoT1JXZsfvcYzQD6nbobmMjWEwJ9OKw5DUvYXcoPeB4Q8OsnbkBjdGJk 7SpPF+bT+e0RO37q0QY7C7+dC6pt8JqPLxSP2ctwA2BOgGr8528sFzMMmRyKlWF40o SzMy4oxZsKVUUHysuZ6wMGS26m6a2GV3TLZYRePn2wpiX2qGxhq9HFr8v0iMHP2fT/ rF3Lx6BG9yBIPHHP5S761KrlhQQ0YG2NTvKvSK4QkH5Wv6mkeGX/aRpIw5uejaHc4F i/asI+pzIwJ1oJBO6IUKUW1x2AXMdC2N2/6T77c2RV61Zh6u+VZ1EQu9r5G3eiiYMD qjsKDZ+0UXv2w== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4d6ycK1MVlz4wCv; Fri, 14 Nov 2025 11:26:25 +1100 (AEDT) Date: Fri, 14 Nov 2025 09:53:10 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 1/8] inany: Let length of sockaddr_inany be implicit from the family Message-ID: References: <20251029062628.1647051-1-david@gibson.dropbear.id.au> <20251029062628.1647051-2-david@gibson.dropbear.id.au> <20251113073304.56859bae@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="vyL1u1KPRUsdqOaQ" Content-Disposition: inline In-Reply-To: <20251113073304.56859bae@elisabeth> Message-ID-Hash: G7U6EHJ4KVHB5PSPBGEIXQYWK5V7PCIK X-Message-ID-Hash: G7U6EHJ4KVHB5PSPBGEIXQYWK5V7PCIK 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: --vyL1u1KPRUsdqOaQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 13, 2025 at 07:33:04AM +0100, Stefano Brivio wrote: > Apologies for the very substantial delay. I have to admit I > procrastinated around 3/8 and 4/8 quite a bit as they look scary (but > much needed, of course). >=20 > Nits only, here: >=20 > On Wed, 29 Oct 2025 17:26:21 +1100 > David Gibson wrote: >=20 > > sockaddr_inany can contain either an IPv4 or IPv6 socket address, so the > > relevant length for bind() or connect() can vary. In pif_sockaddr() we > > return that length, and in sock_l4_sa() we take it as an extra paramete= r. > >=20 > > However, sockaddr_inany always contains exactly a sockaddr_in or a > > sockaddr_in6 each with a fixed size. Therefore we can derive the relev= ant > > length from the family, and don't need to pass it around separately. > >=20 > > Make a tiny helper to get the relevant address length, and update all > > interfaces to use that approach instead. > >=20 > > In the process, fix a buglet in tcp_flow_repair_bind(): we passed > > sizeof(union sockaddr_inany) to bind() instead of the specific length f= or > > the address family. Since the sizeof() is always longer than the speci= fic > > length, this is probably fine, but not theoretically correct. >=20 > (Whoops.) > =20 > > Signed-off-by: David Gibson > > --- > > flow.c | 17 +++++++---------- > > icmp.c | 3 ++- > > inany.h | 18 ++++++++++++++++++ > > pif.c | 14 ++++---------- > > pif.h | 2 +- > > tcp.c | 20 +++++++++----------- > > tcp_splice.c | 5 ++--- > > udp.c | 8 +++----- > > util.c | 9 ++++----- > > util.h | 5 +++-- > > 10 files changed, 53 insertions(+), 48 deletions(-) > >=20 > > diff --git a/flow.c b/flow.c > > index feefda3c..9926f408 100644 > > --- a/flow.c > > +++ b/flow.c > > @@ -170,8 +170,7 @@ struct flowside_sock_args { > > int fd; > > int err; > > enum epoll_type type; > > - const struct sockaddr *sa; > > - socklen_t sl; >=20 > This should be dropped from the struct documentation. Good point, done. > > + const union sockaddr_inany *sa; > > const char *path; > > uint32_t data; >=20 > Note: you'll get a trivial conflict here with "util: Move epoll > registration out of sock_l4_sa()" if you rebase. Actually, there are a heap of conflicts with that patch. I'll definitely respin for that reason if no other. >=20 > > }; > > @@ -187,7 +186,7 @@ static int flowside_sock_splice(void *arg) > > =20 > > ns_enter(a->c); > > =20 > > - a->fd =3D sock_l4_sa(a->c, a->type, a->sa, a->sl, NULL, > > + a->fd =3D sock_l4_sa(a->c, a->type, a->sa, NULL, > > a->sa->sa_family =3D=3D AF_INET6, a->data); > > a->err =3D errno; > > =20 > > @@ -209,11 +208,10 @@ int flowside_sock_l4(const struct ctx *c, enum ep= oll_type type, uint8_t pif, > > { > > const char *ifname =3D NULL; > > union sockaddr_inany sa; > > - socklen_t sl; > > =20 > > ASSERT(pif_is_socket(pif)); > > =20 > > - pif_sockaddr(c, &sa, &sl, pif, &tgt->oaddr, tgt->oport); > > + pif_sockaddr(c, &sa, pif, &tgt->oaddr, tgt->oport); > > =20 > > switch (pif) { > > case PIF_HOST: > > @@ -224,13 +222,13 @@ int flowside_sock_l4(const struct ctx *c, enum ep= oll_type type, uint8_t pif, > > else if (sa.sa_family =3D=3D AF_INET6) > > ifname =3D c->ip6.ifname_out; > > =20 > > - return sock_l4_sa(c, type, &sa, sl, ifname, > > + return sock_l4_sa(c, type, &sa, ifname, > > sa.sa_family =3D=3D AF_INET6, data); > > =20 > > case PIF_SPLICE: { > > struct flowside_sock_args args =3D { > > .c =3D c, .type =3D type, > > - .sa =3D &sa.sa, .sl =3D sl, .data =3D data, > > + .sa =3D &sa, .data =3D data, > > }; > > NS_CALL(flowside_sock_splice, &args); > > errno =3D args.err; > > @@ -259,10 +257,9 @@ int flowside_connect(const struct ctx *c, int s, > > uint8_t pif, const struct flowside *tgt) > > { > > union sockaddr_inany sa; > > - socklen_t sl; > > =20 > > - pif_sockaddr(c, &sa, &sl, pif, &tgt->eaddr, tgt->eport); > > - return connect(s, &sa.sa, sl); > > + pif_sockaddr(c, &sa, pif, &tgt->eaddr, tgt->eport); > > + return connect(s, &sa.sa, socklen_inany(&sa)); > > } > > =20 > > /** flow_log_ - Log flow-related message > > diff --git a/icmp.c b/icmp.c > > index 6dffafb0..62b3bed8 100644 > > --- a/icmp.c > > +++ b/icmp.c > > @@ -301,8 +301,9 @@ int icmp_tap_handler(const struct ctx *c, uint8_t p= if, sa_family_t af, > > ASSERT(flow_proto[pingf->f.type] =3D=3D proto); > > pingf->ts =3D now->tv_sec; > > =20 > > - pif_sockaddr(c, &sa, &msh.msg_namelen, PIF_HOST, &tgt->eaddr, 0); > > + pif_sockaddr(c, &sa, PIF_HOST, &tgt->eaddr, 0); > > msh.msg_name =3D &sa; > > + msh.msg_namelen =3D socklen_inany(&sa); > > msh.msg_iov =3D iov; > > msh.msg_iovlen =3D cnt; > > msh.msg_control =3D NULL; > > diff --git a/inany.h b/inany.h > > index 7ca5cbd3..e3cae2a8 100644 > > --- a/inany.h > > +++ b/inany.h > > @@ -67,6 +67,24 @@ union sockaddr_inany { > > struct sockaddr_in6 sa6; > > }; > > =20 > > +/** socklen_inany() - Return relevant size of an sockaddr_inany >=20 > 'of a' >=20 > ...but the fact that it returns that sounds kind of pleonastic. What > about: >=20 > /** socklen_inany() - Get relevant address length for sockaddr_inany addr= ess Good idea, done. >=20 > > + * @sa: sockaddr_iany socket address >=20 > sockaddr_inany Fixed. > > + * > > + * Returns the correct socket address length to pass to bind() or conn= ect() with > > + * @sa, based on whether it is an IPv4 or IPv6 address. >=20 > Same here, I guess we obviously want it to be correct, and we have a > somewhat standard syntax for return values in documentation, what about: >=20 > * Return: socket address length for bind() or connect(), from IP family = in @sa >=20 > ? Again, good idea, done. >=20 > > + */ > > +static inline socklen_t socklen_inany(const union sockaddr_inany *sa) > > +{ > > + switch (sa->sa_family) { > > + case AF_INET: > > + return sizeof(sa->sa4); >=20 > Wouldn't it be more obvious to return sizeof(struct sockaddr_in), and > sizeof(struct sockaddr_in6) below, instead? I actually had to check that > sa->sa4 matched that (I didn't remember if that was a union). Eh, maybe? > Not a strong preference from my side, both are obviously correct > anyway. I usually prefer to use sizeof() on variables not types when possible - it means things tend to either work or break more obviously if the types of the variables get changed. Leaving this one for now. >=20 > > + case AF_INET6: > > + return sizeof(sa->sa6); > > + default: > > + ASSERT(0); > > + } > > +} >=20 > --=20 > Stefano >=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 --vyL1u1KPRUsdqOaQ Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmkWYUYACgkQzQJF27ox 2GcF/w/+KrjX72tp+Oiln3Q31Qu99yK02NvCTCwWl554I6qMg2gN9t6n8MfXnFAA Lsdk9Uo1W9w2kKVR5PXEundy6dy/Errf2TTOHed1/6H3pkc4qP6vpo18gMLKy4yn VXKjO2sujjC/t8GTIcC4XJRC+D8VJLU70y2nQUJS+Hyx1gffy0IZerRrJTueR0c2 /hf7iSD4aHryVk7XuGIEbIK6H+3JxWiweXV+0kyGALHaK4uS7Yrq34QZpGuCcQjb 5Ik+nLx1nFTVG4rzWN/0PsohbTnas1K7iiwFp712B0RxARmy9X8RXAIYXMY19dnm HApNcRwAH+InqS/wkwtnF5XxIvZOQOz0CwvskS4zN0TGkS2niHaBsB9XBIYCckmK NBTDNCe/Yht94pxs4S4z8xCrLl6YdKQAP0XFsYKNHIr1sBKLajNU1vNZtqXOG269 ImMnLE2gdBi6HwopB/pRAS8zPQOhtPSVmJ+Sm1XYfcR443q0eL0nwHzeoQ0oRoBB I3tWyghoOarv1G+OumJny1XW9AhDjAKtRbSQRoB6MQeOSoKoi0JmiHJuwor4JyFj 7GpIfhNMGelZ4dF/x1ltN6rZXbAKYoVobz/LGCCUGgb5Xk8xWzGSsF1GbnWImPO2 lgMRyzLyWpZvsPkgg4CeMK6nrLUH5AGxFpNUstxVOy5zF6Qfph8= =6AMq -----END PGP SIGNATURE----- --vyL1u1KPRUsdqOaQ--