Added a -p option to bugreportz to show progress.

BUG: 28609499

Change-Id: I1e60078dfda7e5679fbd19f4981a5dc7a69c4fc7

8 files changed, 84 insertions, 34 deletions

diff --git a/cmds/bugreportz/bugreportz.cpp b/cmds/bugreportz/bugreportz.cpp
index 4b81c910dc..7bed2840f9 100644
--- a/cmds/bugreportz/bugreportz.cpp
+++ b/cmds/bugreportz/bugreportz.cpp
@@ -20,9 +20,27 @@
 #include <string.h>
 #include <unistd.h>
 
+#include <string>
+
+#include <android-base/file.h>
+#include <android-base/strings.h>
+
 #include "bugreportz.h"
 
-int bugreportz(int s) {
+static constexpr char PROGRESS_PREFIX[] = "PROGRESS:";
+
+static void write_line(const std::string& line, bool show_progress) {
+    if (line.empty()) return;
+
+    // When not invoked with the -p option, it must skip PROGRESS lines otherwise it
+    // will break adb (which is expecting either OK or FAIL).
+    if (!show_progress && android::base::StartsWith(line, PROGRESS_PREFIX)) return;
+
+    android::base::WriteStringToFd(line, STDOUT_FILENO);
+}
+
+int bugreportz(int s, bool show_progress) {
+    std::string line;
     while (1) {
         char buffer[65536];
         ssize_t bytes_read = TEMP_FAILURE_RETRY(read(s, buffer, sizeof(buffer)));
@@ -37,21 +55,18 @@ int bugreportz(int s) {
             break;
         }
 
-        ssize_t bytes_to_send = bytes_read;
-        ssize_t bytes_written;
-        do {
-            bytes_written = TEMP_FAILURE_RETRY(
-                write(STDOUT_FILENO, buffer + bytes_read - bytes_to_send, bytes_to_send));
-            if (bytes_written == -1) {
-                fprintf(stderr,
-                        "Failed to write data to stdout: read %zd, trying to send %zd "
-                        "(%s)\n",
-                        bytes_read, bytes_to_send, strerror(errno));
-                break;
+        // Writes line by line.
+        for (int i = 0; i < bytes_read; i++) {
+            char c = buffer[i];
+            line.append(1, c);
+            if (c == '\n') {
+                write_line(line, show_progress);
+                line.clear();
             }
-            bytes_to_send -= bytes_written;
-        } while (bytes_written != 0 && bytes_to_send > 0);
+        }
     }
+    // Process final line, in case it didn't finish with newline
+    write_line(line, show_progress);
 
     if (close(s) == -1) {
         fprintf(stderr, "WARNING: error closing socket: %s\n", strerror(errno));
diff --git a/cmds/bugreportz/bugreportz.h b/cmds/bugreportz/bugreportz.h
index d2b79b92f4..304e4b3dc3 100644
--- a/cmds/bugreportz/bugreportz.h
+++ b/cmds/bugreportz/bugreportz.h
@@ -16,6 +16,6 @@
 #define BUGREPORTZ_H
 
 // Calls dumpstate using the given socket and output its result to stdout.
-int bugreportz(int s);
+int bugreportz(int s, bool show_progress);
 
 #endif  // BUGREPORTZ_H
diff --git a/cmds/bugreportz/bugreportz_test.cpp b/cmds/bugreportz/bugreportz_test.cpp
index fb6cdc76e5..dfef17f590 100644
--- a/cmds/bugreportz/bugreportz_test.cpp
+++ b/cmds/bugreportz/bugreportz_test.cpp
@@ -70,12 +70,12 @@ class BugreportzTest : public ::testing::Test {
     // Tests must call WriteToSocket() to set what's written prior to calling it, since the writing
     // end of the pipe will be closed before calling bugreportz() (otherwise that function would
     // hang).
-    void Bugreportz() {
+    void Bugreportz(bool show_progress) {
         close(write_fd_);
         write_fd_ = -1;
 
         CaptureStdout();
-        int status = bugreportz(read_fd_);
+        int status = bugreportz(read_fd_, show_progress);
 
         close(read_fd_);
         read_fd_ = -1;
@@ -90,12 +90,36 @@ class BugreportzTest : public ::testing::Test {
     std::string stdout_;
 };
 
-// Tests 'bugreportz', without any argument - it will just echo dumpstate's output to stdout.
+// Tests 'bugreportz', without any argument - it will ignore progress lines.
 TEST_F(BugreportzTest, NoArgument) {
     WriteToSocket("What happens on 'dumpstate',");
     WriteToSocket("stays on 'bugreportz'.\n");
+    WriteToSocket("PROGRESS:Y U NO OMITTED?\n");  // Should be ommited.
+    WriteToSocket("But ");
+    WriteToSocket("PROGRESS IN THE MIDDLE");  // Ok - not starting a line.
+    WriteToSocket(" is accepted\n");
 
-    Bugreportz();
+    Bugreportz(false);
 
-    AssertStdoutEquals("What happens on 'dumpstate',stays on 'bugreportz'.\n");
+    AssertStdoutEquals(
+        "What happens on 'dumpstate',stays on 'bugreportz'.\n"
+        "But PROGRESS IN THE MIDDLE is accepted\n");
+}
+
+// Tests 'bugreportz -p' - it will just echo dumpstate's output to stdout
+TEST_F(BugreportzTest, WithProgress) {
+    WriteToSocket("What happens on 'dumpstate',");
+    WriteToSocket("stays on 'bugreportz'.\n");
+    WriteToSocket("PROGRESS:IS INEVITABLE\n");
+    WriteToSocket("PROG");
+    WriteToSocket("RESS:IS NOT AUTOMATIC\n");
+    WriteToSocket("Newline is optional");
+
+    Bugreportz(true);
+
+    AssertStdoutEquals(
+        "What happens on 'dumpstate',stays on 'bugreportz'.\n"
+        "PROGRESS:IS INEVITABLE\n"
+        "PROGRESS:IS NOT AUTOMATIC\n"
+        "Newline is optional");
 }
diff --git a/cmds/bugreportz/main.cpp b/cmds/bugreportz/main.cpp
index 6fa33cc5ab..a3ae1ffa4d 100644
--- a/cmds/bugreportz/main.cpp
+++ b/cmds/bugreportz/main.cpp
@@ -26,12 +26,13 @@
 
 #include "bugreportz.h"
 
-static constexpr char VERSION[] = "1.0";
+static constexpr char VERSION[] = "1.1";
 
 static void show_usage() {
     fprintf(stderr,
             "usage: bugreportz [-h | -v]\n"
             "  -h: to display this help message\n"
+            "  -p: display progress\n"
             "  -v: to display the version\n"
             "  or no arguments to generate a zipped bugreport\n");
 }
@@ -41,14 +42,18 @@ static void show_version() {
 }
 
 int main(int argc, char* argv[]) {
+    bool show_progress = false;
     if (argc > 1) {
         /* parse arguments */
         int c;
-        while ((c = getopt(argc, argv, "vh")) != -1) {
+        while ((c = getopt(argc, argv, "hpv")) != -1) {
             switch (c) {
                 case 'h':
                     show_usage();
                     return EXIT_SUCCESS;
+                case 'p':
+                    show_progress = true;
+                    break;
                 case 'v':
                     show_version();
                     return EXIT_SUCCESS;
@@ -57,11 +62,6 @@ int main(int argc, char* argv[]) {
                     return EXIT_FAILURE;
             }
         }
-        // passed an argument not starting with -
-        if (optind > 1 || argv[optind] != nullptr) {
-            show_usage();
-            return EXIT_FAILURE;
-        }
     }
 
     // TODO: code below was copy-and-pasted from bugreport.cpp (except by the
@@ -95,5 +95,5 @@ int main(int argc, char* argv[]) {
         fprintf(stderr, "WARNING: Cannot set socket timeout: %s\n", strerror(errno));
     }
 
-    bugreportz(s);
+    bugreportz(s, show_progress);
 }
diff --git a/cmds/bugreportz/readme.md b/cmds/bugreportz/readme.md
index 85aafcefe4..2bc277e708 100644
--- a/cmds/bugreportz/readme.md
+++ b/cmds/bugreportz/readme.md
@@ -3,6 +3,11 @@
 `bugreportz` is used to generate a zippped bugreport whose path is passed back to `adb`, using
 the simple protocol defined below.
 
+# Version 1.1
+On version 1.1, in addition to the `OK` and `FAILURE` lines, `bugreportz -p` generates progress
+lines in the following format:
+
+- `PROGRESS:<progress>/<total>`, where `<progress>` is the current progress units out of a max of `<total>`.
 
 ## Version 1.0
 On version 1.0, `bugreportz` does not generate any output on `stdout` until the bugreport is
diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp
index 279c01030c..f74fe399d9 100644
--- a/cmds/dumpstate/dumpstate.cpp
+++ b/cmds/dumpstate/dumpstate.cpp
@@ -61,7 +61,7 @@ static time_t now;
 static std::unique_ptr<ZipWriter> zip_writer;
 static std::set<std::string> mount_points;
 void add_mountinfo();
-static int control_socket_fd;
+int control_socket_fd = -1;
 /* suffix of the bugreport files - it's typically the date (when invoked with -d),
  * although it could be changed by the user using a system property */
 static std::string suffix;
@@ -1196,6 +1196,7 @@ int main(int argc, char *argv[]) {
     if (use_control_socket) {
         MYLOGD("Opening control socket\n");
         control_socket_fd = open_socket("dumpstate");
+        do_update_progress = 1;
     }
 
     /* full path of the temporary file containing the bugreport */
@@ -1268,7 +1269,7 @@ int main(int argc, char *argv[]) {
             add_text_zip_entry("version.txt", version);
         }
 
-        if (do_update_progress) {
+        if (do_update_progress && do_broadcast) {
             std::vector<std::string> am_args = {
                  "--receiver-permission", "android.permission.DUMP", "--receiver-foreground",
                  "--es", "android.intent.extra.NAME", suffix,
@@ -1492,9 +1493,9 @@ int main(int argc, char *argv[]) {
         fclose(stderr);
     }
 
-    if (use_control_socket && control_socket_fd >= 0) {
-        MYLOGD("Closing control socket\n");
-        close(control_socket_fd);
+    if (use_control_socket && control_socket_fd != -1) {
+      MYLOGD("Closing control socket\n");
+      close(control_socket_fd);
     }
 
     return 0;
diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h
index 4769974e0d..5e083cc002 100644
--- a/cmds/dumpstate/dumpstate.h
+++ b/cmds/dumpstate/dumpstate.h
@@ -79,7 +79,7 @@ static const int WEIGHT_FILE = 5;
  * It would be better to take advantage of the C++ migration and encapsulate the state in an object,
  * but that will be better handled in a major C++ refactoring, which would also get rid of other C
  * idioms (like using std::string instead of char*, removing varargs, etc...) */
-extern int do_update_progress, progress, weight_total;
+extern int do_update_progress, progress, weight_total, control_socket_fd;
 
 /* full path of the directory where the bugreport files will be written */
 extern std::string bugreport_dir;
diff --git a/cmds/dumpstate/utils.cpp b/cmds/dumpstate/utils.cpp
index 3c39129b14..fd6413d562 100644
--- a/cmds/dumpstate/utils.cpp
+++ b/cmds/dumpstate/utils.cpp
@@ -1214,6 +1214,11 @@ void update_progress(int delta) {
         fprintf(stderr, "Setting progress (%s): %s/%d\n", key, value, weight_total);
     }
 
+    if (control_socket_fd >= 0) {
+        dprintf(control_socket_fd, "PROGRESS:%d/%d\n", progress, weight_total);
+        fsync(control_socket_fd);
+    }
+
     int status = property_set(key, value);
     if (status) {
         MYLOGE("Could not update progress by setting system property %s to %s: %d\n",