Report errors from ChromiumEnv::GetChildren in Posix.

third_party/leveldatabase/env_chromium.cc

Index: third_party/leveldatabase/env_chromium.cc
diff --git a/third_party/leveldatabase/env_chromium.cc b/third_party/leveldatabase/env_chromium.cc
index 331e6e96ca2f64cbbc237df2ee9499d5c311ac79..681aa45d335279345ba057ca87480a1e81c03c4c 100644
--- a/third_party/leveldatabase/env_chromium.cc
+++ b/third_party/leveldatabase/env_chromium.cc
@@ -37,6 +37,7 @@
 #endif
 
 #if defined(OS_POSIX)
+#include <dirent.h>
 #include <fcntl.h>
 #include <sys/resource.h>
 #include <sys/time.h>
@@ -315,6 +316,8 @@ const char* MethodIDToString(MethodID method) {
       return "NewLogger";
     case kSyncParent:
       return "SyncParent";
+    case kGetChildren:
+      return "GetChildren";
     case kNumEntries:
       NOTREACHED();
       return "kNumEntries";
@@ -668,21 +671,61 @@ void ChromiumEnv::RestoreIfNecessary(const std::string& dir,
   }
 }
 
-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);
+#if defined(OS_WIN)

[jsbell, 2013/10/17 18:32:39]

Anonymous namespace around this.

[dgrogan, 2013/10/17 20:22:36]

Done.

+base::PlatformFileError GetDirectoryEntries(

[jsbell, 2013/10/17 18:32:39]

And mark this static. (Examples in base seem to do both.)

[dgrogan, 2013/10/17 20:22:36]

Done. Though I suspect the instances that of this in base are oversights.

+    const base::FilePath& dir_filepath,
+    std::vector<base::FilePath>* result) {
+  // TODO(dgrogan): Replace with FindFirstFile / FindNextFile.
+  base::FileEnumerator iter(dir_filepath, false, base::FileEnumerator::FILES);

[jsbell, 2013/10/17 18:32:39]

This is filtering out directories whereas the new POSIX version isn't. Although
that difference is temporary until this is reimplemented, maybe a TODO about it?

[dgrogan, 2013/10/17 20:22:36]

Added comment.

   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
+base::PlatformFileError GetDirectoryEntries(

[jsbell, 2013/10/17 18:32:39]

Mark static too.

[dgrogan, 2013/10/17 20:22:36]

Done.

+    const base::FilePath& dir_filepath,
+    std::vector<base::FilePath>* result) {
+  base::PlatformFileError return_value = base::PLATFORM_FILE_OK;

[jsbell, 2013/10/17 18:32:39]

return_value is unused

[dgrogan, 2013/10/17 20:22:36]

Done.

+  const std::string dir_string = dir_filepath.AsUTF8Unsafe();

[jsbell, 2013/10/17 18:32:39]

Why not FilePathToString here?

[dgrogan, 2013/10/17 20:22:36]

No good reason, switched to FilePathToString. They should be equivalent right?

[jsbell, 2013/10/17 20:47:42]

On non-Mac,non-CrOS, AsUTF8Unsafe() actually does a locale->UTF32->UTF8
conversion. We're not expecting non-ASCII filenames, but good to be consistent.

+  result->clear();
+  DIR* dir = opendir(dir_string.c_str());
+  if (!dir)
+    return base::ErrnoToPlatformFileError(errno);
+  struct dirent dent_buf;
+  struct dirent* dent;
+  int readdir_return;

[jsbell, 2013/10/17 18:32:39]

Name this readdir_result instead?

[dgrogan, 2013/10/17 20:22:36]

Done.

+  while ((readdir_return = readdir_r(dir, &dent_buf, &dent)) == 0 && dent) {

[jsbell, 2013/10/17 18:32:39]

Nit: No need for {}

[dgrogan, 2013/10/17 20:22:36]

Done.

+    result->push_back(CreateFilePath(dent->d_name));
+  }
+  int saved_errno = errno;
+  closedir(dir);
+  if (readdir_return != 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);
+    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();
 }