From fb4616b20004836c28384935d4b729fd109d6b0d Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 16 Apr 2020 19:34:43 -0700 Subject: [PATCH] adb: fix feature sets on devices that don't have them. Previously, we weren't distinguishing between nonexistent devices and devices that don't support features, resulting in lots of stuff being broken for pre-N devices. Bug: http://b/154272158 Test: adb shell on a JB emulator Change-Id: I0058b2a60678e1ad0503f5abac30157be976c9c4 --- adb/client/adb_client.cpp | 24 ++++++++------ adb/client/adb_client.h | 2 +- adb/client/adb_install.cpp | 7 +++-- adb/client/commandline.cpp | 56 ++++++++++++++++++--------------- adb/client/file_sync_client.cpp | 27 ++++++++-------- 5 files changed, 64 insertions(+), 52 deletions(-) diff --git a/adb/client/adb_client.cpp b/adb/client/adb_client.cpp index 0ad0465eb..31c938c89 100644 --- a/adb/client/adb_client.cpp +++ b/adb/client/adb_client.cpp @@ -416,14 +416,18 @@ std::string format_host_command(const char* command) { return android::base::StringPrintf("%s:%s", prefix, command); } -const FeatureSet& adb_get_feature_set() { - static const android::base::NoDestructor features([] { - std::string result; - if (!adb_query(format_host_command("features"), &result, &result)) { - fprintf(stderr, "failed to get feature set: %s\n", result.c_str()); - return FeatureSet{}; - } - return StringToFeatureSet(result); - }()); - return *features; +const std::optional& adb_get_feature_set(std::string* error) { + static std::string cached_error [[clang::no_destroy]]; + static const std::optional features + [[clang::no_destroy]] ([]() -> std::optional { + std::string result; + if (adb_query(format_host_command("features"), &result, &cached_error)) { + return StringToFeatureSet(result); + } + return std::nullopt; + }()); + if (!features && error) { + *error = cached_error; + } + return features; } diff --git a/adb/client/adb_client.h b/adb/client/adb_client.h index bf0be40b3..27be28f35 100644 --- a/adb/client/adb_client.h +++ b/adb/client/adb_client.h @@ -76,7 +76,7 @@ bool adb_status(borrowed_fd fd, std::string* _Nonnull error); std::string format_host_command(const char* _Nonnull command); // Get the feature set of the current preferred transport. -const FeatureSet& adb_get_feature_set(); +const std::optional& adb_get_feature_set(std::string* _Nullable error); #if defined(__linux__) // Get the path of a file containing the path to the server executable, if the socket spec set via diff --git a/adb/client/adb_install.cpp b/adb/client/adb_install.cpp index 3810cc89f..59c856382 100644 --- a/adb/client/adb_install.cpp +++ b/adb/client/adb_install.cpp @@ -57,11 +57,12 @@ enum class CmdlineOption { None, Enable, Disable }; } static bool can_use_feature(const char* feature) { - auto&& features = adb_get_feature_set(); - if (features.empty()) { + // We ignore errors here, if the device is missing, we'll notice when we try to push install. + auto&& features = adb_get_feature_set(nullptr); + if (!features) { return false; } - return CanUseFeature(features, feature); + return CanUseFeature(*features, feature); } static InstallMode best_install_mode() { diff --git a/adb/client/commandline.cpp b/adb/client/commandline.cpp index 6a7493f08..29f9dc14c 100644 --- a/adb/client/commandline.cpp +++ b/adb/client/commandline.cpp @@ -672,16 +672,17 @@ static int RemoteShell(bool use_shell_protocol, const std::string& type_arg, cha } static int adb_shell(int argc, const char** argv) { - auto&& features = adb_get_feature_set(); - if (features.empty()) { - return 1; + std::string error; + auto&& features = adb_get_feature_set(&error); + if (!features) { + error_exit("%s", error.c_str()); } enum PtyAllocationMode { kPtyAuto, kPtyNo, kPtyYes, kPtyDefinitely }; // Defaults. - char escape_char = '~'; // -e - bool use_shell_protocol = CanUseFeature(features, kFeatureShell2); // -x + 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. @@ -757,7 +758,7 @@ static int adb_shell(int argc, const char** argv) { if (!use_shell_protocol) { if (shell_type_arg != kShellServiceArgPty) { fprintf(stderr, "error: %s only supports allocating a pty\n", - !CanUseFeature(features, kFeatureShell2) ? "device" : "-x"); + !CanUseFeature(*features, kFeatureShell2) ? "device" : "-x"); return 1; } else { // If we're not using the shell protocol, the type argument must be empty. @@ -777,11 +778,13 @@ static int adb_shell(int argc, const char** argv) { } static int adb_abb(int argc, const char** argv) { - auto&& features = adb_get_feature_set(); - if (features.empty()) { + std::string error; + auto&& features = adb_get_feature_set(&error); + if (!features) { + error_exit("%s", error.c_str()); return 1; } - if (!CanUseFeature(features, kFeatureAbb)) { + if (!CanUseFeature(*features, kFeatureAbb)) { error_exit("abb is not supported by the device"); } @@ -1159,9 +1162,9 @@ int send_shell_command(const std::string& command, bool disable_shell_protocol, // Use shell protocol if it's supported and the caller doesn't explicitly // disable it. if (!disable_shell_protocol) { - auto&& features = adb_get_feature_set(); - if (!features.empty()) { - use_shell_protocol = CanUseFeature(features, kFeatureShell2); + auto&& features = adb_get_feature_set(nullptr); + if (features) { + use_shell_protocol = CanUseFeature(*features, kFeatureShell2); } else { // Device was unreachable. attempt_connection = false; @@ -1810,12 +1813,13 @@ int adb_commandline(int argc, const char** argv) { } return adb_connect_command(android::base::StringPrintf("tcpip:%d", port)); } else if (!strcmp(argv[0], "remount")) { - auto&& features = adb_get_feature_set(); - if (features.empty()) { - return 1; + std::string error; + auto&& features = adb_get_feature_set(&error); + if (!features) { + error_exit("%s", error.c_str()); } - if (CanUseFeature(features, kFeatureRemountShell)) { + if (CanUseFeature(*features, kFeatureRemountShell)) { std::vector args = {"shell"}; args.insert(args.cend(), argv, argv + argc); return adb_shell_noinput(args.size(), args.data()); @@ -2034,11 +2038,12 @@ int adb_commandline(int argc, const char** argv) { } else if (!strcmp(argv[0], "track-jdwp")) { return adb_connect_command("track-jdwp"); } else if (!strcmp(argv[0], "track-app")) { - auto&& features = adb_get_feature_set(); - if (features.empty()) { - return 1; + std::string error; + auto&& features = adb_get_feature_set(&error); + if (!features) { + error_exit("%s", error.c_str()); } - if (!CanUseFeature(features, kFeatureTrackApp)) { + if (!CanUseFeature(*features, kFeatureTrackApp)) { error_exit("track-app is not supported by the device"); } TrackAppStreamsCallback callback; @@ -2064,13 +2069,14 @@ int adb_commandline(int argc, const char** argv) { return 0; } else if (!strcmp(argv[0], "features")) { // Only list the features common to both the adb client and the device. - auto&& features = adb_get_feature_set(); - if (features.empty()) { - return 1; + std::string error; + auto&& features = adb_get_feature_set(&error); + if (!features) { + error_exit("%s", error.c_str()); } - for (const std::string& name : features) { - if (CanUseFeature(features, name)) { + for (const std::string& name : *features) { + if (CanUseFeature(*features, name)) { printf("%s\n", name.c_str()); } } diff --git a/adb/client/file_sync_client.cpp b/adb/client/file_sync_client.cpp index f2c673a88..7185939c2 100644 --- a/adb/client/file_sync_client.cpp +++ b/adb/client/file_sync_client.cpp @@ -225,21 +225,22 @@ struct TransferLedger { class SyncConnection { public: - SyncConnection() - : acknowledgement_buffer_(sizeof(sync_status) + SYNC_DATA_MAX), - features_(adb_get_feature_set()) { + SyncConnection() : acknowledgement_buffer_(sizeof(sync_status) + SYNC_DATA_MAX) { acknowledgement_buffer_.resize(0); max = SYNC_DATA_MAX; // TODO: decide at runtime. - if (features_.empty()) { - Error("failed to get feature set"); + std::string error; + auto&& features = adb_get_feature_set(&error); + if (!features) { + Error("failed to get feature set: %s", error.c_str()); } else { - have_stat_v2_ = CanUseFeature(features_, kFeatureStat2); - have_ls_v2_ = CanUseFeature(features_, kFeatureLs2); - have_sendrecv_v2_ = CanUseFeature(features_, kFeatureSendRecv2); - have_sendrecv_v2_brotli_ = CanUseFeature(features_, kFeatureSendRecv2Brotli); - have_sendrecv_v2_lz4_ = CanUseFeature(features_, kFeatureSendRecv2LZ4); - have_sendrecv_v2_dry_run_send_ = CanUseFeature(features_, kFeatureSendRecv2DryRunSend); + features_ = &*features; + have_stat_v2_ = CanUseFeature(*features, kFeatureStat2); + have_ls_v2_ = CanUseFeature(*features, kFeatureLs2); + have_sendrecv_v2_ = CanUseFeature(*features, kFeatureSendRecv2); + have_sendrecv_v2_brotli_ = CanUseFeature(*features, kFeatureSendRecv2Brotli); + have_sendrecv_v2_lz4_ = CanUseFeature(*features, kFeatureSendRecv2LZ4); + have_sendrecv_v2_dry_run_send_ = CanUseFeature(*features, kFeatureSendRecv2DryRunSend); std::string error; fd.reset(adb_connect("sync:", &error)); if (fd < 0) { @@ -283,7 +284,7 @@ class SyncConnection { return compression; } - const FeatureSet& Features() const { return features_; } + const FeatureSet& Features() const { return *features_; } bool IsValid() { return fd >= 0; } @@ -921,7 +922,7 @@ class SyncConnection { private: std::deque> deferred_acknowledgements_; Block acknowledgement_buffer_; - const FeatureSet& features_; + const FeatureSet* features_ = nullptr; bool have_stat_v2_; bool have_ls_v2_; bool have_sendrecv_v2_;