From 07ac8554b477dc81579a5e63a2fbabc740fa8a92 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Tue, 26 Jul 2016 12:14:39 -0700 Subject: [PATCH] Refactored functions that copy sdout and stderr to strings to use a callback. BUG: 28609499 Change-Id: I04aea346e18678ea00797f7f659480edba4436c2 --- adb/bugreport.cpp | 80 ++++++++++++++++++---------- adb/bugreport.h | 10 ++-- adb/bugreport_test.cpp | 116 +++++++++++++++++++++++++++++++---------- adb/commandline.cpp | 37 ++++++------- adb/commandline.h | 72 ++++++++++++++++++++++++- 5 files changed, 232 insertions(+), 83 deletions(-) diff --git a/adb/bugreport.cpp b/adb/bugreport.cpp index 1af5843c6..e267f0f87 100644 --- a/adb/bugreport.cpp +++ b/adb/bugreport.cpp @@ -19,12 +19,59 @@ #include #include "bugreport.h" -#include "commandline.h" #include "file_sync_service.h" static constexpr char BUGZ_OK_PREFIX[] = "OK:"; static constexpr char BUGZ_FAIL_PREFIX[] = "FAIL:"; +// Custom callback used to handle the output of zipped bugreports. +class BugreportStandardStreamsCallback : public StandardStreamsCallbackInterface { + public: + BugreportStandardStreamsCallback(const std::string& dest_file, Bugreport* br) + : br_(br), dest_file_(dest_file), stdout_str_() { + } + + void OnStdout(const char* buffer, int length) { + std::string output; + OnStream(&output, stdout, buffer, length); + stdout_str_.append(output); + } + + void OnStderr(const char* buffer, int length) { + OnStream(nullptr, stderr, buffer, length); + } + + int Done(int unused_) { + int status = -1; + std::string output = android::base::Trim(stdout_str_); + if (android::base::StartsWith(output, BUGZ_OK_PREFIX)) { + const char* zip_file = &output[strlen(BUGZ_OK_PREFIX)]; + std::vector srcs{zip_file}; + status = br_->DoSyncPull(srcs, dest_file_.c_str(), true, dest_file_.c_str()) ? 0 : 1; + if (status != 0) { + fprintf(stderr, "Could not copy file '%s' to '%s'\n", zip_file, dest_file_.c_str()); + } + } else if (android::base::StartsWith(output, BUGZ_FAIL_PREFIX)) { + const char* error_message = &output[strlen(BUGZ_FAIL_PREFIX)]; + fprintf(stderr, "Device failed to take a zipped bugreport: %s\n", error_message); + } else { + fprintf(stderr, + "Unexpected string (%s) returned by bugreportz, " + "device probably does not support it\n", + output.c_str()); + } + + return status; + } + + private: + Bugreport* br_; + const std::string dest_file_; + std::string stdout_str_; + + DISALLOW_COPY_AND_ASSIGN(BugreportStandardStreamsCallback); +}; + int Bugreport::DoIt(TransportType transport_type, const char* serial, int argc, const char** argv) { if (argc == 1) return SendShellCommand(transport_type, serial, "bugreport", false); if (argc != 2) return usage(); @@ -37,41 +84,18 @@ int Bugreport::DoIt(TransportType transport_type, const char* serial, int argc, // TODO: use a case-insensitive comparison (like EndsWithIgnoreCase dest_file += ".zip"; } - std::string output; - fprintf(stderr, "Bugreport is in progress and it could take minutes to complete.\n" "Please be patient and do not cancel or disconnect your device until " "it completes.\n"); - int status = SendShellCommand(transport_type, serial, "bugreportz", false, &output, nullptr); - if (status != 0 || output.empty()) return status; - output = android::base::Trim(output); - - if (android::base::StartsWith(output, BUGZ_OK_PREFIX)) { - const char* zip_file = &output[strlen(BUGZ_OK_PREFIX)]; - std::vector srcs{zip_file}; - status = DoSyncPull(srcs, dest_file.c_str(), true, dest_file.c_str()) ? 0 : 1; - if (status != 0) { - fprintf(stderr, "Could not copy file '%s' to '%s'\n", zip_file, dest_file.c_str()); - } - return status; - } - if (android::base::StartsWith(output, BUGZ_FAIL_PREFIX)) { - const char* error_message = &output[strlen(BUGZ_FAIL_PREFIX)]; - fprintf(stderr, "Device failed to take a zipped bugreport: %s\n", error_message); - return -1; - } - fprintf(stderr, - "Unexpected string (%s) returned by bugreportz, " - "device probably does not support it\n", - output.c_str()); - return -1; + BugreportStandardStreamsCallback bugz_callback(dest_file, this); + return SendShellCommand(transport_type, serial, "bugreportz", false, &bugz_callback); } int Bugreport::SendShellCommand(TransportType transport_type, const char* serial, const std::string& command, bool disable_shell_protocol, - std::string* output, std::string* err) { - return send_shell_command(transport_type, serial, command, disable_shell_protocol, output, err); + StandardStreamsCallbackInterface* callback) { + return send_shell_command(transport_type, serial, command, disable_shell_protocol, callback); } bool Bugreport::DoSyncPull(const std::vector& srcs, const char* dst, bool copy_attrs, diff --git a/adb/bugreport.h b/adb/bugreport.h index ff3a45726..1c39a9f23 100644 --- a/adb/bugreport.h +++ b/adb/bugreport.h @@ -20,8 +20,11 @@ #include #include "adb.h" +#include "commandline.h" class Bugreport { + friend class BugreportStandardStreamsCallback; + public: Bugreport() { } @@ -30,9 +33,10 @@ class Bugreport { protected: // Functions below are abstractions of external functions so they can be // mocked on tests. - virtual int SendShellCommand(TransportType transport_type, const char* serial, - const std::string& command, bool disable_shell_protocol, - std::string* output = nullptr, std::string* err = nullptr); + virtual int SendShellCommand( + TransportType transport_type, const char* serial, const std::string& command, + bool disable_shell_protocol, + StandardStreamsCallbackInterface* callback = &DEFAULT_STANDARD_STREAMS_CALLBACK); virtual bool DoSyncPull(const std::vector& srcs, const char* dst, bool copy_attrs, const char* name); diff --git a/adb/bugreport_test.cpp b/adb/bugreport_test.cpp index dd2ff37fc..15c6f83f2 100644 --- a/adb/bugreport_test.cpp +++ b/adb/bugreport_test.cpp @@ -20,17 +20,19 @@ #include using ::testing::_; +using ::testing::Action; +using ::testing::ActionInterface; using ::testing::DoAll; using ::testing::ElementsAre; using ::testing::HasSubstr; +using ::testing::MakeAction; using ::testing::Return; -using ::testing::SetArgPointee; using ::testing::StrEq; +using ::testing::WithArg; using ::testing::internal::CaptureStderr; using ::testing::internal::GetCapturedStderr; -// Empty function so tests don't need to be linked against -// file_sync_service.cpp, which requires +// Empty function so tests don't need to be linked against file_sync_service.cpp, which requires // SELinux and its transitive dependencies... bool do_sync_pull(const std::vector& srcs, const char* dst, bool copy_attrs, const char* name) { @@ -38,23 +40,55 @@ bool do_sync_pull(const std::vector& srcs, const char* dst, bool co return false; } -// Implemented in commandline.cpp +// Empty functions so tests don't need to be linked against commandline.cpp +DefaultStandardStreamsCallback DEFAULT_STANDARD_STREAMS_CALLBACK(nullptr, nullptr); int usage() { return -42; } - -// Implemented in commandline.cpp int send_shell_command(TransportType transport_type, const char* serial, const std::string& command, - bool disable_shell_protocol, std::string* output, std::string* err) { + bool disable_shell_protocol, StandardStreamsCallbackInterface* callback) { ADD_FAILURE() << "send_shell_command() should have been mocked"; return -42; } +// gmock black magic to provide a WithArg<4>(WriteOnStdout(output)) matcher +typedef void OnStandardStreamsCallbackFunction(StandardStreamsCallbackInterface*); + +class OnStandardStreamsCallbackAction : public ActionInterface { + public: + explicit OnStandardStreamsCallbackAction(const std::string& output) : output_(output) { + } + virtual Result Perform(const ArgumentTuple& args) { + ::std::tr1::get<0>(args)->OnStdout(output_.c_str(), output_.size()); + } + + private: + std::string output_; +}; + +Action WriteOnStdout(const std::string& output) { + return MakeAction(new OnStandardStreamsCallbackAction(output)); +} + +typedef int CallbackDoneFunction(StandardStreamsCallbackInterface*); + +class CallbackDoneAction : public ActionInterface { + public: + virtual Result Perform(const ArgumentTuple& args) { + int status = ::std::tr1::get<0>(args)->Done(123); // Value passed does not matter + return status; + } +}; + +Action ReturnCallbackDone() { + return MakeAction(new CallbackDoneAction()); +} + class BugreportMock : public Bugreport { public: - MOCK_METHOD6(SendShellCommand, + MOCK_METHOD5(SendShellCommand, int(TransportType transport_type, const char* serial, const std::string& command, - bool disable_shell_protocol, std::string* output, std::string* err)); + bool disable_shell_protocol, StandardStreamsCallbackInterface* callback)); MOCK_METHOD4(DoSyncPull, bool(const std::vector& srcs, const char* dst, bool copy_attrs, const char* name)); }; @@ -72,8 +106,7 @@ TEST_F(BugreportTest, InvalidNumberArgs) { // Tests the legacy 'adb bugreport' option TEST_F(BugreportTest, FlatFileFormat) { - EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreport", false, - nullptr, nullptr)) + EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreport", false, _)) .WillOnce(Return(0)); const char* args[1024] = {"bugreport"}; @@ -82,9 +115,9 @@ TEST_F(BugreportTest, FlatFileFormat) { // Tests 'adb bugreport file.zip' when it succeeds TEST_F(BugreportTest, Ok) { - EXPECT_CALL( - br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr)) - .WillOnce(DoAll(SetArgPointee<4>("OK:/device/bugreport.zip"), Return(0))); + EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _)) + .WillOnce(DoAll(WithArg<4>(WriteOnStdout("OK:/device/bugreport.zip")), + WithArg<4>(ReturnCallbackDone()))); EXPECT_CALL(br_, DoSyncPull(ElementsAre(StrEq("/device/bugreport.zip")), StrEq("file.zip"), true, StrEq("file.zip"))) .WillOnce(Return(true)); @@ -95,9 +128,9 @@ TEST_F(BugreportTest, Ok) { // Tests 'adb bugreport file' when it succeeds TEST_F(BugreportTest, OkNoExtension) { - EXPECT_CALL( - br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr)) - .WillOnce(DoAll(SetArgPointee<4>("OK:/device/bugreport.zip"), Return(0))); + EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _)) + .WillOnce(DoAll(WithArg<4>(WriteOnStdout("OK:/device/bugreport.zip")), + WithArg<4>(ReturnCallbackDone()))); EXPECT_CALL(br_, DoSyncPull(ElementsAre(StrEq("/device/bugreport.zip")), StrEq("file.zip"), true, StrEq("file.zip"))) .WillOnce(Return(true)); @@ -106,11 +139,39 @@ TEST_F(BugreportTest, OkNoExtension) { ASSERT_EQ(0, br_.DoIt(kTransportLocal, "HannibalLecter", 2, args)); } +// Tests 'adb bugreport file.zip' when it succeeds but response was sent in +// multiple buffer writers. +TEST_F(BugreportTest, OkSplitBuffer) { + EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _)) + .WillOnce(DoAll(WithArg<4>(WriteOnStdout("OK:/device")), + WithArg<4>(WriteOnStdout("/bugreport.zip")), + WithArg<4>(ReturnCallbackDone()))); + EXPECT_CALL(br_, DoSyncPull(ElementsAre(StrEq("/device/bugreport.zip")), StrEq("file.zip"), + true, StrEq("file.zip"))) + .WillOnce(Return(true)); + + const char* args[1024] = {"bugreport", "file.zip"}; + ASSERT_EQ(0, br_.DoIt(kTransportLocal, "HannibalLecter", 2, args)); +} + // Tests 'adb bugreport file.zip' when the bugreport itself failed TEST_F(BugreportTest, BugreportzReturnedFail) { - EXPECT_CALL( - br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr)) - .WillOnce(DoAll(SetArgPointee<4>("FAIL:D'OH!"), Return(0))); + EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _)) + .WillOnce(DoAll(WithArg<4>(WriteOnStdout("FAIL:D'OH!")), WithArg<4>(ReturnCallbackDone()))); + + CaptureStderr(); + const char* args[1024] = {"bugreport", "file.zip"}; + ASSERT_EQ(-1, br_.DoIt(kTransportLocal, "HannibalLecter", 2, args)); + ASSERT_THAT(GetCapturedStderr(), HasSubstr("D'OH")); +} + +// Tests 'adb bugreport file.zip' when the bugreport itself failed but response +// was sent in +// multiple buffer writes +TEST_F(BugreportTest, BugreportzReturnedFailSplitBuffer) { + EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _)) + .WillOnce(DoAll(WithArg<4>(WriteOnStdout("FAIL")), WithArg<4>(WriteOnStdout(":D'OH!")), + WithArg<4>(ReturnCallbackDone()))); CaptureStderr(); const char* args[1024] = {"bugreport", "file.zip"}; @@ -121,9 +182,9 @@ TEST_F(BugreportTest, BugreportzReturnedFail) { // Tests 'adb bugreport file.zip' when the bugreportz returned an unsupported // response. TEST_F(BugreportTest, BugreportzReturnedUnsupported) { - EXPECT_CALL( - br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr)) - .WillOnce(DoAll(SetArgPointee<4>("bugreportz? What am I, a zombie?"), Return(0))); + EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _)) + .WillOnce(DoAll(WithArg<4>(WriteOnStdout("bugreportz? What am I, a zombie?")), + WithArg<4>(ReturnCallbackDone()))); CaptureStderr(); const char* args[1024] = {"bugreport", "file.zip"}; @@ -133,8 +194,7 @@ TEST_F(BugreportTest, BugreportzReturnedUnsupported) { // Tests 'adb bugreport file.zip' when the bugreportz command fails TEST_F(BugreportTest, BugreportzFailed) { - EXPECT_CALL( - br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr)) + EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _)) .WillOnce(Return(666)); const char* args[1024] = {"bugreport", "file.zip"}; @@ -143,9 +203,9 @@ TEST_F(BugreportTest, BugreportzFailed) { // Tests 'adb bugreport file.zip' when the bugreport could not be pulled TEST_F(BugreportTest, PullFails) { - EXPECT_CALL( - br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr)) - .WillOnce(DoAll(SetArgPointee<4>("OK:/device/bugreport.zip"), Return(0))); + EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _)) + .WillOnce(DoAll(WithArg<4>(WriteOnStdout("OK:/device/bugreport.zip")), + WithArg<4>(ReturnCallbackDone()))); EXPECT_CALL(br_, DoSyncPull(ElementsAre(StrEq("/device/bugreport.zip")), StrEq("file.zip"), true, StrEq("file.zip"))) .WillOnce(Return(false)); diff --git a/adb/commandline.cpp b/adb/commandline.cpp index 6cb9ae2e8..0685b7045 100644 --- a/adb/commandline.cpp +++ b/adb/commandline.cpp @@ -66,6 +66,8 @@ static int uninstall_app_legacy(TransportType t, const char* serial, int argc, c static auto& gProductOutPath = *new std::string(); extern int gListenAll; +DefaultStandardStreamsCallback DEFAULT_STANDARD_STREAMS_CALLBACK(nullptr, nullptr); + static std::string product_file(const char *extra) { if (gProductOutPath.empty()) { fprintf(stderr, "adb: Product directory not specified; " @@ -289,17 +291,14 @@ static void stdin_raw_restore() { // this expects that incoming data will use the shell protocol, in which case // stdout/stderr are routed independently and the remote exit code will be // returned. -// if |output| is non-null, stdout will be appended to it instead. -// if |err| is non-null, stderr will be appended to it instead. -static int read_and_dump(int fd, bool use_shell_protocol=false, std::string* output=nullptr, - std::string* err=nullptr) { +// if |callback| is non-null, stdout/stderr output will be handled by it. +int read_and_dump(int fd, bool use_shell_protocol = false, + StandardStreamsCallbackInterface* callback = &DEFAULT_STANDARD_STREAMS_CALLBACK) { int exit_code = 0; if (fd < 0) return exit_code; std::unique_ptr protocol; int length = 0; - FILE* outfile = stdout; - std::string* outstring = output; char raw_buffer[BUFSIZ]; char* buffer_ptr = raw_buffer; @@ -317,14 +316,13 @@ static int read_and_dump(int fd, bool use_shell_protocol=false, std::string* out if (!protocol->Read()) { break; } + length = protocol->data_length(); switch (protocol->id()) { case ShellProtocol::kIdStdout: - outfile = stdout; - outstring = output; + callback->OnStdout(buffer_ptr, length); break; case ShellProtocol::kIdStderr: - outfile = stderr; - outstring = err; + callback->OnStderr(buffer_ptr, length); break; case ShellProtocol::kIdExit: exit_code = protocol->data()[0]; @@ -340,17 +338,11 @@ static int read_and_dump(int fd, bool use_shell_protocol=false, std::string* out if (length <= 0) { break; } - } - - if (outstring == nullptr) { - fwrite(buffer_ptr, 1, length, outfile); - fflush(outfile); - } else { - outstring->append(buffer_ptr, length); + callback->OnStdout(buffer_ptr, length); } } - return exit_code; + return callback->Done(exit_code); } static void read_status_line(int fd, char* buf, size_t count) @@ -1130,14 +1122,15 @@ static bool adb_root(const char* command) { } int send_shell_command(TransportType transport_type, const char* serial, const std::string& command, - bool disable_shell_protocol, std::string* output, std::string* err) { + bool disable_shell_protocol, StandardStreamsCallbackInterface* callback) { int fd; bool use_shell_protocol = false; while (true) { bool attempt_connection = true; - // Use shell protocol if it's supported and the caller doesn't explicitly disable it. + // Use shell protocol if it's supported and the caller doesn't explicitly + // disable it. if (!disable_shell_protocol) { FeatureSet features; std::string error; @@ -1159,13 +1152,13 @@ int send_shell_command(TransportType transport_type, const char* serial, const s } } - fprintf(stderr,"- waiting for device -\n"); + fprintf(stderr, "- waiting for device -\n"); if (!wait_for_device("wait-for-device", transport_type, serial)) { return 1; } } - int exit_code = read_and_dump(fd, use_shell_protocol, output, err); + int exit_code = read_and_dump(fd, use_shell_protocol, callback); if (adb_close(fd) < 0) { PLOG(ERROR) << "failure closing FD " << fd; diff --git a/adb/commandline.h b/adb/commandline.h index 39764b452..0cf655c94 100644 --- a/adb/commandline.h +++ b/adb/commandline.h @@ -19,13 +19,81 @@ #include "adb.h" +// Callback used to handle the standard streams (stdout and stderr) sent by the +// device's upon receiving a command. +// +class StandardStreamsCallbackInterface { + public: + StandardStreamsCallbackInterface() { + } + // Handles the stdout output from devices supporting the Shell protocol. + virtual void OnStdout(const char* buffer, int length) = 0; + + // Handles the stderr output from devices supporting the Shell protocol. + virtual void OnStderr(const char* buffer, int length) = 0; + + // Indicates the communication is finished and returns the appropriate error + // code. + // + // |status| has the status code returning by the underlying communication + // channels + virtual int Done(int status) = 0; + + protected: + static void OnStream(std::string* string, FILE* stream, const char* buffer, int length) { + if (string != nullptr) { + string->append(buffer, length); + } else { + fwrite(buffer, 1, length, stream); + fflush(stream); + } + } + + private: + DISALLOW_COPY_AND_ASSIGN(StandardStreamsCallbackInterface); +}; + +// Default implementation that redirects the streams to the equilavent host +// stream or to a string +// passed to the constructor. +class DefaultStandardStreamsCallback : public StandardStreamsCallbackInterface { + public: + // If |stdout_str| is non-null, OnStdout will append to it. + // If |stderr_str| is non-null, OnStderr will append to it. + DefaultStandardStreamsCallback(std::string* stdout_str, std::string* stderr_str) + : stdout_str_(stdout_str), stderr_str_(stderr_str) { + } + + void OnStdout(const char* buffer, int length) { + OnStream(stdout_str_, stdout, buffer, length); + } + + void OnStderr(const char* buffer, int length) { + OnStream(stderr_str_, stderr, buffer, length); + } + + int Done(int status) { + return status; + } + + private: + std::string* stdout_str_; + std::string* stderr_str_; + + DISALLOW_COPY_AND_ASSIGN(DefaultStandardStreamsCallback); +}; + +// Singleton. +extern DefaultStandardStreamsCallback DEFAULT_STANDARD_STREAMS_CALLBACK; + int adb_commandline(int argc, const char** argv); int usage(); // Connects to the device "shell" service with |command| and prints the // resulting output. +// if |callback| is non-null, stdout/stderr output will be handled by it. int send_shell_command(TransportType transport_type, const char* serial, const std::string& command, - bool disable_shell_protocol, std::string* output = nullptr, - std::string* err = nullptr); + bool disable_shell_protocol, StandardStreamsCallbackInterface* callback = + &DEFAULT_STANDARD_STREAMS_CALLBACK); #endif // COMMANDLINE_H