Refactored functions that copy sdout and stderr to strings to use a callback.

BUG: 28609499

Change-Id: I04aea346e18678ea00797f7f659480edba4436c2

adb/bugreport.cpp

@@ -19,12 +19,59 @@
 #include <android-base/strings.h>
 
 #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<const char*> 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);
+    BugreportStandardStreamsCallback bugz_callback(dest_file, this);
-    if (status != 0 || output.empty()) return status;
+    return SendShellCommand(transport_type, serial, "bugreportz", false, &bugz_callback);
-    output = android::base::Trim(output);
-
-    if (android::base::StartsWith(output, BUGZ_OK_PREFIX)) {
-        const char* zip_file = &output[strlen(BUGZ_OK_PREFIX)];
-        std::vector<const char*> 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;
 }
 
 int Bugreport::SendShellCommand(TransportType transport_type, const char* serial,
                                 const std::string& command, bool disable_shell_protocol,
-                                std::string* output, std::string* err) {
+                                StandardStreamsCallbackInterface* callback) {
-    return send_shell_command(transport_type, serial, command, disable_shell_protocol, output, err);
+    return send_shell_command(transport_type, serial, command, disable_shell_protocol, callback);
 }
 
 bool Bugreport::DoSyncPull(const std::vector<const char*>& srcs, const char* dst, bool copy_attrs,

adb/bugreport.h

@@ -20,8 +20,11 @@
 #include <vector>
 
 #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,
+    virtual int SendShellCommand(
-                                 const std::string& command, bool disable_shell_protocol,
+        TransportType transport_type, const char* serial, const std::string& command,
-                                 std::string* output = nullptr, std::string* err = nullptr);
+        bool disable_shell_protocol,
+        StandardStreamsCallbackInterface* callback = &DEFAULT_STANDARD_STREAMS_CALLBACK);
 
     virtual bool DoSyncPull(const std::vector<const char*>& srcs, const char* dst, bool copy_attrs,
                             const char* name);

adb/bugreport_test.cpp

@@ -20,17 +20,19 @@
 #include <gtest/gtest.h>
 
 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
+// Empty function so tests don't need to be linked against file_sync_service.cpp, which requires
-// file_sync_service.cpp, which requires
 // SELinux and its transitive dependencies...
 bool do_sync_pull(const std::vector<const char*>& srcs, const char* dst, bool copy_attrs,
                   const char* name) {
@@ -38,23 +40,55 @@ bool do_sync_pull(const std::vector<const char*>& 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<OnStandardStreamsCallbackFunction> {
+  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<OnStandardStreamsCallbackFunction> WriteOnStdout(const std::string& output) {
+    return MakeAction(new OnStandardStreamsCallbackAction(output));
+}
+
+typedef int CallbackDoneFunction(StandardStreamsCallbackInterface*);
+
+class CallbackDoneAction : public ActionInterface<CallbackDoneFunction> {
+  public:
+    virtual Result Perform(const ArgumentTuple& args) {
+        int status = ::std::tr1::get<0>(args)->Done(123);  // Value passed does not matter
+        return status;
+    }
+};
+
+Action<CallbackDoneFunction> 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<const char*>& 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,
+    EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreport", false, _))
-                                      nullptr, nullptr))
         .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(
+    EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
-        br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr))
+        .WillOnce(DoAll(WithArg<4>(WriteOnStdout("OK:/device/bugreport.zip")),
-        .WillOnce(DoAll(SetArgPointee<4>("OK:/device/bugreport.zip"), Return(0)));
+                        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(
+    EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
-        br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr))
+        .WillOnce(DoAll(WithArg<4>(WriteOnStdout("OK:/device/bugreport.zip")),
-        .WillOnce(DoAll(SetArgPointee<4>("OK:/device/bugreport.zip"), Return(0)));
+                        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(
+    EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
-        br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr))
+        .WillOnce(DoAll(WithArg<4>(WriteOnStdout("FAIL:D'OH!")), WithArg<4>(ReturnCallbackDone())));
-        .WillOnce(DoAll(SetArgPointee<4>("FAIL:D'OH!"), Return(0)));
+
+    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(
+    EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
-        br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr))
+        .WillOnce(DoAll(WithArg<4>(WriteOnStdout("bugreportz? What am I, a zombie?")),
-        .WillOnce(DoAll(SetArgPointee<4>("bugreportz? What am I, a zombie?"), Return(0)));
+                        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(
+    EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
-        br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr))
         .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(
+    EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
-        br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _, nullptr))
+        .WillOnce(DoAll(WithArg<4>(WriteOnStdout("OK:/device/bugreport.zip")),
-        .WillOnce(DoAll(SetArgPointee<4>("OK:/device/bugreport.zip"), Return(0)));
+                        WithArg<4>(ReturnCallbackDone())));
     EXPECT_CALL(br_, DoSyncPull(ElementsAre(StrEq("/device/bugreport.zip")), StrEq("file.zip"),
                                 true, StrEq("file.zip")))
         .WillOnce(Return(false));

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 |callback| is non-null, stdout/stderr output will be handled by it.
-// if |err| is non-null, stderr will be appended to it instead.
+int read_and_dump(int fd, bool use_shell_protocol = false,
-static int read_and_dump(int fd, bool use_shell_protocol=false, std::string* output=nullptr,
+                  StandardStreamsCallbackInterface* callback = &DEFAULT_STANDARD_STREAMS_CALLBACK) {
-                         std::string* err=nullptr) {
     int exit_code = 0;
     if (fd < 0) return exit_code;
 
     std::unique_ptr<ShellProtocol> 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;
+                    callback->OnStdout(buffer_ptr, length);
-                    outstring = output;
                     break;
                 case ShellProtocol::kIdStderr:
-                    outfile = stderr;
+                    callback->OnStderr(buffer_ptr, length);
-                    outstring = err;
                     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;
             }
-        }
+            callback->OnStdout(buffer_ptr, length);
-
-        if (outstring == nullptr) {
-            fwrite(buffer_ptr, 1, length, outfile);
-            fflush(outfile);
-        } else {
-            outstring->append(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;

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,
+                       bool disable_shell_protocol, StandardStreamsCallbackInterface* callback =
-                       std::string* err = nullptr);
+                                                        &DEFAULT_STANDARD_STREAMS_CALLBACK);
 
 #endif  // COMMANDLINE_H