Improvements to Env::GetChildren (#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: https://github.com/facebook/rocksdb/pull/7819

Reviewed By: ajkr

Differential Revision: D25837394

Pulled By: jay-zhuang

fbshipit-source-id: 1e137e7218d38b450af9c083f73d5357abcbba2e

4 files changed, 15 insertions, 70 deletions

diff --git a/utilities/backupable/backupable_db.cc b/utilities/backupable/backupable_db.cc
index 075ac90c1..10245ad22 100644
--- a/utilities/backupable/backupable_db.cc
+++ b/utilities/backupable/backupable_db.cc
@@ -743,9 +743,6 @@ Status BackupEngineImpl::Initialize() {
   }
   // create backups_ structure
   for (auto& file : backup_meta_files) {
-    if (file == "." || file == "..") {
-      continue;
-    }
     ROCKS_LOG_INFO(options_.info_log, "Detected backup %s", file.c_str());
     BackupID backup_id = 0;
     sscanf(file.c_str(), "%u", &backup_id);
@@ -1985,9 +1982,6 @@ Status BackupEngineImpl::GarbageCollect() {
       }
     }
     for (auto& child : shared_children) {
-      if (child == "." || child == "..") {
-        continue;
-      }
       std::string rel_fname;
       if (with_checksum) {
         rel_fname = GetSharedFileWithChecksumRel(child);
@@ -2024,10 +2018,6 @@ Status BackupEngineImpl::GarbageCollect() {
     }
   }
   for (auto& child : private_children) {
-    if (child == "." || child == "..") {
-      continue;
-    }
-
     BackupID backup_id = 0;
     bool tmp_dir = child.find(".tmp") != std::string::npos;
     sscanf(child.c_str(), "%u", &backup_id);
@@ -2042,9 +2032,6 @@ Status BackupEngineImpl::GarbageCollect() {
     std::vector<std::string> subchildren;
     if (backup_env_->GetChildren(full_private_path, &subchildren).ok()) {
       for (auto& subchild : subchildren) {
-        if (subchild == "." || subchild == "..") {
-          continue;
-        }
         Status s = backup_env_->DeleteFile(full_private_path + subchild);
         ROCKS_LOG_INFO(options_.info_log, "Deleting %s -- %s",
                        (full_private_path + subchild).c_str(),
diff --git a/utilities/backupable/backupable_db_test.cc b/utilities/backupable/backupable_db_test.cc
index 6e5d0407c..3f2f94b81 100644
--- a/utilities/backupable/backupable_db_test.cc
+++ b/utilities/backupable/backupable_db_test.cc
@@ -417,11 +417,9 @@ class FileManager : public EnvWrapper {
     assert(fname != nullptr);
     while (true) {
       int i = rnd_.Next() % children.size();
-      if (children[i].name != "." && children[i].name != "..") {
-        fname->assign(dir + "/" + children[i].name);
-        *fsize = children[i].size_bytes;
-        return Status::OK();
-      }
+      fname->assign(dir + "/" + children[i].name);
+      *fsize = children[i].size_bytes;
+      return Status::OK();
     }
     // should never get here
     assert(false);
@@ -433,14 +431,10 @@ class FileManager : public EnvWrapper {
     Status s = GetChildren(dir, &children);
     if (!s.ok()) {
       return s;
-    } else if (children.size() <= 2) {  // . and ..
-      return Status::NotFound("");
     }
     while (true) {
       int i = rnd_.Next() % children.size();
-      if (children[i] != "." && children[i] != "..") {
-        return DeleteFile(dir + "/" + children[i]);
-      }
+      return DeleteFile(dir + "/" + children[i]);
     }
     // should never get here
     assert(false);
@@ -453,14 +447,10 @@ class FileManager : public EnvWrapper {
     Status s = GetChildren(dir, &children);
     if (!s.ok()) {
       return s;
-    } else if (children.size() <= 2) {
-      return Status::NotFound("");
     }
     while (true) {
       int i = rnd_.Next() % children.size();
-      if (children[i] != "." && children[i] != "..") {
-        return WriteToFile(dir + "/" + children[i], data);
-      }
+      return WriteToFile(dir + "/" + children[i], data);
     }
     // should never get here
     assert(false);
@@ -828,9 +818,6 @@ class BackupableDBTest : public testing::Test {
     ASSERT_OK(file_manager_->GetChildrenFileAttributes(dir, &children));
     int found_count = 0;
     for (const auto& child : children) {
-      if (child.name == "." || child.name == "..") {
-        continue;
-      }
       const std::string match("match");
       ASSERT_EQ(match, std::regex_replace(child.name, pattern, match));
       ++found_count;
@@ -844,9 +831,6 @@ class BackupableDBTest : public testing::Test {
     ASSERT_OK(file_manager_->GetChildrenFileAttributes(dir, &children));
     int found_count = 0;
     for (const auto& child : children) {
-      if (child.name == "." || child.name == "..") {
-        continue;
-      }
       auto last_underscore = child.name.find_last_of('_');
       auto last_dot = child.name.find_last_of('.');
       ASSERT_NE(child.name, child.name.substr(0, last_underscore));
@@ -1351,7 +1335,7 @@ TEST_F(BackupableDBTest, CorruptFileMaintainSize) {
   const std::string dir = backupdir_ + "/shared_checksum";
   ASSERT_OK(file_manager_->GetChildrenFileAttributes(dir, &children));
   for (const auto& child : children) {
-    if (child.name == "." || child.name == ".." || child.size_bytes == 0) {
+    if (child.size_bytes == 0) {
       continue;
     }
     // corrupt the file by replacing its content by file_size random bytes
@@ -1575,7 +1559,7 @@ TEST_F(BackupableDBTest, FlushCompactDuringBackupCheckpoint) {
       const std::string dir = backupdir_ + "/shared_checksum";
       ASSERT_OK(file_manager_->GetChildrenFileAttributes(dir, &children));
       for (const auto& child : children) {
-        if (child.name == "." || child.name == ".." || child.size_bytes == 0) {
+        if (child.size_bytes == 0) {
           continue;
         }
         const std::string match("match");
@@ -2061,7 +2045,7 @@ TEST_F(BackupableDBTest, FileSizeForIncremental) {
 
     // Corrupt backup SST
     ASSERT_OK(file_manager_->GetChildrenFileAttributes(shared_dir, &children));
-    ASSERT_EQ(children.size(), 3U);  // ".", "..", one sst
+    ASSERT_EQ(children.size(), 1U);  // one sst
     for (const auto& child : children) {
       if (child.name.size() > 4 && child.size_bytes > 0) {
         ASSERT_OK(
@@ -2121,7 +2105,7 @@ TEST_F(BackupableDBTest, FileSizeForIncremental) {
     // Count backup SSTs
     children.clear();
     ASSERT_OK(file_manager_->GetChildrenFileAttributes(shared_dir, &children));
-    ASSERT_EQ(children.size(), 4U);  // ".", "..", two sst
+    ASSERT_EQ(children.size(), 2U);  // two sst
 
     // Try create backup 3
     s = backup_engine_->CreateNewBackup(db_.get(), true /*flush*/);
@@ -2134,18 +2118,18 @@ TEST_F(BackupableDBTest, FileSizeForIncremental) {
       // Acceptable to call it corruption if size is not in name and
       // db session id collision is practically impossible.
       EXPECT_TRUE(s.IsCorruption());
-      EXPECT_EQ(children.size(), 4U);  // no SST added
+      EXPECT_EQ(children.size(), 2U);  // no SST added
     } else if (option == share_no_checksum) {
       // Good to call it corruption if both backups cannot be
       // accommodated.
       EXPECT_TRUE(s.IsCorruption());
-      EXPECT_EQ(children.size(), 4U);  // no SST added
+      EXPECT_EQ(children.size(), 2U);  // no SST added
     } else {
       // Since opening a DB seems sufficient for detecting size corruption
       // on the DB side, this should be a good thing, ...
       EXPECT_OK(s);
       // ... as long as we did actually treat it as a distinct SST file.
-      EXPECT_EQ(children.size(), 5U);  // Another SST added
+      EXPECT_EQ(children.size(), 3U);  // Another SST added
     }
     CloseDBAndBackupEngine();
     ASSERT_OK(DestroyDB(dbname_, options_));
diff --git a/utilities/checkpoint/checkpoint_test.cc b/utilities/checkpoint/checkpoint_test.cc
index 784fb5d46..5991a0de8 100644
--- a/utilities/checkpoint/checkpoint_test.cc
+++ b/utilities/checkpoint/checkpoint_test.cc
@@ -325,13 +325,7 @@ TEST_F(CheckpointTest, ExportColumnFamilyWithLinks) {
     ASSERT_EQ(metadata.files.size(), num_files_expected);
     std::vector<std::string> subchildren;
     ASSERT_OK(env_->GetChildren(export_path_, &subchildren));
-    int num_children = 0;
-    for (const auto& child : subchildren) {
-      if (child != "." && child != "..") {
-        ++num_children;
-      }
-    }
-    ASSERT_EQ(num_children, num_files_expected);
+    ASSERT_EQ(subchildren.size(), num_files_expected);
   };
 
   // Test DefaultColumnFamily
diff --git a/utilities/persistent_cache/persistent_cache_test.cc b/utilities/persistent_cache/persistent_cache_test.cc
index 3ed6e4c02..defff0f21 100644
--- a/utilities/persistent_cache/persistent_cache_test.cc
+++ b/utilities/persistent_cache/persistent_cache_test.cc
@@ -14,6 +14,7 @@
 #include <memory>
 #include <thread>
 
+#include "file/file_util.h"
 #include "utilities/persistent_cache/block_cache_tier.h"
 
 namespace ROCKSDB_NAMESPACE {
@@ -38,30 +39,9 @@ static void OnOpenForWrite(void* arg) {
 }
 #endif
 
-static void RemoveDirectory(const std::string& folder) {
-  std::vector<std::string> files;
-  Status status = Env::Default()->GetChildren(folder, &files);
-  if (!status.ok()) {
-    // we assume the directory does not exist
-    return;
-  }
-
-  // cleanup files with the patter :digi:.rc
-  for (auto file : files) {
-    if (file == "." || file == "..") {
-      continue;
-    }
-    status = Env::Default()->DeleteFile(folder + "/" + file);
-    assert(status.ok());
-  }
-
-  status = Env::Default()->DeleteDir(folder);
-  assert(status.ok());
-}
-
 static void OnDeleteDir(void* arg) {
   char* dir = static_cast<char*>(arg);
-  RemoveDirectory(std::string(dir));
+  ASSERT_OK(DestroyDir(Env::Default(), std::string(dir)));
 }
 
 //