From a31ea55c555aaa9cddab06c038f6e3285770f664 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 2 Mar 2016 16:11:13 -0800 Subject: [PATCH 1/3] adb: cleanup file skipping logic. Bug: http://b/26355212 Change-Id: Iafa250ce6c5ea8da9f5f00125165e5b67ef1013f --- adb/file_sync_client.cpp | 79 +++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 34 deletions(-) diff --git a/adb/file_sync_client.cpp b/adb/file_sync_client.cpp index cdbff11de..09e92bb6f 100644 --- a/adb/file_sync_client.cpp +++ b/adb/file_sync_client.cpp @@ -60,6 +60,18 @@ static void ensure_trailing_separators(std::string& local_path, std::string& rem } } +static bool should_pull_file(mode_t mode) { + return mode & (S_IFREG | S_IFBLK | S_IFCHR); +} + +static bool should_push_file(mode_t mode) { + mode_t mask = S_IFREG; +#if !defined(_WIN32) + mask |= S_IFLNK; +#endif + return mode & mask; +} + struct copyinfo { std::string lpath; std::string rpath; @@ -483,11 +495,6 @@ static bool sync_send(SyncConnection& sc, const char* lpath, const char* rpath, #endif } - if (!S_ISREG(mode)) { - sc.Error("local file '%s' has unsupported mode: 0o%o", lpath, mode); - return false; - } - struct stat st; if (stat(lpath, &st) == -1) { sc.Error("failed to stat local file '%s': %s", lpath, strerror(errno)); @@ -619,13 +626,12 @@ static bool local_build_list(SyncConnection& sc, std::vector* file_lis if (S_ISDIR(st.st_mode)) { dirlist.push_back(ci); } else { - if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode)) { - sc.Error("skipping special file '%s'", lpath.c_str()); + if (!should_push_file(st.st_mode)) { + sc.Warning("skipping special file '%s' (mode = 0o%o)", lpath.c_str(), st.st_mode); ci.skip = true; - } else { - ci.time = st.st_mtime; - ci.size = st.st_size; } + ci.time = st.st_mtime; + ci.size = st.st_size; file_list->push_back(ci); } } @@ -767,6 +773,9 @@ bool do_sync_push(const std::vector& srcs, const char* dst) { success &= copy_local_dir_remote(sc, src_path, dst_dir.c_str(), false, false); continue; + } else if (!should_push_file(st.st_mode)) { + sc.Warning("skipping special file '%s' (mode = 0o%o)", src_path, st.st_mode); + continue; } std::string path_holder; @@ -819,6 +828,10 @@ static bool remote_build_list(SyncConnection& sc, std::vector* file_li } else if (S_ISLNK(mode)) { linklist.push_back(ci); } else { + if (!should_pull_file(ci.mode)) { + sc.Warning("skipping special file '%s' (mode = 0o%o)", ci.rpath.c_str(), ci.mode); + ci.skip = true; + } ci.time = time; ci.size = size; file_list->push_back(ci); @@ -975,11 +988,6 @@ bool do_sync_pull(const std::vector& srcs, const char* dst, src_isdir = remote_symlink_isdir(sc, src_path); } - if ((src_mode & (S_IFREG | S_IFDIR | S_IFBLK | S_IFCHR)) == 0) { - sc.Error("skipping remote object '%s' (mode = 0o%o)", src_path, src_mode); - continue; - } - if (src_isdir) { std::string dst_dir = dst; @@ -998,27 +1006,30 @@ bool do_sync_pull(const std::vector& srcs, const char* dst, success &= copy_remote_dir_local(sc, src_path, dst_dir.c_str(), copy_attrs); continue; - } else { - std::string path_holder; - if (dst_isdir) { - // If we're copying a remote file to a local directory, we - // really want to copy to local_dir + OS_PATH_SEPARATOR + - // basename(remote). - path_holder = android::base::StringPrintf("%s%c%s", dst_path, OS_PATH_SEPARATOR, - adb_basename(src_path).c_str()); - dst_path = path_holder.c_str(); - } + } else if (!should_pull_file(src_mode)) { + sc.Warning("skipping special file '%s' (mode = 0o%o)", src_path, src_mode); + continue; + } - sc.SetExpectedTotalBytes(src_size); - if (!sync_recv(sc, src_path, dst_path)) { - success = false; - continue; - } + std::string path_holder; + if (dst_isdir) { + // If we're copying a remote file to a local directory, we + // really want to copy to local_dir + OS_PATH_SEPARATOR + + // basename(remote). + path_holder = android::base::StringPrintf("%s%c%s", dst_path, OS_PATH_SEPARATOR, + adb_basename(src_path).c_str()); + dst_path = path_holder.c_str(); + } - if (copy_attrs && set_time_and_mode(dst_path, src_time, src_mode) != 0) { - success = false; - continue; - } + sc.SetExpectedTotalBytes(src_size); + if (!sync_recv(sc, src_path, dst_path)) { + success = false; + continue; + } + + if (copy_attrs && set_time_and_mode(dst_path, src_time, src_mode) != 0) { + success = false; + continue; } } From 89ec3a8d0feac727257b428a91b6f84b6a4b1395 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 2 Mar 2016 16:00:02 -0800 Subject: [PATCH 2/3] adb: mkdir the correct directory name when pulling. The directory name should be based off of the local path, not the remote path. Change-Id: I75b089b8734e9dbf8e466b1e00ea18549fd101bb --- adb/file_sync_client.cpp | 2 +- adb/test_device.py | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/adb/file_sync_client.cpp b/adb/file_sync_client.cpp index 09e92bb6f..6a9e16371 100644 --- a/adb/file_sync_client.cpp +++ b/adb/file_sync_client.cpp @@ -813,7 +813,7 @@ static bool remote_build_list(SyncConnection& sc, std::vector* file_li std::vector linklist; // Add an entry for the current directory to ensure it gets created before pulling its contents. - copyinfo ci(adb_dirname(lpath), adb_dirname(rpath), adb_basename(rpath), S_IFDIR); + copyinfo ci(adb_dirname(lpath), adb_dirname(rpath), adb_basename(lpath), S_IFDIR); file_list->push_back(ci); // Put the files/dirs in rpath on the lists. diff --git a/adb/test_device.py b/adb/test_device.py index 2acc444ca..afd7e0840 100644 --- a/adb/test_device.py +++ b/adb/test_device.py @@ -911,6 +911,30 @@ class FileOperationsTest(DeviceTest): if host_dir is not None: shutil.rmtree(host_dir) + def test_pull_dir_nonexistent(self): + """Pull a directory of files from the device to a nonexistent path.""" + try: + host_dir = tempfile.mkdtemp() + dest_dir = os.path.join(host_dir, 'dest') + + self.device.shell(['rm', '-rf', self.DEVICE_TEMP_DIR]) + self.device.shell(['mkdir', '-p', self.DEVICE_TEMP_DIR]) + + # Populate device directory with random files. + temp_files = make_random_device_files( + self.device, in_dir=self.DEVICE_TEMP_DIR, num_files=32) + + self.device.pull(remote=self.DEVICE_TEMP_DIR, local=dest_dir) + + for temp_file in temp_files: + host_path = os.path.join(dest_dir, temp_file.base_name) + self._verify_local(temp_file.checksum, host_path) + + self.device.shell(['rm', '-rf', self.DEVICE_TEMP_DIR]) + finally: + if host_dir is not None: + shutil.rmtree(host_dir) + def test_pull_symlink_dir(self): """Pull a symlink to a directory of symlinks to files.""" try: From 255c5c80777da296499ce0644defc9337ebda785 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 3 Mar 2016 14:49:02 -0800 Subject: [PATCH 3/3] adb: clean up quotes in test_device.py. Change-Id: I7fe7724578ad89a004665d1bbff0d5c02c34c35e --- adb/test_device.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/adb/test_device.py b/adb/test_device.py index afd7e0840..9dab3ae62 100644 --- a/adb/test_device.py +++ b/adb/test_device.py @@ -216,8 +216,8 @@ class ForwardReverseTest(DeviceTest): """Send data through adb forward and read it back via adb reverse""" forward_port = 12345 reverse_port = forward_port + 1 - forward_spec = "tcp:" + str(forward_port) - reverse_spec = "tcp:" + str(reverse_port) + forward_spec = 'tcp:' + str(forward_port) + reverse_spec = 'tcp:' + str(reverse_port) forward_setup = False reverse_setup = False @@ -739,7 +739,7 @@ class FileOperationsTest(DeviceTest): # Create some random files and a subdirectory containing more files. temp_files = make_random_host_files(in_dir=host_dir, num_files=4) - subdir = os.path.join(host_dir, "subdir") + subdir = os.path.join(host_dir, 'subdir') os.mkdir(subdir) subdir_temp_files = make_random_host_files(in_dir=subdir, num_files=4) @@ -756,7 +756,7 @@ class FileOperationsTest(DeviceTest): for subdir_temp_file in subdir_temp_files: remote_path = posixpath.join(self.DEVICE_TEMP_DIR, # BROKEN: http://b/25394682 - # "subdir", + # 'subdir'; temp_file.base_name) self._verify_remote(temp_file.checksum, remote_path) @@ -777,11 +777,11 @@ class FileOperationsTest(DeviceTest): tmp_file.flush() try: self.device.push(local=tmp_file.name, remote='/system/') - self.fail("push should not have succeeded") + self.fail('push should not have succeeded') except subprocess.CalledProcessError as e: output = e.output - self.assertIn("Permission denied", output) + self.assertIn('Permission denied', output) def _test_pull(self, remote_file, checksum): tmp_write = tempfile.NamedTemporaryFile(mode='wb', delete=False) @@ -850,13 +850,13 @@ class FileOperationsTest(DeviceTest): Bug: http://b/27362811 """ - if os.name != "posix": + if os.name != 'posix': raise unittest.SkipTest('requires POSIX') try: host_dir = tempfile.mkdtemp() - real_dir = os.path.join(host_dir, "dir") - symlink = os.path.join(host_dir, "symlink") + real_dir = os.path.join(host_dir, 'dir') + symlink = os.path.join(host_dir, 'symlink') os.mkdir(real_dir) os.symlink(real_dir, symlink) @@ -882,12 +882,12 @@ class FileOperationsTest(DeviceTest): def test_pull_dir_symlink_collision(self): """Pull a directory into a colliding symlink to directory.""" - if os.name != "posix": + if os.name != 'posix': raise unittest.SkipTest('requires POSIX') try: host_dir = tempfile.mkdtemp() - real_dir = os.path.join(host_dir, "real") + real_dir = os.path.join(host_dir, 'real') tmp_dirname = os.path.basename(self.DEVICE_TEMP_DIR) symlink = os.path.join(host_dir, tmp_dirname) os.mkdir(real_dir) @@ -990,7 +990,7 @@ class FileOperationsTest(DeviceTest): try: host_dir = tempfile.mkdtemp() - subdir = posixpath.join(self.DEVICE_TEMP_DIR, "subdir") + subdir = posixpath.join(self.DEVICE_TEMP_DIR, 'subdir') self.device.shell(['rm', '-rf', self.DEVICE_TEMP_DIR]) self.device.shell(['mkdir', '-p', subdir]) @@ -1011,7 +1011,7 @@ class FileOperationsTest(DeviceTest): for subdir_temp_file in subdir_temp_files: local_path = os.path.join(host_dir, - "subdir", + 'subdir', subdir_temp_file.base_name) self._verify_local(subdir_temp_file.checksum, local_path)