[PATCH] daemon: Break CHROOT_ENABLED into smaller macros.

[PATCH] daemon: Break CHROOT_ENABLED into smaller macros.

[Manolis Ragkousis]

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

Hello,

This is an updated version of my previous "daemon: Break CHROOT_ENABLED
into CHROOT_ENABLED and CLONE_ENABLED." patch which also changes how
pivot_root() is defined.

Thank you,
Manolis

[-- 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; name="0001-daemon-Break-CHROOT_ENABLED-into-smaller-macros.patch", Size: 2378 bytes --]

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

We need to check for CLONE_NEWNS only when we want to use the
Linux specific clone(). Otherwise we use fork().  Also we define
pivot_root() only if SYS_pivot_root is defined.

* nix/libstore/build.cc (CHROOT_ENABLED): Break into CHROOT_ENABLED
and CLONE_ENABLED.  Define pivot_root() only if SYS_pivot_root is defined.
(DerivationGoal::startBuilder): Replace CHROOT_ENABLED with CLONE_ENABLED.
---
 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 ae78e65..156ae8c 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -51,7 +51,12 @@
 #include <linux/fs.h>
 #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>
@@ -1998,7 +2003,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;
@@ -2179,10 +2184,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.9.2

Re: [PATCH] daemon: Break CHROOT_ENABLED into smaller macros.

[Ludovic Courtès]

Hi!

Manolis Ragkousis <manolis837@gmail.com> skribis:

> From aea4bf23b699b7ef5d7007b81f296b77324d5b6c Mon Sep 17 00:00:00 2001
> From: Manolis Ragkousis <manolis837@gmail.com>
> Date: Sun, 7 Aug 2016 17:48:30 +0300
> Subject: [PATCH] daemon: Break CHROOT_ENABLED into smaller macros.
>
> We need to check for CLONE_NEWNS only when we want to use the
> Linux specific clone(). Otherwise we use fork().  Also we define
> pivot_root() only if SYS_pivot_root is defined.

Nipick: it’s enough to write 'clone', without parentheses (info
"(standards) GNU Manuals").

> * nix/libstore/build.cc (CHROOT_ENABLED): Break into CHROOT_ENABLED
> and CLONE_ENABLED.  Define pivot_root() only if SYS_pivot_root is defined.
> (DerivationGoal::startBuilder): Replace CHROOT_ENABLED with CLONE_ENABLED.

[...]

> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -51,7 +51,12 @@
>  #include <linux/fs.h>
>  #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)

This a misleading name, and I think it’s best to directly use #ifdef
CLONE_NEWNS.

> +#if defined(SYS_pivot_root)
> +#define pivot_root(new_root, put_old) (syscall(SYS_pivot_root, new_root,put_old))
    ^
Insert space here.

>         - The UTS namespace ensures that builders see a hostname of
>           localhost rather than the actual hostname.
>      */
> -#if CHROOT_ENABLED
> +#if CLONE_ENABLED

#ifdef CLONE_NEWNS.

Could you send an updated patch?

Thank you for being patient enough!

Ludo’.