Tidy up the end-of-data handling in backup

* Increase transfer buffer size to 32K
* Add logging about error conditions and fd teardown
* Pass the fd number as a command line option to the 'bu' subprocess
* Properly harvest the 'bu' subprocess after it's done

Change-Id: Id44dde25778ecf43c5604fd9d01d726ba58861e5

3 files changed, 54 insertions, 10 deletions

diff --git a/adb/backup_service.c b/adb/backup_service.c
index 2e6e754..4f0c8ac 100644
--- a/adb/backup_service.c
+++ b/adb/backup_service.c
@@ -22,6 +22,23 @@
 #define TRACE_TAG  TRACE_ADB
 #include "adb.h"
 
+// socketpair but do *not* mark as close_on_exec
+static int backup_socketpair(int sv[2]) {
+    int rc = unix_socketpair( AF_UNIX, SOCK_STREAM, 0, sv );
+    if (rc < 0)
+        return -1;
+
+    return 0;
+}
+
+static void* backup_child_waiter(void* pid_cookie) {
+    int status;
+
+    waitpid((pid_t) pid_cookie, &status, 0);
+    D("harvested backup/restore child, status=%d\n", status);
+    return NULL;
+}
+
 /* returns the data socket passing the backup data here for forwarding */
 int backup_service(BackupOperation op, char* args) {
     pid_t pid;
@@ -42,12 +59,15 @@ int backup_service(BackupOperation op, char* args) {
 
     // set up the pipe from the subprocess to here
     // parent will read s[0]; child will write s[1]
-    if (adb_socketpair(s)) {
+    if (backup_socketpair(s)) {
         D("can't create backup/restore socketpair\n");
         fprintf(stderr, "unable to create backup/restore socketpair\n");
         return -1;
     }
 
+    D("Backup/restore socket pair: (send=%d, receive=%d)\n", s[1], s[0]);
+    close_on_exec(s[0]);    // only the side we hold on to
+
     // spin off the child process to run the backup command
     pid = fork();
     if (pid < 0) {
@@ -61,12 +81,14 @@ int backup_service(BackupOperation op, char* args) {
 
     // Great, we're off and running.
     if (pid == 0) {
+        // child -- actually run the backup here
         char* p;
         int argc;
+        char portnum[16];
         char** bu_args;
 
-        // child -- actually run the backup here
-        argc = 2; // room for the basic 'bu' argv[0] and '[operation]' argv[1]
+        // fixed args:  [0] is 'bu', [1] is the port number, [2] is the 'operation' string
+        argc = 3;
         for (p = (char*)args; p && *p; ) {
             argc++;
             while (*p && *p != ':') p++;
@@ -74,9 +96,13 @@ int backup_service(BackupOperation op, char* args) {
         }
 
         bu_args = (char**) alloca(argc*sizeof(char*) + 1);
-        bu_args[0] = "bu";
-        bu_args[1] = operation;
-        argc = 2;   // run through again to build the argv array
+
+        // run through again to build the argv array
+        argc = 0;
+        bu_args[argc++] = "bu";
+        snprintf(portnum, sizeof(portnum), "%d", s[1]);
+        bu_args[argc++] = portnum;
+        bu_args[argc++] = operation;
         for (p = (char*)args; p && *p; ) {
             bu_args[argc++] = p;
             while (*p && *p != ':') p++;
@@ -90,7 +116,6 @@ int backup_service(BackupOperation op, char* args) {
         // Close the half of the socket that we don't care about, route 'bu's console
         // to the output socket, and off we go
         adb_close(s[0]);
-        dup2(s[1], socketnum);
 
         // off we go
         execvp("/system/bin/bu", (char * const *)bu_args);
@@ -98,8 +123,18 @@ int backup_service(BackupOperation op, char* args) {
         fprintf(stderr, "Unable to exec 'bu', bailing\n");
         exit(-1);
     } else {
+        adb_thread_t t;
+
         // parent, i.e. adbd -- close the sending half of the socket
+        D("fork() returned pid %d\n", pid);
         adb_close(s[1]);
+
+        // spin a thread to harvest the child process
+        if (adb_thread_create(&t, backup_child_waiter, (void*)pid)) {
+            adb_close(s[0]);
+            D("Unable to create child harvester\n");
+            return -1;
+        }
     }
 
     // we'll be reading from s[0] as the data is sent by the child process
diff --git a/adb/commandline.c b/adb/commandline.c
index 2dc86f2..28bae98 100644
--- a/adb/commandline.c
+++ b/adb/commandline.c
@@ -239,18 +239,23 @@ static void read_and_dump(int fd)
 }
 
 static void copy_to_file(int inFd, int outFd) {
-    char buf[4096];
+    const size_t BUFSIZE = 32 * 1024;
+    char* buf = (char*) malloc(BUFSIZE);
     int len;
     long total = 0;
 
     D("copy_to_file(%d -> %d)\n", inFd, outFd);
     for (;;) {
-        len = adb_read(inFd, buf, sizeof(buf));
+        len = adb_read(inFd, buf, BUFSIZE);
         if (len == 0) {
+            D("copy_to_file() : read 0 bytes; exiting\n");
             break;
         }
         if (len < 0) {
-            if (errno == EINTR) continue;
+            if (errno == EINTR) {
+                D("copy_to_file() : EINTR, retrying\n");
+                continue;
+            }
             D("copy_to_file() : error %d\n", errno);
             break;
         }
@@ -258,6 +263,7 @@ static void copy_to_file(int inFd, int outFd) {
         total += len;
     }
     D("copy_to_file() finished after %lu bytes\n", total);
+    free(buf);
 }
 
 static void *stdin_read_thread(void *x)
diff --git a/adb/sockets.c b/adb/sockets.c
index 7219293..80152f2 100644
--- a/adb/sockets.c
+++ b/adb/sockets.c
@@ -288,6 +288,7 @@ static void local_socket_event_func(int fd, unsigned ev, void *_s)
                     if(errno == EAGAIN) return;
                     if(errno == EINTR) continue;
                 }
+                D(" closing after write because r=%d and errno is %d\n", r, errno);
                 s->close(s);
                 return;
             }
@@ -303,6 +304,7 @@ static void local_socket_event_func(int fd, unsigned ev, void *_s)
             ** we can now destroy it.
             */
         if (s->closing) {
+            D(" closing because 'closing' is set after write\n");
             s->close(s);
             return;
         }
@@ -372,6 +374,7 @@ static void local_socket_event_func(int fd, unsigned ev, void *_s)
         }
         /* Don't allow a forced eof if data is still there */
         if((s->fde.force_eof && !r) || is_eof) {
+            D(" closing because is_eof=%d r=%d s->fde.force_eof=%d\n", is_eof, r, s->fde.force_eof);
             s->close(s);
         }
     }