Unrevert r245135 "Created new Windows LevelDB environment."

third_party/leveldatabase/env_chromium_win32.cc

+// Copyright (c) 2011 The LevelDB Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file. See the AUTHORS file for names of contributors.
+
+#include "base/debug/trace_event.h"
+#include "base/files/file_path.h"
+#include "base/posix/eintr_wrapper.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chromium_logger.h"
+#include "env_chromium_posix.h"
+#include "env_chromium_win32.h"
+
+#if defined(OS_WIN)
+#include <io.h>
+#include "base/win/win_util.h"
+#endif
+
+#if defined(OS_POSIX)
+#include <dirent.h>
+#include <fcntl.h>
+#include <sys/resource.h>
+#endif
+
+using namespace leveldb;
+
+namespace leveldb_env {
+
+namespace {
+
+static std::string GetWin32ErrorMessage(DWORD err)
+{
+    LPTSTR errorText(NULL);
+    FormatMessage(
+      FORMAT_MESSAGE_FROM_SYSTEM
+      |FORMAT_MESSAGE_ALLOCATE_BUFFER

[jsbell, 2013/12/12 01:11:42]

nit: spaces around the | operator, and wrap the line after the operator. Try
using "git cl format" to apply clang-format to the patch.

FYI: http://www.chromium.org/developers/coding-style

[cmumford, 2013/12/12 17:40:51]

thx. didn't know abot git cl format. Next patch will be "git cl" formatted.

+      |FORMAT_MESSAGE_IGNORE_INSERTS,
+      NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+      (LPTSTR)&errorText, 0, NULL);
+    if (errorText != NULL) {
+      if (errorText[0] != L'\0') {

[jsbell, 2013/12/12 19:03:14]

Is there a reason to expect trailing whitespace here? (i.e. does a newline
typically get appended or something?) If so, can you add a comment?

(I'd feel more comfortable with the "trim" operation occurring on the
std::string after the conversion myself - maybe implement a separate static
Trim(std::string*) helper?)

[cmumford, 2013/12/12 20:08:39]

Yes, FormatMessage adds CR/LF :-(, but you are correct - I should have used
TrimWhitespace. Adding that as well as a comment.

+        size_t idx = wcslen(errorText) - 1;
+        while (idx >= 0 && iswspace(errorText[idx])) {
+          errorText[idx] = L'\0';
+          --idx;
+        }
+      }
+      std::string message(UTF16ToUTF8(errorText));
+      LocalFree(errorText);
+      return message;
+    }
+    else {
+      return std::string();
+    }
+}
+
+class ChromiumSequentialFile: public SequentialFile {

[jsbell, 2013/12/12 19:03:14]

I'm slightly nervous that the re-use of this name (ChromiumSequentialFile)
between the win32 and other env will lead to confusion. They're scoped by the
anonymous namespace in their respective compilation units so it's not a
technical issue, but may confuse someone grepping/debugging the code.

I don't feel strongly about it though.

[cmumford, 2013/12/12 20:08:39]

Good point - I'll change it. Will do the same for ChromiumRandomAccessFile.

+ private:
+  std::string filename_;
+  HANDLE file_;
+  const UMALogger* uma_logger_;
+
+ public:
+  ChromiumSequentialFile(const std::string& fname, HANDLE f,
+                         const UMALogger* uma_logger)
+      : filename_(fname), file_(f), uma_logger_(uma_logger) {
+    DCHECK(file_ != INVALID_HANDLE_VALUE);
+  }
+  virtual ~ChromiumSequentialFile() {
+    DCHECK(file_ != INVALID_HANDLE_VALUE);
+    CloseHandle(file_);
+    file_ = INVALID_HANDLE_VALUE;
+  }
+
+  virtual Status Read(size_t n, Slice* result, char* scratch) {
+    Status s;
+    DWORD bytes_read(0);
+    DCHECK(file_ != INVALID_HANDLE_VALUE);
+    if (ReadFile(file_, scratch, n, &bytes_read, NULL)) {
+      *result = Slice(scratch, bytes_read);
+    }
+    else {
+      DWORD err = GetLastError();
+      s = MakeIOErrorWin32(
+        filename_, GetWin32ErrorMessage(err), kSequentialFileRead, err);
+      uma_logger_->RecordErrorAt(kSequentialFileRead);
+      if (bytes_read > 0)
+        *result = Slice(scratch, bytes_read);
+    }
+    return s;
+  }
+
+  virtual Status Skip(uint64_t n) {
+    LARGE_INTEGER li;
+    li.QuadPart = n;
+    if (SetFilePointer(file_, li.LowPart, &li.HighPart, FILE_CURRENT) ==
+        INVALID_SET_FILE_POINTER) {
+      DWORD err = GetLastError();
+      uma_logger_->RecordErrorAt(kSequentialFileSkip);
+      return MakeIOErrorWin32(
+        filename_, GetWin32ErrorMessage(err), kSequentialFileSkip, err);
+    }
+    return Status::OK();
+  }
+};
+
+class ChromiumRandomAccessFile: public RandomAccessFile {
+ private:
+  std::string filename_;
+  ::base::PlatformFile file_;

[jsbell, 2013/12/12 01:11:42]

Isn't the point of this patch to use Windows APIs directly, not the
base::PlatformFile abstraction?

[cmumford, 2013/12/12 17:40:51]

I can do that if you prefer. This is the first sentence from the bug "This could
involve rewriting all of ChromiumEnv to use ::base or just surrounding some
windows CreateFile-style code in #ifdefs." So I think this meets that criteria,
but if you want the Win32 env to be direct to the Win32 API without using base::
let me know.

[jsbell, 2013/12/12 19:03:14]

I think we should go farther; one of our hypotheses for the high corruption rate
on Windows is that the base:: platform file abstractions are hiding something
that ends up being a bug due to implicit assumptions about the API contract. So
dropping down to raw Win32, and potentially comparing against other Win32-envs
for leveldb, is a goal of this effort. 

That wasn't captured in the bug and/or we've hijacked the meaning of the bug. :(

+  const UMALogger* uma_logger_;
+
+ public:
+  ChromiumRandomAccessFile(const std::string& fname, ::base::PlatformFile file,
+                           const UMALogger* uma_logger)
+      : filename_(fname), file_(file), uma_logger_(uma_logger) { }
+  virtual ~ChromiumRandomAccessFile() { ::base::ClosePlatformFile(file_); }
+
+  virtual Status Read(uint64_t offset, size_t n, Slice* result,
+                      char* scratch) const {
+    Status s;
+    int r = ::base::ReadPlatformFile(file_, offset, scratch, n);
+    *result = Slice(scratch, (r < 0) ? 0 : r);
+    if (r < 0) {
+      // An error: return a non-ok status
+      s = MakeIOError(
+          filename_, "Could not perform read", kRandomAccessFileRead);
+      uma_logger_->RecordErrorAt(kRandomAccessFileRead);
+    }
+    return s;
+  }
+};
+
+}  // unnamed namespace
+
+Status MakeIOErrorWin32(Slice filename,
+                   const std::string& message,

[jsbell, 2013/12/12 01:11:42]

indentation

+                   MethodID method,
+                   DWORD error) {
+  char buf[512];
+  if (snprintf(buf,
+           sizeof(buf),

[jsbell, 2013/12/12 01:11:42]

indentation

+           "%s (ChromeMethodErrno: %d::%s::%u)",
+           message.c_str(),
+           method,
+           MethodIDToString(method),
+           error) >= 0) {
+    return Status::IOError(filename, buf);
+  }
+  else {

[jsbell, 2013/12/12 01:11:42]

else should be on the same line as the closing brace, i.e. } else {

+    return Status::IOError(filename, "<unknown>");
+  }
+}
+
+ChromiumWritableFileWin32::ChromiumWritableFileWin32(const std::string& fname,
+                                           HANDLE f,

[jsbell, 2013/12/12 01:11:42]

indentation (etc)

+                                           const UMALogger* uma_logger,
+                                           WriteTracker* tracker,
+                                           bool make_backup)
+    : filename_(fname),
+      file_(f),
+      uma_logger_(uma_logger),
+      tracker_(tracker),
+      file_type_(kOther),
+      make_backup_(make_backup) {
+  DCHECK(f != INVALID_HANDLE_VALUE);
+  base::FilePath path = base::FilePath::FromUTF8Unsafe(fname);
+  if (FilePathToString(path.BaseName()).find("MANIFEST") == 0)
+    file_type_ = kManifest;
+  else if (ChromiumEnv::HasTableExtension(path))
+    file_type_ = kTable;
+  if (file_type_ != kManifest)
+    tracker_->DidCreateNewFile(filename_);
+  parent_dir_ = FilePathToString(ChromiumEnv::CreateFilePath(fname).DirName());
+}
+
+ChromiumWritableFileWin32::~ChromiumWritableFileWin32() {
+  if (file_ != INVALID_HANDLE_VALUE) {
+    // Ignoring any potential errors
+
+    CloseHandle(file_);
+    file_ = INVALID_HANDLE_VALUE;
+  }
+}
+
+Status ChromiumWritableFileWin32::SyncParent() {
+  return Status();

[jsbell, 2013/12/12 01:11:42]

Add a comment here that SyncParent() is (apparently) not a concept that applies
on Win32.

[cmumford, 2013/12/12 17:40:51]

Will do. I found the original review
(https://chromiumcodereview.appspot.com/11445030) and have added his rationale
as a comment in this function.

[jsbell, 2013/12/12 19:03:14]

Thanks! dgrogan@ - should we just close crrev.com/19678017 ?

+}
+
+Status ChromiumWritableFileWin32::Append(const Slice& data) {
+  if (file_type_ == kManifest && tracker_->DoesDirNeedSync(filename_)) {
+    Status s = SyncParent();
+    if (!s.ok())
+      return s;
+    tracker_->DidSyncDir(filename_);
+  }
+
+  DWORD written(0);
+  if (!WriteFile(file_, data.data(), data.size(), &written, NULL)) {
+    DWORD err = GetLastError();
+    uma_logger_->RecordOSError(kWritableFileAppend,
+                               base::LastErrorToPlatformFileError(err));
+    return MakeIOErrorWin32(
+      filename_, GetWin32ErrorMessage(err), kWritableFileAppend, err);
+  }
+  return Status::OK();
+}
+
+Status ChromiumWritableFileWin32::Close() {
+  Status result;
+  DCHECK(file_ != INVALID_HANDLE_VALUE);
+  if (!CloseHandle(file_)) {
+    DWORD err = GetLastError();
+    result = MakeIOErrorWin32(
+      filename_, GetWin32ErrorMessage(err), kWritableFileClose, err);
+    uma_logger_->RecordErrorAt(kWritableFileClose);
+  }
+  file_ = INVALID_HANDLE_VALUE;
+  return result;
+}
+
+Status ChromiumWritableFileWin32::Flush() {
+  Status result;
+  if (!FlushFileBuffers(file_)) {
+    DWORD err = GetLastError();
+    result = MakeIOErrorWin32(
+      filename_, GetWin32ErrorMessage(err), kWritableFileFlush, err);
+    uma_logger_->RecordOSError(kWritableFileFlush,  base::LastErrorToPlatformFileError(err));
+  }
+  return result;
+}
+
+Status ChromiumWritableFileWin32::Sync() {
+  TRACE_EVENT0("leveldb", "ChromiumEnvWin32::Sync");
+  Status result;
+  DWORD error = ERROR_SUCCESS;
+
+  DCHECK(file_ != INVALID_HANDLE_VALUE);
+  if (!FlushFileBuffers(file_))
+    error = GetLastError();
+  if (error != ERROR_SUCCESS) {
+    result = MakeIOErrorWin32(
+      filename_, GetWin32ErrorMessage(error), kWritableFileSync, error);
+    uma_logger_->RecordErrorAt(kWritableFileSync);
+  } else if (make_backup_ && file_type_ == kTable) {
+    bool success = ChromiumEnv::MakeBackup(filename_);
+    uma_logger_->RecordBackupResult(success);
+  }
+  return result;
+}
+
+ChromiumEnvWin32::ChromiumEnvWin32() {
+}
+
+ChromiumEnvWin32::~ChromiumEnvWin32() {
+}
+
+Status ChromiumEnvWin32::NewSequentialFile(const std::string& fname,
+                                      SequentialFile** result) {
+  HANDLE f = CreateFile(UTF8ToUTF16(fname).c_str(), GENERIC_READ, 0, NULL,
+                        OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+  if (f == INVALID_HANDLE_VALUE) {
+    *result = NULL;
+    DWORD err = GetLastError();
+    RecordOSError(kNewSequentialFile, err);
+    return MakeIOErrorWin32(
+        fname, GetWin32ErrorMessage(err), kNewSequentialFile, err);
+  } else {
+    *result = new ChromiumSequentialFile(fname, f, this);
+    return Status::OK();
+  }
+}
+
+void ChromiumEnvWin32::RecordOpenFilesLimit(const std::string& type) {
+#if defined(OS_POSIX)

[jsbell, 2013/12/12 19:03:14]

How could this ever be true?

We may want to leave a comment in this no-op method that Windows doesn't have
file limits in the same way as "POSIX"

[cmumford, 2013/12/12 20:08:39]

I agree. I've deleted the #ifdef and added a comment.

+  struct rlimit nofile;
+  if (getrlimit(RLIMIT_NOFILE, &nofile))
+    return;
+  GetMaxFDHistogram(type)->Add(nofile.rlim_cur);
+#endif
+}
+
+Status ChromiumEnvWin32::NewRandomAccessFile(const std::string& fname,
+                                        RandomAccessFile** result) {
+  int flags = ::base::PLATFORM_FILE_READ | ::base::PLATFORM_FILE_OPEN;
+  bool created;
+  ::base::PlatformFileError error_code;
+  ::base::PlatformFile file = ::base::CreatePlatformFile(
+      ChromiumEnv::CreateFilePath(fname), flags, &created, &error_code);
+  if (error_code == ::base::PLATFORM_FILE_OK) {
+    *result = new ChromiumRandomAccessFile(fname, file, this);
+    RecordOpenFilesLimit("Success");
+    return Status::OK();
+  }
+  if (error_code == ::base::PLATFORM_FILE_ERROR_TOO_MANY_OPENED)
+    RecordOpenFilesLimit("TooManyOpened");
+  else
+    RecordOpenFilesLimit("OtherError");
+  *result = NULL;
+  RecordOSError(kNewRandomAccessFile, error_code);
+  return MakeIOErrorWin32(fname,
+                     PlatformFileErrorString(error_code),
+                     kNewRandomAccessFile,
+                     error_code);
+}
+
+Status ChromiumEnvWin32::NewWritableFile(const std::string& fname,
+                                    WritableFile** result) {
+  *result = NULL;
+  HANDLE f = CreateFile(UTF8ToUTF16(fname).c_str(), GENERIC_WRITE, FILE_SHARE_READ, NULL,
+                        CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
+  if (f == INVALID_HANDLE_VALUE) {
+    DWORD err = GetLastError();
+    RecordErrorAt(kNewWritableFile);
+    return MakeIOErrorWin32(
+        fname, GetWin32ErrorMessage(err), kNewWritableFile, err);
+  } else {
+    *result = new ChromiumWritableFileWin32(fname, f, this, this, make_backup_);
+    return Status::OK();
+  }
+}
+
+base::PlatformFileError ChromiumEnvWin32::GetDirectoryEntries(
+    const base::FilePath& dir_param,
+    std::vector<base::FilePath>* result) const {
+  result->clear();
+  base::FilePath dir_filepath = dir_param.Append(FILE_PATH_LITERAL("*"));
+  WIN32_FIND_DATA find_data;
+  HANDLE find_handle = FindFirstFile(dir_filepath.value().c_str(), &find_data);
+  if (find_handle == INVALID_HANDLE_VALUE) {
+    DWORD last_error = GetLastError();
+    if (last_error == ERROR_FILE_NOT_FOUND)
+      return base::PLATFORM_FILE_OK;
+    return base::LastErrorToPlatformFileError(last_error);
+  }
+  do {
+    base::FilePath filepath(find_data.cFileName);
+    base::FilePath::StringType basename = filepath.BaseName().value();
+    if (basename == FILE_PATH_LITERAL(".") ||
+        basename == FILE_PATH_LITERAL(".."))
+      continue;
+    result->push_back(filepath.BaseName());
+  } while (FindNextFile(find_handle, &find_data));
+  DWORD last_error = GetLastError();
+  base::PlatformFileError return_value = base::PLATFORM_FILE_OK;
+  if (last_error != ERROR_NO_MORE_FILES)
+    return_value = base::LastErrorToPlatformFileError(last_error);
+  FindClose(find_handle);
+  return return_value;
+}
+
+Status ChromiumEnvWin32::NewLogger(const std::string& fname, Logger** result) {
+  FILE* f = _wfopen(UTF8ToUTF16(fname).c_str(), L"w");
+  if (f == NULL) {
+    *result = NULL;
+    int saved_errno = errno;
+    RecordOSError(kNewLogger, saved_errno);
+    return MakeIOError(fname, strerror(saved_errno), kNewLogger, saved_errno);
+  } else {
+    *result = new ChromiumLogger(f);
+    return Status::OK();
+  }
+}
+
+void ChromiumEnvWin32::RecordOSError(MethodID method, DWORD error) const {
+  RecordErrorAt(method);
+  GetOSErrorHistogram(method, ERANGE + 1)->Add(error);
+}
+
+}  // namespace leveldb_env