diff mbox

[RFC] __fxstat: replace if() with #if when checking version

Message ID 20160906155918.GA24871@yury-N73SV
State New, archived
Headers

Commit Message

Yury Norov Sept. 6, 2016, 3:59 p.m. UTC 
  On Tue, Sep 06, 2016 at 03:08:58PM +0200, Andreas Schwab wrote:
> 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.

OK, now I see. It seems like STAT_IS_KERNEL_STAT hint is used not
optimal way. In my understanding, it forces user pass vers == _STAT_VER_KERNEL.
But if so, glibc may avoid generating the text of __xstat_conv(), and
so no struct kernel_stat is needed. Take a look at the patch below. 
Does it make sense to you?

Yury.

Comments

Yury Norov Sept. 6, 2016, 4:06 p.m. UTC | #1 
On Tue, Sep 06, 2016 at 06:59:18PM +0300, Yury Norov wrote:
> On Tue, Sep 06, 2016 at 03:08:58PM +0200, Andreas Schwab wrote:
> > 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.
> 
> OK, now I see. It seems like STAT_IS_KERNEL_STAT hint is used not
> optimal way. In my understanding, it forces user pass vers == _STAT_VER_KERNEL.
> But if so, glibc may avoid generating the text of __xstat_conv(), and
> so no struct kernel_stat is needed. Take a look at the patch below. 
> Does it make sense to you?
> 
> Yury.

Sorry, I was wrong. Everything is right.
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/fxstat.c b/sysdeps/unix/sysv/linux/fxstat.c
index e33023b..e9777df 100644
--- a/sysdeps/unix/sysv/linux/fxstat.c
+++ b/sysdeps/unix/sysv/linux/fxstat.c
@@ -35,20 +35,25 @@ 
 int
 __fxstat (int vers, int fd, struct stat *buf)
 {
+#ifdef STAT_IS_KERNEL_STAT
   if (vers == _STAT_VER_KERNEL)
     return INLINE_SYSCALL (fstat, 2, fd, buf);
-
-#ifdef STAT_IS_KERNEL_STAT
-  return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
+  else
+    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
 #else
-  struct kernel_stat kbuf;
-  int result;
+  if (vers == _STAT_VER_KERNEL)
+    return INLINE_SYSCALL (fstat, 2, fd, buf);
+  else
+    {
+      struct kernel_stat kbuf;
+      int result;
 
-  result = INLINE_SYSCALL (fstat, 2, fd, &kbuf);
-  if (result == 0)
-    result = __xstat_conv (vers, &kbuf, buf);
+      result = INLINE_SYSCALL (fstat, 2, fd, &kbuf);
+      if (result == 0)
+	result = __xstat_conv (vers, &kbuf, buf);
 
-  return result;
+      return result;
+    }
 #endif
 }