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/files/file_path.h"
+#include "gtest/gtest.h"
+#include "util/test/scoped_temp_dir.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::kReuseOrCreate, FilePermissions::kOwnerOnly));

[Mark Mentovai, 2015/03/12 04:47:42]

The first one should use kCreateOrFail. Same on lines 43 and 111.

[scottmg, 2015/03/19 22:06:20]

Done.

+  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"));
+
+  auto handle1 = ScopedFileHandle(LoggingOpenFileForWrite(
+      shared_file, FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly));

[Mark Mentovai, 2015/03/12 04:47:42]

ASSERT_NE(handle1, kInvalidFileHandle) after this, like in FileIO.FileShareMode?
Same for handle2. Also in FileIO.ExclusiveExcludes.

[scottmg, 2015/03/19 22:06:20]

Done.

+  EXPECT_TRUE(LoggingLockFile(handle1.get(), FileLocking::kShared));
+
+  auto handle2 = ScopedFileHandle(LoggingOpenFileForWrite(
+      shared_file, FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly));
+  EXPECT_TRUE(LoggingLockFile(handle2.get(), FileLocking::kShared));
+
+  EXPECT_TRUE(LoggingUnlockFile(handle1.get()));
+  EXPECT_TRUE(LoggingUnlockFile(handle2.get()));
+}
+
+struct ThreadMainInfo {
+#if defined(OS_POSIX)

[Mark Mentovai, 2015/03/12 04:47:42]

OK for now. Robert and I were talking about needing a threading primitive today.

[Robert Sesek, 2015/03/12 15:28:03]

I think it's probably time to create this abstraction (at least for use in test
code, I think this is the third instance of something like this).

[scottmg, 2015/03/19 22:06:20]

Added some version of this to util/test/thread.h. I didn't convert other usages
pending discussion on interface and/or deferral to future CLs.

+  pthread_t pthread;
+#elif defined(OS_WIN)
+  HANDLE thread;
+#endif
+  FileHandle file;

[Mark Mentovai, 2015/03/12 04:47:42]

ScopedFileHandle?

[scottmg, 2015/03/19 22:06:20]

Done.

+  int iterations;
+  int* shared_counter;
+};
+
+#if defined(OS_POSIX)
+void*
+#elif defined(OS_WIN)
+DWORD WINAPI
+#endif  // OS_POSIX
+ThreadMain(void* argument) {
+  ThreadMainInfo* info = reinterpret_cast<ThreadMainInfo*>(argument);
+  for (int i = 0; i < info->iterations; ++i) {
+    EXPECT_TRUE(LoggingLockFile(info->file, FileLocking::kExclusive));

[Mark Mentovai, 2015/03/12 04:47:42]

You haven’t done anything to test that an exclusive lock also blocks an attempt
to get a shared lock, or that one or more shared locks block an attempt to get
an exclusive lock. Maybe you could cheaply parameterize this test so that for
(main_thread_lock_type, other_threads_lock_type) you try all of (exclusive,
exclusive) (what you’ve done), (exclusive, shared), and (shared, exclusive).

[scottmg, 2015/03/19 22:06:20]

Done.

+    *info->shared_counter += 1;

[Mark Mentovai, 2015/03/12 04:47:42]

For the case where the the other threads will be using shared locks, this would
need to become an atomic increment. Bringing base/atomicops.h into mini_chromium
should be fine, that’s a reasonable interface with strong implementations.

[scottmg, 2015/03/19 22:06:20]

Done.

+    EXPECT_TRUE(LoggingUnlockFile(info->file));
+  }
+
+#if defined(OS_POSIX)
+  return nullptr;
+#elif defined(OS_WIN)
+  return 0;
+#endif  // OS_POSIX
+}
+
+void StartThread(ThreadMainInfo* info) {
+#if defined(OS_POSIX)
+  int rv = pthread_create(&info->pthread, nullptr, ThreadMain, info);
+  ASSERT_EQ(0, rv) << "pthread_create";

[Mark Mentovai, 2015/03/12 04:47:42]

From util/test/errors.h, use ErrnoMessage, like

  ASSERT_EQ(0, rv) << ErrnoMessage(rv, "pthread_create");

Same on line 98. Maybe you’d want a similar ErrorMessage() for Windows too.

[scottmg, 2015/03/19 22:06:19]

Done.

+#elif defined(OS_WIN)
+  info->thread = CreateThread(nullptr, 0, ThreadMain, info, 0, nullptr);
+  ASSERT_NE(nullptr, info->thread) << "CreateThread";
+#endif  // OS_POSIX
+}
+
+void JoinThread(ThreadMainInfo* info) {
+#if defined(OS_POSIX)
+  int rv = pthread_join(info->pthread, nullptr);
+  EXPECT_EQ(0, rv) << "pthread_join";
+#elif defined(OS_WIN)
+  DWORD result = WaitForSingleObject(info->thread, INFINITE);
+  EXPECT_EQ(WAIT_OBJECT_0, result) << "WaitForSingleObject";
+#endif  // OS_POSIX
+}
+
+TEST(FileIO, ExclusiveExcludes) {
+  ScopedTempDir temp_dir;
+  base::FilePath shared_file =
+      temp_dir.path().Append(FILE_PATH_LITERAL("file_to_lock"));
+
+  auto initial = ScopedFileHandle(LoggingOpenFileForWrite(
+      shared_file, FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly));
+  ASSERT_TRUE(LoggingLockFile(initial.get(), FileLocking::kExclusive));
+
+  int shared_counter = 0;
+
+  const int kThreads = 10;
+  ThreadMainInfo info[kThreads];
+  int iterations = 0;

[Mark Mentovai, 2015/03/12 04:47:42]

expected_iterations makes it clearer. If shared_counter becomes
actual_iterations, it’s better still.

[scottmg, 2015/03/19 22:06:20]

Done.

+  for (int index = 0; index < kThreads; ++index) {

[Mark Mentovai, 2015/03/12 04:47:42]

#include "base/basictypes.h" and use arraysize(info) here and in the other
loops. Then you can just declare it as info[10] and get rid of kThreads. You
could even use a range-based for here, except you want index for other purposes.
But you still could use range-based for in the other loops.

[scottmg, 2015/03/19 22:06:19]

Done.

+    info[index].file = LoggingOpenFileForWrite(shared_file,
+                                               FileWriteMode::kReuseOrCreate,
+                                               FilePermissions::kOwnerOnly);
+    ASSERT_NE(info[index].file, kInvalidFileHandle);
+    info[index].iterations = index * 10;
+    info[index].shared_counter = &shared_counter;
+    iterations += info[index].iterations;
+
+    ASSERT_NO_FATAL_FAILURE(StartThread(&info[index]));
+  }
+
+  EXPECT_EQ(0, shared_counter);
+
+  ASSERT_TRUE(LoggingUnlockFile(initial.get()));
+
+  for (int index = 0; index < kThreads; ++index) {
+    JoinThread(&info[index]);
+  }
+
+  EXPECT_EQ(shared_counter, iterations);

[Mark Mentovai, 2015/03/12 04:47:42]

These arguments are swapped.

[scottmg, 2015/03/19 22:06:20]

Done.

+
+  for (int index = 0; index < kThreads; ++index) {
+    EXPECT_TRUE(LoggingCloseFile(info[index].file));

[Mark Mentovai, 2015/03/12 04:47:42]

This could have happened in ThreadMain() instead of having a third loop here.

[scottmg, 2015/03/19 22:06:20]

Removed via ScopedFileHandle.

+  }
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace crashpad