Re: [PATCH 5/8] util: Improve sockaddr initialisation in sock_l4()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3187 bytes --]

On Wed, Dec 27, 2023 at 09:25:15PM +0100, Stefano Brivio wrote:
> On Fri,  8 Dec 2023 01:31:37 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently we initialise the address field of the sockaddrs we construct
> > to the any/unspecified address, but not in a very clear way: we use
> > explicit 0 values, which is only interpretable if you know the order of
> > fields in the sockaddr structures.  Use explicit field names, and explicit
> > initialiser macros for the address.
> > 
> > Because we initialise to this default value, we don't need to explicitly
> > set the any/unspecified address later on if the caller didn't pass an
> > overriding bind address.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  util.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/util.c b/util.c
> > index d465e48..0152ae6 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -105,12 +105,12 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
> >  	struct sockaddr_in addr4 = {
> >  		.sin_family = AF_INET,
> >  		.sin_port = htons(port),
> > -		{ 0 }, { 0 },
> > +		.sin_addr = IN4ADDR_ANY_INIT,
> 
> The second { 0 } was meant to initialise .sin_zero, and:
> 
> >  	};
> >  	struct sockaddr_in6 addr6 = {
> >  		.sin6_family = AF_INET6,
> >  		.sin6_port = htons(port),
> > -		0, IN6ADDR_ANY_INIT, 0,
> > +		.sin6_addr = IN6ADDR_ANY_INIT,
> 
> these zeroes were for sin6_flowinfo and sin6_scope_id.
> 
> Not needed because we want zeroes anyway (or the "same as objects that
> have static storage duration"), but see commit eed6933e6c29 ("udp:
> Explicitly initialise sin6_scope_id and sin_zero in sockaddr_in{,6}"):
> gcc versions 7 to 9 used to complain. And gcc 9 isn't *that* old.
> 
> But then, you might ask, why didn't I just use names for all the
> initialisers? Well, there was some issue with sockaddr_in6 or
> sockaddr_in not having a field defined in some header (kernel or a C
> library). I have a vague memory it was about sin6_scope_id, but I can't
> find any note in the git history or anywhere else. :(
> 
> Now, the latest addition to sockaddr_in6 was sin6_scope_id (kernel:
> 2006, glibc: 2000), so that's fine. Also sockaddr_in looks okay,
> including sin_zero, with "#define sin_zero __pad" in the kernel version.
> 
> So... my preferred option would be to leave this untouched: the
> initialisers you replaced are anyway all zeroes. And, after RFC 2553
> (24 years ago), the order of those fields never changed.
> 
> If it really bothers you, let's at least initialise everything
> explicitly by name, because we know that gcc 9 complains, and if we hit
> another version of sockaddr_in6 without a named sin6_scope_id, we'll
> find out, eventually.
> 
> The rest of the series looks good to me and I just applied it (minus
> _this part_ of this patch).

Eh, ok.  I'll worry about this bit if I run across it again.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]