diff options
33 files changed, 662 insertions, 360 deletions
diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index 260ea4b057..71658d819b 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -87,27 +87,27 @@ binder::Status DumpstateService::startBugreport(int, const sp<IDumpstateListener 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_.update_progress_ ? "true" : "false"); + 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, "progress:\n"); ds_.progress_->Dump(fd, " "); - dprintf(fd, "args: %s\n", ds_.args_.c_str()); - dprintf(fd, "extra_options: %s\n", ds_.extra_options_.c_str()); + dprintf(fd, "args: %s\n", ds_.options_->args.c_str()); + dprintf(fd, "extra_options: %s\n", ds_.options_->extra_options.c_str()); dprintf(fd, "version: %s\n", ds_.version_.c_str()); dprintf(fd, "bugreport_dir: %s\n", ds_.bugreport_dir_.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_.extra_options_.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_); 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_.notification_title.c_str()); - dprintf(fd, "notification description: %s\n", ds_.notification_description.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()); return NO_ERROR; } diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 17bb7c3ed6..0b9bca013c 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -682,7 +682,7 @@ void Dumpstate::PrintHeader() const { 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(), args_.c_str(), extra_options_.c_str()); + PropertiesHelper::IsDryRun(), options_->args.c_str(), options_->extra_options.c_str()); printf("\n"); } @@ -1623,7 +1623,7 @@ void Dumpstate::DumpstateBoard() { printf("*** See dumpstate-board.txt entry ***\n"); } -static void ShowUsageAndExit(int exitCode = 1) { +static void ShowUsageAndExit(int exit_code = 1) { fprintf(stderr, "usage: dumpstate [-h] [-b soundfile] [-e soundfile] [-o file] [-d] [-p] " "[-z]] [-s] [-S] [-q] [-B] [-P] [-R] [-V version]\n" @@ -1643,7 +1643,7 @@ static void ShowUsageAndExit(int exitCode = 1) { " -R: take bugreport in remote mode (requires -o, -z, -d and -B, " "shouldn't be used with -P)\n" " -v: prints the dumpstate header and exit\n"); - exit(exitCode); + exit(exit_code); } static void ExitOnInvalidArgs() { @@ -1769,13 +1769,13 @@ static void Vibrate(int duration_ms) { * if we are writing zip files and adds the version file. */ static void PrepareToWriteToFile() { - const Dumpstate::DumpOptions& options = ds.options_; - ds.bugreport_dir_ = dirname(options.use_outfile.c_str()); + ds.bugreport_dir_ = dirname(ds.options_->use_outfile.c_str()); std::string build_id = android::base::GetProperty("ro.build.id", "UNKNOWN_BUILD"); std::string device_name = android::base::GetProperty("ro.product.name", "UNKNOWN_DEVICE"); - ds.base_name_ = android::base::StringPrintf("%s-%s-%s", basename(options.use_outfile.c_str()), - device_name.c_str(), build_id.c_str()); - if (options.do_add_date) { + ds.base_name_ = + android::base::StringPrintf("%s-%s-%s", basename(ds.options_->use_outfile.c_str()), + device_name.c_str(), build_id.c_str()); + if (ds.options_->do_add_date) { char date[80]; strftime(date, sizeof(date), "%Y-%m-%d-%H-%M-%S", localtime(&ds.now_)); ds.name_ = date; @@ -1783,13 +1783,13 @@ static void PrepareToWriteToFile() { ds.name_ = "undated"; } - if (options.telephony_only) { + if (ds.options_->telephony_only) { ds.base_name_ += "-telephony"; - } else if (options.wifi_only) { + } else if (ds.options_->wifi_only) { ds.base_name_ += "-wifi"; } - if (options.do_fb) { + if (ds.options_->do_fb) { ds.screenshot_path_ = ds.GetPath(".png"); } ds.tmp_path_ = ds.GetPath(".tmp"); @@ -1805,7 +1805,7 @@ static void PrepareToWriteToFile() { ds.bugreport_dir_.c_str(), ds.base_name_.c_str(), ds.name_.c_str(), ds.log_path_.c_str(), ds.tmp_path_.c_str(), ds.screenshot_path_.c_str()); - if (options.do_zip_file) { + if (ds.options_->do_zip_file) { ds.path_ = ds.GetPath(".zip"); MYLOGD("Creating initial .zip file (%s)\n", ds.path_.c_str()); create_parent_dirs(ds.path_.c_str()); @@ -1824,7 +1824,6 @@ static void PrepareToWriteToFile() { * printing zipped file status, etc. */ static void FinalizeFile() { - const Dumpstate::DumpOptions& options = ds.options_; /* check if user changed the suffix using system properties */ std::string name = android::base::GetProperty(android::base::StringPrintf("dumpstate.%d.name", ds.pid_), ""); @@ -1853,7 +1852,7 @@ static void FinalizeFile() { } bool do_text_file = true; - if (options.do_zip_file) { + if (ds.options_->do_zip_file) { if (!ds.FinishZipFile()) { MYLOGE("Failed to finish zip file; sending text bugreport instead\n"); do_text_file = true; @@ -1880,7 +1879,7 @@ static void FinalizeFile() { ds.path_.clear(); } } - if (options.use_control_socket) { + if (ds.options_->use_control_socket) { if (do_text_file) { dprintf(ds.control_socket_fd_, "FAIL:could not create zip file, check %s " @@ -1894,7 +1893,6 @@ static void FinalizeFile() { /* Broadcasts that we are done with the bugreport */ static void SendBugreportFinishedBroadcast() { - const Dumpstate::DumpOptions& options = ds.options_; if (!ds.path_.empty()) { MYLOGI("Final bugreport path: %s\n", ds.path_.c_str()); // clang-format off @@ -1908,22 +1906,22 @@ static void SendBugreportFinishedBroadcast() { "--es", "android.intent.extra.DUMPSTATE_LOG", ds.log_path_ }; // clang-format on - if (options.do_fb) { + if (ds.options_->do_fb) { am_args.push_back("--es"); am_args.push_back("android.intent.extra.SCREENSHOT"); am_args.push_back(ds.screenshot_path_); } - if (!ds.notification_title.empty()) { + if (ds.options_->notification_title.empty()) { am_args.push_back("--es"); am_args.push_back("android.intent.extra.TITLE"); - am_args.push_back(ds.notification_title); - if (!ds.notification_description.empty()) { + 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.notification_description); + am_args.push_back(ds.options_->notification_description); } } - if (options.is_remote_mode) { + 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_)); @@ -1936,30 +1934,77 @@ static void SendBugreportFinishedBroadcast() { } } -int Dumpstate::ParseCommandlineOptions(int argc, char* argv[]) { - int ret = -1; // success +// 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) { + options->extra_options = android::base::GetProperty(PROPERTY_EXTRA_OPTIONS, ""); + if (!options->extra_options.empty()) { + // Framework uses a system property to override some command-line args. + // Currently, it contains the type of the requested bugreport. + if (options->extra_options == "bugreportplus") { + // 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; + } else if (options->extra_options == "bugreportremote") { + options->do_vibrate = false; + options->is_remote_mode = true; + options->do_fb = false; + } else if (options->extra_options == "bugreportwear") { + options->do_start_service = true; + options->do_progress_updates = true; + options->do_zip_file = true; + } else if (options->extra_options == "bugreporttelephony") { + options->telephony_only = true; + } else if (options->extra_options == "bugreportwifi") { + options->wifi_only = true; + options->do_zip_file = true; + } else { + MYLOGE("Unknown extra option: %s\n", options->extra_options.c_str()); + } + // Reset the property + android::base::SetProperty(PROPERTY_EXTRA_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->extra_options = 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()); + } +} + +Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[]) { + RunStatus status = RunStatus::OK; int c; while ((c = getopt(argc, argv, "dho:svqzpPBRSV:")) != -1) { switch (c) { // clang-format off - case 'd': options_.do_add_date = true; break; - case 'z': options_.do_zip_file = true; break; - case 'o': options_.use_outfile = optarg; break; - case 's': options_.use_socket = true; break; - case 'S': options_.use_control_socket = true; break; - case 'v': options_.show_header_only = true; break; - case 'q': options_.do_vibrate = false; break; - case 'p': options_.do_fb = true; break; - case 'P': update_progress_ = true; break; - case 'R': options_.is_remote_mode = true; break; - case 'B': options_.do_broadcast = true; break; - case 'V': break; // compatibility no-op + case 'd': do_add_date = true; break; + case 'z': do_zip_file = true; break; + case 'o': use_outfile = optarg; break; + case 's': use_socket = true; break; + case 'S': use_control_socket = true; break; + case 'v': show_header_only = true; break; + case 'q': do_vibrate = false; break; + 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 'h': - ret = 0; + status = RunStatus::HELP; break; default: fprintf(stderr, "Invalid option: %c\n", c); - ret = 1; + status = RunStatus::INVALID_INPUT; break; // clang-format on } @@ -1967,87 +2012,44 @@ int Dumpstate::ParseCommandlineOptions(int argc, char* argv[]) { // TODO: use helper function to convert argv into a string for (int i = 0; i < argc; i++) { - args_ += argv[i]; + args += argv[i]; if (i < argc - 1) { - args_ += " "; + args += " "; } } // Reset next index used by getopt so this can be called multiple times, for eg, in tests. optind = 1; - return ret; -} -// TODO: Move away from system properties when we have binder. -void Dumpstate::SetOptionsFromProperties() { - 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") { - // Currently, the dumpstate binder is only used by Shell to update progress. - options_.do_start_service = true; - update_progress_ = true; - options_.do_fb = false; - } else if (extra_options_ == "bugreportremote") { - options_.do_vibrate = false; - options_.is_remote_mode = true; - options_.do_fb = false; - } else if (extra_options_ == "bugreportwear") { - options_.do_start_service = true; - update_progress_ = true; - options_.do_zip_file = true; - } else if (extra_options_ == "bugreporttelephony") { - options_.telephony_only = true; - } else if (extra_options_ == "bugreportwifi") { - options_.wifi_only = true; - options_.do_zip_file = true; - } else { - MYLOGE("Unknown extra option: %s\n", extra_options_.c_str()); - } - // Reset the property - android::base::SetProperty(PROPERTY_EXTRA_OPTIONS, ""); - } - - notification_title = android::base::GetProperty(PROPERTY_EXTRA_TITLE, ""); - if (!notification_title.empty()) { - // Reset the property - android::base::SetProperty(PROPERTY_EXTRA_TITLE, ""); - - notification_description = android::base::GetProperty(PROPERTY_EXTRA_DESCRIPTION, ""); - if (!notification_description.empty()) { - // Reset the property - android::base::SetProperty(PROPERTY_EXTRA_DESCRIPTION, ""); - } - MYLOGD("notification (title: %s, description: %s)\n", notification_title.c_str(), - notification_description.c_str()); - } + SetOptionsFromProperties(this); + return status; } -bool Dumpstate::ValidateOptions() { - if ((options_.do_zip_file || options_.do_add_date || ds.update_progress_ || - options_.do_broadcast) && - options_.use_outfile.empty()) { +bool Dumpstate::DumpOptions::ValidateOptions() const { + if ((do_zip_file || do_add_date || do_progress_updates || do_broadcast) + && use_outfile.empty()) { return false; } - if (options_.use_control_socket && !options_.do_zip_file) { + if (use_control_socket && !do_zip_file) { return false; } - if (ds.update_progress_ && !options_.do_broadcast) { + if (do_progress_updates && !do_broadcast) { return false; } - if (options_.is_remote_mode && (ds.update_progress_ || !options_.do_broadcast || - !options_.do_zip_file || !options_.do_add_date)) { + if (is_remote_mode && (do_progress_updates || !do_broadcast || !do_zip_file || !do_add_date)) { return false; } return true; } -/* Main entry point for dumpstate. */ -int run_main(int argc, char* argv[]) { +Dumpstate::RunStatus Dumpstate::RunWithOptions(std::unique_ptr<DumpOptions> options) { + if (!options->ValidateOptions()) { + return RunStatus::INVALID_INPUT; + } + options_ = std::move(options); /* set as high priority, and protect from OOM killer */ setpriority(PRIO_PROCESS, 0, -20); @@ -2064,52 +2066,42 @@ int run_main(int argc, char* argv[]) { } } - int status = ds.ParseCommandlineOptions(argc, argv); - if (status != -1) { - ShowUsageAndExit(status); - } - ds.SetOptionsFromProperties(); - if (!ds.ValidateOptions()) { - ExitOnInvalidArgs(); - } - - if (ds.version_ == VERSION_DEFAULT) { - ds.version_ = VERSION_CURRENT; + if (version_ == VERSION_DEFAULT) { + version_ = VERSION_CURRENT; } - if (ds.version_ != VERSION_CURRENT && ds.version_ != VERSION_SPLIT_ANR) { + if (version_ != VERSION_CURRENT && version_ != VERSION_SPLIT_ANR) { MYLOGE("invalid version requested ('%s'); suppported values are: ('%s', '%s', '%s')\n", - ds.version_.c_str(), VERSION_DEFAULT.c_str(), VERSION_CURRENT.c_str(), + version_.c_str(), VERSION_DEFAULT.c_str(), VERSION_CURRENT.c_str(), VERSION_SPLIT_ANR.c_str()); - exit(1); + return RunStatus::INVALID_INPUT; } - const Dumpstate::DumpOptions& options = ds.options_; - if (options.show_header_only) { - ds.PrintHeader(); - exit(0); + if (options_->show_header_only) { + PrintHeader(); + return RunStatus::OK; } // Redirect output if needed - bool is_redirecting = !options.use_socket && !options.use_outfile.empty(); + bool is_redirecting = !options_->use_socket && !options_->use_outfile.empty(); // TODO: temporarily set progress until it's part of the Dumpstate constructor - std::string stats_path = is_redirecting - ? android::base::StringPrintf("%s/dumpstate-stats.txt", - dirname(options.use_outfile.c_str())) - : ""; - ds.progress_.reset(new Progress(stats_path)); + std::string stats_path = + is_redirecting ? android::base::StringPrintf("%s/dumpstate-stats.txt", + dirname(options_->use_outfile.c_str())) + : ""; + progress_.reset(new Progress(stats_path)); /* gets the sequential id */ uint32_t last_id = android::base::GetIntProperty(PROPERTY_LAST_ID, 0); - ds.id_ = ++last_id; + id_ = ++last_id; android::base::SetProperty(PROPERTY_LAST_ID, std::to_string(last_id)); MYLOGI("begin\n"); register_sig_handler(); - if (options.do_start_service) { + if (options_->do_start_service) { MYLOGI("Starting 'dumpstate' service\n"); android::status_t ret; if ((ret = android::os::DumpstateService::Start()) != android::OK) { @@ -2121,43 +2113,43 @@ int run_main(int argc, char* argv[]) { 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", ds.id_, ds.args_.c_str(), - ds.extra_options_.c_str()); + MYLOGI("dumpstate info: id=%d, args='%s', extra_options= %s)\n", id_, options_->args.c_str(), + options_->extra_options.c_str()); - MYLOGI("bugreport format version: %s\n", ds.version_.c_str()); + MYLOGI("bugreport format version: %s\n", version_.c_str()); - ds.do_early_screenshot_ = ds.update_progress_; + do_early_screenshot_ = options_->do_progress_updates; // If we are going to use a socket, do it as early as possible // to avoid timeouts from bugreport. - if (options.use_socket) { + if (options_->use_socket) { redirect_to_socket(stdout, "dumpstate"); } - if (options.use_control_socket) { + if (options_->use_control_socket) { MYLOGD("Opening control socket\n"); - ds.control_socket_fd_ = open_socket("dumpstate"); - ds.update_progress_ = 1; + control_socket_fd_ = open_socket("dumpstate"); + options_->do_progress_updates = 1; } if (is_redirecting) { PrepareToWriteToFile(); - if (ds.update_progress_) { - if (options.do_broadcast) { + 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", ds.name_, - "--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.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); } - if (options.use_control_socket) { - dprintf(ds.control_socket_fd_, "BEGIN:%s\n", ds.path_.c_str()); + if (options_->use_control_socket) { + dprintf(control_socket_fd_, "BEGIN:%s\n", path_.c_str()); } } } @@ -2169,23 +2161,23 @@ int run_main(int argc, char* argv[]) { fclose(cmdline); } - if (options.do_vibrate) { + if (options_->do_vibrate) { Vibrate(150); } - if (options.do_fb && ds.do_early_screenshot_) { - if (ds.screenshot_path_.empty()) { + if (options_->do_fb && do_early_screenshot_) { + if (screenshot_path_.empty()) { // should not have happened MYLOGE("INTERNAL ERROR: skipping early screenshot because path was not set\n"); } else { MYLOGI("taking early screenshot\n"); - ds.TakeScreenshot(); + TakeScreenshot(); } } - if (options.do_zip_file && ds.zip_file != nullptr) { - if (chown(ds.path_.c_str(), AID_SHELL, AID_SHELL)) { - MYLOGE("Unable to change ownership of zip file %s: %s\n", ds.path_.c_str(), + 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(), strerror(errno)); } } @@ -2194,19 +2186,19 @@ int run_main(int argc, char* argv[]) { int dup_stderr_fd; if (is_redirecting) { TEMP_FAILURE_RETRY(dup_stderr_fd = dup(fileno(stderr))); - redirect_to_file(stderr, const_cast<char*>(ds.log_path_.c_str())); - if (chown(ds.log_path_.c_str(), AID_SHELL, AID_SHELL)) { - MYLOGE("Unable to change ownership of dumpstate log file %s: %s\n", - ds.log_path_.c_str(), strerror(errno)); + redirect_to_file(stderr, const_cast<char*>(log_path_.c_str())); + if (chown(log_path_.c_str(), AID_SHELL, AID_SHELL)) { + MYLOGE("Unable to change ownership of dumpstate log file %s: %s\n", log_path_.c_str(), + strerror(errno)); } TEMP_FAILURE_RETRY(dup_stdout_fd = dup(fileno(stdout))); /* TODO: rather than generating a text file now and zipping it later, it would be more efficient to redirect stdout to the zip entry directly, but the libziparchive doesn't support that option yet. */ - redirect_to_file(stdout, const_cast<char*>(ds.tmp_path_.c_str())); - if (chown(ds.tmp_path_.c_str(), AID_SHELL, AID_SHELL)) { + redirect_to_file(stdout, const_cast<char*>(tmp_path_.c_str())); + if (chown(tmp_path_.c_str(), AID_SHELL, AID_SHELL)) { MYLOGE("Unable to change ownership of temporary bugreport file %s: %s\n", - ds.tmp_path_.c_str(), strerror(errno)); + tmp_path_.c_str(), strerror(errno)); } } @@ -2216,18 +2208,18 @@ int run_main(int argc, char* argv[]) { // NOTE: there should be no stdout output until now, otherwise it would break the header. // In particular, DurationReport objects should be created passing 'title, NULL', so their // duration is logged into MYLOG instead. - ds.PrintHeader(); + PrintHeader(); - if (options.telephony_only) { + if (options_->telephony_only) { DumpstateTelephonyOnly(); - ds.DumpstateBoard(); - } else if (options.wifi_only) { + DumpstateBoard(); + } else if (options_->wifi_only) { DumpstateWifiOnly(); } else { // Dump state for the default case. This also drops root. if (!DumpstateDefault()) { // Something went wrong. - return -1; + return RunStatus::ERROR; } } @@ -2237,12 +2229,12 @@ int run_main(int argc, char* argv[]) { } /* rename or zip the (now complete) .tmp file to its final location */ - if (!options.use_outfile.empty()) { + if (!options_->use_outfile.empty()) { FinalizeFile(); } /* vibrate a few but shortly times to let user know it's finished */ - if (options.do_vibrate) { + if (options_->do_vibrate) { for (int i = 0; i < 3; i++) { Vibrate(75); usleep((75 + 50) * 1000); @@ -2250,26 +2242,51 @@ int run_main(int argc, char* argv[]) { } /* tell activity manager we're done */ - if (options.do_broadcast) { + if (options_->do_broadcast) { SendBugreportFinishedBroadcast(); } - MYLOGD("Final progress: %d/%d (estimated %d)\n", ds.progress_->Get(), ds.progress_->GetMax(), - ds.progress_->GetInitialMax()); - ds.progress_->Save(); - MYLOGI("done (id %d)\n", ds.id_); + MYLOGD("Final progress: %d/%d (estimated %d)\n", progress_->Get(), progress_->GetMax(), + progress_->GetInitialMax()); + progress_->Save(); + MYLOGI("done (id %d)\n", id_); if (is_redirecting) { TEMP_FAILURE_RETRY(dup2(dup_stderr_fd, fileno(stderr))); } - if (options.use_control_socket && ds.control_socket_fd_ != -1) { + if (options_->use_control_socket && control_socket_fd_ != -1) { MYLOGD("Closing control socket\n"); - close(ds.control_socket_fd_); + close(control_socket_fd_); } - ds.tombstone_data_.clear(); - ds.anr_data_.clear(); + tombstone_data_.clear(); + anr_data_.clear(); + + return RunStatus::OK; +} +/* Main entry point for dumpstate. */ +int run_main(int argc, char* argv[]) { + std::unique_ptr<Dumpstate::DumpOptions> options = std::make_unique<Dumpstate::DumpOptions>(); + Dumpstate::RunStatus status = options->Initialize(argc, argv); + if (status == Dumpstate::RunStatus::OK) { + status = ds.RunWithOptions(std::move(options)); + } + + switch (status) { + case Dumpstate::RunStatus::OK: + return 0; + break; + case Dumpstate::RunStatus::HELP: + ShowUsageAndExit(0 /* exit code */); + break; + case Dumpstate::RunStatus::INVALID_INPUT: + ExitOnInvalidArgs(); + break; + case Dumpstate::RunStatus::ERROR: + exit(-1); + break; + } return 0; } diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index 389cc2e218..c2f7f6ac23 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -183,6 +183,8 @@ class Dumpstate { friend class DumpstateTest; public: + enum RunStatus { OK, HELP, INVALID_INPUT, ERROR }; + static android::os::dumpstate::CommandOptions DEFAULT_DUMPSYS; static Dumpstate& GetInstance(); @@ -290,22 +292,12 @@ class Dumpstate { /* Returns true if the current version supports priority dump feature. */ bool CurrentVersionSupportsPriorityDumps() const; - // TODO: revisit the return values later. - /* - * Parses commandline arguments and sets runtime options accordingly. - * - * Returns 0 or positive number if the caller should exit with returned value as - * exit code, or returns -1 if caller should proceed with execution. - */ - int ParseCommandlineOptions(int argc, char* argv[]); - - /* Sets runtime options from the system properties. */ - void SetOptionsFromProperties(); + struct DumpOptions; - /* Returns true if the options set so far are consistent. */ - bool ValidateOptions(); + /* Main entry point for running a complete bugreport. Takes ownership of options. */ + RunStatus RunWithOptions(std::unique_ptr<DumpOptions> options); - // TODO: add update_progress_ & other options from DumpState. + // TODO: add other options from DumpState. /* * Structure to hold options that determine the behavior of dumpstate. */ @@ -322,7 +314,22 @@ class Dumpstate { bool do_start_service = false; bool telephony_only = false; bool wifi_only = false; + // Whether progress updates should be published. + bool do_progress_updates = false; std::string use_outfile; + // Extra options passed as system property. + std::string extra_options; + // 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. */ + RunStatus Initialize(int argc, char* argv[]); + + /* Returns true if the options set so far are consistent. */ + bool ValidateOptions() const; }; // TODO: initialize fields on constructor @@ -333,10 +340,7 @@ class Dumpstate { pid_t pid_; // Runtime options. - DumpOptions options_; - - // Whether progress updates should be published. - bool update_progress_ = false; + std::unique_ptr<DumpOptions> options_; // How frequently the progess should be updated;the listener will only be notificated when the // delta from the previous update is more than the threshold. @@ -356,12 +360,6 @@ class Dumpstate { // Bugreport format version; std::string version_ = VERSION_CURRENT; - // Command-line arguments as string - std::string args_; - - // Extra options passed as system property. - std::string extra_options_; - // Full path of the directory where the bugreport files will be written. std::string bugreport_dir_; @@ -398,10 +396,6 @@ class Dumpstate { std::string listener_name_; bool report_section_; - // Notification title and description - std::string notification_title; - std::string notification_description; - // List of open tombstone dump files. std::vector<DumpData> tombstone_data_; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index c57535a6c1..b675c515fe 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -137,96 +137,42 @@ class DumpstateBaseTest : public Test { } }; -class DumpstateTest : public DumpstateBaseTest { +class DumpOptionsTest : public Test { public: - void SetUp() { - DumpstateBaseTest::SetUp(); - SetDryRun(false); - SetBuildType(android::base::GetProperty("ro.build.type", "(unknown)")); - ds.progress_.reset(new Progress()); - ds.update_progress_ = false; - ds.update_progress_threshold_ = 0; - ds.options_ = Dumpstate::DumpOptions(); - } - - // Runs a command and capture `stdout` and `stderr`. - int RunCommand(const std::string& title, const std::vector<std::string>& full_command, - const CommandOptions& options = CommandOptions::DEFAULT) { - CaptureStdout(); - CaptureStderr(); - int status = ds.RunCommand(title, full_command, options); - out = GetCapturedStdout(); - err = GetCapturedStderr(); - return status; - } - - // Dumps a file and capture `stdout` and `stderr`. - int DumpFile(const std::string& title, const std::string& path) { - CaptureStdout(); - CaptureStderr(); - int status = ds.DumpFile(title, path); - out = GetCapturedStdout(); - err = GetCapturedStderr(); - return status; - } - - void SetProgress(long progress, long initial_max, long threshold = 0) { - ds.update_progress_ = true; - ds.update_progress_threshold_ = threshold; - ds.last_updated_progress_ = 0; - ds.progress_.reset(new Progress(initial_max, progress, 1.2)); + virtual ~DumpOptionsTest() { } - - std::string GetProgressMessage(const std::string& listener_name, 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"; - - bool max_increased = old_max > 0; - - std::string message = ""; - if (max_increased) { - message = - android::base::StringPrintf("Adjusting max progress from %d to %d\n", old_max, max); - } - - if (update_progress) { - message += android::base::StringPrintf("Setting progress (%s): %d/%d\n", - listener_name.c_str(), progress, max); - } - - return message; + virtual void SetUp() { + options_ = Dumpstate::DumpOptions(); } - // `stdout` and `stderr` from the last command ran. - std::string out, err; - - Dumpstate& ds = Dumpstate::GetInstance(); + Dumpstate::DumpOptions options_; }; -TEST_F(DumpstateTest, ParseCommandlineOptionsNone) { +TEST_F(DumpOptionsTest, InitializeNone) { // clang-format off char* argv[] = { const_cast<char*>("dumpstate") }; // clang-format on - int ret = ds.ParseCommandlineOptions(ARRAY_SIZE(argv), argv); - EXPECT_EQ(-1, ret); - EXPECT_FALSE(ds.options_.do_add_date); - EXPECT_FALSE(ds.options_.do_zip_file); - EXPECT_EQ("", ds.options_.use_outfile); - EXPECT_FALSE(ds.options_.use_socket); - EXPECT_FALSE(ds.options_.use_control_socket); - EXPECT_FALSE(ds.options_.show_header_only); - EXPECT_TRUE(ds.options_.do_vibrate); - EXPECT_FALSE(ds.options_.do_fb); - EXPECT_FALSE(ds.update_progress_); - EXPECT_FALSE(ds.options_.is_remote_mode); - EXPECT_FALSE(ds.options_.do_broadcast); -} - -TEST_F(DumpstateTest, ParseCommandlineOptionsPartial1) { + Dumpstate::DumpOptions options; + Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); + + EXPECT_EQ(status, Dumpstate::RunStatus::OK); + EXPECT_FALSE(options_.do_add_date); + EXPECT_FALSE(options_.do_zip_file); + EXPECT_EQ("", options_.use_outfile); + EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.use_control_socket); + EXPECT_FALSE(options_.show_header_only); + EXPECT_TRUE(options_.do_vibrate); + 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, InitializePartial1) { // clang-format off char* argv[] = { const_cast<char*>("dumpstate"), @@ -238,25 +184,27 @@ TEST_F(DumpstateTest, ParseCommandlineOptionsPartial1) { }; // clang-format on - int ret = ds.ParseCommandlineOptions(ARRAY_SIZE(argv), argv); - EXPECT_EQ(-1, ret); - EXPECT_TRUE(ds.options_.do_add_date); - EXPECT_TRUE(ds.options_.do_zip_file); + + Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); + + EXPECT_EQ(status, Dumpstate::RunStatus::OK); + EXPECT_TRUE(options_.do_add_date); + EXPECT_TRUE(options_.do_zip_file); // TODO: Maybe we should trim the filename - EXPECT_EQ(" abc", std::string(ds.options_.use_outfile)); - EXPECT_TRUE(ds.options_.use_socket); - EXPECT_TRUE(ds.options_.use_control_socket); + EXPECT_EQ(" abc", std::string(options_.use_outfile)); + EXPECT_TRUE(options_.use_socket); + EXPECT_TRUE(options_.use_control_socket); // Other options retain default values - EXPECT_FALSE(ds.options_.show_header_only); - EXPECT_TRUE(ds.options_.do_vibrate); - EXPECT_FALSE(ds.options_.do_fb); - EXPECT_FALSE(ds.update_progress_); - EXPECT_FALSE(ds.options_.is_remote_mode); - EXPECT_FALSE(ds.options_.do_broadcast); + EXPECT_FALSE(options_.show_header_only); + EXPECT_TRUE(options_.do_vibrate); + EXPECT_FALSE(options_.do_fb); + EXPECT_FALSE(options_.do_progress_updates); + EXPECT_FALSE(options_.is_remote_mode); + EXPECT_FALSE(options_.do_broadcast); } -TEST_F(DumpstateTest, ParseCommandlineOptionsPartial2) { +TEST_F(DumpOptionsTest, InitializePartial2) { // clang-format off char* argv[] = { const_cast<char*>("dumpstate"), @@ -268,93 +216,162 @@ TEST_F(DumpstateTest, ParseCommandlineOptionsPartial2) { const_cast<char*>("-B"), }; // clang-format on - int ret = ds.ParseCommandlineOptions(ARRAY_SIZE(argv), argv); - EXPECT_EQ(-1, ret); - EXPECT_TRUE(ds.options_.show_header_only); - EXPECT_FALSE(ds.options_.do_vibrate); - EXPECT_TRUE(ds.options_.do_fb); - EXPECT_TRUE(ds.update_progress_); - EXPECT_TRUE(ds.options_.is_remote_mode); - EXPECT_TRUE(ds.options_.do_broadcast); + + Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); + + 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_progress_updates); + EXPECT_TRUE(options_.is_remote_mode); + EXPECT_TRUE(options_.do_broadcast); // Other options retain default values - EXPECT_FALSE(ds.options_.do_add_date); - EXPECT_FALSE(ds.options_.do_zip_file); - EXPECT_EQ("", ds.options_.use_outfile); - EXPECT_FALSE(ds.options_.use_socket); - EXPECT_FALSE(ds.options_.use_control_socket); + EXPECT_FALSE(options_.do_add_date); + EXPECT_FALSE(options_.do_zip_file); + EXPECT_EQ("", options_.use_outfile); + EXPECT_FALSE(options_.use_socket); + EXPECT_FALSE(options_.use_control_socket); } -TEST_F(DumpstateTest, ParseCommandlineOptionsHelp) { +TEST_F(DumpOptionsTest, InitializeHelp) { // clang-format off char* argv[] = { const_cast<char*>("dumpstate"), const_cast<char*>("-h") }; // clang-format on - int ret = ds.ParseCommandlineOptions(ARRAY_SIZE(argv), argv); - // -h is for help. Caller exit with code = 0 after printing usage, so expect return = 0. - EXPECT_EQ(0, ret); + Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); + + // -h is for help. + EXPECT_EQ(status, Dumpstate::RunStatus::HELP); } -TEST_F(DumpstateTest, ParseCommandlineOptionsUnknown) { +TEST_F(DumpOptionsTest, InitializeUnknown) { // clang-format off char* argv[] = { const_cast<char*>("dumpstate"), const_cast<char*>("-u") // unknown flag }; // clang-format on - int ret = ds.ParseCommandlineOptions(ARRAY_SIZE(argv), argv); - // -u is unknown. Caller exit with code = 1 to show execution failure, after printing usage, - // so expect return = 1. - EXPECT_EQ(1, ret); + Dumpstate::RunStatus status = options_.Initialize(ARRAY_SIZE(argv), argv); + + // -u is unknown. + EXPECT_EQ(status, Dumpstate::RunStatus::INVALID_INPUT); } -TEST_F(DumpstateTest, ValidateOptionsNeedOutfile1) { - ds.options_.do_zip_file = true; - EXPECT_FALSE(ds.ValidateOptions()); - ds.options_.use_outfile = "a/b/c"; - EXPECT_TRUE(ds.ValidateOptions()); +TEST_F(DumpOptionsTest, ValidateOptionsNeedOutfile1) { + options_.do_zip_file = true; + EXPECT_FALSE(options_.ValidateOptions()); + options_.use_outfile = "a/b/c"; + EXPECT_TRUE(options_.ValidateOptions()); } -TEST_F(DumpstateTest, ValidateOptionsNeedOutfile2) { - ds.options_.do_broadcast = true; - EXPECT_FALSE(ds.ValidateOptions()); - ds.options_.use_outfile = "a/b/c"; - EXPECT_TRUE(ds.ValidateOptions()); +TEST_F(DumpOptionsTest, ValidateOptionsNeedOutfile2) { + options_.do_broadcast = true; + EXPECT_FALSE(options_.ValidateOptions()); + options_.use_outfile = "a/b/c"; + EXPECT_TRUE(options_.ValidateOptions()); } -TEST_F(DumpstateTest, ValidateOptionsNeedZipfile) { - ds.options_.use_control_socket = true; - EXPECT_FALSE(ds.ValidateOptions()); +TEST_F(DumpOptionsTest, ValidateOptionsNeedZipfile) { + options_.use_control_socket = true; + EXPECT_FALSE(options_.ValidateOptions()); - ds.options_.do_zip_file = true; - ds.options_.use_outfile = "a/b/c"; // do_zip_file needs outfile - EXPECT_TRUE(ds.ValidateOptions()); + options_.do_zip_file = true; + options_.use_outfile = "a/b/c"; // do_zip_file needs outfile + EXPECT_TRUE(options_.ValidateOptions()); } -TEST_F(DumpstateTest, ValidateOptionsUpdateProgressNeedsBroadcast) { - ds.update_progress_ = true; - ds.options_.use_outfile = "a/b/c"; // update_progress_ needs outfile - EXPECT_FALSE(ds.ValidateOptions()); +TEST_F(DumpOptionsTest, ValidateOptionsUpdateProgressNeedsBroadcast) { + options_.do_progress_updates = true; + options_.use_outfile = "a/b/c"; // do_progress_updates needs outfile + EXPECT_FALSE(options_.ValidateOptions()); - ds.options_.do_broadcast = true; - EXPECT_TRUE(ds.ValidateOptions()); + options_.do_broadcast = true; + EXPECT_TRUE(options_.ValidateOptions()); } -TEST_F(DumpstateTest, ValidateOptionsRemoteMode) { - ds.options_.is_remote_mode = true; - EXPECT_FALSE(ds.ValidateOptions()); +TEST_F(DumpOptionsTest, ValidateOptionsRemoteMode) { + options_.is_remote_mode = true; + EXPECT_FALSE(options_.ValidateOptions()); - ds.options_.do_broadcast = true; - ds.options_.do_zip_file = true; - ds.options_.do_add_date = true; - ds.options_.use_outfile = "a/b/c"; // do_broadcast needs outfile - EXPECT_TRUE(ds.ValidateOptions()); + options_.do_broadcast = true; + options_.do_zip_file = true; + options_.do_add_date = true; + options_.use_outfile = "a/b/c"; // do_broadcast needs outfile + EXPECT_TRUE(options_.ValidateOptions()); } +class DumpstateTest : public DumpstateBaseTest { + public: + void SetUp() { + DumpstateBaseTest::SetUp(); + SetDryRun(false); + SetBuildType(android::base::GetProperty("ro.build.type", "(unknown)")); + ds.progress_.reset(new Progress()); + ds.update_progress_threshold_ = 0; + ds.options_.reset(new Dumpstate::DumpOptions()); + } + + // Runs a command and capture `stdout` and `stderr`. + int RunCommand(const std::string& title, const std::vector<std::string>& full_command, + const CommandOptions& options = CommandOptions::DEFAULT) { + CaptureStdout(); + CaptureStderr(); + int status = ds.RunCommand(title, full_command, options); + out = GetCapturedStdout(); + err = GetCapturedStderr(); + return status; + } + + // Dumps a file and capture `stdout` and `stderr`. + int DumpFile(const std::string& title, const std::string& path) { + CaptureStdout(); + CaptureStderr(); + int status = ds.DumpFile(title, path); + out = GetCapturedStdout(); + err = GetCapturedStderr(); + return status; + } + + void SetProgress(long progress, long initial_max, long threshold = 0) { + ds.options_->do_progress_updates = true; + ds.update_progress_threshold_ = threshold; + ds.last_updated_progress_ = 0; + ds.progress_.reset(new Progress(initial_max, progress, 1.2)); + } + + std::string GetProgressMessage(const std::string& listener_name, 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"; + + bool max_increased = old_max > 0; + + std::string message = ""; + if (max_increased) { + message = + android::base::StringPrintf("Adjusting max progress from %d to %d\n", old_max, max); + } + + if (update_progress) { + message += android::base::StringPrintf("Setting progress (%s): %d/%d\n", + listener_name.c_str(), progress, max); + } + + return message; + } + + // `stdout` and `stderr` from the last command ran. + std::string out, err; + + Dumpstate& ds = Dumpstate::GetInstance(); +}; + TEST_F(DumpstateTest, RunCommandNoArgs) { EXPECT_EQ(-1, RunCommand("", {})); } diff --git a/cmds/dumpstate/utils.cpp b/cmds/dumpstate/utils.cpp index 77f09b7459..4ad5c4b7d5 100644 --- a/cmds/dumpstate/utils.cpp +++ b/cmds/dumpstate/utils.cpp @@ -916,7 +916,7 @@ void Dumpstate::UpdateProgress(int32_t delta_sec) { bool max_changed = progress_->Inc(delta_sec); // ...but only notifiy listeners when necessary. - if (!update_progress_) return; + if (!options_->do_progress_updates) return; int progress = progress_->Get(); int max = progress_->GetMax(); diff --git a/cmds/installd/dexopt.cpp b/cmds/installd/dexopt.cpp index 174ab21658..b6038f917f 100644 --- a/cmds/installd/dexopt.cpp +++ b/cmds/installd/dexopt.cpp @@ -2693,6 +2693,13 @@ static bool create_boot_image_profile_snapshot(const std::string& package_name, return false; } + // Return false for empty class path since it may otherwise return true below if profiles is + // empty. + if (classpath.empty()) { + PLOG(ERROR) << "Class path is empty"; + return false; + } + // Open and create the snapshot profile. unique_fd snapshot_fd = open_spnashot_profile(AID_SYSTEM, package_name, profile_name); diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp index 1bd7c4fb42..f6cc3afc97 100644 --- a/libs/binder/Binder.cpp +++ b/libs/binder/Binder.cpp @@ -115,6 +115,7 @@ const String16& BBinder::getInterfaceDescriptor() const return sEmptyDescriptor; } +// NOLINTNEXTLINE(google-default-arguments) status_t BBinder::transact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { @@ -137,6 +138,7 @@ status_t BBinder::transact( return err; } +// NOLINTNEXTLINE(google-default-arguments) status_t BBinder::linkToDeath( const sp<DeathRecipient>& /*recipient*/, void* /*cookie*/, uint32_t /*flags*/) @@ -144,6 +146,7 @@ status_t BBinder::linkToDeath( return INVALID_OPERATION; } +// NOLINTNEXTLINE(google-default-arguments) status_t BBinder::unlinkToDeath( const wp<DeathRecipient>& /*recipient*/, void* /*cookie*/, uint32_t /*flags*/, wp<DeathRecipient>* /*outRecipient*/) @@ -208,6 +211,7 @@ BBinder::~BBinder() } +// NOLINTNEXTLINE(google-default-arguments) status_t BBinder::onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t /*flags*/) { diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp index 734212626a..ec170f7a65 100644 --- a/libs/binder/BpBinder.cpp +++ b/libs/binder/BpBinder.cpp @@ -206,6 +206,7 @@ status_t BpBinder::dump(int fd, const Vector<String16>& args) return err; } +// NOLINTNEXTLINE(google-default-arguments) status_t BpBinder::transact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { @@ -220,6 +221,7 @@ status_t BpBinder::transact( return DEAD_OBJECT; } +// NOLINTNEXTLINE(google-default-arguments) status_t BpBinder::linkToDeath( const sp<DeathRecipient>& recipient, void* cookie, uint32_t flags) { @@ -254,6 +256,7 @@ status_t BpBinder::linkToDeath( return DEAD_OBJECT; } +// NOLINTNEXTLINE(google-default-arguments) status_t BpBinder::unlinkToDeath( const wp<DeathRecipient>& recipient, void* cookie, uint32_t flags, wp<DeathRecipient>* outRecipient) diff --git a/libs/binder/IAppOpsCallback.cpp b/libs/binder/IAppOpsCallback.cpp index f9ec593f77..2f4dbeea18 100644 --- a/libs/binder/IAppOpsCallback.cpp +++ b/libs/binder/IAppOpsCallback.cpp @@ -49,6 +49,7 @@ IMPLEMENT_META_INTERFACE(AppOpsCallback, "com.android.internal.app.IAppOpsCallba // ---------------------------------------------------------------------- +// NOLINTNEXTLINE(google-default-arguments) status_t BnAppOpsCallback::onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { diff --git a/libs/binder/IAppOpsService.cpp b/libs/binder/IAppOpsService.cpp index 068664b418..fb0d521cac 100644 --- a/libs/binder/IAppOpsService.cpp +++ b/libs/binder/IAppOpsService.cpp @@ -129,6 +129,7 @@ IMPLEMENT_META_INTERFACE(AppOpsService, "com.android.internal.app.IAppOpsService // ---------------------------------------------------------------------- +// NOLINTNEXTLINE(google-default-arguments) status_t BnAppOpsService::onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { diff --git a/libs/binder/IBatteryStats.cpp b/libs/binder/IBatteryStats.cpp index ad1e69faeb..b307e3e7b5 100644 --- a/libs/binder/IBatteryStats.cpp +++ b/libs/binder/IBatteryStats.cpp @@ -136,6 +136,7 @@ IMPLEMENT_META_INTERFACE(BatteryStats, "com.android.internal.app.IBatteryStats") // ---------------------------------------------------------------------- +// NOLINTNEXTLINE(google-default-arguments) status_t BnBatteryStats::onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { diff --git a/libs/binder/IMemory.cpp b/libs/binder/IMemory.cpp index 507ce53b66..307bc28c0e 100644 --- a/libs/binder/IMemory.cpp +++ b/libs/binder/IMemory.cpp @@ -129,6 +129,7 @@ class BpMemory : public BpInterface<IMemory> public: explicit BpMemory(const sp<IBinder>& impl); virtual ~BpMemory(); + // NOLINTNEXTLINE(google-default-arguments) virtual sp<IMemoryHeap> getMemory(ssize_t* offset=nullptr, size_t* size=nullptr) const; private: @@ -180,6 +181,7 @@ BpMemory::~BpMemory() { } +// NOLINTNEXTLINE(google-default-arguments) sp<IMemoryHeap> BpMemory::getMemory(ssize_t* offset, size_t* size) const { if (mHeap == nullptr) { @@ -224,6 +226,7 @@ BnMemory::BnMemory() { BnMemory::~BnMemory() { } +// NOLINTNEXTLINE(google-default-arguments) status_t BnMemory::onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { @@ -383,6 +386,7 @@ BnMemoryHeap::BnMemoryHeap() { BnMemoryHeap::~BnMemoryHeap() { } +// NOLINTNEXTLINE(google-default-arguments) status_t BnMemoryHeap::onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { diff --git a/libs/binder/IPermissionController.cpp b/libs/binder/IPermissionController.cpp index 89ebc6c1aa..6b99150edf 100644 --- a/libs/binder/IPermissionController.cpp +++ b/libs/binder/IPermissionController.cpp @@ -109,6 +109,7 @@ IMPLEMENT_META_INTERFACE(PermissionController, "android.os.IPermissionController // ---------------------------------------------------------------------- +// NOLINTNEXTLINE(google-default-arguments) status_t BnPermissionController::onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { diff --git a/libs/binder/IResultReceiver.cpp b/libs/binder/IResultReceiver.cpp index 14b5259536..159763d5bf 100644 --- a/libs/binder/IResultReceiver.cpp +++ b/libs/binder/IResultReceiver.cpp @@ -48,6 +48,7 @@ IMPLEMENT_META_INTERFACE(ResultReceiver, "com.android.internal.os.IResultReceive // ---------------------------------------------------------------------- +// NOLINTNEXTLINE(google-default-arguments) status_t BnResultReceiver::onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { diff --git a/libs/binder/IShellCallback.cpp b/libs/binder/IShellCallback.cpp index dd4a65ee84..6c697decca 100644 --- a/libs/binder/IShellCallback.cpp +++ b/libs/binder/IShellCallback.cpp @@ -58,6 +58,7 @@ IMPLEMENT_META_INTERFACE(ShellCallback, "com.android.internal.os.IShellCallback" // ---------------------------------------------------------------------- +// NOLINTNEXTLINE(google-default-arguments) status_t BnShellCallback::onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { diff --git a/libs/binder/include/binder/Binder.h b/libs/binder/include/binder/Binder.h index 0b60b4e5fd..c251468bdb 100644 --- a/libs/binder/include/binder/Binder.h +++ b/libs/binder/include/binder/Binder.h @@ -34,15 +34,18 @@ public: virtual status_t pingBinder(); virtual status_t dump(int fd, const Vector<String16>& args); + // NOLINTNEXTLINE(google-default-arguments) virtual status_t transact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags = 0); + // NOLINTNEXTLINE(google-default-arguments) virtual status_t linkToDeath(const sp<DeathRecipient>& recipient, void* cookie = nullptr, uint32_t flags = 0); + // NOLINTNEXTLINE(google-default-arguments) virtual status_t unlinkToDeath( const wp<DeathRecipient>& recipient, void* cookie = nullptr, uint32_t flags = 0, @@ -60,6 +63,7 @@ public: protected: virtual ~BBinder(); + // NOLINTNEXTLINE(google-default-arguments) virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, diff --git a/libs/binder/include/binder/BpBinder.h b/libs/binder/include/binder/BpBinder.h index c4c8ba3e74..1d4f88113a 100644 --- a/libs/binder/include/binder/BpBinder.h +++ b/libs/binder/include/binder/BpBinder.h @@ -41,14 +41,18 @@ public: virtual status_t pingBinder(); virtual status_t dump(int fd, const Vector<String16>& args); + // NOLINTNEXTLINE(google-default-arguments) virtual status_t transact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags = 0); + // NOLINTNEXTLINE(google-default-arguments) virtual status_t linkToDeath(const sp<DeathRecipient>& recipient, void* cookie = nullptr, uint32_t flags = 0); + + // NOLINTNEXTLINE(google-default-arguments) virtual status_t unlinkToDeath( const wp<DeathRecipient>& recipient, void* cookie = nullptr, uint32_t flags = 0, diff --git a/libs/binder/include/binder/IAppOpsCallback.h b/libs/binder/include/binder/IAppOpsCallback.h index e5b12a9720..b500219e37 100644 --- a/libs/binder/include/binder/IAppOpsCallback.h +++ b/libs/binder/include/binder/IAppOpsCallback.h @@ -43,6 +43,7 @@ public: class BnAppOpsCallback : public BnInterface<IAppOpsCallback> { public: + // NOLINTNEXTLINE(google-default-arguments) virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, diff --git a/libs/binder/include/binder/IAppOpsService.h b/libs/binder/include/binder/IAppOpsService.h index f0c5e1743d..78078513ff 100644 --- a/libs/binder/include/binder/IAppOpsService.h +++ b/libs/binder/include/binder/IAppOpsService.h @@ -67,6 +67,7 @@ public: class BnAppOpsService : public BnInterface<IAppOpsService> { public: + // NOLINTNEXTLINE(google-default-arguments) virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, diff --git a/libs/binder/include/binder/IBatteryStats.h b/libs/binder/include/binder/IBatteryStats.h index 59e806c177..48da865702 100644 --- a/libs/binder/include/binder/IBatteryStats.h +++ b/libs/binder/include/binder/IBatteryStats.h @@ -68,6 +68,7 @@ public: class BnBatteryStats : public BnInterface<IBatteryStats> { public: + // NOLINTNEXTLINE(google-default-arguments) virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, diff --git a/libs/binder/include/binder/IBinder.h b/libs/binder/include/binder/IBinder.h index 3f0dad0f25..14edcbe72a 100644 --- a/libs/binder/include/binder/IBinder.h +++ b/libs/binder/include/binder/IBinder.h @@ -86,6 +86,7 @@ public: Vector<String16>& args, const sp<IShellCallback>& callback, const sp<IResultReceiver>& resultReceiver); + // NOLINTNEXTLINE(google-default-arguments) virtual status_t transact( uint32_t code, const Parcel& data, Parcel* reply, @@ -131,6 +132,7 @@ public: * (Nor should you need to, as there is nothing useful you can * directly do with it now that it has passed on.) */ + // NOLINTNEXTLINE(google-default-arguments) virtual status_t linkToDeath(const sp<DeathRecipient>& recipient, void* cookie = nullptr, uint32_t flags = 0) = 0; @@ -142,6 +144,7 @@ public: * supply a NULL @a recipient, and the recipient previously * added with that cookie will be unlinked. */ + // NOLINTNEXTLINE(google-default-arguments) virtual status_t unlinkToDeath( const wp<DeathRecipient>& recipient, void* cookie = nullptr, uint32_t flags = 0, diff --git a/libs/binder/include/binder/IMemory.h b/libs/binder/include/binder/IMemory.h index 3099bf5fb8..db9f53a797 100644 --- a/libs/binder/include/binder/IMemory.h +++ b/libs/binder/include/binder/IMemory.h @@ -54,7 +54,8 @@ public: class BnMemoryHeap : public BnInterface<IMemoryHeap> { public: - virtual status_t onTransact( + // NOLINTNEXTLINE(google-default-arguments) + virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, @@ -72,6 +73,7 @@ class IMemory : public IInterface public: DECLARE_META_INTERFACE(Memory) + // NOLINTNEXTLINE(google-default-arguments) virtual sp<IMemoryHeap> getMemory(ssize_t* offset=nullptr, size_t* size=nullptr) const = 0; // helpers @@ -84,6 +86,7 @@ public: class BnMemory : public BnInterface<IMemory> { public: + // NOLINTNEXTLINE(google-default-arguments) virtual status_t onTransact( uint32_t code, const Parcel& data, diff --git a/libs/binder/include/binder/IPermissionController.h b/libs/binder/include/binder/IPermissionController.h index 3ec459fc32..26a1b23a9a 100644 --- a/libs/binder/include/binder/IPermissionController.h +++ b/libs/binder/include/binder/IPermissionController.h @@ -56,6 +56,7 @@ public: class BnPermissionController : public BnInterface<IPermissionController> { public: + // NOLINTNEXTLINE(google-default-arguments) virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, diff --git a/libs/binder/include/binder/IResultReceiver.h b/libs/binder/include/binder/IResultReceiver.h index e494fba0b8..00b3d8954c 100644 --- a/libs/binder/include/binder/IResultReceiver.h +++ b/libs/binder/include/binder/IResultReceiver.h @@ -41,6 +41,7 @@ public: class BnResultReceiver : public BnInterface<IResultReceiver> { public: + // NOLINTNEXTLINE(google-default-arguments) virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, diff --git a/libs/binder/include/binder/IServiceManager.h b/libs/binder/include/binder/IServiceManager.h index a998529cd8..e5d8ea67de 100644 --- a/libs/binder/include/binder/IServiceManager.h +++ b/libs/binder/include/binder/IServiceManager.h @@ -61,6 +61,7 @@ public: /** * Register a service. */ + // NOLINTNEXTLINE(google-default-arguments) virtual status_t addService(const String16& name, const sp<IBinder>& service, bool allowIsolated = false, int dumpsysFlags = DUMP_FLAG_PRIORITY_DEFAULT) = 0; @@ -68,6 +69,7 @@ public: /** * Return list of all existing services. */ + // NOLINTNEXTLINE(google-default-arguments) virtual Vector<String16> listServices(int dumpsysFlags = DUMP_FLAG_PRIORITY_ALL) = 0; enum { diff --git a/libs/binder/include/binder/IShellCallback.h b/libs/binder/include/binder/IShellCallback.h index b47e995183..67156787d3 100644 --- a/libs/binder/include/binder/IShellCallback.h +++ b/libs/binder/include/binder/IShellCallback.h @@ -42,6 +42,7 @@ public: class BnShellCallback : public BnInterface<IShellCallback> { public: + // NOLINTNEXTLINE(google-default-arguments) virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, diff --git a/libs/binder/include/binder/IUidObserver.h b/libs/binder/include/binder/IUidObserver.h index d81789e399..9937ad6a29 100644 --- a/libs/binder/include/binder/IUidObserver.h +++ b/libs/binder/include/binder/IUidObserver.h @@ -47,6 +47,7 @@ public: class BnUidObserver : public BnInterface<IUidObserver> { public: + // NOLINTNEXTLINE(google-default-arguments) virtual status_t onTransact(uint32_t code, const Parcel& data, Parcel* reply, diff --git a/libs/binder/ndk/include_ndk/android/binder_auto_utils.h b/libs/binder/ndk/include_ndk/android/binder_auto_utils.h index cc0a29d18a..e52a1d69ae 100644 --- a/libs/binder/ndk/include_ndk/android/binder_auto_utils.h +++ b/libs/binder/ndk/include_ndk/android/binder_auto_utils.h @@ -31,6 +31,7 @@ #include <android/binder_status.h> #include <assert.h> +#include <unistd.h> #ifdef __cplusplus @@ -113,23 +114,23 @@ private: /** * This baseclass owns a single object, used to make various classes RAII. */ -template <typename T, void (*Destroy)(T*)> +template <typename T, typename R, R (*Destroy)(T), T DEFAULT> class ScopedAResource { public: /** * Takes ownership of t. */ - explicit ScopedAResource(T* t = nullptr) : mT(t) {} + explicit ScopedAResource(T t = DEFAULT) : mT(t) {} /** * This deletes the underlying object if it exists. See set. */ - ~ScopedAResource() { set(nullptr); } + ~ScopedAResource() { set(DEFAULT); } /** * Takes ownership of t. */ - void set(T* t) { + void set(T t) { Destroy(mT); mT = t; } @@ -137,12 +138,12 @@ public: /** * This returns the underlying object to be modified but does not affect ownership. */ - T* get() { return mT; } + T get() { return mT; } /** * This returns the const underlying object but does not affect ownership. */ - const T* get() const { return mT; } + const T get() const { return mT; } /** * This allows the value in this class to be set from beneath it. If you call this method and @@ -156,7 +157,7 @@ public: * Other usecases are discouraged. * */ - T** getR() { return &mT; } + T* getR() { return &mT; } // copy-constructing, or move/copy assignment is disallowed ScopedAResource(const ScopedAResource&) = delete; @@ -167,13 +168,13 @@ public: ScopedAResource(ScopedAResource&&) = default; private: - T* mT; + T mT; }; /** * Convenience wrapper. See AParcel. */ -class ScopedAParcel : public ScopedAResource<AParcel, AParcel_delete> { +class ScopedAParcel : public ScopedAResource<AParcel*, void, AParcel_delete, nullptr> { public: /** * Takes ownership of a. @@ -186,7 +187,7 @@ public: /** * Convenience wrapper. See AStatus. */ -class ScopedAStatus : public ScopedAResource<AStatus, AStatus_delete> { +class ScopedAStatus : public ScopedAResource<AStatus*, void, AStatus_delete, nullptr> { public: /** * Takes ownership of a. @@ -205,7 +206,8 @@ public: * Convenience wrapper. See AIBinder_DeathRecipient. */ class ScopedAIBinder_DeathRecipient - : public ScopedAResource<AIBinder_DeathRecipient, AIBinder_DeathRecipient_delete> { + : public ScopedAResource<AIBinder_DeathRecipient*, void, AIBinder_DeathRecipient_delete, + nullptr> { public: /** * Takes ownership of a. @@ -219,7 +221,8 @@ public: /** * Convenience wrapper. See AIBinder_Weak. */ -class ScopedAIBinder_Weak : public ScopedAResource<AIBinder_Weak, AIBinder_Weak_delete> { +class ScopedAIBinder_Weak + : public ScopedAResource<AIBinder_Weak*, void, AIBinder_Weak_delete, nullptr> { public: /** * Takes ownership of a. @@ -234,6 +237,19 @@ public: SpAIBinder promote() { return SpAIBinder(AIBinder_Weak_promote(get())); } }; +/** + * Convenience wrapper for a file descriptor. + */ +class ScopedFileDescriptor : public ScopedAResource<int, int, close, -1> { +public: + /** + * Takes ownership of a. + */ + explicit ScopedFileDescriptor(int a = -1) : ScopedAResource(a) {} + ~ScopedFileDescriptor() {} + ScopedFileDescriptor(ScopedFileDescriptor&&) = default; +}; + } // namespace ndk #endif // __cplusplus diff --git a/libs/binder/ndk/include_ndk/android/binder_parcel.h b/libs/binder/ndk/include_ndk/android/binder_parcel.h index 33c3f6cb4c..4e0132b536 100644 --- a/libs/binder/ndk/include_ndk/android/binder_parcel.h +++ b/libs/binder/ndk/include_ndk/android/binder_parcel.h @@ -193,6 +193,23 @@ binder_status_t AParcel_readNullableStrongBinder(const AParcel* parcel, AIBinder __INTRODUCED_IN(29); /** + * Writes a file descriptor to the next location in a non-null parcel. This does not take ownership + * of fd. + * + * This corresponds to the SDK's android.os.ParcelFileDescriptor. + */ +binder_status_t AParcel_writeParcelFileDescriptor(AParcel* parcel, int fd); + +/** + * Reads an int from the next location in a non-null parcel. + * + * The returned fd must be closed. + * + * This corresponds to the SDK's android.os.ParcelFileDescriptor. + */ +binder_status_t AParcel_readParcelFileDescriptor(const AParcel* parcel, int* fd); + +/** * Writes an AStatus object to the next location in a non-null parcel. * * If the status is considered to be a low-level status and has no additional information other diff --git a/libs/binder/ndk/libbinder_ndk.map.txt b/libs/binder/ndk/libbinder_ndk.map.txt index f84814fa3c..ec6587a7e4 100644 --- a/libs/binder/ndk/libbinder_ndk.map.txt +++ b/libs/binder/ndk/libbinder_ndk.map.txt @@ -38,6 +38,7 @@ LIBBINDER_NDK { # introduced=29 AParcel_readInt64; AParcel_readInt64Array; AParcel_readNullableStrongBinder; + AParcel_readParcelFileDescriptor; AParcel_readStatusHeader; AParcel_readString; AParcel_readStrongBinder; @@ -59,6 +60,7 @@ LIBBINDER_NDK { # introduced=29 AParcel_writeInt32Array; AParcel_writeInt64; AParcel_writeInt64Array; + AParcel_writeParcelFileDescriptor; AParcel_writeStatusHeader; AParcel_writeString; AParcel_writeStrongBinder; diff --git a/libs/binder/ndk/parcel.cpp b/libs/binder/ndk/parcel.cpp index 5ac3965b67..77c0558f58 100644 --- a/libs/binder/ndk/parcel.cpp +++ b/libs/binder/ndk/parcel.cpp @@ -23,13 +23,17 @@ #include <limits> #include <android-base/logging.h> +#include <android-base/unique_fd.h> #include <binder/Parcel.h> +#include <binder/ParcelFileDescriptor.h> #include <utils/Unicode.h> using ::android::IBinder; using ::android::Parcel; using ::android::sp; using ::android::status_t; +using ::android::base::unique_fd; +using ::android::os::ParcelFileDescriptor; template <typename T> using ContiguousArrayAllocator = T* (*)(void* arrayData, size_t length); @@ -210,6 +214,28 @@ binder_status_t AParcel_readNullableStrongBinder(const AParcel* parcel, AIBinder *binder = ret.get(); return PruneStatusT(status); } + +binder_status_t AParcel_writeParcelFileDescriptor(AParcel* parcel, int fd) { + ParcelFileDescriptor parcelFd((unique_fd(fd))); + + status_t status = parcel->get()->writeParcelable(parcelFd); + + // ownership is retained by caller + (void)parcelFd.release().release(); + + return PruneStatusT(status); +} + +binder_status_t AParcel_readParcelFileDescriptor(const AParcel* parcel, int* fd) { + ParcelFileDescriptor parcelFd; + // status_t status = parcelFd.readFromParcel(parcel->get()); + status_t status = parcel->get()->readParcelable(&parcelFd); + if (status != STATUS_OK) return PruneStatusT(status); + + *fd = parcelFd.release().release(); + return STATUS_OK; +} + binder_status_t AParcel_writeStatusHeader(AParcel* parcel, const AStatus* status) { return PruneStatusT(status->get()->writeToParcel(parcel->get())); } diff --git a/opengl/tests/EGLTest/Android.bp b/opengl/tests/EGLTest/Android.bp index f246077037..a9e873ac43 100644 --- a/opengl/tests/EGLTest/Android.bp +++ b/opengl/tests/EGLTest/Android.bp @@ -25,6 +25,7 @@ cc_test { "libhidltransport", "liblog", "libutils", + "libnativewindow", ], include_dirs: [ diff --git a/opengl/tests/EGLTest/EGL_test.cpp b/opengl/tests/EGLTest/EGL_test.cpp index 5927dc14e8..459b1356b8 100644 --- a/opengl/tests/EGLTest/EGL_test.cpp +++ b/opengl/tests/EGLTest/EGL_test.cpp @@ -775,4 +775,169 @@ TEST_F(EGLTest, EGLConfig1010102) { EXPECT_TRUE(eglDestroySurface(mEglDisplay, eglSurface)); } + +TEST_F(EGLTest, EGLInvalidColorspaceAttribute) { + EGLConfig config; + + ASSERT_NO_FATAL_FAILURE(get8BitConfig(config)); + + struct DummyConsumer : public BnConsumerListener { + void onFrameAvailable(const BufferItem& /* item */) override {} + void onBuffersReleased() override {} + void onSidebandStreamChanged() override {} + }; + + // Create a EGLSurface + sp<IGraphicBufferProducer> producer; + sp<IGraphicBufferConsumer> consumer; + BufferQueue::createBufferQueue(&producer, &consumer); + consumer->consumerConnect(new DummyConsumer, false); + sp<Surface> mSTC = new Surface(producer); + sp<ANativeWindow> mANW = mSTC; + + EGLint winAttrs[] = { + // clang-format off + EGL_GL_COLORSPACE_KHR, EGL_BACK_BUFFER, + EGL_NONE, + // clang-format on + }; + + EGLSurface eglSurface = eglCreateWindowSurface(mEglDisplay, config, mANW.get(), winAttrs); + ASSERT_EQ(EGL_BAD_ATTRIBUTE, eglGetError()); + ASSERT_EQ(EGL_NO_SURFACE, eglSurface); +} + +TEST_F(EGLTest, EGLUnsupportedColorspaceFormatCombo) { + EGLint numConfigs; + EGLConfig config; + EGLBoolean success; + + const EGLint attrs[] = { + // clang-format off + EGL_SURFACE_TYPE, EGL_WINDOW_BIT, + EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT, + EGL_RED_SIZE, 16, + EGL_GREEN_SIZE, 16, + EGL_BLUE_SIZE, 16, + EGL_ALPHA_SIZE, 16, + EGL_COLOR_COMPONENT_TYPE_EXT, EGL_COLOR_COMPONENT_TYPE_FLOAT_EXT, + EGL_NONE, + // clang-format on + }; + success = eglChooseConfig(mEglDisplay, attrs, &config, 1, &numConfigs); + ASSERT_EQ(EGL_UNSIGNED_TRUE, success); + ASSERT_EQ(1, numConfigs); + + struct DummyConsumer : public BnConsumerListener { + void onFrameAvailable(const BufferItem& /* item */) override {} + void onBuffersReleased() override {} + void onSidebandStreamChanged() override {} + }; + + // Create a EGLSurface + sp<IGraphicBufferProducer> producer; + sp<IGraphicBufferConsumer> consumer; + BufferQueue::createBufferQueue(&producer, &consumer); + consumer->consumerConnect(new DummyConsumer, false); + sp<Surface> mSTC = new Surface(producer); + sp<ANativeWindow> mANW = mSTC; + + const EGLint winAttrs[] = { + // clang-format off + EGL_GL_COLORSPACE_KHR, EGL_GL_COLORSPACE_DISPLAY_P3_EXT, + EGL_NONE, + // clang-format on + }; + + EGLSurface eglSurface = eglCreateWindowSurface(mEglDisplay, config, mANW.get(), winAttrs); + ASSERT_EQ(EGL_BAD_MATCH, eglGetError()); + ASSERT_EQ(EGL_NO_SURFACE, eglSurface); +} + +TEST_F(EGLTest, EGLCreateWindowFailAndSucceed) { + EGLConfig config; + + ASSERT_NO_FATAL_FAILURE(get8BitConfig(config)); + + struct DummyConsumer : public BnConsumerListener { + void onFrameAvailable(const BufferItem& /* item */) override {} + void onBuffersReleased() override {} + void onSidebandStreamChanged() override {} + }; + + // Create a EGLSurface + sp<IGraphicBufferProducer> producer; + sp<IGraphicBufferConsumer> consumer; + BufferQueue::createBufferQueue(&producer, &consumer); + consumer->consumerConnect(new DummyConsumer, false); + sp<Surface> mSTC = new Surface(producer); + sp<ANativeWindow> mANW = mSTC; + + EGLint winAttrs[] = { + // clang-format off + EGL_GL_COLORSPACE_KHR, EGL_BACK_BUFFER, + EGL_NONE, + // clang-format on + }; + + EGLSurface eglSurface = eglCreateWindowSurface(mEglDisplay, config, mANW.get(), winAttrs); + ASSERT_EQ(EGL_BAD_ATTRIBUTE, eglGetError()); + ASSERT_EQ(EGL_NO_SURFACE, eglSurface); + + // Now recreate surface with a valid colorspace. Ensure proper cleanup is done + // in the first failed attempt (e.g. native_window_api_disconnect). + winAttrs[1] = EGL_GL_COLORSPACE_LINEAR_KHR; + eglSurface = eglCreateWindowSurface(mEglDisplay, config, mANW.get(), winAttrs); + ASSERT_EQ(EGL_SUCCESS, eglGetError()); + ASSERT_NE(EGL_NO_SURFACE, eglSurface); + + EXPECT_TRUE(eglDestroySurface(mEglDisplay, eglSurface)); +} + +TEST_F(EGLTest, EGLCreateWindowTwoColorspaces) { + EGLConfig config; + + ASSERT_NO_FATAL_FAILURE(get8BitConfig(config)); + + struct DummyConsumer : public BnConsumerListener { + void onFrameAvailable(const BufferItem& /* item */) override {} + void onBuffersReleased() override {} + void onSidebandStreamChanged() override {} + }; + + // Create a EGLSurface + sp<IGraphicBufferProducer> producer; + sp<IGraphicBufferConsumer> consumer; + BufferQueue::createBufferQueue(&producer, &consumer); + consumer->consumerConnect(new DummyConsumer, false); + sp<Surface> mSTC = new Surface(producer); + sp<ANativeWindow> mANW = mSTC; + + const EGLint winAttrs[] = { + // clang-format off + EGL_GL_COLORSPACE_KHR, EGL_GL_COLORSPACE_DISPLAY_P3_EXT, + EGL_NONE, + // clang-format on + }; + + EGLSurface eglSurface = eglCreateWindowSurface(mEglDisplay, config, mANW.get(), winAttrs); + ASSERT_EQ(EGL_SUCCESS, eglGetError()); + ASSERT_NE(EGL_NO_SURFACE, eglSurface); + + android_dataspace dataspace = static_cast<android_dataspace>(ANativeWindow_getBuffersDataSpace(mANW.get())); + ASSERT_EQ(dataspace, HAL_DATASPACE_DISPLAY_P3); + + EXPECT_TRUE(eglDestroySurface(mEglDisplay, eglSurface)); + + // Now create with default attribute (EGL_GL_COLORSPACE_LINEAR_KHR) + eglSurface = eglCreateWindowSurface(mEglDisplay, config, mANW.get(), NULL); + ASSERT_EQ(EGL_SUCCESS, eglGetError()); + ASSERT_NE(EGL_NO_SURFACE, eglSurface); + + dataspace = static_cast<android_dataspace>(ANativeWindow_getBuffersDataSpace(mANW.get())); + // Make sure the dataspace has been reset to UNKNOWN + ASSERT_NE(dataspace, HAL_DATASPACE_DISPLAY_P3); + + EXPECT_TRUE(eglDestroySurface(mEglDisplay, eglSurface)); +} } |