From 359b1ff6b4eb4b26fb5acfd25a21c71f0da7929d Mon Sep 17 00:00:00 2001 From: Abhijeet Kaur Date: Fri, 26 Jul 2019 16:01:36 +0100 Subject: Make dumpstate bugreport names as tmp if called by API Since we are migrating away from dumpstate being directly triggered, only API will be calling dumpstate. API would pass final bugreport file fds to dumpstate, that implies dumpstate bugreport files are getting copied over. Marking these files with a suffix of -tmp as: * These (bugreport .zip and screenshot .png) files are eventually getting deleted. * These (bugreport .zip and screenshot .png) file names can be used by API callers (as the final bugreport names) for consistency. Since the files are in the same dir, need to add suffix to differentiate. Bug: 126862297 Test: * Build and flash to the device * Turn on Settings feature flag to use the bugreport API * Take interactive and full bugreports * run `adb shell ls /bugreports` while bugreports are being generated and after the bugreport is captured * Can see tmp files when the bugreport is in progress * Output file names same as before Change-Id: Ia88e373a373d573d9c1e089924290b9526a3d2e1 --- cmds/dumpstate/dumpstate.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 2bcb17deee..76580c5e73 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1986,12 +1986,12 @@ static void PrepareToWriteToFile() { } if (ds.options_->do_fb) { - ds.screenshot_path_ = ds.GetPath(".png"); + ds.screenshot_path_ = ds.GetPath(ds.CalledByApi() ? "-tmp.png" : ".png"); } ds.tmp_path_ = ds.GetPath(".tmp"); ds.log_path_ = ds.GetPath("-dumpstate_log-" + std::to_string(ds.pid_) + ".txt"); - std::string destination = ds.options_->bugreport_fd.get() != -1 + std::string destination = ds.CalledByApi() ? StringPrintf("[fd:%d]", ds.options_->bugreport_fd.get()) : ds.bugreport_internal_dir_.c_str(); MYLOGD( @@ -2005,7 +2005,7 @@ static void PrepareToWriteToFile() { ds.tmp_path_.c_str(), ds.screenshot_path_.c_str()); if (ds.options_->do_zip_file) { - ds.path_ = ds.GetPath(".zip"); + ds.path_ = ds.GetPath(ds.CalledByApi() ? "-tmp.zip" : ".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")); @@ -2040,7 +2040,7 @@ static void FinalizeFile() { MYLOGI("changing suffix from %s to %s\n", ds.name_.c_str(), name.c_str()); ds.name_ = name; if (!ds.screenshot_path_.empty()) { - std::string new_screenshot_path = ds.GetPath(".png"); + std::string new_screenshot_path = ds.GetPath(ds.CalledByApi() ? "-tmp.png" : ".png"); if (rename(ds.screenshot_path_.c_str(), new_screenshot_path.c_str())) { MYLOGE("rename(%s, %s): %s\n", ds.screenshot_path_.c_str(), new_screenshot_path.c_str(), strerror(errno)); @@ -2058,7 +2058,7 @@ static void FinalizeFile() { } else { do_text_file = false; // If the user has changed the suffix, we need to change the zip file name. - std::string new_path = ds.GetPath(".zip"); + std::string new_path = ds.GetPath(ds.CalledByApi() ? "-tmp.zip" : ".zip"); if (ds.path_ != new_path) { MYLOGD("Renaming zip file from %s to %s\n", ds.path_.c_str(), new_path.c_str()); if (rename(ds.path_.c_str(), new_path.c_str())) { @@ -2706,6 +2706,10 @@ bool Dumpstate::IsUserConsentDenied() const { ds.consent_callback_->getResult() == UserConsentResult::DENIED; } +bool Dumpstate::CalledByApi() const { + return ds.options_->bugreport_fd.get() != -1 ? true : false; +} + void Dumpstate::CleanupFiles() { android::os::UnlinkAndLogOnError(tmp_path_); android::os::UnlinkAndLogOnError(screenshot_path_); -- cgit v1.2.3-59-g8ed1b From 402a839ef5e2fdecfd9a0fc75e6250d374bfc83e Mon Sep 17 00:00:00 2001 From: Nandana Dutt Date: Fri, 14 Jun 2019 14:25:13 +0100 Subject: Remove old dumpstate aidl methods In Q we added new aidl methods for dumpstate to communicate with SystemServer. The old methods can be removed now. This change is functionally equivalent to before except for how progress is reported. Dumpstate loads previous run stats and calculates expected run time. If the current run exceeds that time, it used to let the client know via onMaxProgressUpdated(). Given the API world, this CL simplifies this model so that dumpstate conveys just one piece of information, i.e. final progress precentage, rather than current, previous_max, and new_max. Test: atest dumpstate_test Test: bugreport from power button works as expected BUG: 128980174 Change-Id: Id9103649b0f7c8e6ea0b79583ea04825cb5b455f --- cmds/dumpstate/Android.bp | 1 - cmds/dumpstate/DumpstateSectionReporter.cpp | 42 ------------- cmds/dumpstate/DumpstateSectionReporter.h | 65 ------------------- cmds/dumpstate/DumpstateService.cpp | 3 +- .../binder/android/os/IDumpstateListener.aidl | 17 ----- cmds/dumpstate/dumpstate.cpp | 25 ++------ cmds/dumpstate/dumpstate.h | 8 +-- cmds/dumpstate/tests/dumpstate_smoke_test.cpp | 21 ------- cmds/dumpstate/tests/dumpstate_test.cpp | 72 +++++----------------- 9 files changed, 22 insertions(+), 232 deletions(-) delete mode 100644 cmds/dumpstate/DumpstateSectionReporter.cpp delete mode 100644 cmds/dumpstate/DumpstateSectionReporter.h (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/Android.bp b/cmds/dumpstate/Android.bp index 8d383f501a..93bbe902c0 100644 --- a/cmds/dumpstate/Android.bp +++ b/cmds/dumpstate/Android.bp @@ -93,7 +93,6 @@ cc_defaults { "libutils", ], srcs: [ - "DumpstateSectionReporter.cpp", "DumpstateService.cpp", ], static_libs: [ diff --git a/cmds/dumpstate/DumpstateSectionReporter.cpp b/cmds/dumpstate/DumpstateSectionReporter.cpp deleted file mode 100644 index f814bde26d..0000000000 --- a/cmds/dumpstate/DumpstateSectionReporter.cpp +++ /dev/null @@ -1,42 +0,0 @@ -/* - * Copyright (C) 2018 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#define LOG_TAG "dumpstate" - -#include "DumpstateSectionReporter.h" - -namespace android { -namespace os { -namespace dumpstate { - -DumpstateSectionReporter::DumpstateSectionReporter(const std::string& title, - sp listener, - bool sendReport) - : title_(title), listener_(listener), sendReport_(sendReport), status_(OK), size_(-1) { - started_ = std::chrono::steady_clock::now(); -} - -DumpstateSectionReporter::~DumpstateSectionReporter() { - if ((listener_ != nullptr) && (sendReport_)) { - auto elapsed = std::chrono::duration_cast( - std::chrono::steady_clock::now() - started_); - listener_->onSectionComplete(title_, status_, size_, (int32_t)elapsed.count()); - } -} - -} // namespace dumpstate -} // namespace os -} // namespace android diff --git a/cmds/dumpstate/DumpstateSectionReporter.h b/cmds/dumpstate/DumpstateSectionReporter.h deleted file mode 100644 index e971de84c5..0000000000 --- a/cmds/dumpstate/DumpstateSectionReporter.h +++ /dev/null @@ -1,65 +0,0 @@ -/* - * Copyright (C) 2018 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#ifndef ANDROID_OS_DUMPSTATESECTIONREPORTER_H_ -#define ANDROID_OS_DUMPSTATESECTIONREPORTER_H_ - -#include -#include - -namespace android { -namespace os { -namespace dumpstate { - - -/* - * Helper class used to report per section details to a listener. - * - * Typical usage: - * - * DumpstateSectionReporter sectionReporter(title, listener, sendReport); - * sectionReporter.setSize(5000); - * - */ -class DumpstateSectionReporter { - public: - DumpstateSectionReporter(const std::string& title, sp listener, - bool sendReport); - - ~DumpstateSectionReporter(); - - void setStatus(status_t status) { - status_ = status; - } - - void setSize(int size) { - size_ = size; - } - - private: - std::string title_; - android::sp listener_; - bool sendReport_; - status_t status_; - int size_; - std::chrono::time_point started_; -}; - -} // namespace dumpstate -} // namespace os -} // namespace android - -#endif // ANDROID_OS_DUMPSTATESECTIONREPORTER_H_ diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index 37ba4f906e..f98df99534 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -200,8 +200,7 @@ status_t DumpstateService::dump(int fd, const Vector&) { dprintf(fd, "id: %d\n", ds_->id_); dprintf(fd, "pid: %d\n", ds_->pid_); dprintf(fd, "update_progress: %s\n", ds_->options_->do_progress_updates ? "true" : "false"); - dprintf(fd, "update_progress_threshold: %d\n", ds_->update_progress_threshold_); - dprintf(fd, "last_updated_progress: %d\n", ds_->last_updated_progress_); + dprintf(fd, "last_percent_progress: %d\n", ds_->last_reported_percent_progress_); dprintf(fd, "progress:\n"); ds_->progress_->Dump(fd, " "); dprintf(fd, "args: %s\n", ds_->options_->args.c_str()); diff --git a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl index ea1e467dca..e486460753 100644 --- a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl +++ b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl @@ -61,21 +61,4 @@ interface IDumpstateListener { * Called when taking bugreport finishes successfully. */ void onFinished(); - - // TODO(b/111441001): Remove old methods when not used anymore. - void onProgressUpdated(int progress); - void onMaxProgressUpdated(int maxProgress); - - /** - * Called after every section is complete. - * - * @param name section name - * @param status values from status_t - * {@code OK} section completed successfully - * {@code TIMEOUT} dump timed out - * {@code != OK} error - * @param size size in bytes, may be invalid if status != OK - * @param durationMs duration in ms - */ - void onSectionComplete(@utf8InCpp String name, int status, int size, int durationMs); } diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index fe55fe7d4b..6b2e3b70cd 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -80,7 +80,6 @@ #include #include #include "DumpstateInternal.h" -#include "DumpstateSectionReporter.h" #include "DumpstateService.h" #include "dumpstate.h" @@ -105,7 +104,6 @@ using android::base::StringPrintf; using android::os::IDumpstateListener; using android::os::dumpstate::CommandOptions; using android::os::dumpstate::DumpFileToFd; -using android::os::dumpstate::DumpstateSectionReporter; using android::os::dumpstate::PropertiesHelper; // Keep in sync with @@ -1047,7 +1045,6 @@ static Dumpstate::RunStatus RunDumpsysTextByPriority(const std::string& title, i RETURN_IF_USER_DENIED_CONSENT(); std::string path(title); path.append(" - ").append(String8(service).c_str()); - DumpstateSectionReporter section_reporter(path, ds.listener_, ds.report_section_); size_t bytes_written = 0; status_t status = dumpsys.startDumpThread(service, args); if (status == OK) { @@ -1055,12 +1052,10 @@ static Dumpstate::RunStatus RunDumpsysTextByPriority(const std::string& title, i std::chrono::duration elapsed_seconds; status = dumpsys.writeDump(STDOUT_FILENO, service, service_timeout, /* as_proto = */ false, elapsed_seconds, bytes_written); - section_reporter.setSize(bytes_written); dumpsys.writeDumpFooter(STDOUT_FILENO, service, elapsed_seconds); bool dump_complete = (status == OK); dumpsys.stopDumpThread(dump_complete); } - section_reporter.setStatus(status); auto elapsed_duration = std::chrono::duration_cast( std::chrono::steady_clock::now() - start); @@ -1123,7 +1118,6 @@ static Dumpstate::RunStatus RunDumpsysProto(const std::string& title, int priori path.append("_HIGH"); } path.append(kProtoExt); - DumpstateSectionReporter section_reporter(path, ds.listener_, ds.report_section_); status_t status = dumpsys.startDumpThread(service, args); if (status == OK) { status = ds.AddZipEntryFromFd(path, dumpsys.getDumpFd(), service_timeout); @@ -1132,8 +1126,6 @@ static Dumpstate::RunStatus RunDumpsysProto(const std::string& title, int priori } ZipWriter::FileEntry file_entry; ds.zip_writer_->GetLastEntry(&file_entry); - section_reporter.setSize(file_entry.compressed_size); - section_reporter.setStatus(status); auto elapsed_duration = std::chrono::duration_cast( std::chrono::steady_clock::now() - start); @@ -2813,6 +2805,7 @@ int run_main(int argc, char* argv[]) { Dumpstate::Dumpstate(const std::string& version) : pid_(getpid()), options_(new Dumpstate::DumpOptions()), + last_reported_percent_progress_(0), version_(version), now_(time(nullptr)) { } @@ -3575,31 +3568,25 @@ void Dumpstate::UpdateProgress(int32_t delta_sec) { } // Always update progess so stats can be tuned... - bool max_changed = progress_->Inc(delta_sec); + progress_->Inc(delta_sec); // ...but only notifiy listeners when necessary. if (!options_->do_progress_updates) return; int progress = progress_->Get(); int max = progress_->GetMax(); + int percent = 100 * progress / max; - // adjusts max on the fly - if (max_changed && listener_ != nullptr) { - listener_->onMaxProgressUpdated(max); - } - - int32_t last_update_delta = progress - last_updated_progress_; - if (last_updated_progress_ > 0 && last_update_delta < update_progress_threshold_) { + if (last_reported_percent_progress_ > 0 && percent <= last_reported_percent_progress_) { return; } - last_updated_progress_ = progress; + last_reported_percent_progress_ = percent; if (control_socket_fd_ >= 0) { dprintf(control_socket_fd_, "PROGRESS:%d/%d\n", progress, max); fsync(control_socket_fd_); } - int percent = 100 * progress / max; if (listener_ != nullptr) { if (percent % 5 == 0) { // We don't want to spam logcat, so only log multiples of 5. @@ -3611,8 +3598,6 @@ void Dumpstate::UpdateProgress(int32_t delta_sec) { fprintf(stderr, "Setting progress (%s): %d/%d (%d%%)\n", listener_name_.c_str(), progress, max, percent); } - // TODO(b/111441001): Remove in favor of onProgress - listener_->onProgressUpdated(progress); listener_->onProgress(percent); } diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index fe330df30a..77b5e8a9f9 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -405,12 +405,8 @@ class Dumpstate { // Runtime options. std::unique_ptr options_; - // How frequently the progess should be updated;the listener will only be notificated when the - // delta from the previous update is more than the threshold. - int32_t update_progress_threshold_ = 100; - - // Last progress that triggered a listener updated - int32_t last_updated_progress_; + // Last progress that was sent to the listener [0-100]. + int last_reported_percent_progress_ = 0; // Whether it should take an screenshot earlier in the process. bool do_early_screenshot_ = false; diff --git a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp index f7acaf1b9b..181046a7a7 100644 --- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp @@ -167,26 +167,6 @@ class DumpstateListener : public BnDumpstateListener { return binder::Status::ok(); } - binder::Status onProgressUpdated(int32_t progress) override { - dprintf(out_fd_, "\rIn progress %d/%d", progress, max_progress_); - return binder::Status::ok(); - } - - binder::Status onMaxProgressUpdated(int32_t max_progress) override { - std::lock_guard lock(lock_); - max_progress_ = max_progress; - return binder::Status::ok(); - } - - binder::Status onSectionComplete(const ::std::string& name, int32_t, int32_t size_bytes, - int32_t) override { - std::lock_guard lock(lock_); - if (sections_.get() != nullptr) { - sections_->push_back({name, size_bytes}); - } - return binder::Status::ok(); - } - bool getIsFinished() { std::lock_guard lock(lock_); return is_finished_; @@ -199,7 +179,6 @@ class DumpstateListener : public BnDumpstateListener { private: int out_fd_; - int max_progress_ = 5000; int error_code_ = -1; bool is_finished_ = false; std::shared_ptr> sections_; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index 4e6b084ff6..cff1d439d9 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -62,10 +62,6 @@ class DumpstateListenerMock : public IDumpstateListener { MOCK_METHOD1(onProgress, binder::Status(int32_t progress)); MOCK_METHOD1(onError, binder::Status(int32_t error_code)); MOCK_METHOD0(onFinished, binder::Status()); - MOCK_METHOD1(onProgressUpdated, binder::Status(int32_t progress)); - MOCK_METHOD1(onMaxProgressUpdated, binder::Status(int32_t max_progress)); - MOCK_METHOD4(onSectionComplete, binder::Status(const ::std::string& name, int32_t status, - int32_t size, int32_t durationMs)); protected: MOCK_METHOD0(onAsBinder, IBinder*()); @@ -590,7 +586,6 @@ class DumpstateTest : public DumpstateBaseTest { SetDryRun(false); SetBuildType(android::base::GetProperty("ro.build.type", "(unknown)")); ds.progress_.reset(new Progress()); - ds.update_progress_threshold_ = 0; ds.options_.reset(new Dumpstate::DumpOptions()); } @@ -615,10 +610,9 @@ class DumpstateTest : public DumpstateBaseTest { return status; } - void SetProgress(long progress, long initial_max, long threshold = 0) { + void SetProgress(long progress, long initial_max) { + ds.last_reported_percent_progress_ = 0; ds.options_->do_progress_updates = true; - ds.update_progress_threshold_ = threshold; - ds.last_updated_progress_ = 0; ds.progress_.reset(new Progress(initial_max, progress, 1.2)); } @@ -796,73 +790,36 @@ TEST_F(DumpstateTest, RunCommandProgress) { ds.listener_name_ = "FoxMulder"; SetProgress(0, 30); - EXPECT_CALL(*listener, onProgressUpdated(20)); EXPECT_CALL(*listener, onProgress(66)); // 20/30 % EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(20).Build())); std::string progress_message = GetProgressMessage(ds.listener_name_, 20, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); - EXPECT_CALL(*listener, onProgressUpdated(30)); - EXPECT_CALL(*listener, onProgress(100)); // 35/35 % - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(10).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 30, 30); - EXPECT_THAT(out, StrEq("stdout\n")); - EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); - - // Run a command that will increase maximum timeout. - EXPECT_CALL(*listener, onProgressUpdated(31)); - EXPECT_CALL(*listener, onMaxProgressUpdated(37)); - EXPECT_CALL(*listener, onProgress(83)); // 31/37 % - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(1).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 31, 37, 30); // 20% increase + EXPECT_CALL(*listener, onProgress(80)); // 24/30 % + EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(4).Build())); + progress_message = GetProgressMessage(ds.listener_name_, 24, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); // Make sure command ran while in dry_run is counted. SetDryRun(true); - EXPECT_CALL(*listener, onProgressUpdated(35)); - EXPECT_CALL(*listener, onProgress(94)); // 35/37 % - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(4).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 35, 37); + EXPECT_CALL(*listener, onProgress(90)); // 27/30 % + EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(3).Build())); + progress_message = GetProgressMessage(ds.listener_name_, 27, 30); EXPECT_THAT(out, IsEmpty()); EXPECT_THAT(err, StrEq(progress_message)); - ds.listener_.clear(); -} - -TEST_F(DumpstateTest, RunCommandProgressIgnoreThreshold) { - sp listener(new DumpstateListenerMock()); - ds.listener_ = listener; - ds.listener_name_ = "FoxMulder"; - SetProgress(0, 8, 5); // 8 max, 5 threshold - - // First update should always be sent. - EXPECT_CALL(*listener, onProgressUpdated(1)); - EXPECT_CALL(*listener, onProgress(12)); // 1/12 % - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(1).Build())); - std::string progress_message = GetProgressMessage(ds.listener_name_, 1, 8); + SetDryRun(false); + EXPECT_CALL(*listener, onProgress(96)); // 29/30 % + EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(2).Build())); + progress_message = GetProgressMessage(ds.listener_name_, 29, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); - // Fourth update should be ignored because it's between the threshold (5 -1 = 4 < 5). - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(4).Build())); - EXPECT_THAT(out, StrEq("stdout\n")); - EXPECT_THAT(err, StrEq("stderr\n")); - - // Third update should be sent because it reaches threshold (6 - 1 = 5). - EXPECT_CALL(*listener, onProgressUpdated(6)); - EXPECT_CALL(*listener, onProgress(75)); // 6/8 % + EXPECT_CALL(*listener, onProgress(100)); // 30/30 % EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(1).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 6, 8); - EXPECT_THAT(out, StrEq("stdout\n")); - EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); - - // Fourth update should be ignored because it's between the threshold (9 - 6 = 3 < 5). - // But max update should be sent. - EXPECT_CALL(*listener, onMaxProgressUpdated(10)); // 9 * 120% = 10.8 = 10 - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(3).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 9, 10, 8, false); + progress_message = GetProgressMessage(ds.listener_name_, 30, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); @@ -1090,7 +1047,6 @@ TEST_F(DumpstateTest, DumpFileUpdateProgress) { ds.listener_name_ = "FoxMulder"; SetProgress(0, 30); - EXPECT_CALL(*listener, onProgressUpdated(5)); EXPECT_CALL(*listener, onProgress(16)); // 5/30 % EXPECT_EQ(0, DumpFile("", kTestDataPath + "single-line.txt")); -- cgit v1.2.3-59-g8ed1b From 07d587b98905d04c8a5fb05c7f6820c1acfc7a67 Mon Sep 17 00:00:00 2001 From: Abhijeet Kaur Date: Thu, 29 Aug 2019 14:11:57 +0100 Subject: Remove outfile flag as it's unused and all callers migrated Bug: 111441001 Bug: 135186519 Test: builds Test: atest dumpstate_test Change-Id: I9fbaa6765f7384d99239161a38a9962d476e1394 --- cmds/dumpstate/dumpstate.cpp | 3 --- cmds/dumpstate/dumpstate.rc | 3 +-- cmds/dumpstate/tests/dumpstate_smoke_test.cpp | 4 +--- 3 files changed, 2 insertions(+), 8 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index a8e6738228..b781da308c 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2328,9 +2328,6 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) // clang-format off case 'd': do_add_date = true; break; case 'z': do_zip_file = true; break; - // o=use_outfile not supported anymore. - // TODO(b/111441001): Remove when all callers have migrated. - case 'o': break; case 's': use_socket = true; break; case 'S': use_control_socket = true; break; case 'v': show_header_only = true; break; diff --git a/cmds/dumpstate/dumpstate.rc b/cmds/dumpstate/dumpstate.rc index 14937b80b9..e491a4b614 100644 --- a/cmds/dumpstate/dumpstate.rc +++ b/cmds/dumpstate/dumpstate.rc @@ -11,8 +11,7 @@ service dumpstate /system/bin/dumpstate -s # dumpstatez generates a zipped bugreport but also uses a socket to print the file location once # it is finished. -service dumpstatez /system/bin/dumpstate -S -d -z \ - -o /data/user_de/0/com.android.shell/files/bugreports/bugreport +service dumpstatez /system/bin/dumpstate -S -d -z socket dumpstate stream 0660 shell log class main disabled diff --git a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp index 181046a7a7..dbbcdff63e 100644 --- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp @@ -201,9 +201,7 @@ class ZippedBugreportGenerationTest : public Test { (char*)"dumpstate", (char*)"-d", (char*)"-z", - (char*)"-B", - (char*)"-o", - (char*)dirname(android::base::GetExecutablePath().c_str()) + (char*)"-B" }; // clang-format on sp listener(new DumpstateListener(dup(fileno(stdout)), sections)); -- cgit v1.2.3-59-g8ed1b From 69b479a4de8911eb2a3ea76a185953fa756acf53 Mon Sep 17 00:00:00 2001 From: Abhijeet Kaur Date: Tue, 3 Sep 2019 13:40:27 +0100 Subject: Do not send finished broadcasts when called by API Bug: 137825297 Test: builds and takes bugreports Change-Id: I7a393ac928098f147362dd6e728e740cfe2d4ca3 --- cmds/dumpstate/dumpstate.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 3f2469cde4..ca3a9b345b 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2708,7 +2708,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, } /* tell activity manager we're done */ - if (options_->do_broadcast) { + if (options_->do_broadcast && !CalledByApi()) { SendBugreportFinishedBroadcast(); // Note that listener_ is notified in Run(); } -- cgit v1.2.3-59-g8ed1b From 3172b538334b22eafae042048d2bd95c25667ee6 Mon Sep 17 00:00:00 2001 From: Abhijeet Kaur Date: Tue, 15 Oct 2019 15:07:03 +0100 Subject: Allow Shell to trigger bugreports without user consent dialog Bug: 141442785 Test: Takes interactive/full bugreports as expected. Test: atest dumpstate_test Change-Id: Iee09c749bc7f70c94568cd447ea3eece9a59223b --- cmds/dumpstate/dumpstate.cpp | 20 +++++++++++++++----- cmds/dumpstate/dumpstate.h | 5 +++-- 2 files changed, 18 insertions(+), 7 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index e1181a6dc8..dae5c17ce5 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2526,6 +2526,8 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, return RunStatus::OK; } + MYLOGD("dumpstate calling_uid = %d ; calling package = %s \n", + calling_uid, calling_package.c_str()); if (options_->bugreport_fd.get() != -1) { // If the output needs to be copied over to the caller's fd, get user consent. android::String16 package(calling_package.c_str()); @@ -2703,10 +2705,10 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, FinalizeFile(); } - // Share the final file with the caller if the user has consented. + // Share the final file with the caller if the user has consented or Shell is the caller. Dumpstate::RunStatus status = Dumpstate::RunStatus::OK; if (options_->bugreport_fd.get() != -1) { - status = CopyBugreportIfUserConsented(); + status = CopyBugreportIfUserConsented(calling_uid); if (status != Dumpstate::RunStatus::OK && status != Dumpstate::RunStatus::USER_CONSENT_TIMED_OUT) { // Do an early return if there were errors. We make an exception for consent @@ -2776,6 +2778,9 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, } void Dumpstate::CheckUserConsent(int32_t calling_uid, const android::String16& calling_package) { + if (calling_uid == AID_SHELL) { + return; + } consent_callback_ = new ConsentCallback(); const String16 incidentcompanion("incidentcompanion"); sp ics(defaultServiceManager()->getService(incidentcompanion)); @@ -2810,10 +2815,15 @@ Dumpstate::RunStatus Dumpstate::HandleUserConsentDenied() { return USER_CONSENT_DENIED; } -Dumpstate::RunStatus Dumpstate::CopyBugreportIfUserConsented() { +Dumpstate::RunStatus Dumpstate::CopyBugreportIfUserConsented(int32_t calling_uid) { // If the caller has asked to copy the bugreport over to their directory, we need explicit - // user consent. - UserConsentResult consent_result = consent_callback_->getResult(); + // user consent (unless the caller is Shell). + UserConsentResult consent_result; + if (calling_uid == AID_SHELL) { + consent_result = UserConsentResult::APPROVED; + } else { + consent_result = consent_callback_->getResult(); + } if (consent_result == UserConsentResult::UNAVAILABLE) { // User has not responded yet. uint64_t elapsed_ms = consent_callback_->getElapsedTimeMs(); diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 5ba84cad84..430936ebfd 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -498,8 +498,9 @@ class Dumpstate { RunStatus HandleUserConsentDenied(); - // Copies bugreport artifacts over to the caller's directories provided there is user consent. - RunStatus CopyBugreportIfUserConsented(); + // Copies bugreport artifacts over to the caller's directories provided there is user consent or + // called by Shell. + RunStatus CopyBugreportIfUserConsented(int32_t calling_uid); // Used by GetInstance() only. explicit Dumpstate(const std::string& version = VERSION_CURRENT); -- cgit v1.2.3-59-g8ed1b From e370d68932f5c81c4cfdaa4412e65d1fe055ca29 Mon Sep 17 00:00:00 2001 From: Abhijeet Kaur Date: Tue, 1 Oct 2019 16:49:30 +0100 Subject: Sunset legacy bugreport methods Bugreport is now triggered using API and not via broadcasts from dumpstate. As migration to API flow is stable, we can remove methods and broadcasts that were used in non-API bugreport flow. * Finished broadcasts are handled by Shell when onfinished() is called from dumpstate. This is because dumpstate does not have any information about the final files (other than their fds). * Remove system properties title/description as they are handled by Shell. * File rename is not handled by dumpstate as it does not own the final files. File rename is handled by the caller of the API (Shell for instance). * Remove system property to read mode and set properties from it. Modify tests accordingly. Rename extra_options to bugreport_mode. * Deprecate do_broadcast flag, as finished notification is no longer handled by dumpstate. Bug: 136066578 Test: Takes interactive/full bugreport as expected Test: atest dumpstate_test Test: adb bugreport (works as expected) Test: adb bugreport br.zip (works as expected) Change-Id: Ib39798944c4308ae0c87a22a9164567f249adfff --- cmds/dumpstate/DumpstateService.cpp | 3 +- cmds/dumpstate/dumpstate.cpp | 260 +++----------------------- cmds/dumpstate/dumpstate.h | 8 +- cmds/dumpstate/tests/dumpstate_smoke_test.cpp | 1 - cmds/dumpstate/tests/dumpstate_test.cpp | 127 +------------ 5 files changed, 35 insertions(+), 364 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index f98df99534..99dfee63e9 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -204,14 +204,13 @@ status_t DumpstateService::dump(int fd, const Vector&) { dprintf(fd, "progress:\n"); ds_->progress_->Dump(fd, " "); dprintf(fd, "args: %s\n", ds_->options_->args.c_str()); - dprintf(fd, "extra_options: %s\n", ds_->options_->extra_options.c_str()); + dprintf(fd, "bugreport_mode: %s\n", ds_->options_->bugreport_mode.c_str()); dprintf(fd, "version: %s\n", ds_->version_.c_str()); dprintf(fd, "bugreport_dir: %s\n", destination.c_str()); dprintf(fd, "screenshot_path: %s\n", ds_->screenshot_path_.c_str()); dprintf(fd, "log_path: %s\n", ds_->log_path_.c_str()); dprintf(fd, "tmp_path: %s\n", ds_->tmp_path_.c_str()); dprintf(fd, "path: %s\n", ds_->path_.c_str()); - dprintf(fd, "extra_options: %s\n", ds_->options_->extra_options.c_str()); dprintf(fd, "base_name: %s\n", ds_->base_name_.c_str()); dprintf(fd, "name: %s\n", ds_->name_.c_str()); dprintf(fd, "now: %ld\n", ds_->now_); diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index b87582eab7..4688cedc96 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -231,14 +231,6 @@ static bool UnlinkAndLogOnError(const std::string& file) { return true; } -static bool IsFileEmpty(const std::string& file_path) { - std::ifstream file(file_path, std::ios::binary | std::ios::ate); - if(file.bad()) { - MYLOGE("Cannot open file: %s\n", file_path.c_str()); - return true; - } - return file.tellg() <= 0; -} int64_t GetModuleMetadataVersion() { auto binder = defaultServiceManager()->getService(android::String16("package_native")); @@ -288,11 +280,8 @@ static const std::string kDumpstateBoardFiles[] = { }; static const int NUM_OF_DUMPS = arraysize(kDumpstateBoardFiles); -static constexpr char PROPERTY_EXTRA_OPTIONS[] = "dumpstate.options"; static constexpr char PROPERTY_LAST_ID[] = "dumpstate.last_id"; static constexpr char PROPERTY_VERSION[] = "dumpstate.version"; -static constexpr char PROPERTY_EXTRA_TITLE[] = "dumpstate.options.title"; -static constexpr char PROPERTY_EXTRA_DESCRIPTION[] = "dumpstate.options.description"; static const CommandOptions AS_ROOT_20 = CommandOptions::WithTimeout(20).AsRoot().Build(); @@ -696,8 +685,8 @@ void Dumpstate::PrintHeader() const { RunCommandToFd(STDOUT_FILENO, "", {"uptime", "-p"}, CommandOptions::WithTimeout(1).Always().Build()); printf("Bugreport format version: %s\n", version_.c_str()); - printf("Dumpstate info: id=%d pid=%d dry_run=%d args=%s extra_options=%s\n", id_, pid_, - PropertiesHelper::IsDryRun(), options_->args.c_str(), options_->extra_options.c_str()); + printf("Dumpstate info: id=%d pid=%d dry_run=%d args=%s bugreport_mode=%s\n", id_, pid_, + PropertiesHelper::IsDryRun(), options_->args.c_str(), options_->bugreport_mode.c_str()); printf("\n"); } @@ -1905,7 +1894,7 @@ void Dumpstate::DumpstateBoard() { static void ShowUsage() { fprintf(stderr, "usage: dumpstate [-h] [-b soundfile] [-e soundfile] [-d] [-p] " - "[-z]] [-s] [-S] [-q] [-B] [-P] [-R] [-V version]\n" + "[-z]] [-s] [-S] [-q] [-P] [-R] [-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" @@ -1915,11 +1904,8 @@ static void ShowUsage() { " -s: write output to control socket (for init)\n" " -S: write file location to control socket (for init; requires -z)\n" " -q: disable vibrate\n" - " -B: send broadcast when finished\n" - " -P: send broadcast when started and update system properties on " - "progress (requires -B)\n" - " -R: take bugreport in remote mode (requires -z, -d and -B, " - "shouldn't be used with -P)\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" " -w: start binder service and make it wait for a call to startBugreport\n" " -v: prints the dumpstate header and exit\n"); } @@ -1976,41 +1962,6 @@ bool Dumpstate::FinishZipFile() { return true; } -static std::string SHA256_file_hash(const std::string& filepath) { - android::base::unique_fd fd(TEMP_FAILURE_RETRY(open(filepath.c_str(), O_RDONLY | O_NONBLOCK - | O_CLOEXEC | O_NOFOLLOW))); - if (fd == -1) { - MYLOGE("open(%s): %s\n", filepath.c_str(), strerror(errno)); - return nullptr; - } - - SHA256_CTX ctx; - SHA256_Init(&ctx); - - std::vector buffer(65536); - while (1) { - ssize_t bytes_read = TEMP_FAILURE_RETRY(read(fd.get(), buffer.data(), buffer.size())); - if (bytes_read == 0) { - break; - } else if (bytes_read == -1) { - MYLOGE("read(%s): %s\n", filepath.c_str(), strerror(errno)); - return nullptr; - } - - SHA256_Update(&ctx, buffer.data(), bytes_read); - } - - uint8_t hash[SHA256_DIGEST_LENGTH]; - SHA256_Final(hash, &ctx); - - char hash_buffer[SHA256_DIGEST_LENGTH * 2 + 1]; - for(size_t i = 0; i < SHA256_DIGEST_LENGTH; i++) { - sprintf(hash_buffer + (i * 2), "%02x", hash[i]); - } - hash_buffer[sizeof(hash_buffer) - 1] = 0; - return std::string(hash_buffer); -} - static void SendBroadcast(const std::string& action, const std::vector& args) { // clang-format off std::vector am = {"/system/bin/cmd", "activity", "broadcast", "--user", "0", @@ -2103,37 +2054,10 @@ static void PrepareToWriteToFile() { } /* - * Finalizes writing to the file by renaming or zipping the tmp file to the final location, + * Finalizes writing to the file by zipping the tmp file to the final location, * printing zipped file status, etc. */ static void FinalizeFile() { - /* check if user changed the suffix using system properties */ - std::string name = - android::base::GetProperty(android::base::StringPrintf("dumpstate.%d.name", ds.pid_), ""); - bool change_suffix = false; - if (!name.empty()) { - /* must whitelist which characters are allowed, otherwise it could cross directories */ - std::regex valid_regex("^[-_a-zA-Z0-9]+$"); - if (std::regex_match(name.c_str(), valid_regex)) { - change_suffix = true; - } else { - MYLOGE("invalid suffix provided by user: %s\n", name.c_str()); - } - } - if (change_suffix) { - MYLOGI("changing suffix from %s to %s\n", ds.name_.c_str(), name.c_str()); - ds.name_ = name; - if (!ds.screenshot_path_.empty()) { - std::string new_screenshot_path = ds.GetPath(ds.CalledByApi() ? "-tmp.png" : ".png"); - if (rename(ds.screenshot_path_.c_str(), new_screenshot_path.c_str())) { - MYLOGE("rename(%s, %s): %s\n", ds.screenshot_path_.c_str(), - new_screenshot_path.c_str(), strerror(errno)); - } else { - ds.screenshot_path_ = new_screenshot_path; - } - } - } - bool do_text_file = true; if (ds.options_->do_zip_file) { if (!ds.FinishZipFile()) { @@ -2141,25 +2065,6 @@ static void FinalizeFile() { do_text_file = true; } else { do_text_file = false; - // If the user has changed the suffix, we need to change the zip file name. - std::string new_path = ds.GetPath(ds.CalledByApi() ? "-tmp.zip" : ".zip"); - if (ds.path_ != new_path) { - MYLOGD("Renaming zip file from %s to %s\n", ds.path_.c_str(), new_path.c_str()); - if (rename(ds.path_.c_str(), new_path.c_str())) { - MYLOGE("rename(%s, %s): %s\n", ds.path_.c_str(), new_path.c_str(), - strerror(errno)); - } else { - ds.path_ = new_path; - } - } - } - } - if (do_text_file) { - ds.path_ = ds.GetPath(".txt"); - MYLOGD("Generating .txt bugreport at %s from %s\n", ds.path_.c_str(), ds.tmp_path_.c_str()); - if (rename(ds.tmp_path_.c_str(), ds.path_.c_str())) { - MYLOGE("rename(%s, %s): %s\n", ds.tmp_path_.c_str(), ds.path_.c_str(), strerror(errno)); - ds.path_.clear(); } } if (ds.options_->use_control_socket) { @@ -2174,49 +2079,6 @@ static void FinalizeFile() { } } -/* Broadcasts that we are done with the bugreport */ -static void SendBugreportFinishedBroadcast() { - // TODO(b/111441001): use callback instead of broadcast. - if (!ds.path_.empty()) { - MYLOGI("Final bugreport path: %s\n", ds.path_.c_str()); - // clang-format off - - std::vector am_args = { - "--receiver-permission", "android.permission.DUMP", - "--ei", "android.intent.extra.ID", std::to_string(ds.id_), - "--ei", "android.intent.extra.PID", std::to_string(ds.pid_), - "--ei", "android.intent.extra.MAX", std::to_string(ds.progress_->GetMax()), - "--es", "android.intent.extra.BUGREPORT", ds.path_, - "--es", "android.intent.extra.DUMPSTATE_LOG", ds.log_path_ - }; - // clang-format on - if (ds.options_->do_fb && !android::os::IsFileEmpty(ds.screenshot_path_)) { - am_args.push_back("--es"); - am_args.push_back("android.intent.extra.SCREENSHOT"); - am_args.push_back(ds.screenshot_path_); - } - if (!ds.options_->notification_title.empty()) { - am_args.push_back("--es"); - am_args.push_back("android.intent.extra.TITLE"); - am_args.push_back(ds.options_->notification_title); - if (!ds.options_->notification_description.empty()) { - am_args.push_back("--es"); - am_args.push_back("android.intent.extra.DESCRIPTION"); - am_args.push_back(ds.options_->notification_description); - } - } - if (ds.options_->is_remote_mode) { - am_args.push_back("--es"); - am_args.push_back("android.intent.extra.REMOTE_BUGREPORT_HASH"); - am_args.push_back(SHA256_file_hash(ds.path_)); - SendBroadcast("com.android.internal.intent.action.REMOTE_BUGREPORT_FINISHED", am_args); - } else { - SendBroadcast("com.android.internal.intent.action.BUGREPORT_FINISHED", am_args); - } - } else { - MYLOGE("Skipping finished broadcast because bugreport could not be generated\n"); - } -} static inline const char* ModeToString(Dumpstate::BugreportMode mode) { switch (mode) { @@ -2238,10 +2100,9 @@ static inline const char* ModeToString(Dumpstate::BugreportMode mode) { } static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOptions* options) { - options->extra_options = ModeToString(mode); + options->bugreport_mode = ModeToString(mode); switch (mode) { case Dumpstate::BugreportMode::BUGREPORT_FULL: - options->do_broadcast = true; options->do_fb = true; break; case Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE: @@ -2249,88 +2110,32 @@ static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOpt options->do_start_service = true; options->do_progress_updates = true; options->do_fb = false; - options->do_broadcast = true; break; case Dumpstate::BugreportMode::BUGREPORT_REMOTE: options->do_vibrate = false; options->is_remote_mode = true; options->do_fb = false; - options->do_broadcast = true; 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_broadcast = true; break; case Dumpstate::BugreportMode::BUGREPORT_TELEPHONY: options->telephony_only = true; options->do_fb = false; - options->do_broadcast = true; break; case Dumpstate::BugreportMode::BUGREPORT_WIFI: options->wifi_only = true; options->do_zip_file = true; options->do_fb = false; - options->do_broadcast = true; break; case Dumpstate::BugreportMode::BUGREPORT_DEFAULT: break; } } -static Dumpstate::BugreportMode getBugreportModeFromProperty() { - // If the system property is not set, it's assumed to be a default bugreport. - Dumpstate::BugreportMode mode = Dumpstate::BugreportMode::BUGREPORT_DEFAULT; - - std::string extra_options = android::base::GetProperty(PROPERTY_EXTRA_OPTIONS, ""); - if (!extra_options.empty()) { - // Framework uses a system property to override some command-line args. - // Currently, it contains the type of the requested bugreport. - if (extra_options == "bugreportplus") { - mode = Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE; - } else if (extra_options == "bugreportfull") { - mode = Dumpstate::BugreportMode::BUGREPORT_FULL; - } else if (extra_options == "bugreportremote") { - mode = Dumpstate::BugreportMode::BUGREPORT_REMOTE; - } else if (extra_options == "bugreportwear") { - mode = Dumpstate::BugreportMode::BUGREPORT_WEAR; - } else if (extra_options == "bugreporttelephony") { - mode = Dumpstate::BugreportMode::BUGREPORT_TELEPHONY; - } else if (extra_options == "bugreportwifi") { - mode = Dumpstate::BugreportMode::BUGREPORT_WIFI; - } else { - MYLOGE("Unknown extra option: %s\n", extra_options.c_str()); - } - // Reset the property - android::base::SetProperty(PROPERTY_EXTRA_OPTIONS, ""); - } - return mode; -} - -// TODO: Move away from system properties when we have options passed via binder calls. -/* Sets runtime options from the system properties and then clears those properties. */ -static void SetOptionsFromProperties(Dumpstate::DumpOptions* options) { - Dumpstate::BugreportMode mode = getBugreportModeFromProperty(); - SetOptionsFromMode(mode, options); - - options->notification_title = android::base::GetProperty(PROPERTY_EXTRA_TITLE, ""); - if (!options->notification_title.empty()) { - // Reset the property - android::base::SetProperty(PROPERTY_EXTRA_TITLE, ""); - - options->notification_description = - android::base::GetProperty(PROPERTY_EXTRA_DESCRIPTION, ""); - if (!options->notification_description.empty()) { - // Reset the property - android::base::SetProperty(PROPERTY_EXTRA_DESCRIPTION, ""); - } - MYLOGD("notification (title: %s, description: %s)\n", options->notification_title.c_str(), - options->notification_description.c_str()); - } -} - static void LogDumpOptions(const Dumpstate::DumpOptions& options) { MYLOGI("do_zip_file: %d\n", options.do_zip_file); MYLOGI("do_add_date: %d\n", options.do_add_date); @@ -2338,7 +2143,6 @@ static void LogDumpOptions(const Dumpstate::DumpOptions& options) { MYLOGI("use_socket: %d\n", options.use_socket); MYLOGI("use_control_socket: %d\n", options.use_control_socket); MYLOGI("do_fb: %d\n", options.do_fb); - MYLOGI("do_broadcast: %d\n", options.do_broadcast); MYLOGI("is_remote_mode: %d\n", options.is_remote_mode); MYLOGI("show_header_only: %d\n", options.show_header_only); MYLOGI("do_start_service: %d\n", options.do_start_service); @@ -2346,10 +2150,8 @@ static void LogDumpOptions(const Dumpstate::DumpOptions& options) { MYLOGI("wifi_only: %d\n", options.wifi_only); MYLOGI("do_progress_updates: %d\n", options.do_progress_updates); MYLOGI("fd: %d\n", options.bugreport_fd.get()); - MYLOGI("extra_options: %s\n", options.extra_options.c_str()); + MYLOGI("bugreport_mode: %s\n", options.bugreport_mode.c_str()); MYLOGI("args: %s\n", options.args.c_str()); - MYLOGI("notification_title: %s\n", options.notification_title.c_str()); - MYLOGI("notification_description: %s\n", options.notification_description.c_str()); } void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, @@ -2364,7 +2166,6 @@ void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, bugreport_fd.reset(dup(bugreport_fd_in.get())); screenshot_fd.reset(dup(screenshot_fd_in.get())); - extra_options = ModeToString(bugreport_mode); SetOptionsFromMode(bugreport_mode, this); } @@ -2383,7 +2184,6 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) case 'p': do_fb = true; break; case 'P': do_progress_updates = true; break; case 'R': is_remote_mode = true; break; - case 'B': do_broadcast = true; break; case 'V': break; // compatibility no-op case 'w': // This was already processed @@ -2409,7 +2209,6 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) // Reset next index used by getopt so this can be called multiple times, for eg, in tests. optind = 1; - SetOptionsFromProperties(this); return status; } @@ -2418,7 +2217,7 @@ bool Dumpstate::DumpOptions::ValidateOptions() const { return false; } - if ((do_zip_file || do_add_date || do_progress_updates || do_broadcast) && !OutputToFile()) { + if ((do_zip_file || do_add_date || do_progress_updates) && !OutputToFile()) { return false; } @@ -2426,11 +2225,7 @@ bool Dumpstate::DumpOptions::ValidateOptions() const { return false; } - if (do_progress_updates && !do_broadcast) { - return false; - } - - if (is_remote_mode && (do_progress_updates || !do_broadcast || !do_zip_file || !do_add_date)) { + if (is_remote_mode && (do_progress_updates || !do_zip_file || !do_add_date)) { return false; } return true; @@ -2528,7 +2323,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, MYLOGD("dumpstate calling_uid = %d ; calling package = %s \n", calling_uid, calling_package.c_str()); - if (options_->bugreport_fd.get() != -1) { + if (CalledByApi()) { // If the output needs to be copied over to the caller's fd, get user consent. android::String16 package(calling_package.c_str()); CheckUserConsent(calling_uid, package); @@ -2573,8 +2368,8 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, MYLOGI("Running on dry-run mode (to disable it, call 'setprop dumpstate.dry_run false')\n"); } - MYLOGI("dumpstate info: id=%d, args='%s', extra_options= %s)\n", id_, options_->args.c_str(), - options_->extra_options.c_str()); + MYLOGI("dumpstate info: id=%d, args='%s', bugreport_mode= %s)\n", id_, options_->args.c_str(), + options_->bugreport_mode.c_str()); MYLOGI("bugreport format version: %s\n", version_.c_str()); @@ -2601,18 +2396,13 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, PrepareToWriteToFile(); if (options_->do_progress_updates) { - if (options_->do_broadcast) { - // clang-format off - std::vector am_args = { - "--receiver-permission", "android.permission.DUMP", - "--es", "android.intent.extra.NAME", name_, - "--ei", "android.intent.extra.ID", std::to_string(id_), - "--ei", "android.intent.extra.PID", std::to_string(pid_), - "--ei", "android.intent.extra.MAX", std::to_string(progress_->GetMax()), - }; - // clang-format on - SendBroadcast("com.android.internal.intent.action.BUGREPORT_STARTED", am_args); - } + // clang-format off + std::vector 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()); } @@ -2700,14 +2490,14 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, TEMP_FAILURE_RETRY(dup2(dup_stdout_fd, fileno(stdout))); } - // Rename, and/or zip the (now complete) .tmp file within the internal directory. + // Zip the (now complete) .tmp file within the internal directory. if (options_->OutputToFile()) { FinalizeFile(); } // Share the final file with the caller if the user has consented or Shell is the caller. Dumpstate::RunStatus status = Dumpstate::RunStatus::OK; - if (options_->bugreport_fd.get() != -1) { + if (CalledByApi()) { status = CopyBugreportIfUserConsented(calling_uid); if (status != Dumpstate::RunStatus::OK && status != Dumpstate::RunStatus::USER_CONSENT_TIMED_OUT) { @@ -2748,12 +2538,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, } } - /* tell activity manager we're done */ - if (options_->do_broadcast && !CalledByApi()) { - SendBugreportFinishedBroadcast(); - // Note that listener_ is notified in Run(); - } - MYLOGD("Final progress: %d/%d (estimated %d)\n", progress_->Get(), progress_->GetMax(), progress_->GetInitialMax()); progress_->Save(); diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 430936ebfd..4b13adc68b 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -359,7 +359,6 @@ class Dumpstate { bool use_socket = false; bool use_control_socket = false; bool do_fb = false; - bool do_broadcast = false; bool is_remote_mode = false; bool show_header_only = false; bool do_start_service = false; @@ -371,16 +370,15 @@ class Dumpstate { android::base::unique_fd bugreport_fd; // File descriptor to screenshot file. android::base::unique_fd screenshot_fd; - // TODO: rename to MODE. - // Extra options passed as system property. - std::string extra_options; + // Bugreport mode of the bugreport. + std::string bugreport_mode; // Command-line arguments as string std::string args; // Notification title and description std::string notification_title; std::string notification_description; - /* Initializes options from commandline arguments and system properties. */ + /* Initializes options from commandline arguments. */ RunStatus Initialize(int argc, char* argv[]); /* Initializes options from the requested mode. */ diff --git a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp index fbb01f5e99..b9c7568ace 100644 --- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp @@ -195,7 +195,6 @@ class ZippedBugreportGenerationTest : public Test { static Dumpstate& ds; static std::chrono::milliseconds duration; static void SetUpTestCase() { - property_set("dumpstate.options", "bugreportplus"); // clang-format off char* argv[] = { (char*)"dumpstate", diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index cff1d439d9..84eeab39ca 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -37,6 +37,7 @@ #include #include #include +#include namespace android { namespace os { @@ -148,10 +149,9 @@ class DumpOptionsTest : public Test { options_ = Dumpstate::DumpOptions(); } void TearDown() { - // Reset the property - property_set("dumpstate.options", ""); } Dumpstate::DumpOptions options_; + android::base::unique_fd fd; }; TEST_F(DumpOptionsTest, InitializeNone) { @@ -174,7 +174,6 @@ TEST_F(DumpOptionsTest, InitializeNone) { EXPECT_FALSE(options_.do_fb); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); - EXPECT_FALSE(options_.do_broadcast); } TEST_F(DumpOptionsTest, InitializeAdbBugreport) { @@ -200,7 +199,6 @@ TEST_F(DumpOptionsTest, InitializeAdbBugreport) { EXPECT_FALSE(options_.do_fb); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); - EXPECT_FALSE(options_.do_broadcast); EXPECT_FALSE(options_.use_socket); } @@ -226,28 +224,13 @@ TEST_F(DumpOptionsTest, InitializeAdbShellBugreport) { EXPECT_FALSE(options_.do_fb); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); - EXPECT_FALSE(options_.do_broadcast); } TEST_F(DumpOptionsTest, InitializeFullBugReport) { - // clang-format off - char* argv[] = { - const_cast("bugreport"), - const_cast("-d"), - const_cast("-p"), - const_cast("-B"), - const_cast("-z"), - }; - // clang-format on - property_set("dumpstate.options", "bugreportfull"); - - Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); - - EXPECT_EQ(status, Dumpstate::RunStatus::OK); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_FULL, fd, fd); EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_fb); EXPECT_TRUE(options_.do_zip_file); - EXPECT_TRUE(options_.do_broadcast); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -260,23 +243,8 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) { } TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { - // clang-format off - char* argv[] = { - const_cast("bugreport"), - const_cast("-d"), - const_cast("-p"), - const_cast("-B"), - const_cast("-z"), - }; - // clang-format on - - property_set("dumpstate.options", "bugreportplus"); - - Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); - - EXPECT_EQ(status, Dumpstate::RunStatus::OK); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, fd, fd); EXPECT_TRUE(options_.do_add_date); - EXPECT_TRUE(options_.do_broadcast); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.do_progress_updates); EXPECT_TRUE(options_.do_start_service); @@ -291,23 +259,8 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { } TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { - // clang-format off - char* argv[] = { - const_cast("bugreport"), - const_cast("-d"), - const_cast("-p"), - const_cast("-B"), - const_cast("-z"), - }; - // clang-format on - - property_set("dumpstate.options", "bugreportremote"); - - Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); - - EXPECT_EQ(status, Dumpstate::RunStatus::OK); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_REMOTE, fd, fd); EXPECT_TRUE(options_.do_add_date); - EXPECT_TRUE(options_.do_broadcast); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.is_remote_mode); EXPECT_FALSE(options_.do_vibrate); @@ -321,24 +274,9 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { } TEST_F(DumpOptionsTest, InitializeWearBugReport) { - // clang-format off - char* argv[] = { - const_cast("bugreport"), - const_cast("-d"), - const_cast("-p"), - const_cast("-B"), - const_cast("-z"), - }; - // clang-format on - - property_set("dumpstate.options", "bugreportwear"); - - Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); - - EXPECT_EQ(status, Dumpstate::RunStatus::OK); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_WEAR, fd, fd); EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_fb); - EXPECT_TRUE(options_.do_broadcast); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.do_progress_updates); EXPECT_TRUE(options_.do_start_service); @@ -352,24 +290,9 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) { } TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { - // clang-format off - char* argv[] = { - const_cast("bugreport"), - const_cast("-d"), - const_cast("-p"), - const_cast("-B"), - const_cast("-z"), - }; - // clang-format on - - property_set("dumpstate.options", "bugreporttelephony"); - - Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); - - EXPECT_EQ(status, Dumpstate::RunStatus::OK); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_TELEPHONY, fd, fd); EXPECT_TRUE(options_.do_add_date); EXPECT_FALSE(options_.do_fb); - EXPECT_TRUE(options_.do_broadcast); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.telephony_only); @@ -383,24 +306,9 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { } TEST_F(DumpOptionsTest, InitializeWifiBugReport) { - // clang-format off - char* argv[] = { - const_cast("bugreport"), - const_cast("-d"), - const_cast("-p"), - const_cast("-B"), - const_cast("-z"), - }; - // clang-format on - - property_set("dumpstate.options", "bugreportwifi"); - - Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); - - EXPECT_EQ(status, Dumpstate::RunStatus::OK); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_WIFI, fd, fd); EXPECT_TRUE(options_.do_add_date); EXPECT_FALSE(options_.do_fb); - EXPECT_TRUE(options_.do_broadcast); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.wifi_only); @@ -420,20 +328,15 @@ TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { const_cast("bugreport"), const_cast("-d"), const_cast("-p"), - const_cast("-B"), const_cast("-z"), }; // clang-format on - - property_set("dumpstate.options", ""); - Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); EXPECT_EQ(status, Dumpstate::RunStatus::OK); EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_fb); EXPECT_TRUE(options_.do_zip_file); - EXPECT_TRUE(options_.do_broadcast); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -472,7 +375,6 @@ TEST_F(DumpOptionsTest, InitializePartial1) { EXPECT_FALSE(options_.do_fb); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); - EXPECT_FALSE(options_.do_broadcast); } TEST_F(DumpOptionsTest, InitializePartial2) { @@ -484,7 +386,6 @@ TEST_F(DumpOptionsTest, InitializePartial2) { const_cast("-p"), const_cast("-P"), const_cast("-R"), - const_cast("-B"), }; // clang-format on @@ -496,7 +397,6 @@ TEST_F(DumpOptionsTest, InitializePartial2) { EXPECT_TRUE(options_.do_fb); EXPECT_TRUE(options_.do_progress_updates); EXPECT_TRUE(options_.is_remote_mode); - EXPECT_TRUE(options_.do_broadcast); // Other options retain default values EXPECT_FALSE(options_.do_add_date); @@ -544,7 +444,7 @@ TEST_F(DumpOptionsTest, ValidateOptionsNeedOutfile1) { } TEST_F(DumpOptionsTest, ValidateOptionsNeedOutfile2) { - options_.do_broadcast = true; + options_.do_progress_updates = true; // Writing to socket = !writing to file. options_.use_socket = true; EXPECT_FALSE(options_.ValidateOptions()); @@ -561,19 +461,10 @@ TEST_F(DumpOptionsTest, ValidateOptionsNeedZipfile) { EXPECT_TRUE(options_.ValidateOptions()); } -TEST_F(DumpOptionsTest, ValidateOptionsUpdateProgressNeedsBroadcast) { - options_.do_progress_updates = true; - EXPECT_FALSE(options_.ValidateOptions()); - - options_.do_broadcast = true; - EXPECT_TRUE(options_.ValidateOptions()); -} - TEST_F(DumpOptionsTest, ValidateOptionsRemoteMode) { options_.is_remote_mode = true; EXPECT_FALSE(options_.ValidateOptions()); - options_.do_broadcast = true; options_.do_zip_file = true; options_.do_add_date = true; EXPECT_TRUE(options_.ValidateOptions()); -- cgit v1.2.3-59-g8ed1b From ed5d6a6fb26534c8564e51ca2e958b59c9d2fec5 Mon Sep 17 00:00:00 2001 From: Abhijeet Kaur Date: Mon, 7 Oct 2019 15:02:05 +0100 Subject: Remove no-op setListener method from dumpstate listener Bugreports using Bugreport API flow is stable, remove unused legacy flow method: setListener. * Remove DumpstateToken as it was only used by setListener. * Remove listener_name_ and report_section_ Bug: 136066578 Test: Build and flash. Takes bugreport as expected. Test: atest dumpstate_test Test: `adb bugreport` works as expected. Change-Id: I31d1795fa91275091950320c71f4cf085895e4e0 --- cmds/dumpstate/Android.bp | 1 - cmds/dumpstate/DumpstateService.cpp | 38 +------------ cmds/dumpstate/DumpstateService.h | 4 -- cmds/dumpstate/binder/android/os/IDumpstate.aidl | 12 ---- .../binder/android/os/IDumpstateToken.aidl | 24 -------- cmds/dumpstate/dumpstate.cpp | 6 +- cmds/dumpstate/dumpstate.h | 2 - cmds/dumpstate/tests/dumpstate_smoke_test.cpp | 2 - cmds/dumpstate/tests/dumpstate_test.cpp | 66 ++++------------------ 9 files changed, 13 insertions(+), 142 deletions(-) delete mode 100644 cmds/dumpstate/binder/android/os/IDumpstateToken.aidl (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/Android.bp b/cmds/dumpstate/Android.bp index 09aee89e1b..be7e3e1a73 100644 --- a/cmds/dumpstate/Android.bp +++ b/cmds/dumpstate/Android.bp @@ -66,7 +66,6 @@ filegroup { name: "dumpstate_aidl", srcs: [ "binder/android/os/IDumpstateListener.aidl", - "binder/android/os/IDumpstateToken.aidl", "binder/android/os/IDumpstate.aidl", ], path: "binder", diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index f98df99534..2c845e6e56 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -58,8 +58,6 @@ static binder::Status exception(uint32_t code, const std::string& msg) { exit(0); } -class DumpstateToken : public BnDumpstateToken {}; - } // namespace DumpstateService::DumpstateService() : ds_(nullptr) { @@ -81,38 +79,6 @@ status_t DumpstateService::Start() { return android::OK; } -// Note: this method is part of the old flow and is not expected to be used in combination -// with startBugreport. -binder::Status DumpstateService::setListener(const std::string& name, - const sp& listener, - bool getSectionDetails, - sp* returned_token) { - *returned_token = nullptr; - if (name.empty()) { - MYLOGE("setListener(): name not set\n"); - return binder::Status::ok(); - } - if (listener == nullptr) { - MYLOGE("setListener(): listener not set\n"); - return binder::Status::ok(); - } - std::lock_guard lock(lock_); - if (ds_ == nullptr) { - ds_ = &(Dumpstate::GetInstance()); - } - if (ds_->listener_ != nullptr) { - MYLOGE("setListener(%s): already set (%s)\n", name.c_str(), ds_->listener_name_.c_str()); - return binder::Status::ok(); - } - - ds_->listener_name_ = name; - ds_->listener_ = listener; - ds_->report_section_ = getSectionDetails; - *returned_token = new DumpstateToken(); - - return binder::Status::ok(); -} - binder::Status DumpstateService::startBugreport(int32_t calling_uid, const std::string& calling_package, const android::base::unique_fd& bugreport_fd, @@ -121,8 +87,7 @@ binder::Status DumpstateService::startBugreport(int32_t calling_uid, const sp& listener) { MYLOGI("startBugreport() with mode: %d\n", bugreport_mode); - // This is the bugreporting API flow, so ensure there is only one bugreport in progress at a - // time. + // Ensure there is only one bugreport in progress at a time. std::lock_guard lock(lock_); if (ds_ != nullptr) { MYLOGE("Error! There is already a bugreport in progress. Returning."); @@ -216,7 +181,6 @@ status_t DumpstateService::dump(int fd, const Vector&) { dprintf(fd, "name: %s\n", ds_->name_.c_str()); dprintf(fd, "now: %ld\n", ds_->now_); dprintf(fd, "is_zipping: %s\n", ds_->IsZipping() ? "true" : "false"); - dprintf(fd, "listener: %s\n", ds_->listener_name_.c_str()); dprintf(fd, "notification title: %s\n", ds_->options_->notification_title.c_str()); dprintf(fd, "notification description: %s\n", ds_->options_->notification_description.c_str()); diff --git a/cmds/dumpstate/DumpstateService.h b/cmds/dumpstate/DumpstateService.h index 68eda4763a..27954adbae 100644 --- a/cmds/dumpstate/DumpstateService.h +++ b/cmds/dumpstate/DumpstateService.h @@ -24,7 +24,6 @@ #include #include "android/os/BnDumpstate.h" -#include "android/os/BnDumpstateToken.h" #include "dumpstate.h" namespace android { @@ -38,9 +37,6 @@ class DumpstateService : public BinderService, public BnDumpst static char const* getServiceName(); status_t dump(int fd, const Vector& args) override; - binder::Status setListener(const std::string& name, const sp& listener, - bool getSectionDetails, - sp* returned_token) override; binder::Status startBugreport(int32_t calling_uid, const std::string& calling_package, const android::base::unique_fd& bugreport_fd, diff --git a/cmds/dumpstate/binder/android/os/IDumpstate.aidl b/cmds/dumpstate/binder/android/os/IDumpstate.aidl index cb2d8b8d2c..3f359c86c5 100644 --- a/cmds/dumpstate/binder/android/os/IDumpstate.aidl +++ b/cmds/dumpstate/binder/android/os/IDumpstate.aidl @@ -17,24 +17,12 @@ package android.os; import android.os.IDumpstateListener; -import android.os.IDumpstateToken; /** * Binder interface for the currently running dumpstate process. * {@hide} */ interface IDumpstate { - // TODO: remove method once startBugReport is used by Shell. - /* - * Sets the listener for this dumpstate progress. - * - * Returns a token used to monitor dumpstate death, or `nullptr` if the listener was already - * set (the listener behaves like a Highlander: There Can be Only One). - * Set {@code getSectionDetails} to true in order to receive callbacks with per section - * progress details - */ - IDumpstateToken setListener(@utf8InCpp String name, IDumpstateListener listener, - boolean getSectionDetails); // NOTE: If you add to or change these modes, please also change the corresponding enums // in system server, in BugreportParams.java. diff --git a/cmds/dumpstate/binder/android/os/IDumpstateToken.aidl b/cmds/dumpstate/binder/android/os/IDumpstateToken.aidl deleted file mode 100644 index 7f74ceb539..0000000000 --- a/cmds/dumpstate/binder/android/os/IDumpstateToken.aidl +++ /dev/null @@ -1,24 +0,0 @@ -/** - * Copyright (c) 2016, The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package android.os; - -/** - * Token used by the IDumpstateListener to watch for dumpstate death. - * {@hide} - */ -interface IDumpstateToken { -} diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index b87582eab7..7c03ccfd9c 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -3686,13 +3686,11 @@ void Dumpstate::UpdateProgress(int32_t delta_sec) { if (listener_ != nullptr) { if (percent % 5 == 0) { // We don't want to spam logcat, so only log multiples of 5. - MYLOGD("Setting progress (%s): %d/%d (%d%%)\n", listener_name_.c_str(), progress, max, - percent); + MYLOGD("Setting progress: %d/%d (%d%%)\n", progress, max, percent); } else { // stderr is ignored on normal invocations, but useful when calling // /system/bin/dumpstate directly for debuggging. - fprintf(stderr, "Setting progress (%s): %d/%d (%d%%)\n", listener_name_.c_str(), - progress, max, percent); + fprintf(stderr, "Setting progress: %d/%d (%d%%)\n", progress, max, percent); } listener_->onProgress(percent); diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 430936ebfd..3139b77168 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -456,8 +456,6 @@ class Dumpstate { // Binder object listening to progress. android::sp listener_; - std::string listener_name_; - bool report_section_; // List of open tombstone dump files. std::vector tombstone_data_; diff --git a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp index fbb01f5e99..c0ac9e4611 100644 --- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp @@ -206,8 +206,6 @@ class ZippedBugreportGenerationTest : public Test { // clang-format on sp listener(new DumpstateListener(dup(fileno(stdout)), sections)); ds.listener_ = listener; - ds.listener_name_ = "Smokey"; - ds.report_section_ = true; auto start = std::chrono::steady_clock::now(); ds.ParseCommandlineAndRun(ARRAY_SIZE(argv), argv); auto end = std::chrono::steady_clock::now(); diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index cff1d439d9..7010301f9a 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -616,8 +616,8 @@ class DumpstateTest : public DumpstateBaseTest { ds.progress_.reset(new Progress(initial_max, progress, 1.2)); } - std::string GetProgressMessage(const std::string& listener_name, int progress, int max, - int old_max = 0, bool update_progress = true) { + std::string GetProgressMessage(int progress, int max, + int old_max = 0, bool update_progress = true) { EXPECT_EQ(progress, ds.progress_->Get()) << "invalid progress"; EXPECT_EQ(max, ds.progress_->GetMax()) << "invalid max"; @@ -630,9 +630,8 @@ class DumpstateTest : public DumpstateBaseTest { } if (update_progress) { - message += android::base::StringPrintf("Setting progress (%s): %d/%d (%d%%)\n", - listener_name.c_str(), progress, max, - (100 * progress / max)); + message += android::base::StringPrintf("Setting progress: %d/%d (%d%%)\n", + progress, max, (100 * progress / max)); } return message; @@ -787,18 +786,17 @@ TEST_F(DumpstateTest, RunCommandIsKilled) { TEST_F(DumpstateTest, RunCommandProgress) { sp listener(new DumpstateListenerMock()); ds.listener_ = listener; - ds.listener_name_ = "FoxMulder"; SetProgress(0, 30); EXPECT_CALL(*listener, onProgress(66)); // 20/30 % EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(20).Build())); - std::string progress_message = GetProgressMessage(ds.listener_name_, 20, 30); + std::string progress_message = GetProgressMessage(20, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); EXPECT_CALL(*listener, onProgress(80)); // 24/30 % EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(4).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 24, 30); + progress_message = GetProgressMessage(24, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); @@ -806,20 +804,20 @@ TEST_F(DumpstateTest, RunCommandProgress) { SetDryRun(true); EXPECT_CALL(*listener, onProgress(90)); // 27/30 % EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(3).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 27, 30); + progress_message = GetProgressMessage(27, 30); EXPECT_THAT(out, IsEmpty()); EXPECT_THAT(err, StrEq(progress_message)); SetDryRun(false); EXPECT_CALL(*listener, onProgress(96)); // 29/30 % EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(2).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 29, 30); + progress_message = GetProgressMessage(29, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); EXPECT_CALL(*listener, onProgress(100)); // 30/30 % EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(1).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 30, 30); + progress_message = GetProgressMessage(30, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); @@ -1044,14 +1042,12 @@ TEST_F(DumpstateTest, DumpFileOnDryRun) { TEST_F(DumpstateTest, DumpFileUpdateProgress) { sp listener(new DumpstateListenerMock()); ds.listener_ = listener; - ds.listener_name_ = "FoxMulder"; SetProgress(0, 30); EXPECT_CALL(*listener, onProgress(16)); // 5/30 % EXPECT_EQ(0, DumpFile("", kTestDataPath + "single-line.txt")); - std::string progress_message = - GetProgressMessage(ds.listener_name_, 5, 30); // TODO: unhardcode WEIGHT_FILE (5)? + std::string progress_message = GetProgressMessage(5, 30); // TODO: unhardcode WEIGHT_FILE (5)? EXPECT_THAT(err, StrEq(progress_message)); EXPECT_THAT(out, StrEq("I AM LINE1\n")); // dumpstate adds missing newline @@ -1063,48 +1059,6 @@ class DumpstateServiceTest : public DumpstateBaseTest { DumpstateService dss; }; -TEST_F(DumpstateServiceTest, SetListenerNoName) { - sp listener(new DumpstateListenerMock()); - sp token; - EXPECT_TRUE(dss.setListener("", listener, /* getSectionDetails = */ false, &token).isOk()); - ASSERT_THAT(token, IsNull()); -} - -TEST_F(DumpstateServiceTest, SetListenerNoPointer) { - sp token; - EXPECT_TRUE( - dss.setListener("whatever", nullptr, /* getSectionDetails = */ false, &token).isOk()); - ASSERT_THAT(token, IsNull()); -} - -TEST_F(DumpstateServiceTest, SetListenerTwice) { - sp listener(new DumpstateListenerMock()); - sp token; - EXPECT_TRUE( - dss.setListener("whatever", listener, /* getSectionDetails = */ false, &token).isOk()); - ASSERT_THAT(token, NotNull()); - EXPECT_THAT(Dumpstate::GetInstance().listener_name_, StrEq("whatever")); - EXPECT_FALSE(Dumpstate::GetInstance().report_section_); - - token.clear(); - EXPECT_TRUE( - dss.setListener("whatsoever", listener, /* getSectionDetails = */ false, &token).isOk()); - ASSERT_THAT(token, IsNull()); - EXPECT_THAT(Dumpstate::GetInstance().listener_name_, StrEq("whatever")); - EXPECT_FALSE(Dumpstate::GetInstance().report_section_); -} - -TEST_F(DumpstateServiceTest, SetListenerWithSectionDetails) { - sp listener(new DumpstateListenerMock()); - sp token; - Dumpstate::GetInstance().listener_ = nullptr; - EXPECT_TRUE( - dss.setListener("whatever", listener, /* getSectionDetails = */ true, &token).isOk()); - ASSERT_THAT(token, NotNull()); - EXPECT_THAT(Dumpstate::GetInstance().listener_name_, StrEq("whatever")); - EXPECT_TRUE(Dumpstate::GetInstance().report_section_); -} - class ProgressTest : public DumpstateBaseTest { public: Progress GetInstance(int32_t max, double growth_factor, const std::string& path = "") { -- cgit v1.2.3-59-g8ed1b From 3594978ab43eb1f4a41a7235beab97dd3e7202bd Mon Sep 17 00:00:00 2001 From: Sunny Goyal Date: Tue, 19 Nov 2019 15:54:36 -0800 Subject: Adding visible windows dump in bugreport Bug: 64101886 Test: Verified that bug report contain the view dump Change-Id: I802a0517cd10d4a7520b0848c2ddff77abd57871 --- cmds/dumpstate/dumpstate.cpp | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 61e22a4912..8400cdc69b 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -933,6 +933,31 @@ static void DumpIncidentReport() { unlink(path.c_str()); } +static void DumpVisibleWindowViews() { + if (!ds.IsZipping()) { + MYLOGD("Not dumping visible views because it's not a zipped bugreport\n"); + return; + } + DurationReporter duration_reporter("VISIBLE WINDOW VIEWS"); + const std::string path = ds.bugreport_internal_dir_ + "/tmp_visible_window_views"; + auto fd = android::base::unique_fd(TEMP_FAILURE_RETRY(open(path.c_str(), + O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW, + S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH))); + if (fd < 0) { + MYLOGE("Could not open %s to dump visible views.\n", path.c_str()); + return; + } + RunCommandToFd(fd, "", {"cmd", "window", "dump-visible-window-views"}, + CommandOptions::WithTimeout(120).Build()); + bool empty = 0 == lseek(fd, 0, SEEK_END); + if (!empty) { + ds.AddZipEntry("visible_windows.zip", path); + } else { + MYLOGW("Failed to dump visible windows\n"); + } + unlink(path.c_str()); +} + static void DumpIpTablesAsRoot() { RunCommand("IPTABLES", {"iptables", "-L", "-nvx"}); RunCommand("IP6TABLES", {"ip6tables", "-L", "-nvx"}); @@ -1317,6 +1342,8 @@ static Dumpstate::RunStatus dumpstate() { RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(RunCommand, "PROCRANK", {"procrank"}, AS_ROOT_20); + RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(DumpVisibleWindowViews); + DumpFile("VIRTUAL MEMORY STATS", "/proc/vmstat"); DumpFile("VMALLOC INFO", "/proc/vmallocinfo"); DumpFile("SLAB INFO", "/proc/slabinfo"); -- cgit v1.2.3-59-g8ed1b From e89d9c183ac1d7a2e86120b4b5d2610411c5a62d Mon Sep 17 00:00:00 2001 From: Jichao Li Date: Thu, 21 Nov 2019 19:02:51 -0800 Subject: For full & interactive bugreports, collect consent only after critical dumpsys has completed. This way each type of dumpstate can control the timing of the consent more precisely. Right now this is necessary because otherwise consent is going to interfere with sysui states all the time. Also some minor name changes for clarify on the way. Bug: 142921485 Test: build & flash, br captured with API does not have consent screen in critical dumpsys. i.e. Grepping ConfirmationActivity from permission controller within critical dumpsys yields no result. Change-Id: I30c1bd9559af82ebdb6dc56e91ca81d0f2745270 --- cmds/dumpstate/dumpstate.cpp | 33 +++++++++++++++++++-------------- cmds/dumpstate/dumpstate.h | 4 +++- 2 files changed, 22 insertions(+), 15 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 8400cdc69b..aa31d3256f 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1545,11 +1545,7 @@ static Dumpstate::RunStatus dumpstate() { * Returns RunStatus::USER_DENIED_CONSENT if user explicitly denied consent to sharing the bugreport * with the caller. */ -static Dumpstate::RunStatus DumpstateDefault() { - // Invoking the following dumpsys calls before DumpTraces() to try and - // keep the system stats as close to its initial state as possible. - RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(RunDumpsysCritical); - +Dumpstate::RunStatus Dumpstate::DumpstateDefaultAfterCritical() { // Capture first logcat early on; useful to take a snapshot before dumpstate logs take over the // buffer. DoLogcat(); @@ -1634,6 +1630,7 @@ static void DumpstateRadioCommon() { // This method collects dumpsys for telephony debugging only static void DumpstateTelephonyOnly() { DurationReporter duration_reporter("DUMPSTATE"); + const CommandOptions DUMPSYS_COMPONENTS_OPTIONS = CommandOptions::WithTimeout(60).Build(); DumpstateRadioCommon(); @@ -2353,11 +2350,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, MYLOGD("dumpstate calling_uid = %d ; calling package = %s \n", calling_uid, calling_package.c_str()); - if (CalledByApi()) { - // If the output needs to be copied over to the caller's fd, get user consent. - android::String16 package(calling_package.c_str()); - CheckUserConsent(calling_uid, package); - } // Redirect output if needed bool is_redirecting = options_->OutputToFile(); @@ -2500,13 +2492,23 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, PrintHeader(); if (options_->telephony_only) { + MaybeCheckUserConsent(calling_uid, calling_package); DumpstateTelephonyOnly(); DumpstateBoard(); } else if (options_->wifi_only) { + 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. + RunDumpsysCritical(); + + // Run consent check only after critical dumpsys has finished -- so the consent + // isn't going to pollute the system state / logs. + MaybeCheckUserConsent(calling_uid, calling_package); + // Dump state for the default case. This also drops root. - RunStatus s = DumpstateDefault(); + RunStatus s = DumpstateDefaultAfterCritical(); if (s != RunStatus::OK) { if (s == RunStatus::USER_CONSENT_DENIED) { HandleUserConsentDenied(); @@ -2591,17 +2593,20 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, : RunStatus::OK; } -void Dumpstate::CheckUserConsent(int32_t calling_uid, const android::String16& calling_package) { - if (calling_uid == AID_SHELL) { +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 + // bugreporting API (i.e. no fd to copy back). return; } consent_callback_ = new ConsentCallback(); const String16 incidentcompanion("incidentcompanion"); sp ics(defaultServiceManager()->getService(incidentcompanion)); + android::String16 package(calling_package.c_str()); if (ics != nullptr) { MYLOGD("Checking user consent via incidentcompanion service\n"); android::interface_cast(ics)->authorizeReport( - calling_uid, calling_package, String16(), String16(), + calling_uid, package, String16(), String16(), 0x1 /* FLAG_CONFIRMATION_DIALOG */, consent_callback_.get()); } else { MYLOGD("Unable to check user consent; incidentcompanion service unavailable\n"); diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 831574d9aa..7d9b113f1e 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -486,7 +486,9 @@ class Dumpstate { private: RunStatus RunInternal(int32_t calling_uid, const std::string& calling_package); - void CheckUserConsent(int32_t calling_uid, const android::String16& calling_package); + RunStatus DumpstateDefaultAfterCritical(); + + void MaybeCheckUserConsent(int32_t calling_uid, const std::string& calling_package); // Removes the in progress files output files (tmp file, zip/txt file, screenshot), // but leaves the log file alone. -- cgit v1.2.3-59-g8ed1b From 235c6679b8949db848004aa69e816413cd6808b6 Mon Sep 17 00:00:00 2001 From: Nandana Dutt Date: Thu, 14 Nov 2019 15:22:32 +0000 Subject: Make dumpstate logs less verbose * Combine some log lines * Log progress half as frequently BUG: 138254116 Change-Id: Ic372cc04ec3ccd582e3dfe272c6e979974837e60 --- cmds/dumpstate/dumpstate.cpp | 47 +++++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 29 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 8400cdc69b..8371e3cf6f 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2060,12 +2060,12 @@ static void PrepareToWriteToFile() { ? StringPrintf("[fd:%d]", ds.options_->bugreport_fd.get()) : ds.bugreport_internal_dir_.c_str(); MYLOGD( - "Bugreport dir: %s\n" - "Base name: %s\n" - "Suffix: %s\n" - "Log path: %s\n" - "Temporary path: %s\n" - "Screenshot path: %s\n", + "Bugreport dir: [%s] " + "Base name: [%s] " + "Suffix: [%s] " + "Log path: [%s] " + "Temporary path: [%s] " + "Screenshot path: [%s]\n", 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()); @@ -2167,21 +2167,14 @@ static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOpt } static void LogDumpOptions(const Dumpstate::DumpOptions& options) { - MYLOGI("do_zip_file: %d\n", options.do_zip_file); - MYLOGI("do_add_date: %d\n", options.do_add_date); - MYLOGI("do_vibrate: %d\n", options.do_vibrate); - MYLOGI("use_socket: %d\n", options.use_socket); - MYLOGI("use_control_socket: %d\n", options.use_control_socket); - MYLOGI("do_fb: %d\n", options.do_fb); - MYLOGI("is_remote_mode: %d\n", options.is_remote_mode); - MYLOGI("show_header_only: %d\n", options.show_header_only); - MYLOGI("do_start_service: %d\n", options.do_start_service); - MYLOGI("telephony_only: %d\n", options.telephony_only); - MYLOGI("wifi_only: %d\n", options.wifi_only); - MYLOGI("do_progress_updates: %d\n", options.do_progress_updates); - MYLOGI("fd: %d\n", options.bugreport_fd.get()); - MYLOGI("bugreport_mode: %s\n", options.bugreport_mode.c_str()); - MYLOGI("args: %s\n", options.args.c_str()); + MYLOGI( + "do_zip_file: %d do_vibrate: %d use_socket: %d use_control_socket: %d do_fb: %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 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.telephony_only, options.wifi_only, options.do_progress_updates, + options.bugreport_fd.get(), options.bugreport_mode.c_str(), options.args.c_str()); } void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, @@ -2374,8 +2367,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, id_ = ++last_id; android::base::SetProperty(PROPERTY_LAST_ID, std::to_string(last_id)); - MYLOGI("begin\n"); - if (acquire_wake_lock(PARTIAL_WAKE_LOCK, WAKE_LOCK_NAME) < 0) { MYLOGE("Failed to acquire wake lock: %s\n", strerror(errno)); } else { @@ -2398,10 +2389,8 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, MYLOGI("Running on dry-run mode (to disable it, call 'setprop dumpstate.dry_run false')\n"); } - MYLOGI("dumpstate info: id=%d, args='%s', bugreport_mode= %s)\n", id_, options_->args.c_str(), - options_->bugreport_mode.c_str()); - - MYLOGI("bugreport format version: %s\n", version_.c_str()); + MYLOGI("dumpstate info: id=%d, args='%s', bugreport_mode= %s bugreport format version: %s\n", + id_, options_->args.c_str(), options_->bugreport_mode.c_str(), version_.c_str()); do_early_screenshot_ = options_->do_progress_updates; @@ -3498,8 +3487,8 @@ void Dumpstate::UpdateProgress(int32_t delta_sec) { } if (listener_ != nullptr) { - if (percent % 5 == 0) { - // We don't want to spam logcat, so only log multiples of 5. + if (percent % 10 == 0) { + // We don't want to spam logcat, so only log multiples of 10. MYLOGD("Setting progress: %d/%d (%d%%)\n", progress, max, percent); } else { // stderr is ignored on normal invocations, but useful when calling -- cgit v1.2.3-59-g8ed1b From 8540faff16bf57e99e31d059be0379075c81615e Mon Sep 17 00:00:00 2001 From: Hunter Knepshield Date: Tue, 4 Feb 2020 19:47:20 -0800 Subject: Hook IDumpstateDevice 1.1 up to dumpstate binary. Bug reports will now prefer v1.1 of the HAL since it supports well-scoped modes that mirror BugreportManager. For new devices launching with R, v1.1 must be supported if this (optional) HAL is implemented. It's also fairly trivial to update existing devices to 1.1 if an OEM chooses. As for what this means, bug reports will: - Be smaller in many cases, and as a result, faster to collect - Contain less unnecessary PII (e.g. camera logs should now be excluded for MODE_TELEPHONY) There's still some minor cleanup to be done, but for now leaving TODOs. Bug: 143183758 Bug: 143184495 Test: atest dumpstate_test Test: take bug report on Cuttlefish and Blueline, check for v1.1 usage Change-Id: I1660ff3983931fdeed51c85f2342700c89a012aa --- cmds/dumpstate/Android.bp | 1 + cmds/dumpstate/dumpstate.cpp | 83 ++++++++++++++++++++++++--------- cmds/dumpstate/dumpstate.h | 6 +++ cmds/dumpstate/tests/dumpstate_test.cpp | 20 ++++++-- 4 files changed, 85 insertions(+), 25 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/Android.bp b/cmds/dumpstate/Android.bp index be7e3e1a73..acca11acf7 100644 --- a/cmds/dumpstate/Android.bp +++ b/cmds/dumpstate/Android.bp @@ -76,6 +76,7 @@ cc_defaults { defaults: ["dumpstate_cflag_defaults"], shared_libs: [ "android.hardware.dumpstate@1.0", + "android.hardware.dumpstate@1.1", "libziparchive", "libbase", "libbinder", diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 8e0f8a35d6..3a79357a54 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -64,6 +64,8 @@ #include #include #include +#include +#include #include #include #include @@ -85,7 +87,11 @@ #include "DumpstateService.h" #include "dumpstate.h" -using ::android::hardware::dumpstate::V1_0::IDumpstateDevice; +using IDumpstateDevice_1_0 = ::android::hardware::dumpstate::V1_0::IDumpstateDevice; +using IDumpstateDevice_1_1 = ::android::hardware::dumpstate::V1_1::IDumpstateDevice; +using ::android::hardware::dumpstate::V1_1::DumpstateMode; +using ::android::hardware::dumpstate::V1_1::DumpstateStatus; +using ::android::hardware::dumpstate::V1_1::toString; using ::std::literals::chrono_literals::operator""ms; using ::std::literals::chrono_literals::operator""s; @@ -1892,8 +1898,8 @@ void Dumpstate::DumpstateBoard() { std::bind([](std::string path) { android::os::UnlinkAndLogOnError(path); }, paths[i]))); } - sp dumpstate_device(IDumpstateDevice::getService()); - if (dumpstate_device == nullptr) { + sp dumpstate_device_1_0(IDumpstateDevice_1_0::getService()); + if (dumpstate_device_1_0 == nullptr) { MYLOGE("No IDumpstateDevice implementation\n"); return; } @@ -1924,29 +1930,54 @@ void Dumpstate::DumpstateBoard() { handle.get()->data[i] = fd.release(); } - // Given that bugreport is required to diagnose failures, it's better to - // set an arbitrary amount of timeout for IDumpstateDevice than to block the - // rest of bugreport. In the timeout case, we will kill dumpstate board HAL - // and grab whatever dumped - std::packaged_task - dumpstate_task([paths, dumpstate_device, &handle]() -> bool { - android::hardware::Return status = dumpstate_device->dumpstateBoard(handle.get()); + // Given that bugreport is required to diagnose failures, it's better to set an arbitrary amount + // of timeout for IDumpstateDevice than to block the rest of bugreport. In the timeout case, we + // will kill the HAL and grab whatever it dumped in time. + constexpr size_t timeout_sec = 30; + // Prefer version 1.1 if available. New devices launching with R are no longer allowed to + // implement just 1.0. + const char* descriptor_to_kill; + using DumpstateBoardTask = std::packaged_task; + DumpstateBoardTask dumpstate_board_task; + sp dumpstate_device_1_1( + IDumpstateDevice_1_1::castFrom(dumpstate_device_1_0)); + if (dumpstate_device_1_1 != nullptr) { + MYLOGI("Using IDumpstateDevice v1.1"); + descriptor_to_kill = IDumpstateDevice_1_1::descriptor; + dumpstate_board_task = DumpstateBoardTask([this, dumpstate_device_1_1, &handle]() -> bool { + ::android::hardware::Return status = + dumpstate_device_1_1->dumpstateBoard_1_1(handle.get(), options_->dumpstate_hal_mode, + SEC_TO_MSEC(timeout_sec)); if (!status.isOk()) { MYLOGE("dumpstateBoard failed: %s\n", status.description().c_str()); return false; + } else if (status != DumpstateStatus::OK) { + MYLOGE("dumpstateBoard failed with DumpstateStatus::%s\n", toString(status).c_str()); + return false; } return true; }); + } else { + MYLOGI("Using IDumpstateDevice v1.0"); + descriptor_to_kill = IDumpstateDevice_1_0::descriptor; + dumpstate_board_task = DumpstateBoardTask([dumpstate_device_1_0, &handle]() -> bool { + ::android::hardware::Return status = + dumpstate_device_1_0->dumpstateBoard(handle.get()); + if (!status.isOk()) { + MYLOGE("dumpstateBoard failed: %s\n", status.description().c_str()); + return false; + } + return true; + }); + } + auto result = dumpstate_board_task.get_future(); + std::thread(std::move(dumpstate_board_task)).detach(); - auto result = dumpstate_task.get_future(); - std::thread(std::move(dumpstate_task)).detach(); - - constexpr size_t timeout_sec = 30; if (result.wait_for(std::chrono::seconds(timeout_sec)) != std::future_status::ready) { MYLOGE("dumpstateBoard timed out after %zus, killing dumpstate vendor HAL\n", timeout_sec); - if (!android::base::SetProperty("ctl.interface_restart", - android::base::StringPrintf("%s/default", - IDumpstateDevice::descriptor))) { + if (!android::base::SetProperty( + "ctl.interface_restart", + android::base::StringPrintf("%s/default", descriptor_to_kill))) { MYLOGE("Couldn't restart dumpstate HAL\n"); } } @@ -1978,15 +2009,14 @@ void Dumpstate::DumpstateBoard() { continue; } AddZipEntry(kDumpstateBoardFiles[i], paths[i]); + printf("*** See %s entry ***\n", kDumpstateBoardFiles[i].c_str()); } - - printf("*** See dumpstate-board.txt entry ***\n"); } static void ShowUsage() { fprintf(stderr, "usage: dumpstate [-h] [-b soundfile] [-e soundfile] [-d] [-p] " - "[-z]] [-s] [-S] [-q] [-P] [-R] [-V version]\n" + "[-z] [-s] [-S] [-q] [-P] [-R] [-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" @@ -2196,33 +2226,40 @@ static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOpt switch (mode) { case Dumpstate::BugreportMode::BUGREPORT_FULL: options->do_fb = 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->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->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->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->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->dumpstate_hal_mode = DumpstateMode::WIFI; break; case Dumpstate::BugreportMode::BUGREPORT_DEFAULT: break; @@ -2233,11 +2270,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 " "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 args: %s\n", + "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.telephony_only, options.wifi_only, options.do_progress_updates, - options.bugreport_fd.get(), options.bugreport_mode.c_str(), options.args.c_str()); + options.bugreport_fd.get(), options.bugreport_mode.c_str(), + toString(options.dumpstate_hal_mode).c_str(), options.args.c_str()); } void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 7d9b113f1e..111c098992 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -366,6 +367,11 @@ class Dumpstate { bool wifi_only = false; // Whether progress updates should be published. bool do_progress_updates = false; + // The mode we'll use when calling IDumpstateDevice::dumpstateBoard. + // TODO(b/148168577) get rid of the AIDL values, replace them with the HAL values instead. + // The HAL is actually an API surface that can be validated, while the AIDL is not (@hide). + ::android::hardware::dumpstate::V1_1::DumpstateMode dumpstate_hal_mode = + ::android::hardware::dumpstate::V1_1::DumpstateMode::DEFAULT; // File descriptor to output zip file. android::base::unique_fd bugreport_fd; // File descriptor to screenshot file. diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index 718f4592a3..76b996081d 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -36,20 +36,22 @@ #include #include #include -#include #include +#include +#include namespace android { namespace os { namespace dumpstate { +using ::android::hardware::dumpstate::V1_1::DumpstateMode; using ::testing::EndsWith; using ::testing::HasSubstr; -using ::testing::IsNull; using ::testing::IsEmpty; +using ::testing::IsNull; using ::testing::NotNull; -using ::testing::StrEq; using ::testing::StartsWith; +using ::testing::StrEq; using ::testing::Test; using ::testing::internal::CaptureStderr; using ::testing::internal::CaptureStdout; @@ -174,6 +176,7 @@ TEST_F(DumpOptionsTest, InitializeNone) { EXPECT_FALSE(options_.do_fb); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } TEST_F(DumpOptionsTest, InitializeAdbBugreport) { @@ -200,6 +203,7 @@ TEST_F(DumpOptionsTest, InitializeAdbBugreport) { EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } TEST_F(DumpOptionsTest, InitializeAdbShellBugreport) { @@ -224,6 +228,7 @@ TEST_F(DumpOptionsTest, InitializeAdbShellBugreport) { EXPECT_FALSE(options_.do_fb); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } TEST_F(DumpOptionsTest, InitializeFullBugReport) { @@ -231,6 +236,7 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) { EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_fb); EXPECT_TRUE(options_.do_zip_file); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::FULL); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -249,6 +255,7 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { EXPECT_TRUE(options_.do_progress_updates); EXPECT_TRUE(options_.do_start_service); EXPECT_FALSE(options_.do_fb); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::INTERACTIVE); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -265,6 +272,7 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { EXPECT_TRUE(options_.is_remote_mode); EXPECT_FALSE(options_.do_vibrate); EXPECT_FALSE(options_.do_fb); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::REMOTE); // Other options retain default values EXPECT_FALSE(options_.use_control_socket); @@ -280,6 +288,7 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) { EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.do_progress_updates); EXPECT_TRUE(options_.do_start_service); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::WEAR); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -296,6 +305,7 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.telephony_only); EXPECT_TRUE(options_.do_progress_updates); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::CONNECTIVITY); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -311,6 +321,7 @@ TEST_F(DumpOptionsTest, InitializeWifiBugReport) { EXPECT_FALSE(options_.do_fb); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.wifi_only); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::WIFI); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -337,6 +348,7 @@ TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_fb); EXPECT_TRUE(options_.do_zip_file); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); // Other options retain default values EXPECT_TRUE(options_.do_vibrate); @@ -375,6 +387,7 @@ TEST_F(DumpOptionsTest, InitializePartial1) { EXPECT_FALSE(options_.do_fb); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } TEST_F(DumpOptionsTest, InitializePartial2) { @@ -403,6 +416,7 @@ TEST_F(DumpOptionsTest, InitializePartial2) { EXPECT_FALSE(options_.do_zip_file); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.use_control_socket); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } TEST_F(DumpOptionsTest, InitializeHelp) { -- cgit v1.2.3-59-g8ed1b From 0cc6c21b12cf724653ca9d7d4f9ef0296e53abe4 Mon Sep 17 00:00:00 2001 From: Hunter Knepshield Date: Tue, 7 Jan 2020 16:57:10 -0800 Subject: Include carrier services in user builds' connectivity bug reports. In order to achieve this, we give CarrierConfigLoader an extra argument during its dumpsys that tells it to also dump whatever services it's currently connected to. Back in dumpstate, we give this process a slightly larger timeout to account for the additional info. Bug: 146521742 Test: adb shell dumpsys carrier_config --requesting-package Test: take bug report from adb on user build, see no apps dumped with carrier_config Change-Id: I7953f90c6a67a4e032fc176e01cc3bd6dbadce76 --- cmds/dumpstate/dumpstate.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 3a79357a54..814a4edafd 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1672,7 +1672,7 @@ static void DumpstateRadioCommon(bool include_sensitive_info = true) { // information. This information MUST NOT identify user-installed packages (UIDs are OK, package // names are not), and MUST NOT contain logs of user application traffic. // TODO(b/148168577) rename this and other related fields/methods to "connectivity" instead. -static void DumpstateTelephonyOnly() { +static void DumpstateTelephonyOnly(const std::string& calling_package) { DurationReporter duration_reporter("DUMPSTATE"); const CommandOptions DUMPSYS_COMPONENTS_OPTIONS = CommandOptions::WithTimeout(60).Build(); @@ -1695,11 +1695,18 @@ static void DumpstateTelephonyOnly() { RunDumpsys("DUMPSYS", {"connectivity"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); - // TODO(b/146521742) build out an argument to include bound services here for user builds - RunDumpsys("DUMPSYS", {"carrier_config"}, CommandOptions::WithTimeout(90).Build(), - SEC_TO_MSEC(10)); - RunDumpsys("DUMPSYS", {"wifi"}, CommandOptions::WithTimeout(90).Build(), - SEC_TO_MSEC(10)); + if (include_sensitive_info) { + // Carrier apps' services will be dumped below in dumpsys activity service all-non-platform. + RunDumpsys("DUMPSYS", {"carrier_config"}, CommandOptions::WithTimeout(90).Build(), + SEC_TO_MSEC(10)); + } else { + // If the caller is a carrier app and has a carrier service, dump it here since we aren't + // running dumpsys activity service all-non-platform below. Due to the increased output, we + // give a higher timeout as well. + RunDumpsys("DUMPSYS", {"carrier_config", "--requesting-package", calling_package}, + CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(30)); + } + RunDumpsys("DUMPSYS", {"wifi"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); RunDumpsys("DUMPSYS", {"netpolicy"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); RunDumpsys("DUMPSYS", {"network_management"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); @@ -2587,7 +2594,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, if (options_->telephony_only) { MaybeCheckUserConsent(calling_uid, calling_package); - DumpstateTelephonyOnly(); + DumpstateTelephonyOnly(calling_package); DumpstateBoard(); } else if (options_->wifi_only) { MaybeCheckUserConsent(calling_uid, calling_package); -- cgit v1.2.3-59-g8ed1b From 0d2aad706c7977f7dc8f70e6a2fd0426e5c698e1 Mon Sep 17 00:00:00 2001 From: Paul Chang Date: Thu, 13 Feb 2020 20:04:03 +0800 Subject: Support to screenshot to interactive bugreport. - Take screenshot only after critical dumpsys has finished -- so calling app can be shown earlier without disrupting critical dumpsys after starting bugreport. - Show a visual indication to indicate screenshot is taken via IDumpstateListener.onScreenshotTaken(). - Copy the screenshot file to calling app after consent is granted for letting calling app can get and show the screenshot earlier, otherwise calling app may need to wait several minutes to get the screenshot to show. - Rename do_fb --> do_screenshot because do_fb basically means do_screenshot. BUG:149525300 Test: Flash and press bug report shortcut and check the screenshot Change-Id: Ibaca13bfabc1350448d05df12be5ee9291860c0e Merged-In: Ibaca13bfabc1350448d05df12be5ee9291860c0e --- cmds/dumpstate/DumpstateService.cpp | 2 +- .../binder/android/os/IDumpstateListener.aidl | 5 ++ cmds/dumpstate/dumpstate.cpp | 74 ++++++++++++++++------ cmds/dumpstate/dumpstate.h | 5 +- cmds/dumpstate/tests/dumpstate_smoke_test.cpp | 6 ++ cmds/dumpstate/tests/dumpstate_test.cpp | 25 ++++---- 6 files changed, 82 insertions(+), 35 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index 87ea520f84..466575f2fa 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -120,7 +120,7 @@ binder::Status DumpstateService::startBugreport(int32_t calling_uid, options->Initialize(static_cast(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 e486460753..e17f18e204 100644 --- a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl +++ b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl @@ -61,4 +61,9 @@ interface IDumpstateListener { * 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 814a4edafd..344011675d 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -647,6 +647,24 @@ android::binder::Status Dumpstate::ConsentCallback::onReportApproved() { std::lock_guard 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 @@ static Dumpstate::RunStatus dumpstate() { /* 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 @@ static void PrepareToWriteToFile() { 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 inline const char* ModeToString(Dumpstate::BugreportMode mode) { } 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 SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOpt 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 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) 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 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, 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 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, 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 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, 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 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, : 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 @@ void Dumpstate::TakeScreenshot(const std::string& path) { } 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 111c098992..7e277873cb 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -359,7 +359,8 @@ class Dumpstate { // 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 @@ class Dumpstate { 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 dac90d91b0..f26e4db976 100644 --- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp @@ -167,6 +167,12 @@ class DumpstateListener : public BnDumpstateListener { return binder::Status::ok(); } + binder::Status onScreenshotTaken(bool success) override { + std::lock_guard lock(lock_); + dprintf(out_fd_, "\rResult of taking screenshot: %s", success ? "success" : "failure"); + return binder::Status::ok(); + } + bool getIsFinished() { std::lock_guard lock(lock_); return is_finished_; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index 76b996081d..0a0c40ee0d 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -65,6 +65,7 @@ class DumpstateListenerMock : public IDumpstateListener { 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 @@ TEST_F(DumpOptionsTest, InitializeNone) { 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 @@ TEST_F(DumpOptionsTest, InitializeAdbBugreport) { // 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 @@ TEST_F(DumpOptionsTest, InitializeAdbShellBugreport) { 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, InitializeAdbShellBugreport) { 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 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { 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 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { 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, InitializeRemoteBugReport) { 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, InitializeWearBugReport) { 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, InitializeTelephonyBugReport) { 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 @@ TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { 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 @@ TEST_F(DumpOptionsTest, InitializePartial1) { // 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 @@ TEST_F(DumpOptionsTest, InitializePartial2) { 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); -- cgit v1.2.3-59-g8ed1b From 0fe51b11ad3cebb12d1973fbc250c53d6d3cc9b6 Mon Sep 17 00:00:00 2001 From: Hunter Knepshield Date: Thu, 5 Mar 2020 16:58:20 -0800 Subject: Include TelephonyRegistry in connectivity bug reports. This appears to be a simple case of omission, and it often contains useful debugging information that survives telephony process crashes since it lives in the system server. Package names have already been scrubbed from the dump (from change Iad7be2d7), and the output contains no other PII. Bug: 150409813 Bug: 146521742 Test: adb shell am bug-report --telephony, verify TelReg is dumped Change-Id: I068d50f1247d4e266d90b0557c6166cabd1bcf3f --- cmds/dumpstate/dumpstate.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 344011675d..e5bbb286ca 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1728,6 +1728,8 @@ static void DumpstateTelephonyOnly(const std::string& calling_package) { RunDumpsys("DUMPSYS", {"netpolicy"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); RunDumpsys("DUMPSYS", {"network_management"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); + RunDumpsys("DUMPSYS", {"telephony.registry"}, CommandOptions::WithTimeout(90).Build(), + SEC_TO_MSEC(10)); if (include_sensitive_info) { // Contains raw IP addresses, omit from reports on user builds. RunDumpsys("DUMPSYS", {"netd"}, CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); -- cgit v1.2.3-59-g8ed1b From d90cc65d74b26b7346d4a55704606a54b6956b34 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Sat, 8 Feb 2020 16:52:02 -0800 Subject: dumpstate: collect snapshotctl logs without root. Test: bugreport Bug: 148818798 Change-Id: I83e394420225bd9543e013e8ed71b37850d07314 Merged-In: I83e394420225bd9543e013e8ed71b37850d07314 --- cmds/dumpstate/dumpstate.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 814a4edafd..e0ed9fea10 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1459,6 +1459,8 @@ static Dumpstate::RunStatus dumpstate() { ds.AddDir(WMTRACE_DATA_DIR, false); } + ds.AddDir(SNAPSHOTCTL_LOG_DIR, false); + RUN_SLOW_FUNCTION_WITH_CONSENT_CHECK(ds.DumpstateBoard); /* Migrate the ril_dumpstate to a device specific dumpstate? */ @@ -1587,7 +1589,6 @@ Dumpstate::RunStatus Dumpstate::DumpstateDefaultAfterCritical() { ds.AddDir(RECOVERY_DATA_DIR, true); ds.AddDir(UPDATE_ENGINE_LOG_DIR, true); ds.AddDir(LOGPERSIST_DATA_DIR, false); - ds.AddDir(SNAPSHOTCTL_LOG_DIR, false); if (!PropertiesHelper::IsUserBuild()) { ds.AddDir(PROFILE_DATA_DIR_CUR, true); ds.AddDir(PROFILE_DATA_DIR_REF, true); -- cgit v1.2.3-59-g8ed1b From f59c2b7ad16b19ba00765d75d38fec74fe8f9435 Mon Sep 17 00:00:00 2001 From: Paul Chang Date: Tue, 10 Mar 2020 02:08:55 +0800 Subject: Take screenshot only when screenshot is requested - Take screenshot only when screenshot is requested for preventing from taking screenshot unnecessarily BUG: 149525300 Test: Flash and test interactive/full bugreports generated using Shell, and Shell flow does not break during tests Change-Id: Ie5cbe97f9e6e32fecc3d95893b9804b6e716c628 --- cmds/dumpstate/DumpstateService.cpp | 5 +++-- cmds/dumpstate/DumpstateService.h | 3 ++- cmds/dumpstate/binder/android/os/IDumpstate.aidl | 4 +++- cmds/dumpstate/dumpstate.cpp | 14 ++++++++------ cmds/dumpstate/dumpstate.h | 3 ++- cmds/dumpstate/tests/dumpstate_smoke_test.cpp | 8 ++++---- cmds/dumpstate/tests/dumpstate_test.cpp | 12 ++++++------ 7 files changed, 28 insertions(+), 21 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index 466575f2fa..a0b9cbbe20 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -84,7 +84,8 @@ binder::Status DumpstateService::startBugreport(int32_t calling_uid, android::base::unique_fd bugreport_fd, android::base::unique_fd screenshot_fd, int bugreport_mode, - const sp& listener) { + const sp& listener, + bool is_screenshot_requested) { MYLOGI("startBugreport() with mode: %d\n", bugreport_mode); // Ensure there is only one bugreport in progress at a time. @@ -118,7 +119,7 @@ binder::Status DumpstateService::startBugreport(int32_t calling_uid, std::unique_ptr options = std::make_unique(); options->Initialize(static_cast(bugreport_mode), bugreport_fd, - screenshot_fd); + screenshot_fd, is_screenshot_requested); if (bugreport_fd.get() == -1 || (options->do_screenshot && screenshot_fd.get() == -1)) { MYLOGE("Invalid filedescriptor"); diff --git a/cmds/dumpstate/DumpstateService.h b/cmds/dumpstate/DumpstateService.h index 6dc0225f02..ac8d3acbb5 100644 --- a/cmds/dumpstate/DumpstateService.h +++ b/cmds/dumpstate/DumpstateService.h @@ -41,7 +41,8 @@ class DumpstateService : public BinderService, public BnDumpst binder::Status startBugreport(int32_t calling_uid, const std::string& calling_package, android::base::unique_fd bugreport_fd, android::base::unique_fd screenshot_fd, int bugreport_mode, - const sp& listener) override; + const sp& listener, + bool is_screenshot_requested) override; // No-op binder::Status cancelBugreport(); diff --git a/cmds/dumpstate/binder/android/os/IDumpstate.aidl b/cmds/dumpstate/binder/android/os/IDumpstate.aidl index 3f359c86c5..ba008bb27e 100644 --- a/cmds/dumpstate/binder/android/os/IDumpstate.aidl +++ b/cmds/dumpstate/binder/android/os/IDumpstate.aidl @@ -64,10 +64,12 @@ interface IDumpstate { * @param screenshotFd the file to which screenshot should be written * @param bugreportMode the mode that specifies other run time options; must be one of above * @param listener callback for updates; optional + * @param isScreenshotRequested indicates screenshot is requested or not */ void startBugreport(int callingUid, @utf8InCpp String callingPackage, FileDescriptor bugreportFd, FileDescriptor screenshotFd, - int bugreportMode, IDumpstateListener listener); + int bugreportMode, IDumpstateListener listener, + boolean isScreenshotRequested); /* * Cancels the bugreport currently in progress. diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index e7029bd0db..733b71eb56 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2247,20 +2247,21 @@ static inline const char* ModeToString(Dumpstate::BugreportMode mode) { } } -static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOptions* options) { +static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOptions* options, + bool is_screenshot_requested) { // 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_screenshot = true; + options->do_screenshot = is_screenshot_requested; 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_screenshot = true; + options->do_screenshot = is_screenshot_requested; options->dumpstate_hal_mode = DumpstateMode::INTERACTIVE; break; case Dumpstate::BugreportMode::BUGREPORT_REMOTE: @@ -2273,7 +2274,7 @@ static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOpt options->do_start_service = true; options->do_progress_updates = true; options->do_zip_file = true; - options->do_screenshot = true; + options->do_screenshot = is_screenshot_requested; options->dumpstate_hal_mode = DumpstateMode::WEAR; break; // TODO(b/148168577) rename TELEPHONY everywhere to CONNECTIVITY. @@ -2310,7 +2311,8 @@ static void LogDumpOptions(const Dumpstate::DumpOptions& options) { void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, const android::base::unique_fd& bugreport_fd_in, - const android::base::unique_fd& screenshot_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; @@ -2320,7 +2322,7 @@ void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, bugreport_fd.reset(dup(bugreport_fd_in.get())); screenshot_fd.reset(dup(screenshot_fd_in.get())); - SetOptionsFromMode(bugreport_mode, this); + SetOptionsFromMode(bugreport_mode, this, is_screenshot_requested); } Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) { diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 7e277873cb..7b8d2821e5 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -390,7 +390,8 @@ class Dumpstate { /* Initializes options from the requested mode. */ void Initialize(BugreportMode bugreport_mode, const android::base::unique_fd& bugreport_fd, - const android::base::unique_fd& screenshot_fd); + const android::base::unique_fd& screenshot_fd, + bool is_screenshot_requested); /* Returns true if the options set so far are consistent. */ bool ValidateOptions() const; diff --git a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp index f26e4db976..047a1c38f0 100644 --- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp @@ -451,7 +451,7 @@ TEST_F(DumpstateBinderTest, Baseline) { sp listener(new DumpstateListener(dup(fileno(stdout)))); android::binder::Status status = ds_binder->startBugreport(123, "com.dummy.package", std::move(bugreport_fd), std::move(screenshot_fd), - Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener); + Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener, true); // startBugreport is an async call. Verify binder call succeeded first, then wait till listener // gets expected callbacks. EXPECT_TRUE(status.isOk()); @@ -488,7 +488,7 @@ TEST_F(DumpstateBinderTest, ServiceDies_OnInvalidInput) { android::binder::Status status = ds_binder->startBugreport(123, "com.dummy.package", std::move(bugreport_fd), std::move(screenshot_fd), 2000, // invalid bugreport mode - listener); + listener, false); EXPECT_EQ(listener->getErrorCode(), IDumpstateListener::BUGREPORT_ERROR_INVALID_INPUT); // The service should have died, freeing itself up for a new invocation. @@ -519,13 +519,13 @@ TEST_F(DumpstateBinderTest, SimultaneousBugreportsNotAllowed) { sp listener1(new DumpstateListener(dup(fileno(stdout)))); android::binder::Status status = ds_binder->startBugreport(123, "com.dummy.package", std::move(bugreport_fd), std::move(screenshot_fd), - Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener1); + Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener1, true); EXPECT_TRUE(status.isOk()); // try to make another call to startBugreport. This should fail. sp listener2(new DumpstateListener(dup(fileno(stdout)))); status = ds_binder->startBugreport(123, "com.dummy.package", std::move(bugreport_fd2), std::move(screenshot_fd2), - Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener2); + Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener2, true); EXPECT_FALSE(status.isOk()); WaitTillExecutionComplete(listener2.get()); EXPECT_EQ(listener2->getErrorCode(), diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index 0a0c40ee0d..2efb130776 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -233,7 +233,7 @@ TEST_F(DumpOptionsTest, InitializeAdbShellBugreport) { } TEST_F(DumpOptionsTest, InitializeFullBugReport) { - options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_FULL, fd, fd); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_FULL, fd, fd, true); EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_screenshot); EXPECT_TRUE(options_.do_zip_file); @@ -250,7 +250,7 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) { } TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { - options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, fd, fd); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, fd, fd, true); EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.do_progress_updates); @@ -267,7 +267,7 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { } TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { - options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_REMOTE, fd, fd); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_REMOTE, fd, fd, false); EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_zip_file); EXPECT_TRUE(options_.is_remote_mode); @@ -283,7 +283,7 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { } TEST_F(DumpOptionsTest, InitializeWearBugReport) { - options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_WEAR, fd, fd); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_WEAR, fd, fd, true); EXPECT_TRUE(options_.do_add_date); EXPECT_TRUE(options_.do_screenshot); EXPECT_TRUE(options_.do_zip_file); @@ -300,7 +300,7 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) { } TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { - options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_TELEPHONY, fd, fd); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_TELEPHONY, fd, fd, false); EXPECT_TRUE(options_.do_add_date); EXPECT_FALSE(options_.do_screenshot); EXPECT_TRUE(options_.do_zip_file); @@ -317,7 +317,7 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { } TEST_F(DumpOptionsTest, InitializeWifiBugReport) { - options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_WIFI, fd, fd); + options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_WIFI, fd, fd, false); EXPECT_TRUE(options_.do_add_date); EXPECT_FALSE(options_.do_screenshot); EXPECT_TRUE(options_.do_zip_file); -- cgit v1.2.3-59-g8ed1b From 9d17b9456729d16bd34c868e6f660b022816d579 Mon Sep 17 00:00:00 2001 From: Nandana Dutt Date: Thu, 26 Mar 2020 10:02:59 +0000 Subject: Rename tmp files Bugreport handling apps know how to filter out .tmp files but not *.tmp.[ext] files. Fix by moving tmp to the end. Fixes: 148863019 Test: take bugreport, check /bugreports to verify all tmp files have .tmp suffix Test: atest dumpstate_test Change-Id: I559cef85e5ff5f8a463038b4e4ebfd48063bb39f --- cmds/dumpstate/dumpstate.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index ec2b92255f..01b7e635f0 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2171,7 +2171,7 @@ static void PrepareToWriteToFile() { } if (ds.options_->do_screenshot) { - ds.screenshot_path_ = ds.GetPath(ds.CalledByApi() ? "-tmp.png" : ".png"); + ds.screenshot_path_ = ds.GetPath(ds.CalledByApi() ? "-png.tmp" : ".png"); } ds.tmp_path_ = ds.GetPath(".tmp"); ds.log_path_ = ds.GetPath("-dumpstate_log-" + std::to_string(ds.pid_) + ".txt"); @@ -2190,7 +2190,7 @@ static void PrepareToWriteToFile() { ds.tmp_path_.c_str(), ds.screenshot_path_.c_str()); if (ds.options_->do_zip_file) { - ds.path_ = ds.GetPath(ds.CalledByApi() ? "-tmp.zip" : ".zip"); + 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")); -- cgit v1.2.3-59-g8ed1b From 8ae16e67ee8d12ec14d08c0445533ff6da2988b4 Mon Sep 17 00:00:00 2001 From: Nandana Dutt Date: Fri, 27 Mar 2020 10:20:22 +0000 Subject: Handle cancelBugreport Clean up tmp files on receiving cancelBugreport. BUG: 146994281 Test: manual Change-Id: Id759e088ecff8d3098e58470626dea8d5bf1bc20 --- cmds/dumpstate/DumpstateService.cpp | 7 +++---- cmds/dumpstate/dumpstate.cpp | 19 ++++++++++++++++--- cmds/dumpstate/dumpstate.h | 5 ++++- 3 files changed, 23 insertions(+), 8 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index a0b9cbbe20..1824943eb2 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -148,14 +148,13 @@ binder::Status DumpstateService::startBugreport(int32_t calling_uid, } binder::Status DumpstateService::cancelBugreport() { - // This is a no-op since the cancellation is done from java side via setting sys properties. - // See BugreportManagerServiceImpl. - // TODO(b/111441001): maybe make native and java sides use different binder interface - // to avoid these annoyances. + std::lock_guard lock(lock_); + ds_->Cancel(); return binder::Status::ok(); } status_t DumpstateService::dump(int fd, const Vector&) { + std::lock_guard lock(lock_); if (ds_ == nullptr) { dprintf(fd, "Bugreport not in progress yet"); return NO_ERROR; diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index ec2b92255f..83a34a3af2 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -239,6 +239,9 @@ static bool CopyFileToFd(const std::string& input_file, int out_fd) { } static bool UnlinkAndLogOnError(const std::string& file) { + if (file.empty()) { + return false; + } if (unlink(file.c_str())) { MYLOGE("Failed to unlink file (%s): %s\n", file.c_str(), strerror(errno)); return false; @@ -246,7 +249,6 @@ static bool UnlinkAndLogOnError(const std::string& file) { return true; } - int64_t GetModuleMetadataVersion() { auto binder = defaultServiceManager()->getService(android::String16("package_native")); if (binder == nullptr) { @@ -2419,6 +2421,17 @@ Dumpstate::RunStatus Dumpstate::Run(int32_t calling_uid, const std::string& call return status; } +void Dumpstate::Cancel() { + CleanupTmpFiles(); + android::os::UnlinkAndLogOnError(log_path_); + for (int i = 0; i < NUM_OF_DUMPS; i++) { + android::os::UnlinkAndLogOnError(ds.bugreport_internal_dir_ + "/" + + kDumpstateBoardFiles[i]); + } + tombstone_data_.clear(); + anr_data_.clear(); +} + /* * Dumps relevant information to a bugreport based on the given options. * @@ -2755,7 +2768,7 @@ bool Dumpstate::CalledByApi() const { return ds.options_->bugreport_fd.get() != -1 ? true : false; } -void Dumpstate::CleanupFiles() { +void Dumpstate::CleanupTmpFiles() { android::os::UnlinkAndLogOnError(tmp_path_); android::os::UnlinkAndLogOnError(screenshot_path_); android::os::UnlinkAndLogOnError(path_); @@ -2763,7 +2776,7 @@ void Dumpstate::CleanupFiles() { Dumpstate::RunStatus Dumpstate::HandleUserConsentDenied() { MYLOGD("User denied consent; deleting files and returning\n"); - CleanupFiles(); + CleanupTmpFiles(); return USER_CONSENT_DENIED; } diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 7b8d2821e5..9ce662b255 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -334,6 +334,9 @@ class Dumpstate { RunStatus ParseCommandlineAndRun(int argc, char* argv[]); + /* Deletes in-progress files */ + void Cancel(); + /* Sets runtime options. */ void SetOptions(std::unique_ptr options); @@ -502,7 +505,7 @@ class Dumpstate { // Removes the in progress files output files (tmp file, zip/txt file, screenshot), // but leaves the log file alone. - void CleanupFiles(); + void CleanupTmpFiles(); RunStatus HandleUserConsentDenied(); -- cgit v1.2.3-59-g8ed1b From a407fb81372c6571c28d603504b23ece2d6bc79e Mon Sep 17 00:00:00 2001 From: Abhijeet Kaur Date: Fri, 27 Mar 2020 12:51:12 +0000 Subject: Update Last ID early in the bugreport generation The lifecycle of a bugreport triggered by Shell: 1. Triggers a bugreport using bugreport API. At this point shell does not have any state of the bugreport. mBugreportInfos is the sparse array in Shell that tracks bugreportInfos keyed with the ID. The ID of the bugreport is currently incremented in RunInternal() which is called in a different thread triggered by the DumpstateService (for an API call) 2. dumpstate calls one of the callbacks #onProgress, #onError and #onFinished(), it is then that the corresponding bugreportinfo is tracked in mBugreportInfos keyed with the ID. Problem: Consider a case when 2 bugreports are triggered such that Shell has not received #onProgress for the first one, and received #onError for the second one. This results in a race condition as Shell assumes that the first bugreport has called onError. This race condition occurs due to the fact that Shell depends on the callback to track the bugreports. Solution: Update Id at a definite time (not in some other thread) in Initialize so that the service calling the API can track it right away. Bug: 152292912 Test: Tigger consecutive calls to bugreport from ActivityManager is WAI Test: adb bugreport (open the finished bugreport and check that the correct ID is shown) Test: atest dumpstate_test Change-Id: I63966800725d2aaa1d1d9d6eb997b86d564acf44 --- cmds/dumpstate/DumpstateService.cpp | 2 ++ cmds/dumpstate/dumpstate.cpp | 17 ++++++++++------- cmds/dumpstate/dumpstate.h | 10 +++++++++- 3 files changed, 21 insertions(+), 8 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index 1824943eb2..bfcc058c1b 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -137,6 +137,8 @@ binder::Status DumpstateService::startBugreport(int32_t calling_uid, ds_info->calling_package = calling_package; pthread_t thread; + // Initialize dumpstate + ds_->Initialize(); status_t err = pthread_create(&thread, nullptr, dumpstate_thread_main, ds_info); if (err != 0) { delete ds_info; diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 772b9fe069..b475c90c04 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2395,6 +2395,13 @@ void Dumpstate::SetOptions(std::unique_ptr options) { options_ = std::move(options); } +void Dumpstate::Initialize() { + /* gets the sequential id */ + uint32_t last_id = android::base::GetIntProperty(PROPERTY_LAST_ID, 0); + id_ = ++last_id; + android::base::SetProperty(PROPERTY_LAST_ID, std::to_string(last_id)); +} + Dumpstate::RunStatus Dumpstate::Run(int32_t calling_uid, const std::string& calling_package) { Dumpstate::RunStatus status = RunInternal(calling_uid, calling_package); if (listener_ != nullptr) { @@ -2505,11 +2512,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, : ""; progress_.reset(new Progress(stats_path)); - /* gets the sequential id */ - uint32_t last_id = android::base::GetIntProperty(PROPERTY_LAST_ID, 0); - id_ = ++last_id; - android::base::SetProperty(PROPERTY_LAST_ID, std::to_string(last_id)); - if (acquire_wake_lock(PARTIAL_WAKE_LOCK, WAKE_LOCK_NAME) < 0) { MYLOGE("Failed to acquire wake lock: %s\n", strerror(errno)); } else { @@ -2837,8 +2839,9 @@ Dumpstate::RunStatus Dumpstate::ParseCommandlineAndRun(int argc, char* argv[]) { assert(options_->bugreport_fd.get() == -1); // calling_uid and calling_package are for user consent to share the bugreport with - // an app; they are irrelvant here because bugreport is only written to a local - // directory, and not shared. + // an app; they are irrelevant here because bugreport is triggered via command line. + // Update Last ID before calling Run(). + Initialize(); status = Run(-1 /* calling_uid */, "" /* calling_package */); } return status; diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 9ce662b255..498f338227 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -216,6 +216,9 @@ class Dumpstate { /* Checkes whether dumpstate is generating a zipped bugreport. */ bool IsZipping() const; + /* Initialize dumpstate fields before starting bugreport generation */ + void Initialize(); + /* * Forks a command, waits for it to finish, and returns its status. * @@ -329,7 +332,12 @@ class Dumpstate { struct DumpOptions; - /* Main entry point for running a complete bugreport. */ + /* + * Main entry point for running a complete bugreport. + * + * Initialize() dumpstate before calling this method. + * + */ RunStatus Run(int32_t calling_uid, const std::string& calling_package); RunStatus ParseCommandlineAndRun(int argc, char* argv[]); -- cgit v1.2.3-59-g8ed1b From 9ce94670ce0c3e7b96bb10f561169eaeb766c801 Mon Sep 17 00:00:00 2001 From: Abhijeet Kaur Date: Wed, 1 Apr 2020 17:22:36 +0100 Subject: Copy Screenshot to fd if and only if user consent approved Currently the dumpstate screenshot file is copied to the screenshot fd (provided by the API caller) even when user consent times out. Bug: 152944488 Test: Manual Change-Id: I0ede8bc8cf5333ea586218a37fd6456f782a50c7 --- cmds/dumpstate/dumpstate.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index b475c90c04..c85c7cf048 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2677,15 +2677,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, MYLOGI("User denied consent. Returning\n"); return status; } - 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) { - android::os::UnlinkAndLogOnError(screenshot_path_); - } - } if (status == Dumpstate::RunStatus::USER_CONSENT_TIMED_OUT) { MYLOGI( "Did not receive user consent yet." @@ -2815,6 +2806,16 @@ Dumpstate::RunStatus Dumpstate::CopyBugreportIfUserConsented(int32_t calling_uid bool copy_succeeded = android::os::CopyFileToFd(path_, options_->bugreport_fd.get()); if (copy_succeeded) { android::os::UnlinkAndLogOnError(path_); + if (options_->do_screenshot && + options_->screenshot_fd.get() != -1 && + !options_->is_screenshot_copied) { + copy_succeeded = android::os::CopyFileToFd(screenshot_path_, + options_->screenshot_fd.get()); + options_->is_screenshot_copied = copy_succeeded; + if (copy_succeeded) { + android::os::UnlinkAndLogOnError(screenshot_path_); + } + } } return copy_succeeded ? Dumpstate::RunStatus::OK : Dumpstate::RunStatus::ERROR; } else if (consent_result == UserConsentResult::UNAVAILABLE) { -- cgit v1.2.3-59-g8ed1b From c490e66cee92edccbc9135b3fd1c14d6a2090a39 Mon Sep 17 00:00:00 2001 From: Paul Chang Date: Sat, 11 Apr 2020 18:14:09 +0800 Subject: Support to let listener know critical dump is finished BUG: 153809412 Test: atest dumpstate_test Change-Id: Id4b5c762e335adaa645fabca2b00e378a0c30ce4 --- cmds/dumpstate/binder/android/os/IDumpstateListener.aidl | 5 +++++ cmds/dumpstate/dumpstate.cpp | 16 ++++++++++++++++ cmds/dumpstate/dumpstate.h | 3 +++ cmds/dumpstate/tests/dumpstate_smoke_test.cpp | 9 +++++++++ cmds/dumpstate/tests/dumpstate_test.cpp | 2 ++ 5 files changed, 35 insertions(+) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl index 8ff4ca6474..a5e6c68363 100644 --- a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl +++ b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl @@ -68,4 +68,9 @@ interface IDumpstateListener { * Called when screenshot is taken. */ oneway void onScreenshotTaken(boolean success); + + /** + * Called when ui intensive bugreport dumps are finished. + */ + oneway void onUiIntensiveBugreportDumpsFinished(String callingPackage); } diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index c85c7cf048..19111198ab 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2630,11 +2630,13 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, if (options_->telephony_only) { MaybeTakeEarlyScreenshot(); + onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); MaybeCheckUserConsent(calling_uid, calling_package); DumpstateTelephonyOnly(calling_package); DumpstateBoard(); } else if (options_->wifi_only) { MaybeTakeEarlyScreenshot(); + onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); MaybeCheckUserConsent(calling_uid, calling_package); DumpstateWifiOnly(); } else { @@ -2643,6 +2645,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, // Take screenshot and get consent only after critical dumpsys has finished. MaybeTakeEarlyScreenshot(); + onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); MaybeCheckUserConsent(calling_uid, calling_package); // Dump state for the default case. This also drops root. @@ -2732,6 +2735,19 @@ void Dumpstate::MaybeTakeEarlyScreenshot() { TakeScreenshot(); } +void Dumpstate::onUiIntensiveBugreportDumpsFinished(int32_t calling_uid, + const std::string& calling_package) { + if (calling_uid == AID_SHELL || !CalledByApi()) { + return; + } + if (listener_ != nullptr) { + // Let listener know ui intensive bugreport dumps are finished, then it can do event + // handling if required. + android::String16 package(calling_package.c_str()); + listener_->onUiIntensiveBugreportDumpsFinished(package); + } +} + 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 diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 498f338227..28d8936828 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -509,6 +509,9 @@ class Dumpstate { void MaybeTakeEarlyScreenshot(); + void onUiIntensiveBugreportDumpsFinished(int32_t calling_uid, + const std::string& calling_package); + 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 047a1c38f0..6f2d75403d 100644 --- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp @@ -173,6 +173,15 @@ class DumpstateListener : public BnDumpstateListener { return binder::Status::ok(); } + binder::Status onUiIntensiveBugreportDumpsFinished(const android::String16& callingpackage) + override { + std::lock_guard lock(lock_); + std::string callingpackageUtf8 = std::string(String8(callingpackage).string()); + dprintf(out_fd_, "\rCalling package of ui intensive bugreport dumps finished: %s", + callingpackageUtf8.c_str()); + return binder::Status::ok(); + } + bool getIsFinished() { std::lock_guard lock(lock_); return is_finished_; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index 2efb130776..9871a3bd6c 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -66,6 +66,8 @@ class DumpstateListenerMock : public IDumpstateListener { MOCK_METHOD1(onError, binder::Status(int32_t error_code)); MOCK_METHOD0(onFinished, binder::Status()); MOCK_METHOD1(onScreenshotTaken, binder::Status(bool success)); + MOCK_METHOD1(onUiIntensiveBugreportDumpsFinished, + binder::Status(const android::String16& callingpackage)); protected: MOCK_METHOD0(onAsBinder, IBinder*()); -- cgit v1.2.3-59-g8ed1b From f84d369ea42a77fb75641885ec264952cbd35ae3 Mon Sep 17 00:00:00 2001 From: Calin Juravle Date: Tue, 28 Apr 2020 15:31:12 -0700 Subject: Add PackageDexUse file to dumpstate It will allows us to investigate issues related to apks class loader context Test: atest DumpstateTest Bug: 154789494 Change-Id: If5d2bfa005350d36006c07cdd38bf2c01417fd18 --- cmds/dumpstate/dumpstate.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index c85c7cf048..e3eb06365d 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -169,6 +169,7 @@ void add_mountinfo(); #define OTA_METADATA_DIR "/metadata/ota" #define SNAPSHOTCTL_LOG_DIR "/data/misc/snapshotctl_log" #define LINKERCONFIG_DIR "/linkerconfig" +#define PACKAGE_DEX_USE_LIST "/data/system/package-dex-usage.list" // TODO(narayan): Since this information has to be kept in sync // with tombstoned, we should just put it in a common header. @@ -1612,6 +1613,7 @@ Dumpstate::RunStatus Dumpstate::DumpstateDefaultAfterCritical() { if (!PropertiesHelper::IsUserBuild()) { ds.AddDir(PROFILE_DATA_DIR_CUR, true); ds.AddDir(PROFILE_DATA_DIR_REF, true); + ds.AddZipEntry(ZIP_ROOT_DIR + PACKAGE_DEX_USE_LIST, PACKAGE_DEX_USE_LIST); } ds.AddDir(PREREBOOT_DATA_DIR, false); add_mountinfo(); -- cgit v1.2.3-59-g8ed1b From f56bbb2bc0d65190bdb660ab359eb4697e928245 Mon Sep 17 00:00:00 2001 From: mhasank Date: Tue, 26 May 2020 18:02:39 -0700 Subject: arc: Implement smaller dumpstate output Adds a -A commandline option to indicate a much smaller set of dumpstate output safe for ARC++ bug reports. Implements output of system logcat, event logs, networking, and dropbox system server crash/system app crashes only. Bug: 142684959 Bug: 136273873 Test: atest dumpstate_test Change-Id: I192be7ed841cee0a8847e1057209ef2b164bab07 --- cmds/dumpstate/dumpstate.cpp | 68 +++++++++++++++++++++++++++++---- cmds/dumpstate/dumpstate.h | 2 + cmds/dumpstate/main.cpp | 2 +- cmds/dumpstate/tests/dumpstate_test.cpp | 42 ++++++++++++++++++++ 4 files changed, 106 insertions(+), 8 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 9ba4819ee8..ba8dc46596 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1362,6 +1362,53 @@ static void DumpExternalFragmentationInfo() { printf("\n"); } +static void DumpstateArcOnly() { + // Trimmed-down version of dumpstate to only include a whitelisted + // set of logs (system log, event log, and system server / system app + // crashes, and ARC networking logs). See b/136273873 and b/138459828 + // for context. New sections must be first approved by Chrome OS Privacy + // and then added to server side cros monitoring PII scrubber before adding + // them here. See cl/312126645 for an example. + DurationReporter duration_reporter("DUMPSTATE"); + unsigned long timeout_ms; + // calculate timeout + timeout_ms = logcat_timeout({"main", "system", "crash"}); + RunCommand("SYSTEM LOG", + {"logcat", "-v", "threadtime", "-v", "printable", "-v", "uid", "-d", "*:v"}, + CommandOptions::WithTimeoutInMs(timeout_ms).Build()); + timeout_ms = logcat_timeout({"events"}); + RunCommand( + "EVENT LOG", + {"logcat", "-b", "events", "-v", "threadtime", "-v", "printable", "-v", "uid", "-d", "*:v"}, + CommandOptions::WithTimeoutInMs(timeout_ms).Build()); + + printf("========================================================\n"); + printf("== Networking Service\n"); + printf("========================================================\n"); + + // ARC networking service implements dumpsys by reusing the 'wifi' service name. + // The top-level handler is implemented in handleDump() in + // vendor/google_arc/libs/arc-services/src/com/android/server/arc/net/ArcNetworkService.java. + // It outputs a subset of Android system server state relevant for debugging ARC + // connectivity issues, in a PII-free manner. See b/147270970. + RunDumpsys("DUMPSYS NETWORK_SERVICE_LIMITED", {"wifi", "-a"}, + CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); + + printf("========================================================\n"); + printf("== Dropbox crashes\n"); + printf("========================================================\n"); + + RunDumpsys("DROPBOX SYSTEM SERVER CRASHES", {"dropbox", "-p", "system_server_crash"}); + RunDumpsys("DROPBOX SYSTEM APP CRASHES", {"dropbox", "-p", "system_app_crash"}); + + printf("========================================================\n"); + printf("== Final progress (pid %d): %d/%d (estimated %d)\n", ds.pid_, ds.progress_->Get(), + ds.progress_->GetMax(), ds.progress_->GetInitialMax()); + printf("========================================================\n"); + printf("== dumpstate: done (id %d)\n", ds.id_); + printf("========================================================\n"); +} + // Dumps various things. Returns early with status USER_CONSENT_DENIED if user denies consent // via the consent they are shown. Ignores other errors that occur while running various // commands. The consent checking is currently done around long running tasks, which happen to @@ -2048,7 +2095,7 @@ void Dumpstate::DumpstateBoard() { static void ShowUsage() { fprintf(stderr, "usage: dumpstate [-h] [-b soundfile] [-e soundfile] [-d] [-p] " - "[-z] [-s] [-S] [-q] [-P] [-R] [-V version]\n" + "[-z] [-s] [-S] [-q] [-P] [-R] [-A] [-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" @@ -2061,6 +2108,7 @@ static void ShowUsage() { " -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" " -w: start binder service and make it wait for a call to startBugreport\n" + " -A: output limited information that is safe for submission in ARC++ bugreports\n" " -v: prints the dumpstate header and exit\n"); } @@ -2306,13 +2354,12 @@ static void LogDumpOptions(const Dumpstate::DumpOptions& options) { "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", + "arc_only: %d args: %s\n", options.do_zip_file, options.do_vibrate, options.use_socket, options.use_control_socket, 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()); + 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.arc_only, options.args.c_str()); } void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, @@ -2334,7 +2381,7 @@ void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) { RunStatus status = RunStatus::OK; int c; - while ((c = getopt(argc, argv, "dho:svqzpPBRSV:w")) != -1) { + while ((c = getopt(argc, argv, "dho:svqzpAPBRSV:w")) != -1) { switch (c) { // clang-format off case 'd': do_add_date = true; break; @@ -2346,6 +2393,7 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) case 'p': do_screenshot = true; break; case 'P': do_progress_updates = true; break; case 'R': is_remote_mode = true; break; + case 'A': arc_only = true; break; case 'V': break; // compatibility no-op case 'w': // This was already processed @@ -2630,6 +2678,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, // duration is logged into MYLOG instead. PrintHeader(); + // TODO(nandana) reduce code repetition in if branches if (options_->telephony_only) { MaybeTakeEarlyScreenshot(); onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); @@ -2641,6 +2690,11 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); MaybeCheckUserConsent(calling_uid, calling_package); DumpstateWifiOnly(); + } else if (options_->arc_only) { + MaybeTakeEarlyScreenshot(); + onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); + MaybeCheckUserConsent(calling_uid, calling_package); + DumpstateArcOnly(); } else { // Invoke critical dumpsys first to preserve system state, before doing anything else. RunDumpsysCritical(); diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 28d8936828..c8c9f452f0 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -377,6 +377,8 @@ class Dumpstate { bool do_start_service = false; bool telephony_only = false; bool wifi_only = false; + // Trimmed-down version of dumpstate to only include whitelisted logs. + bool arc_only = false; // Whether progress updates should be published. bool do_progress_updates = false; // The mode we'll use when calling IDumpstateDevice::dumpstateBoard. diff --git a/cmds/dumpstate/main.cpp b/cmds/dumpstate/main.cpp index 68d373377c..f1342a5b53 100644 --- a/cmds/dumpstate/main.cpp +++ b/cmds/dumpstate/main.cpp @@ -30,7 +30,7 @@ bool ShouldStartServiceAndWait(int argc, char* argv[]) { bool do_wait = false; int c; // Keep flags in sync with Dumpstate::DumpOptions::Initialize. - while ((c = getopt(argc, argv, "wdho:svqzpPBRSV:")) != -1 && !do_wait) { + while ((c = getopt(argc, argv, "dho:svqzpAPBRSV:w")) != -1 && !do_wait) { switch (c) { case 'w': do_wait = true; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index 9871a3bd6c..7078521b52 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -179,6 +179,7 @@ TEST_F(DumpOptionsTest, InitializeNone) { EXPECT_FALSE(options_.do_screenshot); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); + EXPECT_FALSE(options_.arc_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -206,6 +207,7 @@ TEST_F(DumpOptionsTest, InitializeAdbBugreport) { EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.arc_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -231,6 +233,7 @@ TEST_F(DumpOptionsTest, InitializeAdbShellBugreport) { EXPECT_FALSE(options_.do_screenshot); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); + EXPECT_FALSE(options_.arc_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -249,6 +252,7 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) { EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.do_start_service); + EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { @@ -266,6 +270,7 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { @@ -282,6 +287,7 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializeWearBugReport) { @@ -299,6 +305,7 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { @@ -316,6 +323,7 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializeWifiBugReport) { @@ -333,6 +341,37 @@ TEST_F(DumpOptionsTest, InitializeWifiBugReport) { EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.arc_only); +} + +TEST_F(DumpOptionsTest, InitializeArcOnlyBugreport) { + // clang-format off + char* argv[] = { + const_cast("dumpstatez"), + const_cast("-S"), + const_cast("-d"), + const_cast("-z"), + const_cast("-q"), + const_cast("-A") + }; + // clang-format on + + Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); + + EXPECT_EQ(status, Dumpstate::RunStatus::OK); + EXPECT_TRUE(options_.do_add_date); + EXPECT_TRUE(options_.do_zip_file); + EXPECT_TRUE(options_.use_control_socket); + EXPECT_FALSE(options_.do_vibrate); + EXPECT_TRUE(options_.arc_only); + + // Other options retain default values + EXPECT_FALSE(options_.show_header_only); + EXPECT_FALSE(options_.do_screenshot); + EXPECT_FALSE(options_.do_progress_updates); + EXPECT_FALSE(options_.is_remote_mode); + EXPECT_FALSE(options_.use_socket); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { @@ -361,6 +400,7 @@ TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.wifi_only); + EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializePartial1) { @@ -390,6 +430,7 @@ TEST_F(DumpOptionsTest, InitializePartial1) { EXPECT_FALSE(options_.do_screenshot); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); + EXPECT_FALSE(options_.arc_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -419,6 +460,7 @@ TEST_F(DumpOptionsTest, InitializePartial2) { EXPECT_FALSE(options_.do_zip_file); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.use_control_socket); + EXPECT_FALSE(options_.arc_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } -- cgit v1.2.3-59-g8ed1b From 01dc0c657ad0c7c2b472ab3b6ce0d6544c6de020 Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Wed, 3 Jun 2020 17:13:30 +0800 Subject: Sorts the vector of files by the mtimes GetDumpFds should return a vector sorted with descending order by the mtimes of dumps. Bug: 149242559 Test: atest dumpstate_test Test: adb bugreport && verify last anr dumped Change-Id: I1ed9ceb35ec93b10660f2fb31785899d50bd285d --- cmds/dumpstate/dumpstate.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 9ba4819ee8..5359619d66 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -305,8 +305,9 @@ static const CommandOptions AS_ROOT_20 = CommandOptions::WithTimeout(20).AsRoot( /* * Returns a vector of dump fds under |dir_path| with a given |file_prefix|. - * The returned vector is sorted by the mtimes of the dumps. If |limit_by_mtime| - * is set, the vector only contains files that were written in the last 30 minutes. + * The returned vector is sorted by the mtimes of the dumps with descending + * order. If |limit_by_mtime| is set, the vector only contains files that + * were written in the last 30 minutes. */ static std::vector GetDumpFds(const std::string& dir_path, const std::string& file_prefix, @@ -353,6 +354,10 @@ static std::vector GetDumpFds(const std::string& dir_path, dump_data.emplace_back(DumpData{abs_path, std::move(fd), st.st_mtime}); } + if (!dump_data.empty()) { + std::sort(dump_data.begin(), dump_data.end(), + [](const auto& a, const auto& b) { return a.mtime > b.mtime; }); + } return dump_data; } -- cgit v1.2.3-59-g8ed1b From e0fbd487f944ec82fdb6de74cdf7ce796eeea109 Mon Sep 17 00:00:00 2001 From: mhasank Date: Wed, 10 Jun 2020 14:26:28 -0700 Subject: Revert "arc: Implement smaller dumpstate output" This reverts commit f56bbb2bc0d65190bdb660ab359eb4697e928245. Bug: 142684959 Bug: 136273873 Test: atest dumpstate_test Change-Id: I9c5c007c2f630f06225e5e2b6229c1f3473f181f --- cmds/dumpstate/dumpstate.cpp | 68 ++++----------------------------- cmds/dumpstate/dumpstate.h | 2 - cmds/dumpstate/main.cpp | 2 +- cmds/dumpstate/tests/dumpstate_test.cpp | 42 -------------------- 4 files changed, 8 insertions(+), 106 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index d4c161609c..5359619d66 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1367,53 +1367,6 @@ static void DumpExternalFragmentationInfo() { printf("\n"); } -static void DumpstateArcOnly() { - // Trimmed-down version of dumpstate to only include a whitelisted - // set of logs (system log, event log, and system server / system app - // crashes, and ARC networking logs). See b/136273873 and b/138459828 - // for context. New sections must be first approved by Chrome OS Privacy - // and then added to server side cros monitoring PII scrubber before adding - // them here. See cl/312126645 for an example. - DurationReporter duration_reporter("DUMPSTATE"); - unsigned long timeout_ms; - // calculate timeout - timeout_ms = logcat_timeout({"main", "system", "crash"}); - RunCommand("SYSTEM LOG", - {"logcat", "-v", "threadtime", "-v", "printable", "-v", "uid", "-d", "*:v"}, - CommandOptions::WithTimeoutInMs(timeout_ms).Build()); - timeout_ms = logcat_timeout({"events"}); - RunCommand( - "EVENT LOG", - {"logcat", "-b", "events", "-v", "threadtime", "-v", "printable", "-v", "uid", "-d", "*:v"}, - CommandOptions::WithTimeoutInMs(timeout_ms).Build()); - - printf("========================================================\n"); - printf("== Networking Service\n"); - printf("========================================================\n"); - - // ARC networking service implements dumpsys by reusing the 'wifi' service name. - // The top-level handler is implemented in handleDump() in - // vendor/google_arc/libs/arc-services/src/com/android/server/arc/net/ArcNetworkService.java. - // It outputs a subset of Android system server state relevant for debugging ARC - // connectivity issues, in a PII-free manner. See b/147270970. - RunDumpsys("DUMPSYS NETWORK_SERVICE_LIMITED", {"wifi", "-a"}, - CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); - - printf("========================================================\n"); - printf("== Dropbox crashes\n"); - printf("========================================================\n"); - - RunDumpsys("DROPBOX SYSTEM SERVER CRASHES", {"dropbox", "-p", "system_server_crash"}); - RunDumpsys("DROPBOX SYSTEM APP CRASHES", {"dropbox", "-p", "system_app_crash"}); - - printf("========================================================\n"); - printf("== Final progress (pid %d): %d/%d (estimated %d)\n", ds.pid_, ds.progress_->Get(), - ds.progress_->GetMax(), ds.progress_->GetInitialMax()); - printf("========================================================\n"); - printf("== dumpstate: done (id %d)\n", ds.id_); - printf("========================================================\n"); -} - // Dumps various things. Returns early with status USER_CONSENT_DENIED if user denies consent // via the consent they are shown. Ignores other errors that occur while running various // commands. The consent checking is currently done around long running tasks, which happen to @@ -2100,7 +2053,7 @@ void Dumpstate::DumpstateBoard() { static void ShowUsage() { fprintf(stderr, "usage: dumpstate [-h] [-b soundfile] [-e soundfile] [-d] [-p] " - "[-z] [-s] [-S] [-q] [-P] [-R] [-A] [-V version]\n" + "[-z] [-s] [-S] [-q] [-P] [-R] [-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" @@ -2113,7 +2066,6 @@ static void ShowUsage() { " -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" " -w: start binder service and make it wait for a call to startBugreport\n" - " -A: output limited information that is safe for submission in ARC++ bugreports\n" " -v: prints the dumpstate header and exit\n"); } @@ -2359,12 +2311,13 @@ static void LogDumpOptions(const Dumpstate::DumpOptions& options) { "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 " - "arc_only: %d args: %s\n", + "args: %s\n", options.do_zip_file, options.do_vibrate, options.use_socket, options.use_control_socket, 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.arc_only, options.args.c_str()); + 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()); } void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, @@ -2386,7 +2339,7 @@ void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) { RunStatus status = RunStatus::OK; int c; - while ((c = getopt(argc, argv, "dho:svqzpAPBRSV:w")) != -1) { + while ((c = getopt(argc, argv, "dho:svqzpPBRSV:w")) != -1) { switch (c) { // clang-format off case 'd': do_add_date = true; break; @@ -2398,7 +2351,6 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) case 'p': do_screenshot = true; break; case 'P': do_progress_updates = true; break; case 'R': is_remote_mode = true; break; - case 'A': arc_only = true; break; case 'V': break; // compatibility no-op case 'w': // This was already processed @@ -2683,7 +2635,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, // duration is logged into MYLOG instead. PrintHeader(); - // TODO(nandana) reduce code repetition in if branches if (options_->telephony_only) { MaybeTakeEarlyScreenshot(); onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); @@ -2695,11 +2646,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); MaybeCheckUserConsent(calling_uid, calling_package); DumpstateWifiOnly(); - } else if (options_->arc_only) { - MaybeTakeEarlyScreenshot(); - onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); - MaybeCheckUserConsent(calling_uid, calling_package); - DumpstateArcOnly(); } else { // Invoke critical dumpsys first to preserve system state, before doing anything else. RunDumpsysCritical(); diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index c8c9f452f0..28d8936828 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -377,8 +377,6 @@ class Dumpstate { bool do_start_service = false; bool telephony_only = false; bool wifi_only = false; - // Trimmed-down version of dumpstate to only include whitelisted logs. - bool arc_only = false; // Whether progress updates should be published. bool do_progress_updates = false; // The mode we'll use when calling IDumpstateDevice::dumpstateBoard. diff --git a/cmds/dumpstate/main.cpp b/cmds/dumpstate/main.cpp index f1342a5b53..68d373377c 100644 --- a/cmds/dumpstate/main.cpp +++ b/cmds/dumpstate/main.cpp @@ -30,7 +30,7 @@ bool ShouldStartServiceAndWait(int argc, char* argv[]) { bool do_wait = false; int c; // Keep flags in sync with Dumpstate::DumpOptions::Initialize. - while ((c = getopt(argc, argv, "dho:svqzpAPBRSV:w")) != -1 && !do_wait) { + while ((c = getopt(argc, argv, "wdho:svqzpPBRSV:")) != -1 && !do_wait) { switch (c) { case 'w': do_wait = true; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index 7078521b52..9871a3bd6c 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -179,7 +179,6 @@ TEST_F(DumpOptionsTest, InitializeNone) { EXPECT_FALSE(options_.do_screenshot); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); - EXPECT_FALSE(options_.arc_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -207,7 +206,6 @@ TEST_F(DumpOptionsTest, InitializeAdbBugreport) { EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); - EXPECT_FALSE(options_.arc_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -233,7 +231,6 @@ TEST_F(DumpOptionsTest, InitializeAdbShellBugreport) { EXPECT_FALSE(options_.do_screenshot); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); - EXPECT_FALSE(options_.arc_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -252,7 +249,6 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) { EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.do_start_service); - EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { @@ -270,7 +266,6 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); - EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { @@ -287,7 +282,6 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.use_socket); - EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializeWearBugReport) { @@ -305,7 +299,6 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); - EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { @@ -323,7 +316,6 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); - EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializeWifiBugReport) { @@ -341,37 +333,6 @@ TEST_F(DumpOptionsTest, InitializeWifiBugReport) { EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); - EXPECT_FALSE(options_.arc_only); -} - -TEST_F(DumpOptionsTest, InitializeArcOnlyBugreport) { - // clang-format off - char* argv[] = { - const_cast("dumpstatez"), - const_cast("-S"), - const_cast("-d"), - const_cast("-z"), - const_cast("-q"), - const_cast("-A") - }; - // clang-format on - - Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); - - EXPECT_EQ(status, Dumpstate::RunStatus::OK); - EXPECT_TRUE(options_.do_add_date); - EXPECT_TRUE(options_.do_zip_file); - EXPECT_TRUE(options_.use_control_socket); - EXPECT_FALSE(options_.do_vibrate); - EXPECT_TRUE(options_.arc_only); - - // Other options retain default values - EXPECT_FALSE(options_.show_header_only); - EXPECT_FALSE(options_.do_screenshot); - EXPECT_FALSE(options_.do_progress_updates); - EXPECT_FALSE(options_.is_remote_mode); - EXPECT_FALSE(options_.use_socket); - EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { @@ -400,7 +361,6 @@ TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.wifi_only); - EXPECT_FALSE(options_.arc_only); } TEST_F(DumpOptionsTest, InitializePartial1) { @@ -430,7 +390,6 @@ TEST_F(DumpOptionsTest, InitializePartial1) { EXPECT_FALSE(options_.do_screenshot); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); - EXPECT_FALSE(options_.arc_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -460,7 +419,6 @@ TEST_F(DumpOptionsTest, InitializePartial2) { EXPECT_FALSE(options_.do_zip_file); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.use_control_socket); - EXPECT_FALSE(options_.arc_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } -- cgit v1.2.3-59-g8ed1b From d451a4712405b2ae4a25c293686b110af3412d4e Mon Sep 17 00:00:00 2001 From: mhasank Date: Tue, 26 May 2020 18:02:39 -0700 Subject: arc: Implement smaller dumpstate output Adds a -L commandline option to indicate a much smaller set of dumpstate output safe for feedback reports. Implements output of system logcat, event logs, networking, and dropbox system server crash/system app crashes only. Bug: 142684959 Bug: 136273873 Test: atest dumpstate_test Change-Id: I1d564b949d0a44ff9cc014302d79703294a13d78 --- cmds/dumpstate/dumpstate.cpp | 61 +++++++++++++++++++++++++++++---- cmds/dumpstate/dumpstate.h | 2 ++ cmds/dumpstate/main.cpp | 2 +- cmds/dumpstate/tests/dumpstate_test.cpp | 42 +++++++++++++++++++++++ 4 files changed, 99 insertions(+), 8 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 5359619d66..942c16cd0a 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -1367,6 +1367,46 @@ static void DumpExternalFragmentationInfo() { printf("\n"); } +static void DumpstateLimitedOnly() { + // Trimmed-down version of dumpstate to only include a whitelisted + // set of logs (system log, event log, and system server / system app + // crashes, and networking logs). See b/136273873 and b/138459828 + // for context. + DurationReporter duration_reporter("DUMPSTATE"); + unsigned long timeout_ms; + // calculate timeout + timeout_ms = logcat_timeout({"main", "system", "crash"}); + RunCommand("SYSTEM LOG", + {"logcat", "-v", "threadtime", "-v", "printable", "-v", "uid", "-d", "*:v"}, + CommandOptions::WithTimeoutInMs(timeout_ms).Build()); + timeout_ms = logcat_timeout({"events"}); + RunCommand( + "EVENT LOG", + {"logcat", "-b", "events", "-v", "threadtime", "-v", "printable", "-v", "uid", "-d", "*:v"}, + CommandOptions::WithTimeoutInMs(timeout_ms).Build()); + + printf("========================================================\n"); + printf("== Networking Service\n"); + printf("========================================================\n"); + + RunDumpsys("DUMPSYS NETWORK_SERVICE_LIMITED", {"wifi", "-a"}, + CommandOptions::WithTimeout(90).Build(), SEC_TO_MSEC(10)); + + printf("========================================================\n"); + printf("== Dropbox crashes\n"); + printf("========================================================\n"); + + RunDumpsys("DROPBOX SYSTEM SERVER CRASHES", {"dropbox", "-p", "system_server_crash"}); + RunDumpsys("DROPBOX SYSTEM APP CRASHES", {"dropbox", "-p", "system_app_crash"}); + + printf("========================================================\n"); + printf("== Final progress (pid %d): %d/%d (estimated %d)\n", ds.pid_, ds.progress_->Get(), + ds.progress_->GetMax(), ds.progress_->GetInitialMax()); + printf("========================================================\n"); + printf("== dumpstate: done (id %d)\n", ds.id_); + printf("========================================================\n"); +} + // Dumps various things. Returns early with status USER_CONSENT_DENIED if user denies consent // via the consent they are shown. Ignores other errors that occur while running various // commands. The consent checking is currently done around long running tasks, which happen to @@ -2053,7 +2093,7 @@ void Dumpstate::DumpstateBoard() { static void ShowUsage() { fprintf(stderr, "usage: dumpstate [-h] [-b soundfile] [-e soundfile] [-d] [-p] " - "[-z] [-s] [-S] [-q] [-P] [-R] [-V version]\n" + "[-z] [-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" @@ -2066,6 +2106,7 @@ static void ShowUsage() { " -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" " -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"); } @@ -2311,13 +2352,12 @@ static void LogDumpOptions(const Dumpstate::DumpOptions& options) { "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", + "limited_only: %d args: %s\n", options.do_zip_file, options.do_vibrate, options.use_socket, options.use_control_socket, 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()); + 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.limited_only, options.args.c_str()); } void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, @@ -2339,7 +2379,7 @@ void Dumpstate::DumpOptions::Initialize(BugreportMode bugreport_mode, Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) { RunStatus status = RunStatus::OK; int c; - while ((c = getopt(argc, argv, "dho:svqzpPBRSV:w")) != -1) { + while ((c = getopt(argc, argv, "dho:svqzpLPBRSV:w")) != -1) { switch (c) { // clang-format off case 'd': do_add_date = true; break; @@ -2351,6 +2391,7 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) 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 'w': // This was already processed @@ -2635,6 +2676,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, // duration is logged into MYLOG instead. PrintHeader(); + // TODO(b/158737089) reduce code repetition in if branches if (options_->telephony_only) { MaybeTakeEarlyScreenshot(); onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); @@ -2646,6 +2688,11 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); MaybeCheckUserConsent(calling_uid, calling_package); DumpstateWifiOnly(); + } else if (options_->limited_only) { + MaybeTakeEarlyScreenshot(); + onUiIntensiveBugreportDumpsFinished(calling_uid, calling_package); + MaybeCheckUserConsent(calling_uid, calling_package); + DumpstateLimitedOnly(); } else { // Invoke critical dumpsys first to preserve system state, before doing anything else. RunDumpsysCritical(); diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 28d8936828..dc0848ac36 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -377,6 +377,8 @@ class Dumpstate { bool do_start_service = false; bool telephony_only = false; bool wifi_only = false; + // Trimmed-down version of dumpstate to only include whitelisted logs. + bool limited_only = false; // Whether progress updates should be published. bool do_progress_updates = false; // The mode we'll use when calling IDumpstateDevice::dumpstateBoard. diff --git a/cmds/dumpstate/main.cpp b/cmds/dumpstate/main.cpp index 68d373377c..ec89c0dd6e 100644 --- a/cmds/dumpstate/main.cpp +++ b/cmds/dumpstate/main.cpp @@ -30,7 +30,7 @@ bool ShouldStartServiceAndWait(int argc, char* argv[]) { bool do_wait = false; int c; // Keep flags in sync with Dumpstate::DumpOptions::Initialize. - while ((c = getopt(argc, argv, "wdho:svqzpPBRSV:")) != -1 && !do_wait) { + while ((c = getopt(argc, argv, "dho:svqzpLPBRSV:w")) != -1 && !do_wait) { switch (c) { case 'w': do_wait = true; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index 9871a3bd6c..e94e51c7b1 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -179,6 +179,7 @@ TEST_F(DumpOptionsTest, InitializeNone) { EXPECT_FALSE(options_.do_screenshot); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); + EXPECT_FALSE(options_.limited_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -206,6 +207,7 @@ TEST_F(DumpOptionsTest, InitializeAdbBugreport) { EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.limited_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -231,6 +233,7 @@ TEST_F(DumpOptionsTest, InitializeAdbShellBugreport) { EXPECT_FALSE(options_.do_screenshot); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); + EXPECT_FALSE(options_.limited_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -249,6 +252,7 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) { EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.do_start_service); + EXPECT_FALSE(options_.limited_only); } TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { @@ -266,6 +270,7 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.limited_only); } TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { @@ -282,6 +287,7 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.limited_only); } TEST_F(DumpOptionsTest, InitializeWearBugReport) { @@ -299,6 +305,7 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.limited_only); } TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { @@ -316,6 +323,7 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) { EXPECT_FALSE(options_.show_header_only); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.limited_only); } TEST_F(DumpOptionsTest, InitializeWifiBugReport) { @@ -333,6 +341,37 @@ TEST_F(DumpOptionsTest, InitializeWifiBugReport) { EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.limited_only); +} + +TEST_F(DumpOptionsTest, InitializeLimitedOnlyBugreport) { + // clang-format off + char* argv[] = { + const_cast("dumpstatez"), + const_cast("-S"), + const_cast("-d"), + const_cast("-z"), + const_cast("-q"), + const_cast("-L") + }; + // clang-format on + + Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); + + EXPECT_EQ(status, Dumpstate::RunStatus::OK); + EXPECT_TRUE(options_.do_add_date); + EXPECT_TRUE(options_.do_zip_file); + EXPECT_TRUE(options_.use_control_socket); + EXPECT_FALSE(options_.do_vibrate); + EXPECT_TRUE(options_.limited_only); + + // Other options retain default values + EXPECT_FALSE(options_.show_header_only); + EXPECT_FALSE(options_.do_screenshot); + EXPECT_FALSE(options_.do_progress_updates); + EXPECT_FALSE(options_.is_remote_mode); + EXPECT_FALSE(options_.use_socket); + EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { @@ -361,6 +400,7 @@ TEST_F(DumpOptionsTest, InitializeDefaultBugReport) { EXPECT_FALSE(options_.is_remote_mode); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.wifi_only); + EXPECT_FALSE(options_.limited_only); } TEST_F(DumpOptionsTest, InitializePartial1) { @@ -390,6 +430,7 @@ TEST_F(DumpOptionsTest, InitializePartial1) { EXPECT_FALSE(options_.do_screenshot); EXPECT_FALSE(options_.do_progress_updates); EXPECT_FALSE(options_.is_remote_mode); + EXPECT_FALSE(options_.limited_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } @@ -419,6 +460,7 @@ TEST_F(DumpOptionsTest, InitializePartial2) { EXPECT_FALSE(options_.do_zip_file); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.use_control_socket); + EXPECT_FALSE(options_.limited_only); EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT); } -- cgit v1.2.3-59-g8ed1b From 2d75c44e6129077443d7fde3ea22225284bb0982 Mon Sep 17 00:00:00 2001 From: mhasank Date: Thu, 11 Jun 2020 15:05:25 -0700 Subject: arc: Reintroduce the output file option -o argument was previously removed in favor of new api and consent dialog but now it is being reintroduced to go along with limited mode dumpstate logs that do not require user consent. This argument is effective only during limited mode but we do not throw an error if supplied with other arguemnts for backwards compatibility reasons. Bug: 142684959 Bug: 136273873 Bug: 139379357 Bug: 138805202 Bug: 142685922 Test: adb bugreport Test: adb bugreport xyz.zip Test: adb shell bugreport Test: atest dumpstate_test Test: flash android to dut, android-sh, setenforce 1, arc-bugreport Change-Id: Ib2cadf13c101dd2e957033a7657a647d043f2b72 --- cmds/dumpstate/dumpstate.cpp | 43 ++++++++++++++++++++++++++++++--- cmds/dumpstate/dumpstate.h | 10 +++++++- cmds/dumpstate/tests/dumpstate_test.cpp | 5 +++- 3 files changed, 52 insertions(+), 6 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 942c16cd0a..e6bfa148b9 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -209,6 +209,10 @@ static int Open(std::string path, int flags, mode_t mode = 0) { return fd; } +static int OpenForWrite(std::string path) { + return Open(path, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW, + S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); +} static int OpenForRead(std::string path) { return Open(path, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); @@ -274,6 +278,27 @@ int64_t GetModuleMetadataVersion() { return version_code; } +static bool PathExists(const std::string& path) { + struct stat sb; + return stat(path.c_str(), &sb) == 0; +} + +static bool CopyFileToFile(const std::string& input_file, const std::string& output_file) { + if (input_file == output_file) { + MYLOGD("Skipping copying bugreport file since the destination is the same (%s)\n", + output_file.c_str()); + return false; + } + else if (PathExists(output_file)) { + MYLOGD("Cannot overwrite an existing file (%s)\n", output_file.c_str()); + return false; + } + + MYLOGD("Going to copy bugreport file (%s) to %s\n", input_file.c_str(), output_file.c_str()); + android::base::unique_fd out_fd(OpenForWrite(output_file)); + return CopyFileToFd(input_file, out_fd.get()); +} + } // namespace } // namespace os } // namespace android @@ -2092,11 +2117,12 @@ void Dumpstate::DumpstateBoard() { static void ShowUsage() { fprintf(stderr, - "usage: dumpstate [-h] [-b soundfile] [-e soundfile] [-d] [-p] " + "usage: dumpstate [-h] [-b soundfile] [-e soundfile] [-o directory] [-d] [-p] " "[-z] [-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" @@ -2267,6 +2293,14 @@ static void FinalizeFile() { do_text_file = false; } } + + std::string final_path = ds.path_; + if (ds.options_->OutputToCustomFile()) { + std::string bugreport_dir = dirname(ds.options_->use_outfile.c_str()); + final_path = ds.GetPath(bugreport_dir, ".zip"); + android::os::CopyFileToFile(ds.path_, final_path); + } + if (ds.options_->use_control_socket) { if (do_text_file) { dprintf(ds.control_socket_fd_, @@ -2274,7 +2308,7 @@ static void FinalizeFile() { "for more details\n", ds.log_path_.c_str()); } else { - dprintf(ds.control_socket_fd_, "OK:%s\n", ds.path_.c_str()); + dprintf(ds.control_socket_fd_, "OK:%s\n", final_path.c_str()); } } } @@ -2384,6 +2418,7 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) // clang-format off case 'd': do_add_date = true; break; case 'z': do_zip_file = true; break; + case 'o': use_outfile = optarg; break; case 's': use_socket = true; break; case 'S': use_control_socket = true; break; case 'v': show_header_only = true; break; @@ -2505,8 +2540,8 @@ void Dumpstate::Cancel() { * 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. * - * Bugreports are first generated in a local directory and later copied to the caller's fd if - * supplied. + * Bugreports are first generated in a local directory and later copied to the caller's fd + * or directory if supplied. */ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, const std::string& calling_package) { diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index dc0848ac36..a133f7ed77 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -386,10 +386,12 @@ class Dumpstate { // The HAL is actually an API surface that can be validated, while the AIDL is not (@hide). ::android::hardware::dumpstate::V1_1::DumpstateMode dumpstate_hal_mode = ::android::hardware::dumpstate::V1_1::DumpstateMode::DEFAULT; - // File descriptor to output zip file. + // File descriptor to output zip file. Takes precedence over use_outfile.. android::base::unique_fd bugreport_fd; // File descriptor to screenshot file. android::base::unique_fd screenshot_fd; + // Partial path to output file. + std::string use_outfile; // Bugreport mode of the bugreport. std::string bugreport_mode; // Command-line arguments as string @@ -415,6 +417,12 @@ class Dumpstate { // specified, it is preferred. If not bugreport is written to /bugreports. return !use_socket; } + + /* Returns if options specified require writing to custom file location */ + bool OutputToCustomFile() { + // Custom location is only honored in limited mode. + return limited_only && !use_outfile.empty() && bugreport_fd.get() == -1; + } }; // TODO: initialize fields on constructor diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index e94e51c7b1..e1671457d8 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -172,6 +172,7 @@ TEST_F(DumpOptionsTest, InitializeNone) { EXPECT_FALSE(options_.do_add_date); EXPECT_FALSE(options_.do_zip_file); + EXPECT_EQ("", options_.use_outfile); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.use_control_socket); EXPECT_FALSE(options_.show_header_only); @@ -352,7 +353,8 @@ TEST_F(DumpOptionsTest, InitializeLimitedOnlyBugreport) { const_cast("-d"), const_cast("-z"), const_cast("-q"), - const_cast("-L") + const_cast("-L"), + const_cast("-o abc") }; // clang-format on @@ -364,6 +366,7 @@ TEST_F(DumpOptionsTest, InitializeLimitedOnlyBugreport) { EXPECT_TRUE(options_.use_control_socket); EXPECT_FALSE(options_.do_vibrate); EXPECT_TRUE(options_.limited_only); + EXPECT_EQ(" abc", std::string(options_.use_outfile)); // Other options retain default values EXPECT_FALSE(options_.show_header_only); -- cgit v1.2.3-59-g8ed1b From 3a4cfb4f6f89186c4a6d9e8b8d2e56d96d5455e1 Mon Sep 17 00:00:00 2001 From: mhasank Date: Mon, 15 Jun 2020 18:06:43 -0700 Subject: arc: Rename use_outfile to out_dir + use as is Bug: 142684959 Bug: 136273873 Bug: 139379357 Bug: 138805202 Bug: 142685922 Test: adb shell bugreport Test: atest dumpstate_test Test: flash android to dut, android-sh, setenforce 1, arc-bugreport Change-Id: Ia2bb2d4609467d902ce7424d882ed88422867d0b --- cmds/dumpstate/dumpstate.cpp | 5 ++--- cmds/dumpstate/dumpstate.h | 8 ++++---- cmds/dumpstate/tests/dumpstate_test.cpp | 4 ++-- 3 files changed, 8 insertions(+), 9 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index e6bfa148b9..581d3ded89 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2296,8 +2296,7 @@ static void FinalizeFile() { std::string final_path = ds.path_; if (ds.options_->OutputToCustomFile()) { - std::string bugreport_dir = dirname(ds.options_->use_outfile.c_str()); - final_path = ds.GetPath(bugreport_dir, ".zip"); + final_path = ds.GetPath(ds.options_->out_dir, ".zip"); android::os::CopyFileToFile(ds.path_, final_path); } @@ -2418,7 +2417,7 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) // clang-format off case 'd': do_add_date = true; break; case 'z': do_zip_file = true; break; - case 'o': use_outfile = optarg; break; + case 'o': out_dir = optarg; break; case 's': use_socket = true; break; case 'S': use_control_socket = true; break; case 'v': show_header_only = true; break; diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index a133f7ed77..0d25d307a6 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -386,12 +386,12 @@ class Dumpstate { // The HAL is actually an API surface that can be validated, while the AIDL is not (@hide). ::android::hardware::dumpstate::V1_1::DumpstateMode dumpstate_hal_mode = ::android::hardware::dumpstate::V1_1::DumpstateMode::DEFAULT; - // File descriptor to output zip file. Takes precedence over use_outfile.. + // File descriptor to output zip file. Takes precedence over out_dir. android::base::unique_fd bugreport_fd; // File descriptor to screenshot file. android::base::unique_fd screenshot_fd; - // Partial path to output file. - std::string use_outfile; + // Custom output directory. + std::string out_dir; // Bugreport mode of the bugreport. std::string bugreport_mode; // Command-line arguments as string @@ -421,7 +421,7 @@ class Dumpstate { /* Returns if options specified require writing to custom file location */ bool OutputToCustomFile() { // Custom location is only honored in limited mode. - return limited_only && !use_outfile.empty() && bugreport_fd.get() == -1; + return limited_only && !out_dir.empty() && bugreport_fd.get() == -1; } }; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index e1671457d8..c7df1bb6a3 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -172,7 +172,7 @@ TEST_F(DumpOptionsTest, InitializeNone) { EXPECT_FALSE(options_.do_add_date); EXPECT_FALSE(options_.do_zip_file); - EXPECT_EQ("", options_.use_outfile); + EXPECT_EQ("", options_.out_dir); EXPECT_FALSE(options_.use_socket); EXPECT_FALSE(options_.use_control_socket); EXPECT_FALSE(options_.show_header_only); @@ -366,7 +366,7 @@ TEST_F(DumpOptionsTest, InitializeLimitedOnlyBugreport) { EXPECT_TRUE(options_.use_control_socket); EXPECT_FALSE(options_.do_vibrate); EXPECT_TRUE(options_.limited_only); - EXPECT_EQ(" abc", std::string(options_.use_outfile)); + EXPECT_EQ(" abc", std::string(options_.out_dir)); // Other options retain default values EXPECT_FALSE(options_.show_header_only); -- cgit v1.2.3-59-g8ed1b