libutil: make AutoCloseFD a better resource

add a reset() method to close the wrapped fd instead of assigning magic
constants. also make the from-fd constructor explicit so you can't
accidentally assign the *wrong* magic constant, or even an unrelated
integer that also just happens to be an fd by pure chance.

Change-Id: I51311b0f6e040240886b5103d39d1794a6acc325

src/build-remote/build-remote.cc

@@ -126,7 +126,7 @@ static int main_build_remote(int argc, char * * argv)
             mkdir(currentLoad.c_str(), 0777);
 
             while (true) {
-                bestSlotLock = -1;
+                bestSlotLock.reset();
                 AutoCloseFD lock = openLockFile(currentLoad + "/main-lock", true);
                 lockFile(lock.get(), ltWrite, true);
 
@@ -229,7 +229,7 @@ static int main_build_remote(int argc, char * * argv)
                 futimens(bestSlotLock.get(), NULL);
 #endif
 
-                lock = -1;
+                lock.reset();
 
                 try {
 
@@ -282,7 +282,7 @@ connected:
             copyPaths(*store, *sshStore, store->parseStorePathSet(inputs), NoRepair, NoCheckSigs, substitute);
         }
 
-        uploadLock = -1;
+        uploadLock.reset();
 
         auto drv = store->readDerivation(*drvPath);
 

src/libstore/binary-cache-store.cc

@@ -124,7 +124,7 @@ void BinaryCacheStore::writeNarInfo(ref<NarInfo> narInfo)
 
 AutoCloseFD openFile(const Path & path)
 {
-    auto fd = open(path.c_str(), O_RDONLY | O_CLOEXEC);
+    AutoCloseFD fd{open(path.c_str(), O_RDONLY | O_CLOEXEC)};
     if (!fd)
         throw SysError("opening file '%1%'", path);
     return fd;

src/libstore/build/derivation-goal.cc

@@ -846,8 +846,8 @@ int DerivationGoal::getChildStatus()
 
 void DerivationGoal::closeReadPipes()
 {
-    hook->builderOut.readSide = -1;
+    hook->builderOut.readSide.reset();
-    hook->fromHook.readSide = -1;
+    hook->fromHook.readSide.reset();
 }
 
 
@@ -1227,7 +1227,7 @@ HookReply DerivationGoal::tryBuildHook()
     }
 
     hook->sink = FdSink();
-    hook->toHook.writeSide = -1;
+    hook->toHook.writeSide.reset();
 
     /* Create the log file and pipe. */
     Path logFile = openLogFile();
@@ -1273,7 +1273,7 @@ Path DerivationGoal::openLogFile()
     Path logFileName = fmt("%s/%s%s", dir, baseName.substr(2),
         settings.compressLog ? ".bz2" : "");
 
-    fdLogFile = open(logFileName.c_str(), O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC, 0666);
+    fdLogFile = AutoCloseFD{open(logFileName.c_str(), O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC, 0666)};
     if (!fdLogFile) throw SysError("creating log file '%1%'", logFileName);
 
     logFileSink = std::make_shared<FdSink>(fdLogFile.get());
@@ -1293,7 +1293,7 @@ void DerivationGoal::closeLogFile()
     if (logSink2) logSink2->finish();
     if (logFileSink) logFileSink->flush();
     logSink = logFileSink = 0;
-    fdLogFile = -1;
+    fdLogFile.reset();
 }
 
 

src/libstore/build/hook-instance.cc

@@ -61,8 +61,8 @@ HookInstance::HookInstance()
     });
 
     pid.setSeparatePG(true);
-    fromHook.writeSide = -1;
+    fromHook.writeSide.reset();
-    toHook.readSide = -1;
+    toHook.readSide.reset();
 
     sink = FdSink(toHook.writeSide.get());
     std::map<std::string, Config::SettingInfo> settings;
@@ -76,7 +76,7 @@ HookInstance::HookInstance()
 HookInstance::~HookInstance()
 {
     try {
-        toHook.writeSide = -1;
+        toHook.writeSide.reset();
         if (pid != -1) pid.kill();
     } catch (...) {
         ignoreException();

src/libstore/build/local-derivation-goal.cc

@@ -309,8 +309,8 @@ void LocalDerivationGoal::cleanupHookFinally()
 
 void LocalDerivationGoal::cleanupPreChildKill()
 {
-    sandboxMountNamespace = -1;
+    sandboxMountNamespace.reset();
-    sandboxUserNamespace = -1;
+    sandboxUserNamespace.reset();
 }
 
 
@@ -807,7 +807,7 @@ void LocalDerivationGoal::startBuilder()
     Path logFile = openLogFile();
 
     /* Create a pseudoterminal to get the output of the builder. */
-    builderOut = posix_openpt(O_RDWR | O_NOCTTY);
+    builderOut = AutoCloseFD{posix_openpt(O_RDWR | O_NOCTTY)};
     if (!builderOut)
         throw SysError("opening pseudoterminal master");
 
@@ -834,7 +834,7 @@ void LocalDerivationGoal::startBuilder()
     /* Open the slave side of the pseudoterminal and use it as stderr. */
     auto openSlave = [&]()
     {
-        AutoCloseFD builderOut = open(slaveName.c_str(), O_RDWR | O_NOCTTY);
+        AutoCloseFD builderOut{open(slaveName.c_str(), O_RDWR | O_NOCTTY)};
         if (!builderOut)
             throw SysError("opening pseudoterminal slave");
 
@@ -937,12 +937,12 @@ void LocalDerivationGoal::startBuilder()
         if (helper.wait() != 0)
             throw Error("unable to start build process");
 
-        userNamespaceSync.readSide = -1;
+        userNamespaceSync.readSide.reset();
 
         /* Close the write side to prevent runChild() from hanging
            reading from this. */
         Finally cleanup([&]() {
-            userNamespaceSync.writeSide = -1;
+            userNamespaceSync.writeSide.reset();
         });
 
         auto ss = tokenizeString<std::vector<std::string>>(readLine(sendPid.readSide.get()));
@@ -981,12 +981,12 @@ void LocalDerivationGoal::startBuilder()
 
         /* Save the mount- and user namespace of the child. We have to do this
            *before* the child does a chroot. */
-        sandboxMountNamespace = open(fmt("/proc/%d/ns/mnt", (pid_t) pid).c_str(), O_RDONLY);
+        sandboxMountNamespace = AutoCloseFD{open(fmt("/proc/%d/ns/mnt", (pid_t) pid).c_str(), O_RDONLY)};
         if (sandboxMountNamespace.get() == -1)
             throw SysError("getting sandbox mount namespace");
 
         if (usingUserNamespace) {
-            sandboxUserNamespace = open(fmt("/proc/%d/ns/user", (pid_t) pid).c_str(), O_RDONLY);
+            sandboxUserNamespace = AutoCloseFD{open(fmt("/proc/%d/ns/user", (pid_t) pid).c_str(), O_RDONLY)};
             if (sandboxUserNamespace.get() == -1)
                 throw SysError("getting sandbox user namespace");
         }
@@ -1471,8 +1471,8 @@ void LocalDerivationGoal::startDaemon()
             struct sockaddr_un remoteAddr;
             socklen_t remoteAddrLen = sizeof(remoteAddr);
 
-            AutoCloseFD remote = accept(daemonSocket.get(),
+            AutoCloseFD remote{accept(daemonSocket.get(),
-                (struct sockaddr *) &remoteAddr, &remoteAddrLen);
+                (struct sockaddr *) &remoteAddr, &remoteAddrLen)};
             if (!remote) {
                 if (errno == EINTR || errno == EAGAIN) continue;
                 if (errno == EINVAL || errno == ECONNABORTED) break;
@@ -1705,12 +1705,12 @@ void LocalDerivationGoal::runChild()
 #if __linux__
         if (useChroot) {
 
-            userNamespaceSync.writeSide = -1;
+            userNamespaceSync.writeSide.reset();
 
             if (drainFD(userNamespaceSync.readSide.get()) != "1")
                 throw Error("user namespace initialisation failed");
 
-            userNamespaceSync.readSide = -1;
+            userNamespaceSync.readSide.reset();
 
             if (privateNetwork) {
 

src/libstore/gc.cc

@@ -549,7 +549,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
             if (fds[1].revents) {
                 /* Accept a new connection. */
                 assert(fds[1].revents & POLLIN);
-                AutoCloseFD fdClient = accept(fdServer.get(), nullptr, nullptr);
+                AutoCloseFD fdClient{accept(fdServer.get(), nullptr, nullptr)};
                 if (!fdClient) continue;
 
                 debug("GC roots server accepted new client");
@@ -647,7 +647,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
            by another process. We need to be sure that we can acquire an
            exclusive lock before deleting them. */
         if (baseName.find("tmp-", 0) == 0) {
-            AutoCloseFD tmpDirFd = open(realPath.c_str(), O_RDONLY | O_DIRECTORY);
+            AutoCloseFD tmpDirFd{open(realPath.c_str(), O_RDONLY | O_DIRECTORY)};
             if (tmpDirFd.get() == -1 || !lockFile(tmpDirFd.get(), ltWrite, false)) {
                 debug("skipping locked tempdir '%s'", realPath);
                 return;

src/libstore/local-store.cc

@@ -263,7 +263,7 @@ LocalStore::LocalStore(const Params & params)
         if (stat(reservedPath.c_str(), &st) == -1 ||
             st.st_size != settings.reservedSize)
         {
-            AutoCloseFD fd = open(reservedPath.c_str(), O_WRONLY | O_CREAT | O_CLOEXEC, 0600);
+            AutoCloseFD fd{open(reservedPath.c_str(), O_WRONLY | O_CREAT | O_CLOEXEC, 0600)};
             int res = -1;
 #if HAVE_POSIX_FALLOCATE
             res = posix_fallocate(fd.get(), 0, settings.reservedSize);
@@ -453,7 +453,7 @@ LocalStore::LocalStore(std::string scheme, std::string path, const Params & para
 AutoCloseFD LocalStore::openGCLock()
 {
     Path fnGCLock = stateDir + "/gc.lock";
-    auto fdGCLock = open(fnGCLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600);
+    AutoCloseFD fdGCLock{open(fnGCLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600)};
     if (!fdGCLock)
         throw SysError("opening global GC lock '%1%'", fnGCLock);
     return fdGCLock;
@@ -478,7 +478,7 @@ LocalStore::~LocalStore()
     try {
         auto fdTempRoots(_fdTempRoots.lock());
         if (*fdTempRoots) {
-            *fdTempRoots = -1;
+            fdTempRoots->reset();
             unlink(fnTempRoots.c_str());
         }
     } catch (...) {
@@ -1484,7 +1484,7 @@ std::pair<Path, AutoCloseFD> LocalStore::createTempDirInStore()
            the GC between createTempDir() and when we acquire a lock on it.
            We'll repeat until 'tmpDir' exists and we've locked it. */
         tmpDirFn = createTempDir(realStoreDir, "tmp");
-        tmpDirFd = open(tmpDirFn.c_str(), O_RDONLY | O_DIRECTORY);
+        tmpDirFd = AutoCloseFD{open(tmpDirFn.c_str(), O_RDONLY | O_DIRECTORY)};
         if (tmpDirFd.get() < 0) {
             continue;
         }

src/libstore/lock.cc

@@ -76,7 +76,7 @@ struct SimpleUserLock : UserLock
 
             auto fnUserLock = fmt("%s/userpool/%s", settings.nixStateDir,pw->pw_uid);
 
-            AutoCloseFD fd = open(fnUserLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600);
+            AutoCloseFD fd{open(fnUserLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600)};
             if (!fd)
                 throw SysError("opening user lock '%s'", fnUserLock);
 
@@ -148,7 +148,7 @@ struct AutoUserLock : UserLock
 
             auto fnUserLock = fmt("%s/userpool2/slot-%d", settings.nixStateDir, i);
 
-            AutoCloseFD fd = open(fnUserLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600);
+            AutoCloseFD fd{open(fnUserLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600)};
             if (!fd)
                 throw SysError("opening user lock '%s'", fnUserLock);
 

src/libstore/pathlocks.cc

@@ -17,9 +17,7 @@ namespace nix {
 
 AutoCloseFD openLockFile(const Path & path, bool create)
 {
-    AutoCloseFD fd;
+    AutoCloseFD fd{open(path.c_str(), O_CLOEXEC | O_RDWR | (create ? O_CREAT : 0), 0600)};
-
-    fd = open(path.c_str(), O_CLOEXEC | O_RDWR | (create ? O_CREAT : 0), 0600);
     if (!fd && (create || errno != ENOENT))
         throw SysError("opening lock file '%1%'", path);
 

src/libstore/remote-fs-accessor.cc

@@ -71,7 +71,7 @@ std::pair<ref<FSAccessor>, Path> RemoteFSAccessor::fetch(const Path & path_, boo
             auto narAccessor = makeLazyNarAccessor(listing,
                 [cacheFile](uint64_t offset, uint64_t length) {
 
-                    AutoCloseFD fd = open(cacheFile.c_str(), O_RDONLY | O_CLOEXEC);
+                    AutoCloseFD fd{open(cacheFile.c_str(), O_RDONLY | O_CLOEXEC)};
                     if (!fd)
                         throw SysError("opening NAR cache file '%s'", cacheFile);
 

src/libstore/ssh.cc

@@ -100,8 +100,8 @@ std::unique_ptr<SSHMaster::Connection> SSHMaster::startCommand(const std::string
     }, options);
 
 
-    in.readSide = -1;
+    in.readSide.reset();
-    out.writeSide = -1;
+    out.writeSide.reset();
 
     // Wait for the SSH connection to be established,
     // So that we don't overwrite the password prompt with our progress bar.
@@ -162,7 +162,7 @@ Path SSHMaster::startMaster()
         throw SysError("unable to execute '%s'", args.front());
     }, options);
 
-    out.writeSide = -1;
+    out.writeSide.reset();
 
     std::string reply;
     try {

src/libutil/archive.cc

@@ -44,7 +44,7 @@ static void dumpContents(const Path & path, off_t size,
 {
     sink << "contents" << size;
 
-    AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC);
+    AutoCloseFD fd{open(path.c_str(), O_RDONLY | O_CLOEXEC)};
     if (!fd) throw SysError("opening file '%1%'", path);
 
     std::vector<char> buf(65536);
@@ -318,7 +318,7 @@ struct RestoreSink : ParseSink
     void createRegularFile(const Path & path) override
     {
         Path p = dstPath + path;
-        fd = open(p.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0666);
+        fd = AutoCloseFD{open(p.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0666)};
         if (!fd) throw SysError("creating file '%1%'", p);
     }
 

src/libutil/util.cc

@@ -379,7 +379,7 @@ std::string readFile(int fd)
 
 std::string readFile(const Path & path)
 {
-    AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC);
+    AutoCloseFD fd{open(path.c_str(), O_RDONLY | O_CLOEXEC)};
     if (!fd)
         throw SysError("opening file '%1%'", path);
     return readFile(fd.get());
@@ -388,7 +388,7 @@ std::string readFile(const Path & path)
 
 void readFile(const Path & path, Sink & sink)
 {
-    AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC);
+    AutoCloseFD fd{open(path.c_str(), O_RDONLY | O_CLOEXEC)};
     if (!fd)
         throw SysError("opening file '%s'", path);
     drainFD(fd.get(), sink);
@@ -397,7 +397,7 @@ void readFile(const Path & path, Sink & sink)
 
 void writeFile(const Path & path, std::string_view s, mode_t mode, bool sync)
 {
-    AutoCloseFD fd = open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode);
+    AutoCloseFD fd{open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode)};
     if (!fd)
         throw SysError("opening file '%1%'", path);
     try {
@@ -417,7 +417,7 @@ void writeFile(const Path & path, std::string_view s, mode_t mode, bool sync)
 
 void writeFile(const Path & path, Source & source, mode_t mode, bool sync)
 {
-    AutoCloseFD fd = open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode);
+    AutoCloseFD fd{open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode)};
     if (!fd)
         throw SysError("opening file '%1%'", path);
 
@@ -444,7 +444,7 @@ void writeFile(const Path & path, Source & source, mode_t mode, bool sync)
 
 void syncParent(const Path & path)
 {
-    AutoCloseFD fd = open(dirOf(path).c_str(), O_RDONLY, 0);
+    AutoCloseFD fd{open(dirOf(path).c_str(), O_RDONLY, 0)};
     if (!fd)
         throw SysError("opening file '%1%'", path);
     fd.fsync();
@@ -941,8 +941,8 @@ void Pipe::create()
     closeOnExec(fds[0]);
     closeOnExec(fds[1]);
 #endif
-    readSide = fds[0];
+    readSide = AutoCloseFD{fds[0]};
-    writeSide = fds[1];
+    writeSide = AutoCloseFD{fds[1]};
 }
 
 
@@ -1718,11 +1718,11 @@ void saveMountNamespace()
 #if __linux__
     static std::once_flag done;
     std::call_once(done, []() {
-        fdSavedMountNamespace = open("/proc/self/ns/mnt", O_RDONLY);
+        fdSavedMountNamespace = AutoCloseFD{open("/proc/self/ns/mnt", O_RDONLY)};
         if (!fdSavedMountNamespace)
             throw SysError("saving parent mount namespace");
 
-        fdSavedRoot = open("/proc/self/root", O_RDONLY);
+        fdSavedRoot = AutoCloseFD{open("/proc/self/root", O_RDONLY)};
     });
 #endif
 }
@@ -1777,11 +1777,11 @@ void restoreProcessContext(bool restoreMounts)
 
 AutoCloseFD createUnixDomainSocket()
 {
-    AutoCloseFD fdSocket = socket(PF_UNIX, SOCK_STREAM
+    AutoCloseFD fdSocket{socket(PF_UNIX, SOCK_STREAM
         #ifdef SOCK_CLOEXEC
         | SOCK_CLOEXEC
         #endif
-        , 0);
+        , 0)};
     if (!fdSocket)
         throw SysError("cannot create Unix domain socket");
     closeOnExec(fdSocket.get());

src/libutil/util.hh

@@ -332,7 +332,7 @@ class AutoCloseFD
     int fd;
 public:
     AutoCloseFD();
-    AutoCloseFD(int fd);
+    explicit AutoCloseFD(int fd);
     AutoCloseFD(const AutoCloseFD & fd) = delete;
     AutoCloseFD(AutoCloseFD&& fd);
     ~AutoCloseFD();
@@ -343,6 +343,7 @@ public:
     int release();
     void close();
     void fsync();
+    void reset() { *this = {}; }
 };
 
 

src/nix-channel/nix-channel.cc

@@ -83,7 +83,7 @@ static void update(const StringSet & channelNames)
     writeFull(fd.get(),
         #include "unpack-channel.nix.gen.hh"
         );
-    fd = -1;
+    fd.reset();
     AutoDelete del(unpackChannelPath, false);
 
     // Download each channel.

src/nix/daemon.cc

@@ -294,7 +294,7 @@ static void daemonLoop(std::optional<TrustedFlag> forceTrustClientOpt)
     if (listenFds) {
         if (getEnv("LISTEN_PID") != std::to_string(getpid()) || listenFds != "1")
             throw Error("unexpected systemd environment variables");
-        fdSocket = SD_LISTEN_FDS_START;
+        fdSocket = AutoCloseFD{SD_LISTEN_FDS_START};
         closeOnExec(fdSocket.get());
     }
 
@@ -315,8 +315,8 @@ static void daemonLoop(std::optional<TrustedFlag> forceTrustClientOpt)
             struct sockaddr_un remoteAddr;
             socklen_t remoteAddrLen = sizeof(remoteAddr);
 
-            AutoCloseFD remote = accept(fdSocket.get(),
+            AutoCloseFD remote{accept(fdSocket.get(),
-                (struct sockaddr *) &remoteAddr, &remoteAddrLen);
+                    (struct sockaddr *) &remoteAddr, &remoteAddrLen)};
             checkInterrupt();
             if (!remote) {
                 if (errno == EINTR) continue;
@@ -348,7 +348,7 @@ static void daemonLoop(std::optional<TrustedFlag> forceTrustClientOpt)
             options.dieWithParent = false;
             options.runExitHandlers = true;
             startProcess([&]() {
-                fdSocket = -1;
+                fdSocket.reset();
 
                 //  Background the daemon.
                 if (setsid() == -1)

src/nix/prefetch.cc

@@ -92,7 +92,7 @@ std::tuple<StorePath, Hash> prefetchFile(
             if (executable)
                 mode = 0700;
 
-            AutoCloseFD fd = open(tmpFile.c_str(), O_WRONLY | O_CREAT | O_EXCL, mode);
+            AutoCloseFD fd{open(tmpFile.c_str(), O_WRONLY | O_CREAT | O_EXCL, mode)};
             if (!fd) throw SysError("creating temporary file '%s'", tmpFile);
 
             FdSink sink(fd.get());

src/resolve-system-dependencies/resolve-system-dependencies.cc

@@ -30,7 +30,7 @@ std::set<std::string> readCacheFile(const Path & file)
 
 std::set<std::string> runResolver(const Path & filename)
 {
-    AutoCloseFD fd = open(filename.c_str(), O_RDONLY);
+    AutoCloseFD fd{open(filename.c_str(), O_RDONLY)};
     if (!fd)
         throw SysError("opening '%s'", filename);