Re: [PATCH v2] socket: Implement sockaddr_storage with an anonymous union

From: "Bastien Roucariès" <rouca@debian.org>
To: libc-alpha@sourceware.org, Alejandro Colomar <alx.manpages@gmail.com>
Cc: Alejandro Colomar <alx@kernel.org>,
	linux-man@vger.kernel.org, Eric Blake <eblake@redhat.com>,
	Zack Weinberg <zack@owlfolio.org>,
	Stefan Puiu <stefan.puiu@gmail.com>, Igor Sysoev <igor@sysoev.ru>,
	gcc@gcc.gnu.org, Joseph Myers <joseph@codesourcery.com>
Subject: Re: [PATCH v2] socket: Implement sockaddr_storage with an anonymous union
Date: Fri, 20 Jan 2023 20:46:54 +0000	[thread overview]
Message-ID: <2885282.DR2gb7e7pQ@portable-bastien> (raw)
In-Reply-To: <0646afe5-f726-dd27-9058-17bd042279d5@gmail.com>

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

Le vendredi 20 janvier 2023, 20:38:32 UTC Alejandro Colomar a écrit :
> Hi Bastien,
> 
> On 1/20/23 21:32, Bastien Roucariès wrote:
> [...]
> 
> >> diff --git a/bits/socket.h b/bits/socket.h
> >> index aac8c49b00..c0c23b4e84 100644
> >> --- a/bits/socket.h
> >> +++ b/bits/socket.h
> >> @@ -168,9 +168,14 @@ struct sockaddr
> >>   
> >>   struct sockaddr_storage
> >>     {
> >> -    __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
> >> -    char __ss_padding[_SS_PADSIZE];
> >> -    __ss_aligntype __ss_align;	/* Force desired alignment.  */
> > no this is not correct you break ABI by reducing size
> >> +    union
> >> +      {
> >> +        __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
> >> +        struct sockaddr      sa;
> >> +        struct sockaddr_in   sin;
> >> +        struct sockaddr_in6  sin6;
> >> +        struct sockaddr_un   sun;
> >> +      };
> >>     };
> > 
> > Correct one structure is
> > 
> > struct __private_sock_storage {
> >      __SOCKADDR_COMMON (ssprivate_);   /* Address family, etc. */
> >      char __ss_padding[_SS_PADSIZE];
> >      __ss_aligntype __ss_align; /* Force desired alignment. */
> > }
> 
> What is this structure for?  I expect that it's for declaring a wide-enough and 
> correctly aligned type, but the union containing all the other types already 
> guarantees a size as wide as any other sockaddr_* and with the widest alignment.
> 
> Also, any member that is necessary for superalignment or padding could be added 
> at the end of sockaddr_storage, after the anon union; you don't need the extra 
> struct, I guess.

No we need it, max of structure is struct sockaddr_un   sun and is size of 108.
sizeof(sockaddr_storage) is 128...

Did you see the line of the kernel source I send you ? kernel expect size of 109 for un aka we should pad by a nul byte...

I think it is safer in a first step, to keep the old structure... Maybe later simplify

Did you also see 
https://github.com/bminor/glibc/blob/master/socket/sys/socket.h#L63

Bastien

> 
> Right?
> 
> > 
> >   struct sockaddr_storage
> >     {
> >         union
> >        {
> >           __SOCKADDR_COMMON (ss_);       /* Address family, etc. */
> >          struct sockaddr      sa;
> >           struct sockaddr_in   sin;
> >          struct sockaddr_in6  sin6;
> >          struct sockaddr_un   sun;
> >          struct __private_sock_storage _private;
> >        };
> > };
> > 
> > May it could be dropped later using align construct for modern C and padding
> > 
> 
> Cheers,
> 
> Alex
> 
> > Bastien
> >>   
> >>   
> >>
> > 
> 
> -- 
> <http://www.alejandro-colomar.es/>
> 

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]