Re: 33/33: daemon: Workaround issues for the Hurd.

[Jan Nieuwenhuizen <janneke@gnu.org>]

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

Ludovic Courtès writes:

>> -#if CHROOT_ENABLED
>> +#if CHROOT_ENABLED || __GNU__
>
> Can we instead change the #define CHROOT_ENABLED such that
> CHROOT_ENABLED is always true when __GNU__?  Also with a comment stating
> that GNU supports chroot(2) without being root.

I tried a couple of things and then remembered a patch by Manolis that
already does something like this; but nicer.  So, I am now using that
patch and am only keeping this hack in the second patch (both attached)

>> +#if !__GNU__
>>      int status = pid.wait(true);
>>      if (status != 0)
>>          throw Error(format("cannot kill processes for uid `%1%': %2%") % uid % statusToString(status));
>> +#endif
>
> Do you know what the rationale was?  It looks like it could leave
> zombies behind us.

No, maybe Manolis knows?  What I do know is why I used the patch: before
applying this patch I could only build up to binutils-boot0.
binutils-boot0 would always fail like so

    ./pre-inst-env guix build -e '(@@ (gnu packages commencement) binutils-boot0)' --no-offload
    XXX fails: Workaround for nix daemon
phase `compress-documentation' succeeded after 0.4 seconds
error: cannot kill processes for uid `999': Operation not permitted
guix build: error: cannot kill processes for uid `999': failed with exit code 1

I haven't been seeing zombies but I'll watch for them now; don't know
what's going on here?

Greetings,
janneke


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-daemon-Break-CHROOT_ENABLED-into-smaller-macros.patch --]
[-- Type: text/x-patch, Size: 2272 bytes --]

From 0307646b22fc488e6342f5814fdef336dd154be3 Mon Sep 17 00:00:00 2001
From: Manolis Ragkousis <manolis837@gmail.com>
Date: Sun, 7 Aug 2016 17:48:30 +0300
Subject: [PATCH 1/2] daemon: Break CHROOT_ENABLED into smaller macros.

Checking for CLONE_NEWNS is only needed for using tha Linux specific clone(2),
otherwise we can use fork(2).

* nix/libstore/build.cc (CHROOT_ENABLED): Break into CHROOT_ENABLED
and CLONE_ENABLED.
(DerivationGoal::startBuilder): Replace CHROOT_ENABLED with CLONE_ENABLED.
(DerivationGoal::runChild): Only define pivot_root() if SYS_pivot_root is
defined.
---
 nix/libstore/build.cc | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 17e92c68a7..fc81e14cd1 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -52,7 +52,12 @@
 #endif
 
 
-#define CHROOT_ENABLED HAVE_CHROOT && HAVE_SYS_MOUNT_H && defined(MS_BIND) && defined(MS_PRIVATE) && defined(CLONE_NEWNS) && defined(SYS_pivot_root)
+#define CHROOT_ENABLED HAVE_CHROOT && HAVE_SYS_MOUNT_H && defined(MS_BIND) && defined(MS_PRIVATE)
+#define CLONE_ENABLED defined(CLONE_NEWNS)
+
+#if defined(SYS_pivot_root)
+#define pivot_root(new_root, put_old) (syscall(SYS_pivot_root, new_root,put_old))
+#endif
 
 #if CHROOT_ENABLED
 #include <sys/socket.h>
@@ -2005,7 +2010,7 @@ void DerivationGoal::startBuilder()
        - The UTS namespace ensures that builders see a hostname of
          localhost rather than the actual hostname.
     */
-#if CHROOT_ENABLED
+#if CLONE_ENABLED
     if (useChroot) {
 	char stack[32 * 1024];
 	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
@@ -2186,10 +2191,8 @@ void DerivationGoal::runChild()
             if (mkdir("real-root", 0) == -1)
                 throw SysError("cannot create real-root directory");
 
-#define pivot_root(new_root, put_old) (syscall(SYS_pivot_root, new_root, put_old))
             if (pivot_root(".", "real-root") == -1)
                 throw SysError(format("cannot pivot old root directory onto '%1%'") % (chrootRootDir + "/real-root"));
-#undef pivot_root
 
             if (chroot(".") == -1)
                 throw SysError(format("cannot change root directory to '%1%'") % chrootRootDir);
-- 
2.24.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-daemon-Avoid-killing-issues-for-the-Hurd.patch --]
[-- Type: text/x-patch, Size: 1420 bytes --]

From f7a04d93f8ef43d56809dc7171b8e681982e2b51 Mon Sep 17 00:00:00 2001
From: Manolis Ragkousis <manolis837@gmail.com>
Date: Wed, 28 Dec 2016 02:49:22 +0200
Subject: [PATCH 2/2] daemon: Avoid killing issues for the Hurd.

This allows for native builds on the Hurd, doing

    sudo ./pre-inst-env guix-daemon --disable-chroot --build-users-group=guixbuild &
    ./pre-inst-env guix build hello

XXX This works around

    ./pre-inst-env guix build -e '(@@ (gnu packages commencement) binutils-boot0)' --no-offload
    phase `compress-documentation' succeeded after 0.4 seconds
    error: cannot kill processes for uid `999': Operation not permitted
    guix build: error: cannot kill processes for uid `999': failed with exit code 1

* nix/libutil/util.cc (killUser)[__GNU__]: Avoid wait failure.
---
 nix/libutil/util.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/nix/libutil/util.cc b/nix/libutil/util.cc
index fb2dfad1f7..df2cb1eb09 100644
--- a/nix/libutil/util.cc
+++ b/nix/libutil/util.cc
@@ -872,9 +872,11 @@ void killUser(uid_t uid)
         _exit(0);
     });
 
+#if !__GNU__
     int status = pid.wait(true);
     if (status != 0)
         throw Error(format("cannot kill processes for uid `%1%': %2%") % uid % statusToString(status));
+#endif
 
     /* !!! We should really do some check to make sure that there are
        no processes left running under `uid', but there is no portable
-- 
2.24.0


[-- Attachment #4: Type: text/plain, Size: 152 bytes --]


-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com