From ce6eba531edda8b033f667ec0449f4a533326cb6 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 1 Jul 2025 17:06:26 +0200 Subject: [PATCH] libutil: add makeNonBlocking, resetNonBlocking these are used often enough that deduplicating them is worth it. we do lose some error fidelity, but valid fds will never cause an error here Change-Id: I2b91b4848f546a894a2a6c2d36c32a892fb73c9f --- lix/libstore/build/derivation-goal.cc | 9 ++----- lix/libstore/gc.cc | 10 +++++--- lix/libstore/remote-store.cc | 12 +++------ lix/libutil/async-io.cc | 15 ++++-------- lix/libutil/async-io.hh | 3 ++- lix/libutil/compression.cc | 12 ++------- lix/libutil/file-descriptor.cc | 35 ++++++++++++++++++++++----- lix/libutil/file-descriptor.hh | 17 +++++++++++++ 8 files changed, 66 insertions(+), 47 deletions(-) diff --git a/lix/libstore/build/derivation-goal.cc b/lix/libstore/build/derivation-goal.cc index 48d38ffb4..a7edba697 100644 --- a/lix/libstore/build/derivation-goal.cc +++ b/lix/libstore/build/derivation-goal.cc @@ -1,5 +1,6 @@ #include "lix/libstore/build/derivation-goal.hh" #include "lix/libutil/async.hh" +#include "lix/libutil/file-descriptor.hh" #include "lix/libutil/file-system.hh" #include "lix/libstore/build/hook-instance.hh" #include "lix/libstore/build/worker.hh" @@ -1204,13 +1205,7 @@ struct DerivationGoal::InputStream final : private kj::AsyncObject : fd(fd) , observer(ep, fd, kj::UnixEventPort::FdObserver::OBSERVE_READ) { - int flags = fcntl(fd, F_GETFL); - if (flags < 0) { - throw SysError("fcntl(F_GETFL) failed on fd %i", fd); - } - if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) < 0) { - throw SysError("fcntl(F_SETFL) failed on fd %i", fd); - } + makeNonBlocking(fd); } kj::Promise read(kj::ArrayPtr buffer) diff --git a/lix/libstore/gc.cc b/lix/libstore/gc.cc index 6a9f679b8..d46de9d84 100644 --- a/lix/libstore/gc.cc +++ b/lix/libstore/gc.cc @@ -3,6 +3,7 @@ #include "lix/libstore/pathlocks.hh" #include "lix/libutil/async.hh" #include "lix/libutil/error.hh" +#include "lix/libutil/file-descriptor.hh" #include "lix/libutil/processes.hh" #include "lix/libutil/result.hh" #include "lix/libutil/signals.hh" @@ -456,9 +457,7 @@ class GCOperation { createDirs(dirOf(socketPath)); fdServer = createUnixDomainSocket(socketPath, 0666); - if (fcntl(fdServer.get(), F_SETFL, fcntl(fdServer.get(), F_GETFL) | O_NONBLOCK) == -1) { - throw SysError("making socket '%1%' non-blocking", socketPath); - } + makeNonBlocking(fdServer.get()); serverThread = std::thread([this]() { setCurrentThreadName("gc server"); @@ -547,8 +546,11 @@ void GCOperation::runServerThread() /* On macOS, accepted sockets inherit the non-blocking flag from the server socket, so explicitly make it blocking. */ - if (fcntl(fdClient.get(), F_SETFL, fcntl(fdClient.get(), F_GETFL) & ~O_NONBLOCK) == -1) + try { + makeBlocking(fdClient.get()); + } catch (...) { abort(); + } while (true) { try { diff --git a/lix/libstore/remote-store.cc b/lix/libstore/remote-store.cc index 54adefe33..db4b9d7cf 100644 --- a/lix/libstore/remote-store.cc +++ b/lix/libstore/remote-store.cc @@ -3,6 +3,7 @@ #include "lix/libutil/async.hh" #include "lix/libutil/box_ptr.hh" #include "lix/libutil/error.hh" +#include "lix/libutil/file-descriptor.hh" #include "lix/libutil/result.hh" #include "lix/libutil/serialise.hh" #include "lix/libutil/signals.hh" @@ -784,15 +785,8 @@ try { // SAFETY NOTE: while we're running we own the executor, and thus the stream. // setting these flags is unsafe if the stream is shared with another thread. - const int oldFlags = fcntl(getFD(), F_GETFL); - if (oldFlags == -1 || fcntl(getFD(), F_SETFL, oldFlags & ~O_NONBLOCK) < 0) { - throw SysError("making connection blocking"); - } - KJ_DEFER({ - if (fcntl(getFD(), F_SETFL, oldFlags) < 0) { - throw SysError("restoring connection flags"); - } - }); + const auto oldState = makeBlocking(getFD()); + KJ_DEFER(resetBlockingState(getFD(), oldState)); FdSource from{getFD(), fromBuf}; from.specialEndOfFileError = "Nix daemon disconnected while waiting for a response"; diff --git a/lix/libutil/async-io.cc b/lix/libutil/async-io.cc index 9028c2070..d4f680e0a 100644 --- a/lix/libutil/async-io.cc +++ b/lix/libutil/async-io.cc @@ -148,22 +148,17 @@ AsyncFdIoStream::AsyncFdIoStream(AutoCloseFD fd) : AsyncFdIoStream(shared_fd{}, AsyncFdIoStream::AsyncFdIoStream(shared_fd, int fd) : fd(fd) + , oldState(makeNonBlocking(fd)) , observer(AIO().unixEventPort, fd, kj::UnixEventPort::FdObserver::OBSERVE_READ_WRITE) { - oldFlags = fcntl(fd, F_GETFL, 0); - if (oldFlags == -1 || fcntl(fd, F_SETFL, oldFlags | O_NONBLOCK)) { - throw SysError("making file descriptor non-blocking"); - } } AsyncFdIoStream::~AsyncFdIoStream() noexcept(false) { - if (fcntl(fd, F_SETFL, oldFlags)) { - try { - throw SysError("restoring file descriptor flags"); - } catch (...) { - ignoreExceptionInDestructor(); - } + try { + resetBlockingState(fd, oldState); + } catch (...) { + ignoreExceptionInDestructor(); } } diff --git a/lix/libutil/async-io.hh b/lix/libutil/async-io.hh index 31ccec42a..e2074a402 100644 --- a/lix/libutil/async-io.hh +++ b/lix/libutil/async-io.hh @@ -161,7 +161,8 @@ class AsyncStream : public AsyncInputStream, public AsyncOutputStream class AsyncFdIoStream : public AsyncStream { - int fd, oldFlags; + int fd; + FdBlockingState oldState; AutoCloseFD ownedFd; // only for closing automatically, must equal fd if set kj::UnixEventPort::FdObserver observer; diff --git a/lix/libutil/compression.cc b/lix/libutil/compression.cc index aa751700f..3e2dfbf87 100644 --- a/lix/libutil/compression.cc +++ b/lix/libutil/compression.cc @@ -298,16 +298,8 @@ struct DecompressionStream : DecompressorPipes, AsyncInputStream DecompressionStream(const std::string & method, box_ptr inner) : inner(std::move(inner)) { - if (auto flags = fcntl(compressed.writeSide.get(), F_GETFL); - flags == -1 || fcntl(compressed.writeSide.get(), F_SETFL, flags | O_NONBLOCK)) - { - throw SysError("setting up decompression stream"); - } - if (auto flags = fcntl(uncompressed.readSide.get(), F_GETFL); - flags == -1 || fcntl(uncompressed.readSide.get(), F_SETFL, flags | O_NONBLOCK)) - { - throw SysError("setting up decompression stream"); - } + makeNonBlocking(compressed.writeSide.get()); + makeNonBlocking(uncompressed.readSide.get()); source = std::make_unique(compressed.readSide.get()); sink = std::make_unique(uncompressed.writeSide.get()); diff --git a/lix/libutil/file-descriptor.cc b/lix/libutil/file-descriptor.cc index 83496b6a6..7edec8f49 100644 --- a/lix/libutil/file-descriptor.cc +++ b/lix/libutil/file-descriptor.cc @@ -1,3 +1,4 @@ +#include "file-descriptor.hh" #include "lix/libutil/charptr-cast.hh" #include "lix/libutil/file-system.hh" #include "lix/libutil/finally.hh" @@ -89,18 +90,15 @@ std::string drainFD(int fd, bool block, const size_t reserveSize) Generator drainFDSource(int fd, bool block) { // silence GCC maybe-uninitialized warning in finally - int saved = 0; + FdBlockingState saved{}; if (!block) { - saved = fcntl(fd, F_GETFL); - if (fcntl(fd, F_SETFL, saved | O_NONBLOCK) == -1) - throw SysError("making file descriptor non-blocking"); + saved = makeNonBlocking(fd); } Finally finally([&]() { if (!block) { - if (fcntl(fd, F_SETFL, saved) == -1) - throw SysError("making file descriptor blocking"); + resetBlockingState(fd, saved); } }); @@ -294,4 +292,29 @@ void closeOnExec(int fd) throw SysError("setting close-on-exec flag"); } +FdBlockingState makeNonBlocking(int fd) +{ + const auto oldFlags = fcntl(fd, F_GETFL); + if (oldFlags < 0 || fcntl(fd, F_SETFL, oldFlags | O_NONBLOCK) == -1) { + throw SysError("makeNonBlocking"); + } + return FdBlockingState(oldFlags & O_NONBLOCK); +} + +FdBlockingState makeBlocking(int fd) +{ + const auto oldFlags = fcntl(fd, F_GETFL); + if (fcntl(fd, F_SETFL, oldFlags & ~O_NONBLOCK) == -1) { + throw SysError("makeBlocking"); + } + return FdBlockingState(oldFlags & O_NONBLOCK); +} + +void resetBlockingState(int fd, FdBlockingState prevState) +{ + const auto oldFlags = fcntl(fd, F_GETFL); + if (oldFlags < 0 || fcntl(fd, F_SETFL, (oldFlags & ~O_NONBLOCK) | int(prevState)) == -1) { + throw SysError("resetBlockingState"); + } +} } diff --git a/lix/libutil/file-descriptor.hh b/lix/libutil/file-descriptor.hh index 6c1c698fc..77e1239ea 100644 --- a/lix/libutil/file-descriptor.hh +++ b/lix/libutil/file-descriptor.hh @@ -94,6 +94,23 @@ void closeExtraFDs(); */ void closeOnExec(int fd); +enum class FdBlockingState : int {}; + +/** + * Make the given file descriptor non-blocking. Returns the old flag set; this + * can be passed to resetBlockingState to return the fd to its original state. + */ +FdBlockingState makeNonBlocking(int fd); +/** + * Make the given file descriptor blocking. Returns the old flag set; it can + * be passed to `resetBlockingState` to return the fd to its original state. + */ +FdBlockingState makeBlocking(int fd); +/** + * Undo a `makeNonBlocking` or `makeBlocking` call. + */ +void resetBlockingState(int fd, FdBlockingState prevState); + MakeError(EndOfFile, Error); }