* [PATCH] alpha: Remove anonymous union in struct stat [BZ #27042] @ 2020-12-09 16:20 Matt Turner 2020-12-09 17:11 ` Florian Weimer 2020-12-10 1:19 ` [PATCHv2] " Matt Turner 0 siblings, 2 replies; 10+ messages in thread From: Matt Turner @ 2020-12-09 16:20 UTC (permalink / raw) To: libc-alpha This is clever, but it confuses downstream detection in at least zstd and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro, which is not provided by the path using the anonymous union; glib checks for the presence of 'st_mtimensec' in struct stat but then tries to access that field in struct statx (which might be a bug on its own). --- .../unix/sysv/linux/alpha/bits/struct_stat.h | 23 ++++--------------- 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h b/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h index 1c9b4248b8..dddc5fb5a0 100644 --- a/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h +++ b/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h @@ -27,26 +27,13 @@ 'struct timespec'. This is the type used whenever possible but the Unix namespace rules do not allow the identifier 'timespec' to appear in the <sys/stat.h> header. Therefore we have to handle the use of - this header in strictly standard-compliant sources special. - - Use neat tidy anonymous unions and structures when possible. */ + this header in strictly standard-compliant sources special. */ #ifdef __USE_XOPEN2K8 -# if __GNUC_PREREQ(3,3) -# define __ST_TIME(X) \ - __extension__ union { \ - struct timespec st_##X##tim; \ - struct { \ - __time_t st_##X##time; \ - unsigned long st_##X##timensec; \ - }; \ - } -# else -# define __ST_TIME(X) struct timespec st_##X##tim -# define st_atime st_atim.tv_sec -# define st_mtime st_mtim.tv_sec -# define st_ctime st_ctim.tv_sec -# endif +# define __ST_TIME(X) struct timespec st_##X##tim +# define st_atime st_atim.tv_sec +# define st_mtime st_mtim.tv_sec +# define st_ctime st_ctim.tv_sec #else # define __ST_TIME(X) \ __time_t st_##X##time; \ -- 2.26.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] alpha: Remove anonymous union in struct stat [BZ #27042] 2020-12-09 16:20 [PATCH] alpha: Remove anonymous union in struct stat [BZ #27042] Matt Turner @ 2020-12-09 17:11 ` Florian Weimer 2020-12-10 1:25 ` Matt Turner 2020-12-10 1:19 ` [PATCHv2] " Matt Turner 1 sibling, 1 reply; 10+ messages in thread From: Florian Weimer @ 2020-12-09 17:11 UTC (permalink / raw) To: Matt Turner via Libc-alpha * Matt Turner via Libc-alpha: > This is clever, but it confuses downstream detection in at least zstd > and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro, > which is not provided by the path using the anonymous union; glib checks > for the presence of 'st_mtimensec' in struct stat but then tries to > access that field in struct statx (which might be a bug on its own). A more conservative approach would be adding #define st_atime st_atime #define st_ctime st_ctime #define st_mtime st_mtime to the anonymous union case. Thanks, Florian -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] alpha: Remove anonymous union in struct stat [BZ #27042] 2020-12-09 17:11 ` Florian Weimer @ 2020-12-10 1:25 ` Matt Turner 0 siblings, 0 replies; 10+ messages in thread From: Matt Turner @ 2020-12-10 1:25 UTC (permalink / raw) To: Florian Weimer; +Cc: Matt Turner via Libc-alpha On Wed, Dec 9, 2020 at 12:11 PM Florian Weimer <fweimer@redhat.com> wrote: > > * Matt Turner via Libc-alpha: > > > This is clever, but it confuses downstream detection in at least zstd > > and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro, > > which is not provided by the path using the anonymous union; glib checks > > for the presence of 'st_mtimensec' in struct stat but then tries to > > access that field in struct statx (which might be a bug on its own). > > A more conservative approach would be adding > > #define st_atime st_atime > #define st_ctime st_ctime > #define st_mtime st_mtime > > to the anonymous union case. Thanks. That works for zstd, but not for glib—which might be a bug in glib, like I mentioned. I'll investigate that independently. Andreas immediately closed the bug I filed with the message > The existence of the st_?time macros is an implementation detail. and I have no doubt that he's correct, but it appears that all or basically all of the architectures' headers provide these macros so I'd prefer to bring alpha in line rather than convince multiple projects that their code is technically wrong in a way that only manifests on an architecture they've never heard of. v2 sent that deals with the kernel_stat.h interactions. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv2] alpha: Remove anonymous union in struct stat [BZ #27042] 2020-12-09 16:20 [PATCH] alpha: Remove anonymous union in struct stat [BZ #27042] Matt Turner 2020-12-09 17:11 ` Florian Weimer @ 2020-12-10 1:19 ` Matt Turner 2020-12-10 21:18 ` Adhemerval Zanella 1 sibling, 1 reply; 10+ messages in thread From: Matt Turner @ 2020-12-10 1:19 UTC (permalink / raw) To: libc-alpha This is clever, but it confuses downstream detection in at least zstd and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro, which is not provided by the path using the anonymous union; glib checks for the presence of 'st_mtimensec' in struct stat but then tries to access that field in struct statx (which might be a bug on its own). --- The macros affect the definitions in kernel_stat.h, so rename them. Not thrilled. .../unix/sysv/linux/alpha/bits/struct_stat.h | 81 ++++++++++--------- sysdeps/unix/sysv/linux/alpha/kernel_stat.h | 24 +++--- sysdeps/unix/sysv/linux/alpha/xstatconv.c | 24 +++--- 3 files changed, 66 insertions(+), 63 deletions(-) diff --git a/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h b/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h index 1c9b4248b8..d2aae9fdd7 100644 --- a/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h +++ b/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h @@ -23,37 +23,6 @@ #ifndef _BITS_STRUCT_STAT_H #define _BITS_STRUCT_STAT_H 1 -/* Nanosecond resolution timestamps are stored in a format equivalent to - 'struct timespec'. This is the type used whenever possible but the - Unix namespace rules do not allow the identifier 'timespec' to appear - in the <sys/stat.h> header. Therefore we have to handle the use of - this header in strictly standard-compliant sources special. - - Use neat tidy anonymous unions and structures when possible. */ - -#ifdef __USE_XOPEN2K8 -# if __GNUC_PREREQ(3,3) -# define __ST_TIME(X) \ - __extension__ union { \ - struct timespec st_##X##tim; \ - struct { \ - __time_t st_##X##time; \ - unsigned long st_##X##timensec; \ - }; \ - } -# else -# define __ST_TIME(X) struct timespec st_##X##tim -# define st_atime st_atim.tv_sec -# define st_mtime st_mtim.tv_sec -# define st_ctime st_ctim.tv_sec -# endif -#else -# define __ST_TIME(X) \ - __time_t st_##X##time; \ - unsigned long st_##X##timensec -#endif - - struct stat { __dev_t st_dev; /* Device. */ @@ -77,9 +46,27 @@ struct stat __blksize_t st_blksize; /* Optimal block size for I/O. */ __nlink_t st_nlink; /* Link count. */ int __pad2; /* Real padding. */ - __ST_TIME(a); /* Time of last access. */ - __ST_TIME(m); /* Time of last modification. */ - __ST_TIME(c); /* Time of last status change. */ +#ifdef __USE_XOPEN2K8 + /* Nanosecond resolution timestamps are stored in a format + equivalent to 'struct timespec'. This is the type used + whenever possible but the Unix namespace rules do not allow the + identifier 'timespec' to appear in the <sys/stat.h> header. + Therefore we have to handle the use of this header in strictly + standard-compliant sources special. */ + struct timespec st_atim; /* Time of last access. */ + struct timespec st_mtim; /* Time of last modification. */ + struct timespec st_ctim; /* Time of last status change. */ +# define st_atime st_atim.tv_sec /* Backward compatibility. */ +# define st_mtime st_mtim.tv_sec +# define st_ctime st_ctim.tv_sec +#else + __time_t st_atime; /* Time of last access. */ + unsigned long int st_atimensec; /* Nscecs of last access. */ + __time_t st_mtime; /* Time of last modification. */ + unsigned long int st_mtimensec; /* Nsecs of last modification. */ + __time_t st_ctime; /* Time of last status change. */ + unsigned long int st_ctimensec; /* Nsecs of last status change. */ +#endif long __glibc_reserved[3]; }; @@ -98,15 +85,31 @@ struct stat64 __blksize_t st_blksize; /* Optimal block size for I/O. */ __nlink_t st_nlink; /* Link count. */ int __pad0; /* Real padding. */ - __ST_TIME(a); /* Time of last access. */ - __ST_TIME(m); /* Time of last modification. */ - __ST_TIME(c); /* Time of last status change. */ +#ifdef __USE_XOPEN2K8 + /* Nanosecond resolution timestamps are stored in a format + equivalent to 'struct timespec'. This is the type used + whenever possible but the Unix namespace rules do not allow the + identifier 'timespec' to appear in the <sys/stat.h> header. + Therefore we have to handle the use of this header in strictly + standard-compliant sources special. */ + struct timespec st_atim; /* Time of last access. */ + struct timespec st_mtim; /* Time of last modification. */ + struct timespec st_ctim; /* Time of last status change. */ +# define st_atime st_atim.tv_sec /* Backward compatibility. */ +# define st_mtime st_mtim.tv_sec +# define st_ctime st_ctim.tv_sec +#else + __time_t st_atime; /* Time of last access. */ + unsigned long int st_atimensec; /* Nscecs of last access. */ + __time_t st_mtime; /* Time of last modification. */ + unsigned long int st_mtimensec; /* Nsecs of last modification. */ + __time_t st_ctime; /* Time of last status change. */ + unsigned long int st_ctimensec; /* Nsecs of last status change. */ +#endif long __glibc_reserved[3]; }; #endif -#undef __ST_TIME - /* Tell code we have these members. */ #define _STATBUF_ST_BLKSIZE #define _STATBUF_ST_RDEV diff --git a/sysdeps/unix/sysv/linux/alpha/kernel_stat.h b/sysdeps/unix/sysv/linux/alpha/kernel_stat.h index ff69045f8f..a292920969 100644 --- a/sysdeps/unix/sysv/linux/alpha/kernel_stat.h +++ b/sysdeps/unix/sysv/linux/alpha/kernel_stat.h @@ -9,9 +9,9 @@ struct kernel_stat unsigned int st_gid; unsigned int st_rdev; long int st_size; - unsigned long int st_atime; - unsigned long int st_mtime; - unsigned long int st_ctime; + unsigned long int st_atime_sec; + unsigned long int st_mtime_sec; + unsigned long int st_ctime_sec; unsigned int st_blksize; int st_blocks; unsigned int st_flags; @@ -34,11 +34,11 @@ struct kernel_stat64 unsigned int st_nlink; unsigned int __pad0; - unsigned long st_atime; + unsigned long st_atime_sec; unsigned long st_atimensec; - unsigned long st_mtime; + unsigned long st_mtime_sec; unsigned long st_mtimensec; - unsigned long st_ctime; + unsigned long st_ctime_sec; unsigned long st_ctimensec; long __glibc_reserved[3]; }; @@ -54,9 +54,9 @@ struct glibc2_stat __gid_t st_gid; __dev_t st_rdev; __off_t st_size; - __time_t st_atime; - __time_t st_mtime; - __time_t st_ctime; + __time_t st_atime_sec; + __time_t st_mtime_sec; + __time_t st_ctime_sec; unsigned int st_blksize; int st_blocks; unsigned int st_flags; @@ -74,9 +74,9 @@ struct glibc21_stat __gid_t st_gid; __dev_t st_rdev; __off_t st_size; - __time_t st_atime; - __time_t st_mtime; - __time_t st_ctime; + __time_t st_atime_sec; + __time_t st_mtime_sec; + __time_t st_ctime_sec; __blkcnt64_t st_blocks; __blksize_t st_blksize; unsigned int st_flags; diff --git a/sysdeps/unix/sysv/linux/alpha/xstatconv.c b/sysdeps/unix/sysv/linux/alpha/xstatconv.c index f716a10f34..43224a7f25 100644 --- a/sysdeps/unix/sysv/linux/alpha/xstatconv.c +++ b/sysdeps/unix/sysv/linux/alpha/xstatconv.c @@ -44,9 +44,9 @@ __xstat_conv (int vers, struct kernel_stat *kbuf, void *ubuf) buf->st_gid = kbuf->st_gid; buf->st_rdev = kbuf->st_rdev; buf->st_size = kbuf->st_size; - buf->st_atime = kbuf->st_atime; - buf->st_mtime = kbuf->st_mtime; - buf->st_ctime = kbuf->st_ctime; + buf->st_atime_sec = kbuf->st_atime_sec; + buf->st_mtime_sec = kbuf->st_mtime_sec; + buf->st_ctime_sec = kbuf->st_ctime_sec; buf->st_blksize = kbuf->st_blksize; buf->st_blocks = kbuf->st_blocks; buf->st_flags = kbuf->st_flags; @@ -66,9 +66,9 @@ __xstat_conv (int vers, struct kernel_stat *kbuf, void *ubuf) buf->st_gid = kbuf->st_gid; buf->st_rdev = kbuf->st_rdev; buf->st_size = kbuf->st_size; - buf->st_atime = kbuf->st_atime; - buf->st_mtime = kbuf->st_mtime; - buf->st_ctime = kbuf->st_ctime; + buf->st_atime_sec = kbuf->st_atime_sec; + buf->st_mtime_sec = kbuf->st_mtime_sec; + buf->st_ctime_sec = kbuf->st_ctime_sec; buf->st_blocks = kbuf->st_blocks; buf->st_blksize = kbuf->st_blksize; buf->st_flags = kbuf->st_flags; @@ -98,12 +98,12 @@ __xstat_conv (int vers, struct kernel_stat *kbuf, void *ubuf) buf->st_nlink = kbuf->st_nlink; buf->__pad0 = 0; - buf->st_atime = kbuf->st_atime; - buf->st_atimensec = 0; - buf->st_mtime = kbuf->st_mtime; - buf->st_mtimensec = 0; - buf->st_ctime = kbuf->st_ctime; - buf->st_ctimensec = 0; + buf->st_atim.tv_sec = kbuf->st_atime_sec; + buf->st_atim.tv_nsec = 0; + buf->st_mtim.tv_sec = kbuf->st_mtime_sec; + buf->st_mtim.tv_nsec = 0; + buf->st_ctim.tv_sec = kbuf->st_ctime_sec; + buf->st_ctim.tv_nsec = 0; buf->__glibc_reserved[0] = 0; buf->__glibc_reserved[1] = 0; -- 2.26.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] alpha: Remove anonymous union in struct stat [BZ #27042] 2020-12-10 1:19 ` [PATCHv2] " Matt Turner @ 2020-12-10 21:18 ` Adhemerval Zanella 2020-12-15 16:43 ` Matt Turner 0 siblings, 1 reply; 10+ messages in thread From: Adhemerval Zanella @ 2020-12-10 21:18 UTC (permalink / raw) To: libc-alpha On 09/12/2020 22:19, Matt Turner via Libc-alpha wrote: > This is clever, but it confuses downstream detection in at least zstd > and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro, > which is not provided by the path using the anonymous union; glib checks > for the presence of 'st_mtimensec' in struct stat but then tries to > access that field in struct statx (which might be a bug on its own). > --- > The macros affect the definitions in kernel_stat.h, so rename them. Not > thrilled. LGTM, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > .../unix/sysv/linux/alpha/bits/struct_stat.h | 81 ++++++++++--------- > sysdeps/unix/sysv/linux/alpha/kernel_stat.h | 24 +++--- > sysdeps/unix/sysv/linux/alpha/xstatconv.c | 24 +++--- > 3 files changed, 66 insertions(+), 63 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h b/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h > index 1c9b4248b8..d2aae9fdd7 100644 > --- a/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h > +++ b/sysdeps/unix/sysv/linux/alpha/bits/struct_stat.h > @@ -23,37 +23,6 @@ > #ifndef _BITS_STRUCT_STAT_H > #define _BITS_STRUCT_STAT_H 1 > > -/* Nanosecond resolution timestamps are stored in a format equivalent to > - 'struct timespec'. This is the type used whenever possible but the > - Unix namespace rules do not allow the identifier 'timespec' to appear > - in the <sys/stat.h> header. Therefore we have to handle the use of > - this header in strictly standard-compliant sources special. > - > - Use neat tidy anonymous unions and structures when possible. */ > - > -#ifdef __USE_XOPEN2K8 > -# if __GNUC_PREREQ(3,3) > -# define __ST_TIME(X) \ > - __extension__ union { \ > - struct timespec st_##X##tim; \ > - struct { \ > - __time_t st_##X##time; \ > - unsigned long st_##X##timensec; \ > - }; \ > - } > -# else > -# define __ST_TIME(X) struct timespec st_##X##tim > -# define st_atime st_atim.tv_sec > -# define st_mtime st_mtim.tv_sec > -# define st_ctime st_ctim.tv_sec > -# endif > -#else > -# define __ST_TIME(X) \ > - __time_t st_##X##time; \ > - unsigned long st_##X##timensec > -#endif > - > - > struct stat > { > __dev_t st_dev; /* Device. */ > @@ -77,9 +46,27 @@ struct stat > __blksize_t st_blksize; /* Optimal block size for I/O. */ > __nlink_t st_nlink; /* Link count. */ > int __pad2; /* Real padding. */ > - __ST_TIME(a); /* Time of last access. */ > - __ST_TIME(m); /* Time of last modification. */ > - __ST_TIME(c); /* Time of last status change. */ > +#ifdef __USE_XOPEN2K8 > + /* Nanosecond resolution timestamps are stored in a format > + equivalent to 'struct timespec'. This is the type used > + whenever possible but the Unix namespace rules do not allow the > + identifier 'timespec' to appear in the <sys/stat.h> header. > + Therefore we have to handle the use of this header in strictly > + standard-compliant sources special. */ > + struct timespec st_atim; /* Time of last access. */ > + struct timespec st_mtim; /* Time of last modification. */ > + struct timespec st_ctim; /* Time of last status change. */ > +# define st_atime st_atim.tv_sec /* Backward compatibility. */ > +# define st_mtime st_mtim.tv_sec > +# define st_ctime st_ctim.tv_sec > +#else > + __time_t st_atime; /* Time of last access. */ > + unsigned long int st_atimensec; /* Nscecs of last access. */ > + __time_t st_mtime; /* Time of last modification. */ > + unsigned long int st_mtimensec; /* Nsecs of last modification. */ > + __time_t st_ctime; /* Time of last status change. */ > + unsigned long int st_ctimensec; /* Nsecs of last status change. */ > +#endif > long __glibc_reserved[3]; > }; > > @@ -98,15 +85,31 @@ struct stat64 > __blksize_t st_blksize; /* Optimal block size for I/O. */ > __nlink_t st_nlink; /* Link count. */ > int __pad0; /* Real padding. */ > - __ST_TIME(a); /* Time of last access. */ > - __ST_TIME(m); /* Time of last modification. */ > - __ST_TIME(c); /* Time of last status change. */ > +#ifdef __USE_XOPEN2K8 > + /* Nanosecond resolution timestamps are stored in a format > + equivalent to 'struct timespec'. This is the type used > + whenever possible but the Unix namespace rules do not allow the > + identifier 'timespec' to appear in the <sys/stat.h> header. > + Therefore we have to handle the use of this header in strictly > + standard-compliant sources special. */ > + struct timespec st_atim; /* Time of last access. */ > + struct timespec st_mtim; /* Time of last modification. */ > + struct timespec st_ctim; /* Time of last status change. */ > +# define st_atime st_atim.tv_sec /* Backward compatibility. */ > +# define st_mtime st_mtim.tv_sec > +# define st_ctime st_ctim.tv_sec > +#else > + __time_t st_atime; /* Time of last access. */ > + unsigned long int st_atimensec; /* Nscecs of last access. */ > + __time_t st_mtime; /* Time of last modification. */ > + unsigned long int st_mtimensec; /* Nsecs of last modification. */ > + __time_t st_ctime; /* Time of last status change. */ > + unsigned long int st_ctimensec; /* Nsecs of last status change. */ > +#endif > long __glibc_reserved[3]; > }; > #endif > > -#undef __ST_TIME > - > /* Tell code we have these members. */ > #define _STATBUF_ST_BLKSIZE > #define _STATBUF_ST_RDEV > diff --git a/sysdeps/unix/sysv/linux/alpha/kernel_stat.h b/sysdeps/unix/sysv/linux/alpha/kernel_stat.h > index ff69045f8f..a292920969 100644 > --- a/sysdeps/unix/sysv/linux/alpha/kernel_stat.h > +++ b/sysdeps/unix/sysv/linux/alpha/kernel_stat.h > @@ -9,9 +9,9 @@ struct kernel_stat > unsigned int st_gid; > unsigned int st_rdev; > long int st_size; > - unsigned long int st_atime; > - unsigned long int st_mtime; > - unsigned long int st_ctime; > + unsigned long int st_atime_sec; > + unsigned long int st_mtime_sec; > + unsigned long int st_ctime_sec; > unsigned int st_blksize; > int st_blocks; > unsigned int st_flags; > @@ -34,11 +34,11 @@ struct kernel_stat64 > unsigned int st_nlink; > unsigned int __pad0; > > - unsigned long st_atime; > + unsigned long st_atime_sec; > unsigned long st_atimensec; > - unsigned long st_mtime; > + unsigned long st_mtime_sec; > unsigned long st_mtimensec; > - unsigned long st_ctime; > + unsigned long st_ctime_sec; > unsigned long st_ctimensec; > long __glibc_reserved[3]; > }; > @@ -54,9 +54,9 @@ struct glibc2_stat > __gid_t st_gid; > __dev_t st_rdev; > __off_t st_size; > - __time_t st_atime; > - __time_t st_mtime; > - __time_t st_ctime; > + __time_t st_atime_sec; > + __time_t st_mtime_sec; > + __time_t st_ctime_sec; > unsigned int st_blksize; > int st_blocks; > unsigned int st_flags; > @@ -74,9 +74,9 @@ struct glibc21_stat > __gid_t st_gid; > __dev_t st_rdev; > __off_t st_size; > - __time_t st_atime; > - __time_t st_mtime; > - __time_t st_ctime; > + __time_t st_atime_sec; > + __time_t st_mtime_sec; > + __time_t st_ctime_sec; > __blkcnt64_t st_blocks; > __blksize_t st_blksize; > unsigned int st_flags; > diff --git a/sysdeps/unix/sysv/linux/alpha/xstatconv.c b/sysdeps/unix/sysv/linux/alpha/xstatconv.c > index f716a10f34..43224a7f25 100644 > --- a/sysdeps/unix/sysv/linux/alpha/xstatconv.c > +++ b/sysdeps/unix/sysv/linux/alpha/xstatconv.c > @@ -44,9 +44,9 @@ __xstat_conv (int vers, struct kernel_stat *kbuf, void *ubuf) > buf->st_gid = kbuf->st_gid; > buf->st_rdev = kbuf->st_rdev; > buf->st_size = kbuf->st_size; > - buf->st_atime = kbuf->st_atime; > - buf->st_mtime = kbuf->st_mtime; > - buf->st_ctime = kbuf->st_ctime; > + buf->st_atime_sec = kbuf->st_atime_sec; > + buf->st_mtime_sec = kbuf->st_mtime_sec; > + buf->st_ctime_sec = kbuf->st_ctime_sec; > buf->st_blksize = kbuf->st_blksize; > buf->st_blocks = kbuf->st_blocks; > buf->st_flags = kbuf->st_flags; > @@ -66,9 +66,9 @@ __xstat_conv (int vers, struct kernel_stat *kbuf, void *ubuf) > buf->st_gid = kbuf->st_gid; > buf->st_rdev = kbuf->st_rdev; > buf->st_size = kbuf->st_size; > - buf->st_atime = kbuf->st_atime; > - buf->st_mtime = kbuf->st_mtime; > - buf->st_ctime = kbuf->st_ctime; > + buf->st_atime_sec = kbuf->st_atime_sec; > + buf->st_mtime_sec = kbuf->st_mtime_sec; > + buf->st_ctime_sec = kbuf->st_ctime_sec; > buf->st_blocks = kbuf->st_blocks; > buf->st_blksize = kbuf->st_blksize; > buf->st_flags = kbuf->st_flags; > @@ -98,12 +98,12 @@ __xstat_conv (int vers, struct kernel_stat *kbuf, void *ubuf) > buf->st_nlink = kbuf->st_nlink; > buf->__pad0 = 0; > > - buf->st_atime = kbuf->st_atime; > - buf->st_atimensec = 0; > - buf->st_mtime = kbuf->st_mtime; > - buf->st_mtimensec = 0; > - buf->st_ctime = kbuf->st_ctime; > - buf->st_ctimensec = 0; > + buf->st_atim.tv_sec = kbuf->st_atime_sec; > + buf->st_atim.tv_nsec = 0; > + buf->st_mtim.tv_sec = kbuf->st_mtime_sec; > + buf->st_mtim.tv_nsec = 0; > + buf->st_ctim.tv_sec = kbuf->st_ctime_sec; > + buf->st_ctim.tv_nsec = 0; > > buf->__glibc_reserved[0] = 0; > buf->__glibc_reserved[1] = 0; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] alpha: Remove anonymous union in struct stat [BZ #27042] 2020-12-10 21:18 ` Adhemerval Zanella @ 2020-12-15 16:43 ` Matt Turner 2020-12-15 16:48 ` Adhemerval Zanella 2020-12-15 16:56 ` Florian Weimer 0 siblings, 2 replies; 10+ messages in thread From: Matt Turner @ 2020-12-15 16:43 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: GNU C Library On Thu, Dec 10, 2020 at 4:18 PM Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote: > On 09/12/2020 22:19, Matt Turner via Libc-alpha wrote: > > This is clever, but it confuses downstream detection in at least zstd > > and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro, > > which is not provided by the path using the anonymous union; glib checks > > for the presence of 'st_mtimensec' in struct stat but then tries to > > access that field in struct statx (which might be a bug on its own). > > --- > > The macros affect the definitions in kernel_stat.h, so rename them. Not > > thrilled. > > LGTM, thanks. > > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> Thank you. Should we wait for any other input? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] alpha: Remove anonymous union in struct stat [BZ #27042] 2020-12-15 16:43 ` Matt Turner @ 2020-12-15 16:48 ` Adhemerval Zanella 2020-12-15 16:56 ` Florian Weimer 1 sibling, 0 replies; 10+ messages in thread From: Adhemerval Zanella @ 2020-12-15 16:48 UTC (permalink / raw) To: Matt Turner; +Cc: GNU C Library On 15/12/2020 13:43, Matt Turner wrote: > On Thu, Dec 10, 2020 at 4:18 PM Adhemerval Zanella via Libc-alpha > <libc-alpha@sourceware.org> wrote: >> On 09/12/2020 22:19, Matt Turner via Libc-alpha wrote: >>> This is clever, but it confuses downstream detection in at least zstd >>> and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro, >>> which is not provided by the path using the anonymous union; glib checks >>> for the presence of 'st_mtimensec' in struct stat but then tries to >>> access that field in struct statx (which might be a bug on its own). >>> --- >>> The macros affect the definitions in kernel_stat.h, so rename them. Not >>> thrilled. >> >> LGTM, thanks. >> >> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > Thank you. Should we wait for any other input? I think you are already following Florian suggestion and it align of what other ABIs are already doing. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] alpha: Remove anonymous union in struct stat [BZ #27042] 2020-12-15 16:43 ` Matt Turner 2020-12-15 16:48 ` Adhemerval Zanella @ 2020-12-15 16:56 ` Florian Weimer 2020-12-17 18:18 ` Matt Turner 1 sibling, 1 reply; 10+ messages in thread From: Florian Weimer @ 2020-12-15 16:56 UTC (permalink / raw) To: Matt Turner via Libc-alpha * Matt Turner via Libc-alpha: > On Thu, Dec 10, 2020 at 4:18 PM Adhemerval Zanella via Libc-alpha > <libc-alpha@sourceware.org> wrote: >> On 09/12/2020 22:19, Matt Turner via Libc-alpha wrote: >> > This is clever, but it confuses downstream detection in at least zstd >> > and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro, >> > which is not provided by the path using the anonymous union; glib checks >> > for the presence of 'st_mtimensec' in struct stat but then tries to >> > access that field in struct statx (which might be a bug on its own). >> > --- >> > The macros affect the definitions in kernel_stat.h, so rename them. Not >> > thrilled. >> >> LGTM, thanks. >> >> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > Thank you. Should we wait for any other input? I think Adhemerval's review is sufficient. Thanks. Florian -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] alpha: Remove anonymous union in struct stat [BZ #27042] 2020-12-15 16:56 ` Florian Weimer @ 2020-12-17 18:18 ` Matt Turner 2020-12-17 19:10 ` Adhemerval Zanella 0 siblings, 1 reply; 10+ messages in thread From: Matt Turner @ 2020-12-17 18:18 UTC (permalink / raw) To: Florian Weimer; +Cc: Matt Turner via Libc-alpha, Adhemerval Zanella On Tue, Dec 15, 2020 at 11:56 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Matt Turner via Libc-alpha: > > > On Thu, Dec 10, 2020 at 4:18 PM Adhemerval Zanella via Libc-alpha > > <libc-alpha@sourceware.org> wrote: > >> On 09/12/2020 22:19, Matt Turner via Libc-alpha wrote: > >> > This is clever, but it confuses downstream detection in at least zstd > >> > and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro, > >> > which is not provided by the path using the anonymous union; glib checks > >> > for the presence of 'st_mtimensec' in struct stat but then tries to > >> > access that field in struct statx (which might be a bug on its own). > >> > --- > >> > The macros affect the definitions in kernel_stat.h, so rename them. Not > >> > thrilled. > >> > >> LGTM, thanks. > >> > >> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > > > Thank you. Should we wait for any other input? > > I think Adhemerval's review is sufficient. Thanks. Thank you both. Could I bother one of you to push the patch? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] alpha: Remove anonymous union in struct stat [BZ #27042] 2020-12-17 18:18 ` Matt Turner @ 2020-12-17 19:10 ` Adhemerval Zanella 0 siblings, 0 replies; 10+ messages in thread From: Adhemerval Zanella @ 2020-12-17 19:10 UTC (permalink / raw) To: Matt Turner, Florian Weimer; +Cc: Matt Turner via Libc-alpha On 17/12/2020 15:18, Matt Turner wrote: > On Tue, Dec 15, 2020 at 11:56 AM Florian Weimer <fweimer@redhat.com> wrote: >> >> * Matt Turner via Libc-alpha: >> >>> On Thu, Dec 10, 2020 at 4:18 PM Adhemerval Zanella via Libc-alpha >>> <libc-alpha@sourceware.org> wrote: >>>> On 09/12/2020 22:19, Matt Turner via Libc-alpha wrote: >>>>> This is clever, but it confuses downstream detection in at least zstd >>>>> and GNOME's glib. zstd has preprocessor tests for the 'st_mtime' macro, >>>>> which is not provided by the path using the anonymous union; glib checks >>>>> for the presence of 'st_mtimensec' in struct stat but then tries to >>>>> access that field in struct statx (which might be a bug on its own). >>>>> --- >>>>> The macros affect the definitions in kernel_stat.h, so rename them. Not >>>>> thrilled. >>>> >>>> LGTM, thanks. >>>> >>>> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> >>> >>> Thank you. Should we wait for any other input? >> >> I think Adhemerval's review is sufficient. Thanks. > > Thank you both. Could I bother one of you to push the patch? > I will handle it. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-12-17 19:10 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-09 16:20 [PATCH] alpha: Remove anonymous union in struct stat [BZ #27042] Matt Turner 2020-12-09 17:11 ` Florian Weimer 2020-12-10 1:25 ` Matt Turner 2020-12-10 1:19 ` [PATCHv2] " Matt Turner 2020-12-10 21:18 ` Adhemerval Zanella 2020-12-15 16:43 ` Matt Turner 2020-12-15 16:48 ` Adhemerval Zanella 2020-12-15 16:56 ` Florian Weimer 2020-12-17 18:18 ` Matt Turner 2020-12-17 19:10 ` Adhemerval Zanella
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).