From cb5e7254b66a06b78a5659551a6f28fc67e52267 Mon Sep 17 00:00:00 2001 From: Shea Levy Date: Mon, 11 Jul 2016 15:44:44 -0400 Subject: [PATCH] Modernize AutoCloseFD --- src/libmain/shared.cc | 4 +- src/libstore/build.cc | 86 ++++++++++++++++++------------------ src/libstore/gc.cc | 34 +++++++------- src/libstore/local-store.cc | 22 ++++----- src/libstore/pathlocks.cc | 12 ++--- src/libstore/remote-store.cc | 12 ++--- src/libutil/archive.cc | 15 +++---- src/libutil/hash.cc | 4 +- src/libutil/util.cc | 67 +++++++++++----------------- src/libutil/util.hh | 14 +++--- src/nix-daemon/nix-daemon.cc | 22 ++++----- 11 files changed, 139 insertions(+), 153 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 0b6311516..515d80091 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -325,7 +325,7 @@ RunPager::RunPager() toPager.create(); pid = startProcess([&]() { - if (dup2(toPager.readSide, STDIN_FILENO) == -1) + if (dup2(toPager.readSide.get(), STDIN_FILENO) == -1) throw SysError("dupping stdin"); if (!getenv("LESS")) setenv("LESS", "FRSXMK", 1); @@ -337,7 +337,7 @@ RunPager::RunPager() throw SysError(format("executing ‘%1%’") % pager); }); - if (dup2(toPager.writeSide, STDOUT_FILENO) == -1) + if (dup2(toPager.writeSide.get(), STDOUT_FILENO) == -1) throw SysError("dupping stdout"); } diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 36fcdb845..0a4df8ca7 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -403,7 +403,7 @@ static void commonChildInit(Pipe & logPipe) throw SysError(format("creating a new session")); /* Dup the write side of the logger pipe into stderr. */ - if (dup2(logPipe.writeSide, STDERR_FILENO) == -1) + if (dup2(logPipe.writeSide.get(), STDERR_FILENO) == -1) throw SysError("cannot pipe standard error into log file"); /* Dup stderr to stdout. */ @@ -510,11 +510,11 @@ void UserLock::acquire() continue; AutoCloseFD fd = open(fnUserLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600); - if (fd == -1) + if (!fd) throw SysError(format("opening user lock ‘%1%’") % fnUserLock); - if (lockFile(fd, ltWrite, false)) { - fdUserLock = fd.borrow(); + if (lockFile(fd.get(), ltWrite, false)) { + fdUserLock = std::move(fd); lockedPaths.insert(fnUserLock); user = i; uid = pw->pw_uid; @@ -550,7 +550,7 @@ void UserLock::acquire() void UserLock::release() { if (uid == 0) return; - fdUserLock.close(); /* releases lock */ + fdUserLock = -1; /* releases lock */ assert(lockedPaths.find(fnUserLock) != lockedPaths.end()); lockedPaths.erase(fnUserLock); fnUserLock = ""; @@ -613,11 +613,11 @@ HookInstance::HookInstance() if (chdir("/") == -1) throw SysError("changing into /"); /* Dup the communication pipes. */ - if (dup2(toHook.readSide, STDIN_FILENO) == -1) + if (dup2(toHook.readSide.get(), STDIN_FILENO) == -1) throw SysError("dupping to-hook read side"); /* Use fd 4 for the builder's stdout/stderr. */ - if (dup2(builderOut.writeSide, 4) == -1) + if (dup2(builderOut.writeSide.get(), 4) == -1) throw SysError("dupping builder's stdout/stderr"); Strings args = { @@ -633,15 +633,15 @@ HookInstance::HookInstance() }); pid.setSeparatePG(true); - fromHook.writeSide.close(); - toHook.readSide.close(); + fromHook.writeSide = -1; + toHook.readSide = -1; } HookInstance::~HookInstance() { try { - toHook.writeSide.close(); + toHook.writeSide = -1; pid.kill(true); } catch (...) { ignoreException(); @@ -1414,10 +1414,10 @@ void DerivationGoal::buildDone() /* Close the read side of the logger pipe. */ if (hook) { - hook->builderOut.readSide.close(); - hook->fromHook.readSide.close(); + hook->builderOut.readSide = -1; + hook->fromHook.readSide = -1; } - else builderOut.readSide.close(); + else builderOut.readSide = -1; /* Close the log file. */ closeLogFile(); @@ -1557,7 +1557,7 @@ HookReply DerivationGoal::tryBuildHook() for (auto & i : features) checkStoreName(i); /* !!! abuse */ /* Send the request to the hook. */ - writeLine(worker.hook->toHook.writeSide, (format("%1% %2% %3% %4%") + writeLine(worker.hook->toHook.writeSide.get(), (format("%1% %2% %3% %4%") % (worker.getNrLocalBuilds() < settings.maxBuildJobs ? "1" : "0") % drv->platform % drvPath % concatStringsSep(",", features)).str()); @@ -1565,7 +1565,7 @@ HookReply DerivationGoal::tryBuildHook() whether the hook wishes to perform the build. */ string reply; while (true) { - string s = readLine(worker.hook->fromHook.readSide); + string s = readLine(worker.hook->fromHook.readSide.get()); if (string(s, 0, 2) == "# ") { reply = string(s, 2); break; @@ -1597,22 +1597,22 @@ HookReply DerivationGoal::tryBuildHook() string s; for (auto & i : allInputs) { s += i; s += ' '; } - writeLine(hook->toHook.writeSide, s); + writeLine(hook->toHook.writeSide.get(), s); /* Tell the hooks the missing outputs that have to be copied back from the remote system. */ s = ""; for (auto & i : missingPaths) { s += i; s += ' '; } - writeLine(hook->toHook.writeSide, s); + writeLine(hook->toHook.writeSide.get(), s); - hook->toHook.writeSide.close(); + hook->toHook.writeSide = -1; /* Create the log file and pipe. */ Path logFile = openLogFile(); set fds; - fds.insert(hook->fromHook.readSide); - fds.insert(hook->builderOut.readSide); + fds.insert(hook->fromHook.readSide.get()); + fds.insert(hook->builderOut.readSide.get()); worker.childStarted(shared_from_this(), fds, false, false); return rpAccept; @@ -2142,17 +2142,17 @@ void DerivationGoal::startBuilder() child = clone(childEntry, stack + stackSize, flags & ~CLONE_NEWPID, this); if (child == -1) throw SysError("cloning builder process"); - writeFull(builderOut.writeSide, std::to_string(child) + "\n"); + writeFull(builderOut.writeSide.get(), std::to_string(child) + "\n"); _exit(0); }, options); if (helper.wait(true) != 0) throw Error("unable to start build process"); - userNamespaceSync.readSide.close(); + userNamespaceSync.readSide = -1; pid_t tmp; - if (!string2Int(readLine(builderOut.readSide), tmp)) abort(); + if (!string2Int(readLine(builderOut.readSide.get()), tmp)) abort(); pid = tmp; /* Set the UID/GID mapping of the builder's user @@ -2171,8 +2171,8 @@ void DerivationGoal::startBuilder() /* Signal the builder that we've updated its user namespace. */ - writeFull(userNamespaceSync.writeSide, "1"); - userNamespaceSync.writeSide.close(); + writeFull(userNamespaceSync.writeSide.get(), "1"); + userNamespaceSync.writeSide = -1; } else #endif @@ -2186,12 +2186,12 @@ void DerivationGoal::startBuilder() /* parent */ pid.setSeparatePG(true); - builderOut.writeSide.close(); - worker.childStarted(shared_from_this(), {builderOut.readSide}, true, true); + builderOut.writeSide = -1; + worker.childStarted(shared_from_this(), {builderOut.readSide.get()}, true, true); /* Check if setting up the build environment failed. */ while (true) { - string msg = readLine(builderOut.readSide); + string msg = readLine(builderOut.readSide.get()); if (string(msg, 0, 1) == "\1") { if (msg.size() == 1) break; throw Error(string(msg, 1)); @@ -2215,26 +2215,24 @@ void DerivationGoal::runChild() #if __linux__ if (useChroot) { - userNamespaceSync.writeSide.close(); + userNamespaceSync.writeSide = -1; - if (drainFD(userNamespaceSync.readSide) != "1") + if (drainFD(userNamespaceSync.readSide.get()) != "1") throw Error("user namespace initialisation failed"); - userNamespaceSync.readSide.close(); + userNamespaceSync.readSide = -1; if (privateNetwork) { /* Initialise the loopback interface. */ AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP)); - if (fd == -1) throw SysError("cannot open IP socket"); + if (!fd) throw SysError("cannot open IP socket"); struct ifreq ifr; strcpy(ifr.ifr_name, "lo"); ifr.ifr_flags = IFF_UP | IFF_LOOPBACK | IFF_RUNNING; - if (ioctl(fd, SIOCSIFFLAGS, &ifr) == -1) + if (ioctl(fd.get(), SIOCSIFFLAGS, &ifr) == -1) throw SysError("cannot set loopback interface flags"); - - fd.close(); } /* Set the hostname etc. to fixed values. */ @@ -2919,9 +2917,9 @@ Path DerivationGoal::openLogFile() % (settings.compressLog ? ".bz2" : "")).str(); fdLogFile = open(logFileName.c_str(), O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC, 0666); - if (fdLogFile == -1) throw SysError(format("creating log file ‘%1%’") % logFileName); + if (!fdLogFile) throw SysError(format("creating log file ‘%1%’") % logFileName); - logFileSink = std::make_shared(fdLogFile); + logFileSink = std::make_shared(fdLogFile.get()); if (settings.compressLog) logSink = std::shared_ptr(makeCompressionSink("bzip2", *logFileSink)); @@ -2938,7 +2936,7 @@ void DerivationGoal::closeLogFile() if (logSink2) logSink2->finish(); if (logFileSink) logFileSink->flush(); logSink = logFileSink = 0; - fdLogFile.close(); + fdLogFile = -1; } @@ -2960,8 +2958,8 @@ void DerivationGoal::deleteTmpDir(bool force) void DerivationGoal::handleChildOutput(int fd, const string & data) { - if ((hook && fd == hook->builderOut.readSide) || - (!hook && fd == builderOut.readSide)) + if ((hook && fd == hook->builderOut.readSide.get()) || + (!hook && fd == builderOut.readSide.get())) { logSize += data.size(); if (settings.maxLogSize && logSize > settings.maxLogSize) { @@ -2987,7 +2985,7 @@ void DerivationGoal::handleChildOutput(int fd, const string & data) if (logSink) (*logSink)(data); } - if (hook && fd == hook->fromHook.readSide) + if (hook && fd == hook->fromHook.readSide.get()) printMsg(lvlError, data); // FIXME? } @@ -3274,7 +3272,7 @@ void SubstitutionGoal::tryToRun() thr = std::thread([this]() { try { /* Wake up the worker loop when we're done. */ - Finally updateStats([this]() { outPipe.writeSide.close(); }); + Finally updateStats([this]() { outPipe.writeSide = -1; }); copyStorePath(ref(sub), ref(worker.store.shared_from_this()), storePath, repair); @@ -3285,7 +3283,7 @@ void SubstitutionGoal::tryToRun() } }); - worker.childStarted(shared_from_this(), {outPipe.readSide}, true, false); + worker.childStarted(shared_from_this(), {outPipe.readSide.get()}, true, false); state = &SubstitutionGoal::finished; } @@ -3325,7 +3323,7 @@ void SubstitutionGoal::handleChildOutput(int fd, const string & data) void SubstitutionGoal::handleEOF(int fd) { - if (fd == outPipe.readSide) worker.wakeUp(shared_from_this()); + if (fd == outPipe.readSide.get()) worker.wakeUp(shared_from_this()); } diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 986608d6b..9324508cc 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -34,19 +34,19 @@ int LocalStore::openGCLock(LockType lockType) debug(format("acquiring global GC lock ‘%1%’") % fnGCLock); AutoCloseFD fdGCLock = open(fnGCLock.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600); - if (fdGCLock == -1) + if (!fdGCLock) throw SysError(format("opening global GC lock ‘%1%’") % fnGCLock); - if (!lockFile(fdGCLock, lockType, false)) { + if (!lockFile(fdGCLock.get(), lockType, false)) { printMsg(lvlError, format("waiting for the big garbage collector lock...")); - lockFile(fdGCLock, lockType, true); + lockFile(fdGCLock.get(), lockType, true); } /* !!! Restrict read permission on the GC root. Otherwise any process that can open the file for reading can DoS the collector. */ - return fdGCLock.borrow(); + return fdGCLock.release(); } @@ -149,7 +149,7 @@ void LocalStore::addTempRoot(const Path & path) auto state(_state.lock()); /* Create the temporary roots file for this process. */ - if (state->fdTempRoots == -1) { + if (!state->fdTempRoots) { while (1) { Path dir = (format("%1%/%2%") % stateDir % tempRootsDir).str(); @@ -166,15 +166,15 @@ void LocalStore::addTempRoot(const Path & path) state->fdTempRoots = openLockFile(state->fnTempRoots, true); - fdGCLock.close(); + fdGCLock = -1; debug(format("acquiring read lock on ‘%1%’") % state->fnTempRoots); - lockFile(state->fdTempRoots, ltRead, true); + lockFile(state->fdTempRoots.get(), ltRead, true); /* Check whether the garbage collector didn't get in our way. */ struct stat st; - if (fstat(state->fdTempRoots, &st) == -1) + if (fstat(state->fdTempRoots.get(), &st) == -1) throw SysError(format("statting ‘%1%’") % state->fnTempRoots); if (st.st_size == 0) break; @@ -188,14 +188,14 @@ void LocalStore::addTempRoot(const Path & path) /* Upgrade the lock to a write lock. This will cause us to block if the garbage collector is holding our lock. */ debug(format("acquiring write lock on ‘%1%’") % state->fnTempRoots); - lockFile(state->fdTempRoots, ltWrite, true); + lockFile(state->fdTempRoots.get(), ltWrite, true); string s = path + '\0'; - writeFull(state->fdTempRoots, s); + writeFull(state->fdTempRoots.get(), s); /* Downgrade to a read lock. */ debug(format("downgrading to read lock on ‘%1%’") % state->fnTempRoots); - lockFile(state->fdTempRoots, ltRead, true); + lockFile(state->fdTempRoots.get(), ltRead, true); } @@ -211,7 +211,7 @@ void LocalStore::readTempRoots(PathSet & tempRoots, FDs & fds) debug(format("reading temporary root file ‘%1%’") % path); FDPtr fd(new AutoCloseFD(open(path.c_str(), O_CLOEXEC | O_RDWR, 0666))); - if (*fd == -1) { + if (!*fd) { /* It's okay if the file has disappeared. */ if (errno == ENOENT) continue; throw SysError(format("opening temporary roots file ‘%1%’") % path); @@ -224,10 +224,10 @@ void LocalStore::readTempRoots(PathSet & tempRoots, FDs & fds) /* Try to acquire a write lock without blocking. This can only succeed if the owning process has died. In that case we don't care about its temporary roots. */ - if (lockFile(*fd, ltWrite, false)) { + if (lockFile(fd->get(), ltWrite, false)) { printMsg(lvlError, format("removing stale temporary roots file ‘%1%’") % path); unlink(path.c_str()); - writeFull(*fd, "d"); + writeFull(fd->get(), "d"); continue; } @@ -235,10 +235,10 @@ void LocalStore::readTempRoots(PathSet & tempRoots, FDs & fds) from upgrading to a write lock, therefore it will block in addTempRoot(). */ debug(format("waiting for read lock on ‘%1%’") % path); - lockFile(*fd, ltRead, true); + lockFile(fd->get(), ltRead, true); /* Read the entire file. */ - string contents = readFile(*fd); + string contents = readFile(fd->get()); /* Extract the roots. */ string::size_type pos = 0, end; @@ -721,7 +721,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) } /* Allow other processes to add to the store from here on. */ - fdGCLock.close(); + fdGCLock = -1; fds.clear(); /* Delete the trash directory. */ diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 409eb1a8a..822d5fce3 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -120,11 +120,11 @@ LocalStore::LocalStore(const Params & params) AutoCloseFD fd = open(reservedPath.c_str(), O_WRONLY | O_CREAT | O_CLOEXEC, 0600); int res = -1; #if HAVE_POSIX_FALLOCATE - res = posix_fallocate(fd, 0, settings.reservedSize); + res = posix_fallocate(fd.get(), 0, settings.reservedSize); #endif if (res == -1) { - writeFull(fd, string(settings.reservedSize, 'X')); - ftruncate(fd, settings.reservedSize); + writeFull(fd.get(), string(settings.reservedSize, 'X')); + ftruncate(fd.get(), settings.reservedSize); } } } catch (SysError & e) { /* don't care about errors */ @@ -135,9 +135,9 @@ LocalStore::LocalStore(const Params & params) Path globalLockPath = dbDir + "/big-lock"; globalLock = openLockFile(globalLockPath.c_str(), true); - if (!lockFile(globalLock, ltRead, false)) { + if (!lockFile(globalLock.get(), ltRead, false)) { printMsg(lvlError, "waiting for the big Nix store lock..."); - lockFile(globalLock, ltRead, true); + lockFile(globalLock.get(), ltRead, true); } /* Check the current database schema and if necessary do an @@ -166,9 +166,9 @@ LocalStore::LocalStore(const Params & params) "which is no longer supported. To convert to the new format,\n" "please upgrade Nix to version 1.11 first."); - if (!lockFile(globalLock, ltWrite, false)) { + if (!lockFile(globalLock.get(), ltWrite, false)) { printMsg(lvlError, "waiting for exclusive access to the Nix store..."); - lockFile(globalLock, ltWrite, true); + lockFile(globalLock.get(), ltWrite, true); } /* Get the schema version again, because another process may @@ -197,7 +197,7 @@ LocalStore::LocalStore(const Params & params) writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str()); - lockFile(globalLock, ltRead, true); + lockFile(globalLock.get(), ltRead, true); } else openDB(*state, false); @@ -236,8 +236,8 @@ LocalStore::~LocalStore() auto state(_state.lock()); try { - if (state->fdTempRoots != -1) { - state->fdTempRoots.close(); + if (state->fdTempRoots) { + state->fdTempRoots = -1; unlink(state->fnTempRoots.c_str()); } } catch (...) { @@ -1115,7 +1115,7 @@ bool LocalStore::verifyStore(bool checkContents, bool repair) /* Release the GC lock so that checking content hashes (which can take ages) doesn't block the GC or builds. */ - fdGCLock.close(); + fdGCLock = -1; /* Optionally, check the content hashes (slow). */ if (checkContents) { diff --git a/src/libstore/pathlocks.cc b/src/libstore/pathlocks.cc index d0a0f812e..b9e178d61 100644 --- a/src/libstore/pathlocks.cc +++ b/src/libstore/pathlocks.cc @@ -17,10 +17,10 @@ int openLockFile(const Path & path, bool create) AutoCloseFD fd; fd = open(path.c_str(), O_CLOEXEC | O_RDWR | (create ? O_CREAT : 0), 0600); - if (fd == -1 && (create || errno != ENOENT)) + if (!fd && (create || errno != ENOENT)) throw SysError(format("opening lock file ‘%1%’") % path); - return fd.borrow(); + return fd.release(); } @@ -119,10 +119,10 @@ bool PathLocks::lockPaths(const PathSet & _paths, fd = openLockFile(lockPath, true); /* Acquire an exclusive lock. */ - if (!lockFile(fd, ltWrite, false)) { + if (!lockFile(fd.get(), ltWrite, false)) { if (wait) { if (waitMsg != "") printMsg(lvlError, waitMsg); - lockFile(fd, ltWrite, true); + lockFile(fd.get(), ltWrite, true); } else { /* Failed to lock this path; release all other locks. */ @@ -136,7 +136,7 @@ bool PathLocks::lockPaths(const PathSet & _paths, /* Check that the lock file hasn't become stale (i.e., hasn't been unlinked). */ struct stat st; - if (fstat(fd, &st) == -1) + if (fstat(fd.get(), &st) == -1) throw SysError(format("statting lock file ‘%1%’") % lockPath); if (st.st_size != 0) /* This lock file has been unlinked, so we're holding @@ -149,7 +149,7 @@ bool PathLocks::lockPaths(const PathSet & _paths, } /* Use borrow so that the descriptor isn't closed. */ - fds.push_back(FDPair(fd.borrow(), lockPath)); + fds.push_back(FDPair(fd.release(), lockPath)); lockedPaths.insert(lockPath); } diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 50ad409a9..ab05c3844 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -66,9 +66,9 @@ ref RemoteStore::openConnection() | SOCK_CLOEXEC #endif , 0); - if (conn->fd == -1) + if (!conn->fd) throw SysError("cannot create Unix domain socket"); - closeOnExec(conn->fd); + closeOnExec(conn->fd.get()); string socketPath = settings.nixDaemonSocketFile; @@ -78,11 +78,11 @@ ref RemoteStore::openConnection() throw Error(format("socket path ‘%1%’ is too long") % socketPath); strcpy(addr.sun_path, socketPath.c_str()); - if (connect(conn->fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) + if (connect(conn->fd.get(), (struct sockaddr *) &addr, sizeof(addr)) == -1) throw SysError(format("cannot connect to daemon at ‘%1%’") % socketPath); - conn->from.fd = conn->fd; - conn->to.fd = conn->fd; + conn->from.fd = conn->fd.get(); + conn->to.fd = conn->fd.get(); /* Send the magic greeting, check for the reply. */ try { @@ -531,7 +531,7 @@ RemoteStore::Connection::~Connection() { try { to.flush(); - fd.close(); + fd = -1; } catch (...) { ignoreException(); } diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index c3e4c87a5..edd4a881b 100644 --- a/src/libutil/archive.cc +++ b/src/libutil/archive.cc @@ -42,14 +42,14 @@ static void dumpContents(const Path & path, size_t size, sink << "contents" << size; AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC); - if (fd == -1) throw SysError(format("opening file ‘%1%’") % path); + if (!fd) throw SysError(format("opening file ‘%1%’") % path); unsigned char buf[65536]; size_t left = size; while (left > 0) { size_t n = left > sizeof(buf) ? sizeof(buf) : left; - readFull(fd, buf, n); + readFull(fd.get(), buf, n); left -= n; sink(buf, n); } @@ -303,17 +303,16 @@ struct RestoreSink : ParseSink void createRegularFile(const Path & path) { Path p = dstPath + path; - fd.close(); fd = open(p.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0666); - if (fd == -1) throw SysError(format("creating file ‘%1%’") % p); + if (!fd) throw SysError(format("creating file ‘%1%’") % p); } void isExecutable() { struct stat st; - if (fstat(fd, &st) == -1) + if (fstat(fd.get(), &st) == -1) throw SysError("fstat"); - if (fchmod(fd, st.st_mode | (S_IXUSR | S_IXGRP | S_IXOTH)) == -1) + if (fchmod(fd.get(), st.st_mode | (S_IXUSR | S_IXGRP | S_IXOTH)) == -1) throw SysError("fchmod"); } @@ -321,7 +320,7 @@ struct RestoreSink : ParseSink { #if HAVE_POSIX_FALLOCATE if (len) { - errno = posix_fallocate(fd, 0, 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 @@ -334,7 +333,7 @@ struct RestoreSink : ParseSink void receiveContents(unsigned char * data, unsigned int len) { - writeFull(fd, data, len); + writeFull(fd.get(), data, len); } void createSymlink(const Path & path, const string & target) diff --git a/src/libutil/hash.cc b/src/libutil/hash.cc index 69ea95852..fa4258777 100644 --- a/src/libutil/hash.cc +++ b/src/libutil/hash.cc @@ -255,11 +255,11 @@ Hash hashFile(HashType ht, const Path & path) start(ht, ctx); AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC); - if (fd == -1) throw SysError(format("opening file ‘%1%’") % path); + if (!fd) throw SysError(format("opening file ‘%1%’") % path); unsigned char buf[8192]; ssize_t n; - while ((n = read(fd, buf, sizeof(buf)))) { + while ((n = read(fd.get(), buf, sizeof(buf)))) { checkInterrupt(); if (n == -1) throw SysError(format("reading file ‘%1%’") % path); update(ht, ctx, buf, n); diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 4cc4649c9..f1e714a66 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -274,18 +274,18 @@ string readFile(int fd) string readFile(const Path & path, bool drain) { AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC); - if (fd == -1) + if (!fd) throw SysError(format("opening file ‘%1%’") % path); - return drain ? drainFD(fd) : readFile(fd); + return drain ? drainFD(fd.get()) : readFile(fd.get()); } void writeFile(const Path & path, const string & s) { AutoCloseFD fd = open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, 0666); - if (fd == -1) + if (!fd) throw SysError(format("opening file ‘%1%’") % path); - writeFull(fd, s); + writeFull(fd.get(), s); } @@ -556,28 +556,24 @@ void AutoDelete::reset(const Path & p, bool recursive) { ////////////////////////////////////////////////////////////////////// -AutoCloseFD::AutoCloseFD() +AutoCloseFD::AutoCloseFD() : fd{-1} {} + + +AutoCloseFD::AutoCloseFD(int fd) : fd{fd} {} + + +AutoCloseFD::AutoCloseFD(AutoCloseFD&& that) : fd{that.fd} { - fd = -1; + that.fd = -1; } -AutoCloseFD::AutoCloseFD(int fd) +AutoCloseFD& AutoCloseFD::operator =(AutoCloseFD&& that) { - this->fd = fd; -} - - -AutoCloseFD::AutoCloseFD(const AutoCloseFD & fd) -{ - /* Copying an AutoCloseFD isn't allowed (who should get to close - it?). But as an edge case, allow copying of closed - AutoCloseFDs. This is necessary due to tiresome reasons - involving copy constructor use on default object values in STL - containers (like when you do `map[value]' where value isn't in - the map yet). */ - this->fd = fd.fd; - if (this->fd != -1) abort(); + close(); + fd = that.fd; + that.fd = -1; + return *this; } @@ -591,14 +587,7 @@ AutoCloseFD::~AutoCloseFD() } -void AutoCloseFD::operator =(int fd) -{ - if (this->fd != fd) close(); - this->fd = fd; -} - - -AutoCloseFD::operator int() const +int AutoCloseFD::get() const { return fd; } @@ -610,19 +599,17 @@ void AutoCloseFD::close() if (::close(fd) == -1) /* This should never happen. */ throw SysError(format("closing file descriptor %1%") % fd); - fd = -1; } } -bool AutoCloseFD::isOpen() +AutoCloseFD::operator bool() const { return fd != -1; } -/* Pass responsibility for closing this fd to the caller. */ -int AutoCloseFD::borrow() +int AutoCloseFD::release() { int oldFD = fd; fd = -1; @@ -899,10 +886,10 @@ string runProgram(Path program, bool searchPath, const Strings & args, /* Fork. */ Pid pid = startProcess([&]() { - if (dup2(out.writeSide, STDOUT_FILENO) == -1) + if (dup2(out.writeSide.get(), STDOUT_FILENO) == -1) throw SysError("dupping stdout"); if (!input.empty()) { - if (dup2(in.readSide, STDIN_FILENO) == -1) + if (dup2(in.readSide.get(), STDIN_FILENO) == -1) throw SysError("dupping stdin"); } @@ -917,16 +904,16 @@ string runProgram(Path program, bool searchPath, const Strings & args, throw SysError(format("executing ‘%1%’") % program); }); - out.writeSide.close(); + out.writeSide = -1; /* FIXME: This can deadlock if the input is too long. */ if (!input.empty()) { - in.readSide.close(); - writeFull(in.writeSide, input); - in.writeSide.close(); + in.readSide = -1; + writeFull(in.writeSide.get(), input); + in.writeSide = -1; } - string result = drainFD(out.readSide); + string result = drainFD(out.readSide.get()); /* Wait for the child to finish. */ int status = pid.wait(true); diff --git a/src/libutil/util.hh b/src/libutil/util.hh index ab43637a5..819921dff 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -164,16 +164,18 @@ public: class AutoCloseFD { int fd; + void close(); public: AutoCloseFD(); AutoCloseFD(int fd); - AutoCloseFD(const AutoCloseFD & fd); + AutoCloseFD(const AutoCloseFD & fd) = delete; + AutoCloseFD(AutoCloseFD&& fd); ~AutoCloseFD(); - void operator =(int fd); - operator int() const; - void close(); - bool isOpen(); - int borrow(); + AutoCloseFD& operator =(const AutoCloseFD & fd) = delete; + AutoCloseFD& operator =(AutoCloseFD&& fd); + int get() const; + explicit operator bool() const; + int release(); }; diff --git a/src/nix-daemon/nix-daemon.cc b/src/nix-daemon/nix-daemon.cc index 6a992b953..6e0d869f4 100644 --- a/src/nix-daemon/nix-daemon.cc +++ b/src/nix-daemon/nix-daemon.cc @@ -767,7 +767,7 @@ static void daemonLoop(char * * argv) /* Create and bind to a Unix domain socket. */ fdSocket = socket(PF_UNIX, SOCK_STREAM, 0); - if (fdSocket == -1) + if (!fdSocket) throw SysError("cannot create Unix domain socket"); string socketPath = settings.nixDaemonSocketFile; @@ -793,7 +793,7 @@ static void daemonLoop(char * * argv) (everybody can connect --- provided they have access to the directory containing the socket). */ mode_t oldMode = umask(0111); - int res = bind(fdSocket, (struct sockaddr *) &addr, sizeof(addr)); + int res = bind(fdSocket.get(), (struct sockaddr *) &addr, sizeof(addr)); umask(oldMode); if (res == -1) throw SysError(format("cannot bind to socket ‘%1%’") % socketPath); @@ -801,11 +801,11 @@ static void daemonLoop(char * * argv) if (chdir("/") == -1) /* back to the root */ throw SysError("cannot change current directory"); - if (listen(fdSocket, 5) == -1) + if (listen(fdSocket.get(), 5) == -1) throw SysError(format("cannot listen on socket ‘%1%’") % socketPath); } - closeOnExec(fdSocket); + closeOnExec(fdSocket.get()); /* Loop accepting connections. */ while (1) { @@ -815,18 +815,18 @@ static void daemonLoop(char * * argv) struct sockaddr_un remoteAddr; socklen_t remoteAddrLen = sizeof(remoteAddr); - AutoCloseFD remote = accept(fdSocket, + AutoCloseFD remote = accept(fdSocket.get(), (struct sockaddr *) &remoteAddr, &remoteAddrLen); checkInterrupt(); - if (remote == -1) { + if (!remote) { if (errno == EINTR) continue; throw SysError("accepting connection"); } - closeOnExec(remote); + closeOnExec(remote.get()); bool trusted = false; - PeerInfo peer = getPeerInfo(remote); + PeerInfo peer = getPeerInfo(remote.get()); struct passwd * pw = peer.uidKnown ? getpwuid(peer.uid) : 0; string user = pw ? pw->pw_name : std::to_string(peer.uid); @@ -854,7 +854,7 @@ static void daemonLoop(char * * argv) options.runExitHandlers = true; options.allowVfork = false; startProcess([&]() { - fdSocket.close(); + fdSocket = -1; /* Background the daemon. */ if (setsid() == -1) @@ -870,8 +870,8 @@ static void daemonLoop(char * * argv) } /* Handle the connection. */ - from.fd = remote; - to.fd = remote; + from.fd = remote.get(); + to.fd = remote.get(); processConnection(trusted); exit(0);