From 4926b33742f03b45b64b4aeabfc7492908c04885 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sat, 9 Jan 2021 09:42:21 -0800 Subject: [PATCH] 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 --- HISTORY.md | 1 + db/column_family_test.cc | 12 +-- db/db_encryption_test.cc | 2 +- db/db_test2.cc | 8 +- db/db_wal_test.cc | 24 ++--- env/env_basic_test.cc | 93 +++++++------------ env/fs_posix.cc | 30 +++++- file/delete_scheduler_test.cc | 2 +- file/file_util.cc | 3 - file/sst_file_manager_impl.cc | 4 - include/rocksdb/env.h | 6 +- port/win/env_win.cc | 18 ++-- port/win/port_win.h | 2 +- utilities/backupable/backupable_db.cc | 13 --- utilities/backupable/backupable_db_test.cc | 40 +++----- utilities/checkpoint/checkpoint_test.cc | 8 +- .../persistent_cache/persistent_cache_test.cc | 24 +---- 17 files changed, 104 insertions(+), 186 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 3154335ba..9219f4ad2 100644 --- a/HISTORY.md +++ b/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 diff --git a/db/column_family_test.cc b/db/column_family_test.cc index a4bfcde3a..afb48d56f 100644 --- a/db/column_family_test.cc +++ b/db/column_family_test.cc @@ -898,9 +898,7 @@ TEST_P(ColumnFamilyTest, IgnoreRecoveredLog) { std::vector 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 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); } } } diff --git a/db/db_encryption_test.cc b/db/db_encryption_test.cc index 2322d0867..c2ec3ec8a 100644 --- a/db/db_encryption_test.cc +++ b/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; diff --git a/db/db_test2.cc b/db/db_test2.cc index 8587686e3..33c13e69c 100644 --- a/db/db_test2.cc +++ b/db/db_test2.cc @@ -41,9 +41,7 @@ TEST_F(DBTest2, OpenForReadOnly) { std::vector 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)); } // 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 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)); } // should be empty now and we should be able to delete it ASSERT_OK(env_->DeleteDir(dbname)); diff --git a/db/db_wal_test.cc b/db/db_wal_test.cc index babaf5ad5..0d20b9b19 100644 --- a/db/db_wal_test.cc +++ b/db/db_wal_test.cc @@ -506,9 +506,7 @@ TEST_F(DBWALTest, IgnoreRecoveredLog) { std::vector 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 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); diff --git a/env/env_basic_test.cc b/env/env_basic_test.cc index 1db86dd1c..b11afb1fe 100644 --- a/env/env_basic_test.cc +++ b/env/env_basic_test.cc @@ -10,6 +10,7 @@ #include #include "env/mock_env.h" +#include "file/file_util.h" #include "rocksdb/convenience.h" #include "rocksdb/env.h" #include "rocksdb/env_encryption.h" @@ -17,46 +18,6 @@ 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 base_; - - public: - explicit NormalizingEnvWrapper(std::unique_ptr&& 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* 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* 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 { 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 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 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 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 provider; EXPECT_OK(EncryptionProvider::CreateFromString(ConfigOptions(), provider_id, &provider)); - std::unique_ptr 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 file; + ASSERT_OK(env->NewWritableFile(path, &file, soptions)); + ASSERT_OK(file->Append("test data")); + + // get the children + std::vector 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); diff --git a/env/fs_posix.cc b/env/fs_posix.cc index c38c62811..2b76088af 100644 --- a/env/fs_posix.cc +++ b/env/fs_posix.cc @@ -607,6 +607,7 @@ class PosixFileSystem : public FileSystem { std::vector* 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(); } diff --git a/file/delete_scheduler_test.cc b/file/delete_scheduler_test.cc index a53a50be3..e6cc240b6 100644 --- a/file/delete_scheduler_test.cc +++ b/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++; } } diff --git a/file/file_util.cc b/file/file_util.cc index 7da27f3c1..4b0f17c7f 100644 --- a/file/file_util.cc +++ b/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); diff --git a/file/sst_file_manager_impl.cc b/file/sst_file_manager_impl.cc index 35aa667d2..359ebf999 100644 --- a/file/sst_file_manager_impl.cc +++ b/file/sst_file_manager_impl.cc @@ -510,10 +510,6 @@ SstFileManager* NewSstFileManager(Env* env, std::shared_ptr 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 = diff --git a/include/rocksdb/env.h b/include/rocksdb/env.h index 5a0ace8cc..f8eec967d 100644 --- a/include/rocksdb/env.h +++ b/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 diff --git a/port/win/env_win.cc b/port/win/env_win.cc index 9832dc347..1e6e9d65b 100644 --- a/port/win/env_win.cc +++ b/port/win/env_win.cc @@ -645,7 +645,6 @@ IOStatus WinFileSystem::GetChildren(const std::string& dir, IODebugContext* /*dbg*/) { IOStatus status; result->clear(); - std::vector output; RX_WIN32_FIND_DATA data; memset(&data, 0, sizeof(data)); @@ -677,16 +676,20 @@ IOStatus WinFileSystem::GetChildren(const std::string& dir, UniqueFindClosePtr fc(handle, FindCloseFunc); - if (result->capacity() > 0) { - output.reserve(result->capacity()); - } - // For safety data.cFileName[MAX_PATH - 1] = 0; while (true) { - auto x = RX_FILESTRING(data.cFileName, RX_FNLEN(data.cFileName)); - output.emplace_back(FN_TO_RX(x)); + // filter out '.' and '..' directory entries + // which appear only on some platforms + const bool ignore = + ((data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0) && + (strcmp(data.cFileName, ".") == 0 || strcmp(data.cFileName, "..") == 0); + if (!ignore) { + auto x = RX_FILESTRING(data.cFileName, RX_FNLEN(data.cFileName)); + result->push_back(FN_TO_RX(x)); + } + BOOL ret = -RX_FindNextFile(handle, &data); // If the function fails the return value is zero // and non-zero otherwise. Not TRUE or FALSE. @@ -696,7 +699,6 @@ IOStatus WinFileSystem::GetChildren(const std::string& dir, } data.cFileName[MAX_PATH - 1] = 0; } - output.swap(*result); return status; } diff --git a/port/win/port_win.h b/port/win/port_win.h index 2c5b8ff05..2da6314e1 100644 --- a/port/win/port_win.h +++ b/port/win/port_win.h @@ -388,7 +388,7 @@ extern void SetCpuPriority(ThreadId id, CpuPriority priority); #define RX_FindFirstFileEx FindFirstFileExA #define RX_CreateDirectory CreateDirectoryA #define RX_FindNextFile FindNextFileA -#define RX_WIN32_FIND_DATA WIN32_FIND_DATA +#define RX_WIN32_FIND_DATA WIN32_FIND_DATAA #define RX_CreateDirectory CreateDirectoryA #define RX_RemoveDirectory RemoveDirectoryA #define RX_GetFileAttributesEx GetFileAttributesExA 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 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 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 #include +#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 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(arg); - RemoveDirectory(std::string(dir)); + ASSERT_OK(DestroyDir(Env::Default(), std::string(dir))); } //