From 5a34a473dd14200ffeff496f0e8e4518f7992765 Mon Sep 17 00:00:00 2001 From: Guillaume Bouchard Date: Wed, 29 Apr 2020 13:10:03 +0200 Subject: [PATCH 1/6] builtins.readFile: do not truncate content This closes #3026 by allowing `builtins.readFile` to read a file with a wrongly reported file size, for example, files in `/proc` may report a file size of 0. Reading file in `/proc` is not a good enough motivation, however I do think it just makes nix more robust by allowing more file to be read. Especially, I do considerer the previous behavior to be dangerous because nix was previously reading truncated files. Examples of file system which incorrectly report file size may be network file system or dynamic file system (for performance reason, a dynamic file system such as FUSE may generate the content of the file on demand). ``` nix-repl> builtins.readFile "/proc/version" "" ``` With this commit: ``` nix-repl> builtins.readFile "/proc/version" "Linux version 5.6.7 (nixbld@localhost) (gcc version 9.3.0 (GCC)) #1-NixOS SMP Thu Apr 23 08:38:27 UTC 2020\n" ``` Here is a summary of the behavior changes: - If the reported size is smaller, previous implementation was silently returning a truncated file content. The new implementation is returning the correct file content. - If a file had a bigger reported file size, previous implementation was failing with an exception, but the new implementation is returning the correct file content. This change of behavior is coherent with this pull request. Open questions - The behavior is unchanged for correctly reported file size, however performances may vary because it uses the more complex sink interface. Considering that sink is used a lot, I don't think this impacts the performance a lot. - `builtins.readFile` on an infinite file, such as `/dev/random` may fill the memory. - it does not support adding file to store, such as `${/proc/version}`. --- src/libutil/util.cc | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 615a7656c..d001bc3b7 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -312,14 +312,7 @@ unsigned char getFileType(const Path & path) string readFile(int fd) { - struct stat st; - if (fstat(fd, &st) == -1) - throw SysError("statting file"); - - std::vector buf(st.st_size); - readFull(fd, buf.data(), st.st_size); - - return string((char *) buf.data(), st.st_size); + return drainFD(fd, true); } From 7afcb5af988eb0ce73c9916b809f8528dfb14c0f Mon Sep 17 00:00:00 2001 From: Guillaume Bouchard Date: Wed, 29 Apr 2020 18:42:19 +0200 Subject: [PATCH 2/6] Remove the `drain` argument from `readFile` Now it is always `drain` (see previous commit). --- src/libstore/gc.cc | 4 ++-- src/libutil/serialise.hh | 3 +++ src/libutil/util.cc | 4 ++-- src/libutil/util.hh | 2 +- src/nix/ls.cc | 2 +- 5 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 6bab1e37c..3cd35c945 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -419,7 +419,7 @@ void LocalStore::findRuntimeRoots(Roots & roots, bool censor) try { auto mapFile = fmt("/proc/%s/maps", ent->d_name); - auto mapLines = tokenizeString>(readFile(mapFile, true), "\n"); + auto mapLines = tokenizeString>(readFile(mapFile), "\n"); for (const auto & line : mapLines) { auto match = std::smatch{}; if (std::regex_match(line, match, mapRegex)) @@ -427,7 +427,7 @@ void LocalStore::findRuntimeRoots(Roots & roots, bool censor) } auto envFile = fmt("/proc/%s/environ", ent->d_name); - auto envString = readFile(envFile, true); + auto envString = readFile(envFile); 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); diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index 5780c93a6..a04118512 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -148,6 +148,9 @@ struct StringSink : Sink { ref s; StringSink() : s(make_ref()) { }; + explicit StringSink(const size_t reservedSize) : s(make_ref()) { + s->reserve(reservedSize); + }; StringSink(ref s) : s(s) { }; void operator () (const unsigned char * data, size_t len) override; }; diff --git a/src/libutil/util.cc b/src/libutil/util.cc index d001bc3b7..bb4af747b 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -316,12 +316,12 @@ string readFile(int fd) } -string readFile(const Path & path, bool drain) +string readFile(const Path & path) { AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC); if (!fd) throw SysError(format("opening file '%1%'") % path); - return drain ? drainFD(fd.get()) : readFile(fd.get()); + return readFile(fd.get()); } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index d7ee62bcc..32ef9a79a 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -103,7 +103,7 @@ unsigned char getFileType(const Path & path); /* Read the contents of a file into a string. */ string readFile(int fd); -string readFile(const Path & path, bool drain = false); +string readFile(const Path & path); void readFile(const Path & path, Sink & sink); /* Write a string to a file. */ diff --git a/src/nix/ls.cc b/src/nix/ls.cc index 8590199d7..ee308a6ec 100644 --- a/src/nix/ls.cc +++ b/src/nix/ls.cc @@ -136,7 +136,7 @@ struct CmdLsNar : Command, MixLs void run() override { - list(makeNarAccessor(make_ref(readFile(narPath, true)))); + list(makeNarAccessor(make_ref(readFile(narPath)))); } }; From 2e5be2a7495ac0b204454c74664e590a38d039d3 Mon Sep 17 00:00:00 2001 From: Guillaume Bouchard Date: Wed, 29 Apr 2020 18:44:01 +0200 Subject: [PATCH 3/6] StringSink pre allocate When used with `readFile`, we have a pretty good heuristic of the file size, so `reserve` this in the `string`. This will save some allocation / copy when the string is growing. --- src/libutil/util.cc | 10 +++++++--- src/libutil/util.hh | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index bb4af747b..71db92d77 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -312,7 +312,11 @@ unsigned char getFileType(const Path & path) string readFile(int fd) { - return drainFD(fd, true); + struct stat st; + if (fstat(fd, &st) == -1) + throw SysError("statting file"); + + return drainFD(fd, true, st.st_size); } @@ -658,9 +662,9 @@ void writeFull(int fd, const string & s, bool allowInterrupts) } -string drainFD(int fd, bool block) +string drainFD(int fd, bool block, const size_t reserveSize) { - StringSink sink; + StringSink sink(reserveSize); drainFD(fd, sink, block); return std::move(*sink.s); } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 32ef9a79a..1b263abcc 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -162,7 +162,7 @@ MakeError(EndOfFile, Error); /* Read a file descriptor until EOF occurs. */ -string drainFD(int fd, bool block = true); +string drainFD(int fd, bool block = true, const size_t reserveSize=0); void drainFD(int fd, Sink & sink, bool block = true); From 2aeb874e832ae4c326f1115e55fdcaf7c2f4e916 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Fri, 1 May 2020 23:32:01 +0200 Subject: [PATCH 4/6] Improve help-message for nix-repl * Remove obsolete `printHelp` function * Add an example to demonstrate how to list all available commands within the REPL --- src/nix/repl.cc | 44 ++++++++++---------------------------------- 1 file changed, 10 insertions(+), 34 deletions(-) diff --git a/src/nix/repl.cc b/src/nix/repl.cc index 27727bd25..ea8ff1553 100644 --- a/src/nix/repl.cc +++ b/src/nix/repl.cc @@ -82,40 +82,6 @@ struct NixRepl : gc }; -void printHelp() -{ - std::cout - << "Usage: nix-repl [--help] [--version] [-I path] paths...\n" - << "\n" - << "nix-repl is a simple read-eval-print loop (REPL) for the Nix package manager.\n" - << "\n" - << "Options:\n" - << " --help\n" - << " Prints out a summary of the command syntax and exits.\n" - << "\n" - << " --version\n" - << " Prints out the Nix version number on standard output and exits.\n" - << "\n" - << " -I path\n" - << " Add a path to the Nix expression search path. This option may be given\n" - << " multiple times. See the NIX_PATH environment variable for information on\n" - << " the semantics of the Nix search path. Paths added through -I take\n" - << " precedence over NIX_PATH.\n" - << "\n" - << " paths...\n" - << " A list of paths to files containing Nix expressions which nix-repl will\n" - << " load and add to its scope.\n" - << "\n" - << " A path surrounded in < and > will be looked up in the Nix expression search\n" - << " path, as in the Nix language itself.\n" - << "\n" - << " If an element of paths starts with http:// or https://, it is interpreted\n" - << " as the URL of a tarball that will be downloaded and unpacked to a temporary\n" - << " location. The tarball must include a single top-level directory containing\n" - << " at least a file named default.nix.\n"; -} - - string removeWhitespace(string s) { s = chomp(s); @@ -809,6 +775,16 @@ struct CmdRepl : StoreCommand, MixEvalArgs return "start an interactive environment for evaluating Nix expressions"; } + Examples examples() override + { + return { + Example{ + "Display all special commands within the REPL:", + "nix repl\n nix-repl> :?" + } + }; + } + void run(ref store) override { auto repl = std::make_unique(searchPath, openStore()); From e2fc575c6122b40f5d660ae04f269e118354c101 Mon Sep 17 00:00:00 2001 From: Jude Taylor Date: Mon, 4 May 2020 14:42:06 -0700 Subject: [PATCH 5/6] nix auto-gc: use fragment size --- src/libstore/gc.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 6bab1e37c..d210defe5 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -889,7 +889,7 @@ void LocalStore::autoGC(bool sync) if (statvfs(realStoreDir.c_str(), &st)) throw SysError("getting filesystem info about '%s'", realStoreDir); - return (uint64_t) st.f_bavail * st.f_bsize; + return (uint64_t) st.f_bavail * st.f_frsize; }; std::shared_future future; From fd4911269f9a9ce0f9647c5fb88658074f654eff Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 6 May 2020 10:54:01 +0200 Subject: [PATCH 6/6] Revert "Merge pull request #3558 from LnL7/ssh-ng-stderr" This reverts commit 3ebfbecdd187002569257f7cb183bf9e0b39af1e, reversing changes made to c089c52d5f1cff888552f485775b74226dcbe618. https://github.com/NixOS/nix/pull/3558 --- src/libstore/remote-store.cc | 6 ++---- src/libutil/logging.cc | 10 ---------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 07ef79382..8c55da268 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -779,10 +779,8 @@ std::exception_ptr RemoteStore::Connection::processStderr(Sink * sink, Source * return std::make_exception_ptr(Error(status, error)); } - else if (msg == STDERR_NEXT) { - string s = chomp(readString(from)); - printMsg(lvlVomit, "stderr %s", s); - } + else if (msg == STDERR_NEXT) + printError(chomp(readString(from))); else if (msg == STDERR_START_ACTIVITY) { auto act = readNum(from); diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 777650de5..3cc4ef8f1 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -63,16 +63,6 @@ public: writeToStderr(prefix + filterANSIEscapes(fs.s, !tty) + "\n"); } - void result(ActivityId act, ResultType type, const std::vector & fields) override - { - if (type == resBuildLogLine || type == resPostBuildLogLine) { - assert(0 < fields.size()); - assert(fields[0].type == Logger::Field::tString); - auto lastLine = fields[0].s; - log(lvlInfo, lastLine); - } - } - void startActivity(ActivityId act, Verbosity lvl, ActivityType type, const std::string & s, const Fields & fields, ActivityId parent) override