diff --git a/common/util/BUILD b/common/util/BUILD index 9886ffe3a..85c05f15c 100644 --- a/common/util/BUILD +++ b/common/util/BUILD @@ -44,6 +44,7 @@ cc_library( hdrs = ["file_util.h"], deps = [ ":logging", + "@com_google_absl//absl/status", "@com_google_absl//absl/strings", ], ) diff --git a/common/util/file_util.cc b/common/util/file_util.cc index c26151425..a7ed73fb1 100644 --- a/common/util/file_util.cc +++ b/common/util/file_util.cc @@ -14,7 +14,9 @@ #include "common/util/file_util.h" +#include #include +#include #include #include #include @@ -24,6 +26,7 @@ #include #include +#include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "common/util/logging.h" @@ -46,7 +49,27 @@ absl::string_view Stem(absl::string_view filename) { : filename.substr(0, last_dot_pos); } -bool GetContents(absl::string_view filename, std::string *content) { +// This will always return an error, even if we can't determine anything +// from errno. Returns "fallback_msg" in that case. +static absl::Status CreateErrorStatusFromErrno(const char *fallback_msg) { + using absl::StatusCode; + const char *const system_msg = errno == 0 ? fallback_msg : strerror(errno); + switch (errno) { + case EPERM: + case EACCES: + return absl::Status(StatusCode::kPermissionDenied, system_msg); + case ENOENT: + return absl::Status(StatusCode::kNotFound, system_msg); + case EEXIST: + return absl::Status(StatusCode::kAlreadyExists, system_msg); + case EINVAL: + return absl::Status(StatusCode::kInvalidArgument, system_msg); + default: + return absl::Status(StatusCode::kUnknown, system_msg); + } +} + +absl::Status GetContents(absl::string_view filename, std::string *content) { std::ifstream fs; std::istream* stream = nullptr; if (filename == "-") { @@ -56,27 +79,29 @@ bool GetContents(absl::string_view filename, std::string *content) { fs.open(std::string(filename).c_str()); stream = &fs; } - if (!stream->good()) return false; + if (!stream->good()) return CreateErrorStatusFromErrno("can't read"); content->assign((std::istreambuf_iterator(*stream)), std::istreambuf_iterator()); - return true; + return absl::OkStatus(); } -bool SetContents(absl::string_view filename, absl::string_view content) { +absl::Status SetContents(absl::string_view filename, + absl::string_view content) { std::ofstream f(std::string(filename).c_str()); - if (!f.good()) return false; + if (!f.good()) return CreateErrorStatusFromErrno("can't write."); f << content; - return true; + return absl::OkStatus(); } std::string JoinPath(absl::string_view base, absl::string_view name) { return absl::StrCat(base, "/", name); } -bool CreateDir(absl::string_view dir) { +absl::Status CreateDir(absl::string_view dir) { const std::string path(dir); int ret = mkdir(path.c_str(), 0755); - return ret == 0 || errno == EEXIST; + if (ret == 0 || errno == EEXIST) return absl::OkStatus(); + return CreateErrorStatusFromErrno("can't create directory"); } namespace testing { @@ -85,7 +110,8 @@ ScopedTestFile::ScopedTestFile(absl::string_view base_dir, // There is no secrecy needed for test files, just need to be unique enough. : filename_(JoinPath( base_dir, absl::StrCat("scoped-file-", getpid(), "-", random()))) { - CHECK(SetContents(filename_, content)); + absl::Status status = SetContents(filename_, content); + CHECK(status.ok()) << status.message(); } ScopedTestFile::~ScopedTestFile() { unlink(filename_.c_str()); } diff --git a/common/util/file_util.h b/common/util/file_util.h index 49b8e9d1d..69da5c54a 100644 --- a/common/util/file_util.h +++ b/common/util/file_util.h @@ -19,6 +19,7 @@ #include +#include "absl/status/status.h" #include "absl/strings/string_view.h" namespace verible { @@ -35,18 +36,16 @@ absl::string_view Basename(absl::string_view filename); absl::string_view Stem(absl::string_view filename); // Read file "filename" and store its content in "content" -// TODO(hzeller): consider absl::Status return? -bool GetContents(absl::string_view filename, std::string *content); +absl::Status GetContents(absl::string_view filename, std::string *content); // Create file "filename" and store given content in it. -// TODO(hzeller): consider absl::Status return ? -bool SetContents(absl::string_view filename, absl::string_view content); +absl::Status SetContents(absl::string_view filename, absl::string_view content); // Join directory + filename std::string JoinPath(absl::string_view base, absl::string_view name); // Create directory with given name, return success. -bool CreateDir(absl::string_view dir); +absl::Status CreateDir(absl::string_view dir); namespace testing { // Useful for testing: a temporary file that is pre-populated with a particular diff --git a/common/util/file_util_test.cc b/common/util/file_util_test.cc index bd3ba03a7..0c4a4dfe2 100644 --- a/common/util/file_util_test.cc +++ b/common/util/file_util_test.cc @@ -19,6 +19,9 @@ #include "gtest/gtest.h" #include "absl/strings/string_view.h" +#undef EXPECT_OK +#define EXPECT_OK(value) EXPECT_TRUE((value).ok()) + namespace verible { namespace util { namespace { @@ -43,19 +46,38 @@ TEST(FileUtil, CreateDir) { const std::string test_file = file::JoinPath(test_dir, "foo"); const absl::string_view test_content = "directory create test"; - EXPECT_TRUE(file::CreateDir(test_dir)); + EXPECT_OK(file::CreateDir(test_dir)); - EXPECT_TRUE(file::SetContents(test_file, test_content)); + EXPECT_OK(file::SetContents(test_file, test_content)); std::string read_back_content; - EXPECT_TRUE(file::GetContents(test_file, &read_back_content)); + EXPECT_OK(file::GetContents(test_file, &read_back_content)); EXPECT_EQ(test_content, read_back_content); } +TEST(FileUtil, StatusErrorReporting) { + std::string content; + absl::Status status = file::GetContents("does-not-exist", &content); + EXPECT_FALSE(status.ok()); + EXPECT_EQ(status.code(), absl::StatusCode::kNotFound) << status; + + const std::string test_file = file::JoinPath(testing::TempDir(), "test-err"); + unlink(test_file.c_str()); // Remove file if left from previous test. + EXPECT_OK(file::SetContents(test_file, "foo")); + + EXPECT_OK(file::GetContents(test_file, &content)); + EXPECT_EQ(content, "foo"); + + chmod(test_file.c_str(), 0); // Enforce a permission denied situation + status = file::GetContents(test_file, &content); + EXPECT_FALSE(status.ok()); + EXPECT_EQ(status.code(), absl::StatusCode::kPermissionDenied) << status; +} + TEST(FileUtil, ScopedTestFile) { const absl::string_view test_content = "Hello World!"; file::testing::ScopedTestFile test_file(testing::TempDir(), test_content); std::string read_back_content; - EXPECT_TRUE(file::GetContents(test_file.filename(), &read_back_content)); + EXPECT_OK(file::GetContents(test_file.filename(), &read_back_content)); EXPECT_EQ(test_content, read_back_content); } @@ -66,7 +88,7 @@ TEST(FileUtil, ScopedTestFileStdin) { // closed, resulting in an empty string. const absl::string_view test_content = ""; std::string read_back_content; - EXPECT_TRUE(file::GetContents("-", &read_back_content)); + EXPECT_OK(file::GetContents("-", &read_back_content)); EXPECT_EQ(test_content, read_back_content); } diff --git a/verilog/analysis/verilog_linter.cc b/verilog/analysis/verilog_linter.cc index 12329947b..bd74cd715 100644 --- a/verilog/analysis/verilog_linter.cc +++ b/verilog/analysis/verilog_linter.cc @@ -76,7 +76,7 @@ int LintOneFile(std::ostream* stream, absl::string_view filename, const LinterConfiguration& config, bool parse_fatal, bool lint_fatal) { std::string content; - if (!verible::file::GetContents(filename, &content)) return 2; + if (!verible::file::GetContents(filename, &content).ok()) return 2; // Lex and parse the contents of the file. const auto analyzer = @@ -149,14 +149,17 @@ absl::Status VerilogLinter::Configure( if (!configuration.external_waivers.empty()) { std::string content; - if (verible::file::GetContents(configuration.external_waivers, &content)) { + auto status = + verible::file::GetContents(configuration.external_waivers, &content); + if (status.ok()) { return lint_waiver_.ApplyExternalWaivers(configuration.ActiveRuleIds(), configuration.external_waivers, content); } else { - return absl::UnavailableError( + return absl::Status( + status.code(), absl::StrCat("Unable to read waivers configuration - ", - configuration.external_waivers)); + configuration.external_waivers, "; ", status.message())); } } @@ -236,7 +239,9 @@ LinterConfiguration LinterConfigurationFromFlags() { // Read local configuration file std::string content; - if (verible::file::GetContents(absl::GetFlag(FLAGS_rules_config), &content)) { + const absl::Status config_read_status = + verible::file::GetContents(absl::GetFlag(FLAGS_rules_config), &content); + if (config_read_status.ok()) { RuleBundle local_rules_bundle; std::string error; if (local_rules_bundle.ParseConfiguration(content, '\n', &error)) { @@ -247,8 +252,9 @@ LinterConfiguration LinterConfigurationFromFlags() { } } else if (FLAGS_rules_config.IsModified()) { // If flag is modified and we were unable to open the file, report that - LOG(WARNING) << "Unable to open rules configuration file: " - << absl::GetFlag(FLAGS_rules_config) << std::endl; + LOG(WARNING) << absl::GetFlag(FLAGS_rules_config) + << ": Unable to read rules configuration file " + << config_read_status << std::endl; } // Turn on rules found in config flags. diff --git a/verilog/tools/diff/verilog_diff.cc b/verilog/tools/diff/verilog_diff.cc index fb193c416..4052c1399 100644 --- a/verilog/tools/diff/verilog_diff.cc +++ b/verilog/tools/diff/verilog_diff.cc @@ -105,13 +105,15 @@ int main(int argc, char** argv) { // Open both files. std::string content1; - if (!verible::file::GetContents(args[1], &content1)) { - std::cerr << "Error reading file " << args[1] << std::endl; + absl::Status status = verible::file::GetContents(args[1], &content1); + if (!status.ok()) { + std::cerr << args[1] << ": " << status << std::endl; return kUserErrorCode; } std::string content2; - if (!verible::file::GetContents(args[2], &content2)) { - std::cerr << "Error reading file " << args[2] << std::endl; + status = verible::file::GetContents(args[2], &content2); + if (!status.ok()) { + std::cerr << args[1] << ": " << status << std::endl; return kUserErrorCode; } diff --git a/verilog/tools/formatter/verilog_format.cc b/verilog/tools/formatter/verilog_format.cc index c9c9ab24c..26b07c514 100644 --- a/verilog/tools/formatter/verilog_format.cc +++ b/verilog/tools/formatter/verilog_format.cc @@ -142,8 +142,9 @@ bool formatOneFile(absl::string_view filename, // Read contents into memory first. std::string content; - if (!verible::file::GetContents(filename, &content)) { - FileMsg(filename) << "Couldn't read" << std::endl; + absl::Status status = verible::file::GetContents(filename, &content); + if (!status.ok()) { + FileMsg(filename) << status << std::endl; return false; } @@ -211,8 +212,9 @@ bool formatOneFile(absl::string_view filename, // Don't write if the output is exactly as the input, so that we don't mess // with tools that look for timestamp changes (such as make). if (content != formatted_output) { - if (!verible::file::SetContents(filename, formatted_output)) { - FileMsg(filename) << "error writing to file" << std::endl; + status = verible::file::SetContents(filename, formatted_output); + if (!status.ok()) { + FileMsg(filename) << "error writing result " << status << std::endl; return false; } } else { diff --git a/verilog/tools/obfuscator/verilog_obfuscate.cc b/verilog/tools/obfuscator/verilog_obfuscate.cc index 4611bbf01..221a1dd2e 100644 --- a/verilog/tools/obfuscator/verilog_obfuscate.cc +++ b/verilog/tools/obfuscator/verilog_obfuscate.cc @@ -71,12 +71,14 @@ Output is written to stdout. const auto& save_map_file = absl::GetFlag(FLAGS_save_map); if (!load_map_file.empty()) { std::string load_map_content; - if (!verible::file::GetContents(load_map_file, &load_map_content)) { - std::cerr << "Error reading --load_map file: " << load_map_file - << std::endl; + absl::Status status = + verible::file::GetContents(load_map_file, &load_map_content); + if (!status.ok()) { + std::cerr << "Error reading --load_map file " << load_map_file << ": " + << status << std::endl; return 1; } - const auto status = subst.load(load_map_content); + status = subst.load(load_map_content); if (!status.ok()) { std::cerr << "Error parsing --load_map file: " << load_map_file << '\n' << status.message() << std::endl; @@ -89,7 +91,7 @@ Output is written to stdout. // Read from stdin. std::string content; - if (!verible::file::GetContents("-", &content)) { + if (!verible::file::GetContents("-", &content).ok()) { return 1; } @@ -102,7 +104,7 @@ Output is written to stdout. } if (!decode && !save_map_file.empty()) { - if (!verible::file::SetContents(save_map_file, subst.save())) { + if (!verible::file::SetContents(save_map_file, subst.save()).ok()) { std::cerr << "Error writing --save_map file: " << save_map_file << std::endl; return 1; diff --git a/verilog/tools/syntax/verilog_syntax.cc b/verilog/tools/syntax/verilog_syntax.cc index dbf621fb9..5240b8bc7 100644 --- a/verilog/tools/syntax/verilog_syntax.cc +++ b/verilog/tools/syntax/verilog_syntax.cc @@ -146,7 +146,7 @@ int main(int argc, char** argv) { for (const auto filename : verible::make_range(args.begin() + 1, args.end())) { std::string content; - if (!verible::file::GetContents(filename, &content)) { + if (!verible::file::GetContents(filename, &content).ok()) { exit_status = 1; continue; }