| Message ID | 1427670922-3534376-1-git-send-email-shawn@churchofgit.com |
|---|---|
| State | New |
| Headers | show |
On 29 Mar 2015 16:15, Shawn Landden wrote: > I'm not going to change the ChangeLog because that practice > breaks git merges. you still should post it in the commit message. people don't want to write it for you. > --- a/inet/netinet/in.h > +++ b/inet/netinet/in.h > @@ -214,12 +214,14 @@ struct in6_addr > #ifdef __USE_MISC > uint16_t __u6_addr16[8]; > uint32_t __u6_addr32[4]; > + uint64_t __u6_addr64[2]; > #endif > } __in6_u; > #define s6_addr __in6_u.__u6_addr8 > #ifdef __USE_MISC > # define s6_addr16 __in6_u.__u6_addr16 > # define s6_addr32 __in6_u.__u6_addr32 > +# define s6_addr64 __in6_u.__u6_addr64 > #endif > }; > #endif /* !__USE_KERNEL_IPV6_DEFS */ in general, i like this, but you're changing the alignment requirements which in turn means you're possibly changing the ABI. a library that takes this struct as an argument built against older glibc would require 32bit alignment, but an app might be expecting 64bit after this change. any such impact has to be considered before merging. -mike
On Sun, Mar 29, 2015 at 08:56:09PM -0400, Mike Frysinger wrote: > On 29 Mar 2015 16:15, Shawn Landden wrote: > > I'm not going to change the ChangeLog because that practice > > breaks git merges. > > you still should post it in the commit message. people don't want to write it > for you. > > > --- a/inet/netinet/in.h > > +++ b/inet/netinet/in.h > > @@ -214,12 +214,14 @@ struct in6_addr > > #ifdef __USE_MISC > > uint16_t __u6_addr16[8]; > > uint32_t __u6_addr32[4]; > > + uint64_t __u6_addr64[2]; > > #endif > > } __in6_u; > > #define s6_addr __in6_u.__u6_addr8 > > #ifdef __USE_MISC > > # define s6_addr16 __in6_u.__u6_addr16 > > # define s6_addr32 __in6_u.__u6_addr32 > > +# define s6_addr64 __in6_u.__u6_addr64 > > #endif > > }; > > #endif /* !__USE_KERNEL_IPV6_DEFS */ > > in general, i like this, but you're changing the alignment requirements which in > turn means you're possibly changing the ABI. a library that takes this struct > as an argument built against older glibc would require 32bit alignment, but an > app might be expecting 64bit after this change. any such impact has to be > considered before merging. > -mike This only applies if glibc is allocating the struct in6_addr, which I am pretty sure we do not do.
On Sun, Mar 29, 2015 at 08:56:09PM -0400, Mike Frysinger wrote: > On 29 Mar 2015 16:15, Shawn Landden wrote: > > I'm not going to change the ChangeLog because that practice > > breaks git merges. > > you still should post it in the commit message. people don't want to write it > for you. > > > --- a/inet/netinet/in.h > > +++ b/inet/netinet/in.h > > @@ -214,12 +214,14 @@ struct in6_addr > > #ifdef __USE_MISC > > uint16_t __u6_addr16[8]; > > uint32_t __u6_addr32[4]; > > + uint64_t __u6_addr64[2]; > > #endif > > } __in6_u; > > #define s6_addr __in6_u.__u6_addr8 > > #ifdef __USE_MISC > > # define s6_addr16 __in6_u.__u6_addr16 > > # define s6_addr32 __in6_u.__u6_addr32 > > +# define s6_addr64 __in6_u.__u6_addr64 > > #endif > > }; > > #endif /* !__USE_KERNEL_IPV6_DEFS */ > > in general, i like this, but you're changing the alignment requirements which in > turn means you're possibly changing the ABI. a library that takes this struct > as an argument built against older glibc would require 32bit alignment, but an > app might be expecting 64bit after this change. any such impact has to be It is the other way around, a library would expect 64-bit but the application would only send 32-bit aligned. Yes, I don't think this can go in. > considered before merging. > -mike
On 30 Mar 2015 01:26, Shawn Landden wrote: > On Sun, Mar 29, 2015 at 08:56:09PM -0400, Mike Frysinger wrote: > > On 29 Mar 2015 16:15, Shawn Landden wrote: > > > I'm not going to change the ChangeLog because that practice > > > breaks git merges. > > > > you still should post it in the commit message. people don't want to write it > > for you. > > > > > --- a/inet/netinet/in.h > > > +++ b/inet/netinet/in.h > > > @@ -214,12 +214,14 @@ struct in6_addr > > > #ifdef __USE_MISC > > > uint16_t __u6_addr16[8]; > > > uint32_t __u6_addr32[4]; > > > + uint64_t __u6_addr64[2]; > > > #endif > > > } __in6_u; > > > #define s6_addr __in6_u.__u6_addr8 > > > #ifdef __USE_MISC > > > # define s6_addr16 __in6_u.__u6_addr16 > > > # define s6_addr32 __in6_u.__u6_addr32 > > > +# define s6_addr64 __in6_u.__u6_addr64 > > > #endif > > > }; > > > #endif /* !__USE_KERNEL_IPV6_DEFS */ > > > > in general, i like this, but you're changing the alignment requirements which in > > turn means you're possibly changing the ABI. a library that takes this struct > > as an argument built against older glibc would require 32bit alignment, but an > > app might be expecting 64bit after this change. any such impact has to be > > considered before merging. > > This only applies if glibc is allocating the struct in6_addr, which I am pretty sure > we do not do. i mean apps/libs picking up different defs from the glibc headers, not glibc itself returning an allocated struct -mike
On Sun, Mar 29, 2015 at 04:15:22PM -0700, Shawn Landden wrote: > 64-bit architectures are common so this makes sense to have. > 64-bit arches were not so common 1997-02-16. > > I'm not going to change the ChangeLog because that practice > breaks git merges. > > Bugzilla: #18117 > Signed-off-by: Shawn Landden <shawn@churchofgit.com> > --- > inet/netinet/in.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/inet/netinet/in.h b/inet/netinet/in.h > index f541c58..b64b559 100644 > --- a/inet/netinet/in.h > +++ b/inet/netinet/in.h > @@ -214,12 +214,14 @@ struct in6_addr > #ifdef __USE_MISC > uint16_t __u6_addr16[8]; > uint32_t __u6_addr32[4]; > + uint64_t __u6_addr64[2]; > #endif This breaks ABI by changing the alignment requirement from 4 to 8. I think that's a show-stopper for this proposal. Rich
On Mon, Mar 30, 2015 at 02:34:34AM -0400, Rich Felker wrote: > On Sun, Mar 29, 2015 at 04:15:22PM -0700, Shawn Landden wrote: > > 64-bit architectures are common so this makes sense to have. > > 64-bit arches were not so common 1997-02-16. > > > > I'm not going to change the ChangeLog because that practice > > breaks git merges. > > > > Bugzilla: #18117 > > Signed-off-by: Shawn Landden <shawn@churchofgit.com> > > --- > > inet/netinet/in.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/inet/netinet/in.h b/inet/netinet/in.h > > index f541c58..b64b559 100644 > > --- a/inet/netinet/in.h > > +++ b/inet/netinet/in.h > > @@ -214,12 +214,14 @@ struct in6_addr > > #ifdef __USE_MISC > > uint16_t __u6_addr16[8]; > > uint32_t __u6_addr32[4]; > > + uint64_t __u6_addr64[2]; > > #endif > > This breaks ABI by changing the alignment requirement from 4 to 8. I > think that's a show-stopper for this proposal. agreeed > > Rich
diff --git a/inet/netinet/in.h b/inet/netinet/in.h index f541c58..b64b559 100644 --- a/inet/netinet/in.h +++ b/inet/netinet/in.h @@ -214,12 +214,14 @@ struct in6_addr #ifdef __USE_MISC uint16_t __u6_addr16[8]; uint32_t __u6_addr32[4]; + uint64_t __u6_addr64[2]; #endif } __in6_u; #define s6_addr __in6_u.__u6_addr8 #ifdef __USE_MISC # define s6_addr16 __in6_u.__u6_addr16 # define s6_addr32 __in6_u.__u6_addr32 +# define s6_addr64 __in6_u.__u6_addr64 #endif }; #endif /* !__USE_KERNEL_IPV6_DEFS */
64-bit architectures are common so this makes sense to have. 64-bit arches were not so common 1997-02-16. I'm not going to change the ChangeLog because that practice breaks git merges. Bugzilla: #18117 Signed-off-by: Shawn Landden <shawn@churchofgit.com> --- inet/netinet/in.h | 2 ++ 1 file changed, 2 insertions(+)