am 0cf93dc3: Merge "Write mkdirs in more idiomatic C++ style."

* commit '0cf93dc345cd5e8131009146a3b2767b1c60f77b':
  Write mkdirs in more idiomatic C++ style.

adb/adb_io_test.cpp

@@ -28,30 +28,7 @@
 #include <string>
 
 #include "base/file.h"
-
+#include "base/test_utils.h"
-class TemporaryFile {
- public:
-  TemporaryFile() {
-    init("/data/local/tmp");
-    if (fd == -1) {
-      init("/tmp");
-    }
-  }
-
-  ~TemporaryFile() {
-    close(fd);
-    unlink(filename);
-  }
-
-  int fd;
-  char filename[1024];
-
- private:
-  void init(const char* tmp_dir) {
-    snprintf(filename, sizeof(filename), "%s/TemporaryFile-XXXXXX", tmp_dir);
-    fd = mkstemp(filename);
-  }
-};
 
 TEST(io, ReadFdExactly_whole) {
   const char expected[] = "Foobar";

adb/adb_utils.cpp

@@ -72,24 +72,13 @@ std::string escape_arg(const std::string& s) {
   return result;
 }
 
-int mkdirs(const std::string& path) {
+bool mkdirs(const std::string& path) {
-    // TODO: rewrite this function and merge it with the *other* mkdirs in adb.
+    for (size_t i = adb_dirstart(path, 1); i != std::string::npos; i = adb_dirstart(path, i + 1)) {
-    std::unique_ptr<char> path_rw(strdup(path.c_str()));
+        if (adb_mkdir(path.substr(0, i), 0775) == -1 && errno != EEXIST) {
-    int ret;
+            return false;
-    char* x = path_rw.get() + 1;
-
-    for(;;) {
-        x = const_cast<char*>(adb_dirstart(x));
-        if(x == 0) return 0;
-        *x = 0;
-        ret = adb_mkdir(path_rw.get(), 0775);
-        *x = OS_PATH_SEPARATOR;
-        if((ret < 0) && (errno != EEXIST)) {
-            return ret;
         }
-        x++;
     }
-    return 0;
+    return true;
 }
 
 void dump_hex(const void* data, size_t byte_count) {

adb/adb_utils.h

@@ -22,7 +22,7 @@
 bool getcwd(std::string* cwd);
 bool directory_exists(const std::string& path);
 
-int mkdirs(const std::string& path);
+bool mkdirs(const std::string& path);
 
 std::string escape_arg(const std::string& s);
 

adb/adb_utils_test.cpp

@@ -18,6 +18,13 @@
 
 #include <gtest/gtest.h>
 
+#include <stdlib.h>
+#include <string.h>
+
+#include "sysdeps.h"
+
+#include <base/test_utils.h>
+
 TEST(adb_utils, directory_exists) {
   ASSERT_TRUE(directory_exists("/proc"));
   ASSERT_FALSE(directory_exists("/proc/self")); // Symbolic link.
@@ -132,3 +139,11 @@ TEST(adb_utils, parse_host_and_port) {
   EXPECT_FALSE(parse_host_and_port("1.2.3.4:0", &canonical_address, &host, &port, &error));
   EXPECT_FALSE(parse_host_and_port("1.2.3.4:65536", &canonical_address, &host, &port, &error));
 }
+
+TEST(adb_utils, mkdirs) {
+  TemporaryDir td;
+  EXPECT_TRUE(mkdirs(std::string(td.path) + "/dir/subdir/file"));
+  std::string file = std::string(td.path) + "/file";
+  adb_creat(file.c_str(), 0600);
+  EXPECT_FALSE(mkdirs(file + "/subdir/"));
+}

adb/commandline.cpp

@@ -859,7 +859,7 @@ static std::string find_product_out_path(const char* hint) {
 
     // If there are any slashes in it, assume it's a relative path;
     // make it absolute.
-    if (adb_dirstart(hint) != nullptr) {
+    if (adb_dirstart(hint) != std::string::npos) {
         std::string cwd;
         if (!getcwd(&cwd)) {
             fprintf(stderr, "adb: getcwd failed: %s\n", strerror(errno));
@@ -1467,15 +1467,15 @@ static int delete_file(TransportType transport, const char* serial, char* filena
     return send_shell_command(transport, serial, cmd);
 }
 
-static const char* get_basename(const char* filename)
+static const char* get_basename(const std::string& filename)
 {
-    const char* basename = adb_dirstop(filename);
+    size_t base = adb_dirstop(filename);
-    if (basename) {
+    if (base != std::string::npos) {
-        basename++;
+        ++base;
-        return basename;
     } else {
-        return filename;
+        base = 0;
     }
+    return filename.c_str() + base;
 }
 
 static int install_app(TransportType transport, const char* serial, int argc, const char** argv) {

adb/file_sync_client.cpp

@@ -746,12 +746,9 @@ int do_sync_push(const char *lpath, const char *rpath, int show_progress)
                 /* if we're copying a local file to a remote directory,
                 ** we *really* want to copy to remotedir + "/" + localfilename
                 */
-            const char *name = adb_dirstop(lpath);
+            size_t slash = adb_dirstop(lpath);
-            if(name == 0) {
+            const char *name = (slash == std::string::npos) ? lpath : lpath + slash + 1;
-                name = lpath;
+
-            } else {
-                name++;
-            }
             int  tmplen = strlen(name) + strlen(rpath) + 2;
             char *tmp = reinterpret_cast<char*>(
                 malloc(strlen(name) + strlen(rpath) + 2));
@@ -960,12 +957,9 @@ int do_sync_pull(const char *rpath, const char *lpath, int show_progress, int co
                     /* if we're copying a remote file to a local directory,
                     ** we *really* want to copy to localdir + "/" + remotefilename
                     */
-                const char *name = adb_dirstop(rpath);
+                size_t slash = adb_dirstop(rpath);
-                if(name == 0) {
+                const char *name = (slash == std::string::npos) ? rpath : rpath + slash + 1;
-                    name = rpath;
+
-                } else {
-                    name++;
-                }
                 int  tmplen = strlen(name) + strlen(lpath) + 2;
                 char *tmp = reinterpret_cast<char*>(malloc(tmplen));
                 if(tmp == 0) return 1;

adb/file_sync_service.cpp

@@ -41,40 +41,31 @@ static bool should_use_fs_config(const char* path) {
            strncmp("/oem/", path, strlen("/oem/")) == 0;
 }
 
-static int mkdirs(char *name)
+static bool secure_mkdirs(const std::string& path) {
-{
-    int ret;
-    char *x = name + 1;
     uid_t uid = -1;
     gid_t gid = -1;
     unsigned int mode = 0775;
     uint64_t cap = 0;
 
-    if(name[0] != '/') return -1;
+    if (path[0] != '/') return false;
 
-    for(;;) {
+    for (size_t i = adb_dirstart(path, 1); i != std::string::npos; i = adb_dirstart(path, i + 1)) {
-        x = const_cast<char*>(adb_dirstart(x));
+        std::string name(path.substr(0, i));
-        if(x == 0) return 0;
+        if (should_use_fs_config(name.c_str())) {
-        *x = 0;
+            fs_config(name.c_str(), 1, &uid, &gid, &mode, &cap);
-        if (should_use_fs_config(name)) {
-            fs_config(name, 1, &uid, &gid, &mode, &cap);
         }
-        ret = adb_mkdir(name, mode);
+        if (adb_mkdir(name.c_str(), mode) == -1) {
-        if((ret < 0) && (errno != EEXIST)) {
+            if (errno != EEXIST) {
-            D("mkdir(\"%s\") -> %s\n", name, strerror(errno));
+                return false;
-            *x = '/';
-            return ret;
-        } else if(ret == 0) {
-            ret = chown(name, uid, gid);
-            if (ret < 0) {
-                *x = '/';
-                return ret;
             }
-            selinux_android_restorecon(name, 0);
+        } else {
+            if (chown(name.c_str(), uid, gid) == -1) {
+                return false;
+            }
+            selinux_android_restorecon(name.c_str(), 0);
         }
-        *x++ = '/';
     }
-    return 0;
+    return true;
 }
 
 static int do_stat(int s, const char *path)
@@ -182,7 +173,7 @@ static int handle_send_file(int s, char *path, uid_t uid,
 
     fd = adb_open_mode(path, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, mode);
     if(fd < 0 && errno == ENOENT) {
-        if(mkdirs(path) != 0) {
+        if (!secure_mkdirs(path)) {
             if(fail_errno(s))
                 return -1;
             fd = -1;
@@ -294,7 +285,7 @@ static int handle_send_link(int s, char *path, char *buffer)
 
     ret = symlink(buffer, path);
     if(ret && errno == ENOENT) {
-        if(mkdirs(path) != 0) {
+        if (!secure_mkdirs(path)) {
             fail_errno(s);
             return -1;
         }

adb/sysdeps.h

@@ -54,6 +54,8 @@
 #include <windows.h>
 #include <ws2tcpip.h>
 
+#include <string>
+
 #include "fdevent.h"
 
 #define OS_PATH_SEPARATOR '\\'
@@ -120,7 +122,7 @@ static __inline__  int    adb_unlink(const char*  path)
 #undef  unlink
 #define unlink  ___xxx_unlink
 
-static __inline__ int  adb_mkdir(const char*  path, int mode)
+static __inline__ int  adb_mkdir(const std::string&  path, int mode)
 {
 	return _mkdir(path);
 }
@@ -234,27 +236,25 @@ static __inline__ void  disable_tcp_nagle( int  fd )
 
 extern int  adb_socketpair( int  sv[2] );
 
-static __inline__  char*  adb_dirstart( const char*  path )
+static __inline__ size_t adb_dirstart(const std::string& path, size_t pos = 0) {
-{
+    size_t p  = path.find('/', pos);
-    char*  p  = strchr(path, '/');
+    size_t p2 = path.find('\\', pos);
-    char*  p2 = strchr(path, '\\');
 
-    if ( !p )
+    if ( p == std::string::npos )
         p = p2;
-    else if ( p2 && p2 > p )
+    else if ( p2 != std::string::npos && p2 > p )
         p = p2;
 
     return p;
 }
 
-static __inline__  const char*  adb_dirstop( const char*  path )
+static __inline__ size_t adb_dirstop(const std::string& path) {
-{
+    size_t p  = path.rfind('/');
-    const char*  p  = strrchr(path, '/');
+    size_t p2 = path.rfind('\\');
-    const char*  p2 = strrchr(path, '\\');
 
-    if ( !p )
+    if ( p == std::string::npos )
         p = p2;
-    else if ( p2 && p2 > p )
+    else if ( p2 != std::string::npos && p2 > p )
         p = p2;
 
     return p;
@@ -284,6 +284,8 @@ static __inline__  int  adb_is_absolute_host_path( const char*  path )
 #include <string.h>
 #include <unistd.h>
 
+#include <string>
+
 #define OS_PATH_SEPARATOR '/'
 #define OS_PATH_SEPARATOR_STR "/"
 #define ENV_PATH_SEPARATOR_STR ":"
@@ -510,10 +512,11 @@ static __inline__ void  adb_sleep_ms( int  mseconds )
     usleep( mseconds*1000 );
 }
 
-static __inline__ int  adb_mkdir(const char*  path, int mode)
+static __inline__ int  adb_mkdir(const std::string& path, int mode)
 {
-    return mkdir(path, mode);
+    return mkdir(path.c_str(), mode);
 }
+
 #undef   mkdir
 #define  mkdir  ___xxx_mkdir
 
@@ -521,18 +524,15 @@ static __inline__ void  adb_sysdeps_init(void)
 {
 }
 
-static __inline__ const char*  adb_dirstart(const char*  path)
+static __inline__ size_t adb_dirstart(const std::string& path, size_t pos = 0) {
-{
+    return path.find('/', pos);
-    return strchr(path, '/');
 }
 
-static __inline__ const char*  adb_dirstop(const char*  path)
+static __inline__ size_t adb_dirstop(const std::string& path) {
-{
+    return path.rfind('/');
-    return strrchr(path, '/');
 }
 
-static __inline__  int  adb_is_absolute_host_path( const char*  path )
+static __inline__ int adb_is_absolute_host_path(const char* path) {
-{
     return path[0] == '/';
 }
 

base/Android.mk

@@ -21,6 +21,7 @@ libbase_src_files := \
     logging.cpp \
     stringprintf.cpp \
     strings.cpp \
+    test_utils.cpp \
 
 libbase_test_src_files := \
     file_test.cpp \
@@ -28,7 +29,6 @@ libbase_test_src_files := \
     stringprintf_test.cpp \
     strings_test.cpp \
     test_main.cpp \
-    test_utils.cpp \
 
 libbase_cppflags := \
     -Wall \

base/file_test.cpp

@@ -24,7 +24,7 @@
 
 #include <string>
 
-#include "test_utils.h"
+#include "base/test_utils.h"
 
 TEST(file, ReadFileToString_ENOENT) {
   std::string s("hello");
@@ -47,10 +47,10 @@ TEST(file, ReadFileToString_success) {
 TEST(file, WriteStringToFile) {
   TemporaryFile tf;
   ASSERT_TRUE(tf.fd != -1);
-  ASSERT_TRUE(android::base::WriteStringToFile("abc", tf.filename))
+  ASSERT_TRUE(android::base::WriteStringToFile("abc", tf.path))
     << strerror(errno);
   std::string s;
-  ASSERT_TRUE(android::base::ReadFileToString(tf.filename, &s))
+  ASSERT_TRUE(android::base::ReadFileToString(tf.path, &s))
     << strerror(errno);
   EXPECT_EQ("abc", s);
 }
@@ -61,16 +61,16 @@ TEST(file, WriteStringToFile) {
 TEST(file, WriteStringToFile2) {
   TemporaryFile tf;
   ASSERT_TRUE(tf.fd != -1);
-  ASSERT_TRUE(android::base::WriteStringToFile("abc", tf.filename, 0660,
+  ASSERT_TRUE(android::base::WriteStringToFile("abc", tf.path, 0660,
                                                getuid(), getgid()))
       << strerror(errno);
   struct stat sb;
-  ASSERT_EQ(0, stat(tf.filename, &sb));
+  ASSERT_EQ(0, stat(tf.path, &sb));
   ASSERT_EQ(0660U, static_cast<unsigned int>(sb.st_mode & ~S_IFMT));
   ASSERT_EQ(getuid(), sb.st_uid);
   ASSERT_EQ(getgid(), sb.st_gid);
   std::string s;
-  ASSERT_TRUE(android::base::ReadFileToString(tf.filename, &s))
+  ASSERT_TRUE(android::base::ReadFileToString(tf.path, &s))
     << strerror(errno);
   EXPECT_EQ("abc", s);
 }
@@ -109,7 +109,7 @@ TEST(file, WriteFully) {
   ASSERT_TRUE(tf.fd != -1);
   ASSERT_TRUE(android::base::WriteFully(tf.fd, "abc", 3));
   std::string s;
-  ASSERT_TRUE(android::base::ReadFileToString(tf.filename, &s))
+  ASSERT_TRUE(android::base::ReadFileToString(tf.path, &s))
     << strerror(errno);
   EXPECT_EQ("abc", s);
 }

base/include/base/test_utils.h

@@ -17,16 +17,37 @@
 #ifndef TEST_UTILS_H
 #define TEST_UTILS_H
 
+#include <string>
+
+#include <base/macros.h>
+
 class TemporaryFile {
  public:
   TemporaryFile();
   ~TemporaryFile();
 
   int fd;
-  char filename[1024];
+  char path[1024];
 
  private:
-  void init(const char* tmp_dir);
+  void init(const std::string& tmp_dir);
+
+  DISALLOW_COPY_AND_ASSIGN(TemporaryFile);
 };
 
+#if !defined(_WIN32)
+class TemporaryDir {
+ public:
+  TemporaryDir();
+  ~TemporaryDir();
+
+  char path[1024];
+
+ private:
+  bool init(const std::string& tmp_dir);
+
+  DISALLOW_COPY_AND_ASSIGN(TemporaryDir);
+};
+#endif
+
 #endif // TEST_UTILS_H

base/logging_test.cpp

@@ -21,7 +21,7 @@
 
 #include "base/file.h"
 #include "base/stringprintf.h"
-#include "test_utils.h"
+#include "base/test_utils.h"
 
 #include <gtest/gtest.h>
 

base/test_utils.cpp

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "test_utils.h"
+#include "base/test_utils.h"
 
 #include <fcntl.h>
 #include <stdio.h>
@@ -26,34 +26,55 @@
 #include <windows.h>
 #endif
 
-TemporaryFile::TemporaryFile() {
+#include <string>
+
+static std::string GetSystemTempDir() {
 #if defined(__ANDROID__)
-  init("/data/local/tmp");
+  return "/data/local/tmp";
 #elif defined(_WIN32)
   char wd[MAX_PATH] = {};
   _getcwd(wd, sizeof(wd));
-  init(wd);
+  return wd;
 #else
-  init("/tmp");
+  return "/tmp";
 #endif
 }
 
+TemporaryFile::TemporaryFile() {
+  init(GetSystemTempDir());
+}
+
 TemporaryFile::~TemporaryFile() {
   close(fd);
-  unlink(filename);
+  unlink(path);
 }
 
-void TemporaryFile::init(const char* tmp_dir) {
+void TemporaryFile::init(const std::string& tmp_dir) {
-  snprintf(filename, sizeof(filename), "%s/TemporaryFile-XXXXXX", tmp_dir);
+  snprintf(path, sizeof(path), "%s/TemporaryFile-XXXXXX", tmp_dir.c_str());
 #if !defined(_WIN32)
-  fd = mkstemp(filename);
+  fd = mkstemp(path);
 #else
   // Windows doesn't have mkstemp, and tmpfile creates the file in the root
   // directory, requiring root (?!) permissions. We have to settle for mktemp.
-  if (mktemp(filename) == nullptr) {
+  if (mktemp(path) == nullptr) {
     abort();
   }
 
-  fd = open(filename, O_RDWR | O_NOINHERIT | O_CREAT, _S_IREAD | _S_IWRITE);
+  fd = open(path, O_RDWR | O_NOINHERIT | O_CREAT, _S_IREAD | _S_IWRITE);
 #endif
 }
+
+#if !defined(_WIN32)
+TemporaryDir::TemporaryDir() {
+  init(GetSystemTempDir());
+}
+
+TemporaryDir::~TemporaryDir() {
+  rmdir(path);
+}
+
+bool TemporaryDir::init(const std::string& tmp_dir) {
+  snprintf(path, sizeof(path), "%s/TemporaryDir-XXXXXX", tmp_dir.c_str());
+  return (mkdtemp(path) != nullptr);
+}
+#endif