Merge "Move "adb shell" over to getopt(3), and allow -tt on old devices."
am: 620469a4ac
Change-Id: I4849d18fcd2afe0c1e5d3b2bdf3a3a755e557e91
diff --git a/adb/commandline.cpp b/adb/commandline.cpp
index 54e254a..fe5d280 100644
--- a/adb/commandline.cpp
+++ b/adb/commandline.cpp
@@ -686,100 +686,100 @@
static int adb_shell(int argc, const char** argv) {
FeatureSet features;
std::string error;
-
if (!adb_get_feature_set(&features, &error)) {
fprintf(stderr, "error: %s\n", error.c_str());
return 1;
}
- bool use_shell_protocol = CanUseFeature(features, kFeatureShell2);
- if (!use_shell_protocol) {
- D("shell protocol not supported, using raw data transfer");
- } else {
- D("using shell protocol");
- }
+ enum PtyAllocationMode { kPtyAuto, kPtyNo, kPtyYes, kPtyDefinitely };
+
+ // Defaults.
+ char escape_char = '~'; // -e
+ bool use_shell_protocol = CanUseFeature(features, kFeatureShell2); // -x
+ PtyAllocationMode tty = use_shell_protocol ? kPtyAuto : kPtyDefinitely; // -t/-T
// Parse shell-specific command-line options.
- // argv[0] is always "shell".
- --argc;
- ++argv;
- int t_arg_count = 0;
- char escape_char = '~';
- while (argc) {
- if (!strcmp(argv[0], "-e")) {
- if (argc < 2 || !(strlen(argv[1]) == 1 || strcmp(argv[1], "none") == 0)) {
- fprintf(stderr, "error: -e requires a single-character argument or 'none'\n");
+ argv[0] = "adb shell"; // So getopt(3) error messages start "adb shell".
+ optind = 1; // argv[0] is always "shell", so set `optind` appropriately.
+ int opt;
+ while ((opt = getopt(argc, const_cast<char**>(argv), "+e:ntTx")) != -1) {
+ switch (opt) {
+ case 'e':
+ if (!(strlen(optarg) == 1 || strcmp(optarg, "none") == 0)) {
+ fprintf(stderr, "error: -e requires a single-character argument or 'none'\n");
+ return 1;
+ }
+ escape_char = (strcmp(optarg, "none") == 0) ? 0 : optarg[0];
+ break;
+ case 'n':
+ close_stdin();
+ break;
+ case 'x':
+ // This option basically asks for historical behavior, so set options that
+ // correspond to the historical defaults. This is slightly weird in that -Tx
+ // is fine (because we'll undo the -T) but -xT isn't, but that does seem to
+ // be our least worst choice...
+ use_shell_protocol = false;
+ tty = kPtyDefinitely;
+ escape_char = '~';
+ break;
+ case 't':
+ // Like ssh, -t arguments are cumulative so that multiple -t's
+ // are needed to force a PTY.
+ tty = (tty >= kPtyYes) ? kPtyDefinitely : kPtyYes;
+ break;
+ case 'T':
+ tty = kPtyNo;
+ break;
+ default:
+ // getopt(3) already printed an error message for us.
return 1;
- }
- escape_char = (strcmp(argv[1], "none") == 0) ? 0 : argv[1][0];
- argc -= 2;
- argv += 2;
- } else if (!strcmp(argv[0], "-T") || !strcmp(argv[0], "-t")) {
- // Like ssh, -t arguments are cumulative so that multiple -t's
- // are needed to force a PTY.
- if (argv[0][1] == 't') {
- ++t_arg_count;
- } else {
- t_arg_count = -1;
- }
- --argc;
- ++argv;
- } else if (!strcmp(argv[0], "-x")) {
- use_shell_protocol = false;
- --argc;
- ++argv;
- } else if (!strcmp(argv[0], "-n")) {
- close_stdin();
-
- --argc;
- ++argv;
- } else {
- break;
}
}
- // Legacy shell protocol requires a remote PTY to close the subprocess properly which creates
- // some weird interactions with -tT.
- if (!use_shell_protocol && t_arg_count != 0) {
- if (!CanUseFeature(features, kFeatureShell2)) {
- fprintf(stderr, "error: target doesn't support PTY args -Tt\n");
- } else {
- fprintf(stderr, "error: PTY args -Tt cannot be used with -x\n");
- }
- return 1;
- }
+ bool is_interactive = (optind == argc);
- std::string shell_type_arg;
- if (CanUseFeature(features, kFeatureShell2)) {
- if (t_arg_count < 0) {
+ std::string shell_type_arg = kShellServiceArgPty;
+ if (tty == kPtyNo) {
+ shell_type_arg = kShellServiceArgRaw;
+ } else if (tty == kPtyAuto) {
+ // If stdin isn't a TTY, default to a raw shell; this lets
+ // things like `adb shell < my_script.sh` work as expected.
+ // Non-interactive shells should also not have a pty.
+ if (!unix_isatty(STDIN_FILENO) || !is_interactive) {
shell_type_arg = kShellServiceArgRaw;
- } else if (t_arg_count == 0) {
- // If stdin isn't a TTY, default to a raw shell; this lets
- // things like `adb shell < my_script.sh` work as expected.
- // Otherwise leave |shell_type_arg| blank which uses PTY for
- // interactive shells and raw for non-interactive.
- if (!unix_isatty(STDIN_FILENO)) {
- shell_type_arg = kShellServiceArgRaw;
- }
- } else if (t_arg_count == 1) {
- // A single -t arg isn't enough to override implicit -T.
- if (!unix_isatty(STDIN_FILENO)) {
- fprintf(stderr,
- "Remote PTY will not be allocated because stdin is not a terminal.\n"
- "Use multiple -t options to force remote PTY allocation.\n");
- shell_type_arg = kShellServiceArgRaw;
- } else {
- shell_type_arg = kShellServiceArgPty;
- }
+ }
+ } else if (tty == kPtyYes) {
+ // A single -t arg isn't enough to override implicit -T.
+ if (!unix_isatty(STDIN_FILENO)) {
+ fprintf(stderr,
+ "Remote PTY will not be allocated because stdin is not a terminal.\n"
+ "Use multiple -t options to force remote PTY allocation.\n");
+ shell_type_arg = kShellServiceArgRaw;
+ }
+ }
+
+ D("shell -e 0x%x t=%d use_shell_protocol=%s shell_type_arg=%s\n",
+ escape_char, tty,
+ use_shell_protocol ? "true" : "false",
+ (shell_type_arg == kShellServiceArgPty) ? "pty" : "raw");
+
+ // Raw mode is only supported when talking to a new device *and* using the shell protocol.
+ if (!use_shell_protocol) {
+ if (shell_type_arg != kShellServiceArgPty) {
+ fprintf(stderr, "error: %s only supports allocating a pty\n",
+ !CanUseFeature(features, kFeatureShell2) ? "device" : "-x");
+ return 1;
} else {
- shell_type_arg = kShellServiceArgPty;
+ // If we're not using the shell protocol, the type argument must be empty.
+ shell_type_arg = "";
}
}
std::string command;
- if (argc) {
+ if (optind < argc) {
// We don't escape here, just like ssh(1). http://b/20564385.
- command = android::base::Join(std::vector<const char*>(argv, argv + argc), ' ');
+ command = android::base::Join(std::vector<const char*>(argv + optind, argv + argc), ' ');
}
return RemoteShell(use_shell_protocol, shell_type_arg, escape_char, command);
@@ -1401,7 +1401,7 @@
return !CanUseFeature(features, kFeatureCmd);
}
-int adb_commandline(int argc, const char **argv) {
+int adb_commandline(int argc, const char** argv) {
int no_daemon = 0;
int is_daemon = 0;
int is_server = 0;
diff --git a/adb/test_device.py b/adb/test_device.py
index b12bf88..4c5563f 100644
--- a/adb/test_device.py
+++ b/adb/test_device.py
@@ -371,15 +371,8 @@
def test_pty_logic(self):
"""Tests that a PTY is allocated when it should be.
- PTY allocation behavior should match ssh; some behavior requires
- a terminal stdin to test so this test will be skipped if stdin
- is not a terminal.
+ PTY allocation behavior should match ssh.
"""
- if not self.device.has_shell_protocol():
- raise unittest.SkipTest('PTY arguments unsupported on this device')
- if not os.isatty(sys.stdin.fileno()):
- raise unittest.SkipTest('PTY tests require stdin terminal')
-
def check_pty(args):
"""Checks adb shell PTY allocation.
@@ -409,16 +402,34 @@
# -T: never allocate PTY.
self.assertEqual((False, False), check_pty(['-T']))
- # No args: PTY only if stdin is a terminal and shell is interactive,
- # which is difficult to reliably test from a script.
- self.assertEqual((False, False), check_pty([]))
+ # These tests require a new device.
+ if self.device.has_shell_protocol() and os.isatty(sys.stdin.fileno()):
+ # No args: PTY only if stdin is a terminal and shell is interactive,
+ # which is difficult to reliably test from a script.
+ self.assertEqual((False, False), check_pty([]))
- # -t: PTY if stdin is a terminal.
- self.assertEqual((True, False), check_pty(['-t']))
+ # -t: PTY if stdin is a terminal.
+ self.assertEqual((True, False), check_pty(['-t']))
# -t -t: always allocate PTY.
self.assertEqual((True, True), check_pty(['-t', '-t']))
+ # -tt: always allocate PTY, POSIX style (http://b/32216152).
+ self.assertEqual((True, True), check_pty(['-tt']))
+
+ # -ttt: ssh has weird even/odd behavior with multiple -t flags, but
+ # we follow the man page instead.
+ self.assertEqual((True, True), check_pty(['-ttt']))
+
+ # -ttx: -x and -tt aren't incompatible (though -Tx would be an error).
+ self.assertEqual((True, True), check_pty(['-ttx']))
+
+ # -Ttt: -tt cancels out -T.
+ self.assertEqual((True, True), check_pty(['-Ttt']))
+
+ # -ttT: -T cancels out -tt.
+ self.assertEqual((False, False), check_pty(['-ttT']))
+
def test_shell_protocol(self):
"""Tests the shell protocol on the device.