Re: gnulib support for st_birthtime (second revision of patch)

[Paul Eggert]

Eric Blake <address@hidden> writes:

> I would recommend getting rid of any test for st_spare1 or st_spare4 
> in m4/stat-time.m4.

Thanks for mentioning this; I've always thought that using those
"spare" fields was asking for trouble.  I installed the following;
can you please check it?

> you should add a check for st_birthtim.tv_nsec,

That's also done in the patch below.  (Wasn't it already done?
Anyway, it should be done now.)

One other thing: the API for get_stat_birthtime was different from
get_stat_mtime etc. and was a bit unusual (returning int 1 for
success, 0 for failure -- at least it should have been a boolean).
Anyway, I thought it a bit nicer to use the same API for all the
get_stat_*time functions even if returning tv_nsec < 0 for error is a
bit odd.

2007-03-27  Paul Eggert  <address@hidden>

        * lib/stat-time.h (USE_BIRTHTIME): Remove.
        (get_stat_atime_ns, get_stat_ctime_ns, get_stat_mtime_ns):
        (get_stat_birthtime_ns): Do not try to use "spare" fields.
        (get_stat_birthtime_ns): Simplify compile-time tests.
        (get_stat_birthtime): Change the API to look like
        get_stat_mtime etc., except return a negative tv_nsec on error.
        * m4/stat-time.m4 (gl_STAT_TIME, gl_STAT_BIRTHTIME):
        Don't check for "spare" fields.
        (gl_STAT_BIRTHTIME): Don't check for struct stat.st_birthtimespec.tv_sec
        or for struct stat.st_birthtime, as these tests aren't used.
        * tests/test-stat-time.c (test_birthtime): Adjust to new API.

Index: lib/stat-time.h
===================================================================
RCS file: /cvsroot/gnulib/gnulib/lib/stat-time.h,v
retrieving revision 1.5
diff -u -p -r1.5 stat-time.h
--- lib/stat-time.h     27 Mar 2007 11:01:11 -0000      1.5
+++ lib/stat-time.h     27 Mar 2007 18:51:14 -0000
@@ -45,13 +45,6 @@
 # define STAT_TIMESPEC_NS(st, st_xtim) ((st)->st_xtim.st__tim.tv_nsec)
 #endif

-#if defined HAVE_STRUCT_STAT_ST_BIRTHTIME || defined 
HAVE_STRUCT_STAT_ST_BIRTHTIMENSEC || defined 
HAVE_STRUCT_STAT_ST_BIRTHTIMESPEC_TV_NSEC || defined HAVE_STRUCT_STAT_ST_SPARE4
-# define USE_BIRTHTIME 1
-#else
-# undef USE_BIRTHTIME
-#endif
-
-
 /* Return the nanosecond component of *ST's access time.  */
 static inline long int
 get_stat_atime_ns (struct stat const *st)
@@ -60,8 +53,6 @@ get_stat_atime_ns (struct stat const *st
   return STAT_TIMESPEC (st, st_atim).tv_nsec;
 # elif defined STAT_TIMESPEC_NS
   return STAT_TIMESPEC_NS (st, st_atim);
-# elif defined HAVE_STRUCT_STAT_ST_SPARE1
-  return st->st_spare1 * 1000;
 # else
   return 0;
 # endif
@@ -75,8 +66,6 @@ get_stat_ctime_ns (struct stat const *st
   return STAT_TIMESPEC (st, st_ctim).tv_nsec;
 # elif defined STAT_TIMESPEC_NS
   return STAT_TIMESPEC_NS (st, st_ctim);
-# elif defined HAVE_STRUCT_STAT_ST_SPARE1
-  return st->st_spare3 * 1000;
 # else
   return 0;
 # endif
@@ -90,8 +79,6 @@ get_stat_mtime_ns (struct stat const *st
   return STAT_TIMESPEC (st, st_mtim).tv_nsec;
 # elif defined STAT_TIMESPEC_NS
   return STAT_TIMESPEC_NS (st, st_mtim);
-# elif defined HAVE_STRUCT_STAT_ST_SPARE1
-  return st->st_spare2 * 1000;
 # else
   return 0;
 # endif
@@ -101,22 +88,13 @@ get_stat_mtime_ns (struct stat const *st
 static inline long int
 get_stat_birthtime_ns (struct stat const *st)
 {
-# if defined USE_BIRTHTIME
-#  if defined STAT_TIMESPEC && defined 
HAVE_STRUCT_STAT_ST_BIRTHTIMESPEC_TV_NSEC
+# if defined HAVE_STRUCT_STAT_ST_BIRTHTIMESPEC_TV_NSEC
   return STAT_TIMESPEC (st, st_birthtim).tv_nsec;
-#  elif defined STAT_TIMESPEC_NS && defined 
HAVE_STRUCT_STAT_ST_BIRTHTIMESPEC_TV_SEC
+# elif defined HAVE_STRUCT_STAT_ST_BIRTHTIMENSEC
   return STAT_TIMESPEC_NS (st, st_birthtim);
-#  elif defined HAVE_STRUCT_STAT_ST_SPARE4
-  /* Cygwin, without __CYGWIN_USE_BIG_TYPES__ */
-  return st->st_spare4[1] * 1000L;
-#  else
-  /* Birthtime is available, but not at nanosecond resolution.  */
-  return 0;
-#  endif
 # else
-  /* Birthtime is not available, so indicate this in the returned value.  */
   return 0;
-# endif 
+# endif
 }

 /* Return *ST's access time.  */
@@ -161,69 +139,41 @@ get_stat_mtime (struct stat const *st)
 #endif
 }

-/* Return *ST's birth time, if available, in *PTS.  A nonzero value is
- * returned if the stat structure appears to indicate that the
- * timestamp is available.
- *
- * The return value of this function does not reliably indicate that the 
- * returned data is valid; see the comments within the body of the 
- * function for an explanation.
- */
-static inline int
-get_stat_birthtime (struct stat const *st,
-                   struct timespec *pts)
-{
-#if defined USE_BIRTHTIME
-# ifdef STAT_TIMESPEC
-  *pts = STAT_TIMESPEC (st, st_birthtim);
-# else
+/* Return *ST's birth time, if available; otherwise return a value
+   with negative tv_nsec.  */
+static inline struct timespec
+get_stat_birthtime (struct stat const *st)
+{
   struct timespec t;
-  pts->tv_sec = st->st_birthtime;
-  pts->tv_nsec = get_stat_birthtime_ns (st);
-# endif

-  /* NetBSD sometimes signals the absence of knowledge of the file's
-   * birth time by using zero.  We indicate we don't know, by
-   * returning 0 from this function when that happens.  This is
-   * slightly problematic since (time_t)0 is otherwise a valid, albeit
-   * unlikely, timestamp.  
-   *
-   * NetBSD sometimes returns 0 for unknown values (for example on
-   * ffs) and sometimes begative values for tv_nsec (for example on
-   * NFS).  For some filesystems (e.g. msdos) NetBSD also appears to
-   * fail to update the st_birthtime member at all, and just leaves in
-   * there whatever junk existed int he uninitialised stat structure
-   * the caller provided.  Therefore, callers are advised to initialise
-   * the tv_nsec number to a negative value before they call stat in
-   * order to detect this problem.
-   */
-  if (pts->tv_sec == (time_t)0)
-    {
-      return 0;                        /* result probably invalid, see above. 
*/
-    }
-  else
-    {
-      /* Sometimes NetBSD returns junk in the birth time fields, so 
-       * do a simple range check on the data, and return 0 to indicate
-       * that the data is invalid if it just looks wrong. 
-       */
-      return (pts->tv_nsec >= 0) && (pts->tv_nsec <= 1000000000);
-    }
-#elif (defined _WIN32 || defined __WIN32__) && !defined __CYGWIN__
-  /* Woe32 native platforms (mingw, msvc, but not Cygwin) put the
-   * "file creation time" in st_ctime (!).  See for example the
-   * article
-   * <http://msdn2.microsoft.com/de-de/library/14h5k7ff(VS.80).aspx>
-   */
-  pts->tv_sec = st->st_ctime;
-  pts->tv_nsec = 0;
-  return 1;                    /* result is valid */
+#ifdef HAVE_STRUCT_STAT_ST_BIRTHTIMESPEC_TV_NSEC
+  t = STAT_TIMESPEC (st, st_birthtim);
+#elif defined HAVE_STRUCT_STAT_ST_BIRTHTIMENSEC
+  t.tv_sec = st->st_birthtime;
+  t.tv_nsec = st->st_birthtimensec;
+
+  /* NetBSD sometimes signals the absence of knowledge by using zero.
+     Attempt to work around this bug.  This sometimes reports failure
+     even for valid time stamps.  Also, sometimes NetBSD returns junk
+     in the birth time fields; work around this bug if it it is
+     detected.  There's no need to detect negative tv_nsec junk as
+     negative tv_nsec already indicates an error.  */
+  if (t.tv_sec == 0 || 1000000000 <= t.tv_nsec)
+    t.tv_nsec = -1;
+
+#elif (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
+  /* Woe32 native platforms (but not Cygwin) put the "file creation
+     time" in st_ctime (!).  See
+     <http://msdn2.microsoft.com/de-de/library/14h5k7ff(VS.80).aspx>.  */
+  t.tv_sec = st->st_ctime;
+  t.tv_nsec = 0;
 #else
-  /* Birth time not supported.  */
-  pts->tv_sec = 0;
-  pts->tv_nsec = 0;
-  return 0;                    /* result is not valid */
+  /* Birth time is not supported.  Set tv_sec to avoid undefined behavior.  */
+  t.tv_sec = -1;
+  t.tv_nsec = -1;
 #endif
+
+  return t;
 }

 #endif
Index: m4/stat-time.m4
===================================================================
RCS file: /cvsroot/gnulib/gnulib/m4/stat-time.m4,v
retrieving revision 1.5
diff -u -p -r1.5 stat-time.m4
--- m4/stat-time.m4     27 Mar 2007 11:01:11 -0000      1.5
+++ m4/stat-time.m4     27 Mar 2007 18:51:14 -0000
@@ -13,9 +13,8 @@ dnl From Paul Eggert.
 # st_atimespec.tv_nsec - FreeBSD, NetBSD, if ! defined _POSIX_SOURCE
 # st_atimensec - FreeBSD, NetBSD, if defined _POSIX_SOURCE
 # st_atim.st__tim.tv_nsec - UnixWare (at least 2.1.2 through 7.1)
-# st_spare1 - Cygwin?

-# st_birthtimespec present on NetBSD (probably also FreBSD, OpenBSD)
+# st_birthtimespec - FreeBSD, NetBSD (hidden on OpenBSD 3.9, anyway)

 AC_DEFUN([gl_STAT_TIME],
 [
@@ -49,11 +48,7 @@ AC_DEFUN([gl_STAT_TIME],
      fi],
     [AC_CHECK_MEMBERS([struct stat.st_atimespec.tv_nsec], [],
        [AC_CHECK_MEMBERS([struct stat.st_atimensec], [],
-         [AC_CHECK_MEMBERS([struct stat.st_atim.st__tim.tv_nsec], [],
-            [AC_CHECK_MEMBERS([struct stat.st_spare1], [],
-               [],
-               [#include <sys/types.h>
-                #include <sys/stat.h>])],
+         [AC_CHECK_MEMBERS([struct stat.st_atim.st__tim.tv_nsec], [], [],
             [#include <sys/types.h>
              #include <sys/stat.h>])],
          [#include <sys/types.h>
@@ -64,23 +59,20 @@ AC_DEFUN([gl_STAT_TIME],
      #include <sys/stat.h>])
 ])

-# Checks for st_birthtime, which is a feature from UFS2 (FreeBSD, NetBSD, 
OpenBSD, etc.)
-# There was a time when this field was named st_createtime (21 June 2002 to 16 
July 2002)
-# But that window is very small and applied only to development code, so 
systems still
-# using that configuration are not a realistic development target.
-# See revisions 1.10 and 1.11 of FreeBSD's src/sys/ufs/ufs/dinode.h.
+# Check for st_birthtime, a feature from UFS2 (FreeBSD, NetBSD, OpenBSD, etc.).
+# There was a time when this field was named st_createtime (21 June
+# 2002 to 16 July 2002) But that window is very small and applied only
+# to development code, so systems still using that configuration are
+# not supported.  See revisions 1.10 and 1.11 of FreeBSD's
+# src/sys/ufs/ufs/dinode.h.
 #
 AC_DEFUN([gl_STAT_BIRTHTIME],
 [
   AC_REQUIRE([AC_C_INLINE])
   AC_REQUIRE([gl_USE_SYSTEM_EXTENSIONS])
   AC_CHECK_HEADERS_ONCE([sys/time.h])
-  AC_CHECK_MEMBERS([struct stat.st_birthtimespec.tv_sec, struct 
stat.st_birthtimespec.tv_nsec], [],
-    [AC_CHECK_MEMBERS([struct stat.st_birthtime, struct 
stat.st_birthtimensec], [], 
-       [AC_CHECK_MEMBERS([struct stat.st_spare4], [], 
-          [],
-          [#include <sys/types.h>
-           #include <sys/stat.h>])],
+  AC_CHECK_MEMBERS([struct stat.st_birthtimespec.tv_nsec], [],
+    [AC_CHECK_MEMBERS([struct stat.st_birthtimensec], [], [],
        [#include <sys/types.h>
        #include <sys/stat.h>])],
     [#include <sys/types.h>
Index: tests/test-stat-time.c
===================================================================
RCS file: /cvsroot/gnulib/gnulib/tests/test-stat-time.c,v
retrieving revision 1.2
diff -u -p -r1.2 test-stat-time.c
--- tests/test-stat-time.c      27 Mar 2007 16:53:53 -0000      1.2
+++ tests/test-stat-time.c      27 Mar 2007 18:51:14 -0000
@@ -135,10 +135,9 @@ test_birthtime (const struct stat *stati
   /* Collect the birth times.. */
   for (i = 0; i < NFILES; ++i)
     {
-      if (!get_stat_birthtime (&statinfo[i], &birthtimes[i]))
-       {
-         return;
-       }
+      birthtimes[i] = get_stat_birthtime (&statinfo[i]);
+      if (birthtimes[i].tv_nsec < 0)
+       return;
     }

   ASSERT (modtimes[0].tv_sec < birthtimes[1].tv_sec); /* mtime(stamp1) < 
birthtime(renamed) */