Improvements to Env::GetChildren (facebook#7819)

Summary:
The main improvement here is to not include `.` or `..` in the results of `Env::GetChildren`. The occurrence of `.` or `..`; it is non-portable, dependent on the Operating System and the File System. See: https://www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html

There were lots of duplicate checks spread through the RocksDB codebase previously to skip `.` and `..`. This new removes the need for those at the source.

Also some minor fixes to `Env::GetChildren`:
* Improve error handling in POSIX implementation
* Remove unnecessary array allocation on Windows
* Fix struct name for Windows Non-UTF-8 API

Pull Request resolved: facebook#7819

Reviewed By: ajkr

Differential Revision: D25837394

Pulled By: jay-zhuang

fbshipit-source-id: 1e137e7218d38b450af9c083f73d5357abcbba2e

HISTORY.md

@@ -30,6 +30,7 @@
 
 ### Public API Change
 * Deprecated public but rarely-used FilterBitsBuilder::CalculateNumEntry, which is replaced with ApproximateNumEntries taking a size_t parameter and returning size_t.
+* To improve portability the functions `Env::GetChildren` and `Env::GetChildrenFileAttributes` will no longer return entries for the special directories `.` or `..`.
 
 ## 6.15.0 (11/13/2020)
 ### Bug Fixes

db/column_family_test.cc

@@ -898,9 +898,7 @@ TEST_P(ColumnFamilyTest, IgnoreRecoveredLog) {
   std::vector<std::string> old_files;
   ASSERT_OK(env_->GetChildren(backup_logs, &old_files));
   for (auto& file : old_files) {
-    if (file != "." && file != "..") {
-      ASSERT_OK(env_->DeleteFile(backup_logs + "/" + file));
-    }
+    ASSERT_OK(env_->DeleteFile(backup_logs + "/" + file));
   }
 
   column_family_options_.merge_operator =
@@ -929,9 +927,7 @@ TEST_P(ColumnFamilyTest, IgnoreRecoveredLog) {
   std::vector<std::string> logs;
   ASSERT_OK(env_->GetChildren(db_options_.wal_dir, &logs));
   for (auto& log : logs) {
-    if (log != ".." && log != ".") {
-      CopyFile(db_options_.wal_dir + "/" + log, backup_logs + "/" + log);
-    }
+    CopyFile(db_options_.wal_dir + "/" + log, backup_logs + "/" + log);
   }
 
   // recover the DB
@@ -956,9 +952,7 @@ TEST_P(ColumnFamilyTest, IgnoreRecoveredLog) {
     if (iter == 0) {
       // copy the logs from backup back to wal dir
       for (auto& log : logs) {
-        if (log != ".." && log != ".") {
-          CopyFile(backup_logs + "/" + log, db_options_.wal_dir + "/" + log);
-        }
+        CopyFile(backup_logs + "/" + log, db_options_.wal_dir + "/" + log);
       }
     }
   }

db/db_encryption_test.cc

@@ -44,7 +44,7 @@ TEST_F(DBEncryptionTest, CheckEncrypted) {
   Env* target = GetTargetEnv();
   int hits = 0;
   for (auto it = fileNames.begin() ; it != fileNames.end(); ++it) {
-    if ((*it == "..") || (*it == ".") || (*it == "LOCK")) {
+    if (*it == "LOCK") {
       continue;
     }
     auto filePath = dbname_ + "/" + *it;

db/db_test2.cc

@@ -41,9 +41,7 @@ TEST_F(DBTest2, OpenForReadOnly) {
   std::vector<std::string> files;
   ASSERT_OK(env_->GetChildren(dbname, &files));
   for (auto& f : files) {
-    if (f != "." && f != "..") {
-      ASSERT_OK(env_->DeleteFile(dbname + "/" + f));
-    }
+    ASSERT_OK(env_->DeleteFile(dbname + "/" + f));
   }
   // <dbname> should be empty now and we should be able to delete it
   ASSERT_OK(env_->DeleteDir(dbname));
@@ -75,9 +73,7 @@ TEST_F(DBTest2, OpenForReadOnlyWithColumnFamilies) {
   std::vector<std::string> files;
   ASSERT_OK(env_->GetChildren(dbname, &files));
   for (auto& f : files) {
-    if (f != "." && f != "..") {
-      ASSERT_OK(env_->DeleteFile(dbname + "/" + f));
-    }
+    ASSERT_OK(env_->DeleteFile(dbname + "/" + f));
   }
   // <dbname> should be empty now and we should be able to delete it
   ASSERT_OK(env_->DeleteDir(dbname));

db/db_wal_test.cc

@@ -506,9 +506,7 @@ TEST_F(DBWALTest, IgnoreRecoveredLog) {
     std::vector<std::string> old_files;
     ASSERT_OK(env_->GetChildren(backup_logs, &old_files));
     for (auto& file : old_files) {
-      if (file != "." && file != "..") {
-        ASSERT_OK(env_->DeleteFile(backup_logs + "/" + file));
-      }
+      ASSERT_OK(env_->DeleteFile(backup_logs + "/" + file));
     }
     Options options = CurrentOptions();
     options.create_if_missing = true;
@@ -528,9 +526,7 @@ TEST_F(DBWALTest, IgnoreRecoveredLog) {
     std::vector<std::string> logs;
     ASSERT_OK(env_->GetChildren(options.wal_dir, &logs));
     for (auto& log : logs) {
-      if (log != ".." && log != ".") {
-        CopyFile(options.wal_dir + "/" + log, backup_logs + "/" + log);
-      }
+      CopyFile(options.wal_dir + "/" + log, backup_logs + "/" + log);
     }
 
     // recover the DB
@@ -541,9 +537,7 @@ TEST_F(DBWALTest, IgnoreRecoveredLog) {
 
     // copy the logs from backup back to wal dir
     for (auto& log : logs) {
-      if (log != ".." && log != ".") {
-        CopyFile(backup_logs + "/" + log, options.wal_dir + "/" + log);
-      }
+      CopyFile(backup_logs + "/" + log, options.wal_dir + "/" + log);
     }
     // this should ignore the log files, recovery should not happen again
     // if the recovery happens, the same merge operator would be called twice,
@@ -559,9 +553,7 @@ TEST_F(DBWALTest, IgnoreRecoveredLog) {
     // copy the logs from backup back to wal dir
     ASSERT_OK(env_->CreateDirIfMissing(options.wal_dir));
     for (auto& log : logs) {
-      if (log != ".." && log != ".") {
-        CopyFile(backup_logs + "/" + log, options.wal_dir + "/" + log);
-      }
+      CopyFile(backup_logs + "/" + log, options.wal_dir + "/" + log);
     }
     // assert that we successfully recovered only from logs, even though we
     // destroyed the DB
@@ -574,11 +566,9 @@ TEST_F(DBWALTest, IgnoreRecoveredLog) {
     // copy the logs from backup back to wal dir
     ASSERT_OK(env_->CreateDirIfMissing(options.wal_dir));
     for (auto& log : logs) {
-      if (log != ".." && log != ".") {
-        CopyFile(backup_logs + "/" + log, options.wal_dir + "/" + log);
-        // we won't be needing this file no more
-        ASSERT_OK(env_->DeleteFile(backup_logs + "/" + log));
-      }
+      CopyFile(backup_logs + "/" + log, options.wal_dir + "/" + log);
+      // we won't be needing this file no more
+      ASSERT_OK(env_->DeleteFile(backup_logs + "/" + log));
     }
     Status s = TryReopen(options);
     ASSERT_NOK(s);

env/env_basic_test.cc

@@ -10,53 +10,14 @@
 #include <vector>
 
 #include "env/mock_env.h"
+#include "file/file_util.h"
 #include "rocksdb/convenience.h"
 #include "rocksdb/env.h"
 #include "rocksdb/env_encryption.h"
 #include "test_util/testharness.h"
 
 namespace ROCKSDB_NAMESPACE {
 
-// Normalizes trivial differences across Envs such that these test cases can
-// run on all Envs.
-class NormalizingEnvWrapper : public EnvWrapper {
- private:
-  std::unique_ptr<Env> base_;
-
- public:
-  explicit NormalizingEnvWrapper(std::unique_ptr<Env>&& base)
-      : EnvWrapper(base.get()), base_(std::move(base)) {}
-  explicit NormalizingEnvWrapper(Env* base) : EnvWrapper(base) {}
-
-  // Removes . and .. from directory listing
-  Status GetChildren(const std::string& dir,
-                     std::vector<std::string>* result) override {
-    Status status = EnvWrapper::GetChildren(dir, result);
-    if (status.ok()) {
-      result->erase(std::remove_if(result->begin(), result->end(),
-                                   [](const std::string& s) {
-                                     return s == "." || s == "..";
-                                   }),
-                    result->end());
-    }
-    return status;
-  }
-
-  // Removes . and .. from directory listing
-  Status GetChildrenFileAttributes(
-      const std::string& dir, std::vector<FileAttributes>* result) override {
-    Status status = EnvWrapper::GetChildrenFileAttributes(dir, result);
-    if (status.ok()) {
-      result->erase(std::remove_if(result->begin(), result->end(),
-                                   [](const FileAttributes& fa) {
-                                     return fa.name == "." || fa.name == "..";
-                                   }),
-                    result->end());
-    }
-    return status;
-  }
-};
-
 class EnvBasicTestWithParam : public testing::Test,
                               public ::testing::WithParamInterface<Env*> {
  public:
@@ -68,32 +29,17 @@ class EnvBasicTestWithParam : public testing::Test,
     test_dir_ = test::PerThreadDBPath(env_, "env_basic_test");
   }
 
-  void SetUp() override {
-    env_->CreateDirIfMissing(test_dir_).PermitUncheckedError();
-  }
+  void SetUp() override { ASSERT_OK(env_->CreateDirIfMissing(test_dir_)); }
 
-  void TearDown() override {
-    std::vector<std::string> files;
-    env_->GetChildren(test_dir_, &files).PermitUncheckedError();
-    for (const auto& file : files) {
-      // don't know whether it's file or directory, try both. The tests must
-      // only create files or empty directories, so one must succeed, else the
-      // directory's corrupted.
-      Status s = env_->DeleteFile(test_dir_ + "/" + file);
-      if (!s.ok()) {
-        ASSERT_OK(env_->DeleteDir(test_dir_ + "/" + file));
-      }
-    }
-  }
+  void TearDown() override { ASSERT_OK(DestroyDir(env_, test_dir_)); }
 };
 
 class EnvMoreTestWithParam : public EnvBasicTestWithParam {};
 
-static std::unique_ptr<Env> def_env(new NormalizingEnvWrapper(Env::Default()));
 INSTANTIATE_TEST_CASE_P(EnvDefault, EnvBasicTestWithParam,
-                        ::testing::Values(def_env.get()));
+                        ::testing::Values(Env::Default()));
 INSTANTIATE_TEST_CASE_P(EnvDefault, EnvMoreTestWithParam,
-                        ::testing::Values(def_env.get()));
+                        ::testing::Values(Env::Default()));
 
 static std::unique_ptr<Env> mock_env(new MockEnv(Env::Default()));
 INSTANTIATE_TEST_CASE_P(MockEnv, EnvBasicTestWithParam,
@@ -104,8 +50,7 @@ static Env* NewTestEncryptedEnv(Env* base, const std::string& provider_id) {
   std::shared_ptr<EncryptionProvider> provider;
   EXPECT_OK(EncryptionProvider::CreateFromString(ConfigOptions(), provider_id,
                                                  &provider));
-  std::unique_ptr<Env> encrypted(NewEncryptedEnv(base, provider));
-  return new NormalizingEnvWrapper(std::move(encrypted));
+  return NewEncryptedEnv(base, provider);
 }
 
 // next statements run env test against default encryption code.
@@ -374,6 +319,32 @@ TEST_P(EnvMoreTestWithParam, GetChildren) {
   ASSERT_EQ(0U, children.size());
 }
 
+TEST_P(EnvMoreTestWithParam, GetChildrenIgnoresDotAndDotDot) {
+  auto* env = Env::Default();
+  ASSERT_OK(env->CreateDirIfMissing(test_dir_));
+
+  // Create a single file
+  std::string path = test_dir_;
+  const EnvOptions soptions;
+#ifdef OS_WIN
+  path.append("\\test_file");
+#else
+  path.append("/test_file");
+#endif
+  std::string data("test data");
+  std::unique_ptr<WritableFile> file;
+  ASSERT_OK(env->NewWritableFile(path, &file, soptions));
+  ASSERT_OK(file->Append("test data"));
+
+  // get the children
+  std::vector<std::string> result;
+  ASSERT_OK(env->GetChildren(test_dir_, &result));
+
+  // expect only one file named `test_data`, i.e. no `.` or `..` names
+  ASSERT_EQ(result.size(), 1);
+  ASSERT_EQ(result.at(0), "test_file");
+}
+
 }  // namespace ROCKSDB_NAMESPACE
 int main(int argc, char** argv) {
   ::testing::InitGoogleTest(&argc, argv);

env/fs_posix.cc

@@ -607,6 +607,7 @@ class PosixFileSystem : public FileSystem {
                        std::vector<std::string>* result,
                        IODebugContext* /*dbg*/) override {
     result->clear();
+
     DIR* d = opendir(dir.c_str());
     if (d == nullptr) {
       switch (errno) {
@@ -618,11 +619,34 @@ class PosixFileSystem : public FileSystem {
           return IOError("While opendir", dir, errno);
       }
     }
+
+    const auto pre_read_errno = errno;  // errno may be modified by readdir
     struct dirent* entry;
-    while ((entry = readdir(d)) != nullptr) {
-      result->push_back(entry->d_name);
+    while ((entry = readdir(d)) != nullptr && errno == pre_read_errno) {
+      // filter out '.' and '..' directory entries
+      // which appear only on some platforms
+      const bool ignore =
+          entry->d_type == DT_DIR &&
+          (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0);
+      if (!ignore) {
+        result->push_back(entry->d_name);
+      }
     }
-    closedir(d);
+
+    // always attempt to close the dir
+    const auto pre_close_errno = errno;  // errno may be modified by closedir
+    const int close_result = closedir(d);
+
+    if (pre_close_errno != pre_read_errno) {
+      // error occured during readdir
+      return IOError("While readdir", dir, pre_close_errno);
+    }
+
+    if (close_result != 0) {
+      // error occured during closedir
+      return IOError("While closedir", dir, errno);
+    }
+
     return IOStatus::OK();
   }
 

file/delete_scheduler_test.cc

@@ -57,7 +57,7 @@ class DeleteSchedulerTest : public testing::Test {
 
     int normal_cnt = 0;
     for (auto& f : files_in_dir) {
-      if (!DeleteScheduler::IsTrashFile(f) && f != "." && f != "..") {
+      if (!DeleteScheduler::IsTrashFile(f)) {
         normal_cnt++;
       }
     }

file/file_util.cc

@@ -230,9 +230,6 @@ Status DestroyDir(Env* env, const std::string& dir) {
   s = env->GetChildren(dir, &files_in_dir);
   if (s.ok()) {
     for (auto& file_in_dir : files_in_dir) {
-      if (file_in_dir == "." || file_in_dir == "..") {
-        continue;
-      }
       std::string path = dir + "/" + file_in_dir;
       bool is_dir = false;
       s = env->IsDirectory(path, &is_dir);

file/sst_file_manager_impl.cc

@@ -510,10 +510,6 @@ SstFileManager* NewSstFileManager(Env* env, std::shared_ptr<FileSystem> fs,
     s = fs->GetChildren(trash_dir, IOOptions(), &files_in_trash, nullptr);
     if (s.ok()) {
       for (const std::string& trash_file : files_in_trash) {
-        if (trash_file == "." || trash_file == "..") {
-          continue;
-        }
-
         std::string path_in_trash = trash_dir + "/" + trash_file;
         res->OnAddFile(path_in_trash);
         Status file_delete =

include/rocksdb/env.h

@@ -283,7 +283,8 @@ class Env {
   virtual Status FileExists(const std::string& fname) = 0;
 
   // Store in *result the names of the children of the specified directory.
-  // The names are relative to "dir".
+  // The names are relative to "dir", and shall never include the
+  // names `.` or `..`.
   // Original contents of *results are dropped.
   // Returns OK if "dir" exists and "*result" contains its children.
   //         NotFound if "dir" does not exist, the calling process does not have
@@ -296,7 +297,8 @@ class Env {
   // In case the implementation lists the directory prior to iterating the files
   // and files are concurrently deleted, the deleted files will be omitted from
   // result.
-  // The name attributes are relative to "dir".
+  // The name attributes are relative to "dir", and shall never include the
+  // names `.` or `..`.
   // Original contents of *results are dropped.
   // Returns OK if "dir" exists and "*result" contains its children.
   //         NotFound if "dir" does not exist, the calling process does not have