Permalink
Browse files

adb: fix subprocess exit handling, oom adjust fixes, extra debugging.

* Add support for correctly handling subprocess termination in shell service  (b/3400254 b/3482112 b/2249397)
 - have a waitpid() track the subprocess, then notify the fdevent via a socket
 - force an eof on the pty master in fdevent's new subproc handler.
 - modify fdevent to force-read the pty after an exit.
* Migrate the "shell:blabla" handling to "#if !ADB_HOST" sections, where it
 belongs.
* Fix the race around OOM adjusting.
  - Do it in the child before exec() instead of the in the parent as the
   child could already have started or not (no /proc/pid/... yet).
* Allow for multi-threaded D() invocations to not clobber each other.
  - Allow locks across object files.
  - Add lock within D()
  - Make sure sysdesp init (mutex init also) is called early.
* Add some missing close(fd) calls
  - Match similar existing practices near dup2()
* Add extra D() invocations related to FD handling.
* Warn about using debugging as stderr/stdout is used for protocol.
* Fix some errno handling and make D() correctly handle it.
* Add new adb trace_mask: services.
* Make fdevent_loop's handle BADFDs more gracefully (could occur some subproc closed its pts explicitely).
* Remove obsolete commandline args reported in help. (b/3509092)


Change-Id: I928287fdf4f1a86777e22ce105f9581685f46e35
  • Loading branch information...
1 parent cdae7a1 commit 408fa57864c01113deaa213e5c1848a9c594ae92 JP Abgrall committed Mar 16, 2011
Showing with 488 additions and 124 deletions.
  1. +15 −11 adb/adb.c
  2. +33 −14 adb/adb.h
  3. +3 −0 adb/adb_client.c
  4. +9 −11 adb/commandline.c
  5. +215 −26 adb/fdevent.c
  6. +2 −0 adb/fdevent.h
  7. +2 −1 adb/file_sync_client.c
  8. +2 −0 adb/file_sync_service.c
  9. +14 −2 adb/mutex_list.h
  10. +94 −22 adb/services.c
  11. +25 −8 adb/sockets.c
  12. +9 −1 adb/sysdeps.h
  13. +17 −9 adb/transport.c
  14. +26 −0 adb/transport.h
  15. +3 −3 adb/usb_linux.c
  16. +8 −7 adb/usb_linux_client.c
  17. +1 −1 adb/usb_vendors.h
  18. +10 −8 adb/usb_windows.c
View
@@ -36,6 +36,9 @@
#include "usb_vendors.h"
#endif
+#if ADB_TRACE
+ADB_MUTEX_DEFINE( D_lock );
+#endif
int HOST = 0;
@@ -90,6 +93,7 @@ void adb_trace_init(void)
{ "sysdeps", TRACE_SYSDEPS },
{ "transport", TRACE_TRANSPORT },
{ "jdwp", TRACE_JDWP },
+ { "services", TRACE_SERVICES },
{ NULL, 0 }
};
@@ -591,14 +595,6 @@ static int install_listener(const char *local_name, const char *connect_to, atra
return 0;
}
-#ifdef HAVE_FORKEXEC
-static void sigchld_handler(int n)
-{
- int status;
- while(waitpid(-1, &status, WNOHANG) > 0) ;
-}
-#endif
-
#ifdef HAVE_WIN32_PROC
static BOOL WINAPI ctrlc_handler(DWORD type)
{
@@ -641,13 +637,15 @@ void start_logging(void)
fd = unix_open("/dev/null", O_RDONLY);
dup2(fd, 0);
+ adb_close(fd);
fd = unix_open("/tmp/adb.log", O_WRONLY | O_CREAT | O_APPEND, 0640);
if(fd < 0) {
fd = unix_open("/dev/null", O_WRONLY);
}
dup2(fd, 1);
dup2(fd, 2);
+ adb_close(fd);
fprintf(stderr,"--- adb starting (pid %d) ---\n", getpid());
#endif
}
@@ -807,9 +805,10 @@ int launch_server(int server_port)
// wait for the "OK\n" message
adb_close(fd[1]);
int ret = adb_read(fd[0], temp, 3);
+ int saved_errno = errno;
adb_close(fd[0]);
if (ret < 0) {
- fprintf(stderr, "could not read ok from ADB Server, errno = %d\n", errno);
+ fprintf(stderr, "could not read ok from ADB Server, errno = %d\n", saved_errno);
return -1;
}
if (ret != 3 || temp[0] != 'O' || temp[1] != 'K' || temp[2] != '\n') {
@@ -848,7 +847,7 @@ int adb_main(int is_daemon, int server_port)
#ifdef HAVE_WIN32_PROC
SetConsoleCtrlHandler( ctrlc_handler, TRUE );
#elif defined(HAVE_FORKEXEC)
- signal(SIGCHLD, sigchld_handler);
+ // No SIGCHLD. Let the service subproc handle its children.
signal(SIGPIPE, SIG_IGN);
#endif
@@ -957,7 +956,9 @@ int adb_main(int is_daemon, int server_port)
// listen on default port
local_init(DEFAULT_ADB_LOCAL_TRANSPORT_PORT);
}
+ D("adb_main(): pre init_jdwp()\n");
init_jdwp();
+ D("adb_main(): post init_jdwp()\n");
#endif
if (is_daemon)
@@ -971,6 +972,7 @@ int adb_main(int is_daemon, int server_port)
#endif
start_logging();
}
+ D("Event loop starting\n");
fdevent_loop();
@@ -1269,8 +1271,9 @@ int recovery_mode = 0;
int main(int argc, char **argv)
{
#if ADB_HOST
- adb_trace_init();
adb_sysdeps_init();
+ adb_trace_init();
+ D("Handling commandline()\n");
return adb_commandline(argc - 1, argv + 1);
#else
if((argc > 1) && (!strcmp(argv[1],"recovery"))) {
@@ -1279,6 +1282,7 @@ int main(int argc, char **argv)
}
start_device_log();
+ D("Handling main()\n");
return adb_main(0, DEFAULT_ADB_PORT);
#endif
}
View
@@ -19,6 +19,8 @@
#include <limits.h>
+#include "transport.h" /* readx(), writex() */
+
#define MAX_PAYLOAD 4096
#define A_SYNC 0x434e5953
@@ -315,13 +317,6 @@ void put_apacket(apacket *p);
int check_header(apacket *p);
int check_data(apacket *p);
-/* convenience wrappers around read/write that will retry on
-** EINTR and/or short read/write. Returns 0 on success, -1
-** on error or EOF.
-*/
-int readx(int fd, void *ptr, size_t len);
-int writex(int fd, const void *ptr, size_t len);
-
/* define ADB_TRACE to 1 to enable tracing support, or 0 to disable it */
#define ADB_TRACE 1
@@ -331,33 +326,56 @@ int writex(int fd, const void *ptr, size_t len);
* the adb_trace_init() function implemented in adb.c
*/
typedef enum {
- TRACE_ADB = 0,
+ TRACE_ADB = 0, /* 0x001 */
TRACE_SOCKETS,
TRACE_PACKETS,
TRACE_TRANSPORT,
- TRACE_RWX,
+ TRACE_RWX, /* 0x010 */
TRACE_USB,
TRACE_SYNC,
TRACE_SYSDEPS,
- TRACE_JDWP,
+ TRACE_JDWP, /* 0x100 */
+ TRACE_SERVICES,
} AdbTrace;
#if ADB_TRACE
- int adb_trace_mask;
-
+ extern int adb_trace_mask;
+ extern unsigned char adb_trace_output_count;
void adb_trace_init(void);
# define ADB_TRACING ((adb_trace_mask & (1 << TRACE_TAG)) != 0)
/* you must define TRACE_TAG before using this macro */
- #define D(...) \
+# define D(...) \
+ do { \
+ if (ADB_TRACING) { \
+ int save_errno = errno; \
+ adb_mutex_lock(&D_lock); \
+ fprintf(stderr, "%s::%s():", \
+ __FILE__, __FUNCTION__); \
+ errno = save_errno; \
+ fprintf(stderr, __VA_ARGS__ ); \
+ fflush(stderr); \
+ adb_mutex_unlock(&D_lock); \
+ errno = save_errno; \
+ } \
+ } while (0)
+# define DR(...) \
do { \
- if (ADB_TRACING) \
+ if (ADB_TRACING) { \
+ int save_errno = errno; \
+ adb_mutex_lock(&D_lock); \
+ errno = save_errno; \
fprintf(stderr, __VA_ARGS__ ); \
+ fflush(stderr); \
+ adb_mutex_unlock(&D_lock); \
+ errno = save_errno; \
+ } \
} while (0)
#else
# define D(...) ((void)0)
+# define DR(...) ((void)0)
# define ADB_TRACING 0
#endif
@@ -413,6 +431,7 @@ int connection_state(atransport *t);
#define CS_NOPERM 5 /* Insufficient permissions to communicate with the device */
extern int HOST;
+extern int SHELL_EXIT_NOTIFY_FD;
#define CHUNK_SIZE (64*1024)
View
@@ -202,6 +202,7 @@ int _adb_connect(const char *service)
return -1;
}
+ D("_adb_connect: return fd %d\n", fd);
return fd;
}
@@ -210,6 +211,7 @@ int adb_connect(const char *service)
// first query the adb server's version
int fd = _adb_connect("host:version");
+ D("adb_connect: service %s\n", service);
if(fd == -2) {
fprintf(stdout,"* daemon not running. starting it now on port %d *\n",
__adb_server_port);
@@ -266,6 +268,7 @@ int adb_connect(const char *service)
if(fd == -2) {
fprintf(stderr,"** daemon still not running");
}
+ D("adb_connect: return fd %d\n", fd);
return fd;
error:
View
@@ -37,12 +37,6 @@
#include "adb_client.h"
#include "file_sync_service.h"
-enum {
- IGNORE_DATA,
- WIPE_DATA,
- FLASH_DATA
-};
-
static int do_cmd(transport_type ttype, char* serial, char *cmd, ...);
void get_my_path(char *s, size_t maxLen);
@@ -138,11 +132,6 @@ void help()
" adb help - show this help message\n"
" adb version - show version num\n"
"\n"
- "DATAOPTS:\n"
- " (no option) - don't touch the data partition\n"
- " -w - wipe the data partition\n"
- " -d - flash the data partition\n"
- "\n"
"scripting:\n"
" adb wait-for-device - block until device is online\n"
" adb start-server - ensure that there is a server running\n"
@@ -218,7 +207,9 @@ static void read_and_dump(int fd)
int len;
while(fd >= 0) {
+ D("read_and_dump(): pre adb_read(fd=%d)\n", fd);
len = adb_read(fd, buf, 4096);
+ D("read_and_dump(): post adb_read(fd=%d): len=%d\n", fd, len);
if(len == 0) {
break;
}
@@ -246,7 +237,9 @@ static void *stdin_read_thread(void *x)
for(;;) {
/* fdi is really the client's stdin, so use read, not adb_read here */
+ D("stdin_read_thread(): pre unix_read(fdi=%d,...)\n", fdi);
r = unix_read(fdi, buf, 1024);
+ D("stdin_read_thread(): post unix_read(fdi=%d,...)\n", fdi);
if(r == 0) break;
if(r < 0) {
if(errno == EINTR) continue;
@@ -853,6 +846,7 @@ int adb_commandline(int argc, char **argv)
}
if(argc < 2) {
+ D("starting interactive shell\n");
r = interactive_shell();
if (h) {
printf("\x1b[0m");
@@ -877,9 +871,12 @@ int adb_commandline(int argc, char **argv)
}
for(;;) {
+ D("interactive shell loop. buff=%s\n", buf);
fd = adb_connect(buf);
if(fd >= 0) {
+ D("about to read_and_dump(fd=%d)\n", fd);
read_and_dump(fd);
+ D("read_and_dump() done.\n");
adb_close(fd);
r = 0;
} else {
@@ -896,6 +893,7 @@ int adb_commandline(int argc, char **argv)
printf("\x1b[0m");
fflush(stdout);
}
+ D("interactive shell loop. return r=%d\n", r);
return r;
}
}
Oops, something went wrong.

0 comments on commit 408fa57

Please sign in to comment.