Streaming bugreport content to stdout 2/2
- Stream compressed content via the socket from dumpstate
- Remove do_zip_file, do_add_data vars in dumpstate
- getopt '-d' and '-z' options are now no-op
- Rename use_control_socket -> progress_updates_to_socket
- Rename use_socket -> stream_to_socket
do_zip_file and do_add_data are now default behaviors.
Also adding a function interface to hook dumpstate `open_socket`
function that would make it easy to do integration tests of streaming
zipped bugreport via socket. Stub the function and capture data from
target file to verify content.
Bug: 162910469
Test: adb bugreport --stream > test.zip; 7z t test.zip
Test: adb shell bugreportz -s > test.zip; 7z t test.zip
Test: atest dumpstate_test
Test: atest dumpstate_test:ZippedBugReportStreamTest
Test: atest dumpstate_smoke_test
Test: manually test combo keys voldown+volup+power to trigger bugreport
verify logcat kernel buffer shows "Starting service 'bugreport'
from keychord 114 115 116" and device viberated as expected
Change-Id: Icf79bbc42a5616e85625bd604e543fbf973911da
diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp
index 616304c..67527b2 100644
--- a/cmds/dumpstate/dumpstate.cpp
+++ b/cmds/dumpstate/dumpstate.cpp
@@ -719,6 +719,9 @@
return timeout_ms > MINIMUM_LOGCAT_TIMEOUT_MS ? timeout_ms : MINIMUM_LOGCAT_TIMEOUT_MS;
}
+// Opens a socket and returns its file descriptor.
+static int open_socket(const char* service);
+
Dumpstate::ConsentCallback::ConsentCallback() : result_(UNAVAILABLE), start_time_(Nanotime()) {
}
@@ -2286,20 +2289,18 @@
static void ShowUsage() {
fprintf(stderr,
- "usage: dumpstate [-h] [-b soundfile] [-e soundfile] [-o directory] [-d] [-p] "
- "[-z] [-s] [-S] [-q] [-P] [-R] [-L] [-V version]\n"
+ "usage: dumpstate [-h] [-b soundfile] [-e soundfile] [-o directory] [-p] "
+ "[-s] [-S] [-q] [-P] [-R] [-L] [-V version]\n"
" -h: display this help message\n"
" -b: play sound file instead of vibrate, at beginning of job\n"
" -e: play sound file instead of vibrate, at end of job\n"
" -o: write to custom directory (only in limited mode)\n"
- " -d: append date to filename\n"
" -p: capture screenshot to filename.png\n"
- " -z: generate zipped file\n"
- " -s: write output to control socket (for init)\n"
- " -S: write file location to control socket (for init; requires -z)\n"
+ " -s: write zipped file to control socket (for init)\n"
+ " -S: write file location to control socket (for init)\n"
" -q: disable vibrate\n"
" -P: send broadcast when started and do progress updates\n"
- " -R: take bugreport in remote mode (requires -z and -d, shouldn't be used with -P)\n"
+ " -R: take bugreport in remote mode (shouldn't be used with -P)\n"
" -w: start binder service and make it wait for a call to startBugreport\n"
" -L: output limited information that is safe for submission in feedback reports\n"
" -v: prints the dumpstate header and exit\n");
@@ -2398,21 +2399,17 @@
/*
* Prepares state like filename, screenshot path, etc in Dumpstate. Also initializes ZipWriter
- * if we are writing zip files and adds the version file.
+ * and adds the version file. Return false if zip_file could not be open to write.
*/
-static void PrepareToWriteToFile() {
+static bool PrepareToWriteToFile() {
MaybeResolveSymlink(&ds.bugreport_internal_dir_);
std::string build_id = android::base::GetProperty("ro.build.id", "UNKNOWN_BUILD");
std::string device_name = android::base::GetProperty("ro.product.name", "UNKNOWN_DEVICE");
ds.base_name_ = StringPrintf("bugreport-%s-%s", device_name.c_str(), build_id.c_str());
- if (ds.options_->do_add_date) {
- char date[80];
- strftime(date, sizeof(date), "%Y-%m-%d-%H-%M-%S", localtime(&ds.now_));
- ds.name_ = date;
- } else {
- ds.name_ = "undated";
- }
+ char date[80];
+ strftime(date, sizeof(date), "%Y-%m-%d-%H-%M-%S", localtime(&ds.now_));
+ ds.name_ = date;
if (ds.options_->telephony_only) {
ds.base_name_ += "-telephony";
@@ -2439,18 +2436,17 @@
destination.c_str(), ds.base_name_.c_str(), ds.name_.c_str(), ds.log_path_.c_str(),
ds.tmp_path_.c_str(), ds.screenshot_path_.c_str());
- if (ds.options_->do_zip_file) {
- ds.path_ = ds.GetPath(ds.CalledByApi() ? "-zip.tmp" : ".zip");
- MYLOGD("Creating initial .zip file (%s)\n", ds.path_.c_str());
- create_parent_dirs(ds.path_.c_str());
- ds.zip_file.reset(fopen(ds.path_.c_str(), "wb"));
- if (ds.zip_file == nullptr) {
- MYLOGE("fopen(%s, 'wb'): %s\n", ds.path_.c_str(), strerror(errno));
- } else {
- ds.zip_writer_.reset(new ZipWriter(ds.zip_file.get()));
- }
- ds.AddTextZipEntry("version.txt", ds.version_);
+ ds.path_ = ds.GetPath(ds.CalledByApi() ? "-zip.tmp" : ".zip");
+ MYLOGD("Creating initial .zip file (%s)\n", ds.path_.c_str());
+ create_parent_dirs(ds.path_.c_str());
+ ds.zip_file.reset(fopen(ds.path_.c_str(), "wb"));
+ if (ds.zip_file == nullptr) {
+ MYLOGE("fopen(%s, 'wb'): %s\n", ds.path_.c_str(), strerror(errno));
+ return false;
}
+ ds.zip_writer_.reset(new ZipWriter(ds.zip_file.get()));
+ ds.AddTextZipEntry("version.txt", ds.version_);
+ return true;
}
/*
@@ -2458,14 +2454,9 @@
* printing zipped file status, etc.
*/
static void FinalizeFile() {
- bool do_text_file = true;
- if (ds.options_->do_zip_file) {
- if (!ds.FinishZipFile()) {
- MYLOGE("Failed to finish zip file; sending text bugreport instead\n");
- do_text_file = true;
- } else {
- do_text_file = false;
- }
+ bool do_text_file = !ds.FinishZipFile();
+ if (do_text_file) {
+ MYLOGE("Failed to finish zip file; sending text bugreport instead\n");
}
std::string final_path = ds.path_;
@@ -2474,7 +2465,9 @@
android::os::CopyFileToFile(ds.path_, final_path);
}
- if (ds.options_->use_control_socket) {
+ if (ds.options_->stream_to_socket) {
+ android::os::CopyFileToFd(ds.path_, ds.control_socket_fd_);
+ } else if (ds.options_->progress_updates_to_socket) {
if (do_text_file) {
dprintf(ds.control_socket_fd_,
"FAIL:could not create zip file, check %s "
@@ -2530,7 +2523,6 @@
break;
case Dumpstate::BugreportMode::BUGREPORT_WEAR:
options->do_progress_updates = true;
- options->do_zip_file = true;
options->do_screenshot = is_screenshot_requested;
options->dumpstate_hal_mode = DumpstateMode::WEAR;
break;
@@ -2543,7 +2535,6 @@
break;
case Dumpstate::BugreportMode::BUGREPORT_WIFI:
options->wifi_only = true;
- options->do_zip_file = true;
options->do_screenshot = false;
options->dumpstate_hal_mode = DumpstateMode::WIFI;
break;
@@ -2554,11 +2545,11 @@
static void LogDumpOptions(const Dumpstate::DumpOptions& options) {
MYLOGI(
- "do_zip_file: %d do_vibrate: %d use_socket: %d use_control_socket: %d do_screenshot: %d "
+ "do_vibrate: %d stream_to_socket: %d progress_updates_to_socket: %d do_screenshot: %d "
"is_remote_mode: %d show_header_only: %d telephony_only: %d "
"wifi_only: %d do_progress_updates: %d fd: %d bugreport_mode: %s dumpstate_hal_mode: %s "
"limited_only: %d args: %s\n",
- options.do_zip_file, options.do_vibrate, options.use_socket, options.use_control_socket,
+ options.do_vibrate, options.stream_to_socket, options.progress_updates_to_socket,
options.do_screenshot, options.is_remote_mode, options.show_header_only,
options.telephony_only, options.wifi_only,
options.do_progress_updates, options.bugreport_fd.get(), options.bugreport_mode.c_str(),
@@ -2569,11 +2560,6 @@
const android::base::unique_fd& bugreport_fd_in,
const android::base::unique_fd& screenshot_fd_in,
bool is_screenshot_requested) {
- // In the new API world, date is always added; output is always a zip file.
- // TODO(111441001): remove these options once they are obsolete.
- do_add_date = true;
- do_zip_file = true;
-
// Duplicate the fds because the passed in fds don't outlive the binder transaction.
bugreport_fd.reset(dup(bugreport_fd_in.get()));
screenshot_fd.reset(dup(screenshot_fd_in.get()));
@@ -2587,18 +2573,20 @@
while ((c = getopt(argc, argv, "dho:svqzpLPBRSV:w")) != -1) {
switch (c) {
// clang-format off
- case 'd': do_add_date = true; break;
- case 'z': do_zip_file = true; break;
case 'o': out_dir = optarg; break;
- case 's': use_socket = true; break;
- case 'S': use_control_socket = true; break;
+ case 's': stream_to_socket = true; break;
+ case 'S': progress_updates_to_socket = true; break;
case 'v': show_header_only = true; break;
case 'q': do_vibrate = false; break;
case 'p': do_screenshot = true; break;
case 'P': do_progress_updates = true; break;
case 'R': is_remote_mode = true; break;
case 'L': limited_only = true; break;
- case 'V': break; // compatibility no-op
+ case 'V':
+ case 'd':
+ case 'z':
+ // compatibility no-op
+ break;
case 'w':
// This was already processed
break;
@@ -2627,19 +2615,15 @@
}
bool Dumpstate::DumpOptions::ValidateOptions() const {
- if (bugreport_fd.get() != -1 && !do_zip_file) {
+ if (bugreport_fd.get() != -1 && stream_to_socket) {
return false;
}
- if ((do_zip_file || do_add_date || do_progress_updates) && !OutputToFile()) {
+ if ((progress_updates_to_socket || do_progress_updates) && stream_to_socket) {
return false;
}
- if (use_control_socket && !do_zip_file) {
- return false;
- }
-
- if (is_remote_mode && (do_progress_updates || !do_zip_file || !do_add_date)) {
+ if (is_remote_mode && (do_progress_updates || stream_to_socket)) {
return false;
}
return true;
@@ -2714,11 +2698,9 @@
* The temporary bugreport is then populated via printfs, dumping contents of files and
* output of commands to stdout.
*
- * If zipping, the temporary bugreport file is added to the zip archive. Else it's renamed to final
- * text file.
+ * A bunch of other files and dumps are added to the zip archive.
*
- * If zipping, a bunch of other files and dumps also get added to the zip archive. The log file also
- * gets added to the archive.
+ * The temporary bugreport file and the log file also get added to the archive.
*
* Bugreports are first generated in a local directory and later copied to the caller's fd
* or directory if supplied.
@@ -2766,14 +2748,9 @@
MYLOGD("dumpstate calling_uid = %d ; calling package = %s \n",
calling_uid, calling_package.c_str());
- // Redirect output if needed
- bool is_redirecting = options_->OutputToFile();
-
// TODO: temporarily set progress until it's part of the Dumpstate constructor
std::string stats_path =
- is_redirecting
- ? android::base::StringPrintf("%s/dumpstate-stats.txt", bugreport_internal_dir_.c_str())
- : "";
+ android::base::StringPrintf("%s/dumpstate-stats.txt", bugreport_internal_dir_.c_str());
progress_.reset(new Progress(stats_path));
if (acquire_wake_lock(PARTIAL_WAKE_LOCK, WAKE_LOCK_NAME) < 0) {
@@ -2796,35 +2773,33 @@
// If we are going to use a socket, do it as early as possible
// to avoid timeouts from bugreport.
- if (options_->use_socket) {
- if (!redirect_to_socket(stdout, "dumpstate")) {
- return ERROR;
- }
- }
-
- if (options_->use_control_socket) {
+ if (options_->stream_to_socket || options_->progress_updates_to_socket) {
MYLOGD("Opening control socket\n");
- control_socket_fd_ = open_socket("dumpstate");
+ control_socket_fd_ = open_socket_fn_("dumpstate");
if (control_socket_fd_ == -1) {
return ERROR;
}
- options_->do_progress_updates = 1;
+ if (options_->progress_updates_to_socket) {
+ options_->do_progress_updates = 1;
+ }
}
- if (is_redirecting) {
- PrepareToWriteToFile();
+ if (!PrepareToWriteToFile()) {
+ return ERROR;
+ }
- if (options_->do_progress_updates) {
- // clang-format off
- std::vector<std::string> am_args = {
- "--receiver-permission", "android.permission.DUMP",
- };
- // clang-format on
- // Send STARTED broadcast for apps that listen to bugreport generation events
- SendBroadcast("com.android.internal.intent.action.BUGREPORT_STARTED", am_args);
- if (options_->use_control_socket) {
- dprintf(control_socket_fd_, "BEGIN:%s\n", path_.c_str());
- }
+ // Interactive, wear & telephony modes are default to true.
+ // and may enable from cli option or when using control socket
+ if (options_->do_progress_updates) {
+ // clang-format off
+ std::vector<std::string> am_args = {
+ "--receiver-permission", "android.permission.DUMP",
+ };
+ // clang-format on
+ // Send STARTED broadcast for apps that listen to bugreport generation events
+ SendBroadcast("com.android.internal.intent.action.BUGREPORT_STARTED", am_args);
+ if (options_->progress_updates_to_socket) {
+ dprintf(control_socket_fd_, "BEGIN:%s\n", path_.c_str());
}
}
@@ -2839,40 +2814,38 @@
Vibrate(150);
}
- if (options_->do_zip_file && zip_file != nullptr) {
+ if (zip_file != nullptr) {
if (chown(path_.c_str(), AID_SHELL, AID_SHELL)) {
MYLOGE("Unable to change ownership of zip file %s: %s\n", path_.c_str(),
- strerror(errno));
+ strerror(errno));
}
}
int dup_stdout_fd;
int dup_stderr_fd;
- if (is_redirecting) {
- // Redirect stderr to log_path_ for debugging.
- TEMP_FAILURE_RETRY(dup_stderr_fd = dup(fileno(stderr)));
- if (!redirect_to_file(stderr, const_cast<char*>(log_path_.c_str()))) {
- return ERROR;
- }
- if (chown(log_path_.c_str(), AID_SHELL, AID_SHELL)) {
- MYLOGE("Unable to change ownership of dumpstate log file %s: %s\n", log_path_.c_str(),
- strerror(errno));
- }
+ // Redirect stderr to log_path_ for debugging.
+ TEMP_FAILURE_RETRY(dup_stderr_fd = dup(fileno(stderr)));
+ if (!redirect_to_file(stderr, const_cast<char*>(log_path_.c_str()))) {
+ return ERROR;
+ }
+ if (chown(log_path_.c_str(), AID_SHELL, AID_SHELL)) {
+ MYLOGE("Unable to change ownership of dumpstate log file %s: %s\n", log_path_.c_str(),
+ strerror(errno));
+ }
- // Redirect stdout to tmp_path_. This is the main bugreport entry and will be
- // moved into zip file later, if zipping.
- TEMP_FAILURE_RETRY(dup_stdout_fd = dup(fileno(stdout)));
- // TODO: why not write to a file instead of stdout to overcome this problem?
- /* TODO: rather than generating a text file now and zipping it later,
- it would be more efficient to redirect stdout to the zip entry
- directly, but the libziparchive doesn't support that option yet. */
- if (!redirect_to_file(stdout, const_cast<char*>(tmp_path_.c_str()))) {
- return ERROR;
- }
- if (chown(tmp_path_.c_str(), AID_SHELL, AID_SHELL)) {
- MYLOGE("Unable to change ownership of temporary bugreport file %s: %s\n",
- tmp_path_.c_str(), strerror(errno));
- }
+ // Redirect stdout to tmp_path_. This is the main bugreport entry and will be
+ // moved into zip file later, if zipping.
+ TEMP_FAILURE_RETRY(dup_stdout_fd = dup(fileno(stdout)));
+ // TODO: why not write to a file instead of stdout to overcome this problem?
+ /* TODO: rather than generating a text file now and zipping it later,
+ it would be more efficient to redirect stdout to the zip entry
+ directly, but the libziparchive doesn't support that option yet. */
+ if (!redirect_to_file(stdout, const_cast<char*>(tmp_path_.c_str()))) {
+ return ERROR;
+ }
+ if (chown(tmp_path_.c_str(), AID_SHELL, AID_SHELL)) {
+ MYLOGE("Unable to change ownership of temporary bugreport file %s: %s\n",
+ tmp_path_.c_str(), strerror(errno));
}
// Don't buffer stdout
@@ -2926,14 +2899,10 @@
}
/* close output if needed */
- if (is_redirecting) {
- TEMP_FAILURE_RETRY(dup2(dup_stdout_fd, fileno(stdout)));
- }
+ TEMP_FAILURE_RETRY(dup2(dup_stdout_fd, fileno(stdout)));
// Zip the (now complete) .tmp file within the internal directory.
- if (options_->OutputToFile()) {
- FinalizeFile();
- }
+ FinalizeFile();
// Share the final file with the caller if the user has consented or Shell is the caller.
Dumpstate::RunStatus status = Dumpstate::RunStatus::OK;
@@ -2976,11 +2945,9 @@
progress_->Save();
MYLOGI("done (id %d)\n", id_);
- if (is_redirecting) {
- TEMP_FAILURE_RETRY(dup2(dup_stderr_fd, fileno(stderr)));
- }
+ TEMP_FAILURE_RETRY(dup2(dup_stderr_fd, fileno(stderr)));
- if (options_->use_control_socket && control_socket_fd_ != -1) {
+ if (control_socket_fd_ != -1) {
MYLOGD("Closing control socket\n");
close(control_socket_fd_);
}
@@ -3051,10 +3018,7 @@
}
void Dumpstate::EnableParallelRunIfNeeded() {
- // The thread pool needs to create temporary files to receive dump results.
- // That's why we only enable it when the bugreport client chooses to output
- // to a file.
- if (!PropertiesHelper::IsParallelRun() || !options_->OutputToFile()) {
+ if (!PropertiesHelper::IsParallelRun()) {
return;
}
dump_pool_ = std::make_unique<DumpPool>(bugreport_internal_dir_);
@@ -3199,7 +3163,8 @@
options_(new Dumpstate::DumpOptions()),
last_reported_percent_progress_(0),
version_(version),
- now_(time(nullptr)) {
+ now_(time(nullptr)),
+ open_socket_fn_(open_socket) {
}
Dumpstate& Dumpstate::GetInstance() {
@@ -3833,7 +3798,7 @@
RunCommand(title, dumpsys, options, false, out_fd);
}
-int open_socket(const char *service) {
+static int open_socket(const char* service) {
int s = android_get_control_socket(service);
if (s < 0) {
MYLOGE("android_get_control_socket(%s): %s\n", service, strerror(errno));
@@ -3868,19 +3833,6 @@
return fd;
}
-/* redirect output to a service control socket */
-bool redirect_to_socket(FILE* redirect, const char* service) {
- int fd = open_socket(service);
- if (fd == -1) {
- return false;
- }
- fflush(redirect);
- // TODO: handle dup2 failure
- TEMP_FAILURE_RETRY(dup2(fd, fileno(redirect)));
- close(fd);
- return true;
-}
-
// TODO: should call is_valid_output_file and/or be merged into it.
void create_parent_dirs(const char *path) {
char *chp = const_cast<char *> (path);