From mboxrd@z Thu Jan  1 00:00:00 1970
From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=)
Subject: Re: [PATCH] daemon: Break CHROOT_ENABLED into smaller macros.
Date: Wed, 07 Sep 2016 10:18:33 +0200
Message-ID: <87a8fkqg12.fsf@gnu.org>
References: <85b0be72-305b-d001-6009-f996c37e422f@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Return-path: <guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org>
Received: from eggs.gnu.org ([2001:4830:134:3::10]:57085)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <ludo@gnu.org>) id 1bhY4B-0007Ta-Ff
	for guix-devel@gnu.org; Wed, 07 Sep 2016 04:18:44 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <ludo@gnu.org>) id 1bhY45-0003Df-Cc
	for guix-devel@gnu.org; Wed, 07 Sep 2016 04:18:42 -0400
In-Reply-To: <85b0be72-305b-d001-6009-f996c37e422f@gmail.com> (Manolis
	Ragkousis's message of "Thu, 11 Aug 2016 11:17:21 +0300")
List-Id: "Development of GNU Guix and the GNU System distribution."
	<guix-devel.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/archive/html/guix-devel/>
List-Post: <mailto:guix-devel@gnu.org>
List-Help: <mailto:guix-devel-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=subscribe>
Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org
Sender: "Guix-devel" <guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org>
To: Manolis Ragkousis <manolis837@gmail.com>
Cc: guix-devel <guix-devel@gnu.org>

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=E2=80=99s 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
>=20=20
> -#define CHROOT_ENABLED HAVE_CHROOT && HAVE_SYS_MOUNT_H && defined(MS_BIN=
D) && defined(MS_PRIVATE) && defined(CLONE_NEWNS) && defined(SYS_pivot_root)
> +#define CHROOT_ENABLED HAVE_CHROOT && HAVE_SYS_MOUNT_H && defined(MS_BIN=
D) && defined(MS_PRIVATE)
> +#define CLONE_ENABLED defined(CLONE_NEWNS)

This a misleading name, and I think it=E2=80=99s 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=E2=80=99.