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

[Stefano Brivio <sbrivio@redhat.com>]

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

-- 
Stefano