Add Locking calls to file_io.h plus implementations and test

util/file/file_io_test.cc

Index: util/file/file_io_test.cc
diff --git a/util/file/file_io_test.cc b/util/file/file_io_test.cc
new file mode 100644
index 0000000000000000000000000000000000000000..52a2b5d0a5ef5b4c98179bdc8f7d6673c54941ad
--- /dev/null
+++ b/util/file/file_io_test.cc
@@ -0,0 +1,178 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/file/file_io.h"
+
+#include "base/atomicops.h"
+#include "base/basictypes.h"
+#include "base/files/file_path.h"
+#include "gtest/gtest.h"
+#include "util/test/errors.h"
+#include "util/test/scoped_temp_dir.h"
+#include "util/test/thread.h"
+
+namespace crashpad {
+namespace test {
+namespace {
+
+enum class ReadOrWrite : bool {
+  Read,

[Mark Mentovai, 2015/03/20 22:11:37]

I’m digging these enum classes! kRead, kWrite to make constantness more obvious
at use sites?

[scottmg, 2015/03/20 22:32:53]

Done.

+  Write,
+};
+
+void FileShareModeTest(ReadOrWrite first, ReadOrWrite second) {

[Mark Mentovai, 2015/03/20 22:11:36]

These were good tests to add!

+  ScopedTempDir temp_dir;
+  base::FilePath shared_file =
+      temp_dir.path().Append(FILE_PATH_LITERAL("shared_file"));
+  {
+    // Create an empty file to work on.
+    ScopedFileHandle create(
+        LoggingOpenFileForWrite(shared_file,
+                                FileWriteMode::kCreateOrFail,
+                                FilePermissions::kOwnerOnly));
+  }
+
+  auto handle1 = ScopedFileHandle(
+      (first == ReadOrWrite::Read)
+          ? LoggingOpenFileForRead(shared_file)
+          : LoggingOpenFileForWrite(shared_file,
+                                    FileWriteMode::kReuseOrCreate,
+                                    FilePermissions::kOwnerOnly));
+  ASSERT_NE(handle1, kInvalidFileHandle);
+  auto handle2 = ScopedFileHandle(
+      (second == ReadOrWrite::Read)
+          ? LoggingOpenFileForRead(shared_file)
+          : LoggingOpenFileForWrite(shared_file,
+                                    FileWriteMode::kReuseOrCreate,
+                                    FilePermissions::kOwnerOnly));
+  EXPECT_NE(handle2, kInvalidFileHandle);

[Mark Mentovai, 2015/03/20 22:11:36]

EXPECT_NE(handle1, handle2) also, because we’re kind of depending on that for
some things. At least for the next set of tests.

[scottmg, 2015/03/20 22:32:53]

Done.

+}
+
+TEST(FileIO, FileShareModes) {

[Mark Mentovai, 2015/03/20 22:11:37]

Can you have a separate TEST for each of these? Like FileShareModes_Read_Read,
etc. That way, when one of them fails, it’ll be obvious which variant is a
problem.

[scottmg, 2015/03/20 22:32:53]

Done.

+  FileShareModeTest(ReadOrWrite::Read, ReadOrWrite::Read);
+  FileShareModeTest(ReadOrWrite::Read, ReadOrWrite::Write);
+  FileShareModeTest(ReadOrWrite::Write, ReadOrWrite::Read);
+  FileShareModeTest(ReadOrWrite::Write, ReadOrWrite::Write);
+}
+
+TEST(FileIO, MultipleSharedLocks) {
+  ScopedTempDir temp_dir;
+  base::FilePath shared_file =
+      temp_dir.path().Append(FILE_PATH_LITERAL("file_to_lock"));
+
+  {
+    // Create an empty file to lock.
+    ScopedFileHandle create(
+        LoggingOpenFileForWrite(shared_file,
+                                FileWriteMode::kCreateOrFail,
+                                FilePermissions::kOwnerOnly));
+  }
+
+  auto handle1 = ScopedFileHandle(LoggingOpenFileForRead(shared_file));
+  ASSERT_NE(handle1, kInvalidFileHandle);
+  EXPECT_TRUE(LoggingLockFile(handle1.get(), FileLocking::kShared));
+
+  auto handle2 = ScopedFileHandle(LoggingOpenFileForRead(shared_file));
+  ASSERT_NE(handle1, kInvalidFileHandle);
+  EXPECT_TRUE(LoggingLockFile(handle2.get(), FileLocking::kShared));
+
+  EXPECT_TRUE(LoggingUnlockFile(handle1.get()));
+  EXPECT_TRUE(LoggingUnlockFile(handle2.get()));
+}
+
+class LockingTestThread : public Thread {
+ public:
+  void Main() override {

[Mark Mentovai, 2015/03/20 22:11:37]

Doesn’t need to be public. private’s fine. There’s no good reason for any Tom,
Dick, or Harry with a LockingTestThread object to be able to call this, other
than to get it called by the base via Start().

[scottmg, 2015/03/20 22:32:53]

Done.

+    for (int i = 0; i < iterations; ++i) {
+      EXPECT_TRUE(LoggingLockFile(file.get(), lock_type));
+      base::subtle::NoBarrier_AtomicIncrement(actual_iterations, 1);
+      EXPECT_TRUE(LoggingUnlockFile(file.get()));
+    }
+  }
+
+  ScopedFileHandle file;

[Mark Mentovai, 2015/03/20 22:11:37]

private, trailing underscores, DISALLOW_COPY_AND_ASSIGN(), default constructor,
setters as needed. I get the struct heritage here, but now that this is a
grown-up object with a vtable and everything, having direct access from the
outside is a little unexpected.

[scottmg, 2015/03/20 22:32:53]

Done.

+  FileLocking lock_type;
+  int iterations;
+  base::subtle::Atomic32* actual_iterations;
+};
+
+void LockingTest(FileLocking main_lock, FileLocking other_locks) {
+  ScopedTempDir temp_dir;
+  base::FilePath shared_file =
+      temp_dir.path().Append(FILE_PATH_LITERAL("file_to_lock"));
+
+  {
+    // Create an empty file to lock.
+    ScopedFileHandle create(
+        LoggingOpenFileForWrite(shared_file,
+                                FileWriteMode::kCreateOrFail,
+                                FilePermissions::kOwnerOnly));
+  }
+
+  auto initial = ScopedFileHandle(
+      (main_lock == FileLocking::kShared)
+          ? LoggingOpenFileForRead(shared_file)
+          : LoggingOpenFileForWrite(shared_file,
+                                    FileWriteMode::kReuseOrCreate,
+                                    FilePermissions::kOwnerOnly));
+  ASSERT_NE(initial, kInvalidFileHandle);
+  ASSERT_TRUE(LoggingLockFile(initial.get(), main_lock));
+
+  base::subtle::Atomic32 actual_iterations = 0;
+
+  LockingTestThread threads[20];
+  int expected_iterations = 0;
+  for (size_t index = 0; index < arraysize(threads); ++index) {
+    threads[index].file = ScopedFileHandle(
+        (other_locks == FileLocking::kShared)
+            ? LoggingOpenFileForRead(shared_file)
+            : LoggingOpenFileForWrite(shared_file,
+                                      FileWriteMode::kReuseOrCreate,
+                                      FilePermissions::kOwnerOnly));
+    ASSERT_NE(threads[index].file, kInvalidFileHandle);
+    threads[index].lock_type = other_locks;
+    threads[index].iterations = static_cast<int>(index * 10);
+    threads[index].actual_iterations = &actual_iterations;
+    expected_iterations += threads[index].iterations;
+
+    ASSERT_NO_FATAL_FAILURE(threads[index].Start());
+  }
+
+  base::subtle::Atomic32 result =
+      base::subtle::Release_Load(&actual_iterations);
+  EXPECT_EQ(0, result);
+
+  ASSERT_TRUE(LoggingUnlockFile(initial.get()));
+
+  for (auto& t : threads)
+    t.Join();
+
+  EXPECT_EQ(expected_iterations, actual_iterations);
+}
+
+TEST(FileIO, ExclusiveVsExclusives) {
+  LockingTest(FileLocking::kExclusive, FileLocking::kExclusive);
+}
+
+TEST(FileIO, ExclusiveVsShareds) {
+  LockingTest(FileLocking::kExclusive, FileLocking::kShared);
+}
+
+TEST(FileIO, SharedVsExclusives) {
+  LockingTest(FileLocking::kShared, FileLocking::kExclusive);
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace crashpad