| Message ID | 1473166521-24827-1-git-send-email-ynorov@caviumnetworks.com |
|---|---|
| State | New |
| Headers | show |
On Sep 06 2016, Yury Norov <ynorov@caviumnetworks.com> wrote: > __fxstat() is always passed with _STAT_VER as vers parameter and it's > in internal namespace. It is part of the glibc ABI. > If patch is found reasonable, I can check and fix other platforms and > stat syscalls. Could someone explain me, what for we need 'vers', > if we pass it with the only value everywhere. Maybe it's time to remove it > completely? _STAT_VER has changed over time. Andreas.
On Tue, Sep 6, 2016 at 8:55 AM, Yury Norov <ynorov@caviumnetworks.com> wrote: > __fxstat() is always passed with _STAT_VER as vers parameter and it's > in internal namespace. > > It means we can replace runtime argument check with compile-time #ifdef. > It helps to make the body of _fxstat() smaller, and suppresses errors > if struct kernel_stat or __xstat_conv() is not declared, when they > are not needed. We can't do this. I think you may have already figured this out, but let me write out a clear explanation for the benefit of future people who may have the same idea. _STAT_VER exists because the contents of struct stat have changed over time. When an old executable is used with a new C library, the 'vers' argument will indicate that the executable expects the old layout, and the library will translate from the kernel's (presumably newer) stat layout to the executable's older expectations. It is the same problem that the kernel solves by changing the syscall numbers for stat/fstat/lstat whenever they change the structure. Therefore, the 'vers' argument *can* vary at runtime, even though all the code in the *current* source tree always passes the same value. To make this work, we have inline functions in <sys/stat.h>, backed up by out-of-line copies in libc_nonshared.a, that present the documented API (stat/fstat/lstat) and call __xstat/__fxstat/__lxstat with the additional argument. That is why these __-name functions are part of the public ABI for libc.so. Same principle as __printf_chk for instance. If we were designing this mechanism today from scratch we would probably use symbol versions - unless there's some nonobvious reason why that wouldn't work - but this mechanism *predates* symbol versioning. It has been around since the 1990s if not earlier. Maybe you could look into arranging to use symbol versions from now (or better, the next time the kernel changes the stat structure) going forward. But the old __(|f|l)xstat symbols need to stick around for binary compatibility, and their implementations need to test their 'vers' argument at runtime. zw
diff --git a/sysdeps/unix/sysv/linux/fxstat.c b/sysdeps/unix/sysv/linux/fxstat.c index e33023b..2e79f8b 100644 --- a/sysdeps/unix/sysv/linux/fxstat.c +++ b/sysdeps/unix/sysv/linux/fxstat.c @@ -35,20 +35,22 @@ int __fxstat (int vers, int fd, struct stat *buf) { - if (vers == _STAT_VER_KERNEL) - return INLINE_SYSCALL (fstat, 2, fd, buf); +#if _STAT_VER == _STAT_VER_KERNEL + return INLINE_SYSCALL (fstat, 2, fd, buf); +#else -#ifdef STAT_IS_KERNEL_STAT +# ifdef STAT_IS_KERNEL_STAT return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL); -#else +# else struct kernel_stat kbuf; int result; result = INLINE_SYSCALL (fstat, 2, fd, &kbuf); if (result == 0) - result = __xstat_conv (vers, &kbuf, buf); + result = __xstat_conv (_STAT_VER, &kbuf, buf); return result; +# endif #endif }
__fxstat() is always passed with _STAT_VER as vers parameter and it's in internal namespace. It means we can replace runtime argument check with compile-time #ifdef. It helps to make the body of _fxstat() smaller, and suppresses errors if struct kernel_stat or __xstat_conv() is not declared, when they are not needed. If patch is found reasonable, I can check and fix other platforms and stat syscalls. Could someone explain me, what for we need 'vers', if we pass it with the only value everywhere. Maybe it's time to remove it completely? * sysdeps/unix/sysv/linux/fxstat.c: Replace if() with #if when checking version Signed-off-by: Yury Norov <ynorov@caviumnetworks.com> --- sysdeps/unix/sysv/linux/fxstat.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)