From 402a839ef5e2fdecfd9a0fc75e6250d374bfc83e Mon Sep 17 00:00:00 2001
From: Nandana Dutt <nandana@google.com>
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/DumpstateService.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

(limited to 'cmds/dumpstate/DumpstateService.cpp')

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<String16>&) {
     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());
-- 
cgit v1.2.3-59-g8ed1b


From e370d68932f5c81c4cfdaa4412e65d1fe055ca29 Mon Sep 17 00:00:00 2001
From: Abhijeet Kaur <abkaur@google.com>
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/DumpstateService.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<String16>&) {
     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<uint8_t> 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<std::string>& args) {
     // clang-format off
     std::vector<std::string> 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<std::string> 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<std::string> 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<std::string> am_args = {
+                 "--receiver-permission", "android.permission.DUMP",
+            };
+            // clang-format on
+            // Send STARTED broadcast for apps that listen to bugreport generation events
+            SendBroadcast("com.android.internal.intent.action.BUGREPORT_STARTED", am_args);
             if (options_->use_control_socket) {
                 dprintf(control_socket_fd_, "BEGIN:%s\n", path_.c_str());
             }
@@ -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 <android-base/stringprintf.h>
 #include <android-base/strings.h>
 #include <cutils/properties.h>
+#include <android-base/unique_fd.h>
 
 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<char*>("bugreport"),
-        const_cast<char*>("-d"),
-        const_cast<char*>("-p"),
-        const_cast<char*>("-B"),
-        const_cast<char*>("-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<char*>("bugreport"),
-        const_cast<char*>("-d"),
-        const_cast<char*>("-p"),
-        const_cast<char*>("-B"),
-        const_cast<char*>("-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<char*>("bugreport"),
-        const_cast<char*>("-d"),
-        const_cast<char*>("-p"),
-        const_cast<char*>("-B"),
-        const_cast<char*>("-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<char*>("bugreport"),
-        const_cast<char*>("-d"),
-        const_cast<char*>("-p"),
-        const_cast<char*>("-B"),
-        const_cast<char*>("-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<char*>("bugreport"),
-        const_cast<char*>("-d"),
-        const_cast<char*>("-p"),
-        const_cast<char*>("-B"),
-        const_cast<char*>("-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<char*>("bugreport"),
-        const_cast<char*>("-d"),
-        const_cast<char*>("-p"),
-        const_cast<char*>("-B"),
-        const_cast<char*>("-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<char*>("bugreport"),
         const_cast<char*>("-d"),
         const_cast<char*>("-p"),
-        const_cast<char*>("-B"),
         const_cast<char*>("-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<char*>("-p"),
         const_cast<char*>("-P"),
         const_cast<char*>("-R"),
-        const_cast<char*>("-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 <abkaur@google.com>
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/DumpstateService.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<IDumpstateListener>& listener,
-                                             bool getSectionDetails,
-                                             sp<IDumpstateToken>* 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<std::mutex> 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<IDumpstateListener>& 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<std::mutex> 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<String16>&) {
     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 <binder/BinderService.h>
 
 #include "android/os/BnDumpstate.h"
-#include "android/os/BnDumpstateToken.h"
 #include "dumpstate.h"
 
 namespace android {
@@ -38,9 +37,6 @@ class DumpstateService : public BinderService<DumpstateService>, public BnDumpst
     static char const* getServiceName();
 
     status_t dump(int fd, const Vector<String16>& args) override;
-    binder::Status setListener(const std::string& name, const sp<IDumpstateListener>& listener,
-                               bool getSectionDetails,
-                               sp<IDumpstateToken>* 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<android::os::IDumpstateListener> listener_;
-    std::string listener_name_;
-    bool report_section_;
 
     // List of open tombstone dump files.
     std::vector<DumpData> 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<DumpstateListener> 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<DumpstateListenerMock> 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<DumpstateListenerMock> 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<DumpstateListenerMock> listener(new DumpstateListenerMock());
-    sp<IDumpstateToken> token;
-    EXPECT_TRUE(dss.setListener("", listener, /* getSectionDetails = */ false, &token).isOk());
-    ASSERT_THAT(token, IsNull());
-}
-
-TEST_F(DumpstateServiceTest, SetListenerNoPointer) {
-    sp<IDumpstateToken> token;
-    EXPECT_TRUE(
-        dss.setListener("whatever", nullptr, /* getSectionDetails = */ false, &token).isOk());
-    ASSERT_THAT(token, IsNull());
-}
-
-TEST_F(DumpstateServiceTest, SetListenerTwice) {
-    sp<DumpstateListenerMock> listener(new DumpstateListenerMock());
-    sp<IDumpstateToken> 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<DumpstateListenerMock> listener(new DumpstateListenerMock());
-    sp<IDumpstateToken> 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 3b25ae153fb5aaa1d2f70600b45604e3665f0da2 Mon Sep 17 00:00:00 2001
From: Jiyong Park <jiyong@google.com>
Date: Mon, 25 Nov 2019 11:06:16 +0900
Subject: unique_fd is passed by value in AIDL interfaces

FileDescriptor type in AIDL was translated into const unique_fd& in C++.
Now, it is unique_fd, i.e. passed by value, to make it easier to keep it
beyond the scope of the call.

Bug: 144943748
Test: m
Change-Id: Id0fead2717b1f6a3b9a1867efa8b588b59d3fbd2
---
 cmds/dumpstate/DumpstateService.cpp           |  4 ++--
 cmds/dumpstate/DumpstateService.h             |  4 ++--
 cmds/dumpstate/tests/dumpstate_smoke_test.cpp | 12 ++++++++----
 cmds/installd/InstalldNativeService.cpp       |  2 +-
 cmds/installd/InstalldNativeService.h         |  2 +-
 5 files changed, 14 insertions(+), 10 deletions(-)

(limited to 'cmds/dumpstate/DumpstateService.cpp')

diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp
index ccd74dbc74..87ea520f84 100644
--- a/cmds/dumpstate/DumpstateService.cpp
+++ b/cmds/dumpstate/DumpstateService.cpp
@@ -81,8 +81,8 @@ status_t DumpstateService::Start() {
 
 binder::Status DumpstateService::startBugreport(int32_t calling_uid,
                                                 const std::string& calling_package,
-                                                const android::base::unique_fd& bugreport_fd,
-                                                const android::base::unique_fd& screenshot_fd,
+                                                android::base::unique_fd bugreport_fd,
+                                                android::base::unique_fd screenshot_fd,
                                                 int bugreport_mode,
                                                 const sp<IDumpstateListener>& listener) {
     MYLOGI("startBugreport() with mode: %d\n", bugreport_mode);
diff --git a/cmds/dumpstate/DumpstateService.h b/cmds/dumpstate/DumpstateService.h
index 27954adbae..6dc0225f02 100644
--- a/cmds/dumpstate/DumpstateService.h
+++ b/cmds/dumpstate/DumpstateService.h
@@ -39,8 +39,8 @@ class DumpstateService : public BinderService<DumpstateService>, public BnDumpst
     status_t dump(int fd, const Vector<String16>& args) override;
 
     binder::Status startBugreport(int32_t calling_uid, const std::string& calling_package,
-                                  const android::base::unique_fd& bugreport_fd,
-                                  const android::base::unique_fd& screenshot_fd, int bugreport_mode,
+                                  android::base::unique_fd bugreport_fd,
+                                  android::base::unique_fd screenshot_fd, int bugreport_mode,
                                   const sp<IDumpstateListener>& listener) override;
 
     // No-op
diff --git a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
index f1884f8f5c..dac90d91b0 100644
--- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
@@ -444,7 +444,7 @@ TEST_F(DumpstateBinderTest, Baseline) {
 
     sp<DumpstateListener> listener(new DumpstateListener(dup(fileno(stdout))));
     android::binder::Status status =
-        ds_binder->startBugreport(123, "com.dummy.package", bugreport_fd, screenshot_fd,
+        ds_binder->startBugreport(123, "com.dummy.package", std::move(bugreport_fd), std::move(screenshot_fd),
                                   Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener);
     // startBugreport is an async call. Verify binder call succeeded first, then wait till listener
     // gets expected callbacks.
@@ -480,7 +480,7 @@ TEST_F(DumpstateBinderTest, ServiceDies_OnInvalidInput) {
     // Call startBugreport with bad arguments.
     sp<DumpstateListener> listener(new DumpstateListener(dup(fileno(stdout))));
     android::binder::Status status =
-        ds_binder->startBugreport(123, "com.dummy.package", bugreport_fd, screenshot_fd,
+        ds_binder->startBugreport(123, "com.dummy.package", std::move(bugreport_fd), std::move(screenshot_fd),
                                   2000,  // invalid bugreport mode
                                   listener);
     EXPECT_EQ(listener->getErrorCode(), IDumpstateListener::BUGREPORT_ERROR_INVALID_INPUT);
@@ -501,20 +501,24 @@ TEST_F(DumpstateBinderTest, SimultaneousBugreportsNotAllowed) {
 
     // Prepare arguments
     unique_fd bugreport_fd(OpenForWrite("/data/local/tmp/tmp.zip"));
+    unique_fd bugreport_fd2(dup(bugreport_fd.get()));
     unique_fd screenshot_fd(OpenForWrite("/data/local/tmp/tmp.png"));
+    unique_fd screenshot_fd2(dup(screenshot_fd.get()));
 
     EXPECT_NE(bugreport_fd.get(), -1);
+    EXPECT_NE(bugreport_fd2.get(), -1);
     EXPECT_NE(screenshot_fd.get(), -1);
+    EXPECT_NE(screenshot_fd2.get(), -1);
 
     sp<DumpstateListener> listener1(new DumpstateListener(dup(fileno(stdout))));
     android::binder::Status status =
-        ds_binder->startBugreport(123, "com.dummy.package", bugreport_fd, screenshot_fd,
+        ds_binder->startBugreport(123, "com.dummy.package", std::move(bugreport_fd), std::move(screenshot_fd),
                                   Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener1);
     EXPECT_TRUE(status.isOk());
 
     // try to make another call to startBugreport. This should fail.
     sp<DumpstateListener> listener2(new DumpstateListener(dup(fileno(stdout))));
-    status = ds_binder->startBugreport(123, "com.dummy.package", bugreport_fd, screenshot_fd,
+    status = ds_binder->startBugreport(123, "com.dummy.package", std::move(bugreport_fd2), std::move(screenshot_fd2),
                                        Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE, listener2);
     EXPECT_FALSE(status.isOk());
     WaitTillExecutionComplete(listener2.get());
diff --git a/cmds/installd/InstalldNativeService.cpp b/cmds/installd/InstalldNativeService.cpp
index e6e232ce2f..e3b3424c62 100644
--- a/cmds/installd/InstalldNativeService.cpp
+++ b/cmds/installd/InstalldNativeService.cpp
@@ -2401,7 +2401,7 @@ struct fsverity_measurement {
 #endif
 
 binder::Status InstalldNativeService::installApkVerity(const std::string& filePath,
-        const ::android::base::unique_fd& verityInputAshmem, int32_t contentSize) {
+        android::base::unique_fd verityInputAshmem, int32_t contentSize) {
     ENFORCE_UID(AID_SYSTEM);
     CHECK_ARGUMENT_PATH(filePath);
     std::lock_guard<std::recursive_mutex> lock(mLock);
diff --git a/cmds/installd/InstalldNativeService.h b/cmds/installd/InstalldNativeService.h
index ef91bf84be..fb0273098a 100644
--- a/cmds/installd/InstalldNativeService.h
+++ b/cmds/installd/InstalldNativeService.h
@@ -133,7 +133,7 @@ public:
     binder::Status deleteOdex(const std::string& apkPath, const std::string& instructionSet,
             const std::unique_ptr<std::string>& outputPath);
     binder::Status installApkVerity(const std::string& filePath,
-            const ::android::base::unique_fd& verityInput, int32_t contentSize);
+            android::base::unique_fd verityInput, int32_t contentSize);
     binder::Status assertFsverityRootHashMatches(const std::string& filePath,
             const std::vector<uint8_t>& expectedHash);
     binder::Status reconcileSecondaryDexFile(const std::string& dexPath,
-- 
cgit v1.2.3-59-g8ed1b


From 0d2aad706c7977f7dc8f70e6a2fd0426e5c698e1 Mon Sep 17 00:00:00 2001
From: Paul Chang <changpa@google.com>
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/DumpstateService.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<Dumpstate::BugreportMode>(bugreport_mode), bugreport_fd,
                         screenshot_fd);
 
-    if (bugreport_fd.get() == -1 || (options->do_fb && screenshot_fd.get() == -1)) {
+    if (bugreport_fd.get() == -1 || (options->do_screenshot && screenshot_fd.get() == -1)) {
         MYLOGE("Invalid filedescriptor");
         signalErrorAndExit(listener, IDumpstateListener::BUGREPORT_ERROR_INVALID_INPUT);
     }
diff --git a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl
index 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<std::mutex> lock(lock_);
     result_ = APPROVED;
     MYLOGD("User approved consent to share bugreport\n");
+
+    // Maybe copy screenshot so calling app can display the screenshot to the user as soon as
+    // consent is granted.
+    if (ds.options_->is_screenshot_copied) {
+        return android::binder::Status::ok();
+    }
+
+    if (!ds.options_->do_screenshot || ds.options_->screenshot_fd.get() == -1 ||
+        !ds.do_early_screenshot_) {
+        return android::binder::Status::ok();
+    }
+
+    bool copy_succeeded = android::os::CopyFileToFd(ds.screenshot_path_,
+                                                    ds.options_->screenshot_fd.get());
+    ds.options_->is_screenshot_copied = copy_succeeded;
+    if (copy_succeeded) {
+        android::os::UnlinkAndLogOnError(ds.screenshot_path_);
+    }
     return android::binder::Status::ok();
 }
 
@@ -1407,7 +1425,7 @@ 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<std::mutex> lock(lock_);
+        dprintf(out_fd_, "\rResult of taking screenshot: %s", success ? "success" : "failure");
+        return binder::Status::ok();
+    }
+
     bool getIsFinished() {
         std::lock_guard<std::mutex> lock(lock_);
         return is_finished_;
diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp
index 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 f59c2b7ad16b19ba00765d75d38fec74fe8f9435 Mon Sep 17 00:00:00 2001
From: Paul Chang <changpa@google.com>
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/DumpstateService.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<IDumpstateListener>& listener) {
+                                                const sp<IDumpstateListener>& 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<Dumpstate::DumpOptions> options = std::make_unique<Dumpstate::DumpOptions>();
     options->Initialize(static_cast<Dumpstate::BugreportMode>(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<DumpstateService>, 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<IDumpstateListener>& listener) override;
+                                  const sp<IDumpstateListener>& 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<DumpstateListener> 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<DumpstateListener> 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<DumpstateListener> 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 8ae16e67ee8d12ec14d08c0445533ff6da2988b4 Mon Sep 17 00:00:00 2001
From: Nandana Dutt <nandana@google.com>
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/DumpstateService.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<std::mutex> lock(lock_);
+    ds_->Cancel();
     return binder::Status::ok();
 }
 
 status_t DumpstateService::dump(int fd, const Vector<String16>&) {
+    std::lock_guard<std::mutex> 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<DumpOptions> 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 <abkaur@google.com>
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/DumpstateService.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<DumpOptions> 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