commit b80537e266604012bb01a5acb50f14587f30d54d
author Spencer Low <CompareAndSwap@gmail.com> Wed Feb 10 15:03:50 2016 -0800
committer Josh Gao <jmgao@google.com> Mon Feb 22 15:57:08 2016 -0800
tree 07f6546ba25ca0dc1079a5eadb9d52401fb1d504
parent 55c198dafc039c318c2e700c090d690c5dad255f

adb: mkdirs fixes

Fix pathological case where the directory to be created can't be created
because there is already a file there. This was previously returning
success because the wrong var was passed to directory_exists().

Fix test to exercise this situation. Also clarify tests.

Change-Id: I0dc0f14084e0eda4e1498874d4ab2a6445d322ac
Signed-off-by: Spencer Low <CompareAndSwap@gmail.com>
(cherry picked from commit 85c45bd5a185f09f24bb0d790b2038fe72b567a9)

adb/adb_utils.cpp

diff --git a/adb/adb_utils.cpp b/adb/adb_utils.cpp
index 474d1b4..8a16e51 100644
--- a/adb/adb_utils.cpp
+++ b/adb/adb_utils.cpp
@@ -134,7 +134,7 @@
   return result;
 }
 
-// Given a relative or absolute filepath, create the parent directory hierarchy
+// Given a relative or absolute filepath, create the directory hierarchy
 // as needed. Returns true if the hierarchy is/was setup.
 bool mkdirs(const std::string& path) {
   // TODO: all the callers do unlink && mkdirs && adb_creat ---
@@ -157,12 +157,12 @@
     return true;
   }
 
+  const std::string parent(adb_dirname(path));
+
   // If dirname returned the same path as what we passed in, don't go recursive.
   // This can happen on Windows when walking up the directory hierarchy and not
   // finding anything that already exists (unlike POSIX that will eventually
   // find . or /).
-  const std::string parent(adb_dirname(path));
-
   if (parent == path) {
     errno = ENOENT;
     return false;
@@ -174,14 +174,14 @@
   }
 
   // Now that the parent directory hierarchy of 'path' has been ensured,
-  // create parent itself.
+  // create path itself.
   if (adb_mkdir(path, 0775) == -1) {
-    // Can't just check for errno == EEXIST because it might be a file that
-    // exists.
     const int saved_errno = errno;
-    if (directory_exists(parent)) {
+    // If someone else created the directory, that is ok.
+    if (directory_exists(path)) {
       return true;
     }
+    // There might be a pre-existing file at 'path', or there might have been some other error.
     errno = saved_errno;
     return false;
   }

adb/adb_utils_test.cpp

diff --git a/adb/adb_utils_test.cpp b/adb/adb_utils_test.cpp
index dfe6f20..f1ebaa1 100644
--- a/adb/adb_utils_test.cpp
+++ b/adb/adb_utils_test.cpp
@@ -112,20 +112,26 @@
 }
 
 void test_mkdirs(const std::string basepath) {
-  EXPECT_TRUE(mkdirs(adb_dirname(basepath)));
-  EXPECT_NE(-1, adb_creat(basepath.c_str(), 0600));
-  EXPECT_FALSE(mkdirs(basepath + "/subdir/"));
+  // Test creating a directory hierarchy.
+  EXPECT_TRUE(mkdirs(basepath));
+  // Test finding an existing directory hierarchy.
+  EXPECT_TRUE(mkdirs(basepath));
+  const std::string filepath = basepath + "/file";
+  // Verify that the hierarchy was created by trying to create a file in it.
+  EXPECT_NE(-1, adb_creat(filepath.c_str(), 0600));
+  // If a file exists where we want a directory, the operation should fail.
+  EXPECT_FALSE(mkdirs(filepath));
 }
 
 TEST(adb_utils, mkdirs) {
   TemporaryDir td;
 
   // Absolute paths.
-  test_mkdirs(std::string(td.path) + "/dir/subdir/file");
+  test_mkdirs(std::string(td.path) + "/dir/subdir");
 
   // Relative paths.
   ASSERT_EQ(0, chdir(td.path)) << strerror(errno);
-  test_mkdirs(std::string("relative/subrel/file"));
+  test_mkdirs(std::string("relative/subrel"));
 }
 
 #if !defined(_WIN32)