From b9c2b4d5b4cd5d52a950e6dd90eb2e2e79891fa0 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 19 Sep 2012 16:17:54 -0400 Subject: [PATCH] Remove setting of the immutable bit Using the immutable bit is problematic, especially in conjunction with store optimisation. For instance, if the garbage collector deletes a file, it has to clear its immutable bit, but if the file has additional hard links, we can't set the bit afterwards because we don't know the remaining paths. So now that we support having the entire Nix store as a read-only mount, we may as well drop the immutable bit. Unfortunately, we have to keep the code to clear the immutable bit for backwards compatibility. --- src/libstore/build.cc | 13 +---- src/libstore/local-store.cc | 2 - src/libstore/optimise-store.cc | 86 ++++++++++++---------------------- src/libutil/immutable.cc | 22 +-------- src/libutil/immutable.hh | 6 --- 5 files changed, 34 insertions(+), 95 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 0b7790a5bb..4051adacca 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1355,11 +1355,6 @@ void DerivationGoal::buildDone() /* Delete the chroot (if we were using one). */ autoDelChroot.reset(); /* this runs the destructor */ - /* Deleting the chroot will have caused the immutable bits on - hard-linked inputs to be cleared. So set them again. */ - foreach (PathSet::iterator, i, regularInputPaths) - makeImmutable(*i); - /* Delete redirected outputs (when doing hash rewriting). */ foreach (PathSet::iterator, i, redirectedOutputs) deletePath(*i); @@ -1766,11 +1761,8 @@ void DerivationGoal::startBuilder() /* Hard-linking fails if we exceed the maximum link count on a file (e.g. 32000 of ext3), which is quite possible after a `nix-store - --optimise'. It can also fail if another - process called makeImmutable() on *i after we - did makeMutable(). In those cases, make a copy - instead. */ - if (errno != EMLINK && errno != EPERM) + --optimise'. */ + if (errno != EMLINK) throw SysError(format("linking `%1%' to `%2%'") % p % *i); StringSink sink; dumpPath(*i, sink); @@ -1778,7 +1770,6 @@ void DerivationGoal::startBuilder() restorePath(p, source); } - makeImmutable(*i); regularInputPaths.insert(*i); } } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index bb755b8211..3fd772f26f 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -517,8 +517,6 @@ void canonicalisePathMetaData(const Path & path, bool recurse) foreach (Strings::iterator, i, names) canonicalisePathMetaData(path + "/" + *i, true); } - - makeImmutable(path); } diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 9d0242bbc8..43b3c9b4fb 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -33,8 +33,7 @@ struct MakeReadOnly ~MakeReadOnly() { try { - /* This will make the path read-only (and restore the - immutable bit on platforms that support it). */ + /* This will make the path read-only. */ if (path != "") canonicalisePathMetaData(path, false); } catch (...) { ignoreException(); @@ -43,14 +42,6 @@ struct MakeReadOnly }; -struct MakeImmutable -{ - Path path; - MakeImmutable(const Path & path) : path(path) { } - ~MakeImmutable() { makeImmutable(path); } -}; - - void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) { checkInterrupt(); @@ -101,7 +92,6 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) if (!pathExists(linkPath)) { /* Nope, create a hard link in the links directory. */ makeMutable(path); - MakeImmutable mk1(path); if (link(path.c_str(), linkPath.c_str()) == 0) return; if (errno != EEXIST) throw SysError(format("cannot link `%1%' to `%2%'") % linkPath % path); @@ -134,57 +124,41 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) MakeReadOnly makeReadOnly(mustToggle ? dirOf(path) : ""); /* If ‘linkPath’ is immutable, we can't create hard links to it, - so make it mutable first (and make it immutable again when - we're done). We also have to make ‘path’ mutable, otherwise - rename() will fail to delete it. */ + so make it mutable first. We also have to make ‘path’ mutable, + otherwise rename() will fail to delete it. */ makeMutable(path); - MakeImmutable mk2(path); + makeMutable(linkPath); - /* Another process might be doing the same thing (creating a new - link to ‘linkPath’) and make ‘linkPath’ immutable before we're - done. In that case, just retry. */ - unsigned int retries = 1024; - while (--retries > 0) { - makeMutable(linkPath); - MakeImmutable mk1(linkPath); + Path tempLink = (format("%1%/.tmp-link-%2%-%3%") + % settings.nixStore % getpid() % rand()).str(); - Path tempLink = (format("%1%/.tmp-link-%2%-%3%") - % settings.nixStore % getpid() % rand()).str(); - - if (link(linkPath.c_str(), tempLink.c_str()) == -1) { - if (errno == EMLINK) { - /* Too many links to the same file (>= 32000 on most - file systems). This is likely to happen with empty - files. Just shrug and ignore. */ - if (st.st_size) - printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); - return; - } - if (errno == EPERM) continue; - throw SysError(format("cannot link `%1%' to `%2%'") % tempLink % linkPath); + if (link(linkPath.c_str(), tempLink.c_str()) == -1) { + if (errno == EMLINK) { + /* Too many links to the same file (>= 32000 on most file + systems). This is likely to happen with empty files. + Just shrug and ignore. */ + if (st.st_size) + printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); + return; } - - /* Atomically replace the old file with the new hard link. */ - if (rename(tempLink.c_str(), path.c_str()) == -1) { - if (unlink(tempLink.c_str()) == -1) - printMsg(lvlError, format("unable to unlink `%1%'") % tempLink); - if (errno == EMLINK) { - /* Some filesystems generate too many links on the - rename, rather than on the original link. - (Probably it temporarily increases the st_nlink - field before decreasing it again.) */ - if (st.st_size) - printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); - return; - } - if (errno == EPERM) continue; - throw SysError(format("cannot rename `%1%' to `%2%'") % tempLink % path); - } - - break; + throw SysError(format("cannot link `%1%' to `%2%'") % tempLink % linkPath); } - if (retries == 0) throw Error(format("cannot link `%1%' to `%2%'") % path % linkPath); + /* Atomically replace the old file with the new hard link. */ + if (rename(tempLink.c_str(), path.c_str()) == -1) { + if (unlink(tempLink.c_str()) == -1) + printMsg(lvlError, format("unable to unlink `%1%'") % tempLink); + if (errno == EMLINK) { + /* Some filesystems generate too many links on the rename, + rather than on the original link. (Probably it + temporarily increases the st_nlink field before + decreasing it again.) */ + if (st.st_size) + printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); + return; + } + throw SysError(format("cannot rename `%1%' to `%2%'") % tempLink % path); + } stats.filesLinked++; stats.bytesFreed += st.st_size; diff --git a/src/libutil/immutable.cc b/src/libutil/immutable.cc index f72f856254..766af49393 100644 --- a/src/libutil/immutable.cc +++ b/src/libutil/immutable.cc @@ -16,7 +16,7 @@ namespace nix { -void changeMutable(const Path & path, bool mut) +void makeMutable(const Path & path) { #if defined(FS_IOC_SETFLAGS) && defined(FS_IOC_GETFLAGS) && defined(FS_IMMUTABLE_FL) @@ -38,30 +38,12 @@ void changeMutable(const Path & path, bool mut) /* Silently ignore errors getting/setting the immutable flag so that we work correctly on filesystems that don't support it. */ if (ioctl(fd, FS_IOC_GETFLAGS, &flags)) return; - old = flags; - - if (mut) flags &= ~FS_IMMUTABLE_FL; - else flags |= FS_IMMUTABLE_FL; - + flags &= ~FS_IMMUTABLE_FL; if (old == flags) return; - if (ioctl(fd, FS_IOC_SETFLAGS, &flags)) return; - #endif } -void makeImmutable(const Path & path) -{ - changeMutable(path, false); -} - - -void makeMutable(const Path & path) -{ - changeMutable(path, true); -} - - } diff --git a/src/libutil/immutable.hh b/src/libutil/immutable.hh index 8af4190049..8e98b76a41 100644 --- a/src/libutil/immutable.hh +++ b/src/libutil/immutable.hh @@ -4,12 +4,6 @@ namespace nix { -/* Make the given path immutable, i.e., prevent it from being modified - in any way, even by root. This is a no-op on platforms that do not - support this, or if the calling user is not privileged. On Linux, - this is implemented by doing the equivalent of ‘chattr +i path’. */ -void makeImmutable(const Path & path); - /* Make the given path mutable. */ void makeMutable(const Path & path);