WT-2672 handle system calls that don't set errno (#2765)

Define system call success as a 0 return, and split error handling into two parts: if the call returns -1, use errno, otherwise expect the failing return to be an error value.

Replace calls to remove with unlink, so we know errno will be set.  Do the best we can with rename, there's no easy workaround.

POSIX requires posix_madvise return an errno value, but some OS versions return a -1/errno pair instead (at least FreeBSD and OS X). I don't care about retrying posix_madvise calls on failure, but since WT_SYSCALL_RETRY includes the necessary error handling magic, wrap the posix_madvise calls in WT_SYSCALL_RETRY.

(cherry picked from commit ced588aecd604ab56670b8cdf66b8105e58f4fe3)

6 files changed, 50 insertions, 32 deletions

diff --git a/dist/s_string.ok b/dist/s_string.ok
index eed034abb47..06173fe978c 100644
--- a/dist/s_string.ok
+++ b/dist/s_string.ok
@@ -439,6 +439,7 @@ bzDecompressInit
 bzalloc
 bzfree
 bzip
+call's
 calloc
 cas
 catfmt
diff --git a/src/include/os.h b/src/include/os.h
index 2ff41d39f46..44cceee6c40 100644
--- a/src/include/os.h
+++ b/src/include/os.h
@@ -17,15 +17,26 @@
 #define	WT_SYSCALL_RETRY(call, ret) do {				\
 	int __retry;							\
 	for (__retry = 0; __retry < 10; ++__retry) {			\
-		if ((call) == 0) {					\
-			(ret) = 0;					\
-			break;						\
-		}							\
-		switch ((ret) = __wt_errno()) {				\
-		case 0:							\
-			/* The call failed but didn't set errno. */	\
-			(ret) = WT_ERROR;				\
+		/*							\
+		 * A call returning 0 indicates success; any call where	\
+		 * 0 is not the only successful return must provide an	\
+		 * expression evaluating to 0 in all successful	cases.	\
+		 */							\
+		if (((ret) = (call)) == 0)				\
 			break;						\
+		/*							\
+		 * The call's error was either returned by the call or	\
+		 * is in errno, and there are cases where it depends on	\
+		 * the software release as to which it is (for example,	\
+		 * posix_fadvise on FreeBSD and OS X). Failing calls	\
+		 * must either return a non-zero error value, or -1 if	\
+		 * the error value is in errno. (The WiredTiger errno	\
+		 * function returns WT_ERROR if errno is 0, which isn't	\
+		 * ideal but won't discard the failure.)		\
+		 */							\
+		if ((ret) == -1)					\
+			(ret) = __wt_errno();				\
+		switch (ret) {						\
 		case EAGAIN:						\
 		case EBUSY:						\
 		case EINTR:						\
diff --git a/src/os_posix/os_dir.c b/src/os_posix/os_dir.c
index 78ae5f8edd4..02f12ec7311 100644
--- a/src/os_posix/os_dir.c
+++ b/src/os_posix/os_dir.c
@@ -36,7 +36,7 @@ __wt_posix_directory_list(WT_SESSION_IMPL *session, const char *dir,
 	dirsz = 0;
 	entries = NULL;
 
-	WT_SYSCALL_RETRY(((dirp = opendir(path)) == NULL ? 1 : 0), ret);
+	WT_SYSCALL_RETRY(((dirp = opendir(path)) == NULL ? -1 : 0), ret);
 	if (ret != 0)
 		WT_ERR_MSG(session, ret, "%s: directory-list: opendir", path);
 
diff --git a/src/os_posix/os_fs.c b/src/os_posix/os_fs.c
index 86aa8db8f4f..7d8f3b937b6 100644
--- a/src/os_posix/os_fs.c
+++ b/src/os_posix/os_fs.c
@@ -52,7 +52,7 @@ __posix_sync(WT_SESSION_IMPL *session,
 	 * "This is currently implemented on HFS, MS-DOS (FAT), and Universal
 	 * Disk Format (UDF) file systems."
 	 */
-	WT_SYSCALL_RETRY(fcntl(fd, F_FULLFSYNC, 0), ret);
+	WT_SYSCALL_RETRY(fcntl(fd, F_FULLFSYNC, 0) == -1 ? -1 : 0, ret);
 	if (ret == 0)
 		return (0);
 	/*
@@ -107,7 +107,7 @@ __posix_directory_sync(WT_SESSION_IMPL *session, const char *path)
 	}
 
 	WT_SYSCALL_RETRY((
-	    (fd = open(path, O_RDONLY, 0444)) == -1 ? 1 : 0), ret);
+	    (fd = open(path, O_RDONLY, 0444)) == -1 ? -1 : 0), ret);
 	if (ret != 0)
 		WT_ERR_MSG(session, ret, "%s: directory-sync: open", path);
 
@@ -172,14 +172,19 @@ __posix_file_remove(WT_SESSION_IMPL *session, const char *name)
 #endif
 
 	WT_RET(__wt_filename(session, name, &path));
-	name = path;
-
-	WT_SYSCALL_RETRY(remove(name), ret);
-	if (ret != 0)
-		__wt_err(session, ret, "%s: file-remove: remove", name);
 
+	/*
+	 * ISO C doesn't require remove return -1 on failure or set errno (note
+	 * POSIX 1003.1 extends C with those requirements). Regardless, use the
+	 * unlink system call, instead of remove, to simplify error handling;
+	 * where we're not doing any special checking for standards compliance,
+	 * using unlink may be marginally safer.
+	 */
+	WT_SYSCALL_RETRY(unlink(path), ret);
 	__wt_free(session, path);
-	return (ret);
+	if (ret == 0)
+		return (0);
+	WT_RET_MSG(session, ret, "%s: file-remove: unlink", name);
 }
 
 /*
@@ -203,18 +208,22 @@ __posix_file_rename(WT_SESSION_IMPL *session, const char *from, const char *to)
 
 	from_path = to_path = NULL;
 	WT_ERR(__wt_filename(session, from, &from_path));
-	from = from_path;
 	WT_ERR(__wt_filename(session, to, &to_path));
-	to = to_path;
 
-	WT_SYSCALL_RETRY(rename(from, to), ret);
-	if (ret != 0)
-		__wt_err(session, ret,
-		    "%s to %s: file-rename: rename", from, to);
+	/*
+	 * ISO C doesn't require rename return -1 on failure or set errno (note
+	 * POSIX 1003.1 extends C with those requirements). Be cautious, force
+	 * any non-zero return to -1 so we'll check errno. We can still end up
+	 * with the wrong errno (if errno is garbage), or the generic WT_ERROR
+	 * return (if errno is 0), but we've done the best we can.
+	 */
+	WT_SYSCALL_RETRY(rename(from_path, to_path) != 0 ? -1 : 0, ret);
 
 err:	__wt_free(session, from_path);
 	__wt_free(session, to_path);
-	return (ret);
+	if (ret == 0)
+		return (0);
+	WT_RET_MSG(session, ret, "%s to %s: file-rename: rename", from, to);
 }
 
 /*
@@ -360,7 +369,7 @@ __posix_handle_lock(WT_SESSION_IMPL *session, WT_FH *fh, bool lock)
 	fl.l_type = lock ? F_WRLCK : F_UNLCK;
 	fl.l_whence = SEEK_SET;
 
-	WT_SYSCALL_RETRY(fcntl(fh->fd, F_SETLK, &fl), ret);
+	WT_SYSCALL_RETRY(fcntl(fh->fd, F_SETLK, &fl) == -1 ? -1 : 0, ret);
 	if (ret == 0)
 		return (0);
 	WT_RET_MSG(session, ret, "%s: handle-lock: fcntl", fh->name);
@@ -560,7 +569,7 @@ __posix_handle_open(WT_SESSION_IMPL *session,
 		f |= O_CLOEXEC;
 #endif
 		WT_SYSCALL_RETRY((
-		    (fd = open(name, f, 0444)) == -1 ? 1 : 0), ret);
+		    (fd = open(name, f, 0444)) == -1 ? -1 : 0), ret);
 		if (ret != 0)
 			WT_ERR_MSG(session, ret, "%s: handle-open: open", name);
 		WT_ERR(__posix_handle_open_cloexec(session, fd, name));
@@ -622,7 +631,7 @@ __posix_handle_open(WT_SESSION_IMPL *session,
 #endif
 	}
 
-	WT_SYSCALL_RETRY(((fd = open(name, f, mode)) == -1 ? 1 : 0), ret);
+	WT_SYSCALL_RETRY(((fd = open(name, f, mode)) == -1 ? -1 : 0), ret);
 	if (ret != 0)
 		WT_ERR_MSG(session, ret,
 		    direct_io ?
diff --git a/src/os_posix/os_map.c b/src/os_posix/os_map.c
index de28891ffd1..e161e268f6d 100644
--- a/src/os_posix/os_map.c
+++ b/src/os_posix/os_map.c
@@ -98,6 +98,7 @@ __posix_map_preload_madvise(
 	if (size <= (size_t)conn->page_size ||
 	    (ret = posix_madvise(blk, size, POSIX_MADV_WILLNEED)) == 0)
 		return (0);
+
 	WT_RET_MSG(session, ret,
 	    "%s: memory-map preload: posix_madvise: POSIX_MADV_WILLNEED",
 	    fh->name);
@@ -145,6 +146,7 @@ __posix_map_discard_madvise(
 
 	if ((ret = posix_madvise(blk, size, POSIX_MADV_DONTNEED)) == 0)
 		return (0);
+
 	WT_RET_MSG(session, ret,
 	    "%s: memory-map discard: posix_madvise: POSIX_MADV_DONTNEED",
 	    fh->name);
diff --git a/src/os_win/os_fs.c b/src/os_win/os_fs.c
index 95c0ea40ce6..4ac613fc9f9 100644
--- a/src/os_win/os_fs.c
+++ b/src/os_win/os_fs.c
@@ -286,11 +286,6 @@ __win_handle_lock(WT_SESSION_IMPL *session, WT_FH *fh, bool lock)
 	 * WiredTiger requires this function be able to acquire locks past
 	 * the end of file.
 	 *
-	 * Note we're using fcntl(2) locking: all fcntl locks associated with a
-	 * file for a given process are removed when any file descriptor for the
-	 * file is closed by the process, even if a lock was never requested for
-	 * that file descriptor.
-	 *
 	 * http://msdn.microsoft.com/
 	 *    en-us/library/windows/desktop/aa365202%28v=vs.85%29.aspx
 	 *