Report errors from ChromiumEnv::GetChildren in Posix.

third_party/leveldatabase/env_chromium.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 <errno.h>
 #include <stdio.h>
 
 #include <deque>
 
 #include "base/at_exit.h"
@@ skipping 19 matching lines @@
 #include "port/port.h"
 #include "third_party/re2/re2/re2.h"
 #include "util/logging.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>
 #include <sys/time.h>
 #endif
 
 using namespace leveldb;
 
 namespace leveldb_env {
 
 namespace {
@@ skipping 258 matching lines @@
     case kLockFile:
       return "LockFile";
     case kUnlockFile:
       return "UnlockFile";
     case kGetTestDirectory:
       return "GetTestDirectory";
     case kNewLogger:
       return "NewLogger";
     case kSyncParent:
       return "SyncParent";
+    case kGetChildren:
+      return "GetChildren";
     case kNumEntries:
       NOTREACHED();
       return "kNumEntries";
   }
   NOTREACHED();
   return "Unknown";
 }
 
 Status MakeIOError(Slice filename,
                    const char* message,
@@ skipping 333 matching lines @@
   base::FilePath dir_filepath = base::FilePath::FromUTF8Unsafe(dir);
   for (std::set<base::FilePath>::iterator it = backups_only.begin();
        it != backups_only.end();
        ++it) {
     base::FilePath restored_table_name =
         RestoreFromBackup(dir_filepath.Append(*it));
     result->push_back(FilePathToString(restored_table_name.BaseName()));
   }
 }
 
-Status ChromiumEnv::GetChildren(const std::string& dir,
-                                std::vector<std::string>* result) {
-  result->clear();
-  // TODO(jorlow): Unfortunately, the FileEnumerator swallows errors, so
-  //               we'll always return OK. Maybe manually check for error
-  //               conditions like the file not existing?
-  base::FileEnumerator iter(
-      CreateFilePath(dir), false, base::FileEnumerator::FILES);
+namespace {
+#if defined(OS_WIN)
+static base::PlatformFileError GetDirectoryEntries(
+    const base::FilePath& dir_filepath,
+    std::vector<base::FilePath>* result) {
+  // TODO(dgrogan): Replace with FindFirstFile / FindNextFile. Note that until
+  // that happens this is filtering out directories whereas the Posix version
+  // below is not. There shouldn't be any directories so this shouldn't be an
+  // issue.
+  base::FileEnumerator iter(dir_filepath, false, base::FileEnumerator::FILES);
   base::FilePath current = iter.Next();
   while (!current.empty()) {
-    result->push_back(FilePathToString(current.BaseName()));
+    result->push_back(current.BaseName());
     current = iter.Next();
   }
+  return base::PLATFORM_FILE_OK;
+}
+#else
+static base::PlatformFileError GetDirectoryEntries(
+    const base::FilePath& dir_filepath,
+    std::vector<base::FilePath>* result) {
+  const std::string dir_string = FilePathToString(dir_filepath);
+  result->clear();
+  DIR* dir = opendir(dir_string.c_str());
+  if (!dir)
+    return base::ErrnoToPlatformFileError(errno);
+  struct dirent dent_buf;
+  struct dirent* dent;
+  int readdir_result;
+  while ((readdir_result = readdir_r(dir, &dent_buf, &dent)) == 0 && dent)
+    result->push_back(CreateFilePath(dent->d_name));
+  int saved_errno = errno;
+  closedir(dir);
+  if (readdir_result != 0)
+    return base::ErrnoToPlatformFileError(saved_errno);
+  return base::PLATFORM_FILE_OK;
+}
+#endif
+}
+
+Status ChromiumEnv::GetChildren(const std::string& dir_string,
+                                std::vector<std::string>* result) {
+  std::vector<base::FilePath> entries;
+  base::PlatformFileError error =
+      GetDirectoryEntries(CreateFilePath(dir_string), &entries);
+  if (error != base::PLATFORM_FILE_OK) {
+    RecordOSError(kGetChildren, error);

[Nico, 2013/10/18 02:28:30]

Does this mean that PlatformFileError needs to have stable values now? If so,
I'm not sure I like this. Could you convert this to some enum defined in this
file instead, with a comment saying "this is used in a histogram, only add
values"?

[dgrogan, 2013/10/18 20:52:59]

That ship may have already sailed. It's not just us who have been recording
PlatformFileError in histograms, but the disk cache has been also.

What's the reservation about PlatformFileError having stable values?

+    return MakeIOError(
+        dir_string, "Could not open/read directory", kGetChildren, error);
+  }
+  for (std::vector<base::FilePath>::iterator it = entries.begin();
+       it != entries.end();
+       ++it) {
+    result->push_back(FilePathToString(*it));
+  }
+
   if (make_backup_)
-    RestoreIfNecessary(dir, result);
+    RestoreIfNecessary(dir_string, result);
   return Status::OK();
 }
 
 Status ChromiumEnv::DeleteFile(const std::string& fname) {
   Status result;
   base::FilePath fname_filepath = CreateFilePath(fname);
   // TODO(jorlow): Should we assert this is a file?
   if (!::base::DeleteFile(fname_filepath, false)) {
     result = MakeIOError(fname, "Could not delete file.", kDeleteFile);
     RecordErrorAt(kDeleteFile);
@@ skipping 375 matching lines @@
 Env* IDBEnv() {
   return leveldb_env::idb_env.Pointer();
 }
 
 Env* Env::Default() {
   return leveldb_env::default_env.Pointer();
 }
 
 }  // namespace leveldb