Use LockFile/UnlockFile for Settings to port to Windows

client/settings.cc

Index: client/settings.cc
diff --git a/client/settings.cc b/client/settings.cc
index aa2c4f8f21ac9d52ab5c78de9fb6467a9e97d174..48983a6fcd8f282329304d1ebac06f8deb332a77 100644
--- a/client/settings.cc
+++ b/client/settings.cc
@@ -16,10 +16,6 @@
 
 #include <limits>
 
-#include <fcntl.h>
-#include <unistd.h>
-#include <uuid/uuid.h>
-
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/posix/eintr_wrapper.h"
@@ -61,10 +57,11 @@ Settings::~Settings() {
 bool Settings::Initialize() {
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
 
-  ScopedFileHandle handle(HANDLE_EINTR(

[scottmg, 2015/04/01 00:49:14]

I think the/a case that has changed here:

- X enters, attempts open, succeeds because the file didn't exist.
- Y enters, attempts open, fails because the file does exist.
- Y continues down to the "normal" case, opens for RW.
- Now they're racing for the lock.
- If X locks, all is well as it will initialize, release, and Y continues
- if Y locks, then ReadSettings will fail, causing RecoverSettings to
InitializeSettings (which is what we want), except after unlock, it'll happen
again on X which is a bit weird.

Which makes me think that simply deleting the "initialize" path would be
equivalent.

But, I'm not sure if I understand the subtlety of atomicity of O_EXLOCK or the
various conditions, so please correct me if I'm way off here.

[Robert Sesek, 2015/04/01 15:08:26]

I think RecoverSettings won't call InitializeSettings again, since it takes the
O_EXLOCK and then tries to read them again, to prevent the double-recovery case.
Yes, it could be. Non-existent file is technically just another error/recovery
condition, so I'd be OK with removing the entire Initialize step I think. The
only issue is that then when creating the database for the first time, there
will be some inaccurate (or misleading) log messages saying that we're trying to
"recover" the settings file.
Yes, this is the race condition I mentioned on the bug.

-      open(file_path().value().c_str(),
-           O_CREAT | O_EXCL | O_WRONLY | O_EXLOCK,
-           0644)));
+  ScopedLockedFileHandle handle(
+      LoggingOpenFileForWrite(file_path(),
+                              FileWriteMode::kCreateOrFail,
+                              FilePermissions::kWorldReadable),
+      FileLocking::kExclusive);
 
   // The file was created, so this is a new database that needs to be
   // initialized with a client ID.
@@ -112,7 +109,7 @@ bool Settings::SetUploadsEnabled(bool enabled) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
   Data settings;
-  ScopedFileHandle handle = OpenForWritingAndReadSettings(&settings);
+  ScopedLockedFileHandle handle = OpenForWritingAndReadSettings(&settings);
   if (!handle.is_valid())
     return false;
 
@@ -140,7 +137,7 @@ bool Settings::SetLastUploadAttemptTime(time_t time) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
   Data settings;
-  ScopedFileHandle handle = OpenForWritingAndReadSettings(&settings);
+  ScopedLockedFileHandle handle = OpenForWritingAndReadSettings(&settings);
   if (!handle.is_valid())
     return false;
 
@@ -149,22 +146,49 @@ bool Settings::SetLastUploadAttemptTime(time_t time) {
   return WriteSettings(handle.get(), settings);
 }
 
-ScopedFileHandle Settings::OpenForReading() {
-  ScopedFileHandle handle(HANDLE_EINTR(
-      open(file_path().value().c_str(), O_RDONLY | O_SHLOCK)));
-  PLOG_IF(ERROR, !handle.is_valid()) << "open for reading";
+Settings::ScopedLockedFileHandle::ScopedLockedFileHandle() : handle_() {
+}
+
+Settings::ScopedLockedFileHandle::ScopedLockedFileHandle(FileHandle file,
+                                                         FileLocking locking)
+    : handle_(file) {
+  if (handle_.is_valid()) {
+    if (!LoggingLockFile(handle_.get(), locking))
+      handle_.reset();
+  }
+}
+
+Settings::ScopedLockedFileHandle::~ScopedLockedFileHandle() {
+  FreeIfNecessary();
+}
+
+Settings::ScopedLockedFileHandle Settings::OpenForReading() {
+  ScopedLockedFileHandle handle(LoggingOpenFileForRead(file_path()),
+                                FileLocking::kShared);
   return handle.Pass();
 }
 
-ScopedFileHandle Settings::OpenForReadingAndWriting() {
-  ScopedFileHandle handle(HANDLE_EINTR(
-      open(file_path().value().c_str(), O_RDWR | O_EXLOCK | O_CREAT, 0644)));
-  PLOG_IF(ERROR, !handle.is_valid()) << "open for writing";
+Settings::ScopedLockedFileHandle Settings::OpenForReadingAndWriting() {
+  ScopedLockedFileHandle handle(
+      LoggingOpenFileForReadAndWrite(file_path(),
+                                     FileWriteMode::kReuseOrCreate,
+                                     FilePermissions::kWorldReadable),
+      FileLocking::kExclusive);
   return handle.Pass();
 }
 
+void Settings::ScopedLockedFileHandle::reset() {
+  FreeIfNecessary();
+  handle_.reset();
+}
+
+void Settings::ScopedLockedFileHandle::FreeIfNecessary() {
+  if (is_valid())
+    LoggingUnlockFile(handle_.get());
+}
+
 bool Settings::OpenAndReadSettings(Data* out_data) {
-  ScopedFileHandle handle = OpenForReading();
+  ScopedLockedFileHandle handle = OpenForReading();
   if (!handle.is_valid())
     return false;
 
@@ -178,14 +202,15 @@ bool Settings::OpenAndReadSettings(Data* out_data) {
   return RecoverSettings(kInvalidFileHandle, out_data);
 }
 
-ScopedFileHandle Settings::OpenForWritingAndReadSettings(Data* out_data) {
-  ScopedFileHandle handle = OpenForReadingAndWriting();
+Settings::ScopedLockedFileHandle Settings::OpenForWritingAndReadSettings(
+    Data* out_data) {
+  ScopedLockedFileHandle handle = OpenForReadingAndWriting();
   if (!handle.is_valid())
-    return ScopedFileHandle();
+    return ScopedLockedFileHandle();
 
   if (!ReadSettings(handle.get(), out_data)) {
     if (!RecoverSettings(handle.get(), out_data))
-      return ScopedFileHandle();
+      return ScopedLockedFileHandle();
   }
 
   return handle.Pass();
@@ -215,16 +240,14 @@ bool Settings::WriteSettings(FileHandle handle, const Data& data) {
   if (LoggingSeekFile(handle, 0, SEEK_SET) != 0)
     return false;
 
-  if (HANDLE_EINTR(ftruncate(handle, 0)) != 0) {
-    PLOG(ERROR) << "ftruncate settings file";
+  if (!LoggingTruncateFile(handle))
     return false;
-  }
 
   return LoggingWriteFile(handle, &data, sizeof(Data));
 }
 
 bool Settings::RecoverSettings(FileHandle handle, Data* out_data) {
-  ScopedFileHandle scoped_handle;
+  ScopedLockedFileHandle scoped_handle;
   if (handle == kInvalidFileHandle) {
     scoped_handle = OpenForReadingAndWriting();
     handle = scoped_handle.get();
@@ -235,7 +258,7 @@ bool Settings::RecoverSettings(FileHandle handle, Data* out_data) {
       return true;
   }
 
-  LOG(INFO) << "Recovering settings file " << file_path().value();
+  LOG(INFO) << "Recovering settings file " << file_path().value().c_str();
 
   if (handle == kInvalidFileHandle) {
     LOG(ERROR) << "Invalid file handle";
@@ -249,11 +272,9 @@ bool Settings::RecoverSettings(FileHandle handle, Data* out_data) {
 }
 
 bool Settings::InitializeSettings(FileHandle handle) {
-  uuid_t uuid;
-  uuid_generate(uuid);
-
   Data settings;
-  settings.client_id.InitializeFromBytes(uuid);
+  if (!UUID::GenerateFromSystem(&settings.client_id))
+    return false;
 
   return WriteSettings(handle, settings);
 }