[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] daemon: Split CHROOT_ENABLED into CHROOT_ENABLED and CONTAIN
From: |
Mark H Weaver |
Subject: |
Re: [PATCH] daemon: Split CHROOT_ENABLED into CHROOT_ENABLED and CONTAINER_ENABLED. |
Date: |
Fri, 21 Aug 2015 22:28:18 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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