Merge "Support to screenshot to interactive bugreport." into rvc-dev
diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp
index 87ea520..466575f 100644
--- a/cmds/dumpstate/DumpstateService.cpp
+++ b/cmds/dumpstate/DumpstateService.cpp
@@ -120,7 +120,7 @@
options->Initialize(static_cast<Dumpstate::BugreportMode>(bugreport_mode), bugreport_fd,
screenshot_fd);
- if (bugreport_fd.get() == -1 || (options->do_fb && screenshot_fd.get() == -1)) {
+ if (bugreport_fd.get() == -1 || (options->do_screenshot && screenshot_fd.get() == -1)) {
MYLOGE("Invalid filedescriptor");
signalErrorAndExit(listener, IDumpstateListener::BUGREPORT_ERROR_INVALID_INPUT);
}
diff --git a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl
index e486460..e17f18e 100644
--- a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl
+++ b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl
@@ -61,4 +61,9 @@
* Called when taking bugreport finishes successfully.
*/
void onFinished();
+
+ /**
+ * Called when screenshot is taken.
+ */
+ void onScreenshotTaken(boolean success);
}
diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp
index 814a4ed..3440116 100644
--- a/cmds/dumpstate/dumpstate.cpp
+++ b/cmds/dumpstate/dumpstate.cpp
@@ -647,6 +647,24 @@
std::lock_guard<std::mutex> lock(lock_);
result_ = APPROVED;
MYLOGD("User approved consent to share bugreport\n");
+
+ // Maybe copy screenshot so calling app can display the screenshot to the user as soon as
+ // consent is granted.
+ if (ds.options_->is_screenshot_copied) {
+ return android::binder::Status::ok();
+ }
+
+ if (!ds.options_->do_screenshot || ds.options_->screenshot_fd.get() == -1 ||
+ !ds.do_early_screenshot_) {
+ return android::binder::Status::ok();
+ }
+
+ bool copy_succeeded = android::os::CopyFileToFd(ds.screenshot_path_,
+ ds.options_->screenshot_fd.get());
+ ds.options_->is_screenshot_copied = copy_succeeded;
+ if (copy_succeeded) {
+ android::os::UnlinkAndLogOnError(ds.screenshot_path_);
+ }
return android::binder::Status::ok();
}
@@ -1407,7 +1425,7 @@
/* Dump Bluetooth HCI logs */
ds.AddDir("/data/misc/bluetooth/logs", true);
- if (ds.options_->do_fb && !ds.do_early_screenshot_) {
+ if (ds.options_->do_screenshot && !ds.do_early_screenshot_) {
MYLOGI("taking late screenshot\n");
ds.TakeScreenshot();
}
@@ -2149,7 +2167,7 @@
ds.base_name_ += "-wifi";
}
- if (ds.options_->do_fb) {
+ if (ds.options_->do_screenshot) {
ds.screenshot_path_ = ds.GetPath(ds.CalledByApi() ? "-tmp.png" : ".png");
}
ds.tmp_path_ = ds.GetPath(".tmp");
@@ -2229,43 +2247,45 @@
}
static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOptions* options) {
+ // Modify com.android.shell.BugreportProgressService#isDefaultScreenshotRequired as well for
+ // default system screenshots.
options->bugreport_mode = ModeToString(mode);
switch (mode) {
case Dumpstate::BugreportMode::BUGREPORT_FULL:
- options->do_fb = true;
+ options->do_screenshot = true;
options->dumpstate_hal_mode = DumpstateMode::FULL;
break;
case Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE:
// Currently, the dumpstate binder is only used by Shell to update progress.
options->do_start_service = true;
options->do_progress_updates = true;
- options->do_fb = false;
+ options->do_screenshot = true;
options->dumpstate_hal_mode = DumpstateMode::INTERACTIVE;
break;
case Dumpstate::BugreportMode::BUGREPORT_REMOTE:
options->do_vibrate = false;
options->is_remote_mode = true;
- options->do_fb = false;
+ options->do_screenshot = false;
options->dumpstate_hal_mode = DumpstateMode::REMOTE;
break;
case Dumpstate::BugreportMode::BUGREPORT_WEAR:
options->do_start_service = true;
options->do_progress_updates = true;
options->do_zip_file = true;
- options->do_fb = true;
+ options->do_screenshot = true;
options->dumpstate_hal_mode = DumpstateMode::WEAR;
break;
// TODO(b/148168577) rename TELEPHONY everywhere to CONNECTIVITY.
case Dumpstate::BugreportMode::BUGREPORT_TELEPHONY:
options->telephony_only = true;
options->do_progress_updates = true;
- options->do_fb = false;
+ options->do_screenshot = false;
options->dumpstate_hal_mode = DumpstateMode::CONNECTIVITY;
break;
case Dumpstate::BugreportMode::BUGREPORT_WIFI:
options->wifi_only = true;
options->do_zip_file = true;
- options->do_fb = false;
+ options->do_screenshot = false;
options->dumpstate_hal_mode = DumpstateMode::WIFI;
break;
case Dumpstate::BugreportMode::BUGREPORT_DEFAULT:
@@ -2275,12 +2295,13 @@
static void LogDumpOptions(const Dumpstate::DumpOptions& options) {
MYLOGI(
- "do_zip_file: %d do_vibrate: %d use_socket: %d use_control_socket: %d do_fb: %d "
+ "do_zip_file: %d do_vibrate: %d use_socket: %d use_control_socket: %d do_screenshot: %d "
"is_remote_mode: %d show_header_only: %d do_start_service: %d telephony_only: %d "
"wifi_only: %d do_progress_updates: %d fd: %d bugreport_mode: %s dumpstate_hal_mode: %s "
"args: %s\n",
options.do_zip_file, options.do_vibrate, options.use_socket, options.use_control_socket,
- options.do_fb, options.is_remote_mode, options.show_header_only, options.do_start_service,
+ options.do_screenshot, options.is_remote_mode, options.show_header_only,
+ options.do_start_service,
options.telephony_only, options.wifi_only, options.do_progress_updates,
options.bugreport_fd.get(), options.bugreport_mode.c_str(),
toString(options.dumpstate_hal_mode).c_str(), options.args.c_str());
@@ -2313,7 +2334,7 @@
case 'S': use_control_socket = true; break;
case 'v': show_header_only = true; break;
case 'q': do_vibrate = false; break;
- case 'p': do_fb = true; break;
+ case 'p': do_screenshot = true; break;
case 'P': do_progress_updates = true; break;
case 'R': is_remote_mode = true; break;
case 'V': break; // compatibility no-op
@@ -2543,11 +2564,6 @@
Vibrate(150);
}
- if (options_->do_fb && do_early_screenshot_) {
- MYLOGI("taking early screenshot\n");
- TakeScreenshot();
- }
-
if (options_->do_zip_file && 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(),
@@ -2593,19 +2609,20 @@
PrintHeader();
if (options_->telephony_only) {
+ MaybeTakeEarlyScreenshot();
MaybeCheckUserConsent(calling_uid, calling_package);
DumpstateTelephonyOnly(calling_package);
DumpstateBoard();
} else if (options_->wifi_only) {
+ MaybeTakeEarlyScreenshot();
MaybeCheckUserConsent(calling_uid, calling_package);
DumpstateWifiOnly();
} else {
- // Invoking the critical dumpsys calls before DumpTraces() to try and
- // keep the system stats as close to its initial state as possible.
+ // Invoke critical dumpsys first to preserve system state, before doing anything else.
RunDumpsysCritical();
- // Run consent check only after critical dumpsys has finished -- so the consent
- // isn't going to pollute the system state / logs.
+ // Take screenshot and get consent only after critical dumpsys has finished.
+ MaybeTakeEarlyScreenshot();
MaybeCheckUserConsent(calling_uid, calling_package);
// Dump state for the default case. This also drops root.
@@ -2640,7 +2657,9 @@
MYLOGI("User denied consent. Returning\n");
return status;
}
- if (options_->do_fb && options_->screenshot_fd.get() != -1) {
+ if (options_->do_screenshot &&
+ options_->screenshot_fd.get() != -1 &&
+ !options_->is_screenshot_copied) {
bool copy_succeeded = android::os::CopyFileToFd(screenshot_path_,
options_->screenshot_fd.get());
if (copy_succeeded) {
@@ -2694,6 +2713,14 @@
: RunStatus::OK;
}
+void Dumpstate::MaybeTakeEarlyScreenshot() {
+ if (!options_->do_screenshot || !do_early_screenshot_) {
+ return;
+ }
+
+ TakeScreenshot();
+}
+
void Dumpstate::MaybeCheckUserConsent(int32_t calling_uid, const std::string& calling_package) {
if (calling_uid == AID_SHELL || !CalledByApi()) {
// No need to get consent for shell triggered dumpstates, or not through
@@ -3630,6 +3657,11 @@
} else {
MYLOGE("Failed to take screenshot on %s\n", real_path.c_str());
}
+ if (listener_ != nullptr) {
+ // Show a visual indication to indicate screenshot is taken via
+ // IDumpstateListener.onScreenshotTaken()
+ listener_->onScreenshotTaken(status == 0);
+ }
}
bool is_dir(const char* pathname) {
diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h
index 111c098..7e27787 100644
--- a/cmds/dumpstate/dumpstate.h
+++ b/cmds/dumpstate/dumpstate.h
@@ -359,7 +359,8 @@
// Writes bugreport content to a socket; only flatfile format is supported.
bool use_socket = false;
bool use_control_socket = false;
- bool do_fb = false;
+ bool do_screenshot = false;
+ bool is_screenshot_copied = false;
bool is_remote_mode = false;
bool show_header_only = false;
bool do_start_service = false;
@@ -494,6 +495,8 @@
RunStatus DumpstateDefaultAfterCritical();
+ void MaybeTakeEarlyScreenshot();
+
void MaybeCheckUserConsent(int32_t calling_uid, const std::string& calling_package);
// Removes the in progress files output files (tmp file, zip/txt file, screenshot),
diff --git a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
index dac90d9..f26e4db 100644
--- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
@@ -167,6 +167,12 @@
return binder::Status::ok();
}
+ binder::Status onScreenshotTaken(bool success) override {
+ std::lock_guard<std::mutex> lock(lock_);
+ dprintf(out_fd_, "\rResult of taking screenshot: %s", success ? "success" : "failure");
+ return binder::Status::ok();
+ }
+
bool getIsFinished() {
std::lock_guard<std::mutex> lock(lock_);
return is_finished_;
diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp
index 76b9960..0a0c40e 100644
--- a/cmds/dumpstate/tests/dumpstate_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_test.cpp
@@ -65,6 +65,7 @@
MOCK_METHOD1(onProgress, binder::Status(int32_t progress));
MOCK_METHOD1(onError, binder::Status(int32_t error_code));
MOCK_METHOD0(onFinished, binder::Status());
+ MOCK_METHOD1(onScreenshotTaken, binder::Status(bool success));
protected:
MOCK_METHOD0(onAsBinder, IBinder*());
@@ -173,7 +174,7 @@
EXPECT_FALSE(options_.use_control_socket);
EXPECT_FALSE(options_.show_header_only);
EXPECT_TRUE(options_.do_vibrate);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_FALSE(options_.do_progress_updates);
EXPECT_FALSE(options_.is_remote_mode);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT);
@@ -199,7 +200,7 @@
// Other options retain default values
EXPECT_TRUE(options_.do_vibrate);
EXPECT_FALSE(options_.show_header_only);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_FALSE(options_.do_progress_updates);
EXPECT_FALSE(options_.is_remote_mode);
EXPECT_FALSE(options_.use_socket);
@@ -225,7 +226,7 @@
EXPECT_FALSE(options_.do_zip_file);
EXPECT_FALSE(options_.use_control_socket);
EXPECT_FALSE(options_.show_header_only);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_FALSE(options_.do_progress_updates);
EXPECT_FALSE(options_.is_remote_mode);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT);
@@ -234,7 +235,7 @@
TEST_F(DumpOptionsTest, InitializeFullBugReport) {
options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_FULL, fd, fd);
EXPECT_TRUE(options_.do_add_date);
- EXPECT_TRUE(options_.do_fb);
+ EXPECT_TRUE(options_.do_screenshot);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::FULL);
@@ -254,7 +255,7 @@
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.do_progress_updates);
EXPECT_TRUE(options_.do_start_service);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_TRUE(options_.do_screenshot);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::INTERACTIVE);
// Other options retain default values
@@ -271,7 +272,7 @@
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.is_remote_mode);
EXPECT_FALSE(options_.do_vibrate);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::REMOTE);
// Other options retain default values
@@ -284,7 +285,7 @@
TEST_F(DumpOptionsTest, InitializeWearBugReport) {
options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_WEAR, fd, fd);
EXPECT_TRUE(options_.do_add_date);
- EXPECT_TRUE(options_.do_fb);
+ EXPECT_TRUE(options_.do_screenshot);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.do_progress_updates);
EXPECT_TRUE(options_.do_start_service);
@@ -301,7 +302,7 @@
TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) {
options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_TELEPHONY, fd, fd);
EXPECT_TRUE(options_.do_add_date);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.telephony_only);
EXPECT_TRUE(options_.do_progress_updates);
@@ -318,7 +319,7 @@
TEST_F(DumpOptionsTest, InitializeWifiBugReport) {
options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_WIFI, fd, fd);
EXPECT_TRUE(options_.do_add_date);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.wifi_only);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::WIFI);
@@ -346,7 +347,7 @@
EXPECT_EQ(status, Dumpstate::RunStatus::OK);
EXPECT_TRUE(options_.do_add_date);
- EXPECT_TRUE(options_.do_fb);
+ EXPECT_TRUE(options_.do_screenshot);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT);
@@ -384,7 +385,7 @@
// Other options retain default values
EXPECT_FALSE(options_.show_header_only);
EXPECT_TRUE(options_.do_vibrate);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_FALSE(options_.do_progress_updates);
EXPECT_FALSE(options_.is_remote_mode);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT);
@@ -407,7 +408,7 @@
EXPECT_EQ(status, Dumpstate::RunStatus::OK);
EXPECT_TRUE(options_.show_header_only);
EXPECT_FALSE(options_.do_vibrate);
- EXPECT_TRUE(options_.do_fb);
+ EXPECT_TRUE(options_.do_screenshot);
EXPECT_TRUE(options_.do_progress_updates);
EXPECT_TRUE(options_.is_remote_mode);