From 6208ca72093a0e05c56561dab349423f9bff069b Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 1 Dec 2023 17:03:28 -0500 Subject: [PATCH] Separate `SystemError` from `SysError` Most of this is a `catch SysError` -> `catch SystemError` sed. This is a rather pure-churn change I would like to get out of the way. **The intersting part is `src/libutil/error.hh`.** On Unix, we will only throw the `SysError` concrete class, which has the same constructors that `SystemError` used to have. On Windows, we will throw `WinError` *and* `SysError`. `WinError` (which will be created in a later PR), will use a `DWORD` instead of `int` error value, and `GetLastError()`, which is the Windows equivalent of the `errno` machinery. Windows will *also* use `SysError` because Window's "libc" (MSVCRT) implements the POSIX interface, and we use it too. As the docs describe, while we *throw* one of the 3 choices above (2 concrete classes or the alias), we should always *catch* `SystemError`. This ensures no matter how the implementation changes for Windows (e.g. between `SysError` and `WinError`) the catching logic stays the same and stays correct. Co-Authored-By volth Co-Authored-By Eugene Butler --- src/libcmd/repl.cc | 2 +- src/libstore/build/local-derivation-goal.cc | 4 +- src/libstore/gc.cc | 2 +- src/libstore/globals.cc | 2 +- src/libstore/keys.cc | 2 +- src/libstore/local-store.cc | 2 +- src/libstore/optimise-store.cc | 2 +- src/libstore/remote-fs-accessor.cc | 4 +- src/libutil/args.cc | 2 +- src/libutil/cgroup.cc | 2 +- src/libutil/config.cc | 2 +- src/libutil/error.hh | 42 ++++++++++++++++++--- src/libutil/file-descriptor.cc | 2 +- src/libutil/logging.cc | 2 +- src/libutil/serialise.cc | 2 +- src/libutil/util.cc | 2 +- src/nix-build/nix-build.cc | 2 +- src/nix/config-check.cc | 2 +- tests/unit/libutil/logging.cc | 6 +-- 19 files changed, 59 insertions(+), 27 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 72e3559df..918b2e53a 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -254,7 +254,7 @@ void NixRepl::mainLoop() rl_readline_name = "nix-repl"; try { createDirs(dirOf(historyFile)); - } catch (SysError & e) { + } catch (SystemError & e) { logWarning(e.info()); } #ifndef USE_READLINE diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index b01d9e237..f85301950 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1495,7 +1495,7 @@ void LocalDerivationGoal::startDaemon() daemon::processConnection(store, from, to, NotTrusted, daemon::Recursive); debug("terminated daemon connection"); - } catch (SysError &) { + } catch (SystemError &) { ignoreException(); } }); @@ -1707,7 +1707,7 @@ void LocalDerivationGoal::runChild() try { if (drv->isBuiltin() && drv->builder == "builtin:fetchurl") netrcData = readFile(settings.netrcFile); - } catch (SysError &) { } + } catch (SystemError &) { } #if __linux__ if (useChroot) { diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 2bd3a2edc..5cbce0748 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -413,7 +413,7 @@ void LocalStore::findRuntimeRoots(Roots & roots, bool censor) auto env_end = std::sregex_iterator{}; for (auto i = std::sregex_iterator{envString.begin(), envString.end(), storePathRegex}; i != env_end; ++i) unchecked[i->str()].emplace(envFile); - } catch (SysError & e) { + } catch (SystemError & e) { if (errno == ENOENT || errno == EACCES || errno == ESRCH) continue; throw; diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 50584e06c..d22ae4ca0 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -118,7 +118,7 @@ void loadConfFile() try { std::string contents = readFile(path); globalConfig.applyConfig(contents, path); - } catch (SysError &) { } + } catch (SystemError &) { } }; applyConfigFile(settings.nixConfDir + "/nix.conf"); diff --git a/src/libstore/keys.cc b/src/libstore/keys.cc index 2cc50970f..70478e7ad 100644 --- a/src/libstore/keys.cc +++ b/src/libstore/keys.cc @@ -19,7 +19,7 @@ PublicKeys getDefaultPublicKeys() try { SecretKey secretKey(readFile(secretKeyFile)); publicKeys.emplace(secretKey.name, secretKey.toPublicKey()); - } catch (SysError & e) { + } catch (SystemError & e) { /* Ignore unreadable key files. That's normal in a multi-user installation. */ } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 0f3c37c8a..5a399c8be 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -276,7 +276,7 @@ LocalStore::LocalStore(const Params & params) [[gnu::unused]] auto res2 = ftruncate(fd.get(), settings.reservedSize); } } - } catch (SysError & e) { /* don't care about errors */ + } catch (SystemError & e) { /* don't care about errors */ } /* Acquire the big fat lock in shared mode to make sure that no diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index a494e6ecc..78e4f6d86 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -242,7 +242,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, /* Atomically replace the old file with the new hard link. */ try { renameFile(tempLink, path); - } catch (SysError & e) { + } catch (SystemError & e) { if (unlink(tempLink.c_str()) == -1) printError("unable to unlink '%1%'", tempLink); if (errno == EMLINK) { diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc index 03e57a565..b44edfe89 100644 --- a/src/libstore/remote-fs-accessor.cc +++ b/src/libstore/remote-fs-accessor.cc @@ -87,13 +87,13 @@ std::pair, CanonPath> RemoteFSAccessor::fetch(const CanonPat nars.emplace(storePath.hashPart(), narAccessor); return {narAccessor, restPath}; - } catch (SysError &) { } + } catch (SystemError &) { } try { auto narAccessor = makeNarAccessor(nix::readFile(cacheFile)); nars.emplace(storePath.hashPart(), narAccessor); return {narAccessor, restPath}; - } catch (SysError &) { } + } catch (SystemError &) { } } StringSink sink; diff --git a/src/libutil/args.cc b/src/libutil/args.cc index e2668c673..5187e7396 100644 --- a/src/libutil/args.cc +++ b/src/libutil/args.cc @@ -304,7 +304,7 @@ void RootArgs::parseCmdline(const Strings & _cmdline, bool allowShebang) for (auto pos = savedArgs.begin(); pos != savedArgs.end();pos++) cmdline.push_back(*pos); } - } catch (SysError &) { } + } catch (SystemError &) { } } for (auto pos = cmdline.begin(); pos != cmdline.end(); ) { diff --git a/src/libutil/cgroup.cc b/src/libutil/cgroup.cc index 4c2bf31ff..de83b5ad1 100644 --- a/src/libutil/cgroup.cc +++ b/src/libutil/cgroup.cc @@ -95,7 +95,7 @@ static CgroupStats destroyCgroup(const Path & cgroup, bool returnStats) using namespace std::string_literals; warn("killing stray builder process %d (%s)...", pid, trim(replaceStrings(cmdline, "\0"s, " "))); - } catch (SysError &) { + } catch (SystemError &) { } } // FIXME: pid wraparound diff --git a/src/libutil/config.cc b/src/libutil/config.cc index a3310f4ec..37f5b50c7 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -124,7 +124,7 @@ static void applyConfigInner(const std::string & contents, const std::string & p try { std::string includedContents = readFile(path); applyConfigInner(includedContents, p, parsedContents); - } catch (SysError &) { + } catch (SystemError &) { // TODO: Do we actually want to ignore this? Or is it better to fail? } } else if (!ignoreMissing) { diff --git a/src/libutil/error.hh b/src/libutil/error.hh index 234cbe1f6..764fac1ce 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -178,20 +178,50 @@ MakeError(Error, BaseError); MakeError(UsageError, Error); MakeError(UnimplementedError, Error); -class SysError : public Error +/** + * To use in catch-blocks. + */ +MakeError(SystemError, Error); + +/** + * POSIX system error, created using `errno`, `strerror` friends. + * + * Throw this, but prefer not to catch this, and catch `SystemError` + * instead. This allows implementations to freely switch between this + * and `WinError` without breaking catch blocks. + * + * However, it is permissible to catch this and rethrow so long as + * certain conditions are not met (e.g. to catch only if `errNo = + * EFooBar`). In that case, try to also catch the equivalent `WinError` + * code. + * + * @todo Rename this to `PosixError` or similar. At this point Windows + * support is too WIP to justify the code churn, but if it is finished + * then a better identifier becomes moe worth it. + */ +class SysError : public SystemError { public: int errNo; + /** + * Construct using the explicitly-provided error number. `strerror` + * will be used to try to add additional information to the message. + */ template - SysError(int errNo_, const Args & ... args) - : Error("") + SysError(int errNo, const Args & ... args) + : SystemError(""), errNo(errNo) { - errNo = errNo_; auto hf = hintfmt(args...); err.msg = hintfmt("%1%: %2%", normaltxt(hf.str()), strerror(errNo)); } + /** + * Construct using the ambient `errno`. + * + * Be sure to not perform another `errno`-modifying operation before + * calling this constructor! + */ template SysError(const Args & ... args) : SysError(errno, args ...) @@ -199,7 +229,9 @@ public: } }; -/** Throw an exception for the purpose of checking that exception handling works; see 'initLibUtil()'. +/** + * Throw an exception for the purpose of checking that exception + * handling works; see 'initLibUtil()'. */ void throwExceptionSelfCheck(); diff --git a/src/libutil/file-descriptor.cc b/src/libutil/file-descriptor.cc index 38dd70c8e..692be3383 100644 --- a/src/libutil/file-descriptor.cc +++ b/src/libutil/file-descriptor.cc @@ -231,7 +231,7 @@ void closeMostFDs(const std::set & exceptions) } } return; - } catch (SysError &) { + } catch (SystemError &) { } #endif diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 183aee2dc..d68ddacc0 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -116,7 +116,7 @@ void writeToStderr(std::string_view s) { try { writeFull(STDERR_FILENO, s, false); - } catch (SysError & e) { + } catch (SystemError & e) { /* Ignore failing writes to stderr. We need to ignore write errors to ensure that cleanup code that logs to stderr runs to completion if the other side of stderr has been closed diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 76b378e18..316105603 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -53,7 +53,7 @@ void FdSink::writeUnbuffered(std::string_view data) written += data.size(); try { writeFull(fd, data); - } catch (SysError & e) { + } catch (SystemError & e) { _good = false; throw; } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 7b4b1d031..b23362b5c 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -20,7 +20,7 @@ void initLibUtil() { // When exception handling fails, the message tends to be printed by the // C++ runtime, followed by an abort. // For example on macOS we might see an error such as - // libc++abi: terminating with uncaught exception of type nix::SysError: error: C++ exception handling is broken. This would appear to be a problem with the way Nix was compiled and/or linked and/or loaded. + // libc++abi: terminating with uncaught exception of type nix::SystemError: error: C++ exception handling is broken. This would appear to be a problem with the way Nix was compiled and/or linked and/or loaded. bool caught = false; try { throwExceptionSelfCheck(); diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index ee2addb72..1ad4b387c 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -148,7 +148,7 @@ static void main_nix_build(int argc, char * * argv) args.push_back(word); } } - } catch (SysError &) { } + } catch (SystemError &) { } } struct MyArgs : LegacyArgs, MixEvalArgs diff --git a/src/nix/config-check.cc b/src/nix/config-check.cc index 410feca2f..8d4717e15 100644 --- a/src/nix/config-check.cc +++ b/src/nix/config-check.cc @@ -107,7 +107,7 @@ struct CmdConfigCheck : StoreCommand if (profileDir.find("/profiles/") == std::string::npos) dirs.insert(dir); } - } catch (SysError &) {} + } catch (SystemError &) {} } if (!dirs.empty()) { diff --git a/tests/unit/libutil/logging.cc b/tests/unit/libutil/logging.cc index c6dfe63d3..8950a26d4 100644 --- a/tests/unit/libutil/logging.cc +++ b/tests/unit/libutil/logging.cc @@ -73,7 +73,7 @@ namespace nix { } - TEST(logEI, picksUpSysErrorExitCode) { + TEST(logEI, picksUpSystemErrorExitCode) { MakeError(TestError, Error); ErrorInfo::programName = std::optional("error-unit-test"); @@ -81,12 +81,12 @@ namespace nix { try { auto x = readFile(-1); } - catch (SysError &e) { + catch (SystemError &e) { testing::internal::CaptureStderr(); logError(e.info()); auto str = testing::internal::GetCapturedStderr(); - ASSERT_STREQ(str.c_str(), "\x1B[31;1merror:\x1B[0m\x1B[34;1m --- SysError --- error-unit-test\x1B[0m\nstatting file: \x1B[33;1mBad file descriptor\x1B[0m\n"); + ASSERT_STREQ(str.c_str(), "\x1B[31;1merror:\x1B[0m\x1B[34;1m --- SystemError --- error-unit-test\x1B[0m\nstatting file: \x1B[33;1mBad file descriptor\x1B[0m\n"); } }