Filesystem error improvements

src/libcmd/repl.cc

@@ -710,7 +710,7 @@ void NixRepl::loadFlake(const std::string & flakeRefS)
     try {
         cwd = std::filesystem::current_path();
     } catch (std::filesystem::filesystem_error & e) {
-        throw SysError("cannot determine current working directory");
+        throw SystemError(e.code(), "cannot determine current working directory");
     }
 
     auto flakeRef = parseFlakeRef(fetchSettings, flakeRefS, cwd.string(), true);

src/libfetchers/git-utils.cc

@@ -247,7 +247,8 @@ static void initRepoAtomically(std::filesystem::path & path, GitRepo::Options op
             || e.code() == std::errc::directory_not_empty) {
             return;
         } else
-            throw SysError("moving temporary git repository from %s to %s", PathFmt(tmpDir), PathFmt(path));
+            throw SystemError(
+                e.code(), "moving temporary git repository from %s to %s", PathFmt(tmpDir), PathFmt(path));
     }
     // we successfully moved the repository, so the temporary directory no longer exists.
     delTmpDir.cancel();

src/libstore/builtins/unpack-channel.cc

@@ -36,8 +36,8 @@ static void builtinUnpackChannel(const BuiltinBuilderContext & ctx)
     auto target = out / channelName;
     try {
         std::filesystem::rename(fileName, target);
-    } catch (std::filesystem::filesystem_error &) {
-        throw SysError("failed to rename %1% to %2%", fileName, target.string());
+    } catch (std::filesystem::filesystem_error & e) {
+        throw SystemError(e.code(), "failed to rename %1% to %2%", fileName, target.string());
     }
 }
 

src/libstore/gc.cc

@@ -272,7 +272,7 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R
             || e.code() == std::errc::not_a_directory)
             printInfo("cannot read potential root '%1%'", path);
         else
-            throw;
+            throw SystemError(e.code(), "finding GC roots in '%1%'", path);
     }
 
     catch (SystemError & e) {

src/libstore/local-gc.cc

@@ -35,7 +35,7 @@ static void readProcLink(const std::filesystem::path & file, UncheckedRoots & ro
         if (e.code() == std::errc::no_such_file_or_directory || e.code() == std::errc::permission_denied
             || e.code() == std::errc::no_such_process)
             return;
-        throw;
+        throw SystemError(e.code(), "reading symlink '%s'", PathFmt(file));
     }
     if (buf.is_absolute())
         roots[buf.string()].emplace(file.string());

src/libstore/machines.cc

@@ -111,8 +111,8 @@ static std::vector<std::string> expandBuilderLines(const std::string & builders)
                 std::string text;
                 try {
                     text = readFile(path);
-                } catch (const SysError & e) {
-                    if (e.errNo != ENOENT)
+                } catch (const SystemError & e) {
+                    if (!e.is(std::errc::no_such_file_or_directory))
                         throw;
                     debug("cannot find machines file '%s'", path);
                     continue;

src/libstore/optimise-store.cc

@@ -202,14 +202,13 @@ void LocalStore::optimisePath_(
                    full.  When that happens, it's fine to ignore it: we
                    just effectively disable deduplication of this
                    file.
-                   TODO: Get rid of errno, use error code.
                    */
-                printInfo("cannot link %s to '%s': %s", PathFmt(linkPath), path, strerror(errno));
+                printInfo("cannot link %s to '%s': %s", PathFmt(linkPath), path, e.code().message());
                 return;
             }
 
             else
-                throw;
+                throw SystemError(e.code(), "creating hard link from %1% to %2%", PathFmt(linkPath), path);
         }
     }
 
@@ -250,7 +249,7 @@ void LocalStore::optimisePath_(
                 printInfo("%1% has maximum number of links", PathFmt(linkPath));
             return;
         }
-        throw;
+        throw SystemError(e.code(), "creating hard link from %1% to %2%", PathFmt(linkPath), PathFmt(tempLink));
     }
 
     /* Atomically replace the old file with the new hard link. */
@@ -271,7 +270,7 @@ void LocalStore::optimisePath_(
             debug("%s has reached maximum number of links", PathFmt(linkPath));
             return;
         }
-        throw;
+        throw SystemError(e.code(), "renaming %1% to %2%", PathFmt(tempLink), path);
     }
 
     stats.filesLinked++;

src/libstore/profiles.cc

@@ -103,7 +103,7 @@ static void removeFile(const std::filesystem::path & path)
     try {
         std::filesystem::remove(path);
     } catch (std::filesystem::filesystem_error & e) {
-        throw SysError("removing file %1%", PathFmt(path));
+        throw SystemError(e.code(), "removing file %1%", PathFmt(path));
     }
 }
 
@@ -324,8 +324,6 @@ std::filesystem::path getDefaultProfile(ProfileDirsOptions settings)
         return absPath(readLink(profileLink), &linkDir);
     } catch (Error &) {
         return profileLink;
-    } catch (std::filesystem::filesystem_error &) {
-        return profileLink;
     }
 }
 

src/libutil-tests/file-system.cc

@@ -319,7 +319,7 @@ TEST(chmodIfNeeded, works)
 
 TEST(chmodIfNeeded, nonexistent)
 {
-    ASSERT_THROW(chmodIfNeeded("/schnitzel/darmstadt/pommes", 0755), SysError);
+    ASSERT_THROW(chmodIfNeeded("/schnitzel/darmstadt/pommes", 0755), SystemError);
 }
 
 /* ----------------------------------------------------------------------------
@@ -340,7 +340,7 @@ TEST(DirectoryIterator, works)
 
 TEST(DirectoryIterator, nonexistent)
 {
-    ASSERT_THROW(DirectoryIterator("/schnitzel/darmstadt/pommes"), SysError);
+    ASSERT_THROW(DirectoryIterator("/schnitzel/darmstadt/pommes"), SystemError);
 }
 
 /* ----------------------------------------------------------------------------

src/libutil/file-system.cc

@@ -40,9 +40,9 @@ DirectoryIterator::DirectoryIterator(const std::filesystem::path & p)
         // **Attempt to create the underlying directory_iterator**
         it_ = std::filesystem::directory_iterator(p);
     } catch (const std::filesystem::filesystem_error & e) {
-        // **Catch filesystem_error and throw SysError**
-        // Adapt the error message as needed for SysError
-        throw SysError("cannot read directory %s", PathFmt(p));
+        // **Catch filesystem_error and throw SystemError**
+        // Adapt the error message as needed for SystemError
+        throw SystemError(e.code(), "cannot read directory %s", PathFmt(p));
     }
 }
 
@@ -280,7 +280,11 @@ bool pathAccessible(const std::filesystem::path & path)
 std::filesystem::path readLink(const std::filesystem::path & path)
 {
     checkInterrupt();
-    return std::filesystem::read_symlink(path);
+    try {
+        return std::filesystem::read_symlink(path);
+    } catch (std::filesystem::filesystem_error & e) {
+        throw SystemError(e.code(), "reading symbolic link '%s'", PathFmt(path));
+    }
 }
 
 Path readLink(const Path & path)
@@ -463,7 +467,7 @@ void createDirs(const std::filesystem::path & path)
     try {
         std::filesystem::create_directories(path);
     } catch (std::filesystem::filesystem_error & e) {
-        throw SysError("creating directory '%1%'", path.string());
+        throw SystemError(e.code(), "creating directory '%1%'", path.string());
     }
 }
 
@@ -664,15 +668,15 @@ void replaceSymlink(const std::filesystem::path & target, const std::filesystem:
         } catch (std::filesystem::filesystem_error & e) {
             if (e.code() == std::errc::file_exists)
                 continue;
-            throw SysError("creating symlink %1% -> %2%", PathFmt(tmp), PathFmt(target));
+            throw SystemError(e.code(), "creating symlink %1% -> %2%", PathFmt(tmp), PathFmt(target));
         }
 
         try {
             std::filesystem::rename(tmp, link);
         } catch (std::filesystem::filesystem_error & e) {
             if (e.code() == std::errc::file_exists)
                 continue;
-            throw SysError("renaming %1% to %2%", PathFmt(tmp), PathFmt(link));
+            throw SystemError(e.code(), "renaming %1% to %2%", PathFmt(tmp), PathFmt(link));
         }
 
         break;
@@ -773,7 +777,7 @@ std::filesystem::path makeParentCanonical(const std::filesystem::path & rawPath)
         }
         return std::filesystem::canonical(parent) / path.filename();
     } catch (std::filesystem::filesystem_error & e) {
-        throw SysError("canonicalising parent path of %1%", PathFmt(path));
+        throw SystemError(e.code(), "canonicalising parent path of %1%", PathFmt(path));
     }
 }
 

src/libutil/include/nix/util/error.hh

@@ -127,20 +127,20 @@ public:
     BaseError & operator=(BaseError &&) = default;
 
     template<typename... Args>
-    BaseError(unsigned int status, const Args &... args)
-        : err{.level = lvlError, .msg = HintFmt(args...), .pos = {}, .status = status}
+    BaseError(unsigned int status, Args &&... args)
+        : err{.level = lvlError, .msg = HintFmt(std::forward<Args>(args)...), .pos = {}, .status = status}
     {
     }
 
     template<typename... Args>
-    explicit BaseError(const std::string & fs, const Args &... args)
-        : err{.level = lvlError, .msg = HintFmt(fs, args...), .pos = {}}
+    explicit BaseError(const std::string & fs, Args &&... args)
+        : err{.level = lvlError, .msg = HintFmt(fs, std::forward<Args>(args)...), .pos = {}}
     {
     }
 
     template<typename... Args>
-    BaseError(const Suggestions & sug, const Args &... args)
-        : err{.level = lvlError, .msg = HintFmt(args...), .pos = {}, .suggestions = sug}
+    BaseError(const Suggestions & sug, Args &&... args)
+        : err{.level = lvlError, .msg = HintFmt(std::forward<Args>(args)...), .pos = {}, .suggestions = sug}
     {
     }
 
@@ -204,9 +204,9 @@ public:
      * @param args... Format string arguments.
      */
     template<typename... Args>
-    void addTrace(std::shared_ptr<const Pos> && pos, std::string_view fs, const Args &... args)
+    void addTrace(std::shared_ptr<const Pos> && pos, std::string_view fs, Args &&... args)
     {
-        addTrace(std::move(pos), HintFmt(std::string(fs), args...));
+        addTrace(std::move(pos), HintFmt(std::string(fs), std::forward<Args>(args)...));
     }
 
     /**
@@ -248,19 +248,39 @@ MakeError(UnimplementedError, Error);
 class SystemError : public Error
 {
     std::error_code errorCode;
+    std::string errorDetails;
 
-public:
+protected:
+
+    /**
+     * Just here to allow derived classes to use the right constructor
+     * (the protected one).
+     */
+    struct Disambig
+    {};
+
+    /**
+     * Protected constructor for subclasses that provide their own error message.
+     * The error message is appended to the formatted hint.
+     */
     template<typename... Args>
-    SystemError(std::errc posixErrNo, Args &&... args)
-        : Error(std::forward<Args>(args)...)
-        , errorCode(std::make_error_code(posixErrNo))
+    SystemError(Disambig, std::error_code errorCode, std::string_view errorDetails, Args &&... args)
+        : Error("")
+        , errorCode(errorCode)
+        , errorDetails(errorDetails)
     {
+        auto hf = HintFmt(std::forward<Args>(args)...);
+        err.msg = HintFmt("%s: %s", Uncolored(hf.str()), errorDetails);
     }
 
+public:
+    /**
+     * Construct with an error code. The error code's message is automatically
+     * appended to the error message.
+     */
     template<typename... Args>
     SystemError(std::error_code errorCode, Args &&... args)
-        : Error(std::forward<Args>(args)...)
-        , errorCode(errorCode)
+        : SystemError(Disambig{}, errorCode, errorCode.message(), std::forward<Args>(args)...)
     {
     }
 
@@ -301,12 +321,14 @@ public:
      * will be used to try to add additional information to the message.
      */
     template<typename... Args>
-    SysError(int errNo, const Args &... args)
-        : SystemError(static_cast<std::errc>(errNo), "")
+    SysError(int errNo, Args &&... args)
+        : SystemError(
+              Disambig{},
+              std::make_error_code(static_cast<std::errc>(errNo)),
+              strerror(errNo),
+              std::forward<Args>(args)...)
         , errNo(errNo)
     {
-        auto hf = HintFmt(args...);
-        err.msg = HintFmt("%1%: %2%", Uncolored(hf.str()), strerror(errNo));
     }
 
     /**
@@ -316,8 +338,8 @@ public:
      * calling this constructor!
      */
     template<typename... Args>
-    SysError(const Args &... args)
-        : SysError(errno, args...)
+    SysError(Args &&... args)
+        : SysError(errno, std::forward<Args>(args)...)
     {
     }
 };
@@ -381,12 +403,14 @@ public:
      * information to the message.
      */
     template<typename... Args>
-    WinError(DWORD lastError, const Args &... args)
-        : SystemError(std::error_code(lastError, std::system_category()), "")
+    WinError(DWORD lastError, Args &&... args)
+        : SystemError(
+              Disambig{},
+              std::error_code(lastError, std::system_category()),
+              renderError(lastError),
+              std::forward<Args>(args)...)
         , lastError(lastError)
     {
-        auto hf = HintFmt(args...);
-        err.msg = HintFmt("%1%: %2%", Uncolored(hf.str()), renderError(lastError));
     }
 
     /**
@@ -396,14 +420,14 @@ public:
      * before calling this constructor!
      */
     template<typename... Args>
-    WinError(const Args &... args)
-        : WinError(GetLastError(), args...)
+    WinError(Args &&... args)
+        : WinError(GetLastError(), std::forward<Args>(args)...)
     {
     }
 
 private:
 
-    std::string renderError(DWORD lastError);
+    static std::string renderError(DWORD lastError);
 };
 
 } // namespace windows

src/libutil/include/nix/util/file-system-at.hh

@@ -75,7 +75,7 @@ namespace linux {
  *
  * @see https://man7.org/linux/man-pages/man2/openat2.2.html
  * @see https://man7.org/linux/man-pages/man2/open_how.2type.html
-v*
+ *
  * @param flags O_* flags
  * @param mode Mode for O_{CREAT,TMPFILE}
  * @param resolve RESOLVE_* flags

src/libutil/include/nix/util/file-system.hh

@@ -189,21 +189,19 @@ Path readLink(const Path & path);
  */
 std::filesystem::path readLink(const std::filesystem::path & path);
 
-#ifdef _WIN32
-namespace windows {
-
 /**
- * Get the path associated with a file handle.
+ * Get the path associated with a file descriptor.
  *
  * @note One MUST only use this for error handling, because it creates
  * TOCTOU issues. We don't mind if error messages point to out of date
  * paths (that is a rather trivial TOCTOU --- the error message is best
  * effort) but for anything else we do.
+ *
+ * @note this function will clobber `errno` (Unix) / "last error"
+ * (Windows), so care must be used to get those error codes, then call
+ * this, then build a `SysError` / `WinError` with the saved error code.
  */
-std::filesystem::path handleToPath(Descriptor handle);
-
-} // namespace windows
-#endif
+std::filesystem::path descriptorToPath(Descriptor fd);
 
 /**
  * Open a `Descriptor` with read-only access to the given directory.

src/libutil/posix-source-accessor.cc

@@ -170,7 +170,7 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath &
                 if (e.code() == std::errc::permission_denied || e.code() == std::errc::operation_not_permitted)
                     return std::nullopt;
                 else
-                    throw;
+                    throw SystemError(e.code(), "getting status of '%s'", PathFmt(entry.path()));
             }
         }();
         res.emplace(entry.path().filename().string(), type);