Add Locking calls to file_io.h plus implementations and test

util/file/file_io_test.cc

+// 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 {
+
+TEST(FileIO, FileShareMode) {
+  ScopedTempDir temp_dir;
+  base::FilePath shared_file =
+      temp_dir.path().Append(FILE_PATH_LITERAL("shared_file"));
+  auto handle1 = ScopedFileHandle(LoggingOpenFileForWrite(
+      shared_file, FileWriteMode::kCreateOrFail, FilePermissions::kOwnerOnly));
+  ASSERT_NE(handle1, kInvalidFileHandle);
+  auto handle2 = ScopedFileHandle(LoggingOpenFileForWrite(
+      shared_file, FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly));
+  EXPECT_NE(handle2, kInvalidFileHandle);
+}
+
+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()));
+}
+
+struct ThreadData {
+  ScopedFileHandle file;
+  FileLocking lock_type;
+  int iterations;
+  base::subtle::Atomic32* actual_iterations;
+};
+
+void ThreadMain(ThreadData* info) {
+  for (int i = 0; i < info->iterations; ++i) {
+    EXPECT_TRUE(LoggingLockFile(info->file.get(), info->lock_type));
+    base::subtle::Barrier_AtomicIncrement(info->actual_iterations, 1);

[Mark Mentovai, 2015/03/20 15:03:55]

I don’t think you need a memory barrier here.

[scottmg, 2015/03/20 21:07:34]

Done.

+    EXPECT_TRUE(LoggingUnlockFile(info->file.get()));
+  }
+}
+
+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;

[Mark Mentovai, 2015/03/20 15:03:55]

I’m not sure if the best practice for initialization is to use direct assignment
like this or to declare it and then do an Acquire_Store() immediately
afterwards.

[Robert Sesek, 2015/03/20 16:31:35]

Most instances in Chromium seem to initialize without an Acquire.

[scottmg, 2015/03/20 21:07:34]

Left as direct assignment.

+
+  ThreadData info[20];
+  Thread<ThreadData> threads[arraysize(info)];
+  base::subtle::Atomic32 expected_iterations = 0;

[Mark Mentovai, 2015/03/20 15:03:56]

This is not actually used for atomicity, it’s just typed that way for
compatibility with actual_iterations. Can you add a comment to that effect,
because otherwise it’s a red flag to see it being manipulated with direct
operations like += instead of atomic operations.

[scottmg, 2015/03/20 21:07:34]

Right. I switched it back to int to make usage less unclear as it was
unnecessary anyway.

+  for (size_t index = 0; index < arraysize(info); ++index) {
+    info[index].file = ScopedFileHandle(
+        (other_locks == FileLocking::kShared)
+            ? LoggingOpenFileForRead(shared_file)
+            : LoggingOpenFileForWrite(shared_file,
+                                      FileWriteMode::kReuseOrCreate,
+                                      FilePermissions::kOwnerOnly));
+    ASSERT_NE(info[index].file, kInvalidFileHandle);
+    info[index].lock_type = other_locks;
+    info[index].iterations = index * 10;
+    info[index].actual_iterations = &actual_iterations;
+    expected_iterations += info[index].iterations;
+
+    ASSERT_NO_FATAL_FAILURE(threads[index].Start(&ThreadMain, &info[index]));
+  }
+
+  EXPECT_EQ(0, actual_iterations);

[Mark Mentovai, 2015/03/20 15:03:55]

This one should have a barrier, so technically it should be Atomic32 result =
Release_Load(&actual_iterations);, and then EXPECT on release.

[scottmg, 2015/03/20 21:07:34]

Done.

+
+  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