[feedback] Clean up store.*
This clean up started in another CL and I decided to split it into its
own change.
TESTED=`fx test crash-reports-tests`
Change-Id: I7f04c91897910da352065122a291063754132471
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/405001
Commit-Queue: Alex Pankhurst <pankhurst@google.com>
Testability-Review: Alex Pankhurst <pankhurst@google.com>
Testability-Review: Francois Rousseau <frousseau@google.com>
Reviewed-by: Francois Rousseau <frousseau@google.com>
diff --git a/src/developer/forensics/crash_reports/BUILD.gn b/src/developer/forensics/crash_reports/BUILD.gn
index db5986e..ec21e6b 100644
--- a/src/developer/forensics/crash_reports/BUILD.gn
+++ b/src/developer/forensics/crash_reports/BUILD.gn
@@ -355,6 +355,7 @@
deps = [
"//sdk/lib/syslog/cpp",
"//src/lib/files",
+ "//src/lib/fxl",
"//third_party/rapidjson",
"//zircon/public/lib/fit",
]
diff --git a/src/developer/forensics/crash_reports/store.cc b/src/developer/forensics/crash_reports/store.cc
index 5bd28124..804a45a 100644
--- a/src/developer/forensics/crash_reports/store.cc
+++ b/src/developer/forensics/crash_reports/store.cc
@@ -14,6 +14,7 @@
#include "src/lib/files/directory.h"
#include "src/lib/files/file.h"
#include "src/lib/files/path.h"
+#include "src/lib/fxl/strings/join_strings.h"
#include "third_party/rapidjson/include/rapidjson/document.h"
#include "third_party/rapidjson/include/rapidjson/prettywriter.h"
#include "third_party/rapidjson/include/rapidjson/stringbuffer.h"
@@ -30,6 +31,23 @@
kMinidumpFilename,
};
+// Join |paths| in order into a single path.
+std::string JoinPaths(const std::vector<std::string>& paths) {
+ return fxl::JoinStrings(paths, "/");
+}
+
+// Recursively delete |path|.
+bool DeletePath(const std::string& path) { return files::DeletePath(path, /*recursive=*/true); }
+
+// Get the contents of a directory without ".".
+std::vector<std::string> GetDirectoryContents(const std::string& dir) {
+ std::vector<std::string> contents;
+ files::ReadDirContents(dir, &contents);
+
+ contents.erase(std::remove(contents.begin(), contents.end(), "."), contents.end());
+ return contents;
+}
+
bool WriteAnnotationsAsJson(const std::string& path,
const std::map<std::string, std::string>& annotations) {
rapidjson::Document json(rapidjson::kObjectType);
@@ -85,15 +103,6 @@
return files::ReadFileToVector(path, attachment);
}
-// Get the contents of a directory without ".".
-std::vector<std::string> GetDirectoryContents(const std::string& dir) {
- std::vector<std::string> contents;
- files::ReadDirContents(dir, &contents);
-
- contents.erase(std::remove(contents.begin(), contents.end(), "."), contents.end());
- return contents;
-}
-
} // namespace
Store::Store(const std::string& root_dir) : root_dir_(root_dir) {}
@@ -107,20 +116,16 @@
}
const Uid id = next_id_++;
- const std::string report_dir =
- files::JoinPath(files::JoinPath(root_dir_, report.ProgramShortname()), std::to_string(id));
+ const std::string dir = JoinPaths({root_dir_, report.ProgramShortname(), std::to_string(id)});
- auto cleanup_on_error =
- fit::defer([report_dir] { files::DeletePath(report_dir, /*recursive=*/true); });
+ auto cleanup_on_error = fit::defer([dir] { DeletePath(dir); });
- if (!files::CreateDirectory(report_dir)) {
- FX_LOGS(ERROR) << "Failed to create directory for report: " << report_dir;
+ if (!files::CreateDirectory(dir)) {
+ FX_LOGS(ERROR) << "Failed to create directory for report: " << dir;
return std::nullopt;
}
- auto MakeFilepath = [report_dir](const std::string& filename) {
- return files::JoinPath(report_dir, filename);
- };
+ auto MakeFilepath = [dir](const std::string& filename) { return files::JoinPath(dir, filename); };
if (!WriteAnnotationsAsJson(MakeFilepath(kAnnotationsFilename), report.Annotations())) {
FX_LOGS(ERROR) << "Failed to write annotations";
@@ -141,8 +146,10 @@
}
}
- id_to_metadata_[id] =
- Metadata{.report_dir = report_dir, .program_shortname = report.ProgramShortname()};
+ id_to_metadata_[id] = ReportMetadata{
+ .dir = dir,
+ .program_shortname = report.ProgramShortname(),
+ };
cleanup_on_error.cancel();
return id;
}
@@ -152,15 +159,14 @@
return std::nullopt;
}
- const std::vector<std::string> report_files =
- GetDirectoryContents(id_to_metadata_[id].report_dir);
+ const std::vector<std::string> report_files = GetDirectoryContents(id_to_metadata_[id].dir);
if (report_files.empty()) {
return std::nullopt;
}
- auto MakeFilepath = [report_dir = id_to_metadata_[id].report_dir](const std::string& filename) {
- return files::JoinPath(report_dir, filename);
+ auto MakeFilepath = [dir = id_to_metadata_[id].dir](const std::string& filename) {
+ return files::JoinPath(dir, filename);
};
std::map<std::string, std::string> annotations;
@@ -202,15 +208,15 @@
// The report is stored under /tmp/store/<program shortname>/$id.
// We first delete /tmp/store/<program shortname>/$id and then if $id was the only report for
// <program shortname>, we also delete /tmp/store/<program name>.
- if (!files::DeletePath(id_to_metadata_.at(id).report_dir, /*recursive=*/true)) {
- FX_LOGS(ERROR) << "Failed to delete reports at " << id_to_metadata_.at(id).report_dir;
+ if (!DeletePath(id_to_metadata_.at(id).dir)) {
+ FX_LOGS(ERROR) << "Failed to delete report at " << id_to_metadata_.at(id).dir;
}
const std::string program_path =
files::JoinPath(root_dir_, id_to_metadata_.at(id).program_shortname);
const std::vector<std::string> dir_contents = GetDirectoryContents(program_path);
if (dir_contents.empty()) {
- if (!files::DeletePath(program_path, /*recursive=*/true)) {
+ if (!DeletePath(program_path)) {
FX_LOGS(ERROR) << "Failed to delete " << program_path;
}
}
@@ -219,7 +225,7 @@
}
void Store::RemoveAll() {
- if (!files::DeletePath(root_dir_, /*recursive=*/true)) {
+ if (!DeletePath(root_dir_)) {
FX_LOGS(ERROR) << "Failed to delete all reports";
}
files::CreateDirectory(root_dir_);
diff --git a/src/developer/forensics/crash_reports/store.h b/src/developer/forensics/crash_reports/store.h
index 1c35ac6..3e4d794 100644
--- a/src/developer/forensics/crash_reports/store.h
+++ b/src/developer/forensics/crash_reports/store.h
@@ -40,16 +40,16 @@
bool Contains(const Uid& id) const;
private:
- struct Metadata {
+ struct ReportMetadata {
// The directory containing the report's files, e.g., /tmp/crashes/foo/<report Uid>
- std::string report_dir;
+ std::string dir;
// "foo" in the above example, it shouldn't contain forward slashes.
std::string program_shortname;
};
std::string root_dir_;
- std::map<Uid, Metadata> id_to_metadata_;
+ std::map<Uid, ReportMetadata> id_to_metadata_;
// TODO(47137): Uids may collide across component instances, however this will no longer happen
// once we rebuild the store's metadata when the component starts up.
