Re: [PATCH] daemon: Split CHROOT_ENABLED into CHROOT_ENABLED and CONTAINER_ENABLED.

[Mark H Weaver]

Manolis Ragkousis <address@hidden> writes:

> With this patch, the daemon can perform chrooted builds on Hurd, without
> creating problems to other parts of the daemon that can't be supported.
>
> So as Mark said, the cases are:
>
> 1. CONTAINER_ENABLED and CHROOT_ENABLED are both true.
> In this case, the daemon works as expected, which is what happens in Linux 
> now.
>
> 2. CONTAINER_ENABLED is false and CHROOT_ENABLED is true.
> Here, things like namespaces cannot be supported, but we can still
> perform chrooted builds.
>
> 3. CONTAINER_ENABLED and CHROOT_ENABLED are both false.
> Here, the daemon is unusable on the system, as it should.
>
> From 9faae6784c63a47f3cc8faa160c208f60dad1e9c Mon Sep 17 00:00:00 2001
> From: Manolis Ragkousis <address@hidden>
> Date: Thu, 20 Aug 2015 13:50:04 +0300
> Subject: [PATCH] daemon: Split CHROOT_ENABLED into CHROOT_ENABLED and
>  CONTAINER_ENABLED.
>
> * nix/libstore/build.cc (CHROOT_ENABLED): Split.
>   (DerivationGoal::startBuilder): Replace CHROOT_ENABLED with 
> CONTAINER_ENABLED.
>   (DerivationGoal::runChild): Same.
> ---
>  nix/libstore/build.cc | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> index a9eedce..7cde735 100644
> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -51,7 +51,15 @@
>  #include <linux/fs.h>
>  #endif
>  
> -#define CHROOT_ENABLED HAVE_CHROOT && HAVE_UNSHARE && HAVE_SYS_MOUNT_H && 
> defined(MS_BIND) && defined(MS_PRIVATE) && defined(CLONE_NEWNS) && 
> defined(SYS_pivot_root)
> +/* In non Linux systems we can still support chroot builds, even
> +   though <sys/mount.h> doesn't exist.*/
> +#if __linux__
> +#define CHROOT_ENABLED HAVE_CHROOT && HAVE_SYS_MOUNT_H
> +#else
> +#define CHROOT_ENABLED HAVE_CHROOT
> +#endif
> +
> +#define CONTAINER_ENABLED CHROOT_ENABLED && defined(MS_BIND) && 
> defined(MS_PRIVATE) && defined(CLONE_NEWNS) && defined(SYS_pivot_root)
>  
>  #if CHROOT_ENABLED
>  #include <sys/socket.h>
> @@ -1946,7 +1954,7 @@ void DerivationGoal::startBuilder()
>         - The UTS namespace ensures that builders see a hostname of
>           localhost rather than the actual hostname.
>      */
> -#if CHROOT_ENABLED
> +#if CONTAINER_ENABLED
>      if (useChroot) {
>       char stack[32 * 1024];
>       int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | 
> SIGCHLD;
> @@ -1994,7 +2002,7 @@ void DerivationGoal::runChild()
>  
>          commonChildInit(builderOut);
>  
> -#if CHROOT_ENABLED
> +#if CONTAINER_ENABLED
>          if (useChroot) {
>              /* Initialise the loopback interface. */
>              AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP));

This still doesn't look right.  What about all the code starting from
line 2040 that sets up /dev and /etc in the chroot, as well as all the
store directories, and notably the code starting from line 2123 that
actually does the chroot?

As you have it now, I don't think it's actually doing the chroot at all
when CONTAINER_ENABLED is false.  Am I missing something?

      Mark