unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
* [bug#43340] [PATCH 0/5] Speed up archive export/import
@ 2020-09-11 14:40 Ludovic Courtès
  2020-09-11 14:51 ` [bug#43340] [PATCH 1/5] daemon: Generalize 'HookInstance' to 'Agent' Ludovic Courtès
  2020-09-11 15:01 ` [bug#43340] [PATCH 0/5] Speed up archive export/import Ludovic Courtès
  0 siblings, 2 replies; 12+ messages in thread
From: Ludovic Courtès @ 2020-09-11 14:40 UTC (permalink / raw)
  To: 43340; +Cc: Ludovic Courtès

Hi!

This patch series goes on top of <https://issues.guix.gnu.org/43285>.
It addresses the performance issue described at:

  https://lists.gnu.org/archive/html/guix-devel/2020-09/msg00073.html

Specifically, it implements option #4 (spawning ‘guix authenticate’
once for the whole session, instead of spawning it every time a
store item needs to be signed or authenticated), achieving a ~15x
speedup, which is not bad.  :-)

There’s way more C++ code than I would like, and it’s probably not
pretty code (I always end up typing things like “C++ list append”
in a search engine to find the obscure incantation that does that).
There’s now a query/reply protocol between the daemon and ‘guix
authenticate’.  Nothing fancy, but it allows for strings that contain
whitespace or newlines, which is necessary here.

That’s it!

Ludo’.

Ludovic Courtès (5):
  daemon: Generalize 'HookInstance' to 'Agent'.
  daemon: Isolate signing and signature verification functions.
  daemon: Move 'Agent' to libutil.
  daemon: Spawn 'guix authenticate' once for all.
  authenticate: Cache the ACL and key pairs.

 guix/scripts/authenticate.scm | 163 ++++++++++++++++++++++++++--------
 nix/libstore/build.cc         | 146 +++++-------------------------
 nix/libstore/local-store.cc   | 103 +++++++++++++++++----
 nix/libutil/util.cc           |  84 ++++++++++++++++++
 nix/libutil/util.hh           |  25 ++++++
 tests/guix-authenticate.sh    |  45 ++++++----
 tests/store.scm               |   8 +-
 7 files changed, 372 insertions(+), 202 deletions(-)

-- 
2.28.0





^ permalink raw reply	[flat|nested] 12+ messages in thread

* [bug#43340] [PATCH 1/5] daemon: Generalize 'HookInstance' to 'Agent'.
  2020-09-11 14:40 [bug#43340] [PATCH 0/5] Speed up archive export/import Ludovic Courtès
@ 2020-09-11 14:51 ` Ludovic Courtès
  2020-09-11 14:51   ` [bug#43340] [PATCH 2/5] daemon: Isolate signing and signature verification functions Ludovic Courtès
                     ` (3 more replies)
  2020-09-11 15:01 ` [bug#43340] [PATCH 0/5] Speed up archive export/import Ludovic Courtès
  1 sibling, 4 replies; 12+ messages in thread
From: Ludovic Courtès @ 2020-09-11 14:51 UTC (permalink / raw)
  To: 43340; +Cc: Ludovic Courtès

* nix/libstore/build.cc (HookInstance): Rename to...
(Agent): ... this.  Rename 'toHook' and 'fromHook' similarly and update
users.  Change constructor to require a command and an argument list.
(DerivationGoal::tryBuildHook): Pass arguments to the 'Agent'
constructor.
---
 nix/libstore/build.cc | 90 ++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 39 deletions(-)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 29266f1dd6..b6fad493a9 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -85,7 +85,7 @@ static string pathNullDevice = "/dev/null";
 
 /* Forward definition. */
 class Worker;
-struct HookInstance;
+struct Agent;
 
 
 /* A pointer to a goal. */
@@ -265,7 +265,7 @@ public:
 
     LocalStore & store;
 
-    std::shared_ptr<HookInstance> hook;
+    std::shared_ptr<Agent> hook;
 
     Worker(LocalStore & store);
     ~Worker();
@@ -590,37 +590,41 @@ void UserLock::kill()
 //////////////////////////////////////////////////////////////////////
 
 
-struct HookInstance
+/* An "agent" is a helper program that runs in the background and that we talk
+   to over pipes, such as the "guix offload" program.  */
+struct Agent
 {
-    /* Pipes for talking to the build hook. */
-    Pipe toHook;
+    /* Pipes for talking to the agent. */
+    Pipe toAgent;
 
-    /* Pipe for the hook's standard output/error. */
-    Pipe fromHook;
+    /* Pipe for the agent's standard output/error. */
+    Pipe fromAgent;
 
-    /* Pipe for the builder's standard output/error. */
+    /* Pipe for build standard output/error--e.g., for build processes started
+       by "guix offload".  */
     Pipe builderOut;
 
-    /* The process ID of the hook. */
+    /* The process ID of the agent. */
     Pid pid;
 
-    HookInstance();
+    /* The 'guix' sub-command and arguments passed to the agent.  */
+    Agent(const string &command, const Strings &args);
 
-    ~HookInstance();
+    ~Agent();
 };
 
 
-HookInstance::HookInstance()
+Agent::Agent(const string &command, const Strings &args)
 {
-    debug("starting build hook");
+    debug(format("starting agent '%1%'") % command);
 
     const Path &buildHook = settings.guixProgram;
 
     /* Create a pipe to get the output of the child. */
-    fromHook.create();
+    fromAgent.create();
 
     /* Create the communication pipes. */
-    toHook.create();
+    toAgent.create();
 
     /* Create a pipe to get the output of the builder. */
     builderOut.create();
@@ -628,38 +632,38 @@ HookInstance::HookInstance()
     /* Fork the hook. */
     pid = startProcess([&]() {
 
-        commonChildInit(fromHook);
+        commonChildInit(fromAgent);
 
         if (chdir("/") == -1) throw SysError("changing into `/");
 
         /* Dup the communication pipes. */
-        if (dup2(toHook.readSide, STDIN_FILENO) == -1)
+        if (dup2(toAgent.readSide, 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)
             throw SysError("dupping builder's stdout/stderr");
 
-        execl(buildHook.c_str(), buildHook.c_str(), "offload",
-	    settings.thisSystem.c_str(),
-            (format("%1%") % settings.maxSilentTime).str().c_str(),
-            (format("%1%") % settings.printBuildTrace).str().c_str(),
-            (format("%1%") % settings.buildTimeout).str().c_str(),
-            NULL);
+	Strings allArgs;
+	allArgs.push_back(buildHook);
+	allArgs.push_back(command);
+	allArgs.insert(allArgs.end(), args.begin(), args.end()); // append
 
-        throw SysError(format("executing `%1% offload'") % buildHook);
+        execv(buildHook.c_str(), stringsToCharPtrs(allArgs).data());
+
+        throw SysError(format("executing `%1% %2%'") % buildHook % command);
     });
 
     pid.setSeparatePG(true);
-    fromHook.writeSide.close();
-    toHook.readSide.close();
+    fromAgent.writeSide.close();
+    toAgent.readSide.close();
 }
 
 
-HookInstance::~HookInstance()
+Agent::~Agent()
 {
     try {
-        toHook.writeSide.close();
+        toAgent.writeSide.close();
         pid.kill(true);
     } catch (...) {
         ignoreException();
@@ -760,7 +764,7 @@ private:
     Pipe builderOut;
 
     /* The build hook. */
-    std::shared_ptr<HookInstance> hook;
+    std::shared_ptr<Agent> hook;
 
     /* Whether we're currently doing a chroot build. */
     bool useChroot;
@@ -1440,7 +1444,7 @@ void DerivationGoal::buildDone()
     /* Close the read side of the logger pipe. */
     if (hook) {
         hook->builderOut.readSide.close();
-        hook->fromHook.readSide.close();
+        hook->fromAgent.readSide.close();
     }
     else builderOut.readSide.close();
 
@@ -1587,8 +1591,16 @@ HookReply DerivationGoal::tryBuildHook()
 {
     if (!settings.useBuildHook) return rpDecline;
 
-    if (!worker.hook)
-        worker.hook = std::shared_ptr<HookInstance>(new HookInstance);
+    if (!worker.hook) {
+	Strings args = {
+	    settings.thisSystem.c_str(),
+            (format("%1%") % settings.maxSilentTime).str().c_str(),
+            (format("%1%") % settings.printBuildTrace).str().c_str(),
+            (format("%1%") % settings.buildTimeout).str().c_str()
+	};
+
+        worker.hook = std::shared_ptr<Agent>(new Agent("offload", args));
+    }
 
     /* Tell the hook about system features (beyond the system type)
        required from the build machine.  (The hook could parse the
@@ -1597,7 +1609,7 @@ HookReply DerivationGoal::tryBuildHook()
     foreach (Strings::iterator, i, features) checkStoreName(*i); /* !!! abuse */
 
     /* Send the request to the hook. */
-    writeLine(worker.hook->toHook.writeSide, (format("%1% %2% %3% %4%")
+    writeLine(worker.hook->toAgent.writeSide, (format("%1% %2% %3% %4%")
         % (worker.getNrLocalBuilds() < settings.maxBuildJobs ? "1" : "0")
         % drv.platform % drvPath % concatStringsSep(",", features)).str());
 
@@ -1605,7 +1617,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->fromAgent.readSide);
         if (string(s, 0, 2) == "# ") {
             reply = string(s, 2);
             break;
@@ -1637,21 +1649,21 @@ HookReply DerivationGoal::tryBuildHook()
 
     string s;
     foreach (PathSet::iterator, i, allInputs) { s += *i; s += ' '; }
-    writeLine(hook->toHook.writeSide, s);
+    writeLine(hook->toAgent.writeSide, s);
 
     /* Tell the hooks the missing outputs that have to be copied back
        from the remote system. */
     s = "";
     foreach (PathSet::iterator, i, missingPaths) { s += *i; s += ' '; }
-    writeLine(hook->toHook.writeSide, s);
+    writeLine(hook->toAgent.writeSide, s);
 
-    hook->toHook.writeSide.close();
+    hook->toAgent.writeSide.close();
 
     /* Create the log file and pipe. */
     Path logFile = openLogFile();
 
     set<int> fds;
-    fds.insert(hook->fromHook.readSide);
+    fds.insert(hook->fromAgent.readSide);
     fds.insert(hook->builderOut.readSide);
     worker.childStarted(shared_from_this(), hook->pid, fds, false, true);
 
@@ -2785,7 +2797,7 @@ void DerivationGoal::handleChildOutput(int fd, const string & data)
             writeFull(fdLogFile, data);
     }
 
-    if (hook && fd == hook->fromHook.readSide)
+    if (hook && fd == hook->fromAgent.readSide)
         writeToStderr(prefix + data);
 }
 
-- 
2.28.0





^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [bug#43340] [PATCH 2/5] daemon: Isolate signing and signature verification functions.
  2020-09-11 14:51 ` [bug#43340] [PATCH 1/5] daemon: Generalize 'HookInstance' to 'Agent' Ludovic Courtès
@ 2020-09-11 14:51   ` Ludovic Courtès
  2020-09-11 14:51   ` [bug#43340] [PATCH 3/5] daemon: Move 'Agent' to libutil Ludovic Courtès
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Ludovic Courtès @ 2020-09-11 14:51 UTC (permalink / raw)
  To: 43340; +Cc: Ludovic Courtès

* nix/libstore/local-store.cc (signHash, verifySignature): New
functions.
(LocalStore::exportPath): Use 'signHash' instead of inline code.
(LocalStore::importPath): Use 'verifySignature' instead of inline code.
---
 nix/libstore/local-store.cc | 43 ++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/nix/libstore/local-store.cc b/nix/libstore/local-store.cc
index e6badd3721..cbbd8e901d 100644
--- a/nix/libstore/local-store.cc
+++ b/nix/libstore/local-store.cc
@@ -1238,6 +1238,34 @@ static std::string runAuthenticationProgram(const Strings & args)
     return runProgram(settings.guixProgram, false, fullArgs);
 }
 
+/* Sign HASH with the key stored in file SECRETKEY.  Return the signature as a
+   string, or raise an exception upon error.  */
+static std::string signHash(const string &secretKey, const Hash &hash)
+{
+    Strings args;
+    args.push_back("sign");
+    args.push_back(secretKey);
+    args.push_back(printHash(hash));
+
+    return runAuthenticationProgram(args);
+}
+
+/* Verify SIGNATURE and return the base16-encoded hash over which it was
+   computed.  */
+static std::string verifySignature(const string &signature)
+{
+    Path tmpDir = createTempDir("", "guix", true, true, 0700);
+    AutoDelete delTmp(tmpDir);
+
+    Path sigFile = tmpDir + "/sig";
+    writeFile(sigFile, signature);
+
+    Strings args;
+    args.push_back("verify");
+    args.push_back(sigFile);
+    return runAuthenticationProgram(args);
+}
+
 void LocalStore::exportPath(const Path & path, bool sign,
     Sink & sink)
 {
@@ -1280,12 +1308,7 @@ void LocalStore::exportPath(const Path & path, bool sign,
         Path secretKey = settings.nixConfDir + "/signing-key.sec";
         checkSecrecy(secretKey);
 
-        Strings args;
-        args.push_back("sign");
-        args.push_back(secretKey);
-        args.push_back(printHash(hash));
-
-        string signature = runAuthenticationProgram(args);
+	string signature = signHash(secretKey, hash);
 
         writeString(signature, hashAndWriteSink);
 
@@ -1364,13 +1387,7 @@ Path LocalStore::importPath(bool requireSignature, Source & source)
         string signature = readString(hashAndReadSource);
 
         if (requireSignature) {
-            Path sigFile = tmpDir + "/sig";
-            writeFile(sigFile, signature);
-
-            Strings args;
-            args.push_back("verify");
-            args.push_back(sigFile);
-            string hash2 = runAuthenticationProgram(args);
+	    string hash2 = verifySignature(signature);
 
             /* Note: runProgram() throws an exception if the signature
                is invalid. */
-- 
2.28.0





^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [bug#43340] [PATCH 3/5] daemon: Move 'Agent' to libutil.
  2020-09-11 14:51 ` [bug#43340] [PATCH 1/5] daemon: Generalize 'HookInstance' to 'Agent' Ludovic Courtès
  2020-09-11 14:51   ` [bug#43340] [PATCH 2/5] daemon: Isolate signing and signature verification functions Ludovic Courtès
@ 2020-09-11 14:51   ` Ludovic Courtès
  2020-09-12  7:21     ` Mathieu Othacehe
  2020-09-11 14:51   ` [bug#43340] [PATCH 4/5] daemon: Spawn 'guix authenticate' once for all Ludovic Courtès
  2020-09-11 14:51   ` [bug#43340] [PATCH 5/5] authenticate: Cache the ACL and key pairs Ludovic Courtès
  3 siblings, 1 reply; 12+ messages in thread
From: Ludovic Courtès @ 2020-09-11 14:51 UTC (permalink / raw)
  To: 43340; +Cc: Ludovic Courtès

* nix/libstore/build.cc (DerivationGoal::tryBuildHook): Add "offload" to
'args' and pass settings.guixProgram as the first argument to
Agent::Agent.
(pathNullDevice, commonChildInit, Agent, Agent::Agent)
(Agent::~Agent): Move to...
* nix/libutil/util.cc: ... here.
* nix/libutil/util.hh (struct Agent, commonChildInit): New
declarations.
---
 nix/libstore/build.cc | 118 +-----------------------------------------
 nix/libutil/util.cc   |  84 ++++++++++++++++++++++++++++++
 nix/libutil/util.hh   |  25 +++++++++
 3 files changed, 111 insertions(+), 116 deletions(-)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index b6fad493a9..73532dd24f 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -80,9 +80,6 @@ namespace nix {
 using std::map;
 
 
-static string pathNullDevice = "/dev/null";
-
-
 /* Forward definition. */
 class Worker;
 struct Agent;
@@ -397,33 +394,6 @@ void Goal::trace(const format & f)
 //////////////////////////////////////////////////////////////////////
 
 
-/* Common initialisation performed in child processes. */
-static void commonChildInit(Pipe & logPipe)
-{
-    /* Put the child in a separate session (and thus a separate
-       process group) so that it has no controlling terminal (meaning
-       that e.g. ssh cannot open /dev/tty) and it doesn't receive
-       terminal signals. */
-    if (setsid() == -1)
-        throw SysError(format("creating a new session"));
-
-    /* Dup the write side of the logger pipe into stderr. */
-    if (dup2(logPipe.writeSide, STDERR_FILENO) == -1)
-        throw SysError("cannot pipe standard error into log file");
-
-    /* Dup stderr to stdout. */
-    if (dup2(STDERR_FILENO, STDOUT_FILENO) == -1)
-        throw SysError("cannot dup stderr into stdout");
-
-    /* Reroute stdin to /dev/null. */
-    int fdDevNull = open(pathNullDevice.c_str(), O_RDWR);
-    if (fdDevNull == -1)
-        throw SysError(format("cannot open `%1%'") % pathNullDevice);
-    if (dup2(fdDevNull, STDIN_FILENO) == -1)
-        throw SysError("cannot dup null device into stdin");
-    close(fdDevNull);
-}
-
 /* Restore default handling of SIGPIPE, otherwise some programs will
    randomly say "Broken pipe". */
 static void restoreSIGPIPE()
@@ -586,91 +556,6 @@ void UserLock::kill()
     killUser(uid);
 }
 
-
-//////////////////////////////////////////////////////////////////////
-
-
-/* An "agent" is a helper program that runs in the background and that we talk
-   to over pipes, such as the "guix offload" program.  */
-struct Agent
-{
-    /* Pipes for talking to the agent. */
-    Pipe toAgent;
-
-    /* Pipe for the agent's standard output/error. */
-    Pipe fromAgent;
-
-    /* Pipe for build standard output/error--e.g., for build processes started
-       by "guix offload".  */
-    Pipe builderOut;
-
-    /* The process ID of the agent. */
-    Pid pid;
-
-    /* The 'guix' sub-command and arguments passed to the agent.  */
-    Agent(const string &command, const Strings &args);
-
-    ~Agent();
-};
-
-
-Agent::Agent(const string &command, const Strings &args)
-{
-    debug(format("starting agent '%1%'") % command);
-
-    const Path &buildHook = settings.guixProgram;
-
-    /* Create a pipe to get the output of the child. */
-    fromAgent.create();
-
-    /* Create the communication pipes. */
-    toAgent.create();
-
-    /* Create a pipe to get the output of the builder. */
-    builderOut.create();
-
-    /* Fork the hook. */
-    pid = startProcess([&]() {
-
-        commonChildInit(fromAgent);
-
-        if (chdir("/") == -1) throw SysError("changing into `/");
-
-        /* Dup the communication pipes. */
-        if (dup2(toAgent.readSide, 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)
-            throw SysError("dupping builder's stdout/stderr");
-
-	Strings allArgs;
-	allArgs.push_back(buildHook);
-	allArgs.push_back(command);
-	allArgs.insert(allArgs.end(), args.begin(), args.end()); // append
-
-        execv(buildHook.c_str(), stringsToCharPtrs(allArgs).data());
-
-        throw SysError(format("executing `%1% %2%'") % buildHook % command);
-    });
-
-    pid.setSeparatePG(true);
-    fromAgent.writeSide.close();
-    toAgent.readSide.close();
-}
-
-
-Agent::~Agent()
-{
-    try {
-        toAgent.writeSide.close();
-        pid.kill(true);
-    } catch (...) {
-        ignoreException();
-    }
-}
-
-
 //////////////////////////////////////////////////////////////////////
 
 
@@ -1593,13 +1478,14 @@ HookReply DerivationGoal::tryBuildHook()
 
     if (!worker.hook) {
 	Strings args = {
+	    "offload",
 	    settings.thisSystem.c_str(),
             (format("%1%") % settings.maxSilentTime).str().c_str(),
             (format("%1%") % settings.printBuildTrace).str().c_str(),
             (format("%1%") % settings.buildTimeout).str().c_str()
 	};
 
-        worker.hook = std::shared_ptr<Agent>(new Agent("offload", args));
+        worker.hook = std::shared_ptr<Agent>(new Agent(settings.guixProgram, args));
     }
 
     /* Tell the hook about system features (beyond the system type)
diff --git a/nix/libutil/util.cc b/nix/libutil/util.cc
index 17d145b4c6..59a2981359 100644
--- a/nix/libutil/util.cc
+++ b/nix/libutil/util.cc
@@ -1142,5 +1142,89 @@ void ignoreException()
     }
 }
 
+static const string pathNullDevice = "/dev/null";
+
+/* Common initialisation performed in child processes. */
+void commonChildInit(Pipe & logPipe)
+{
+    /* Put the child in a separate session (and thus a separate
+       process group) so that it has no controlling terminal (meaning
+       that e.g. ssh cannot open /dev/tty) and it doesn't receive
+       terminal signals. */
+    if (setsid() == -1)
+        throw SysError(format("creating a new session"));
+
+    /* Dup the write side of the logger pipe into stderr. */
+    if (dup2(logPipe.writeSide, STDERR_FILENO) == -1)
+        throw SysError("cannot pipe standard error into log file");
+
+    /* Dup stderr to stdout. */
+    if (dup2(STDERR_FILENO, STDOUT_FILENO) == -1)
+        throw SysError("cannot dup stderr into stdout");
+
+    /* Reroute stdin to /dev/null. */
+    int fdDevNull = open(pathNullDevice.c_str(), O_RDWR);
+    if (fdDevNull == -1)
+        throw SysError(format("cannot open `%1%'") % pathNullDevice);
+    if (dup2(fdDevNull, STDIN_FILENO) == -1)
+        throw SysError("cannot dup null device into stdin");
+    close(fdDevNull);
+}
+
+//////////////////////////////////////////////////////////////////////
+
+Agent::Agent(const string &command, const Strings &args)
+{
+    debug(format("starting agent '%1%'") % command);
+
+    /* Create a pipe to get the output of the child. */
+    fromAgent.create();
+
+    /* Create the communication pipes. */
+    toAgent.create();
+
+    /* Create a pipe to get the output of the builder. */
+    builderOut.create();
+
+    /* Fork the hook. */
+    pid = startProcess([&]() {
+
+        commonChildInit(fromAgent);
+
+        if (chdir("/") == -1) throw SysError("changing into `/");
+
+        /* Dup the communication pipes. */
+        if (dup2(toAgent.readSide, 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)
+            throw SysError("dupping builder's stdout/stderr");
+
+	Strings allArgs;
+	allArgs.push_back(command);
+	allArgs.insert(allArgs.end(), args.begin(), args.end()); // append
+
+        execv(command.c_str(), stringsToCharPtrs(allArgs).data());
+
+        throw SysError(format("executing `%1%'") % command);
+    });
+
+    pid.setSeparatePG(true);
+    fromAgent.writeSide.close();
+    toAgent.readSide.close();
+}
+
+
+Agent::~Agent()
+{
+    try {
+        toAgent.writeSide.close();
+        pid.kill(true);
+    } catch (...) {
+        ignoreException();
+    }
+}
+
 
 }
diff --git a/nix/libutil/util.hh b/nix/libutil/util.hh
index 9e3c14bdd4..13cff44316 100644
--- a/nix/libutil/util.hh
+++ b/nix/libutil/util.hh
@@ -264,6 +264,29 @@ public:
     void setKillSignal(int signal);
 };
 
+/* An "agent" is a helper program that runs in the background and that we talk
+   to over pipes, such as the "guix offload" program.  */
+struct Agent
+{
+    /* Pipes for talking to the agent. */
+    Pipe toAgent;
+
+    /* Pipe for the agent's standard output/error. */
+    Pipe fromAgent;
+
+    /* Pipe for build standard output/error--e.g., for build processes started
+       by "guix offload".  */
+    Pipe builderOut;
+
+    /* The process ID of the agent. */
+    Pid pid;
+
+    /* The command and arguments passed to the agent.  */
+    Agent(const string &command, const Strings &args);
+
+    ~Agent();
+};
+
 
 /* Kill all processes running under the specified uid by sending them
    a SIGKILL. */
@@ -295,6 +318,8 @@ void closeMostFDs(const set<int> & exceptions);
 /* Set the close-on-exec flag for the given file descriptor. */
 void closeOnExec(int fd);
 
+/* Common initialisation performed in child processes. */
+void commonChildInit(Pipe & logPipe);
 
 /* User interruption. */
 
-- 
2.28.0





^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [bug#43340] [PATCH 4/5] daemon: Spawn 'guix authenticate' once for all.
  2020-09-11 14:51 ` [bug#43340] [PATCH 1/5] daemon: Generalize 'HookInstance' to 'Agent' Ludovic Courtès
  2020-09-11 14:51   ` [bug#43340] [PATCH 2/5] daemon: Isolate signing and signature verification functions Ludovic Courtès
  2020-09-11 14:51   ` [bug#43340] [PATCH 3/5] daemon: Move 'Agent' to libutil Ludovic Courtès
@ 2020-09-11 14:51   ` Ludovic Courtès
  2020-09-12  7:20     ` Mathieu Othacehe
  2020-09-11 14:51   ` [bug#43340] [PATCH 5/5] authenticate: Cache the ACL and key pairs Ludovic Courtès
  3 siblings, 1 reply; 12+ messages in thread
From: Ludovic Courtès @ 2020-09-11 14:51 UTC (permalink / raw)
  To: 43340; +Cc: Ludovic Courtès

Previously, we'd spawn 'guix authenticate' once for each item that has
to be signed (when exporting) or authenticated (when importing).  Now,
we spawn it once for all and then follow a request/reply protocol.  This
reduces the wall-clock time of:

  guix archive --export -r $(guix build coreutils -d)

from 30s to 2s.

* guix/scripts/authenticate.scm (sign-with-key): Return the signature
instead of displaying it.  Raise a &formatted-message instead of calling
'leave'.
(validate-signature): Likewise.
(read-command): New procedure.
(guix-authenticate)[send-reply]: New procedure.
Change to read commands from current-input-port.
* nix/libstore/local-store.cc (runAuthenticationProgram): Remove.
(authenticationAgent, readInteger, readAuthenticateReply): New
functions.
(signHash, verifySignature): Rewrite in terms of the agent.
* tests/store.scm ("import not signed"): Remove 'pk' call.
("import signed by unauthorized key"): Check the error message of C.
* tests/guix-authenticate.sh: Rewrite using the new protocol.
---
 guix/scripts/authenticate.scm | 119 ++++++++++++++++++++++++++--------
 nix/libstore/local-store.cc   |  86 +++++++++++++++++++-----
 tests/guix-authenticate.sh    |  45 +++++++------
 tests/store.scm               |   8 +--
 4 files changed, 190 insertions(+), 68 deletions(-)

diff --git a/guix/scripts/authenticate.scm b/guix/scripts/authenticate.scm
index fceac13a84..34737481d5 100644
--- a/guix/scripts/authenticate.scm
+++ b/guix/scripts/authenticate.scm
@@ -21,6 +21,10 @@
   #:use-module (gcrypt pk-crypto)
   #:use-module (guix pki)
   #:use-module (guix ui)
+  #:use-module (guix diagnostics)
+  #:use-module (srfi srfi-34)
+  #:use-module (srfi srfi-35)
+  #:use-module (rnrs bytevectors)
   #:use-module (ice-9 binary-ports)
   #:use-module (ice-9 rdelim)
   #:use-module (ice-9 match)
@@ -39,41 +43,77 @@
   (compose string->canonical-sexp read-string))
 
 (define (sign-with-key key-file sha256)
-  "Sign the hash SHA256 (a bytevector) with KEY-FILE, and write an sexp that
-includes both the hash and the actual signature."
+  "Sign the hash SHA256 (a bytevector) with KEY-FILE, and return the signature
+as a canonical sexp that includes both the hash and the actual signature."
   (let* ((secret-key (call-with-input-file key-file read-canonical-sexp))
          (public-key (if (string-suffix? ".sec" key-file)
                          (call-with-input-file
                              (string-append (string-drop-right key-file 4)
                                             ".pub")
                            read-canonical-sexp)
-                         (leave
-                          (G_ "cannot find public key for secret key '~a'~%")
-                          key-file)))
+                         (raise
+                          (formatted-message
+                           (G_ "cannot find public key for secret key '~a'~%")
+                           key-file))))
          (data       (bytevector->hash-data sha256
                                             #:key-type (key-type public-key)))
          (signature  (signature-sexp data secret-key public-key)))
-    (display (canonical-sexp->string signature))
-    #t))
+    signature))
 
 (define (validate-signature signature)
   "Validate SIGNATURE, a canonical sexp.  Check whether its public key is
-authorized, verify the signature, and print the signed data to stdout upon
-success."
+authorized, verify the signature, and return the signed data (a bytevector)
+upon success."
   (let* ((subject (signature-subject signature))
          (data    (signature-signed-data signature)))
     (if (and data subject)
         (if (authorized-key? subject)
             (if (valid-signature? signature)
-                (let ((hash (hash-data->bytevector data)))
-                  (display (bytevector->base16-string hash))
-                  #t)                              ; success
-                (leave (G_ "error: invalid signature: ~a~%")
-                       (canonical-sexp->string signature)))
-            (leave (G_ "error: unauthorized public key: ~a~%")
-                   (canonical-sexp->string subject)))
-        (leave (G_ "error: corrupt signature data: ~a~%")
-               (canonical-sexp->string signature)))))
+                (hash-data->bytevector data)      ; success
+                (raise
+                 (formatted-message (G_ "invalid signature: ~a")
+                                    (canonical-sexp->string signature))))
+            (raise
+             (formatted-message (G_ "unauthorized public key: ~a")
+                                (canonical-sexp->string subject))))
+        (raise
+         (formatted-message (G_ "corrupt signature data: ~a")
+                            (canonical-sexp->string signature))))))
+
+(define (read-command port)
+  "Read a command from PORT and return the command and arguments as a list of
+strings.  Return the empty list when the end-of-file is reached.
+
+Commands are newline-terminated and must look something like this:
+
+  COMMAND 3:abc 5:abcde 1:x
+
+where COMMAND is an alphanumeric sequence and the remainder is the command
+arguments.  Each argument is written as its length (in characters), followed
+by colon, followed by the given number of characters."
+  (define (consume-whitespace port)
+    (let ((chr (lookahead-u8 port)))
+      (when (eqv? chr (char->integer #\space))
+        (get-u8 port)
+        (consume-whitespace port))))
+
+  (match (read-delimited " \t\n\r" port)
+    ((? eof-object?)
+     '())
+    (command
+     (let loop ((result (list command)))
+       (consume-whitespace port)
+       (let ((next (lookahead-u8 port)))
+         (cond ((eqv? next (char->integer #\newline))
+                (get-u8 port)
+                (reverse result))
+               ((eof-object? next)
+                (reverse result))
+               (else
+                (let* ((len (string->number (read-delimited ":" port)))
+                       (str (utf8->string
+                             (get-bytevector-n port len))))
+                  (loop (cons str result))))))))))
 
 \f
 ;;;
@@ -81,6 +121,13 @@ success."
 ;;;
 
 (define (guix-authenticate . args)
+  (define (send-reply code str)
+    ;; Send CODE and STR as a reply to our client.
+    (let ((bv (string->utf8 str)))
+      (format #t "~a ~a:" code (bytevector-length bv))
+      (put-bytevector (current-output-port) bv)
+      (force-output (current-output-port))))
+
   ;; Signature sexps written to stdout may contain binary data, so force
   ;; ISO-8859-1 encoding so that things are not mangled.  See
   ;; <http://bugs.gnu.org/17312> for details.
@@ -91,21 +138,37 @@ success."
   (with-fluids ((%default-port-encoding "ISO-8859-1")
                 (%default-port-conversion-strategy 'error))
     (match args
-      (("sign" key-file hash)
-       (sign-with-key key-file (base16-string->bytevector hash)))
-      (("verify" signature-file)
-       (call-with-input-file signature-file
-         (lambda (port)
-           (validate-signature (string->canonical-sexp
-                                (read-string port))))))
-
       (("--help")
        (display (G_ "Usage: guix authenticate OPTION...
 Sign or verify the signature on the given file.  This tool is meant to
 be used internally by 'guix-daemon'.\n")))
       (("--version")
        (show-version-and-exit "guix authenticate"))
-      (else
-       (leave (G_ "wrong arguments"))))))
+      (()
+       (let loop ()
+         (guard (c ((formatted-message? c)
+                    (send-reply 500
+                                (apply format #f
+                                       (G_ (formatted-message-string c))
+                                       (formatted-message-arguments c)))))
+           ;; Read a request on standard input and reply.
+           (match (read-command (current-input-port))
+             (("sign" signing-key (= base16-string->bytevector hash))
+              (let ((signature (sign-with-key signing-key hash)))
+                (send-reply 0 (canonical-sexp->string signature))))
+             (("verify" signature)
+              (send-reply 0
+                          (bytevector->base16-string
+                           (validate-signature
+                            (string->canonical-sexp signature)))))
+             (()
+              (exit 0))
+             (commands
+              (warning (G_ "~s: invalid command; ignoring~%") commands)
+              (send-reply 404 "invalid command"))))
+
+         (loop)))
+      (_
+       (leave (G_ "wrong arguments~%"))))))
 
 ;;; authenticate.scm ends here
diff --git a/nix/libstore/local-store.cc b/nix/libstore/local-store.cc
index cbbd8e901d..9bc7a0bc4f 100644
--- a/nix/libstore/local-store.cc
+++ b/nix/libstore/local-store.cc
@@ -1231,39 +1231,91 @@ static void checkSecrecy(const Path & path)
 }
 
 
-static std::string runAuthenticationProgram(const Strings & args)
+/* Return the authentication agent, a "guix authenticate" process started
+   lazily.  */
+static std::shared_ptr<Agent> authenticationAgent()
 {
-    Strings fullArgs = { "authenticate" };
-    fullArgs.insert(fullArgs.end(), args.begin(), args.end()); // append
-    return runProgram(settings.guixProgram, false, fullArgs);
+    static std::shared_ptr<Agent> agent;
+
+    if (!agent) {
+	Strings args = { "authenticate" };
+	agent = std::shared_ptr<Agent>(new Agent(settings.guixProgram, args));
+    }
+
+    return agent;
+}
+
+/* Read an integer and the byte that immediately follows it from FD.  Return
+   the integer.  */
+static int readInteger(int fd)
+{
+    string str;
+
+    while (1) {
+        char ch;
+        ssize_t rd = read(fd, &ch, 1);
+        if (rd == -1) {
+            if (errno != EINTR)
+                throw SysError("reading an integer");
+        } else if (rd == 0)
+            throw EndOfFile("unexpected EOF reading an integer");
+        else {
+	    if (strchr("0123456789", ch)) {
+		str += ch;
+	    } else {
+		break;
+	    }
+        }
+    }
+
+    return stoi(str);
+}
+
+/* Read from FD a reply coming from 'guix authenticate'.  The reply has the
+   form "CODE LEN:STR".  CODE is an integer, where zero indicates success.
+   LEN specifies the length in bytes of the string that immediately
+   follows.  */
+static std::string readAuthenticateReply(int fd)
+{
+    int code = readInteger(fd);
+    int len = readInteger(fd);
+
+    string str;
+    str.resize(len);
+    readFull(fd, (unsigned char *) &str[0], len);
+
+    if (code == 0)
+	return str;
+    else
+	throw Error(str);
 }
 
 /* Sign HASH with the key stored in file SECRETKEY.  Return the signature as a
    string, or raise an exception upon error.  */
 static std::string signHash(const string &secretKey, const Hash &hash)
 {
-    Strings args;
-    args.push_back("sign");
-    args.push_back(secretKey);
-    args.push_back(printHash(hash));
+    auto agent = authenticationAgent();
+    auto hexHash = printHash(hash);
 
-    return runAuthenticationProgram(args);
+    writeLine(agent->toAgent.writeSide,
+	      (format("sign %1%:%2% %3%:%4%")
+	       % secretKey.size() % secretKey
+	       % hexHash.size() % hexHash).str());
+
+    return readAuthenticateReply(agent->fromAgent.readSide);
 }
 
 /* Verify SIGNATURE and return the base16-encoded hash over which it was
    computed.  */
 static std::string verifySignature(const string &signature)
 {
-    Path tmpDir = createTempDir("", "guix", true, true, 0700);
-    AutoDelete delTmp(tmpDir);
+    auto agent = authenticationAgent();
 
-    Path sigFile = tmpDir + "/sig";
-    writeFile(sigFile, signature);
+    writeLine(agent->toAgent.writeSide,
+	      (format("verify %1%:%2%")
+	       % signature.size() % signature).str());
 
-    Strings args;
-    args.push_back("verify");
-    args.push_back(sigFile);
-    return runAuthenticationProgram(args);
+    return readAuthenticateReply(agent->fromAgent.readSide);
 }
 
 void LocalStore::exportPath(const Path & path, bool sign,
diff --git a/tests/guix-authenticate.sh b/tests/guix-authenticate.sh
index 773443453d..f3b36ee41d 100644
--- a/tests/guix-authenticate.sh
+++ b/tests/guix-authenticate.sh
@@ -28,33 +28,38 @@ rm -f "$sig" "$hash"
 
 trap 'rm -f "$sig" "$hash"' EXIT
 
+key="$abs_top_srcdir/tests/signing-key.sec"
+key_len="`echo -n $key | wc -c`"
+
 # A hexadecimal string as long as a sha256 hash.
 hash="2749f0ea9f26c6c7be746a9cff8fa4c2f2a02b000070dba78429e9a11f87c6eb"
+hash_len="`echo -n $hash | wc -c`"
 
-guix authenticate sign				\
-    "$abs_top_srcdir/tests/signing-key.sec"	\
-    "$hash" > "$sig"
+echo "sign $key_len:$key $hash_len:$hash" | guix authenticate > "$sig"
 test -f "$sig"
+case "$(cat $sig)" in
+    "0 "*) ;;
+    *)     echo "broken signature: $(cat $sig)"
+	   exit 42;;
+esac
 
-hash2="`guix authenticate verify "$sig"`"
-test "$hash2" = "$hash"
+# Remove the leading "0".
+sed -i "$sig" -e's/^0 //g'
+
+hash2="$(echo verify $(cat "$sig") | guix authenticate)"
+test "$(echo $hash2 | cut -d : -f 2)" = "$hash"
 
 # Detect corrupt signatures.
-if guix authenticate verify /dev/null
-then false
-else true
-fi
+code="$(echo "verify 5:wrong" | guix authenticate | cut -f1 -d ' ')"
+test "$code" -ne 0
 
 # Detect invalid signatures.
 # The signature has (payload (data ... (hash sha256 #...#))).  We proceed by
 # modifying this hash.
 sed -i "$sig"											\
     -e's|#[A-Z0-9]\{64\}#|#0000000000000000000000000000000000000000000000000000000000000000#|g'
-if guix authenticate verify "$sig"
-then false
-else true
-fi
-
+code="$(echo "verify $(cat $sig)" | guix authenticate | cut -f1 -d ' ')"
+test "$code" -ne 0
 
 # Test for <http://bugs.gnu.org/17312>: make sure 'guix authenticate' produces
 # valid signatures when run in the C locale.
@@ -63,9 +68,11 @@ hash="5eff0b55c9c5f5e87b4e34cd60a2d5654ca1eb78c7b3c67c3179fed1cff07b4c"
 LC_ALL=C
 export LC_ALL
 
-guix authenticate sign "$abs_top_srcdir/tests/signing-key.sec" "$hash" \
-     > "$sig"
+echo "sign $key_len:$key $hash_len:$hash" | guix authenticate > "$sig"
 
-guix authenticate verify "$sig"
-hash2="`guix authenticate verify "$sig"`"
-test "$hash2" = "$hash"
+# Remove the leading "0".
+sed -i "$sig" -e's/^0 //g'
+
+echo "verify $(cat $sig)" | guix authenticate
+hash2="$(echo "verify $(cat $sig)" | guix authenticate | cut -f2 -d ' ')"
+test "$(echo $hash2 | cut -d : -f 2)" = "$hash"
diff --git a/tests/store.scm b/tests/store.scm
index 8ff76e8f98..3a2a21a250 100644
--- a/tests/store.scm
+++ b/tests/store.scm
@@ -990,7 +990,7 @@
 
     ;; Ensure 'import-paths' raises an exception.
     (guard (c ((store-protocol-error? c)
-               (and (not (zero? (store-protocol-error-status (pk 'C c))))
+               (and (not (zero? (store-protocol-error-status c)))
                     (string-contains (store-protocol-error-message c)
                                      "lacks a signature"))))
       (let* ((source   (open-bytevector-input-port dump))
@@ -1030,9 +1030,9 @@
 
     ;; Ensure 'import-paths' raises an exception.
     (guard (c ((store-protocol-error? c)
-               ;; XXX: The daemon-provided error message currently doesn't
-               ;; mention the reason of the failure.
-               (not (zero? (store-protocol-error-status c)))))
+               (and (not (zero? (store-protocol-error-status c)))
+                    (string-contains (store-protocol-error-message c)
+                                     "unauthorized public key"))))
       (let* ((source   (open-bytevector-input-port dump))
              (imported (import-paths %store source)))
         (pk 'unauthorized-imported imported)
-- 
2.28.0





^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [bug#43340] [PATCH 5/5] authenticate: Cache the ACL and key pairs.
  2020-09-11 14:51 ` [bug#43340] [PATCH 1/5] daemon: Generalize 'HookInstance' to 'Agent' Ludovic Courtès
                     ` (2 preceding siblings ...)
  2020-09-11 14:51   ` [bug#43340] [PATCH 4/5] daemon: Spawn 'guix authenticate' once for all Ludovic Courtès
@ 2020-09-11 14:51   ` Ludovic Courtès
  3 siblings, 0 replies; 12+ messages in thread
From: Ludovic Courtès @ 2020-09-11 14:51 UTC (permalink / raw)
  To: 43340; +Cc: Ludovic Courtès

In practice we're always using the same key pair,
/etc/guix/signing-key.{pub,sec}.  Keeping them in cache allows us to
avoid redundant I/O and parsing when signing multiple store items in a
row.

* guix/scripts/authenticate.scm (load-key-pair): New procedure.
(sign-with-key): Remove 'key-file' parameter and add 'public-key' and
'secret-key'.  Adjust accordingly.
(validate-signature): Add 'acl' parameter and pass it to
'authorized-key?'.
(guix-authenticate): Call 'current-acl' upfront and cache its result.
Add 'key-pairs' as an argument to 'loop' and use it as a cache of key
pairs.
---
 guix/scripts/authenticate.scm | 108 +++++++++++++++++++++-------------
 1 file changed, 66 insertions(+), 42 deletions(-)

diff --git a/guix/scripts/authenticate.scm b/guix/scripts/authenticate.scm
index 34737481d5..95005641c4 100644
--- a/guix/scripts/authenticate.scm
+++ b/guix/scripts/authenticate.scm
@@ -24,10 +24,12 @@
   #:use-module (guix diagnostics)
   #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-35)
+  #:use-module (srfi srfi-71)
   #:use-module (rnrs bytevectors)
   #:use-module (ice-9 binary-ports)
   #:use-module (ice-9 rdelim)
   #:use-module (ice-9 match)
+  #:use-module (ice-9 vlist)
   #:export (guix-authenticate))
 
 ;;; Commentary:
@@ -42,32 +44,40 @@
   ;; Read a gcrypt sexp from a port and return it.
   (compose string->canonical-sexp read-string))
 
-(define (sign-with-key key-file sha256)
-  "Sign the hash SHA256 (a bytevector) with KEY-FILE, and return the signature
-as a canonical sexp that includes both the hash and the actual signature."
-  (let* ((secret-key (call-with-input-file key-file read-canonical-sexp))
-         (public-key (if (string-suffix? ".sec" key-file)
-                         (call-with-input-file
+(define (load-key-pair key-file)
+  "Load the key pair whose secret key lives at KEY-FILE.  Return a pair of
+canonical sexps representing those keys."
+  (catch 'system-error
+    (lambda ()
+      (let* ((secret-key (call-with-input-file key-file read-canonical-sexp))
+             (public-key (call-with-input-file
                              (string-append (string-drop-right key-file 4)
                                             ".pub")
-                           read-canonical-sexp)
-                         (raise
-                          (formatted-message
-                           (G_ "cannot find public key for secret key '~a'~%")
-                           key-file))))
-         (data       (bytevector->hash-data sha256
-                                            #:key-type (key-type public-key)))
-         (signature  (signature-sexp data secret-key public-key)))
-    signature))
+                           read-canonical-sexp)))
+        (cons public-key secret-key)))
+    (lambda args
+      (let ((errno (system-error-errno args)))
+        (raise
+         (formatted-message
+          (G_ "failed to load key pair at '~a': ~a~%")
+          key-file (strerror errno)))))))
 
-(define (validate-signature signature)
+(define (sign-with-key public-key secret-key sha256)
+  "Sign the hash SHA256 (a bytevector) with SECRET-KEY (a canonical sexp), and
+return the signature as a canonical sexp that includes SHA256, PUBLIC-KEY, and
+the actual signature."
+  (let ((data (bytevector->hash-data sha256
+                                     #:key-type (key-type public-key))))
+    (signature-sexp data secret-key public-key)))
+
+(define (validate-signature signature acl)
   "Validate SIGNATURE, a canonical sexp.  Check whether its public key is
-authorized, verify the signature, and return the signed data (a bytevector)
-upon success."
+authorized in ACL, verify the signature, and return the signed data (a
+bytevector) upon success."
   (let* ((subject (signature-subject signature))
          (data    (signature-signed-data signature)))
     (if (and data subject)
-        (if (authorized-key? subject)
+        (if (authorized-key? subject acl)
             (if (valid-signature? signature)
                 (hash-data->bytevector data)      ; success
                 (raise
@@ -145,29 +155,43 @@ be used internally by 'guix-daemon'.\n")))
       (("--version")
        (show-version-and-exit "guix authenticate"))
       (()
-       (let loop ()
-         (guard (c ((formatted-message? c)
-                    (send-reply 500
-                                (apply format #f
-                                       (G_ (formatted-message-string c))
-                                       (formatted-message-arguments c)))))
-           ;; Read a request on standard input and reply.
-           (match (read-command (current-input-port))
-             (("sign" signing-key (= base16-string->bytevector hash))
-              (let ((signature (sign-with-key signing-key hash)))
-                (send-reply 0 (canonical-sexp->string signature))))
-             (("verify" signature)
-              (send-reply 0
-                          (bytevector->base16-string
-                           (validate-signature
-                            (string->canonical-sexp signature)))))
-             (()
-              (exit 0))
-             (commands
-              (warning (G_ "~s: invalid command; ignoring~%") commands)
-              (send-reply 404 "invalid command"))))
-
-         (loop)))
+       (let ((acl (current-acl)))
+         (let loop ((key-pairs vlist-null))
+           (guard (c ((formatted-message? c)
+                      (send-reply 500
+                                  (apply format #f
+                                         (G_ (formatted-message-string c))
+                                         (formatted-message-arguments c)))))
+             ;; Read a request on standard input and reply.
+             (match (read-command (current-input-port))
+               (("sign" signing-key (= base16-string->bytevector hash))
+                (let* ((key-pairs keys
+                                  (match (vhash-assoc signing-key key-pairs)
+                                    ((_ . keys)
+                                     (values key-pairs keys))
+                                    (#f
+                                     (let ((keys (load-key-pair signing-key)))
+                                       (values (vhash-cons signing-key keys
+                                                           key-pairs)
+                                               keys)))))
+                       (signature (match keys
+                                    ((public . secret)
+                                     (sign-with-key public secret hash)))))
+                  (send-reply 0 (canonical-sexp->string signature))
+                  (loop key-pairs)))
+               (("verify" signature)
+                (send-reply 0
+                            (bytevector->base16-string
+                             (validate-signature
+                              (string->canonical-sexp signature)
+                              acl)))
+                (loop key-pairs))
+               (()
+                (exit 0))
+               (commands
+                (warning (G_ "~s: invalid command; ignoring~%") commands)
+                (send-reply 404 "invalid command")
+                (loop key-pairs)))))))
       (_
        (leave (G_ "wrong arguments~%"))))))
 
-- 
2.28.0





^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [bug#43340] [PATCH 0/5] Speed up archive export/import
  2020-09-11 14:40 [bug#43340] [PATCH 0/5] Speed up archive export/import Ludovic Courtès
  2020-09-11 14:51 ` [bug#43340] [PATCH 1/5] daemon: Generalize 'HookInstance' to 'Agent' Ludovic Courtès
@ 2020-09-11 15:01 ` Ludovic Courtès
  2020-09-12  7:12   ` Mathieu Othacehe
  1 sibling, 1 reply; 12+ messages in thread
From: Ludovic Courtès @ 2020-09-11 15:01 UTC (permalink / raw)
  To: 43340

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

Ludovic Courtès <ludo@gnu.org> skribis:

> This patch series goes on top of <https://issues.guix.gnu.org/43285>.
> It addresses the performance issue described at:
>
>   https://lists.gnu.org/archive/html/guix-devel/2020-09/msg00073.html
>
> Specifically, it implements option #4 (spawning ‘guix authenticate’
> once for the whole session, instead of spawning it every time a
> store item needs to be signed or authenticated), achieving a ~15x
> speedup, which is not bad.  :-)

Below is the new Gantt chart for:

  perf timechart record guix archive --export -r $(guix build coreutils -d) -v3 >/tmp/dump

Most of the work happens in ‘guix authenticate’.


[-- Attachment #2: new timechart --]
[-- Type: image/png, Size: 42448 bytes --]

[-- Attachment #3: Type: text/plain, Size: 12 bytes --]


Ludo’.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [bug#43340] [PATCH 0/5] Speed up archive export/import
  2020-09-11 15:01 ` [bug#43340] [PATCH 0/5] Speed up archive export/import Ludovic Courtès
@ 2020-09-12  7:12   ` Mathieu Othacehe
  2020-09-13 13:07     ` Ludovic Courtès
  2020-09-14 13:47     ` bug#43340: " Ludovic Courtès
  0 siblings, 2 replies; 12+ messages in thread
From: Mathieu Othacehe @ 2020-09-12  7:12 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43340


Hey Ludo,

>> Specifically, it implements option #4 (spawning ‘guix authenticate’
>> once for the whole session, instead of spawning it every time a
>> store item needs to be signed or authenticated), achieving a ~15x
>> speedup, which is not bad.  :-)

Woo, congrats!

> Below is the new Gantt chart for:
>
>   perf timechart record guix archive --export -r $(guix build coreutils -d) -v3 >/tmp/dump
>
> Most of the work happens in ‘guix authenticate’.

I never used the "timechart" sub-command but it sounds really
nice. Regarding the option you chose, I think it's the more appropriate
right now. It's very delicate to dedicate time and effort to tweak
guix-daemon and how we use it, having in mind that we'd like to get rid
of it.

However, the potential short term gains can be so huge, that for now
it's the best thing to do. I should just do like you, and dive into it
to see what can be done for contention and locking when using
'build-paths' RPC via Cuirass.

In the meantime, a short review that I hope to complete next week.

Thanks for your efforts,

Mathieu




^ permalink raw reply	[flat|nested] 12+ messages in thread

* [bug#43340] [PATCH 4/5] daemon: Spawn 'guix authenticate' once for all.
  2020-09-11 14:51   ` [bug#43340] [PATCH 4/5] daemon: Spawn 'guix authenticate' once for all Ludovic Courtès
@ 2020-09-12  7:20     ` Mathieu Othacehe
  0 siblings, 0 replies; 12+ messages in thread
From: Mathieu Othacehe @ 2020-09-12  7:20 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43340


> +                    (send-reply 500

Reply codes could be factorized in an enum.

> +    if (!agent) {
> +	Strings args = { "authenticate" };
> +	agent = std::shared_ptr<Agent>(new Agent(settings.guixProgram, args));
> +    }

make_shared should be preferred to the direct use of new.

> +	    if (strchr("0123456789", ch)) {

You can maybe use isdigit?

Thanks,

Mathieu




^ permalink raw reply	[flat|nested] 12+ messages in thread

* [bug#43340] [PATCH 3/5] daemon: Move 'Agent' to libutil.
  2020-09-11 14:51   ` [bug#43340] [PATCH 3/5] daemon: Move 'Agent' to libutil Ludovic Courtès
@ 2020-09-12  7:21     ` Mathieu Othacehe
  0 siblings, 0 replies; 12+ messages in thread
From: Mathieu Othacehe @ 2020-09-12  7:21 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43340


This one and the two previous ones look good to me.

Thanks,

Mathieu




^ permalink raw reply	[flat|nested] 12+ messages in thread

* [bug#43340] [PATCH 0/5] Speed up archive export/import
  2020-09-12  7:12   ` Mathieu Othacehe
@ 2020-09-13 13:07     ` Ludovic Courtès
  2020-09-14 13:47     ` bug#43340: " Ludovic Courtès
  1 sibling, 0 replies; 12+ messages in thread
From: Ludovic Courtès @ 2020-09-13 13:07 UTC (permalink / raw)
  To: Mathieu Othacehe; +Cc: 43340

Hi!

Mathieu Othacehe <othacehe@gnu.org> skribis:

>>> Specifically, it implements option #4 (spawning ‘guix authenticate’
>>> once for the whole session, instead of spawning it every time a
>>> store item needs to be signed or authenticated), achieving a ~15x
>>> speedup, which is not bad.  :-)
>
> Woo, congrats!

To be clear, it’s 15x on the pathological case where we send only small
store items; the difference is obviously less significant if few store
items are exported or if the store items are large.  Now, I expect it to
make a noticeable difference on the typical build farm workload.

>> Below is the new Gantt chart for:
>>
>>   perf timechart record guix archive --export -r $(guix build coreutils -d) -v3 >/tmp/dump
>>
>> Most of the work happens in ‘guix authenticate’.
>
> I never used the "timechart" sub-command but it sounds really
> nice. Regarding the option you chose, I think it's the more appropriate
> right now. It's very delicate to dedicate time and effort to tweak
> guix-daemon and how we use it, having in mind that we'd like to get rid
> of it.

Yeah.

> However, the potential short term gains can be so huge, that for now
> it's the best thing to do. I should just do like you, and dive into it
> to see what can be done for contention and locking when using
> 'build-paths' RPC via Cuirass.

Yup, makes sense!

> In the meantime, a short review that I hope to complete next week.

Thanks for your feedback!

Ludo’.




^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#43340: [PATCH 0/5] Speed up archive export/import
  2020-09-12  7:12   ` Mathieu Othacehe
  2020-09-13 13:07     ` Ludovic Courtès
@ 2020-09-14 13:47     ` Ludovic Courtès
  1 sibling, 0 replies; 12+ messages in thread
From: Ludovic Courtès @ 2020-09-14 13:47 UTC (permalink / raw)
  To: Mathieu Othacehe; +Cc: 43340-done

Hello!

I went ahead and pushed this series as
7d516c17da50dfc8ce635a21c37533d1fe27b43b.

Changes compared to what I posted:

  • Use ‘make_shared’ and ‘isdigit’ (I’d never get a C++ developer
    position…).

  • Add an enum for reply codes in ‘guix authenticate’.

  • Fix a thinko in the last patch of the series: ensure ‘loop’ is
    called even when an exception is caught.

To people who use offloading: please give it a try by running the daemon
from a recent checkout with:

  sudo -E ./pre-inst-env  guix-daemon --build-users-group=guixbuild

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-09-14 13:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-11 14:40 [bug#43340] [PATCH 0/5] Speed up archive export/import Ludovic Courtès
2020-09-11 14:51 ` [bug#43340] [PATCH 1/5] daemon: Generalize 'HookInstance' to 'Agent' Ludovic Courtès
2020-09-11 14:51   ` [bug#43340] [PATCH 2/5] daemon: Isolate signing and signature verification functions Ludovic Courtès
2020-09-11 14:51   ` [bug#43340] [PATCH 3/5] daemon: Move 'Agent' to libutil Ludovic Courtès
2020-09-12  7:21     ` Mathieu Othacehe
2020-09-11 14:51   ` [bug#43340] [PATCH 4/5] daemon: Spawn 'guix authenticate' once for all Ludovic Courtès
2020-09-12  7:20     ` Mathieu Othacehe
2020-09-11 14:51   ` [bug#43340] [PATCH 5/5] authenticate: Cache the ACL and key pairs Ludovic Courtès
2020-09-11 15:01 ` [bug#43340] [PATCH 0/5] Speed up archive export/import Ludovic Courtès
2020-09-12  7:12   ` Mathieu Othacehe
2020-09-13 13:07     ` Ludovic Courtès
2020-09-14 13:47     ` bug#43340: " Ludovic Courtès

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).