PR #295: Use absl::Status in file operations.

This is an un-changed fresh one-patch version of #294 - the other one had issues while importing due to frequent master-refreshes ? Not clear.

GitHub PR #295

Copybara import of the project:

  - 07ac212 Use absl::Status in file operations. by Henner Zeller <[email protected]>

Closes #295

PiperOrigin-RevId: 310573302

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",
     ],
 )

common/util/file_util.cc

@@ -14,7 +14,9 @@
 
 #include "common/util/file_util.h"
 
+#include <errno.h>
 #include <stdlib.h>
+#include <string.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
@@ -24,6 +26,7 @@
 #include <streambuf>
 #include <string>
 
+#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<char>(*stream)),
                   std::istreambuf_iterator<char>());
-  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()); }

common/util/file_util.h

@@ -19,6 +19,7 @@
 
 #include <string>
 
+#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

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);
 }
 

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.

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;
   }
 

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 {

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;

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;
     }