From 6b9516ced325e93880e9c8ef2793380866a1c579 Mon Sep 17 00:00:00 2001 From: Narayan Kamath Date: Fri, 27 Oct 2017 11:15:51 +0100 Subject: dumpstate: always include last ANR as a separate entry. For consistency, as well as to make tooling easier. Most of this change is an extended yak-shave to try and close DumpData FDs in a consistent manner. Several layers of abstraction were assuming that it was their job to close input FDs, leading to double closes in some cases. These double closes are benign prior to this change to dump a given DumpData twice, given that dumpstate is single threaded. Bug: 68188758 Test: adb bugreport Change-Id: I6302f4cb553139c0c26defb4859c4eca5b18ec93 --- cmds/dumpstate/DumpstateInternal.cpp | 1 - cmds/dumpstate/DumpstateUtil.cpp | 7 +++--- cmds/dumpstate/dumpstate.cpp | 45 +++++++++++++++++++++++++----------- cmds/dumpstate/utils.cpp | 12 ++++------ 4 files changed, 41 insertions(+), 24 deletions(-) diff --git a/cmds/dumpstate/DumpstateInternal.cpp b/cmds/dumpstate/DumpstateInternal.cpp index 5149b7fc4f..83e30a22ff 100644 --- a/cmds/dumpstate/DumpstateInternal.cpp +++ b/cmds/dumpstate/DumpstateInternal.cpp @@ -176,7 +176,6 @@ int DumpFileFromFdToFd(const std::string& title, const std::string& path_string, } } } - close(fd); if (!newline) dprintf(out_fd, "\n"); if (!title.empty()) dprintf(out_fd, "\n"); diff --git a/cmds/dumpstate/DumpstateUtil.cpp b/cmds/dumpstate/DumpstateUtil.cpp index ea7109bacd..ede4254a9b 100644 --- a/cmds/dumpstate/DumpstateUtil.cpp +++ b/cmds/dumpstate/DumpstateUtil.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include "DumpstateInternal.h" @@ -182,8 +183,8 @@ bool PropertiesHelper::IsDryRun() { } int DumpFileToFd(int out_fd, const std::string& title, const std::string& path) { - int fd = TEMP_FAILURE_RETRY(open(path.c_str(), O_RDONLY | O_NONBLOCK | O_CLOEXEC)); - if (fd < 0) { + android::base::unique_fd fd(TEMP_FAILURE_RETRY(open(path.c_str(), O_RDONLY | O_NONBLOCK | O_CLOEXEC))); + if (fd.get() < 0) { int err = errno; if (title.empty()) { dprintf(out_fd, "*** Error dumping %s: %s\n", path.c_str(), strerror(err)); @@ -194,7 +195,7 @@ int DumpFileToFd(int out_fd, const std::string& title, const std::string& path) fsync(out_fd); return -1; } - return DumpFileFromFdToFd(title, path, fd, out_fd, PropertiesHelper::IsDryRun()); + return DumpFileFromFdToFd(title, path, fd.get(), out_fd, PropertiesHelper::IsDryRun()); } int RunCommandToFd(int fd, const std::string& title, const std::vector& full_command, diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index b1a03fa0f0..66536035d3 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -207,20 +207,36 @@ static bool AddDumps(const std::vector::const_iterator start, const std::string& name = it->name; const int fd = it->fd; dumped = true; + + // Seek to the beginning of the file before dumping any data. A given + // DumpData entry might be dumped multiple times in the report. + // + // For example, the most recent ANR entry is dumped to the body of the + // main entry and it also shows up as a separate entry in the bugreport + // ZIP file. + if (lseek(fd, 0, SEEK_SET) != static_cast(0)) { + MYLOGE("Unable to add %s to zip file, lseek failed: %s\n", name.c_str(), + strerror(errno)); + } + if (ds.IsZipping() && add_to_zip) { if (!ds.AddZipEntryFromFd(ZIP_ROOT_DIR + name, fd)) { - MYLOGE("Unable to add %s %s to zip file\n", name.c_str(), type_name); + MYLOGE("Unable to add %s to zip file, addZipEntryFromFd failed\n", name.c_str()); } } else { dump_file_from_fd(type_name, name.c_str(), fd); } - - close(fd); } return dumped; } +static void CloseDumpFds(const std::vector* dumps) { + for (auto it = dumps->begin(); it != dumps->end(); ++it) { + close(it->fd); + } +} + // for_each_pid() callback to get mount info about a process. void do_mountinfo(int pid, const char* name __attribute__((unused))) { char path[PATH_MAX]; @@ -887,9 +903,9 @@ static void AddGlobalAnrTraceFile(const bool add_to_zip, const std::string& anr_ MYLOGD("AddGlobalAnrTraceFile(): dump_traces_dir=%s, anr_traces_dir=%s, already_dumped=%d\n", dump_traces_dir.c_str(), anr_traces_dir.c_str(), already_dumped); - int fd = TEMP_FAILURE_RETRY( - open(anr_traces_file.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK)); - if (fd < 0) { + android::base::unique_fd fd(TEMP_FAILURE_RETRY( + open(anr_traces_file.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK))); + if (fd.get() < 0) { printf("*** NO ANR VM TRACES FILE (%s): %s\n\n", anr_traces_file.c_str(), strerror(errno)); } else { if (add_to_zip) { @@ -901,7 +917,7 @@ static void AddGlobalAnrTraceFile(const bool add_to_zip, const std::string& anr_ } else { MYLOGD("Dumping last ANR traces (%s) to the main bugreport entry\n", anr_traces_file.c_str()); - dump_file_from_fd("VM TRACES AT LAST ANR", anr_traces_file.c_str(), fd); + dump_file_from_fd("VM TRACES AT LAST ANR", anr_traces_file.c_str(), fd.get()); } } } @@ -934,12 +950,12 @@ static void AddAnrTraceDir(const bool add_to_zip, const std::string& anr_traces_ AddDumps(anr_data->begin(), anr_data->begin() + 1, "VM TRACES AT LAST ANR", add_to_zip); - if (anr_data->size() > 1) { - // NOTE: Historical ANRs are always added as separate entries in the - // bugreport zip file. - AddDumps(anr_data->begin() + 1, anr_data->end(), - "HISTORICAL ANR", true /* add_to_zip */); - } + // The "last" ANR will always be included as separate entry in the zip file. In addition, + // it will be present in the body of the main entry if |add_to_zip| == false. + // + // Historical ANRs are always included as separate entries in the bugreport zip file. + AddDumps(anr_data->begin() + ((add_to_zip) ? 1 : 0), anr_data->end(), + "HISTORICAL ANR", true /* add_to_zip */); } else { printf("*** NO ANRs to dump in %s\n\n", ANR_DIR.c_str()); } @@ -2033,5 +2049,8 @@ int main(int argc, char *argv[]) { close(ds.control_socket_fd_); } + CloseDumpFds(tombstone_data.get()); + CloseDumpFds(anr_data.get()); + return 0; } diff --git a/cmds/dumpstate/utils.cpp b/cmds/dumpstate/utils.cpp index c2c9071812..6ff0dae01c 100644 --- a/cmds/dumpstate/utils.cpp +++ b/cmds/dumpstate/utils.cpp @@ -632,7 +632,7 @@ int dump_files(const std::string& title, const char* dir, bool (*skip)(const cha struct dirent *d; char *newpath = NULL; const char *slash = "/"; - int fd, retval = 0; + int retval = 0; if (!title.empty()) { printf("------ %s (%s) ------\n", title.c_str(), dir); @@ -674,13 +674,13 @@ int dump_files(const std::string& title, const char* dir, bool (*skip)(const cha } continue; } - fd = TEMP_FAILURE_RETRY(open(newpath, O_RDONLY | O_NONBLOCK | O_CLOEXEC)); - if (fd < 0) { - retval = fd; + android::base::unique_fd fd(TEMP_FAILURE_RETRY(open(newpath, O_RDONLY | O_NONBLOCK | O_CLOEXEC))); + if (fd.get() < 0) { + retval = -1; printf("*** %s: %s\n", newpath, strerror(errno)); continue; } - (*dump_from_fd)(NULL, newpath, fd); + (*dump_from_fd)(NULL, newpath, fd.get()); } closedir(dirp); if (!title.empty()) { @@ -699,11 +699,9 @@ int dump_file_from_fd(const char *title, const char *path, int fd) { int flags = fcntl(fd, F_GETFL); if (flags == -1) { printf("*** %s: failed to get flags on fd %d: %s\n", path, fd, strerror(errno)); - close(fd); return -1; } else if (!(flags & O_NONBLOCK)) { printf("*** %s: fd must have O_NONBLOCK set.\n", path); - close(fd); return -1; } return DumpFileFromFdToFd(title, path, fd, STDOUT_FILENO, PropertiesHelper::IsDryRun()); -- cgit v1.2.3-59-g8ed1b