From 35af0433cf863eb056c7bd149d45efd44669fae5 Mon Sep 17 00:00:00 2001
From: Peter Dillinger <peterd@fb.com>
Date: Tue, 6 Apr 2021 23:30:55 -0700
Subject: [PATCH] Fix crash test with backup as read-only DB (#8161)

Summary:
Forgot to re-test crash test after adding read-only filesystem
enforcement to https://github.com/facebook/rocksdb/issues/8142. The problem is ReadOnlyFileSystem would reject
CreateDirIfMissing whenever DBOptions::create_if_missing=true. The fix
that is better for users is to allow CreateDirIfMissing in
ReadOnlyFileSystem if the directory exists, so that they don't cause a
failure on using create_if_missing with opening backups as read-only
DBs. Added this option test to the unit test (in addition to being in the
crash test).

Also fixed a couple of lints.

And some better messaging from 'make format' so that when you run it
with uncommitted changes, it's clear that it's only checking the
uncommitted changes.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/8161

Test Plan: local blackbox_crash_test with amplified backup_one_in

Reviewed By: ajkr

Differential Revision: D27614409

Pulled By: pdillinger

fbshipit-source-id: 63ccb626c7e34c200d61c6bca2a8f60da9015179
---
 build_tools/format-diff.sh                 |  2 ++
 env/fs_readonly.h                          | 17 ++++++++++++-----
 env/fs_remap.h                             |  2 +-
 utilities/backupable/backupable_db_test.cc |  1 +
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/build_tools/format-diff.sh b/build_tools/format-diff.sh
index af15859cf..c2842dfa4 100755
--- a/build_tools/format-diff.sh
+++ b/build_tools/format-diff.sh
@@ -136,9 +136,11 @@ then
   FORMAT_UPSTREAM_MERGE_BASE="$(git merge-base "$FORMAT_UPSTREAM" HEAD)"
   # Get the differences
   diffs=$(git diff -U0 "$FORMAT_UPSTREAM_MERGE_BASE" | $CLANG_FORMAT_DIFF -p 1)
+  echo "Checking format of changes not yet in $FORMAT_UPSTREAM..."
 else
   # Check the format of uncommitted lines,
   diffs=$(git diff -U0 HEAD | $CLANG_FORMAT_DIFF -p 1)
+  echo "Checking format of uncommitted changes..."
 fi
 
 if [ -z "$diffs" ]
diff --git a/env/fs_readonly.h b/env/fs_readonly.h
index 77ea1ded8..89875106e 100644
--- a/env/fs_readonly.h
+++ b/env/fs_readonly.h
@@ -23,7 +23,7 @@ class ReadOnlyFileSystem : public FileSystemWrapper {
   }
 
  public:
-  ReadOnlyFileSystem(const std::shared_ptr<FileSystem>& base)
+  explicit ReadOnlyFileSystem(const std::shared_ptr<FileSystem>& base)
       : FileSystemWrapper(base) {}
 
   IOStatus NewWritableFile(const std::string& /*fname*/,
@@ -61,10 +61,17 @@ class ReadOnlyFileSystem : public FileSystemWrapper {
                      IODebugContext* /*dbg*/) override {
     return FailReadOnly();
   }
-  IOStatus CreateDirIfMissing(const std::string& /*dirname*/,
-                              const IOOptions& /*options*/,
-                              IODebugContext* /*dbg*/) override {
-    return FailReadOnly();
+  IOStatus CreateDirIfMissing(const std::string& dirname,
+                              const IOOptions& options,
+                              IODebugContext* dbg) override {
+    // Allow if dir already exists
+    bool is_dir = false;
+    IOStatus s = IsDirectory(dirname, options, &is_dir, dbg);
+    if (s.ok() && is_dir) {
+      return s;
+    } else {
+      return FailReadOnly();
+    }
   }
   IOStatus DeleteDir(const std::string& /*dirname*/,
                      const IOOptions& /*options*/,
diff --git a/env/fs_remap.h b/env/fs_remap.h
index f95ff9de2..4975822f6 100644
--- a/env/fs_remap.h
+++ b/env/fs_remap.h
@@ -20,7 +20,7 @@ namespace ROCKSDB_NAMESPACE {
 // guarantees.
 class RemapFileSystem : public FileSystemWrapper {
  public:
-  RemapFileSystem(const std::shared_ptr<FileSystem>& base);
+  explicit RemapFileSystem(const std::shared_ptr<FileSystem>& base);
 
  protected:
   // Returns status and mapped-to path in the wrapped filesystem.
diff --git a/utilities/backupable/backupable_db_test.cc b/utilities/backupable/backupable_db_test.cc
index f4e7f3d09..7d24936f5 100644
--- a/utilities/backupable/backupable_db_test.cc
+++ b/utilities/backupable/backupable_db_test.cc
@@ -2660,6 +2660,7 @@ TEST_F(BackupableDBTest, OpenBackupAsReadOnlyDB) {
   backup_engine_->GetBackupInfo(&backup_info, /*with file details*/ true);
   ASSERT_EQ(backup_info.size(), 2);
   CloseBackupEngine();
+  opts.create_if_missing = true;  // check also OK (though pointless)
   opts.env = backup_info[1].env_for_open.get();
   name = backup_info[1].name_for_open;
   // Note: keeping backup_info[1] alive