Merge "Remove outfile option from dumpstate" (4bcfa50c) · Commits · e / os / platform_frameworks_native

cmds/dumpstate/DumpstateService.cpp

+4 −2

Original line number	Diff line number	Diff line
		@@ -144,6 +144,9 @@ binder::Status DumpstateService::startBugreport(int32_t /* calling_uid */,
		}

		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 @@ status_t DumpstateService::dump(int fd, const Vector<String16>&) {
		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());

cmds/dumpstate/dumpstate.cpp

+20 −49

Original line number	Diff line number	Diff line
		@@ -135,10 +135,6 @@ static int Open(std::string path, int flags, mode_t mode = 0) {
		return fd;
		}

		static int OpenForWrite(std::string path) {
		return Open(path, O_WRONLY \| O_CREAT \| O_TRUNC \| O_CLOEXEC \| O_NOFOLLOW,
		S_IRUSR \| S_IWUSR \| S_IRGRP \| S_IROTH);
		}

		static int OpenForRead(std::string path) {
		return Open(path, O_RDONLY \| O_CLOEXEC \| O_NOFOLLOW);
		@@ -169,17 +165,6 @@ static bool CopyFileToFd(const std::string& input_file, int out_fd) {
		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
		@@ -1800,16 +1785,9 @@ static void MaybeResolveSymlink(std::string* path) {
		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_));
		@@ -1832,17 +1810,16 @@ static void PrepareToWriteToFile() {

		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");
		@@ -1909,18 +1886,13 @@ static void FinalizeFile() {
		}
		}
		// 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) {
		@@ -1946,8 +1918,8 @@ static void FinalizeFile() {
		/* 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 = {
		@@ -1955,7 +1927,7 @@ static void SendBugreportFinishedBroadcast() {
		"--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
		@@ -1977,7 +1949,7 @@ static void SendBugreportFinishedBroadcast() {
		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);
		@@ -2115,7 +2087,6 @@ 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("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());
		@@ -2146,7 +2117,9 @@ Dumpstate::RunStatus Dumpstate::DumpOptions::Initialize(int argc, char* argv[])
		// clang-format off
		case 'd': do_add_date = true; break;
		case 'z': do_zip_file = true; break;
		case 'o': use_outfile = optarg; break;
		// 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;
		@@ -2187,9 +2160,7 @@ bool Dumpstate::DumpOptions::ValidateOptions() const {
		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;
		}

		@@ -2251,8 +2222,8 @@ Dumpstate::RunStatus Dumpstate::Run() {
		* 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_);
		@@ -2293,7 +2264,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal() {
		}

		// 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 =
		@@ -2444,7 +2415,7 @@ Dumpstate::RunStatus Dumpstate::RunInternal() {
		}

		/* rename or zip the (now complete) .tmp file to its final location */
		if (!options_->use_outfile.empty()) {
		if (options_->OutputToFile()) {
		FinalizeFile();
		}

cmds/dumpstate/dumpstate.h

+9 −10

Original line number	Diff line number	Diff line
		@@ -331,6 +331,7 @@ class Dumpstate {
		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 @@ class Dumpstate {
		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 @@ class Dumpstate {

		/* 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 @@ class Dumpstate {
		// 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};

cmds/dumpstate/tests/dumpstate_test.cpp

+8 −26

Original line number	Diff line number	Diff line
		@@ -173,7 +173,6 @@ TEST_F(DumpOptionsTest, InitializeNone) {

		EXPECT_FALSE(options_.do_add_date);
		EXPECT_FALSE(options_.do_zip_file);
		EXPECT_EQ("", options_.use_outfile);
		EXPECT_FALSE(options_.use_socket);
		EXPECT_FALSE(options_.use_control_socket);
		EXPECT_FALSE(options_.show_header_only);
		@@ -191,7 +190,6 @@ TEST_F(DumpOptionsTest, InitializeAdbBugreport) {
		const_cast<char*>("-S"),
		const_cast<char*>("-d"),
		const_cast<char*>("-z"),
		const_cast<char*>("-o abc"),
		};
		// clang-format on

		@@ -201,7 +199,6 @@ TEST_F(DumpOptionsTest, InitializeAdbBugreport) {
		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 @@ TEST_F(DumpOptionsTest, InitializeAdbShellBugreport) {

		// 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 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) {
		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 @@ TEST_F(DumpOptionsTest, InitializeFullBugReport) {
		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 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) {
		const_cast<char*>("-p"),
		const_cast<char*>("-B"),
		const_cast<char*>("-z"),
		const_cast<char*>("-o abc"),
		};
		// clang-format on

		@@ -294,7 +287,6 @@ TEST_F(DumpOptionsTest, InitializeInteractiveBugReport) {
		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 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) {
		const_cast<char*>("-p"),
		const_cast<char*>("-B"),
		const_cast<char*>("-z"),
		const_cast<char*>("-o abc"),
		};
		// clang-format on

		@@ -327,7 +318,6 @@ TEST_F(DumpOptionsTest, InitializeRemoteBugReport) {
		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 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) {
		const_cast<char*>("-p"),
		const_cast<char*>("-B"),
		const_cast<char*>("-z"),
		const_cast<char*>("-o abc"),
		};
		// clang-format on

		@@ -359,7 +348,6 @@ TEST_F(DumpOptionsTest, InitializeWearBugReport) {
		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 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) {
		const_cast<char*>("-p"),
		const_cast<char*>("-B"),
		const_cast<char*>("-z"),
		const_cast<char*>("-o abc"),
		};
		// clang-format on

		@@ -391,7 +378,6 @@ TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) {
		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 @@ TEST_F(DumpOptionsTest, InitializeWifiBugReport) {
		const_cast<char*>("-p"),
		const_cast<char*>("-B"),
		const_cast<char*>("-z"),
		const_cast<char*>("-o abc"),
		};
		// clang-format on

		@@ -424,7 +409,6 @@ TEST_F(DumpOptionsTest, InitializeWifiBugReport) {
		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 @@ TEST_F(DumpOptionsTest, InitializeDefaultBugReport) {
		const_cast<char*>("-p"),
		const_cast<char*>("-B"),
		const_cast<char*>("-z"),
		const_cast<char*>("-o abc"),
		};
		// clang-format on

		@@ -457,7 +440,6 @@ TEST_F(DumpOptionsTest, InitializeDefaultBugReport) {
		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 @@ TEST_F(DumpOptionsTest, InitializePartial1) {
		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 @@ TEST_F(DumpOptionsTest, InitializePartial1) {
		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 @@ TEST_F(DumpOptionsTest, InitializePartial2) {
		// 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, InitializeUnknown) {

		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 @@ TEST_F(DumpOptionsTest, ValidateOptionsNeedZipfile) {
		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 @@ TEST_F(DumpOptionsTest, ValidateOptionsRemoteMode) {
		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());
		}