From ca1dc3f70bf98e2424b7b2666ee2180675b67451 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Wed, 11 Sep 2024 00:27:39 -0700 Subject: [PATCH] archive: refactor bad mutable-state API in the NAR parse listener Remove the mutable state stuff that assumes that one file is being written a time. It's true that we don't write multiple files interleaved, but that mutable state is evil. Change-Id: Ia1481da48255d901e4b09a9b783e7af44fae8cff --- src/libstore/nar-accessor.cc | 30 ++++----- src/libstore/store-api.cc | 23 +++++-- src/libutil/archive.cc | 114 ++++++++++++++++++++--------------- src/libutil/archive.hh | 39 +++++++++--- 4 files changed, 127 insertions(+), 79 deletions(-) diff --git a/src/libstore/nar-accessor.cc b/src/libstore/nar-accessor.cc index fa7d5e3cb..7600de6e7 100644 --- a/src/libstore/nar-accessor.cc +++ b/src/libstore/nar-accessor.cc @@ -45,11 +45,12 @@ struct NarAccessor : public FSAccessor uint64_t pos = 0; + public: NarIndexer(NarAccessor & acc, Source & source) : acc(acc), source(source) { } - void createMember(const Path & path, NarMember member) + NarMember & createMember(const Path & path, NarMember member) { size_t level = std::count(path.begin(), path.end(), '/'); while (parents.size() > level) parents.pop(); @@ -63,6 +64,8 @@ struct NarAccessor : public FSAccessor auto result = parents.top()->children.emplace(baseNameOf(path), std::move(member)); parents.push(&result.first->second); } + + return *parents.top(); } void createDirectory(const Path & path) override @@ -70,28 +73,17 @@ struct NarAccessor : public FSAccessor createMember(path, {FSAccessor::Type::tDirectory, false, 0, 0}); } - void createRegularFile(const Path & path) override + std::unique_ptr createRegularFile(const Path & path, uint64_t size, bool executable) override { - createMember(path, {FSAccessor::Type::tRegular, false, 0, 0}); - } + auto & memb = createMember(path, {FSAccessor::Type::tRegular, false, 0, 0}); - void closeRegularFile() override - { } - - void isExecutable() override - { - parents.top()->isExecutable = true; - } - - void preallocateContents(uint64_t size) override - { assert(size <= std::numeric_limits::max()); - parents.top()->size = (uint64_t) size; - parents.top()->start = pos; - } + memb.size = (uint64_t) size; + memb.start = pos; + memb.isExecutable = executable; - void receiveContents(std::string_view data) override - { } + return std::make_unique(); + } void createSymlink(const Path & path, const std::string & target) override { diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 1619e5062..ed8657774 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -386,17 +386,28 @@ namespace { */ struct RetrieveRegularNARVisitor : NARParseVisitor { + struct MyFileHandle : public FileHandle + { + Sink & sink; + + void receiveContents(std::string_view data) override + { + sink(data); + } + + private: + MyFileHandle(Sink & sink) : sink(sink) {} + + friend struct RetrieveRegularNARVisitor; + }; + Sink & sink; RetrieveRegularNARVisitor(Sink & sink) : sink(sink) { } - void createRegularFile(const Path & path) override + std::unique_ptr createRegularFile(const Path & path, uint64_t size, bool executable) override { - } - - void receiveContents(std::string_view data) override - { - sink(data); + return std::unique_ptr(new MyFileHandle{sink}); } void createDirectory(const Path & path) override diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index 78d49026a..225483804 100644 --- a/src/libutil/archive.cc +++ b/src/libutil/archive.cc @@ -347,16 +347,13 @@ static WireFormatGenerator restore(NARParseVisitor & sink, Generator }, [&](nar::File f) { return [](auto f, auto & sink) -> WireFormatGenerator { - sink.createRegularFile(f.path); - sink.preallocateContents(f.size); - if (f.executable) { - sink.isExecutable(); - } + auto handle = sink.createRegularFile(f.path, f.size, f.executable); + while (auto block = f.contents.next()) { - sink.receiveContents(std::string_view{block->data(), block->size()}); + handle->receiveContents(std::string_view{block->data(), block->size()}); co_yield *block; } - sink.closeRegularFile(); + handle->close(); }(std::move(f), sink); }, [&](nar::Symlink sl) { @@ -422,8 +419,67 @@ void parseDump(NARParseVisitor & sink, Source & source) struct NARRestoreVisitor : NARParseVisitor { Path dstPath; - AutoCloseFD fd; +private: + class MyFileHandle : public FileHandle + { + AutoCloseFD fd; + + MyFileHandle(AutoCloseFD && fd, uint64_t size, bool executable) : FileHandle(), fd(std::move(fd)) + { + if (executable) { + makeExecutable(); + } + + maybePreallocateContents(size); + } + + void makeExecutable() + { + struct stat st; + if (fstat(fd.get(), &st) == -1) + throw SysError("fstat"); + if (fchmod(fd.get(), st.st_mode | (S_IXUSR | S_IXGRP | S_IXOTH)) == -1) + throw SysError("fchmod"); + } + + void maybePreallocateContents(uint64_t len) + { + if (!archiveSettings.preallocateContents) + return; + +#if HAVE_POSIX_FALLOCATE + if (len) { + errno = posix_fallocate(fd.get(), 0, len); + /* Note that EINVAL may indicate that the underlying + filesystem doesn't support preallocation (e.g. on + OpenSolaris). Since preallocation is just an + optimisation, ignore it. */ + if (errno && errno != EINVAL && errno != EOPNOTSUPP && errno != ENOSYS) + throw SysError("preallocating file of %1% bytes", len); + } +#endif + } + + public: + + ~MyFileHandle() = default; + + virtual void close() override + { + /* Call close explicitly to make sure the error is checked */ + fd.close(); + } + + void receiveContents(std::string_view data) override + { + writeFull(fd.get(), data); + } + + friend struct NARRestoreVisitor; + }; + +public: void createDirectory(const Path & path) override { Path p = dstPath + path; @@ -431,49 +487,13 @@ struct NARRestoreVisitor : NARParseVisitor throw SysError("creating directory '%1%'", p); }; - void createRegularFile(const Path & path) override + std::unique_ptr createRegularFile(const Path & path, uint64_t size, bool executable) override { Path p = dstPath + path; - fd = AutoCloseFD{open(p.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0666)}; + AutoCloseFD fd = AutoCloseFD{open(p.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0666)}; if (!fd) throw SysError("creating file '%1%'", p); - } - void closeRegularFile() override - { - /* Call close explicitly to make sure the error is checked */ - fd.close(); - } - - void isExecutable() override - { - struct stat st; - if (fstat(fd.get(), &st) == -1) - throw SysError("fstat"); - if (fchmod(fd.get(), st.st_mode | (S_IXUSR | S_IXGRP | S_IXOTH)) == -1) - throw SysError("fchmod"); - } - - void preallocateContents(uint64_t len) override - { - if (!archiveSettings.preallocateContents) - return; - -#if HAVE_POSIX_FALLOCATE - if (len) { - errno = posix_fallocate(fd.get(), 0, len); - /* Note that EINVAL may indicate that the underlying - filesystem doesn't support preallocation (e.g. on - OpenSolaris). Since preallocation is just an - optimisation, ignore it. */ - if (errno && errno != EINVAL && errno != EOPNOTSUPP && errno != ENOSYS) - throw SysError("preallocating file of %1% bytes", len); - } -#endif - } - - void receiveContents(std::string_view data) override - { - writeFull(fd.get(), data); + return std::unique_ptr(new MyFileHandle(std::move(fd), size, executable)); } void createSymlink(const Path & path, const std::string & target) override diff --git a/src/libutil/archive.hh b/src/libutil/archive.hh index 5e8db4c53..c633bee00 100644 --- a/src/libutil/archive.hh +++ b/src/libutil/archive.hh @@ -83,15 +83,40 @@ WireFormatGenerator dumpString(std::string_view s); */ struct NARParseVisitor { - virtual void createDirectory(const Path & path) { }; + /** + * A type-erased file handle specific to this particular NARParseVisitor. + */ + struct FileHandle + { + FileHandle() {} + FileHandle(FileHandle const &) = delete; + FileHandle & operator=(FileHandle &) = delete; - virtual void createRegularFile(const Path & path) { }; - virtual void closeRegularFile() { }; - virtual void isExecutable() { }; - virtual void preallocateContents(uint64_t size) { }; - virtual void receiveContents(std::string_view data) { }; + /** Puts one block of data into the file */ + virtual void receiveContents(std::string_view data) { } - virtual void createSymlink(const Path & path, const std::string & target) { }; + /** + * Explicitly closes the file. Further operations may throw an assert. + * This exists so that closing can fail and throw an exception without doing so in a destructor. + */ + virtual void close() { } + + virtual ~FileHandle() = default; + }; + + virtual void createDirectory(const Path & path) { } + + /** + * Creates a regular file in the extraction output with the given size and executable flag. + * The size is guaranteed to be the true size of the file. + */ + [[nodiscard]] + virtual std::unique_ptr createRegularFile(const Path & path, uint64_t size, bool executable) + { + return std::make_unique(); + } + + virtual void createSymlink(const Path & path, const std::string & target) { } }; namespace nar {