* Simplify communication with the hook a bit (don't use file

  descriptors 3/4, just use stdin/stderr).

6 files changed, 68 insertions, 117 deletions

diff --git a/doc/manual/env-common.xml b/doc/manual/env-common.xml
index d6ebbf6547..d67ef714d0 100644
--- a/doc/manual/env-common.xml
+++ b/doc/manual/env-common.xml
@@ -151,12 +151,12 @@ $ mount -o bind /mnt/otherdisk/nix /nix</screen>
   <para>On the basis of this information, and whatever persistent
   state the build hook keeps about other machines and their current
   load, it has to decide what to do with the build.  It should print
-  out on file descriptor 3 one of the following responses (terminated
-  by a newline, <literal>"\n"</literal>):
+  out on standard error one of the following responses (terminated by
+  a newline, <literal>"\n"</literal>):
 
   <variablelist>
 
-    <varlistentry><term><literal>decline</literal></term>
+    <varlistentry><term><literal># decline</literal></term>
 
       <listitem><para>The build hook is not willing or able to perform
       the build; the calling Nix process should do the build itself,
@@ -164,7 +164,7 @@ $ mount -o bind /mnt/otherdisk/nix /nix</screen>
 
     </varlistentry>
 
-    <varlistentry><term><literal>postpone</literal></term>
+    <varlistentry><term><literal># postpone</literal></term>
 
       <listitem><para>The build hook cannot perform the build now, but
       can do so in the future (e.g., because all available build slots
@@ -174,7 +174,7 @@ $ mount -o bind /mnt/otherdisk/nix /nix</screen>
 
     </varlistentry>
 
-    <varlistentry><term><literal>accept</literal></term>
+    <varlistentry><term><literal># accept</literal></term>
 
       <listitem><para>The build hook has accepted the
       build.</para></listitem>
@@ -185,37 +185,12 @@ $ mount -o bind /mnt/otherdisk/nix /nix</screen>
 
   </para>
 
-  <para>If the build hook accepts the build, it is possible that it is
-  no longer necessary to do the build because some other process has
-  performed the build in the meantime.  To prevent races, the hook
-  must read from file descriptor 4 a single line that tells it whether
-  to continue:
-
-  <variablelist>
-
-    <varlistentry><term><literal>cancel</literal></term>
-
-      <listitem><para>The build has already been done, so the hook
-      should exit.</para></listitem>
-
-    </varlistentry>
-  
-    <varlistentry><term><literal>okay</literal></term>
-
-      <listitem><para>The hook should proceed with the build.  At this
-      point, the calling Nix process has acquired locks on the output
-      path, so no other Nix process will perform the
-      build.</para></listitem>
-
-    </varlistentry>
-
-  </variablelist>
-
-  </para>
-
-  <para>If the hook has been told to proceed, Nix will store in the
-  hook’s current directory a number of text files that contain
-  information about the derivation:
+  <para>After sending <literal># accept</literal>, the hook should
+  read one line from standard input, which will be the string
+  <literal>okay</literal>.  It can then proceed with the build.
+  Before sending <literal>okay</literal>, Nix will store in the hook’s
+  current directory a number of text files that contain information
+  about the derivation:
 
   <variablelist>
 
@@ -255,7 +230,9 @@ $ mount -o bind /mnt/otherdisk/nix /nix</screen>
   <para>The hook should copy the inputs to the remote machine,
   register the validity of the inputs, perform the remote build, and
   copy the outputs back to the local machine.  An exit code other than
-  <literal>0</literal> indicates that the hook has failed.</para>
+  <literal>0</literal> indicates that the hook has failed.  An exit
+  code equal to 100 means that the remote build failed (as opposed to,
+  e.g., a network error).</para>
 
   </listitem>
 
diff --git a/scripts/build-remote.pl.in b/scripts/build-remote.pl.in
index 1c4f974406..91b57d5f24 100755
--- a/scripts/build-remote.pl.in
+++ b/scripts/build-remote.pl.in
@@ -29,9 +29,7 @@ $maxSilentTime = 0 unless defined $maxSilentTime;
 
 sub sendReply {
     my $reply = shift;
-    open OUT, ">&3" or die;
-    print OUT "$reply\n";
-    close OUT;
+    print STDERR "# $reply\n";
 }
 
 sub decline {
@@ -121,11 +119,8 @@ if (!defined $machine) {
 
 # Yes we did, accept.
 sendReply "accept";
-open IN, "<&4" or die;
-my $x = <IN>;
+my $x = <STDIN>;
 chomp $x;
-#print "got $x\n";  
-close IN;
 
 if ($x ne "okay") {
     exit 0;
diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 92bf1cab0b..12061bee78 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -660,7 +660,6 @@ private:
 
     /* Pipes for talking to the build hook (if any). */
     Pipe toHook;
-    Pipe fromHook;
 
     /* Whether we're currently doing a chroot build. */
     bool useChroot;
@@ -1207,49 +1206,6 @@ void DerivationGoal::buildDone()
 }
 
 
-static string readLine(int fd)
-{
-    string s;
-    while (1) {
-        checkInterrupt();
-        char ch;
-        ssize_t rd = read(fd, &ch, 1);
-        if (rd == -1) {
-            if (errno != EINTR)
-                throw SysError("reading a line");
-        } else if (rd == 0)
-            throw Error("unexpected EOF reading a line");
-        else {
-            if (ch == '\n') return s;
-            s += ch;
-        }
-    }
-}
-
-
-static void writeLine(int fd, string s)
-{
-    s += '\n';
-    writeFull(fd, (const unsigned char *) s.c_str(), s.size());
-}
-
-
-/* !!! ugly hack */
-static void drain(int fd)
-{
-    unsigned char buffer[1024];
-    while (1) {
-        checkInterrupt();
-        ssize_t rd = read(fd, buffer, sizeof buffer);
-        if (rd == -1) {
-            if (errno != EINTR)
-                throw SysError("draining");
-        } else if (rd == 0) break;
-        else writeToStderr(buffer, rd);
-    }
-}
-
-
 DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 {
     if (!useBuildHook) return rpDecline;
@@ -1266,7 +1222,6 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 
     /* Create the communication pipes. */
     toHook.create();
-    fromHook.create();
 
     /* Fork the hook. */
     pid = fork();
@@ -1310,16 +1265,20 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
     worker.childStarted(shared_from_this(),
         pid, singleton<set<int> >(logPipe.readSide), false);
 
-    fromHook.writeSide.close();
     toHook.readSide.close();
 
     /* Read the first line of input, which should be a word indicating
-       whether the hook wishes to perform the build.  !!! potential
-       for deadlock here: we should also read from the child's logger
-       pipe. */
+       whether the hook wishes to perform the build. */
     string reply;
     try {
-        reply = readLine(fromHook.readSide);
+        while (true) {
+            string s = readLine(logPipe.readSide);
+            if (string(s, 0, 2) == "# ") {
+                reply = string(s, 2);
+                break;
+            }
+            handleChildOutput(logPipe.readSide, s + "\n");
+        }
     } catch (Error & e) {
         terminateBuildHook(true);
         throw;
@@ -1335,7 +1294,7 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 
     else if (reply == "accept") {
 
-        printMsg(lvlInfo, format("running hook to build path(s) %1%")
+        printMsg(lvlInfo, format("using hook to build path(s) %1%")
             % showPaths(outputPaths(drv.outputs)));
         
         /* Write the information that the hook needs to perform the
@@ -1377,13 +1336,13 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
         writeStringToFile(referencesFN,
             makeValidityRegistration(allInputs, true, false));
 
-        /* Tell the hook to proceed. */ 
+        /* Tell the hook to proceed. */
         writeLine(toHook.writeSide, "okay");
+        toHook.writeSide.close();
 
-        if (printBuildTrace) {
+        if (printBuildTrace)
             printMsg(lvlError, format("@ build-started %1% %2% %3% %4%")
                 % drvPath % drv.outputs["out"].path % drv.platform % logFile);
-        }
         
         return rpAccept;
     }
@@ -1394,7 +1353,6 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 
 void DerivationGoal::terminateBuildHook(bool kill)
 {
-    /* !!! drain stdout of hook */
     debug("terminating build hook");
     pid_t savedPid = pid;
     if (kill)
@@ -1404,10 +1362,8 @@ void DerivationGoal::terminateBuildHook(bool kill)
     /* `false' means don't wake up waiting goals, since we want to
        keep this build slot ourselves. */
     worker.childTerminated(savedPid, false);
-    fromHook.readSide.close();
     toHook.writeSide.close();
     fdLogFile.close();
-    drain(logPipe.readSide);
     logPipe.readSide.close();
     deleteTmpDir(true); /* get rid of the hook's temporary directory */
 }
@@ -1993,24 +1949,14 @@ void DerivationGoal::initChild()
         throw SysError(format("changing into `%1%'") % tmpDir);
 
     /* When running a hook, dup the communication pipes. */
-    bool inHook = fromHook.writeSide.isOpen();
-    if (inHook) {
-        fromHook.readSide.close();
-        if (dup2(fromHook.writeSide, 3) == -1)
-            throw SysError("dupping from-hook write side");
-
+    if (usingBuildHook) {
         toHook.writeSide.close();
-        if (dup2(toHook.readSide, 4) == -1)
+        if (dup2(toHook.readSide, STDIN_FILENO) == -1)
             throw SysError("dupping to-hook read side");
     }
 
     /* Close all other file descriptors. */
-    set<int> exceptions;
-    if (inHook) {
-        exceptions.insert(3);
-        exceptions.insert(4);
-    }
-    closeMostFDs(exceptions);
+    closeMostFDs(set<int>());
 }
 
 
diff --git a/src/libutil/util.cc b/src/libutil/util.cc
index 02dd53a179..1cb94215ee 100644
--- a/src/libutil/util.cc
+++ b/src/libutil/util.cc
@@ -229,6 +229,33 @@ void writeFile(const Path & path, const string & s)
 }
 
 
+string readLine(int fd)
+{
+    string s;
+    while (1) {
+        checkInterrupt();
+        char ch;
+        ssize_t rd = read(fd, &ch, 1);
+        if (rd == -1) {
+            if (errno != EINTR)
+                throw SysError("reading a line");
+        } else if (rd == 0)
+            throw Error("unexpected EOF reading a line");
+        else {
+            if (ch == '\n') return s;
+            s += ch;
+        }
+    }
+}
+
+
+void writeLine(int fd, string s)
+{
+    s += '\n';
+    writeFull(fd, (const unsigned char *) s.c_str(), s.size());
+}
+
+
 static void _computePathSize(const Path & path,
     unsigned long long & bytes, unsigned long long & blocks)
 {
diff --git a/src/libutil/util.hh b/src/libutil/util.hh
index 33b06ca955..5744e56922 100644
--- a/src/libutil/util.hh
+++ b/src/libutil/util.hh
@@ -60,6 +60,12 @@ string readFile(const Path & path);
 /* Write a string to a file. */
 void writeFile(const Path & path, const string & s);
 
+/* Read a line from a file descriptor. */
+string readLine(int fd);
+
+/* Write a line to a file descriptor. */
+void writeLine(int fd, string s);
+
 /* Compute the sum of the sizes of all files in `path'. */
 void computePathSize(const Path & path,
     unsigned long long & bytes, unsigned long long & blocks);
diff --git a/tests/build-hook.hook.sh b/tests/build-hook.hook.sh
index 45536f8de3..18eba283e1 100755
--- a/tests/build-hook.hook.sh
+++ b/tests/build-hook.hook.sh
@@ -11,11 +11,11 @@ outPath=$(sed 's/Derive(\[("out",\"\([^\"]*\)\".*/\1/' $drv)
 echo "output path is $outPath" >&2
 
 if $(echo $outPath | grep -q input-1); then
-    echo "accept" >&3
-    read x <&4
+    echo "# accept" >&2
+    read x
     echo "got $x"
     mkdir $outPath
     echo "BAR" > $outPath/foo
 else
-    echo "decline" >&3
+    echo "# decline" >&2
 fi