On 05/21/2013 03:08 PM, Eric Blake wrote:
> On 05/21/2013 06:52 AM, Bernhard Voelker wrote:
>> + if (-1 == fd)
>> + {
>> + ASSERT ((fd = creat (BASE "naptmp", 0600)) != -1); /* Never closed.
>> */
>> + ASSERT (unlink (BASE "naptmp") == 0);
>
> unlink() of an open fd is not guaranteed to succeed, and indeed fails on
> mingw; you can also provoke the failure in some NFS setups. You'd need
> to add some cleanup (maybe an atexit hook will work) that closes and
> only then removes the witness file.
>
Good catch, thanks.
Here's a new version of the patch.
Have a nice day,
Berny
>From 503f61f658b5021225b4b1d0b749675c96de6419 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <m...@bernhard-voelker.de>
Date: Tue, 21 May 2013 15:24:54 +0200
Subject: [PATCH] tests/nap.h: avoid race
The recent change in nap.h (5191133e) decreased the probability of lost
races to about a third, however such problems could still be observed
in virtual machines and openSUSE's OBS.
Instead of calulating the nap() time once and using it (together with
a small correction multiplier), avoid the race alltogether by verifying
on a reference file whether a timestamp difference has happened.
* tests/nap.h (nap_works): Change return value to bool. Change passing
the old file's status by value instead of by reference as this function
does no longer update that timestamp; rename the function argument from
st to old_st. Remove the local variables cdiff and mdiff because that
function now returns true/false instead of the precise delay.
(guess_delay): Remove function.
(clear_tmp_file): Add new function to unlink the witness file.
(nap): Instead of re-using the delay which has been calulated during the
first call, avoid the race by actually verifying that a timestamp
difference can be observed on the current file system. Use an adaptive
approach for the delay to minimize execution time. Assert that the
maximum delay is <= 2 seconds. Use atexit to call clear_tmp_file
when the process terminates.
---
tests/nap.h | 107 ++++++++++++++++++++++++++----------------------------------
1 file changed, 46 insertions(+), 61 deletions(-)
diff --git a/tests/nap.h b/tests/nap.h
index d8dbad2..2102f98 100644
--- a/tests/nap.h
+++ b/tests/nap.h
@@ -20,6 +20,7 @@
# define GLTEST_NAP_H
# include <limits.h>
+#include <stdbool.h>
/* Return A - B, in ns.
Return 0 if the true result would be negative.
@@ -53,86 +54,70 @@ get_stat (int fd, struct stat *st, int do_write)
}
/* Given a file whose descriptor is FD, see whether delaying by DELAY
- nanoseconds causes a change in a file's time stamp. *ST is the
- file's status, recently gotten. Update *ST to reflect the latest
- status gotten. If successful, return the needed delay, in
- nanoseconds as determined by the observed time stamps; this may be
- greater than DELAY if we crossed a quantization boundary. If
- unsuccessful, return 0. */
-static int
-nap_works (int fd, int delay, struct stat *st)
+ nanoseconds causes a change in a file's time stamp. OLD_ST is the
+ file's status, recently gotten. */
+static bool
+nap_works (int fd, int delay, struct stat old_st)
{
- struct stat old_st = *st;
+ struct stat st;
struct timespec delay_spec;
- int cdiff, mdiff;
delay_spec.tv_sec = delay / 1000000000;
delay_spec.tv_nsec = delay % 1000000000;
ASSERT (nanosleep (&delay_spec, 0) == 0);
- get_stat (fd, st, 1);
+ get_stat (fd, &st, 1);
/* Return the greater of the ctime and the mtime differences, or
zero if it cannot be determined, or INT_MAX if either overflows. */
- cdiff = diff_timespec (get_stat_ctime (st), get_stat_ctime (&old_st));
- if (cdiff != 0)
- {
- mdiff = diff_timespec (get_stat_mtime (st), get_stat_mtime (&old_st));
- if (mdiff != 0)
- return cdiff < mdiff ? mdiff : cdiff;
- }
- return 0;
+ if ( diff_timespec (get_stat_ctime (&st), get_stat_ctime (&old_st))
+ && diff_timespec (get_stat_mtime (&st), get_stat_mtime (&old_st)))
+ return true;
+
+ return false;
}
-static int
-guess_delay (void)
+#define TEMPFILE BASE "nap.tmp"
+
+static void
+clear_temp_file (void)
{
- /* Try a 1-ns sleep first, for speed. If that doesn't work, try 100
- ns, 1 microsecond, 1 ms, etc. xfs has a quantization of about 10
- milliseconds, even though it has a granularity of 1 nanosecond,
- and NTFS has a default quantization of 15.25 milliseconds, even
- though it has a granularity of 100 nanoseconds, so 15.25 ms is a
- good quantization to try. If that doesn't work, try 1 second.
- The worst case is 2 seconds, needed for FAT. */
- static int const delaytab[] = {1, 1000, 1000000, 15250000, 1000000000 };
- int fd = creat (BASE "tmp", 0600);
- int i;
- int delay = 2000000000;
- struct stat st;
- ASSERT (0 <= fd);
- get_stat (fd, &st, 0);
- for (i = 0; i < sizeof delaytab / sizeof delaytab[0]; i++)
- {
- int d = nap_works (fd, delaytab[i], &st);
- if (d != 0)
- {
- delay = d;
- break;
- }
- }
- ASSERT (close (fd) == 0);
- ASSERT (unlink (BASE "tmp") == 0);
- return delay;
+ unlink (TEMPFILE);
}
/* Sleep long enough to notice a timestamp difference on the file
- system in the current directory. Assumes that BASE is defined,
- and requires that the test module depends on nanosleep. */
+ system in the current directory. Use an adaptive approach, trying
+ to find the smallest delay which works on the current file system
+ to make the timestamp difference appear. Assert a maximum delay of
+ 2 seconds. Assumes that BASE is defined, and requires that the test
+ module depends on nanosleep. */
static void
nap (void)
{
- static struct timespec delay;
- if (!delay.tv_sec && !delay.tv_nsec)
+ static int fd = -1;
+ struct stat old_st;
+ static int delay = 1;
+
+ if (-1 == fd)
+ {
+ ASSERT ((fd = creat (TEMPFILE, 0600)) != -1); /* Never closed. */
+ atexit (clear_temp_file);
+ get_stat (fd, &old_st, 0);
+ }
+ else
{
- int d = guess_delay ();
-
- /* Multiply by 1.125 (rounding up), to avoid problems if the
- file system's clock is a bit slower than nanosleep's.
- Ceiling it at INT_MAX, though. */
- int delta = (d >> 3) + ((d & 7) != 0);
- d = delta < INT_MAX - d ? d + delta : INT_MAX;
- delay.tv_sec = d / 1000000000;
- delay.tv_nsec = d % 1000000000;
+ ASSERT (0 <= fd);
+ get_stat (fd, &old_st, 1);
}
- ASSERT (nanosleep (&delay, 0) == 0);
+
+ if (1 < delay)
+ delay = delay / 2; /* Try half of the previous delay. */
+ ASSERT (0 < delay);
+
+ for ( ; delay <= 2000000000; delay = delay * 2)
+ if (nap_works (fd, delay, old_st))
+ return;
+
+ /* Bummer: even the highest nap delay didn't work. */
+ ASSERT (0);
}
#endif /* GLTEST_NAP_H */
--
1.8.2.2
