fix handling of empty string as checkpoint directory

Summary:
- made `CreateCheckpoint` properly return `InvalidArgument` when called with an empty directory. Previously it triggered an assertion failure due to a bug in the logic.
- made `ldb` set empty `checkpoint_dir` if that's what the user specifies, so that we can use it to properly test `CreateCheckpoint` in the future.

Differential Revision: D6874562

fbshipit-source-id: dcc1bd41768261d9338987fa7711444289707ed7

HISTORY.md

@@ -4,6 +4,7 @@
 * Iterator::SeekForPrev is now a pure virtual method. This is to prevent user who implement the Iterator interface fail to implement SeekForPrev by mistake.
 * Add `include_end` option to make the range end exclusive when `include_end == false` in `DeleteFilesInRange()`.
 * Add `CompactRangeOptions::allow_write_stall`, which makes `CompactRange` start working immediately, even if it causes user writes to stall. The default value is false, meaning we add delay to `CompactRange` calls until stalling can be avoided when possible. Note this delay is not present in previous RocksDB versions.
+* Creating checkpoint with empty directory now returns `Status::InvalidArgument`; previously, it returned `Status::IOError`.
 
 ### New Features
 * Improve the performance of iterators doing long range scans by using readahead.

tools/ldb_cmd.cc

@@ -2616,10 +2616,7 @@ CheckPointCommand::CheckPointCommand(
     : LDBCommand(options, flags, false /* is_read_only */,
                  BuildCmdLineOptions({ARG_CHECKPOINT_DIR})) {
   auto itr = options.find(ARG_CHECKPOINT_DIR);
-  if (itr == options.end()) {
-    exec_state_ = LDBCommandExecuteResult::Failed(
-        "--" + ARG_CHECKPOINT_DIR + ": missing checkpoint directory");
-  } else {
+  if (itr != options.end()) {
     checkpoint_dir_ = itr->second;
   }
 }

utilities/checkpoint/checkpoint_impl.cc

@@ -62,9 +62,10 @@ Status CheckpointImpl::CreateCheckpoint(const std::string& checkpoint_dir,
 
   size_t final_nonslash_idx = checkpoint_dir.find_last_not_of('/');
   if (final_nonslash_idx == std::string::npos) {
-    // npos means it's only slashes, which means it's the root directory, but it
-    // shouldn't be because we verified above the directory doesn't exist.
-    assert(false);
+    // npos means it's only slashes or empty. Non-empty means it's the root
+    // directory, but it shouldn't be because we verified above the directory
+    // doesn't exist.
+    assert(checkpoint_dir.empty());
     return Status::InvalidArgument("invalid checkpoint directory name");
   }
 

utilities/checkpoint/checkpoint_test.cc

@@ -564,6 +564,15 @@ TEST_F(CheckpointTest, CurrentFileModifiedWhileCheckpointing2PC) {
   delete txdb;
 }
 
+TEST_F(CheckpointTest, CheckpointInvalidDirectoryName) {
+  for (std::string checkpoint_dir : {"", "/", "////"}) {
+    Checkpoint* checkpoint;
+    ASSERT_OK(Checkpoint::Create(db_, &checkpoint));
+    ASSERT_TRUE(checkpoint->CreateCheckpoint("").IsInvalidArgument());
+    delete checkpoint;
+  }
+}
+
 }  // namespace rocksdb
 
 int main(int argc, char** argv) {