Merge "Remove outfile option from dumpstate" am: 4bcfa50ce9 am: 12cefbc507
am: a6366c9fe0
Change-Id: I1a850ab2a20a25e1777670dc06c9c7c9841cfa17
diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp
index 1d5b738..ba9a967 100644
--- a/cmds/dumpstate/DumpstateService.cpp
+++ b/cmds/dumpstate/DumpstateService.cpp
@@ -144,6 +144,9 @@
}
status_t DumpstateService::dump(int fd, const Vector<String16>&) {
+ std::string destination = ds_.options_->bugreport_fd.get() != -1
+ ? StringPrintf("[fd:%d]", ds_.options_->bugreport_fd.get())
+ : ds_.bugreport_internal_dir_.c_str();
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");
@@ -154,8 +157,7 @@
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, "bugreport_internal_dir_: %s\n", ds_.bugreport_internal_dir_.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());
diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp
index 8c8cd51..4634ecd 100644
--- a/cmds/dumpstate/dumpstate.cpp
+++ b/cmds/dumpstate/dumpstate.cpp
@@ -135,10 +135,6 @@
return fd;
}
-static int OpenForWrite(std::string path) {
- return Open(path, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW,
- S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
-}
static int OpenForRead(std::string path) {
return Open(path, O_RDONLY | O_CLOEXEC | O_NOFOLLOW);
@@ -169,17 +165,6 @@
return false;
}
-static bool CopyFileToFile(const std::string& input_file, const std::string& output_file) {
- if (input_file == output_file) {
- MYLOGD("Skipping copying bugreport file since the destination is the same (%s)\n",
- output_file.c_str());
- return false;
- }
-
- MYLOGD("Going to copy bugreport file (%s) to %s\n", input_file.c_str(), output_file.c_str());
- android::base::unique_fd out_fd(OpenForWrite(output_file));
- return CopyFileToFd(input_file, out_fd.get());
-}
} // namespace
} // namespace os
@@ -1806,16 +1791,9 @@
static void PrepareToWriteToFile() {
MaybeResolveSymlink(&ds.bugreport_internal_dir_);
- std::string base_name_part1 = "bugreport";
- if (!ds.options_->use_outfile.empty()) {
- ds.bugreport_dir_ = dirname(ds.options_->use_outfile.c_str());
- base_name_part1 = basename(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_ =
- StringPrintf("%s-%s-%s", base_name_part1.c_str(), device_name.c_str(), build_id.c_str());
+ ds.base_name_ = StringPrintf("bugreport-%s-%s", 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_));
@@ -1838,17 +1816,16 @@
std::string destination = ds.options_->bugreport_fd.get() != -1
? StringPrintf("[fd:%d]", ds.options_->bugreport_fd.get())
- : ds.bugreport_dir_.c_str();
+ : ds.bugreport_internal_dir_.c_str();
MYLOGD(
"Bugreport dir: %s\n"
- "Internal Bugreport dir: %s\n"
"Base name: %s\n"
"Suffix: %s\n"
"Log path: %s\n"
"Temporary path: %s\n"
"Screenshot path: %s\n",
- destination.c_str(), ds.bugreport_internal_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());
+ destination.c_str(), ds.base_name_.c_str(), ds.name_.c_str(), ds.log_path_.c_str(),
+ ds.tmp_path_.c_str(), ds.screenshot_path_.c_str());
if (ds.options_->do_zip_file) {
ds.path_ = ds.GetPath(".zip");
@@ -1915,18 +1892,13 @@
}
}
// The zip file lives in an internal directory. Copy it over to output.
- bool copy_succeeded = false;
if (ds.options_->bugreport_fd.get() != -1) {
- copy_succeeded = android::os::CopyFileToFd(ds.path_, ds.options_->bugreport_fd.get());
- } else {
- ds.final_path_ = ds.GetPath(ds.bugreport_dir_, ".zip");
- copy_succeeded = android::os::CopyFileToFile(ds.path_, ds.final_path_);
- }
- if (copy_succeeded) {
- if (remove(ds.path_.c_str())) {
+ bool copy_succeeded =
+ android::os::CopyFileToFd(ds.path_, ds.options_->bugreport_fd.get());
+ if (!copy_succeeded && remove(ds.path_.c_str())) {
MYLOGE("remove(%s): %s", ds.path_.c_str(), strerror(errno));
}
- }
+ } // else - the file just remains in the internal directory.
}
}
if (do_text_file) {
@@ -1952,8 +1924,8 @@
/* Broadcasts that we are done with the bugreport */
static void SendBugreportFinishedBroadcast() {
// TODO(b/111441001): use callback instead of broadcast.
- if (!ds.final_path_.empty()) {
- MYLOGI("Final bugreport path: %s\n", ds.final_path_.c_str());
+ if (!ds.path_.empty()) {
+ MYLOGI("Final bugreport path: %s\n", ds.path_.c_str());
// clang-format off
std::vector<std::string> am_args = {
@@ -1961,7 +1933,7 @@
"--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.final_path_,
+ "--es", "android.intent.extra.BUGREPORT", ds.path_,
"--es", "android.intent.extra.DUMPSTATE_LOG", ds.log_path_
};
// clang-format on
@@ -1983,7 +1955,7 @@
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.final_path_));
+ 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);
@@ -2121,7 +2093,6 @@
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("use_outfile: %s\n", options.use_outfile.c_str());
MYLOGI("extra_options: %s\n", options.extra_options.c_str());
MYLOGI("args: %s\n", options.args.c_str());
MYLOGI("notification_title: %s\n", options.notification_title.c_str());
@@ -2152,7 +2123,9 @@
// clang-format off
case 'd': do_add_date = true; break;
case 'z': do_zip_file = true; break;
- case 'o': use_outfile = optarg; break;
+ // o=use_outfile not supported anymore.
+ // TODO(b/111441001): Remove when all callers have migrated.
+ case 'o': break;
case 's': use_socket = true; break;
case 'S': use_control_socket = true; break;
case 'v': show_header_only = true; break;
@@ -2193,9 +2166,7 @@
return false;
}
- bool has_out_file_options = !use_outfile.empty() || bugreport_fd.get() != -1;
- if ((do_zip_file || do_add_date || do_progress_updates || do_broadcast) &&
- !has_out_file_options) {
+ if ((do_zip_file || do_add_date || do_progress_updates || do_broadcast) && !OutputToFile()) {
return false;
}
@@ -2257,8 +2228,8 @@
* If zipping, a bunch of other files and dumps also get added to the zip archive. The log file also
* gets added to the archive.
*
- * Bugreports are first generated in a local directory and later copied to the caller's fd or
- * directory.
+ * Bugreports are first generated in a local directory and later copied to the caller's fd if
+ * supplied.
*/
Dumpstate::RunStatus Dumpstate::RunInternal() {
LogDumpOptions(*options_);
@@ -2299,7 +2270,7 @@
}
// Redirect output if needed
- bool is_redirecting = !options_->use_socket && !options_->use_outfile.empty();
+ bool is_redirecting = options_->OutputToFile();
// TODO: temporarily set progress until it's part of the Dumpstate constructor
std::string stats_path =
@@ -2450,7 +2421,7 @@
}
/* rename or zip the (now complete) .tmp file to its final location */
- if (!options_->use_outfile.empty()) {
+ if (options_->OutputToFile()) {
FinalizeFile();
}
diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h
index c620c07..3dfe4e9 100644
--- a/cmds/dumpstate/dumpstate.h
+++ b/cmds/dumpstate/dumpstate.h
@@ -331,6 +331,7 @@
bool do_add_date = false;
bool do_zip_file = false;
bool do_vibrate = true;
+ // Writes bugreport content to a socket; only flatfile format is supported.
bool use_socket = false;
bool use_control_socket = false;
bool do_fb = false;
@@ -342,13 +343,11 @@
bool wifi_only = false;
// Whether progress updates should be published.
bool do_progress_updates = false;
- // File descriptor to output zip file. Takes precedence over use_outfile.
+ // File descriptor to output zip file.
android::base::unique_fd bugreport_fd;
// File descriptor to screenshot file.
// TODO(b/111441001): Use this fd.
android::base::unique_fd screenshot_fd;
- // Partial path to output file.
- std::string use_outfile;
// TODO: rename to MODE.
// Extra options passed as system property.
std::string extra_options;
@@ -367,6 +366,13 @@
/* Returns true if the options set so far are consistent. */
bool ValidateOptions() const;
+
+ /* Returns if options specified require writing bugreport to a file */
+ bool OutputToFile() const {
+ // If we are not writing to socket, we will write to a file. If bugreport_fd is
+ // specified, it is preferred. If not bugreport is written to /bugreports.
+ return !use_socket;
+ }
};
// TODO: initialize fields on constructor
@@ -424,13 +430,6 @@
// Full path of the temporary file containing the screenshot (when requested).
std::string screenshot_path_;
- // TODO(b/111441001): remove when obsolete.
- // Full path of the final zip file inside the caller-specified directory, if available.
- std::string final_path_;
-
- // The caller-specified directory, if available.
- std::string bugreport_dir_;
-
// Pointer to the zipped file.
std::unique_ptr<FILE, int (*)(FILE*)> zip_file{nullptr, fclose};
diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp
index cd9b97f..0302c46 100644
--- a/cmds/dumpstate/tests/dumpstate_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_test.cpp
@@ -173,7 +173,6 @@
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);
@@ -191,7 +190,6 @@
const_cast<char*>("-S"),
const_cast<char*>("-d"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -201,7 +199,6 @@
EXPECT_TRUE(options_.do_add_date);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.use_control_socket);
- EXPECT_EQ(" abc", std::string(options_.use_outfile));
// Other options retain default values
EXPECT_TRUE(options_.do_vibrate);
@@ -228,7 +225,6 @@
// Other options retain default values
EXPECT_TRUE(options_.do_vibrate);
- EXPECT_EQ("", options_.use_outfile);
EXPECT_FALSE(options_.do_add_date);
EXPECT_FALSE(options_.do_zip_file);
EXPECT_FALSE(options_.use_control_socket);
@@ -247,7 +243,6 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
property_set("dumpstate.options", "bugreportfull");
@@ -259,7 +254,6 @@
EXPECT_TRUE(options_.do_fb);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.do_broadcast);
- EXPECT_EQ(" abc", std::string(options_.use_outfile));
// Other options retain default values
EXPECT_TRUE(options_.do_vibrate);
@@ -279,7 +273,6 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -294,7 +287,6 @@
EXPECT_TRUE(options_.do_progress_updates);
EXPECT_TRUE(options_.do_start_service);
EXPECT_FALSE(options_.do_fb);
- EXPECT_EQ(" abc", std::string(options_.use_outfile));
// Other options retain default values
EXPECT_TRUE(options_.do_vibrate);
@@ -312,7 +304,6 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -327,7 +318,6 @@
EXPECT_TRUE(options_.is_remote_mode);
EXPECT_FALSE(options_.do_vibrate);
EXPECT_FALSE(options_.do_fb);
- EXPECT_EQ(" abc", std::string(options_.use_outfile));
// Other options retain default values
EXPECT_FALSE(options_.use_control_socket);
@@ -344,7 +334,6 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -359,7 +348,6 @@
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.do_progress_updates);
EXPECT_TRUE(options_.do_start_service);
- EXPECT_EQ(" abc", std::string(options_.use_outfile));
// Other options retain default values
EXPECT_TRUE(options_.do_vibrate);
@@ -377,7 +365,6 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -391,7 +378,6 @@
EXPECT_TRUE(options_.do_broadcast);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.telephony_only);
- EXPECT_EQ(" abc", std::string(options_.use_outfile));
// Other options retain default values
EXPECT_TRUE(options_.do_vibrate);
@@ -410,7 +396,6 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -424,7 +409,6 @@
EXPECT_TRUE(options_.do_broadcast);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.wifi_only);
- EXPECT_EQ(" abc", std::string(options_.use_outfile));
// Other options retain default values
EXPECT_TRUE(options_.do_vibrate);
@@ -444,7 +428,6 @@
const_cast<char*>("-p"),
const_cast<char*>("-B"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
};
// clang-format on
@@ -457,7 +440,6 @@
EXPECT_TRUE(options_.do_fb);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.do_broadcast);
- EXPECT_EQ(" abc", std::string(options_.use_outfile));
// Other options retain default values
EXPECT_TRUE(options_.do_vibrate);
@@ -475,7 +457,6 @@
const_cast<char*>("dumpstate"),
const_cast<char*>("-d"),
const_cast<char*>("-z"),
- const_cast<char*>("-o abc"),
const_cast<char*>("-s"),
const_cast<char*>("-S"),
@@ -488,7 +469,6 @@
EXPECT_TRUE(options_.do_add_date);
EXPECT_TRUE(options_.do_zip_file);
// TODO: Maybe we should trim the filename
- EXPECT_EQ(" abc", std::string(options_.use_outfile));
EXPECT_TRUE(options_.use_socket);
EXPECT_TRUE(options_.use_control_socket);
@@ -527,7 +507,6 @@
// Other options retain default values
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);
}
@@ -562,15 +541,21 @@
TEST_F(DumpOptionsTest, ValidateOptionsNeedOutfile1) {
options_.do_zip_file = true;
+ // Writing to socket = !writing to file.
+ options_.use_socket = true;
EXPECT_FALSE(options_.ValidateOptions());
- options_.use_outfile = "a/b/c";
+
+ options_.use_socket = false;
EXPECT_TRUE(options_.ValidateOptions());
}
TEST_F(DumpOptionsTest, ValidateOptionsNeedOutfile2) {
options_.do_broadcast = true;
+ // Writing to socket = !writing to file.
+ options_.use_socket = true;
EXPECT_FALSE(options_.ValidateOptions());
- options_.use_outfile = "a/b/c";
+
+ options_.use_socket = false;
EXPECT_TRUE(options_.ValidateOptions());
}
@@ -579,13 +564,11 @@
EXPECT_FALSE(options_.ValidateOptions());
options_.do_zip_file = true;
- options_.use_outfile = "a/b/c"; // do_zip_file needs outfile
EXPECT_TRUE(options_.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());
options_.do_broadcast = true;
@@ -599,7 +582,6 @@
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());
}