Merge pull request #15079 from NixOS/auto-cleanup-cleanups

Clean up `AutoRemoveJail`, `AutoDelete`, and `AutoUnmount`

src/libfetchers/tarball.cc

@@ -177,7 +177,8 @@ static DownloadTarballResult downloadTarball_(
            the entire file to disk so libarchive can access it
            in random-access mode. */
         auto [fdTemp, path] = createTempFile("nix-zipfile");
-        cleanupTemp.reset(path);
+        cleanupTemp.cancel();
+        cleanupTemp = {path};
         debug("downloading '%s' into '%s'...", url, path);
         {
             FdSink sink(fdTemp.get());

src/libutil/file-system.cc

@@ -465,33 +465,31 @@ AutoDelete::AutoDelete(const std::filesystem::path & p, bool recursive)
     this->recursive = recursive;
 }
 
+void AutoDelete::deletePath()
+{
+    if (del) {
+        if (recursive)
+            nix::deletePath(_path);
+        else
+            std::filesystem::remove(_path);
+        cancel();
+    }
+}
+
 AutoDelete::~AutoDelete()
 {
     try {
-        if (del) {
-            if (recursive)
-                deletePath(_path);
-            else {
-                std::filesystem::remove(_path);
-            }
-        }
+        deletePath();
     } catch (...) {
         ignoreExceptionInDestructor();
     }
 }
 
-void AutoDelete::cancel()
+void AutoDelete::cancel() noexcept
 {
     del = false;
 }
 
-void AutoDelete::reset(const std::filesystem::path & p, bool recursive)
-{
-    _path = p;
-    this->recursive = recursive;
-    del = true;
-}
-
 //////////////////////////////////////////////////////////////////////
 
 #ifdef __FreeBSD__
@@ -500,7 +498,7 @@ AutoUnmount::AutoUnmount()
 {
 }
 
-AutoUnmount::AutoUnmount(Path & p)
+AutoUnmount::AutoUnmount(const std::filesystem::path & p)
     : path(p)
     , del(true)
 {
@@ -509,20 +507,26 @@ AutoUnmount::AutoUnmount(Path & p)
 AutoUnmount::~AutoUnmount()
 {
     try {
-        if (del) {
-            if (unmount(path.c_str(), 0) < 0) {
-                throw SysError("Failed to unmount path %1%", path);
-            }
-        }
+        unmount();
     } catch (...) {
         ignoreExceptionInDestructor();
     }
 }
 
-void AutoUnmount::cancel()
+void AutoUnmount::cancel() noexcept
 {
     del = false;
 }
+
+void AutoUnmount::unmount()
+{
+    if (del) {
+        if (::unmount(path.c_str(), 0) < 0) {
+            throw SysError("Failed to unmount path %1%", PathFmt(path));
+        }
+    }
+    cancel();
+}
 #endif
 
 //////////////////////////////////////////////////////////////////////

src/libutil/freebsd/freebsd-jail.cc

@@ -11,39 +11,35 @@
 
 namespace nix {
 
-AutoRemoveJail::AutoRemoveJail()
-    : del{false}
-{
-}
+AutoRemoveJail::AutoRemoveJail() = default;
 
 AutoRemoveJail::AutoRemoveJail(int jid)
     : jid(jid)
-    , del(true)
 {
 }
 
+void AutoRemoveJail::remove()
+{
+    if (jid != INVALID_JAIL) {
+        if (jail_remove(jid) < 0) {
+            throw SysError("Failed to remove jail %1%", jid);
+        }
+    }
+    cancel();
+}
+
 AutoRemoveJail::~AutoRemoveJail()
 {
     try {
-        if (del) {
-            if (jail_remove(jid) < 0) {
-                throw SysError("Failed to remove jail %1%", jid);
-            }
-        }
+        remove();
     } catch (...) {
         ignoreExceptionInDestructor();
     }
 }
 
-void AutoRemoveJail::cancel()
+void AutoRemoveJail::cancel() noexcept
 {
-    del = false;
-}
-
-void AutoRemoveJail::reset(int j)
-{
-    del = true;
-    jid = j;
+    jid = INVALID_JAIL;
 }
 
 //////////////////////////////////////////////////////////////////////

src/libutil/freebsd/include/nix/util/freebsd-jail.hh

@@ -7,32 +7,51 @@ namespace nix {
 
 class AutoRemoveJail
 {
-    int jid;
-    bool del;
+    static constexpr int INVALID_JAIL = -1;
+    int jid = INVALID_JAIL;
 public:
+    AutoRemoveJail() = default;
     AutoRemoveJail(int jid);
     AutoRemoveJail(const AutoRemoveJail &) = delete;
     AutoRemoveJail & operator=(const AutoRemoveJail &) = delete;
 
     AutoRemoveJail(AutoRemoveJail && other) noexcept
         : jid(other.jid)
-        , del(other.del)
     {
         other.cancel();
     }
 
     AutoRemoveJail & operator=(AutoRemoveJail && other) noexcept
     {
-        jid = other.jid;
-        del = other.del;
-        other.cancel();
+        swap(*this, other);
         return *this;
     }
 
-    AutoRemoveJail();
+    friend void swap(AutoRemoveJail & lhs, AutoRemoveJail & rhs) noexcept
+    {
+        using std::swap;
+        swap(lhs.jid, rhs.jid);
+    }
+
+    operator int() const
+    {
+        return jid;
+    }
+
     ~AutoRemoveJail();
-    void cancel();
-    void reset(int j);
+
+    /**
+     * Remove the jail and cancel this `AutoRemoveJail`, so jail removal is not
+     * attempted a second time by the destructor.
+     *
+     * The destructor calls this ignoring any exception.
+     */
+    void remove();
+
+    /**
+     * Cancel the jail removal.
+     */
+    void cancel() noexcept;
 };
 
 } // namespace nix

src/libutil/include/nix/util/file-system.hh

@@ -332,15 +332,37 @@ public:
         x.del = false;
     }
 
+    AutoDelete & operator=(AutoDelete && x) noexcept
+    {
+        swap(*this, x);
+        return *this;
+    }
+
+    friend void swap(AutoDelete & lhs, AutoDelete & rhs) noexcept
+    {
+        using std::swap;
+        swap(lhs._path, rhs._path);
+        swap(lhs.del, rhs.del);
+        swap(lhs.recursive, rhs.recursive);
+    }
+
     AutoDelete(const std::filesystem::path & p, bool recursive = true);
     AutoDelete(const AutoDelete &) = delete;
-    AutoDelete & operator=(AutoDelete &&) = delete;
     AutoDelete & operator=(const AutoDelete &) = delete;
     ~AutoDelete();
 
-    void cancel();
+    /**
+     * Delete the file the path points to, and cancel this `AutoDelete`,
+     * so deletion is not attempted a second time by the destructor.
+     *
+     * The destructor calls this, but ignoring any exception.
+     */
+    void deletePath();
 
-    void reset(const std::filesystem::path & p, bool recursive = true);
+    /**
+     * Cancel the pending deletion
+     */
+    void cancel() noexcept;
 
     const std::filesystem::path & path() const
     {
@@ -520,11 +542,11 @@ private:
 #ifdef __FreeBSD__
 class AutoUnmount
 {
-    Path path;
+    std::filesystem::path path;
     bool del;
 public:
     AutoUnmount();
-    AutoUnmount(Path &);
+    AutoUnmount(const std::filesystem::path &);
     AutoUnmount(const AutoUnmount &) = delete;
 
     AutoUnmount(AutoUnmount && other) noexcept
@@ -535,13 +557,31 @@ public:
 
     AutoUnmount & operator=(AutoUnmount && other) noexcept
     {
-        path = std::move(other.path);
-        del = std::exchange(other.del, false);
+        swap(*this, other);
         return *this;
     }
 
+    friend void swap(AutoUnmount & lhs, AutoUnmount & rhs) noexcept
+    {
+        using std::swap;
+        swap(lhs.path, rhs.path);
+        swap(lhs.del, rhs.del);
+    }
+
     ~AutoUnmount();
-    void cancel();
+
+    /**
+     * Cancel the unmounting
+     */
+    void cancel() noexcept;
+
+    /**
+     * Unmount the mountpoint right away (if it exists), resetting the
+     * `AutoUnmount`
+     *
+     * The destructor calls this, but ignoring any exception.
+     */
+    void unmount();
 };
 #endif