DO NOT MERGE: Show bugreport progress.
adb calls bugreportz to generate a bugreport; initially, bugreportz
would only report the final status of the operation (OK or FAIL), but
now it sends intermediate PROGRESS lines reporting its progress (in the
form of current/max).
Similarly, the initial implementation of 'adb bugreport <zip_file>'
would print an initial 'please wait' message and wait for the full
stdout before parsing the result, but now it uses a new callback class
to handle the stdout as it is generated by bugreportz.
BUG: 28609499
Change-Id: I6644fc39a686279e1635f946a47f3847b547d1c1
(cherry picked from commit 1e3df205b0f3ccfdfad94e69b6b28e0a0596455b)
(cherry picked from commit 5b78699f804c9fedb5f8ce757239270b266d17ff)
diff --git a/Android.mk b/Android.mk
index 5a0780c..0a9c454 100644
--- a/Android.mk
+++ b/Android.mk
@@ -187,6 +187,7 @@
adb_client.cpp \
bugreport.cpp \
bugreport_test.cpp \
+ line_printer.cpp \
services.cpp \
shell_service_protocol.cpp \
shell_service_protocol_test.cpp \
diff --git a/bugreport.cpp b/bugreport.cpp
index e267f0f..8a3eb08 100644
--- a/bugreport.cpp
+++ b/bugreport.cpp
@@ -23,55 +23,92 @@
static constexpr char BUGZ_OK_PREFIX[] = "OK:";
static constexpr char BUGZ_FAIL_PREFIX[] = "FAIL:";
+static constexpr char BUGZ_PROGRESS_PREFIX[] = "PROGRESS:";
+static constexpr char BUGZ_PROGRESS_SEPARATOR[] = "/";
// 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_() {
+ BugreportStandardStreamsCallback(const std::string& dest_file, bool show_progress, Bugreport* br)
+ : br_(br), dest_file_(dest_file), show_progress_(show_progress), status_(-1), line_() {
}
void OnStdout(const char* buffer, int length) {
- std::string output;
- OnStream(&output, stdout, buffer, length);
- stdout_str_.append(output);
+ for (int i = 0; i < length; i++) {
+ char c = buffer[i];
+ if (c == '\n') {
+ ProcessLine(line_);
+ line_.clear();
+ } else {
+ line_.append(1, c);
+ }
+ }
}
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;
+ // Process remaining line, if any...
+ ProcessLine(line_);
+ // ..then return.
+ return status_;
}
private:
+ void ProcessLine(const std::string& line) {
+ if (line.empty()) return;
+
+ if (android::base::StartsWith(line, BUGZ_OK_PREFIX)) {
+ if (show_progress_) {
+ // Make sure pull message doesn't conflict with generation message.
+ br_->UpdateProgress(dest_file_, 100, 100, true);
+ }
+
+ const char* zip_file = &line[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(line, BUGZ_FAIL_PREFIX)) {
+ const char* error_message = &line[strlen(BUGZ_FAIL_PREFIX)];
+ fprintf(stderr, "Device failed to take a zipped bugreport: %s\n", error_message);
+ status_ = -1;
+ } else if (show_progress_ && android::base::StartsWith(line, BUGZ_PROGRESS_PREFIX)) {
+ // progress_line should have the following format:
+ //
+ // BUGZ_PROGRESS_PREFIX:PROGRESS/TOTAL
+ //
+ size_t idx1 = line.rfind(BUGZ_PROGRESS_PREFIX) + strlen(BUGZ_PROGRESS_PREFIX);
+ size_t idx2 = line.rfind(BUGZ_PROGRESS_SEPARATOR);
+ int progress = std::stoi(line.substr(idx1, (idx2 - idx1)));
+ int total = std::stoi(line.substr(idx2 + 1));
+ br_->UpdateProgress(dest_file_, progress, total);
+ } else {
+ fprintf(stderr,
+ "WARNING: unexpected line (%s) returned by bugreportz, "
+ "device probably does not support zipped bugreports.\n"
+ "Try 'adb bugreport' instead.",
+ line.c_str());
+ }
+ }
+
Bugreport* br_;
const std::string dest_file_;
- std::string stdout_str_;
+ bool show_progress_;
+ int status_;
+
+ // Temporary buffer containing the characters read since the last newline
+ // (\n).
+ std::string line_;
DISALLOW_COPY_AND_ASSIGN(BugreportStandardStreamsCallback);
};
+// Implemented in commandline.cpp
+int usage();
+
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();
@@ -84,12 +121,50 @@
// TODO: use a case-insensitive comparison (like EndsWithIgnoreCase
dest_file += ".zip";
}
- 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");
- BugreportStandardStreamsCallback bugz_callback(dest_file, this);
- return SendShellCommand(transport_type, serial, "bugreportz", false, &bugz_callback);
+
+ // Gets bugreportz version.
+ std::string bugz_stderr;
+ DefaultStandardStreamsCallback version_callback(nullptr, &bugz_stderr);
+ int status = SendShellCommand(transport_type, serial, "bugreportz -v", false, &version_callback);
+
+ if (status != 0) {
+ fprintf(stderr,
+ "Failed to get bugreportz version: 'bugreport -v' returned '%s' "
+ "(code %d)."
+ "\nIf the device does not support it, try running 'adb bugreport' "
+ "to get a "
+ "flat-file bugreport.",
+ bugz_stderr.c_str(), status);
+ return status;
+ }
+ std::string bugz_version = android::base::Trim(bugz_stderr);
+
+ bool show_progress = true;
+ std::string bugz_command = "bugreportz -p";
+ if (bugz_version == "1.0") {
+ // 1.0 does not support progress notifications, so print a disclaimer
+ // message instead.
+ 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");
+ show_progress = false;
+ bugz_command = "bugreportz";
+ }
+ BugreportStandardStreamsCallback bugz_callback(dest_file, show_progress, this);
+ return SendShellCommand(transport_type, serial, bugz_command, false, &bugz_callback);
+}
+
+void Bugreport::UpdateProgress(const std::string& file_name, int progress, int total,
+ bool keep_info_line) {
+ int progress_percentage = (progress * 100 / total);
+ line_printer_.Print(android::base::StringPrintf("[%3d%%] generating %s", progress_percentage,
+ file_name.c_str()),
+ LinePrinter::INFO);
+ if (keep_info_line) {
+ line_printer_.KeepInfoLine();
+ }
}
int Bugreport::SendShellCommand(TransportType transport_type, const char* serial,
diff --git a/bugreport.h b/bugreport.h
index 1c39a9f..fd11a4a 100644
--- a/bugreport.h
+++ b/bugreport.h
@@ -21,12 +21,13 @@
#include "adb.h"
#include "commandline.h"
+#include "line_printer.h"
class Bugreport {
friend class BugreportStandardStreamsCallback;
public:
- Bugreport() {
+ Bugreport() : line_printer_() {
}
int DoIt(TransportType transport_type, const char* serial, int argc, const char** argv);
@@ -42,6 +43,9 @@
const char* name);
private:
+ virtual void UpdateProgress(const std::string& file_name, int progress, int total,
+ bool keep_info_line = false);
+ LinePrinter line_printer_;
DISALLOW_COPY_AND_ASSIGN(Bugreport);
};
diff --git a/bugreport_test.cpp b/bugreport_test.cpp
index 15c6f83..e695b24 100644
--- a/bugreport_test.cpp
+++ b/bugreport_test.cpp
@@ -51,37 +51,63 @@
return -42;
}
+enum StreamType {
+ kStreamStdout,
+ kStreamStderr,
+};
+
// 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) {
+ explicit OnStandardStreamsCallbackAction(StreamType type, const std::string& output)
+ : type_(type), output_(output) {
}
virtual Result Perform(const ArgumentTuple& args) {
- ::std::tr1::get<0>(args)->OnStdout(output_.c_str(), output_.size());
+ if (type_ == kStreamStdout) {
+ ::std::tr1::get<0>(args)->OnStdout(output_.c_str(), output_.size());
+ }
+ if (type_ == kStreamStderr) {
+ ::std::tr1::get<0>(args)->OnStderr(output_.c_str(), output_.size());
+ }
}
private:
+ StreamType type_;
std::string output_;
};
+// Matcher used to emulated StandardStreamsCallbackInterface.OnStdout(buffer,
+// length)
Action<OnStandardStreamsCallbackFunction> WriteOnStdout(const std::string& output) {
- return MakeAction(new OnStandardStreamsCallbackAction(output));
+ return MakeAction(new OnStandardStreamsCallbackAction(kStreamStdout, output));
+}
+
+// Matcher used to emulated StandardStreamsCallbackInterface.OnStderr(buffer,
+// length)
+Action<OnStandardStreamsCallbackFunction> WriteOnStderr(const std::string& output) {
+ return MakeAction(new OnStandardStreamsCallbackAction(kStreamStderr, output));
}
typedef int CallbackDoneFunction(StandardStreamsCallbackInterface*);
class CallbackDoneAction : public ActionInterface<CallbackDoneFunction> {
public:
+ explicit CallbackDoneAction(int status) : status_(status) {
+ }
virtual Result Perform(const ArgumentTuple& args) {
- int status = ::std::tr1::get<0>(args)->Done(123); // Value passed does not matter
+ int status = ::std::tr1::get<0>(args)->Done(status_);
return status;
}
+
+ private:
+ int status_;
};
-Action<CallbackDoneFunction> ReturnCallbackDone() {
- return MakeAction(new CallbackDoneAction());
+// Matcher used to emulated StandardStreamsCallbackInterface.Done(status)
+Action<CallbackDoneFunction> ReturnCallbackDone(int status = -1337) {
+ return MakeAction(new CallbackDoneAction(status));
}
class BugreportMock : public Bugreport {
@@ -91,10 +117,22 @@
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));
+ MOCK_METHOD4(UpdateProgress, void(const std::string&, int, int, bool));
};
class BugreportTest : public ::testing::Test {
public:
+ void SetBugreportzVersion(const std::string& version) {
+ EXPECT_CALL(br_,
+ SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz -v", false, _))
+ .WillOnce(DoAll(WithArg<4>(WriteOnStderr(version.c_str())),
+ WithArg<4>(ReturnCallbackDone(0))));
+ }
+
+ void ExpectProgress(int progress, int total, bool keep_info_line = false) {
+ EXPECT_CALL(br_, UpdateProgress(StrEq("file.zip"), progress, total, keep_info_line));
+ }
+
BugreportMock br_;
};
@@ -113,8 +151,10 @@
ASSERT_EQ(0, br_.DoIt(kTransportLocal, "HannibalLecter", 1, args));
}
-// Tests 'adb bugreport file.zip' when it succeeds
-TEST_F(BugreportTest, Ok) {
+// Tests 'adb bugreport file.zip' when it succeeds and device does not support
+// progress.
+TEST_F(BugreportTest, OkLegacy) {
+ SetBugreportzVersion("1.0");
EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
.WillOnce(DoAll(WithArg<4>(WriteOnStdout("OK:/device/bugreport.zip")),
WithArg<4>(ReturnCallbackDone())));
@@ -126,22 +166,10 @@
ASSERT_EQ(0, br_.DoIt(kTransportLocal, "HannibalLecter", 2, args));
}
-// Tests 'adb bugreport file' when it succeeds
-TEST_F(BugreportTest, OkNoExtension) {
- 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));
-
- const char* args[1024] = {"bugreport", "file"};
- 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) {
+// multiple buffer writers and without progress updates.
+TEST_F(BugreportTest, OkLegacySplitBuffer) {
+ SetBugreportzVersion("1.0");
EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
.WillOnce(DoAll(WithArg<4>(WriteOnStdout("OK:/device")),
WithArg<4>(WriteOnStdout("/bugreport.zip")),
@@ -154,35 +182,87 @@
ASSERT_EQ(0, br_.DoIt(kTransportLocal, "HannibalLecter", 2, args));
}
+// Tests 'adb bugreport file.zip' when it succeeds and displays progress.
+TEST_F(BugreportTest, Ok) {
+ SetBugreportzVersion("1.1");
+ ExpectProgress(1, 100);
+ ExpectProgress(10, 100);
+ ExpectProgress(50, 100);
+ ExpectProgress(99, 100);
+ ExpectProgress(100, 100, true);
+ // clang-format off
+ EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz -p", false, _))
+ .WillOnce(DoAll(
+ // Progress line in one write
+ WithArg<4>(WriteOnStdout("PROGRESS:1/100\n")),
+ // Add some bogus lines
+ WithArg<4>(WriteOnStdout("\nDUDE:SWEET\n\n")),
+ // Multiple progress lines in one write
+ WithArg<4>(WriteOnStdout("PROGRESS:10/100\nPROGRESS:50/100\n")),
+ // Progress line in multiple writes
+ WithArg<4>(WriteOnStdout("PROG")),
+ WithArg<4>(WriteOnStdout("RESS:99")),
+ WithArg<4>(WriteOnStdout("/100\n")),
+ // Split last message as well, just in case
+ WithArg<4>(WriteOnStdout("OK:/device/bugreport")),
+ WithArg<4>(WriteOnStdout(".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' when it succeeds
+TEST_F(BugreportTest, OkNoExtension) {
+ SetBugreportzVersion("1.1");
+ ExpectProgress(100, 100, true);
+ EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz -p", false, _))
+ .WillOnce(DoAll(WithArg<4>(WriteOnStdout("OK:/device/bugreport.zip\n")),
+ 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"};
+ 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, _))
- .WillOnce(DoAll(WithArg<4>(WriteOnStdout("FAIL:D'OH!")), WithArg<4>(ReturnCallbackDone())));
+ SetBugreportzVersion("1.1");
+ EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz -p", false, _))
+ .WillOnce(
+ DoAll(WithArg<4>(WriteOnStdout("FAIL:D'OH!\n")), 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"));
+ 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!")),
+ SetBugreportzVersion("1.1");
+ EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz -p", false, _))
+ .WillOnce(DoAll(WithArg<4>(WriteOnStdout("FAIL")), WithArg<4>(WriteOnStdout(":D'OH!\n")),
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"));
+ ASSERT_THAT(GetCapturedStderr(), HasSubstr("D'OH!"));
}
// Tests 'adb bugreport file.zip' when the bugreportz returned an unsupported
// response.
TEST_F(BugreportTest, BugreportzReturnedUnsupported) {
- EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
+ SetBugreportzVersion("1.1");
+ EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz -p", false, _))
.WillOnce(DoAll(WithArg<4>(WriteOnStdout("bugreportz? What am I, a zombie?")),
WithArg<4>(ReturnCallbackDone())));
@@ -192,9 +272,19 @@
ASSERT_THAT(GetCapturedStderr(), HasSubstr("bugreportz? What am I, a zombie?"));
}
-// Tests 'adb bugreport file.zip' when the bugreportz command fails
+// Tests 'adb bugreport file.zip' when the bugreportz -v command failed
+TEST_F(BugreportTest, BugreportzVersionFailed) {
+ EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz -v", false, _))
+ .WillOnce(Return(666));
+
+ const char* args[1024] = {"bugreport", "file.zip"};
+ ASSERT_EQ(666, br_.DoIt(kTransportLocal, "HannibalLecter", 2, args));
+}
+
+// Tests 'adb bugreport file.zip' when the main bugreportz command failed
TEST_F(BugreportTest, BugreportzFailed) {
- EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
+ SetBugreportzVersion("1.1");
+ EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz -p", false, _))
.WillOnce(Return(666));
const char* args[1024] = {"bugreport", "file.zip"};
@@ -203,7 +293,9 @@
// Tests 'adb bugreport file.zip' when the bugreport could not be pulled
TEST_F(BugreportTest, PullFails) {
- EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz", false, _))
+ SetBugreportzVersion("1.1");
+ ExpectProgress(100, 100, true);
+ EXPECT_CALL(br_, SendShellCommand(kTransportLocal, "HannibalLecter", "bugreportz -p", 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"),