am fd18d9e2: Merge "adb: put legacy shell: service back in."

* commit 'fd18d9e254874557aa44d42bd6d6bdf4352b0e36':
  adb: put legacy shell: service back in.

adb/adb.h

@@ -50,7 +50,7 @@ constexpr size_t MAX_PAYLOAD = MAX_PAYLOAD_V2;
 std::string adb_version();
 
 // Increment this when we want to force users to start a new adb server.
-#define ADB_SERVER_VERSION 33
+#define ADB_SERVER_VERSION 34
 
 class atransport;
 struct usb_handle;

adb/commandline.cpp

@@ -49,6 +49,7 @@
 #include "adb_io.h"
 #include "adb_utils.h"
 #include "file_sync_service.h"
+#include "services.h"
 #include "shell_service.h"
 #include "transport.h"
 
@@ -108,10 +109,11 @@ static void help() {
         "                                 ('-a' means copy timestamp and mode)\n"
         "  adb sync [ <directory> ]     - copy host->device only if changed\n"
         "                                 (-l means list but don't copy)\n"
-        "  adb shell                    - run remote shell interactively\n"
+        "  adb shell [-Ttx]             - run remote shell interactively\n"
-        "  adb shell [-Tt] <command>    - run remote shell command\n"
+        "  adb shell [-Ttx] <command>   - run remote shell command\n"
         "                                 (-T disables PTY allocation)\n"
         "                                 (-t forces PTY allocation)\n"
+        "                                 (-x disables remote exit codes and stdout/stderr separation)\n"
         "  adb emu <command>            - run emulator console command\n"
         "  adb logcat [ <filter-spec> ] - View device log\n"
         "  adb forward --list           - list all forward socket connections.\n"
@@ -793,12 +795,45 @@ static bool wait_for_device(const char* service, TransportType t, const char* se
     return adb_command(cmd);
 }
 
+// Returns a shell service string with the indicated arguments and command.
+static std::string ShellServiceString(bool use_shell_protocol,
+                                      const std::string& type_arg,
+                                      const std::string& command) {
+    std::vector<std::string> args;
+    if (use_shell_protocol) {
+        args.push_back(kShellServiceArgShellProtocol);
+    }
+    if (!type_arg.empty()) {
+        args.push_back(type_arg);
+    }
+
+    // Shell service string can look like: shell[,arg1,arg2,...]:[command].
+    return android::base::StringPrintf("shell%s%s:%s",
+                                       args.empty() ? "" : ",",
+                                       android::base::Join(args, ',').c_str(),
+                                       command.c_str());
+}
+
+// Connects to the device "shell" service with |command| and prints the
+// resulting output.
 static int send_shell_command(TransportType transport_type, const char* serial,
-                              const std::string& command) {
+                              const std::string& command,
+                              bool disable_shell_protocol) {
+    // Only use shell protocol if it's supported and the caller doesn't want
+    // to explicitly disable it.
+    bool use_shell_protocol = false;
+    if (!disable_shell_protocol) {
+        FeatureSet features = GetFeatureSet(transport_type, serial);
+        use_shell_protocol = CanUseFeature(features, kFeatureShell2);
+    }
+
+    std::string service_string = ShellServiceString(use_shell_protocol, "",
+                                                    command);
+
     int fd;
     while (true) {
         std::string error;
-        fd = adb_connect(command, &error);
+        fd = adb_connect(service_string, &error);
         if (fd >= 0) {
             break;
         }
@@ -807,8 +842,7 @@ static int send_shell_command(TransportType transport_type, const char* serial,
         wait_for_device("wait-for-device", transport_type, serial);
     }
 
-    FeatureSet features = GetFeatureSet(transport_type, serial);
+    int exit_code = read_and_dump(fd, use_shell_protocol);
-    int exit_code = read_and_dump(fd, features.count(kFeatureShell2) > 0);
 
     if (adb_close(fd) < 0) {
         PLOG(ERROR) << "failure closing FD " << fd;
@@ -821,7 +855,7 @@ static int logcat(TransportType transport, const char* serial, int argc, const c
     char* log_tags = getenv("ANDROID_LOG_TAGS");
     std::string quoted = escape_arg(log_tags == nullptr ? "" : log_tags);
 
-    std::string cmd = "shell:export ANDROID_LOG_TAGS=\"" + quoted + "\"; exec logcat";
+    std::string cmd = "export ANDROID_LOG_TAGS=\"" + quoted + "\"; exec logcat";
 
     if (!strcmp(argv[0], "longcat")) {
         cmd += " -v long";
@@ -833,7 +867,8 @@ static int logcat(TransportType transport, const char* serial, int argc, const c
         cmd += " " + escape_arg(*argv++);
     }
 
-    return send_shell_command(transport, serial, cmd);
+    // No need for shell protocol with logcat, always disable for simplicity.
+    return send_shell_command(transport, serial, cmd, true);
 }
 
 static int backup(int argc, const char** argv) {
@@ -1249,7 +1284,7 @@ int adb_commandline(int argc, const char **argv) {
 
         FeatureSet features = GetFeatureSet(transport_type, serial);
 
-        bool use_shell_protocol = (features.count(kFeatureShell2) > 0);
+        bool use_shell_protocol = CanUseFeature(features, kFeatureShell2);
         if (!use_shell_protocol) {
             D("shell protocol not supported, using raw data transfer");
         } else {
@@ -1263,19 +1298,22 @@ int adb_commandline(int argc, const char **argv) {
         std::string shell_type_arg;
         while (argc) {
             if (!strcmp(argv[0], "-T") || !strcmp(argv[0], "-t")) {
-                if (features.count(kFeatureShell2) == 0) {
+                if (!CanUseFeature(features, kFeatureShell2)) {
                     fprintf(stderr, "error: target doesn't support PTY args -Tt\n");
                     return 1;
                 }
-                shell_type_arg = argv[0];
+                shell_type_arg = (argv[0][1] == 'T') ? kShellServiceArgRaw
+                                                     : kShellServiceArgPty;
+                --argc;
+                ++argv;
+            } else if (!strcmp(argv[0], "-x")) {
+                use_shell_protocol = false;
                 --argc;
                 ++argv;
             } else {
                 break;
             }
         }
-        std::string service_string = android::base::StringPrintf(
-                "shell%s:", shell_type_arg.c_str());
 
         if (h) {
             printf("\x1b[41;33m");
@@ -1284,6 +1322,8 @@ int adb_commandline(int argc, const char **argv) {
 
         if (!argc) {
             D("starting interactive shell");
+            std::string service_string =
+                    ShellServiceString(use_shell_protocol, shell_type_arg, "");
             r = interactive_shell(service_string, use_shell_protocol);
             if (h) {
                 printf("\x1b[0m");
@@ -1293,8 +1333,10 @@ int adb_commandline(int argc, const char **argv) {
         }
 
         // We don't escape here, just like ssh(1). http://b/20564385.
-        service_string += android::base::Join(
+        std::string command = android::base::Join(
                 std::vector<const char*>(argv, argv + argc), ' ');
+        std::string service_string =
+                ShellServiceString(use_shell_protocol, shell_type_arg, command);
 
         while (true) {
             D("non-interactive shell loop. cmd=%s", service_string.c_str());
@@ -1397,7 +1439,9 @@ int adb_commandline(int argc, const char **argv) {
     }
     else if (!strcmp(argv[0], "bugreport")) {
         if (argc != 1) return usage();
-        return send_shell_command(transport_type, serial, "shell:bugreport");
+        // No need for shell protocol with bugreport, always disable for
+        // simplicity.
+        return send_shell_command(transport_type, serial, "bugreport", true);
     }
     else if (!strcmp(argv[0], "forward") || !strcmp(argv[0], "reverse")) {
         bool reverse = !strcmp(argv[0], "reverse");
@@ -1574,7 +1618,7 @@ int adb_commandline(int argc, const char **argv) {
         // Only list the features common to both the adb client and the device.
         FeatureSet features = GetFeatureSet(transport_type, serial);
         for (const std::string& name : features) {
-            if (supported_features().count(name) > 0) {
+            if (CanUseFeature(features, name)) {
                 printf("%s\n", name.c_str());
             }
         }
@@ -1586,13 +1630,15 @@ int adb_commandline(int argc, const char **argv) {
 }
 
 static int pm_command(TransportType transport, const char* serial, int argc, const char** argv) {
-    std::string cmd = "shell:pm";
+    std::string cmd = "pm";
 
     while (argc-- > 0) {
         cmd += " " + escape_arg(*argv++);
     }
 
-    return send_shell_command(transport, serial, cmd);
+    // TODO(dpursell): add command-line arguments to install/uninstall to
+    // manually disable shell protocol if needed.
+    return send_shell_command(transport, serial, cmd, false);
 }
 
 static int uninstall_app(TransportType transport, const char* serial, int argc, const char** argv) {
@@ -1613,8 +1659,8 @@ static int uninstall_app(TransportType transport, const char* serial, int argc,
 }
 
 static int delete_file(TransportType transport, const char* serial, const std::string& filename) {
-    std::string cmd = "shell:rm -f " + escape_arg(filename);
+    std::string cmd = "rm -f " + escape_arg(filename);
-    return send_shell_command(transport, serial, cmd);
+    return send_shell_command(transport, serial, cmd, false);
 }
 
 static int install_app(TransportType transport, const char* serial, int argc, const char** argv) {

adb/services.cpp

@@ -46,6 +46,7 @@
 #include "adb_utils.h"
 #include "file_sync_service.h"
 #include "remount_service.h"
+#include "services.h"
 #include "shell_service.h"
 #include "transport.h"
 
@@ -195,33 +196,37 @@ void reverse_service(int fd, void* arg)
 }
 
 // Shell service string can look like:
-//   shell[args]:[command]
+//   shell[,arg1,arg2,...]:[command]
-// Currently the only supported args are -T (force raw) and -t (force PTY).
 static int ShellService(const std::string& args, const atransport* transport) {
     size_t delimiter_index = args.find(':');
     if (delimiter_index == std::string::npos) {
         LOG(ERROR) << "No ':' found in shell service arguments: " << args;
         return -1;
     }
+
     const std::string service_args = args.substr(0, delimiter_index);
     const std::string command = args.substr(delimiter_index + 1);
 
-    SubprocessType type;
+    // Defaults:
-    if (service_args.empty()) {
+    //   PTY for interactive, raw for non-interactive.
-        // Default: use PTY for interactive, raw for non-interactive.
+    //   No protocol.
-        type = (command.empty() ? SubprocessType::kPty : SubprocessType::kRaw);
+    SubprocessType type(command.empty() ? SubprocessType::kPty
-    } else if (service_args == "-T") {
+                                        : SubprocessType::kRaw);
-        type = SubprocessType::kRaw;
+    SubprocessProtocol protocol = SubprocessProtocol::kNone;
-    } else if (service_args == "-t") {
-        type = SubprocessType::kPty;
-    } else {
-        LOG(ERROR) << "Unsupported shell service arguments: " << args;
-        return -1;
-    }
 
-    SubprocessProtocol protocol =
+    for (const std::string& arg : android::base::Split(service_args, ",")) {
-            (transport->CanUseFeature(kFeatureShell2) ? SubprocessProtocol::kShell
+        if (arg == kShellServiceArgRaw) {
-                                                      : SubprocessProtocol::kNone);
+            type = SubprocessType::kRaw;
+        } else if (arg == kShellServiceArgPty) {
+            type = SubprocessType::kPty;
+        } else if (arg == kShellServiceArgShellProtocol) {
+            protocol = SubprocessProtocol::kShell;
+        }
+        else if (!arg.empty()) {
+            LOG(ERROR) << "Unsupported shell service arguments: " << args;
+            return -1;
+        }
+    }
 
     return StartSubprocess(command.c_str(), type, protocol);
 }

adb/services.h

@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef SERVICES_H_
+#define SERVICES_H_
+
+constexpr char kShellServiceArgRaw[] = "raw";
+constexpr char kShellServiceArgPty[] = "pty";
+constexpr char kShellServiceArgShellProtocol[] = "v2";
+
+#endif  // SERVICES_H_

adb/transport.cpp

@@ -808,6 +808,11 @@ FeatureSet StringToFeatureSet(const std::string& features_string) {
     return FeatureSet(names.begin(), names.end());
 }
 
+bool CanUseFeature(const FeatureSet& feature_set, const std::string& feature) {
+    return feature_set.count(feature) > 0 &&
+            supported_features().count(feature) > 0;
+}
+
 bool atransport::has_feature(const std::string& feature) const {
     return features_.count(feature) > 0;
 }
@@ -816,10 +821,6 @@ void atransport::SetFeatures(const std::string& features_string) {
     features_ = StringToFeatureSet(features_string);
 }
 
-bool atransport::CanUseFeature(const std::string& feature) const {
-    return has_feature(feature) && supported_features().count(feature) > 0;
-}
-
 void atransport::AddDisconnect(adisconnect* disconnect) {
     disconnects_.push_back(disconnect);
 }

adb/transport.h

@@ -33,9 +33,12 @@ const FeatureSet& supported_features();
 std::string FeatureSetToString(const FeatureSet& features);
 FeatureSet StringToFeatureSet(const std::string& features_string);
 
+// Returns true if both local features and |feature_set| support |feature|.
+bool CanUseFeature(const FeatureSet& feature_set, const std::string& feature);
+
 // Do not use any of [:;=,] in feature strings, they have special meaning
 // in the connection banner.
-constexpr char kFeatureShell2[] = "shell_2";
+constexpr char kFeatureShell2[] = "shell_v2";
 
 class atransport {
 public:
@@ -100,10 +103,6 @@ public:
     // Loads the transport's feature set from the given string.
     void SetFeatures(const std::string& features_string);
 
-    // Returns true if both we and the other end of the transport support the
-    // feature.
-    bool CanUseFeature(const std::string& feature) const;
-
     void AddDisconnect(adisconnect* disconnect);
     void RemoveDisconnect(adisconnect* disconnect);
     void RunDisconnects();