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);
+#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();
 }
 
 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