adb: fix adb install and adb push exit code, error handling, unittest
adb push was not returning a bad exit code when write_data_file() or
write_data_link() failed. I encountered this when running the unittest
on Windows which can get into situations where stat() succeeds, but
open() fails due to pre-existing exclusive file access (which typically
doesn't exist on unix).
The same code is used by adb install, so this also fixes its error
handling.
Fixed some fd leaks and propagation of errors when reading a file.
Fixed a unittest to close temp files before reading them.
Change-Id: Ieba0026fa4c79eb0484676e4f2faaac9603ef584
Signed-off-by: Spencer Low <CompareAndSwap@gmail.com>
diff --git a/adb/file_sync_client.cpp b/adb/file_sync_client.cpp
index e70d550..94876ee 100644
--- a/adb/file_sync_client.cpp
+++ b/adb/file_sync_client.cpp
@@ -179,39 +179,41 @@
return sync_start_stat(sc, path) && sync_finish_stat(sc, timestamp, mode, size);
}
-static int write_data_file(SyncConnection& sc, const char* path, syncsendbuf* sbuf, bool show_progress) {
- int err = 0;
+static bool write_data_file(SyncConnection& sc, const char* path, syncsendbuf* sbuf, bool show_progress) {
unsigned long long size = 0;
+ if (show_progress) {
+ // Determine local file size.
+ struct stat st;
+ if (stat(path, &st) == -1) {
+ fprintf(stderr, "cannot stat '%s': %s\n", path, strerror(errno));
+ return false;
+ }
+
+ size = st.st_size;
+ }
int lfd = adb_open(path, O_RDONLY);
if (lfd < 0) {
fprintf(stderr, "cannot open '%s': %s\n", path, strerror(errno));
- return -1;
- }
-
- if (show_progress) {
- // Determine local file size.
- struct stat st;
- if (stat(path, &st)) {
- fprintf(stderr, "cannot stat '%s': %s\n", path, strerror(errno));
- return -1;
- }
-
- size = st.st_size;
+ return false;
}
sbuf->id = ID_DATA;
while (true) {
int ret = adb_read(lfd, sbuf->data, sc.max);
if (ret <= 0) {
- if (ret < 0) fprintf(stderr, "cannot read '%s': %s\n", path, strerror(errno));
+ if (ret < 0) {
+ fprintf(stderr, "cannot read '%s': %s\n", path, strerror(errno));
+ adb_close(lfd);
+ return false;
+ }
break;
}
sbuf->size = ret;
if (!WriteFdExactly(sc.fd, sbuf, sizeof(unsigned) * 2 + ret)) {
- err = -1;
- break;
+ adb_close(lfd);
+ return false;
}
sc.total_bytes += ret;
@@ -221,17 +223,17 @@
}
adb_close(lfd);
- return err;
+ return true;
}
#if defined(_WIN32)
-extern int write_data_link(SyncConnection& sc, const char* path, syncsendbuf* sbuf) __attribute__((error("no symlinks on Windows")));
+extern bool write_data_link(SyncConnection& sc, const char* path, syncsendbuf* sbuf) __attribute__((error("no symlinks on Windows")));
#else
-static int write_data_link(SyncConnection& sc, const char* path, syncsendbuf* sbuf) {
+static bool write_data_link(SyncConnection& sc, const char* path, syncsendbuf* sbuf) {
ssize_t len = readlink(path, sbuf->data, sc.max - 1);
if (len < 0) {
fprintf(stderr, "error reading link '%s': %s\n", path, strerror(errno));
- return -1;
+ return false;
}
sbuf->data[len] = '\0';
@@ -239,12 +241,12 @@
sbuf->id = ID_DATA;
if (!WriteFdExactly(sc.fd, sbuf, sizeof(unsigned) * 2 + len + 1)) {
- return -1;
+ return false;
}
sc.total_bytes += len + 1;
- return 0;
+ return true;
}
#endif
@@ -257,9 +259,9 @@
if (!SendRequest(sc.fd, ID_SEND, path_and_mode.c_str())) goto fail;
if (S_ISREG(mode)) {
- write_data_file(sc, lpath, sbuf, show_progress);
+ if (!write_data_file(sc, lpath, sbuf, show_progress)) return false;
} else if (S_ISLNK(mode)) {
- write_data_link(sc, lpath, sbuf);
+ if (!write_data_link(sc, lpath, sbuf)) return false;
} else {
goto fail;
}
@@ -325,6 +327,7 @@
while (true) {
if(!ReadFdExactly(sc.fd, &msg.data, sizeof(msg.data))) {
+ adb_close(lfd);
return -1;
}
id = msg.data.id;
diff --git a/adb/test_device.py b/adb/test_device.py
index c893ad4..aed70f9 100644
--- a/adb/test_device.py
+++ b/adb/test_device.py
@@ -215,12 +215,18 @@
def test_install_argument_escaping(self):
"""Make sure that install argument escaping works."""
# http://b/20323053
- tf = tempfile.NamedTemporaryFile('wb', suffix='-text;ls;1.apk')
+ tf = tempfile.NamedTemporaryFile('wb', suffix='-text;ls;1.apk',
+ delete=False)
+ tf.close()
self.assertIn("-text;ls;1.apk", self.device.install(tf.name))
+ os.remove(tf.name)
# http://b/3090932
- tf = tempfile.NamedTemporaryFile('wb', suffix="-Live Hold'em.apk")
+ tf = tempfile.NamedTemporaryFile('wb', suffix="-Live Hold'em.apk",
+ delete=False)
+ tf.close()
self.assertIn("-Live Hold'em.apk", self.device.install(tf.name))
+ os.remove(tf.name)
class RootUnrootTest(DeviceTest):