From 5d1c05b07561c841c68eb3ff9698ce9d2355fe41 Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 9 Nov 2020 13:47:06 +0100 Subject: SubstitutionGoal -> PathSubstitutionGoal To prepare for the upcoming DrvOutputSubstitutionGoal --- src/libstore/build/substitution-goal.cc | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) (limited to 'src/libstore/build/substitution-goal.cc') diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index c4b0de78d..5d88b8758 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -5,20 +5,20 @@ namespace nix { -SubstitutionGoal::SubstitutionGoal(const StorePath & storePath, Worker & worker, RepairFlag repair, std::optional ca) +PathSubstitutionGoal::PathSubstitutionGoal(const StorePath & storePath, Worker & worker, RepairFlag repair, std::optional ca) : Goal(worker) , storePath(storePath) , repair(repair) , ca(ca) { - state = &SubstitutionGoal::init; + state = &PathSubstitutionGoal::init; name = fmt("substitution of '%s'", worker.store.printStorePath(this->storePath)); trace("created"); maintainExpectedSubstitutions = std::make_unique>(worker.expectedSubstitutions); } -SubstitutionGoal::~SubstitutionGoal() +PathSubstitutionGoal::~PathSubstitutionGoal() { try { if (thr.joinable()) { @@ -32,13 +32,13 @@ SubstitutionGoal::~SubstitutionGoal() } -void SubstitutionGoal::work() +void PathSubstitutionGoal::work() { (this->*state)(); } -void SubstitutionGoal::init() +void PathSubstitutionGoal::init() { trace("init"); @@ -59,7 +59,7 @@ void SubstitutionGoal::init() } -void SubstitutionGoal::tryNext() +void PathSubstitutionGoal::tryNext() { trace("trying next substituter"); @@ -154,16 +154,16 @@ void SubstitutionGoal::tryNext() paths referenced by this one. */ for (auto & i : info->references) if (i != storePath) /* ignore self-references */ - addWaitee(worker.makeSubstitutionGoal(i)); + addWaitee(worker.makePathSubstitutionGoal(i)); if (waitees.empty()) /* to prevent hang (no wake-up event) */ referencesValid(); else - state = &SubstitutionGoal::referencesValid; + state = &PathSubstitutionGoal::referencesValid; } -void SubstitutionGoal::referencesValid() +void PathSubstitutionGoal::referencesValid() { trace("all references realised"); @@ -177,12 +177,12 @@ void SubstitutionGoal::referencesValid() if (i != storePath) /* ignore self-references */ assert(worker.store.isValidPath(i)); - state = &SubstitutionGoal::tryToRun; + state = &PathSubstitutionGoal::tryToRun; worker.wakeUp(shared_from_this()); } -void SubstitutionGoal::tryToRun() +void PathSubstitutionGoal::tryToRun() { trace("trying to run"); @@ -221,11 +221,11 @@ void SubstitutionGoal::tryToRun() worker.childStarted(shared_from_this(), {outPipe.readSide.get()}, true, false); - state = &SubstitutionGoal::finished; + state = &PathSubstitutionGoal::finished; } -void SubstitutionGoal::finished() +void PathSubstitutionGoal::finished() { trace("substitute finished"); @@ -249,7 +249,7 @@ void SubstitutionGoal::finished() } /* Try the next substitute. */ - state = &SubstitutionGoal::tryNext; + state = &PathSubstitutionGoal::tryNext; worker.wakeUp(shared_from_this()); return; } @@ -278,12 +278,12 @@ void SubstitutionGoal::finished() } -void SubstitutionGoal::handleChildOutput(int fd, const string & data) +void PathSubstitutionGoal::handleChildOutput(int fd, const string & data) { } -void SubstitutionGoal::handleEOF(int fd) +void PathSubstitutionGoal::handleEOF(int fd) { if (fd == outPipe.readSide.get()) worker.wakeUp(shared_from_this()); } -- cgit v1.2.3 From 3e6017f911127555cfbed71fe4a4df8f70d08bbb Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 8 Mar 2021 15:07:33 +0100 Subject: pathInfoIsTrusted -> pathInfoIsUntrusted I guess the rationale behind the old name wath that `pathInfoIsTrusted(info)` returns `true` iff we would need to `blindly` trust the path (because it has no valid signature and `requireSigs` is set), but I find it to be a really confusing footgun because it's quite natural to give it the opposite meaning. --- src/libstore/build/substitution-goal.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/libstore/build/substitution-goal.cc') diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 5d88b8758..7b1ac126e 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -142,7 +142,7 @@ void PathSubstitutionGoal::tryNext() /* Bail out early if this substituter lacks a valid signature. LocalStore::addToStore() also checks for this, but only after we've downloaded the path. */ - if (!sub->isTrusted && worker.store.pathInfoIsTrusted(*info)) + if (!sub->isTrusted && worker.store.pathInfoIsUntrusted(*info)) { warn("substituter '%s' does not have a valid signature for path '%s'", sub->getUri(), worker.store.printStorePath(storePath)); -- cgit v1.2.3 From 8a29052cb2f52ef2c82c36fb3818fd0f66349729 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 7 Apr 2021 12:21:31 +0200 Subject: PathSubstitutionGoal: Clean up pipe If there were many top-level goals (which are not destroyed until the very end), commands like $ nix copy --to 'ssh://localhost?remote-store=/tmp/nix' \ /run/current-system --no-check-sigs --substitute-on-destination could fail with "Too many open files". So now we do some explicit cleanup from amDone(). It would be cleaner to separate goals from their temporary internal state, but that would be a bigger refactor. --- src/libstore/build/substitution-goal.cc | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) (limited to 'src/libstore/build/substitution-goal.cc') diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 7b1ac126e..e56cfadbe 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -20,15 +20,7 @@ PathSubstitutionGoal::PathSubstitutionGoal(const StorePath & storePath, Worker & PathSubstitutionGoal::~PathSubstitutionGoal() { - try { - if (thr.joinable()) { - // FIXME: signal worker thread to quit. - thr.join(); - worker.childTerminated(this); - } - } catch (...) { - ignoreException(); - } + cleanup(); } @@ -63,6 +55,8 @@ void PathSubstitutionGoal::tryNext() { trace("trying next substituter"); + cleanup(); + if (subs.size() == 0) { /* None left. Terminate this goal and let someone else deal with it. */ @@ -205,7 +199,7 @@ void PathSubstitutionGoal::tryToRun() thr = std::thread([this]() { try { /* Wake up the worker loop when we're done. */ - Finally updateStats([this]() { outPipe.writeSide = -1; }); + Finally updateStats([this]() { outPipe.writeSide.close(); }); Activity act(*logger, actSubstitute, Logger::Fields{worker.store.printStorePath(storePath), sub->getUri()}); PushActivity pact(act.id); @@ -288,4 +282,21 @@ void PathSubstitutionGoal::handleEOF(int fd) if (fd == outPipe.readSide.get()) worker.wakeUp(shared_from_this()); } + +void PathSubstitutionGoal::cleanup() +{ + try { + if (thr.joinable()) { + // FIXME: signal worker thread to quit. + thr.join(); + worker.childTerminated(this); + } + + outPipe.close(); + } catch (...) { + ignoreException(); + } +} + + } -- cgit v1.2.3