Don't HANDLE_EINTR(close). Either IGNORE_EINTR(close) or just close.
It is incorrect to wrap close in HANDLE_EINTR on Linux. Correctness is
generally undefined on Mac, but as of r223369, it is incorrect in Chrome on
Mac.
To avoid new offenders, a PRESUBMIT check ensures that HANDLE_EINTR is not
used with close, and that IGNORE_EINTR is only used with close. Unnecessary
#includes of eintr_wrapper.h are also removed.
base/posix/einter_wrapper.h, PRESUBMIT.py, and ppapi/tests/test_broker.cc
contain non-mechanical changes. Variable naming within the latter is updated
per r178174. Missing #includes for <errno.h> in
content/zygote/zygote_main_linux.cc and tools/android/common/daemon.cc were
manually added. Mechanical changes were generated by running:
sed -E -i '' \
-e 's/((=|if|return|CHECK|EXPECT|ASSERT).*)HANDLE(_EINTR\(.*close)/\1IGNORE\3/' \
-e 's/(ignore_result|void ?)\(HANDLE_EINTR\((.*close\(.*)\)\)/\2/' \
-e 's/(\(void\) ?)?HANDLE_EINTR\((.*close\(.*)\)/\2/' \
$(git grep -El 'HANDLE_EINTR.*close')
sed -E -i '' -e '/#include.*eintr_wrapper\.h"/d' \
$(grep -EL '(HANDLE|IGNORE)_EINTR' \
$(git grep -El '#include.*eintr_wrapper\.h"'))
BUG=269623
R=agl@chromium.org, jln@chromium.org
TBR=OWNERS
Review URL: https://codereview.chromium.org/100253002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@238390 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: d89eec809e2d25aee8d386186653699f5017b15b
diff --git a/base/debug/debugger_posix.cc b/base/debug/debugger_posix.cc
index cab4c48..60ad521 100644
--- a/base/debug/debugger_posix.cc
+++ b/base/debug/debugger_posix.cc
@@ -157,7 +157,7 @@
char buf[1024];
ssize_t num_read = HANDLE_EINTR(read(status_fd, buf, sizeof(buf)));
- if (HANDLE_EINTR(close(status_fd)) < 0)
+ if (IGNORE_EINTR(close(status_fd)) < 0)
return false;
if (num_read <= 0)
diff --git a/base/file_util.h b/base/file_util.h
index 35225c7..acced28 100644
--- a/base/file_util.h
+++ b/base/file_util.h
@@ -414,7 +414,7 @@
public:
inline void operator()(int* x) const {
if (x && *x >= 0) {
- if (HANDLE_EINTR(close(*x)) < 0)
+ if (IGNORE_EINTR(close(*x)) < 0)
DPLOG(ERROR) << "close";
}
}
diff --git a/base/file_util_posix.cc b/base/file_util_posix.cc
index b6a4945..0557350 100644
--- a/base/file_util_posix.cc
+++ b/base/file_util_posix.cc
@@ -538,7 +538,7 @@
int fd = CreateAndOpenFdForTemporaryFile(directory, path);
if (fd < 0)
return false;
- ignore_result(HANDLE_EINTR(close(fd)));
+ close(fd);
return true;
}
@@ -557,14 +557,14 @@
FILE* file = fdopen(fd, "a+");
if (!file)
- ignore_result(HANDLE_EINTR(close(fd)));
+ close(fd);
return file;
}
bool CreateTemporaryFileInDir(const FilePath& dir, FilePath* temp_file) {
base::ThreadRestrictions::AssertIOAllowed(); // For call to close().
int fd = CreateAndOpenFdForTemporaryFile(dir, temp_file);
- return ((fd >= 0) && !HANDLE_EINTR(close(fd)));
+ return ((fd >= 0) && !IGNORE_EINTR(close(fd)));
}
static bool CreateTemporaryDirInDirImpl(const FilePath& base_dir,
@@ -740,7 +740,7 @@
return -1;
ssize_t bytes_read = HANDLE_EINTR(read(fd, data, size));
- if (int ret = HANDLE_EINTR(close(fd)) < 0)
+ if (int ret = IGNORE_EINTR(close(fd)) < 0)
return ret;
return bytes_read;
}
@@ -752,7 +752,7 @@
return -1;
int bytes_written = WriteFileDescriptor(fd, data, size);
- if (int ret = HANDLE_EINTR(close(fd)) < 0)
+ if (int ret = IGNORE_EINTR(close(fd)) < 0)
return ret;
return bytes_written;
}
@@ -779,7 +779,7 @@
return -1;
int bytes_written = WriteFileDescriptor(fd, data, size);
- if (int ret = HANDLE_EINTR(close(fd)) < 0)
+ if (int ret = IGNORE_EINTR(close(fd)) < 0)
return ret;
return bytes_written;
}
@@ -936,7 +936,7 @@
int outfile = HANDLE_EINTR(creat(to_path.value().c_str(), 0666));
if (outfile < 0) {
- ignore_result(HANDLE_EINTR(close(infile)));
+ close(infile);
return false;
}
@@ -967,9 +967,9 @@
} while (bytes_written_per_read < bytes_read);
}
- if (HANDLE_EINTR(close(infile)) < 0)
+ if (IGNORE_EINTR(close(infile)) < 0)
result = false;
- if (HANDLE_EINTR(close(outfile)) < 0)
+ if (IGNORE_EINTR(close(outfile)) < 0)
result = false;
return result;
diff --git a/base/files/dir_reader_linux.h b/base/files/dir_reader_linux.h
index 3e0721e..cb0cbd3 100644
--- a/base/files/dir_reader_linux.h
+++ b/base/files/dir_reader_linux.h
@@ -37,7 +37,7 @@
~DirReaderLinux() {
if (fd_ >= 0) {
- if (HANDLE_EINTR(close(fd_)))
+ if (IGNORE_EINTR(close(fd_)))
RAW_LOG(ERROR, "Failed to close directory handle");
}
}
diff --git a/base/files/file_path_watcher_kqueue.cc b/base/files/file_path_watcher_kqueue.cc
index 2ffb836..e035f22 100644
--- a/base/files/file_path_watcher_kqueue.cc
+++ b/base/files/file_path_watcher_kqueue.cc
@@ -210,7 +210,7 @@
return;
}
- if (HANDLE_EINTR(close(*fd)) != 0) {
+ if (IGNORE_EINTR(close(*fd)) != 0) {
DPLOG(ERROR) << "close";
}
*fd = kNoFileDescriptor;
@@ -497,7 +497,7 @@
if (!is_cancelled()) {
set_cancelled();
kqueue_watcher_.StopWatchingFileDescriptor();
- if (HANDLE_EINTR(close(kqueue_)) != 0) {
+ if (IGNORE_EINTR(close(kqueue_)) != 0) {
DPLOG(ERROR) << "close kqueue";
}
kqueue_ = -1;
diff --git a/base/files/file_posix.cc b/base/files/file_posix.cc
index 2bd9cd0..028a382 100644
--- a/base/files/file_posix.cc
+++ b/base/files/file_posix.cc
@@ -221,7 +221,7 @@
bool ClosePlatformFile(PlatformFile file) {
base::ThreadRestrictions::AssertIOAllowed();
- return !HANDLE_EINTR(close(file));
+ return !IGNORE_EINTR(close(file));
}
int64 SeekPlatformFile(PlatformFile file,
diff --git a/base/files/memory_mapped_file_posix.cc b/base/files/memory_mapped_file_posix.cc
index ba00946..c4c477a 100644
--- a/base/files/memory_mapped_file_posix.cc
+++ b/base/files/memory_mapped_file_posix.cc
@@ -9,7 +9,6 @@
#include <unistd.h>
#include "base/logging.h"
-#include "base/posix/eintr_wrapper.h"
#include "base/threading/thread_restrictions.h"
namespace base {
@@ -44,7 +43,7 @@
if (data_ != NULL)
munmap(data_, length_);
if (file_ != kInvalidPlatformFileValue)
- ignore_result(HANDLE_EINTR(close(file_)));
+ close(file_);
data_ = NULL;
length_ = 0;
diff --git a/base/memory/discardable_memory_android.cc b/base/memory/discardable_memory_android.cc
index 3850439..8fee53e 100644
--- a/base/memory/discardable_memory_android.cc
+++ b/base/memory/discardable_memory_android.cc
@@ -12,7 +12,6 @@
#include "base/file_util.h"
#include "base/lazy_instance.h"
#include "base/logging.h"
-#include "base/posix/eintr_wrapper.h"
#include "base/synchronization/lock.h"
#include "third_party/ashmem/ashmem.h"
diff --git a/base/message_loop/message_loop_unittest.cc b/base/message_loop/message_loop_unittest.cc
index f378db2..e6d25ec 100644
--- a/base/message_loop/message_loop_unittest.cc
+++ b/base/message_loop/message_loop_unittest.cc
@@ -944,9 +944,9 @@
// and don't run the message loop, just destroy it.
}
}
- if (HANDLE_EINTR(close(pipefds[0])) < 0)
+ if (IGNORE_EINTR(close(pipefds[0])) < 0)
PLOG(ERROR) << "close";
- if (HANDLE_EINTR(close(pipefds[1])) < 0)
+ if (IGNORE_EINTR(close(pipefds[1])) < 0)
PLOG(ERROR) << "close";
}
@@ -969,9 +969,9 @@
controller.StopWatchingFileDescriptor();
}
}
- if (HANDLE_EINTR(close(pipefds[0])) < 0)
+ if (IGNORE_EINTR(close(pipefds[0])) < 0)
PLOG(ERROR) << "close";
- if (HANDLE_EINTR(close(pipefds[1])) < 0)
+ if (IGNORE_EINTR(close(pipefds[1])) < 0)
PLOG(ERROR) << "close";
}
diff --git a/base/message_loop/message_pump_libevent.cc b/base/message_loop/message_pump_libevent.cc
index 6d862d1..26be687 100644
--- a/base/message_loop/message_pump_libevent.cc
+++ b/base/message_loop/message_pump_libevent.cc
@@ -125,11 +125,11 @@
event_del(wakeup_event_);
delete wakeup_event_;
if (wakeup_pipe_in_ >= 0) {
- if (HANDLE_EINTR(close(wakeup_pipe_in_)) < 0)
+ if (IGNORE_EINTR(close(wakeup_pipe_in_)) < 0)
DPLOG(ERROR) << "close";
}
if (wakeup_pipe_out_ >= 0) {
- if (HANDLE_EINTR(close(wakeup_pipe_out_)) < 0)
+ if (IGNORE_EINTR(close(wakeup_pipe_out_)) < 0)
DPLOG(ERROR) << "close";
}
event_base_free(event_base_);
diff --git a/base/message_loop/message_pump_libevent_unittest.cc b/base/message_loop/message_pump_libevent_unittest.cc
index 52ca95b..bf6d21c 100644
--- a/base/message_loop/message_pump_libevent_unittest.cc
+++ b/base/message_loop/message_pump_libevent_unittest.cc
@@ -30,9 +30,9 @@
}
virtual void TearDown() OVERRIDE {
- if (HANDLE_EINTR(close(pipefds_[0])) < 0)
+ if (IGNORE_EINTR(close(pipefds_[0])) < 0)
PLOG(ERROR) << "close";
- if (HANDLE_EINTR(close(pipefds_[1])) < 0)
+ if (IGNORE_EINTR(close(pipefds_[1])) < 0)
PLOG(ERROR) << "close";
}
diff --git a/base/platform_file_posix.cc b/base/platform_file_posix.cc
index 2bd9cd0..028a382 100644
--- a/base/platform_file_posix.cc
+++ b/base/platform_file_posix.cc
@@ -221,7 +221,7 @@
bool ClosePlatformFile(PlatformFile file) {
base::ThreadRestrictions::AssertIOAllowed();
- return !HANDLE_EINTR(close(file));
+ return !IGNORE_EINTR(close(file));
}
int64 SeekPlatformFile(PlatformFile file,
diff --git a/base/posix/eintr_wrapper.h b/base/posix/eintr_wrapper.h
index 8e26752..854c43a 100644
--- a/base/posix/eintr_wrapper.h
+++ b/base/posix/eintr_wrapper.h
@@ -9,6 +9,9 @@
// caller will nonetheless see an EINTR in Debug builds.
//
// On Windows, this wrapper macro does nothing.
+//
+// Don't wrap close calls in HANDLE_EINTR. Use IGNORE_EINTR if the return
+// value of close is significant. See http://crbug.com/269623.
#ifndef BASE_POSIX_EINTR_WRAPPER_H_
#define BASE_POSIX_EINTR_WRAPPER_H_
@@ -20,6 +23,7 @@
#include <errno.h>
#if defined(NDEBUG)
+
#define HANDLE_EINTR(x) ({ \
typeof(x) eintr_wrapper_result; \
do { \
@@ -42,9 +46,21 @@
#endif // NDEBUG
+#define IGNORE_EINTR(x) ({ \
+ typeof(x) eintr_wrapper_result; \
+ do { \
+ eintr_wrapper_result = (x); \
+ if (eintr_wrapper_result == -1 && errno == EINTR) { \
+ eintr_wrapper_result = 0; \
+ } \
+ } while (0); \
+ eintr_wrapper_result; \
+})
+
#else
#define HANDLE_EINTR(x) (x)
+#define IGNORE_EINTR(x) (x)
#endif // OS_POSIX
diff --git a/base/posix/file_descriptor_shuffle.cc b/base/posix/file_descriptor_shuffle.cc
index b5b7339..7bc9e26 100644
--- a/base/posix/file_descriptor_shuffle.cc
+++ b/base/posix/file_descriptor_shuffle.cc
@@ -89,7 +89,7 @@
}
void FileDescriptorTableInjection::Close(int fd) {
- int ret = HANDLE_EINTR(close(fd));
+ int ret = IGNORE_EINTR(close(fd));
DPCHECK(ret == 0);
}
diff --git a/base/posix/unix_domain_socket_linux_unittest.cc b/base/posix/unix_domain_socket_linux_unittest.cc
index 1343555..22bb172 100644
--- a/base/posix/unix_domain_socket_linux_unittest.cc
+++ b/base/posix/unix_domain_socket_linux_unittest.cc
@@ -45,7 +45,7 @@
ASSERT_EQ(1U, message_fds.size());
// Close the reply FD.
- ASSERT_EQ(0, HANDLE_EINTR(close(message_fds.front())));
+ ASSERT_EQ(0, IGNORE_EINTR(close(message_fds.front())));
// Check that the thread didn't get blocked.
WaitableEvent event(false, false);
@@ -63,7 +63,7 @@
int fds[2];
ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds));
file_util::ScopedFD scoped_fd1(&fds[1]);
- ASSERT_EQ(0, HANDLE_EINTR(close(fds[0])));
+ ASSERT_EQ(0, IGNORE_EINTR(close(fds[0])));
// Have the thread send a synchronous message via the socket. Unless the
// message is sent with MSG_NOSIGNAL, this shall result in SIGPIPE.
diff --git a/base/process/launch_posix.cc b/base/process/launch_posix.cc
index de6286d..8dc8f9e 100644
--- a/base/process/launch_posix.cc
+++ b/base/process/launch_posix.cc
@@ -230,7 +230,7 @@
// Since we're just trying to close anything we can find,
// ignore any error return values of close().
- ignore_result(HANDLE_EINTR(close(fd)));
+ close(fd);
}
return;
}
@@ -264,7 +264,7 @@
// these FDs are >= |max_fds|, so we can check against that here
// before closing. See https://bugs.kde.org/show_bug.cgi?id=191758
if (fd < static_cast<int>(max_fds)) {
- int ret = HANDLE_EINTR(close(fd));
+ int ret = IGNORE_EINTR(close(fd));
DPCHECK(ret == 0);
}
}
diff --git a/base/process/process_util_unittest.cc b/base/process/process_util_unittest.cc
index 2e53306..67d6618 100644
--- a/base/process/process_util_unittest.cc
+++ b/base/process/process_util_unittest.cc
@@ -442,7 +442,7 @@
int written = HANDLE_EINTR(write(write_pipe, &num_open_files,
sizeof(num_open_files)));
DCHECK_EQ(static_cast<size_t>(written), sizeof(num_open_files));
- int ret = HANDLE_EINTR(close(write_pipe));
+ int ret = IGNORE_EINTR(close(write_pipe));
DPCHECK(ret == 0);
return 0;
@@ -458,7 +458,7 @@
base::ProcessHandle handle = this->SpawnChild(
"ProcessUtilsLeakFDChildProcess", fd_mapping_vec, false);
CHECK(handle);
- int ret = HANDLE_EINTR(close(fds[1]));
+ int ret = IGNORE_EINTR(close(fds[1]));
DPCHECK(ret == 0);
// Read number of open files in client process from pipe;
@@ -474,7 +474,7 @@
CHECK(base::WaitForSingleProcess(handle, base::TimeDelta::FromSeconds(1)));
#endif
base::CloseProcessHandle(handle);
- ret = HANDLE_EINTR(close(fds[0]));
+ ret = IGNORE_EINTR(close(fds[0]));
DPCHECK(ret == 0);
return num_open_files;
@@ -502,11 +502,11 @@
ASSERT_EQ(fds_after, fds_before);
int ret;
- ret = HANDLE_EINTR(close(sockets[0]));
+ ret = IGNORE_EINTR(close(sockets[0]));
DPCHECK(ret == 0);
- ret = HANDLE_EINTR(close(sockets[1]));
+ ret = IGNORE_EINTR(close(sockets[1]));
DPCHECK(ret == 0);
- ret = HANDLE_EINTR(close(dev_null));
+ ret = IGNORE_EINTR(close(dev_null));
DPCHECK(ret == 0);
}
@@ -535,13 +535,13 @@
CHECK_EQ(0, clone_flags);
#endif // OS_LINUX
EXPECT_TRUE(base::LaunchProcess(args, options, NULL));
- PCHECK(HANDLE_EINTR(close(fds[1])) == 0);
+ PCHECK(IGNORE_EINTR(close(fds[1])) == 0);
char buf[512];
const ssize_t n = HANDLE_EINTR(read(fds[0], buf, sizeof(buf)));
PCHECK(n > 0);
- PCHECK(HANDLE_EINTR(close(fds[0])) == 0);
+ PCHECK(IGNORE_EINTR(close(fds[0])) == 0);
return std::string(buf, n);
}
diff --git a/base/test/multiprocess_test_android.cc b/base/test/multiprocess_test_android.cc
index 9367426..51c2ce0 100644
--- a/base/test/multiprocess_test_android.cc
+++ b/base/test/multiprocess_test_android.cc
@@ -8,7 +8,6 @@
#include "base/containers/hash_tables.h"
#include "base/logging.h"
-#include "base/posix/eintr_wrapper.h"
#include "testing/multiprocess_func_list.h"
namespace base {
@@ -42,7 +41,7 @@
const int kFdForAndroidLogging = 3; // FD used by __android_log_write().
for (int fd = kFdForAndroidLogging + 1; fd < getdtablesize(); ++fd) {
if (fds_to_keep_open.find(fd) == fds_to_keep_open.end()) {
- HANDLE_EINTR(close(fd));
+ close(fd);
}
}
for (FileHandleMappingVector::const_iterator it = fds_to_remap.begin();
@@ -52,7 +51,7 @@
if (dup2(old_fd, new_fd) < 0) {
PLOG(FATAL) << "dup2";
}
- HANDLE_EINTR(close(old_fd));
+ close(old_fd);
}
_exit(multi_process_function_list::InvokeChildProcessTest(procname));
return 0;