Define the Settings interface for a CrashReportDatabase and provide a Mac implementation.

client/settings_mac.mm

Index: client/settings_mac.mm
diff --git a/client/settings_mac.mm b/client/settings_mac.mm
new file mode 100644
index 0000000000000000000000000000000000000000..394e843c18fd5c55b455bf5c34018a734eb0bed4
--- /dev/null
+++ b/client/settings_mac.mm
@@ -0,0 +1,217 @@
+// 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.
+
+#import "client/settings_mac.h"
+
+#include <fcntl.h>
+#include <unistd.h>
+#include <uuid/uuid.h>
+
+#include "base/logging.h"
+#include "base/posix/eintr_wrapper.h"
+#include "base/strings/sys_string_conversions.h"
+#include "util/misc/uuid.h"
+
+namespace crashpad {
+namespace internal {
+
+namespace {
+
+NSString* const kClientID = @"ClientID";
+NSString* const kUploadsEnabled = @"UploadsEnabled";
+NSString* const kLastUploadAttemptTime = @"LastUploadAttemptTime";
+
+}  // namespace
+
+SettingsMac::SettingsMac(const base::FilePath& prefs_file)
+    : prefs_file_(prefs_file) {
+}
+
+SettingsMac::~SettingsMac() {
+}
+
+bool SettingsMac::Initialize() {
+  ScopedFileHandle fd(HANDLE_EINTR(
+      open(prefs_file(),
+           O_CREAT|O_EXCL|O_EXLOCK|O_WRONLY,

[Mark Mentovai, 2015/03/08 05:21:56]

Spaces around the |s, everywhere you use bitwise OR.

[Robert Sesek, 2015/03/08 22:15:36]

Done.

+           0644)));
+
+  // The file was created, so this is a new database that needs to be
+  // initialized with a client ID.
+  if (fd.is_valid()) {
+    uuid_t client_id;
+    uuid_generate(client_id);
+
+    std::string client_id_string(UUID(client_id).ToString());
+
+    NSDictionary* plist =
+        @{ kClientID : base::SysUTF8ToNSString(client_id_string) };
+
+    return WritePlist(fd.get(), plist);
+  }
+
+  // The file wasn't opened successfully, try opening it without creating.
+  fd.reset(HANDLE_EINTR(open(prefs_file(), O_RDONLY|O_SHLOCK)));
+  PLOG_IF(ERROR, !fd.is_valid()) << "open " << prefs_file();
+  return fd.is_valid();
+}
+
+bool SettingsMac::GetClientID(std::string* client_id) {

[Mark Mentovai, 2015/03/08 05:21:56]

These are going to be called from weird places that don’t have run loops. All of
the public entry points that use Objective-C should be wrapped in
@autoreleasepool.

[Robert Sesek, 2015/03/08 22:15:36]

Done. I didn't know the context of when these would be called, so I put it on
the caller via the class comment. Removed and just provide a pool in each public
method.

+  base::scoped_nsobject<id> value = GetSetting(kClientID);
+  if (!value)

[Mark Mentovai, 2015/03/08 05:21:56]

If the plist couldn’t be read at any point, we should reinitialize it with a
fresh client ID. We shouldn’t let a corrupt file ruin the database forever. Log
a message and recover.

If the plist could be read but is missing the client ID key or the value for it
is not of the expected type, we should set a new client ID and get on with life.
I also think that if what we read back isn’t formatted validly for a UUID, we
should trash it and set a new one.

Recovery operations should relock the file with a write lock, retry the read to
make sure nobody else already fixed the file, and then do the recovery.

[Robert Sesek, 2015/03/08 22:15:36]

Done.
Done.
Do you feel strongly? I don't think this is necessary to impose, and it would
also prohibit migrating the old client IDs if we wanted to do that.
Done.

[Mark Mentovai, 2015/03/09 14:01:23]

Robert Sesek wrote:
I discussed this with Anthony last week. We don’t need to do a migration.

+    return false;
+
+  if (![value isKindOfClass:[NSString class]]) {

[Mark Mentovai, 2015/03/08 05:21:56]

Why not ObjCCast?

[Robert Sesek, 2015/03/08 22:15:36]

I didn't see any advantage to using that because I still want an error logged
and the type is |id| so I'm not going to benefit from the result of the cast.

+    LOG(ERROR) << [kClientID UTF8String] << " is not a NSString";
+    return false;
+  }
+
+  *client_id = base::SysNSStringToUTF8(value);
+  return true;
+}
+
+bool SettingsMac::GetUploadsEnabled(bool* enabled) {
+  base::scoped_nsobject<id> value = GetSetting(kUploadsEnabled);
+  if (!value)
+    return false;
+
+  if (![value isKindOfClass:[NSValue class]]) {
+    LOG(ERROR) << [kUploadsEnabled UTF8String] << " is not a NSValue";
+    return false;
+  }
+
+  *enabled = [value boolValue];
+  return true;

[Mark Mentovai, 2015/03/08 05:21:56]

With this implementation, a fresh database will return false for
GetUploadsEnabled() until someone calls SetUploadsEnabled(). I think it’d be
more useful to just provide a default false value when the database is created
(or if the key is of the wrong type, etc). The caller can’t know whether a new
settings file was created and thus that it needs to push in its preferred
default value, because this class is the only thing that knows that a new
settings file was created. That leaves callers in the awkward spot of treating a
failure return of this method as “just use your preferred default” but there’s
no way to disambiguate it from a genuine error, and it makes the caller’s code
more complex.

[Robert Sesek, 2015/03/08 22:15:36]

Done.

+}
+
+bool SettingsMac::SetUploadsEnabled(bool enabled) {
+  return SetSetting(kUploadsEnabled, [NSNumber numberWithBool:enabled]);
+}
+
+bool SettingsMac::GetLastUploadAttemptTime(time_t* time) {
+  base::scoped_nsobject<id> value = GetSetting(kLastUploadAttemptTime);
+  if (!value)
+    return false;
+
+  if (![value isKindOfClass:[NSValue class]]) {
+    LOG(ERROR) << [kLastUploadAttemptTime UTF8String] << " is not a NSValue";
+    return false;
+  }
+
+  *time = [value doubleValue];
+  return true;
+}
+
+bool SettingsMac::SetLastUploadAttemptTime(time_t time) {
+  return SetSetting(kLastUploadAttemptTime, [NSNumber numberWithDouble:time]);
+}
+
+base::scoped_nsobject<NSMutableDictionary> SettingsMac::ReadPlist(
+    FileHandle fd) {
+  base::scoped_nsobject<NSMutableDictionary> plist;
+
+  base::scoped_nsobject<NSFileHandle> file_handle(
+      [[NSFileHandle alloc] initWithFileDescriptor:fd closeOnDealloc:NO]);
+  NSData* data = nil;
+  @try {
+    data = [file_handle readDataToEndOfFile];
+  } @catch (NSException* exception) {
+    LOG(ERROR) << "Failed to read settings: " << [exception description];
+    return plist;
+  }
+  if (!data)
+    return plist;
+
+  NSError* error = nil;
+  NSPropertyListMutabilityOptions options = NSPropertyListMutableContainers;
+  plist.reset(
+      [[NSPropertyListSerialization propertyListWithData:data
+                                                 options:options
+                                                  format:nullptr
+                                                   error:&error] retain]);
+  LOG_IF(ERROR, error != nil) << "Failed to deserialize settings: "
+                              << [error description];
+  return plist;
+}
+
+bool SettingsMac::WritePlist(FileHandle fd, NSDictionary* plist) {
+  NSError* error = nil;
+  NSPropertyListFormat format = NSPropertyListXMLFormat_v1_0;

[Mark Mentovai, 2015/03/08 05:21:56]

Not the faster binary format?

[Robert Sesek, 2015/03/08 22:15:36]

Done. The Foundation guides don't actually provide guidance or a "do the
sensible thing" value.

+  NSData* data = [NSPropertyListSerialization dataWithPropertyList:plist
+                                                            format:format
+                                                           options:0
+                                                             error:&error];
+  LOG_IF(ERROR, error != nil) << "Failed to serialize settings: "
+                              << [error description];
+  if (!data)
+    return false;
+
+  if (HANDLE_EINTR(ftruncate(fd, 0)) != 0) {
+    PLOG(ERROR) << "ftruncate failed on settings file";
+    return false;
+  }
+
+  if (LoggingSeekFile(fd, 0, SEEK_SET) != 0) {
+    return false;
+  }
+
+  base::scoped_nsobject<NSFileHandle> file_handle(
+      [[NSFileHandle alloc] initWithFileDescriptor:fd closeOnDealloc:NO]);
+  @try {
+    [file_handle writeData:data];
+  } @catch (NSException* exception) {
+    LOG(ERROR) << "Failed to write settings: " << [exception description];
+    return false;
+  }
+
+  return true;
+}
+
+base::scoped_nsobject<id> SettingsMac::GetSetting(NSString* key) {
+  base::scoped_nsobject<id> value;

[Mark Mentovai, 2015/03/08 05:21:56]

You don’t need to declare this ’til much later, if even at all. You could just
make the error case returns use base::scoped_nsobject<id>(), and the successful
return use base::scoped_nsobject<id>([[plist objectForKey:key] retain]).

[Robert Sesek, 2015/03/08 22:15:36]

I dislike having to read and write such a verbose early return value when what I
really want to say is `nil` but the scoper can't convert that like it can
nullptr_t.

Now that all callers have a NSAutoreleasePool in place, I'm just going to let
these be returned autoreleased.

+
+  ScopedFileHandle fd(HANDLE_EINTR(open(prefs_file(), O_RDONLY|O_SHLOCK)));
+  if (!fd.is_valid()) {
+    PLOG(ERROR) << "open for get setting";
+    return value;
+  }
+
+  base::scoped_nsobject<NSMutableDictionary> plist(ReadPlist(fd.get()));
+  if (!plist.get())
+    return value;
+
+  value.reset([[plist objectForKey:key] retain]);
+  return value;
+}
+
+bool SettingsMac::SetSetting(NSString* key, NSObject* value) {
+  base::scoped_nsobject<NSMutableDictionary> plist;
+
+  ScopedFileHandle fd(HANDLE_EINTR(open(prefs_file(), O_RDWR|O_EXLOCK)));
+  if (!fd.is_valid()) {
+    LOG(ERROR) << "open for set setting";

[Mark Mentovai, 2015/03/08 05:21:56]

PLOG

[Robert Sesek, 2015/03/08 22:15:36]

Done.

+    return plist;
+  }
+
+  plist = ReadPlist(fd.get());
+  if (!plist.get())
+    return false;
+
+  [plist setObject:value forKey:key];
+
+  return WritePlist(fd.get(), plist);
+}
+
+}  // namespace internal
+}  // namespace crashpad