tqcq/leveldb
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Add Env::Remove{File,Dir} which obsolete Env::Delete{File,Dir}.

Browse Source 
The "DeleteFile" method name causes pain for Windows developers, because
<windows.h> #defines a DeleteFile macro to DeleteFileW or DeleteFileA.
Current code uses workarounds, like #undefining DeleteFile everywhere an
Env is declared, implemented, or used.

This CL removes the need for workarounds by renaming Env::DeleteFile to
Env::RemoveFile. For consistency, Env::DeleteDir is also renamed to
Env::RemoveDir. A few internal methods are also renamed for consistency.
Software that supports Windows is expected to migrate any Env
implementations and usage to Remove{File,Dir}, and never use the name
Env::Delete{File,Dir} in its code.

The renaming is done in a backwards-compatible way, at the risk of
making it slightly more difficult to build a new correct Env
implementation. The backwards compatibility is achieved using the
following hacks:

1) Env::Remove{File,Dir} methods are added, with a default
    implementation that calls into Env::Delete{File,Dir}. This makes old
    Env implementations compatible with code that calls into the updated
    API.
2) The Env::Delete{File,Dir} methods are no longer pure virtuals.
    Instead, they gain a default implementation that calls into
    Env::Remove{File,Dir}. This makes updated Env implementations
    compatible with code that calls into the old API.

The cost of this approach is that it's possible to write an Env without
overriding either Rename{File,Dir} or Delete{File,Dir}, without getting
a compiler warning. However, attempting to run the test suite will
immediately fail with an infinite call stack ending in
{Remove,Delete}{File,Dir}, making developers aware of the problem.

PiperOrigin-RevId: 288710907
This commit is contained in:
Victor Costan 2020-01-08 09:14:53 -08:00 committed by Victor Costan
parent d152b23f3b
commit a0191e5563
25 changed files with 138 additions and 97 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
benchmarks/db_bench.cc
View File

@ -414,7 +414,7 @@ class Benchmark {
g_env->GetChildren(FLAGS_db, &files); g_env->GetChildren(FLAGS_db, &files);
for (size_t i = 0; i < files.size(); i++) { for (size_t i = 0; i < files.size(); i++) {
if (Slice(files[i]).starts_with("heap-")) { if (Slice(files[i]).starts_with("heap-")) {
g_env->DeleteFile(std::string(FLAGS_db) + "/" + files[i]); g_env->RemoveFile(std::string(FLAGS_db) + "/" + files[i]);
} }
} }
if (!FLAGS_use_existing_db) { if (!FLAGS_use_existing_db) {
@ -912,7 +912,7 @@ class Benchmark {
delete file; delete file;
if (!ok) { if (!ok) {
fprintf(stderr, "heap profiling not supported\n"); fprintf(stderr, "heap profiling not supported\n");
g_env->DeleteFile(fname); g_env->RemoveFile(fname);
} }
} }
}; };

2
benchmarks/db_bench_sqlite3.cc
View File

@ -328,7 +328,7 @@ class Benchmark {
std::string file_name(test_dir); std::string file_name(test_dir);
file_name += "/"; file_name += "/";
file_name += files[i]; file_name += files[i];
Env::Default()->DeleteFile(file_name.c_str()); Env::Default()->RemoveFile(file_name.c_str());
} }
} }
} }

2
benchmarks/db_bench_tree_db.cc
View File

@ -301,7 +301,7 @@ class Benchmark {
std::string file_name(test_dir); std::string file_name(test_dir);
file_name += "/"; file_name += "/";
file_name += files[i]; file_name += files[i];
Env::Default()->DeleteFile(file_name.c_str()); Env::Default()->RemoveFile(file_name.c_str());
} }
} }
} }

2
db/builder.cc
View File

@ -71,7 +71,7 @@ Status BuildTable(const std::string& dbname, Env* env, const Options& options,
if (s.ok() && meta->file_size > 0) { if (s.ok() && meta->file_size > 0) {
// Keep it // Keep it
} else { } else {
env->DeleteFile(fname); env->RemoveFile(fname);
} }
return s; return s;
} }

20
db/db_impl.cc
View File

@ -206,7 +206,7 @@ Status DBImpl::NewDB() {
// Make "CURRENT" file that points to the new manifest file. // Make "CURRENT" file that points to the new manifest file.
s = SetCurrentFile(env_, dbname_, 1); s = SetCurrentFile(env_, dbname_, 1);
} else { } else {
env_->DeleteFile(manifest); env_->RemoveFile(manifest);
} }
return s; return s;
} }
@ -220,7 +220,7 @@ void DBImpl::MaybeIgnoreError(Status* s) const {
} }
} }
void DBImpl::DeleteObsoleteFiles() { void DBImpl::RemoveObsoleteFiles() {
mutex_.AssertHeld(); mutex_.AssertHeld();
if (!bg_error_.ok()) { if (!bg_error_.ok()) {
@ -282,7 +282,7 @@ void DBImpl::DeleteObsoleteFiles() {
// are therefore safe to delete while allowing other threads to proceed. // are therefore safe to delete while allowing other threads to proceed.
mutex_.Unlock(); mutex_.Unlock();
for (const std::string& filename : files_to_delete) { for (const std::string& filename : files_to_delete) {
env_->DeleteFile(dbname_ + "/" + filename); env_->RemoveFile(dbname_ + "/" + filename);
} }
mutex_.Lock(); mutex_.Lock();
} }
@ -569,7 +569,7 @@ void DBImpl::CompactMemTable() {
imm_->Unref(); imm_->Unref();
imm_ = nullptr; imm_ = nullptr;
has_imm_.store(false, std::memory_order_release); has_imm_.store(false, std::memory_order_release);
DeleteObsoleteFiles(); RemoveObsoleteFiles();
} else { } else {
RecordBackgroundError(s); RecordBackgroundError(s);
} }
@ -729,7 +729,7 @@ void DBImpl::BackgroundCompaction() {
// Move file to next level // Move file to next level
assert(c->num_input_files(0) == 1); assert(c->num_input_files(0) == 1);
FileMetaData* f = c->input(0, 0); FileMetaData* f = c->input(0, 0);
c->edit()->DeleteFile(c->level(), f->number); c->edit()->RemoveFile(c->level(), f->number);
c->edit()->AddFile(c->level() + 1, f->number, f->file_size, f->smallest, c->edit()->AddFile(c->level() + 1, f->number, f->file_size, f->smallest,
f->largest); f->largest);
status = versions_->LogAndApply(c->edit(), &mutex_); status = versions_->LogAndApply(c->edit(), &mutex_);
@ -749,7 +749,7 @@ void DBImpl::BackgroundCompaction() {
} }
CleanupCompaction(compact); CleanupCompaction(compact);
c->ReleaseInputs(); c->ReleaseInputs();
DeleteObsoleteFiles(); RemoveObsoleteFiles();
} }
delete c; delete c;
@ -1506,7 +1506,7 @@ Status DB::Open(const Options& options, const std::string& dbname, DB** dbptr) {
s = impl->versions_->LogAndApply(&edit, &impl->mutex_); s = impl->versions_->LogAndApply(&edit, &impl->mutex_);
} }
if (s.ok()) { if (s.ok()) {
impl->DeleteObsoleteFiles(); impl->RemoveObsoleteFiles();
impl->MaybeScheduleCompaction(); impl->MaybeScheduleCompaction();
} }
impl->mutex_.Unlock(); impl->mutex_.Unlock();
@ -1539,15 +1539,15 @@ Status DestroyDB(const std::string& dbname, const Options& options) {
for (size_t i = 0; i < filenames.size(); i++) { for (size_t i = 0; i < filenames.size(); i++) {
if (ParseFileName(filenames[i], &number, &type) && if (ParseFileName(filenames[i], &number, &type) &&
type != kDBLockFile) { // Lock file will be deleted at end type != kDBLockFile) { // Lock file will be deleted at end
Status del = env->DeleteFile(dbname + "/" + filenames[i]); Status del = env->RemoveFile(dbname + "/" + filenames[i]);
if (result.ok() && !del.ok()) { if (result.ok() && !del.ok()) {
result = del; result = del;
} }
} }
} }
env->UnlockFile(lock); // Ignore error since state is already gone env->UnlockFile(lock); // Ignore error since state is already gone
env->DeleteFile(lockname); env->RemoveFile(lockname);
env->DeleteDir(dbname); // Ignore error in case dir contains other files env->RemoveDir(dbname); // Ignore error in case dir contains other files
} }
return result; return result;
} }

2
db/db_impl.h
View File

@ -116,7 +116,7 @@ class DBImpl : public DB {
void MaybeIgnoreError(Status* s) const; void MaybeIgnoreError(Status* s) const;
// Delete any unneeded files and stale in-memory entries. // Delete any unneeded files and stale in-memory entries.
void DeleteObsoleteFiles() EXCLUSIVE_LOCKS_REQUIRED(mutex_); void RemoveObsoleteFiles() EXCLUSIVE_LOCKS_REQUIRED(mutex_);
// Compact the in-memory write buffer to disk. Switches to a new // Compact the in-memory write buffer to disk. Switches to a new
// log-file/memtable and writes a new descriptor iff successful. // log-file/memtable and writes a new descriptor iff successful.

8
db/db_test.cc
View File

@ -509,7 +509,7 @@ class DBTest : public testing::Test {
FileType type; FileType type;
for (size_t i = 0; i < filenames.size(); i++) { for (size_t i = 0; i < filenames.size(); i++) {
if (ParseFileName(filenames[i], &number, &type) && type == kTableFile) { if (ParseFileName(filenames[i], &number, &type) && type == kTableFile) {
EXPECT_LEVELDB_OK(env_->DeleteFile(TableFileName(dbname_, number))); EXPECT_LEVELDB_OK(env_->RemoveFile(TableFileName(dbname_, number)));
return true; return true;
} }
} }
@ -1666,7 +1666,7 @@ TEST_F(DBTest, DBOpen_Options) {
TEST_F(DBTest, DestroyEmptyDir) { TEST_F(DBTest, DestroyEmptyDir) {
std::string dbname = testing::TempDir() + "db_empty_dir"; std::string dbname = testing::TempDir() + "db_empty_dir";
TestEnv env(Env::Default()); TestEnv env(Env::Default());
env.DeleteDir(dbname); env.RemoveDir(dbname);
ASSERT_TRUE(!env.FileExists(dbname)); ASSERT_TRUE(!env.FileExists(dbname));
Options opts; Options opts;
@ -1693,7 +1693,7 @@ TEST_F(DBTest, DestroyEmptyDir) {
TEST_F(DBTest, DestroyOpenDB) { TEST_F(DBTest, DestroyOpenDB) {
std::string dbname = testing::TempDir() + "open_db_dir"; std::string dbname = testing::TempDir() + "open_db_dir";
env_->DeleteDir(dbname); env_->RemoveDir(dbname);
ASSERT_TRUE(!env_->FileExists(dbname)); ASSERT_TRUE(!env_->FileExists(dbname));
Options opts; Options opts;
@ -2279,7 +2279,7 @@ void BM_LogAndApply(int iters, int num_base_files) {
for (int i = 0; i < iters; i++) { for (int i = 0; i < iters; i++) {
VersionEdit vedit; VersionEdit vedit;
vedit.DeleteFile(2, fnum); vedit.RemoveFile(2, fnum);
InternalKey start(MakeKey(2 * fnum), 1, kTypeValue); InternalKey start(MakeKey(2 * fnum), 1, kTypeValue);
InternalKey limit(MakeKey(2 * fnum + 1), 1, kTypeDeletion); InternalKey limit(MakeKey(2 * fnum + 1), 1, kTypeDeletion);
vedit.AddFile(2, fnum++, 1 /* file size */, start, limit); vedit.AddFile(2, fnum++, 1 /* file size */, start, limit);

22
db/fault_injection_test.cc
View File

@ -77,7 +77,7 @@ Status Truncate(const std::string& filename, uint64_t length) {
if (s.ok()) { if (s.ok()) {
s = env->RenameFile(tmp_name, filename); s = env->RenameFile(tmp_name, filename);
} else { } else {
env->DeleteFile(tmp_name); env->RemoveFile(tmp_name);
} }
} }
} }
@ -138,12 +138,12 @@ class FaultInjectionTestEnv : public EnvWrapper {
WritableFile** result) override; WritableFile** result) override;
Status NewAppendableFile(const std::string& fname, Status NewAppendableFile(const std::string& fname,
WritableFile** result) override; WritableFile** result) override;
Status DeleteFile(const std::string& f) override; Status RemoveFile(const std::string& f) override;
Status RenameFile(const std::string& s, const std::string& t) override; Status RenameFile(const std::string& s, const std::string& t) override;
void WritableFileClosed(const FileState& state); void WritableFileClosed(const FileState& state);
Status DropUnsyncedFileData(); Status DropUnsyncedFileData();
Status DeleteFilesCreatedAfterLastDirSync(); Status RemoveFilesCreatedAfterLastDirSync();
void DirWasSynced(); void DirWasSynced();
bool IsFileCreatedSinceLastDirSync(const std::string& filename); bool IsFileCreatedSinceLastDirSync(const std::string& filename);
void ResetState(); void ResetState();
@ -303,8 +303,8 @@ void FaultInjectionTestEnv::UntrackFile(const std::string& f) {
new_files_since_last_dir_sync_.erase(f); new_files_since_last_dir_sync_.erase(f);
} }
Status FaultInjectionTestEnv::DeleteFile(const std::string& f) { Status FaultInjectionTestEnv::RemoveFile(const std::string& f) {
Status s = EnvWrapper::DeleteFile(f); Status s = EnvWrapper::RemoveFile(f);
EXPECT_LEVELDB_OK(s); EXPECT_LEVELDB_OK(s);
if (s.ok()) { if (s.ok()) {
UntrackFile(f); UntrackFile(f);
@ -340,17 +340,17 @@ void FaultInjectionTestEnv::ResetState() {
SetFilesystemActive(true); SetFilesystemActive(true);
} }
Status FaultInjectionTestEnv::DeleteFilesCreatedAfterLastDirSync() { Status FaultInjectionTestEnv::RemoveFilesCreatedAfterLastDirSync() {
// Because DeleteFile access this container make a copy to avoid deadlock // Because RemoveFile access this container make a copy to avoid deadlock
mutex_.Lock(); mutex_.Lock();
std::set<std::string> new_files(new_files_since_last_dir_sync_.begin(), std::set<std::string> new_files(new_files_since_last_dir_sync_.begin(),
new_files_since_last_dir_sync_.end()); new_files_since_last_dir_sync_.end());
mutex_.Unlock(); mutex_.Unlock();
Status status; Status status;
for (const auto& new_file : new_files) { for (const auto& new_file : new_files) {
Status delete_status = DeleteFile(new_file); Status remove_status = RemoveFile(new_file);
if (!delete_status.ok() && status.ok()) { if (!remove_status.ok() && status.ok()) {
status = std::move(delete_status); status = std::move(remove_status);
} }
} }
return status; return status;
@ -482,7 +482,7 @@ class FaultInjectionTest : public testing::Test {
ASSERT_LEVELDB_OK(env_->DropUnsyncedFileData()); ASSERT_LEVELDB_OK(env_->DropUnsyncedFileData());
break; break;
case RESET_DELETE_UNSYNCED_FILES: case RESET_DELETE_UNSYNCED_FILES:
ASSERT_LEVELDB_OK(env_->DeleteFilesCreatedAfterLastDirSync()); ASSERT_LEVELDB_OK(env_->RemoveFilesCreatedAfterLastDirSync());
break; break;
default: default:
assert(false); assert(false);

2
db/filename.cc
View File

@ -133,7 +133,7 @@ Status SetCurrentFile(Env* env, const std::string& dbname,
s = env->RenameFile(tmp, CurrentFileName(dbname)); s = env->RenameFile(tmp, CurrentFileName(dbname));
} }
if (!s.ok()) { if (!s.ok()) {
env->DeleteFile(tmp); env->RemoveFile(tmp);
} }
return s; return s;
} }

12
db/recovery_test.cc
View File

@ -100,19 +100,19 @@ class RecoveryTest : public testing::Test {
std::string LogName(uint64_t number) { return LogFileName(dbname_, number); } std::string LogName(uint64_t number) { return LogFileName(dbname_, number); }
size_t DeleteLogFiles() { size_t RemoveLogFiles() {
// Linux allows unlinking open files, but Windows does not. // Linux allows unlinking open files, but Windows does not.
// Closing the db allows for file deletion. // Closing the db allows for file deletion.
Close(); Close();
std::vector<uint64_t> logs = GetFiles(kLogFile); std::vector<uint64_t> logs = GetFiles(kLogFile);
for (size_t i = 0; i < logs.size(); i++) { for (size_t i = 0; i < logs.size(); i++) {
EXPECT_LEVELDB_OK(env_->DeleteFile(LogName(logs[i]))) << LogName(logs[i]); EXPECT_LEVELDB_OK(env_->RemoveFile(LogName(logs[i]))) << LogName(logs[i]);
} }
return logs.size(); return logs.size();
} }
void DeleteManifestFile() { void RemoveManifestFile() {
ASSERT_LEVELDB_OK(env_->DeleteFile(ManifestFileName())); ASSERT_LEVELDB_OK(env_->RemoveFile(ManifestFileName()));
} }
uint64_t FirstLogFile() { return GetFiles(kLogFile)[0]; } uint64_t FirstLogFile() { return GetFiles(kLogFile)[0]; }
@ -212,7 +212,7 @@ TEST_F(RecoveryTest, LargeManifestCompacted) {
TEST_F(RecoveryTest, NoLogFiles) { TEST_F(RecoveryTest, NoLogFiles) {
ASSERT_LEVELDB_OK(Put("foo", "bar")); ASSERT_LEVELDB_OK(Put("foo", "bar"));
ASSERT_EQ(1, DeleteLogFiles()); ASSERT_EQ(1, RemoveLogFiles());
Open(); Open();
ASSERT_EQ("NOT_FOUND", Get("foo")); ASSERT_EQ("NOT_FOUND", Get("foo"));
Open(); Open();
@ -327,7 +327,7 @@ TEST_F(RecoveryTest, MultipleLogFiles) {
TEST_F(RecoveryTest, ManifestMissing) { TEST_F(RecoveryTest, ManifestMissing) {
ASSERT_LEVELDB_OK(Put("foo", "bar")); ASSERT_LEVELDB_OK(Put("foo", "bar"));
Close(); Close();
DeleteManifestFile(); RemoveManifestFile();
Status status = OpenWithStatus(); Status status = OpenWithStatus();
ASSERT_TRUE(status.IsCorruption()); ASSERT_TRUE(status.IsCorruption());

6
db/repair.cc
View File

@ -341,7 +341,7 @@ class Repairer {
} }
} }
if (!s.ok()) { if (!s.ok()) {
env_->DeleteFile(copy); env_->RemoveFile(copy);
} }
} }
@ -386,7 +386,7 @@ class Repairer {
file = nullptr; file = nullptr;
if (!status.ok()) { if (!status.ok()) {
env_->DeleteFile(tmp); env_->RemoveFile(tmp);
} else { } else {
// Discard older manifests // Discard older manifests
for (size_t i = 0; i < manifests_.size(); i++) { for (size_t i = 0; i < manifests_.size(); i++) {
@ -398,7 +398,7 @@ class Repairer {
if (status.ok()) { if (status.ok()) {
status = SetCurrentFile(env_, dbname_, 1); status = SetCurrentFile(env_, dbname_, 1);
} else { } else {
env_->DeleteFile(tmp); env_->RemoveFile(tmp);
} }
} }
return status; return status;

2
db/version_edit.cc
View File

@ -232,7 +232,7 @@ std::string VersionEdit::DebugString() const {
r.append(compact_pointers_[i].second.DebugString()); r.append(compact_pointers_[i].second.DebugString());
} }
for (const auto& deleted_files_kvp : deleted_files_) { for (const auto& deleted_files_kvp : deleted_files_) {
r.append("\n DeleteFile: "); r.append("\n RemoveFile: ");
AppendNumberTo(&r, deleted_files_kvp.first); AppendNumberTo(&r, deleted_files_kvp.first);
r.append(" "); r.append(" ");
AppendNumberTo(&r, deleted_files_kvp.second); AppendNumberTo(&r, deleted_files_kvp.second);

2
db/version_edit.h
View File

@ -71,7 +71,7 @@ class VersionEdit {
} }
// Delete the specified "file" from the specified "level". // Delete the specified "file" from the specified "level".
void DeleteFile(int level, uint64_t file) { void RemoveFile(int level, uint64_t file) {
deleted_files_.insert(std::make_pair(level, file)); deleted_files_.insert(std::make_pair(level, file));
} }

2
db/version_edit_test.cc
View File

@ -27,7 +27,7 @@ TEST(VersionEditTest, EncodeDecode) {
edit.AddFile(3, kBig + 300 + i, kBig + 400 + i, edit.AddFile(3, kBig + 300 + i, kBig + 400 + i,
InternalKey("foo", kBig + 500 + i, kTypeValue), InternalKey("foo", kBig + 500 + i, kTypeValue),
InternalKey("zoo", kBig + 600 + i, kTypeDeletion)); InternalKey("zoo", kBig + 600 + i, kTypeDeletion));
edit.DeleteFile(4, kBig + 700 + i); edit.RemoveFile(4, kBig + 700 + i);
edit.SetCompactPointer(i, InternalKey("x", kBig + 900 + i, kTypeValue)); edit.SetCompactPointer(i, InternalKey("x", kBig + 900 + i, kTypeValue));
} }

4
db/version_set.cc
View File

@ -853,7 +853,7 @@ Status VersionSet::LogAndApply(VersionEdit* edit, port::Mutex* mu) {
delete descriptor_file_; delete descriptor_file_;
descriptor_log_ = nullptr; descriptor_log_ = nullptr;
descriptor_file_ = nullptr; descriptor_file_ = nullptr;
env_->DeleteFile(new_manifest_file); env_->RemoveFile(new_manifest_file);
} }
} }
@ -1502,7 +1502,7 @@ bool Compaction::IsTrivialMove() const {
void Compaction::AddInputDeletions(VersionEdit* edit) { void Compaction::AddInputDeletions(VersionEdit* edit) {
for (int which = 0; which < 2; which++) { for (int which = 0; which < 2; which++) {
for (size_t i = 0; i < inputs_[which].size(); i++) { for (size_t i = 0; i < inputs_[which].size(); i++) {
edit->DeleteFile(level_ + which, inputs_[which][i]->number); edit->RemoveFile(level_ + which, inputs_[which][i]->number);
} }
} }
} }

2
doc/impl.md
View File

@ -166,7 +166,7 @@ So maybe even the sharding is not necessary on modern filesystems?
## Garbage collection of files ## Garbage collection of files
`DeleteObsoleteFiles()` is called at the end of every compaction and at the end `RemoveObsoleteFiles()` is called at the end of every compaction and at the end
of recovery. It finds the names of all files in the database. It deletes all log of recovery. It finds the names of all files in the database. It deletes all log
files that are not the current log file. It deletes all table files that are not files that are not the current log file. It deletes all table files that are not
referenced from some level and are not the output of an active compaction. referenced from some level and are not the output of an active compaction.

10
helpers/memenv/memenv.cc
View File

@ -309,7 +309,7 @@ class InMemoryEnv : public EnvWrapper {
return Status::OK(); return Status::OK();
} }
void DeleteFileInternal(const std::string& fname) void RemoveFileInternal(const std::string& fname)
EXCLUSIVE_LOCKS_REQUIRED(mutex_) { EXCLUSIVE_LOCKS_REQUIRED(mutex_) {
if (file_map_.find(fname) == file_map_.end()) { if (file_map_.find(fname) == file_map_.end()) {
return; return;
@ -319,19 +319,19 @@ class InMemoryEnv : public EnvWrapper {
file_map_.erase(fname); file_map_.erase(fname);
} }
Status DeleteFile(const std::string& fname) override { Status RemoveFile(const std::string& fname) override {
MutexLock lock(&mutex_); MutexLock lock(&mutex_);
if (file_map_.find(fname) == file_map_.end()) { if (file_map_.find(fname) == file_map_.end()) {
return Status::IOError(fname, "File not found"); return Status::IOError(fname, "File not found");
} }
DeleteFileInternal(fname); RemoveFileInternal(fname);
return Status::OK(); return Status::OK();
} }
Status CreateDir(const std::string& dirname) override { return Status::OK(); } Status CreateDir(const std::string& dirname) override { return Status::OK(); }
Status DeleteDir(const std::string& dirname) override { return Status::OK(); } Status RemoveDir(const std::string& dirname) override { return Status::OK(); }
Status GetFileSize(const std::string& fname, uint64_t* file_size) override { Status GetFileSize(const std::string& fname, uint64_t* file_size) override {
MutexLock lock(&mutex_); MutexLock lock(&mutex_);
@ -350,7 +350,7 @@ class InMemoryEnv : public EnvWrapper {
return Status::IOError(src, "File not found"); return Status::IOError(src, "File not found");
} }
DeleteFileInternal(target); RemoveFileInternal(target);
file_map_[target] = file_map_[src]; file_map_[target] = file_map_[src];
file_map_.erase(src); file_map_.erase(src);
return Status::OK(); return Status::OK();

6
helpers/memenv/memenv_test.cc
View File

@ -88,12 +88,12 @@ TEST_F(MemEnvTest, Basics) {
ASSERT_TRUE(!rand_file); ASSERT_TRUE(!rand_file);
// Check that deleting works. // Check that deleting works.
ASSERT_TRUE(!env_->DeleteFile("/dir/non_existent").ok()); ASSERT_TRUE(!env_->RemoveFile("/dir/non_existent").ok());
ASSERT_LEVELDB_OK(env_->DeleteFile("/dir/g")); ASSERT_LEVELDB_OK(env_->RemoveFile("/dir/g"));
ASSERT_TRUE(!env_->FileExists("/dir/g")); ASSERT_TRUE(!env_->FileExists("/dir/g"));
ASSERT_LEVELDB_OK(env_->GetChildren("/dir", &children)); ASSERT_LEVELDB_OK(env_->GetChildren("/dir", &children));
ASSERT_EQ(0, children.size()); ASSERT_EQ(0, children.size());
ASSERT_LEVELDB_OK(env_->DeleteDir("/dir")); ASSERT_LEVELDB_OK(env_->RemoveDir("/dir"));
} }
TEST_F(MemEnvTest, ReadWrite) { TEST_F(MemEnvTest, ReadWrite) {

73
include/leveldb/env.h
View File

@ -22,21 +22,18 @@
#include "leveldb/export.h" #include "leveldb/export.h"
#include "leveldb/status.h" #include "leveldb/status.h"
// This workaround can be removed when leveldb::Env::DeleteFile is removed.
#if defined(_WIN32) #if defined(_WIN32)
// The leveldb::Env class below contains a DeleteFile method. // On Windows, the method name DeleteFile (below) introduces the risk of
// At the same time, <windows.h>, a fairly popular header // triggering undefined behavior by exposing the compiler to different
// file for Windows applications, defines a DeleteFile macro. // declarations of the Env class in different translation units.
// //
// Without any intervention on our part, the result of this // This is because <windows.h>, a fairly popular header file for Windows
// unfortunate coincidence is that the name of the // applications, defines a DeleteFile macro. So, files that include the Windows
// leveldb::Env::DeleteFile method seen by the compiler depends on // header before this header will contain an altered Env declaration.
// whether <windows.h> was included before or after the LevelDB
// headers.
// //
// To avoid headaches, we undefined DeleteFile (if defined) and // This workaround ensures that the compiler sees the same Env declaration,
// redefine it at the bottom of this file. This way <windows.h> // independently of whether <windows.h> was included.
// can be included before this file (or not at all) and the
// exported method will always be leveldb::Env::DeleteFile.
#if defined(DeleteFile) #if defined(DeleteFile)
#undef DeleteFile #undef DeleteFile
#define LEVELDB_DELETEFILE_UNDEFINED #define LEVELDB_DELETEFILE_UNDEFINED
@ -54,7 +51,7 @@ class WritableFile;
class LEVELDB_EXPORT Env { class LEVELDB_EXPORT Env {
public: public:
Env() = default; Env();
Env(const Env&) = delete; Env(const Env&) = delete;
Env& operator=(const Env&) = delete; Env& operator=(const Env&) = delete;
@ -122,15 +119,48 @@ class LEVELDB_EXPORT Env {
// Original contents of *results are dropped. // Original contents of *results are dropped.
virtual Status GetChildren(const std::string& dir, virtual Status GetChildren(const std::string& dir,
std::vector<std::string>* result) = 0; std::vector<std::string>* result) = 0;
// Delete the named file. // Delete the named file.
virtual Status DeleteFile(const std::string& fname) = 0; //
// The default implementation calls DeleteFile, to support legacy Env
// implementations. Updated Env implementations must override RemoveFile and
// ignore the existence of DeleteFile. Updated code calling into the Env API
// must call RemoveFile instead of DeleteFile.
//
// A future release will remove DeleteDir and the default implementation of
// RemoveDir.
virtual Status RemoveFile(const std::string& fname);
// DEPRECATED: Modern Env implementations should override RemoveFile instead.
//
// The default implementation calls RemoveFile, to support legacy Env user
// code that calls this method on modern Env implementations. Modern Env user
// code should call RemoveFile.
//
// A future release will remove this method.
virtual Status DeleteFile(const std::string& fname);
// Create the specified directory. // Create the specified directory.
virtual Status CreateDir(const std::string& dirname) = 0; virtual Status CreateDir(const std::string& dirname) = 0;
// Delete the specified directory. // Delete the specified directory.
virtual Status DeleteDir(const std::string& dirname) = 0; //
// The default implementation calls DeleteDir, to support legacy Env
// implementations. Updated Env implementations must override RemoveDir and
// ignore the existence of DeleteDir. Modern code calling into the Env API
// must call RemoveDir instead of DeleteDir.
//
// A future release will remove DeleteDir and the default implementation of
// RemoveDir.
virtual Status RemoveDir(const std::string& dirname);
// DEPRECATED: Modern Env implementations should override RemoveDir instead.
//
// The default implementation calls RemoveDir, to support legacy Env user
// code that calls this method on modern Env implementations. Modern Env user
// code should call RemoveDir.
//
// A future release will remove this method.
virtual Status DeleteDir(const std::string& dirname);
// Store the size of fname in *file_size. // Store the size of fname in *file_size.
virtual Status GetFileSize(const std::string& fname, uint64_t* file_size) = 0; virtual Status GetFileSize(const std::string& fname, uint64_t* file_size) = 0;
@ -333,14 +363,14 @@ class LEVELDB_EXPORT EnvWrapper : public Env {
std::vector<std::string>* r) override { std::vector<std::string>* r) override {
return target_->GetChildren(dir, r); return target_->GetChildren(dir, r);
} }
Status DeleteFile(const std::string& f) override { Status RemoveFile(const std::string& f) override {
return target_->DeleteFile(f); return target_->RemoveFile(f);
} }
Status CreateDir(const std::string& d) override { Status CreateDir(const std::string& d) override {
return target_->CreateDir(d); return target_->CreateDir(d);
} }
Status DeleteDir(const std::string& d) override { Status RemoveDir(const std::string& d) override {
return target_->DeleteDir(d); return target_->RemoveDir(d);
} }
Status GetFileSize(const std::string& f, uint64_t* s) override { Status GetFileSize(const std::string& f, uint64_t* s) override {
return target_->GetFileSize(f, s); return target_->GetFileSize(f, s);
@ -375,7 +405,8 @@ class LEVELDB_EXPORT EnvWrapper : public Env {
} // namespace leveldb } // namespace leveldb
// Redefine DeleteFile if necessary. // This workaround can be removed when leveldb::Env::DeleteFile is removed.
// Redefine DeleteFile if it was undefined earlier.
#if defined(_WIN32) && defined(LEVELDB_DELETEFILE_UNDEFINED) #if defined(_WIN32) && defined(LEVELDB_DELETEFILE_UNDEFINED)
#if defined(UNICODE) #if defined(UNICODE)
#define DeleteFile DeleteFileW #define DeleteFile DeleteFileW

16
util/env.cc
View File

@ -4,14 +4,28 @@
#include "leveldb/env.h" #include "leveldb/env.h"
// This workaround can be removed when leveldb::Env::DeleteFile is removed.
// See env.h for justification.
#if defined(_WIN32) && defined(LEVELDB_DELETEFILE_UNDEFINED)
#undef DeleteFile
#endif
namespace leveldb { namespace leveldb {
Env::Env() = default;
Env::~Env() = default; Env::~Env() = default;
Status Env::NewAppendableFile(const std::string& fname, WritableFile** result) { Status Env::NewAppendableFile(const std::string& fname, WritableFile** result) {
return Status::NotSupported("NewAppendableFile", fname); return Status::NotSupported("NewAppendableFile", fname);
} }
Status Env::RemoveDir(const std::string& dirname) { return DeleteDir(dirname); }
Status Env::DeleteDir(const std::string& dirname) { return RemoveDir(dirname); }
Status Env::RemoveFile(const std::string& fname) { return DeleteFile(fname); }
Status Env::DeleteFile(const std::string& fname) { return RemoveFile(fname); }
SequentialFile::~SequentialFile() = default; SequentialFile::~SequentialFile() = default;
RandomAccessFile::~RandomAccessFile() = default; RandomAccessFile::~RandomAccessFile() = default;
@ -47,7 +61,7 @@ static Status DoWriteStringToFile(Env* env, const Slice& data,
} }
delete file; // Will auto-close if we did not close above delete file; // Will auto-close if we did not close above
if (!s.ok()) { if (!s.ok()) {
env->DeleteFile(fname); env->RemoveFile(fname);
} }
return s; return s;
} }

4
util/env_posix.cc
View File

@ -587,7 +587,7 @@ class PosixEnv : public Env {
return Status::OK(); return Status::OK();
} }
Status DeleteFile(const std::string& filename) override { Status RemoveFile(const std::string& filename) override {
if (::unlink(filename.c_str()) != 0) { if (::unlink(filename.c_str()) != 0) {
return PosixError(filename, errno); return PosixError(filename, errno);
} }
@ -601,7 +601,7 @@ class PosixEnv : public Env {
return Status::OK(); return Status::OK();
} }
Status DeleteDir(const std::string& dirname) override { Status RemoveDir(const std::string& dirname) override {
if (::rmdir(dirname.c_str()) != 0) { if (::rmdir(dirname.c_str()) != 0) {
return PosixError(dirname, errno); return PosixError(dirname, errno);
} }

14
util/env_posix_test.cc
View File

@ -209,7 +209,7 @@ TEST_F(EnvPosixTest, TestOpenOnRead) {
for (int i = 0; i < kNumFiles; i++) { for (int i = 0; i < kNumFiles; i++) {
delete files[i]; delete files[i];
} }
ASSERT_LEVELDB_OK(env_->DeleteFile(test_file)); ASSERT_LEVELDB_OK(env_->RemoveFile(test_file));
} }
#if HAVE_O_CLOEXEC #if HAVE_O_CLOEXEC
@ -228,7 +228,7 @@ TEST_F(EnvPosixTest, TestCloseOnExecSequentialFile) {
CheckCloseOnExecDoesNotLeakFDs(open_fds); CheckCloseOnExecDoesNotLeakFDs(open_fds);
delete file; delete file;
ASSERT_LEVELDB_OK(env_->DeleteFile(file_path)); ASSERT_LEVELDB_OK(env_->RemoveFile(file_path));
} }
TEST_F(EnvPosixTest, TestCloseOnExecRandomAccessFile) { TEST_F(EnvPosixTest, TestCloseOnExecRandomAccessFile) {
@ -256,7 +256,7 @@ TEST_F(EnvPosixTest, TestCloseOnExecRandomAccessFile) {
for (int i = 0; i < kReadOnlyFileLimit; i++) { for (int i = 0; i < kReadOnlyFileLimit; i++) {
delete mmapped_files[i]; delete mmapped_files[i];
} }
ASSERT_LEVELDB_OK(env_->DeleteFile(file_path)); ASSERT_LEVELDB_OK(env_->RemoveFile(file_path));
} }
TEST_F(EnvPosixTest, TestCloseOnExecWritableFile) { TEST_F(EnvPosixTest, TestCloseOnExecWritableFile) {
@ -273,7 +273,7 @@ TEST_F(EnvPosixTest, TestCloseOnExecWritableFile) {
CheckCloseOnExecDoesNotLeakFDs(open_fds); CheckCloseOnExecDoesNotLeakFDs(open_fds);
delete file; delete file;
ASSERT_LEVELDB_OK(env_->DeleteFile(file_path)); ASSERT_LEVELDB_OK(env_->RemoveFile(file_path));
} }
TEST_F(EnvPosixTest, TestCloseOnExecAppendableFile) { TEST_F(EnvPosixTest, TestCloseOnExecAppendableFile) {
@ -290,7 +290,7 @@ TEST_F(EnvPosixTest, TestCloseOnExecAppendableFile) {
CheckCloseOnExecDoesNotLeakFDs(open_fds); CheckCloseOnExecDoesNotLeakFDs(open_fds);
delete file; delete file;
ASSERT_LEVELDB_OK(env_->DeleteFile(file_path)); ASSERT_LEVELDB_OK(env_->RemoveFile(file_path));
} }
TEST_F(EnvPosixTest, TestCloseOnExecLockFile) { TEST_F(EnvPosixTest, TestCloseOnExecLockFile) {
@ -307,7 +307,7 @@ TEST_F(EnvPosixTest, TestCloseOnExecLockFile) {
CheckCloseOnExecDoesNotLeakFDs(open_fds); CheckCloseOnExecDoesNotLeakFDs(open_fds);
ASSERT_LEVELDB_OK(env_->UnlockFile(lock)); ASSERT_LEVELDB_OK(env_->UnlockFile(lock));
ASSERT_LEVELDB_OK(env_->DeleteFile(file_path)); ASSERT_LEVELDB_OK(env_->RemoveFile(file_path));
} }
TEST_F(EnvPosixTest, TestCloseOnExecLogger) { TEST_F(EnvPosixTest, TestCloseOnExecLogger) {
@ -324,7 +324,7 @@ TEST_F(EnvPosixTest, TestCloseOnExecLogger) {
CheckCloseOnExecDoesNotLeakFDs(open_fds); CheckCloseOnExecDoesNotLeakFDs(open_fds);
delete file; delete file;
ASSERT_LEVELDB_OK(env_->DeleteFile(file_path)); ASSERT_LEVELDB_OK(env_->RemoveFile(file_path));
} }
#endif // HAVE_O_CLOEXEC #endif // HAVE_O_CLOEXEC

8
util/env_test.cc
View File

@ -193,7 +193,7 @@ TEST_F(EnvTest, ReopenWritableFile) {
std::string test_dir; std::string test_dir;
ASSERT_LEVELDB_OK(env_->GetTestDirectory(&test_dir)); ASSERT_LEVELDB_OK(env_->GetTestDirectory(&test_dir));
std::string test_file_name = test_dir + "/reopen_writable_file.txt"; std::string test_file_name = test_dir + "/reopen_writable_file.txt";
env_->DeleteFile(test_file_name); env_->RemoveFile(test_file_name);
WritableFile* writable_file; WritableFile* writable_file;
ASSERT_LEVELDB_OK(env_->NewWritableFile(test_file_name, &writable_file)); ASSERT_LEVELDB_OK(env_->NewWritableFile(test_file_name, &writable_file));
@ -210,14 +210,14 @@ TEST_F(EnvTest, ReopenWritableFile) {
ASSERT_LEVELDB_OK(ReadFileToString(env_, test_file_name, &data)); ASSERT_LEVELDB_OK(ReadFileToString(env_, test_file_name, &data));
ASSERT_EQ(std::string("42"), data); ASSERT_EQ(std::string("42"), data);
env_->DeleteFile(test_file_name); env_->RemoveFile(test_file_name);
} }
TEST_F(EnvTest, ReopenAppendableFile) { TEST_F(EnvTest, ReopenAppendableFile) {
std::string test_dir; std::string test_dir;
ASSERT_LEVELDB_OK(env_->GetTestDirectory(&test_dir)); ASSERT_LEVELDB_OK(env_->GetTestDirectory(&test_dir));
std::string test_file_name = test_dir + "/reopen_appendable_file.txt"; std::string test_file_name = test_dir + "/reopen_appendable_file.txt";
env_->DeleteFile(test_file_name); env_->RemoveFile(test_file_name);
WritableFile* appendable_file; WritableFile* appendable_file;
ASSERT_LEVELDB_OK(env_->NewAppendableFile(test_file_name, &appendable_file)); ASSERT_LEVELDB_OK(env_->NewAppendableFile(test_file_name, &appendable_file));
@ -234,7 +234,7 @@ TEST_F(EnvTest, ReopenAppendableFile) {
ASSERT_LEVELDB_OK(ReadFileToString(env_, test_file_name, &data)); ASSERT_LEVELDB_OK(ReadFileToString(env_, test_file_name, &data));
ASSERT_EQ(std::string("hello world!42"), data); ASSERT_EQ(std::string("hello world!42"), data);
env_->DeleteFile(test_file_name); env_->RemoveFile(test_file_name);
} }
} // namespace leveldb } // namespace leveldb

8
util/env_windows.cc
View File

@ -33,10 +33,6 @@
#include "util/mutexlock.h" #include "util/mutexlock.h"
#include "util/windows_logger.h" #include "util/windows_logger.h"
#if defined(DeleteFile)
#undef DeleteFile
#endif // defined(DeleteFile)
namespace leveldb { namespace leveldb {
namespace { namespace {
@ -505,7 +501,7 @@ class WindowsEnv : public Env {
return Status::OK(); return Status::OK();
} }
Status DeleteFile(const std::string& filename) override { Status RemoveFile(const std::string& filename) override {
if (!::DeleteFileA(filename.c_str())) { if (!::DeleteFileA(filename.c_str())) {
return WindowsError(filename, ::GetLastError()); return WindowsError(filename, ::GetLastError());
} }
@ -519,7 +515,7 @@ class WindowsEnv : public Env {
return Status::OK(); return Status::OK();
} }
Status DeleteDir(const std::string& dirname) override { Status RemoveDir(const std::string& dirname) override {
if (!::RemoveDirectoryA(dirname.c_str())) { if (!::RemoveDirectoryA(dirname.c_str())) {
return WindowsError(dirname, ::GetLastError()); return WindowsError(dirname, ::GetLastError());
} }

2
util/env_windows_test.cc
View File

@ -52,7 +52,7 @@ TEST_F(EnvWindowsTest, TestOpenOnRead) {
for (int i = 0; i < kNumFiles; i++) { for (int i = 0; i < kNumFiles; i++) {
delete files[i]; delete files[i];
} }
ASSERT_LEVELDB_OK(env_->DeleteFile(test_file)); ASSERT_LEVELDB_OK(env_->RemoveFile(test_file));
} }
} // namespace leveldb } // namespace leveldb