Adds a -P option so dumpstate can report its progress.
The old workflow was:
1. dumpstate starts.
2. When dumpstate finishes, it sends a BUGREPORT_FINISHED event.
3. Shell's BugreportReceiver receives the BUGREPORT_FINISHED and issues a
system notification so user can share the bug report.
The new workflow is:
1. When dumpstate starts, it sends a BUGREPORT_STARTED with its pid and
the estimated total effort.
2. When Shell's BugreportReceiver receives the BUGREPORT_STARTED, it:
2.1 Issues a system notification so user can watch the
progresss (which is 0% initially).
2.2 Starts a service (BugreportProgressService) responsible for
polling the dumpstate progress (using system properties and the
pid) and updating the system notification.
3. As dumpstate progress, it updates the proper system property.
4. When dumpstate finishes, it sends a BUGREPORT_FINISHED event.
5. When Shell's BugreportReceiver receives the BUGREPORT_FINISHED, it:
5.1 Finishes the service if necessary.
5.2 Issues a system notification so user can share the bug report.
This CL handles the dumpstate changes only, the Shell changes will be
handled in a separate CL.
BUG: 25794470
Change-Id: I6e04203411802c88ab0d093420ccdfd26700d565
diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp
index 4703c2f..61f8365 100644
--- a/cmds/dumpstate/dumpstate.cpp
+++ b/cmds/dumpstate/dumpstate.cpp
@@ -602,6 +602,7 @@
" -e: play sound file instead of vibrate, at end of job\n"
" -q: disable vibrate\n"
" -B: send broadcast when finished (requires -o)\n"
+ " -P: send broadacast when started and update system properties on progress (requires -o and -B)\n"
);
}
@@ -712,16 +713,17 @@
/* parse arguments */
int c;
- while ((c = getopt(argc, argv, "dho:svqzpB")) != -1) {
+ while ((c = getopt(argc, argv, "dho:svqzpPB")) != -1) {
switch (c) {
- case 'd': do_add_date = 1; break;
- case 'z': do_zip_file = 1; break;
- case 'o': use_outfile = optarg; break;
- case 's': use_socket = 1; break;
+ case 'd': do_add_date = 1; break;
+ case 'z': do_zip_file = 1; break;
+ case 'o': use_outfile = optarg; break;
+ case 's': use_socket = 1; break;
case 'v': break; // compatibility no-op
- case 'q': do_vibrate = 0; break;
- case 'p': do_fb = 1; break;
- case 'B': do_broadcast = 1; break;
+ case 'q': do_vibrate = 0; break;
+ case 'p': do_fb = 1; break;
+ case 'P': do_update_progress = 1; break;
+ case 'B': do_broadcast = 1; break;
case '?': printf("\n");
case 'h':
usage();
@@ -729,11 +731,15 @@
}
}
- if ((do_zip_file || do_add_date || do_broadcast) && !use_outfile) {
+ if ((do_zip_file || do_add_date || do_update_progress || do_broadcast) && !use_outfile) {
usage();
exit(1);
}
+ if (do_update_progress && !do_broadcast) {
+ usage();
+ exit(1);
+ }
// If we are going to use a socket, do it as early as possible
// to avoid timeouts from bugreport.
@@ -741,6 +747,49 @@
redirect_to_socket(stdout, "dumpstate");
}
+ /* redirect output if needed */
+ std::string text_path, zip_path, tmp_path, entry_name;
+
+ /* pointer to the actual path, be it zip or text */
+ std::string path;
+
+ time_t now = time(NULL);
+
+ bool is_redirecting = !use_socket && use_outfile;
+
+ if (is_redirecting) {
+ text_path = use_outfile;
+ if (do_add_date) {
+ char date[80];
+ strftime(date, sizeof(date), "-%Y-%m-%d-%H-%M-%S", localtime(&now));
+ text_path += date;
+ }
+ if (do_fb) {
+ screenshot_path = text_path + ".png";
+ }
+ zip_path = text_path + ".zip";
+ text_path += ".txt";
+ tmp_path = text_path + ".tmp";
+ entry_name = basename(text_path.c_str());
+
+ ALOGD("Temporary path: %s\ntext path: %s\nzip path: %s\nzip entry: %s",
+ tmp_path.c_str(), text_path.c_str(), zip_path.c_str(), entry_name.c_str());
+
+ if (do_update_progress) {
+ if (!entry_name.empty()) {
+ std::vector<std::string> am_args = {
+ "--receiver-permission", "android.permission.DUMP",
+ "--es", "android.intent.extra.NAME", entry_name,
+ "--ei", "android.intent.extra.PID", std::to_string(getpid()),
+ "--ei", "android.intent.extra.MAX", std::to_string(WEIGHT_TOTAL),
+ };
+ send_broadcast("android.intent.action.BUGREPORT_STARTED", am_args);
+ } else {
+ ALOGE("Skipping started broadcast because entry name could not be inferred\n");
+ }
+ }
+ }
+
/* open the vibrator before dropping root */
std::unique_ptr<FILE, int(*)(FILE*)> vibrator(NULL, fclose);
if (do_vibrate) {
@@ -802,29 +851,7 @@
return -1;
}
- /* redirect output if needed */
- std::string text_path, zip_path, tmp_path, entry_name;
-
- /* pointer to the actual path, be it zip or text */
- std::string path;
-
- time_t now = time(NULL);
-
- if (!use_socket && use_outfile) {
- text_path = use_outfile;
- if (do_add_date) {
- char date[80];
- strftime(date, sizeof(date), "-%Y-%m-%d-%H-%M-%S", localtime(&now));
- text_path += date;
- }
- if (do_fb) {
- screenshot_path = text_path + ".png";
- }
- zip_path = text_path + ".zip";
- text_path += ".txt";
- tmp_path = text_path + ".tmp";
- entry_name = basename(text_path.c_str());
-
+ if (is_redirecting) {
ALOGD("Temporary path: %s\ntext path: %s\nzip path: %s\nzip entry: %s",
tmp_path.c_str(), text_path.c_str(), zip_path.c_str(), entry_name.c_str());
/* TODO: rather than generating a text file now and zipping it later,
@@ -844,7 +871,7 @@
}
/* close output if needed */
- if (!use_socket && use_outfile) {
+ if (is_redirecting) {
fclose(stdout);
}
@@ -870,11 +897,12 @@
}
/* tell activity manager we're done */
- if (do_broadcast && use_outfile) {
+ if (do_broadcast) {
if (!path.empty()) {
ALOGI("Final bugreport path: %s\n", path.c_str());
std::vector<std::string> am_args = {
"--receiver-permission", "android.permission.DUMP",
+ "--ei", "android.intent.extra.PID", std::to_string(getpid()),
"--es", "android.intent.extra.BUGREPORT", path
};
if (do_fb) {
@@ -884,7 +912,7 @@
}
send_broadcast("android.intent.action.BUGREPORT_FINISHED", am_args);
} else {
- ALOGE("Skipping broadcast because bugreport could not be generated\n");
+ ALOGE("Skipping finished broadcast because bugreport could not be generated\n");
}
}
diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h
index 18ee168..2ba5ccb 100644
--- a/cmds/dumpstate/dumpstate.h
+++ b/cmds/dumpstate/dumpstate.h
@@ -45,6 +45,28 @@
typedef void (for_each_pid_func)(int, const char *);
typedef void (for_each_tid_func)(int, int, const char *);
+/* Estimated total weight of bugreport generation.
+ *
+ * Each section contributes to the total weight by an individual weight, so the overall progress
+ * can be calculated by dividing the all completed weight by the total weight.
+ *
+ * This value is defined empirically and it need to be adjusted as more sections are added.
+ * It does not need to match the exact sum of all sections, but it should to be more than it,
+ * otherwise the calculated progress would be more than 100%.
+ */
+static const int WEIGHT_TOTAL = 4000;
+
+/* Most simple commands have 10 as timeout, so 5 is a good estimate */
+static const int WEIGHT_FILE = 5;
+
+/*
+ * TOOD: the dumpstate internal state is getting fragile; for example, this variable is defined
+ * here, declared at utils.cpp, and used on utils.cpp and dumpstate.cpp.
+ * 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;
+
/* prints the contents of a file */
int dump_file(const char *title, const char *path);
@@ -74,6 +96,9 @@
/* sends a broadcast using Activity Manager */
void send_broadcast(const std::string& action, const std::vector<std::string>& args);
+/* updates the overall progress of dumpstate by the given weight increment */
+void update_progress(int weight);
+
/* prints all the system properties */
void print_properties();
diff --git a/cmds/dumpstate/utils.cpp b/cmds/dumpstate/utils.cpp
index 4316c96..0958aef 100644
--- a/cmds/dumpstate/utils.cpp
+++ b/cmds/dumpstate/utils.cpp
@@ -35,7 +35,9 @@
#include <vector>
#include <sys/prctl.h>
+#define LOG_TAG "dumpstate"
#include <cutils/debugger.h>
+#include <cutils/log.h>
#include <cutils/properties.h>
#include <cutils/sockets.h>
#include <private/android_filesystem_config.h>
@@ -266,7 +268,7 @@
}
printf(") ------\n");
}
- ON_DRY_RUN({ close(fd); return 0; });
+ ON_DRY_RUN({ update_progress(WEIGHT_FILE); close(fd); return 0; });
bool newline = false;
fd_set read_set;
@@ -304,6 +306,7 @@
}
}
}
+ update_progress(WEIGHT_FILE);
close(fd);
if (!newline) printf("\n");
@@ -455,7 +458,6 @@
return true;
}
-/* forks a command and waits for it to finish */
int run_command(const char *title, int timeout_seconds, const char *command, ...) {
fflush(stdout);
@@ -472,13 +474,19 @@
if (title) printf(") ------\n");
fflush(stdout);
- ON_DRY_RUN_RETURN(0);
+ ON_DRY_RUN({ update_progress(timeout_seconds); va_end(ap); return 0; });
- return run_command_always(title, timeout_seconds, args);
+ int status = run_command_always(title, timeout_seconds, args);
+ va_end(ap);
+ return status;
}
/* forks a command and waits for it to finish */
int run_command_always(const char *title, int timeout_seconds, const char *args[]) {
+ /* TODO: for now we're simplifying the progress calculation by using the timeout as the weight.
+ * It's a good approximation for most cases, except when calling dumpsys, where its weight
+ * should be much higher proportionally to its timeout. */
+ int weight = timeout_seconds;
const char *command = args[0];
uint64_t start = nanotime();
@@ -537,6 +545,9 @@
}
if (title) printf("[%s: %.3fs elapsed]\n\n", command, (float)elapsed / NANOS_PER_SEC);
+ if (weight > 0) {
+ update_progress(weight);
+ }
return status;
}
@@ -826,3 +837,29 @@
}
fclose(fp);
}
+
+/* overall progress */
+int progress = 0;
+int do_update_progress = 1; // Set by dumpstate.cpp
+
+// TODO: make this function thread safe if sections are generated in parallel.
+void update_progress(int delta) {
+ if (!do_update_progress) return;
+
+ progress += delta;
+
+ char key[PROPERTY_KEY_MAX];
+ char value[PROPERTY_VALUE_MAX];
+ sprintf(key, "dumpstate.%d.progress", getpid());
+ sprintf(value, "%d", progress);
+
+ // stderr is ignored on normal invocations, but useful when calling /system/bin/dumpstate
+ // directly for debuggging.
+ fprintf(stderr, "Setting progress (%s): %s/%d\n", key, value, WEIGHT_TOTAL);
+
+ int status = property_set(key, value);
+ if (status) {
+ ALOGW("Could not update progress by setting system property %s to %s: %d\n",
+ key, value, status);
+ }
+}