From 47d67c96ec991ef1690b4c07188335cbc4bfa2aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Vall=C3=A9e?= Date: Wed, 6 May 2015 16:26:00 -0400 Subject: [PATCH] Write mkdirs in more idiomatic C++ style. ~ Rewrote mkdirs to be in C++ style. ~ Replaced adb_dir{start,stop} with std::string params and (r)find. + Added test for mkdirs. Also make base/test_utils.h public and support temporary directories as well as files. Change-Id: I6fcbdc5e0099f3359d3aac6b00c436f250ca1329 --- adb/adb_io_test.cpp | 25 +-------------- adb/adb_utils.cpp | 21 +++---------- adb/adb_utils.h | 2 +- adb/adb_utils_test.cpp | 15 +++++++++ adb/commandline.cpp | 14 ++++----- adb/file_sync_client.cpp | 18 ++++------- adb/file_sync_service.cpp | 43 ++++++++++---------------- adb/sysdeps.h | 46 ++++++++++++++-------------- base/Android.mk | 2 +- base/file_test.cpp | 14 ++++----- base/{ => include/base}/test_utils.h | 25 +++++++++++++-- base/logging_test.cpp | 2 +- base/test_utils.cpp | 43 +++++++++++++++++++------- 13 files changed, 139 insertions(+), 131 deletions(-) rename base/{ => include/base}/test_utils.h (68%) diff --git a/adb/adb_io_test.cpp b/adb/adb_io_test.cpp index 8fd5cbf38..0ae21db5f 100644 --- a/adb/adb_io_test.cpp +++ b/adb/adb_io_test.cpp @@ -28,30 +28,7 @@ #include #include "base/file.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); - } -}; +#include "base/test_utils.h" TEST(io, ReadFdExactly_whole) { const char expected[] = "Foobar"; diff --git a/adb/adb_utils.cpp b/adb/adb_utils.cpp index 6fa6c2e81..42191c6f3 100644 --- a/adb/adb_utils.cpp +++ b/adb/adb_utils.cpp @@ -72,24 +72,13 @@ std::string escape_arg(const std::string& s) { return result; } -int mkdirs(const std::string& path) { - // TODO: rewrite this function and merge it with the *other* mkdirs in adb. - std::unique_ptr path_rw(strdup(path.c_str())); - int ret; - char* x = path_rw.get() + 1; - - for(;;) { - x = const_cast(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; +bool mkdirs(const std::string& path) { + for (size_t i = adb_dirstart(path, 1); i != std::string::npos; i = adb_dirstart(path, i + 1)) { + if (adb_mkdir(path.substr(0, i), 0775) == -1 && errno != EEXIST) { + return false; } - x++; } - return 0; + return true; } void dump_hex(const void* data, size_t byte_count) { diff --git a/adb/adb_utils.h b/adb/adb_utils.h index 673aaac39..64cbd9d97 100644 --- a/adb/adb_utils.h +++ b/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); diff --git a/adb/adb_utils_test.cpp b/adb/adb_utils_test.cpp index 7aa610ab5..20dba2771 100644 --- a/adb/adb_utils_test.cpp +++ b/adb/adb_utils_test.cpp @@ -18,6 +18,13 @@ #include +#include +#include + +#include "sysdeps.h" + +#include + 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/")); +} diff --git a/adb/commandline.cpp b/adb/commandline.cpp index d54faec58..b0bcc8898 100644 --- a/adb/commandline.cpp +++ b/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); - if (basename) { - basename++; - return basename; + size_t base = adb_dirstop(filename); + if (base != std::string::npos) { + ++base; } else { - return filename; + base = 0; } + return filename.c_str() + base; } static int install_app(TransportType transport, const char* serial, int argc, const char** argv) { diff --git a/adb/file_sync_client.cpp b/adb/file_sync_client.cpp index 49d42a38f..1f4e95d1a 100644 --- a/adb/file_sync_client.cpp +++ b/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); - if(name == 0) { - name = lpath; - } else { - name++; - } + size_t slash = adb_dirstop(lpath); + const char *name = (slash == std::string::npos) ? lpath : lpath + slash + 1; + int tmplen = strlen(name) + strlen(rpath) + 2; char *tmp = reinterpret_cast( 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); - if(name == 0) { - name = rpath; - } else { - name++; - } + size_t slash = adb_dirstop(rpath); + const char *name = (slash == std::string::npos) ? rpath : rpath + slash + 1; + int tmplen = strlen(name) + strlen(lpath) + 2; char *tmp = reinterpret_cast(malloc(tmplen)); if(tmp == 0) return 1; diff --git a/adb/file_sync_service.cpp b/adb/file_sync_service.cpp index 2067836f0..94401f3e1 100644 --- a/adb/file_sync_service.cpp +++ b/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) -{ - int ret; - char *x = name + 1; +static bool secure_mkdirs(const std::string& path) { 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(;;) { - x = const_cast(adb_dirstart(x)); - if(x == 0) return 0; - *x = 0; - if (should_use_fs_config(name)) { - fs_config(name, 1, &uid, &gid, &mode, &cap); + for (size_t i = adb_dirstart(path, 1); i != std::string::npos; i = adb_dirstart(path, i + 1)) { + std::string name(path.substr(0, i)); + if (should_use_fs_config(name.c_str())) { + fs_config(name.c_str(), 1, &uid, &gid, &mode, &cap); } - ret = adb_mkdir(name, mode); - if((ret < 0) && (errno != EEXIST)) { - D("mkdir(\"%s\") -> %s\n", name, strerror(errno)); - *x = '/'; - return ret; - } else if(ret == 0) { - ret = chown(name, uid, gid); - if (ret < 0) { - *x = '/'; - return ret; + if (adb_mkdir(name.c_str(), mode) == -1) { + if (errno != EEXIST) { + return false; } - 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; } diff --git a/adb/sysdeps.h b/adb/sysdeps.h index 729bbcb20..3a3ffda93 100644 --- a/adb/sysdeps.h +++ b/adb/sysdeps.h @@ -54,6 +54,8 @@ #include #include +#include + #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 ) -{ - char* p = strchr(path, '/'); - char* p2 = strchr(path, '\\'); +static __inline__ size_t adb_dirstart(const std::string& path, size_t pos = 0) { + size_t p = path.find('/', pos); + size_t p2 = path.find('\\', pos); - 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 ) -{ - const char* p = strrchr(path, '/'); - const char* p2 = strrchr(path, '\\'); +static __inline__ size_t adb_dirstop(const std::string& path) { + size_t p = path.rfind('/'); + size_t p2 = path.rfind('\\'); - 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 #include +#include + #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) -{ - return strchr(path, '/'); +static __inline__ size_t adb_dirstart(const std::string& path, size_t pos = 0) { + return path.find('/', pos); } -static __inline__ const char* adb_dirstop(const char* path) -{ - return strrchr(path, '/'); +static __inline__ size_t adb_dirstop(const std::string& path) { + return path.rfind('/'); } -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] == '/'; } diff --git a/base/Android.mk b/base/Android.mk index 7bd317b77..4e135f653 100644 --- a/base/Android.mk +++ b/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 \ diff --git a/base/file_test.cpp b/base/file_test.cpp index b138094e3..4056684ef 100644 --- a/base/file_test.cpp +++ b/base/file_test.cpp @@ -24,7 +24,7 @@ #include -#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(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); } diff --git a/base/test_utils.h b/base/include/base/test_utils.h similarity index 68% rename from base/test_utils.h rename to base/include/base/test_utils.h index 132d3a767..83f0f1ceb 100644 --- a/base/test_utils.h +++ b/base/include/base/test_utils.h @@ -17,16 +17,37 @@ #ifndef TEST_UTILS_H #define TEST_UTILS_H +#include + +#include + 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 diff --git a/base/logging_test.cpp b/base/logging_test.cpp index c91857a0a..1a92c9451 100644 --- a/base/logging_test.cpp +++ b/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 diff --git a/base/test_utils.cpp b/base/test_utils.cpp index 0517bc713..dceb8b788 100644 --- a/base/test_utils.cpp +++ b/base/test_utils.cpp @@ -14,7 +14,7 @@ * limitations under the License. */ -#include "test_utils.h" +#include "base/test_utils.h" #include #include @@ -26,34 +26,55 @@ #include #endif -TemporaryFile::TemporaryFile() { +#include + +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) { - snprintf(filename, sizeof(filename), "%s/TemporaryFile-XXXXXX", tmp_dir); +void TemporaryFile::init(const std::string& 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