[Buildroot] [PATCH 1/2] fs: Bail out on errors in fakeroot script

[Buildroot] [PATCH 1/2] fs: Bail out on errors in fakeroot script

[Stefan Sørensen]

Currently, the generated fakeroot script has no error checking causing
make to continue building even if some of the fakeroot script commands
have failed. This can cause e.g. using an invalid device tables to go
unnoticed.

So add a "set -e" to the start of the fakeroot script so it will exit
with a failure code as soon as one of the script commands fails.

Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
---
 fs/common.mk | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/common.mk b/fs/common.mk
index 13bf4ad..1d3926f 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -72,6 +72,7 @@ $$(BINARIES_DIR)/rootfs.$(1): target-finalize $$(ROOTFS_$(2)_DEPENDENCIES)
 	rm -f $$(FAKEROOT_SCRIPT)
 	rm -f $$(TARGET_DIR_WARNING_FILE)
 	rm -f $$(USERS_TABLE)
+	echo "set -e" >> $$(FAKEROOT_SCRIPT)
 	echo "chown -h -R 0:0 $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
 ifneq ($$(ROOTFS_DEVICE_TABLES),)
 	cat $$(ROOTFS_DEVICE_TABLES) > $$(FULL_DEVICE_TABLE)
-- 
1.9.3

[Buildroot] [PATCH 2/2] fs: add custom fakeroot script support

[Stefan Sørensen]

This patch add a new config option, BR2_ROOTFS_FAKEROOT_SCRIPT, a list of
scripts that are executed in the fakeroot environment as the last step
before creating the filesystem image.

The scripts can for example be used to invoke a tool that operates on
file ownerships, device nodes or any other custom action that requires
root privileges.

Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>
---
 fs/common.mk     |  1 +
 system/Config.in | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/fs/common.mk b/fs/common.mk
index 1d3926f..5c4cd6a 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -87,6 +87,7 @@ ifneq ($$(ROOTFS_USERS_TABLES),)
 endif
 	printf '$$(subst $$(sep),\n,$$(PACKAGES_USERS))' >> $$(USERS_TABLE)
 	PATH=$$(BR_PATH) $$(TOPDIR)/support/scripts/mkusers $$(USERS_TABLE) $$(TARGET_DIR) >> $$(FAKEROOT_SCRIPT)
+	$$(foreach s,$$(call qstrip,$$(BR2_ROOTFS_FAKEROOT_SCRIPT)),echo $$(s) $$(TARGET_DIR) >> $$(FAKEROOT_SCRIPT)$$(sep))
 	echo "$$(ROOTFS_$(2)_CMD)" >> $$(FAKEROOT_SCRIPT)
 	chmod a+x $$(FAKEROOT_SCRIPT)
 	PATH=$$(BR_PATH) $$(HOST_DIR)/usr/bin/fakeroot -- $$(FAKEROOT_SCRIPT)
diff --git a/system/Config.in b/system/Config.in
index 95e10ab..b416377 100644
--- a/system/Config.in
+++ b/system/Config.in
@@ -421,6 +421,22 @@ config BR2_ROOTFS_POST_BUILD_SCRIPT
 	  argument. Make sure the exit code of those scripts are 0, otherwise
 	  make will stop after calling them.
 
+config BR2_ROOTFS_FAKEROOT_SCRIPT
+	string "Custom scripts to run as fake root"
+	default ""
+	help
+	  Specify a space-separated list of scripts to be run as part
+	  of the fakeroot script, just before Buildroot packs the
+	  files into selected filesystem images.
+
+	  This can for example be used to invoke a tool that operates on
+	  file ownerships, device nodes or any other custom action that
+	  requires root privileges.
+
+	  These scripts are called with the target directory name as
+	  first argument. The script is executed from the main Buildroot
+	  source directory as the current directory.
+
 config BR2_ROOTFS_POST_IMAGE_SCRIPT
 	string "Custom scripts to run after creating filesystem images"
 	default ""
-- 
1.9.3

[Buildroot] [PATCH 1/2] fs: Bail out on errors in fakeroot script

[Yann E. MORIN]

Stefan, All,

On 2015-02-24 16:47 +0100, Stefan S?rensen spake thusly:
> Currently, the generated fakeroot script has no error checking causing
> make to continue building even if some of the fakeroot script commands
> have failed. This can cause e.g. using an invalid device tables to go
> unnoticed.
> 
> So add a "set -e" to the start of the fakeroot script so it will exit
> with a failure code as soon as one of the script commands fails.
> 
> Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>

Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> ---
>  fs/common.mk | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/common.mk b/fs/common.mk
> index 13bf4ad..1d3926f 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -72,6 +72,7 @@ $$(BINARIES_DIR)/rootfs.$(1): target-finalize $$(ROOTFS_$(2)_DEPENDENCIES)
>  	rm -f $$(FAKEROOT_SCRIPT)
>  	rm -f $$(TARGET_DIR_WARNING_FILE)
>  	rm -f $$(USERS_TABLE)
> +	echo "set -e" >> $$(FAKEROOT_SCRIPT)
>  	echo "chown -h -R 0:0 $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
>  ifneq ($$(ROOTFS_DEVICE_TABLES),)
>  	cat $$(ROOTFS_DEVICE_TABLES) > $$(FULL_DEVICE_TABLE)
> -- 
> 1.9.3
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 2/2] fs: add custom fakeroot script support

[Yann E. MORIN]

Stefan, All,

On 2015-02-24 16:47 +0100, Stefan S?rensen spake thusly:
> This patch add a new config option, BR2_ROOTFS_FAKEROOT_SCRIPT, a list of
> scripts that are executed in the fakeroot environment as the last step
> before creating the filesystem image.

I'm not specially opposed to that feature (and in fact I think this
would be nice to have!), but parts of the justification are not
completely right, see below...

> The scripts can for example be used to invoke a tool that operates on
> file ownerships,

For file ownership, we already have BR2_ROOTFS_DEVICE_TABLE: "Path to
the permission tables". The naming of the option may be a bit
misleading, indeed, but that's historical...

> device nodes

Ditto, we also already have BR2_ROOTFS_STATIC_DEVICE_TABLE: "Path to the
device tables". That can indeed only be used when using static /dev
management, but when /dev is managed dynamically (devtmpfs, eudev, mdev
or systemd), having custom device tables is completely useless (as /dev
is a mountpoint, and existing /dev entries would become invisible).

> or any other custom action that requires
> root privileges.

What would be those kind of "custom actions" that are not covered by the
above?

One thing I can see as problematic is assigning ownership to a whole
directory tree, of which the content might be unpredictable at configure
time (e.g. so that it is not possible to provide a permission table for
all the entries); something one might trivially do (with your solution):

    #!/bin/sh
    TARGET_DIR="${1}"
    chown -R uid:gid ${TARGET_DIR}/foo/bar/

But even with what we currently have, this is already doable (albeit
this is a bit of a hack). One would set:

    BR2_ROOTFS_DEVICE_TABLE="system/device_table.txt $(BUILD_DIR)/tmp-perms"
    BR2_ROOTFS_POST_BUILD_SCRIPT="/path/to/post-build.script"
    BR2_ROOTFS_POST_SCRIPT_ARGS"$(BUILD_DIR)"

and then have /path/to/post-build.script contain something like:

    #!/bin/sh
    TARGET_DIR="${1}"
    BUILD_DIR="${2}"
    uid=42
    gid=31415
    find ${TARGET_DIR}/foo/bar/ \( -type f -o -type d \) \
    |while read entry; do
        [ -f "${entry}" ] && t=f
        [ -d "${entry}" ] && t=d
        m="$( stat -c '%a' "${entry}" )"
        m=$((m&0760)) # Whatever...
        printf "%s %s %s %s %s - - - - -\n" \
              "${entry#${TARGET_DIR}}" ${t} ${m} ${uid} ${gid}
    done >"${BUILD_DIR}/tmp-perms"

Granted, that's neither trivial nor elegant, but that's doable.

[--SNIP--]
> diff --git a/system/Config.in b/system/Config.in
> index 95e10ab..b416377 100644
> --- a/system/Config.in
> +++ b/system/Config.in
> @@ -421,6 +421,22 @@ config BR2_ROOTFS_POST_BUILD_SCRIPT
>  	  argument. Make sure the exit code of those scripts are 0, otherwise
>  	  make will stop after calling them.
>  
> +config BR2_ROOTFS_FAKEROOT_SCRIPT
> +	string "Custom scripts to run as fake root"

I'm not very happy with talking about 'fake root' (which is really
'fakeroot'). I'd prefer we avoid mentionning fakeroot, which is
internal implementation details.

Note also that those scripts would be run once for each filesystem type
the user has enabled; if tar, squashfs, cramfs and ext2/3/4 are enabled,
the scripts would be run four times. Thus, those scripts must be
idempotent (like for the post-build scripts, mind you).

In the end (and after quite some thinking!), I stil fail to see a
compelling reason. Do you have an example?

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 2/2] fs: add custom fakeroot script support

[Sørensen, Stefan]

On Tue, 2015-02-24 at 22:39 +0100, Yann E. MORIN wrote:

> > The scripts can for example be used to invoke a tool that operates on
> > file ownerships,
> 
> For file ownership, we already have BR2_ROOTFS_DEVICE_TABLE: "Path to
> the permission tables". The naming of the option may be a bit
> misleading, indeed, but that's historical...

As you mention yourself, bulk ownership changes are problematic with
device tables. Generating the device table in the post-build script
doesn't seem very elegant to me.

> > or any other custom action that requires
> > root privileges.
> 
> What would be those kind of "custom actions" that are not covered by the
> above?

An example could be changes to files/device nodes that are created
earlier in the script. We use it to remove /dev/console created
by /fs/cpio.mk (our image format doesn't support device nodes).

Creating a manifest file of the whole filesystem, including stuff added
by the fakeroot script, would also need to be done just before image
creation (although here there is no need for being root). 

> > +config BR2_ROOTFS_FAKEROOT_SCRIPT
> > +	string "Custom scripts to run as fake root"
> 
> I'm not very happy with talking about 'fake root' (which is really
> 'fakeroot'). I'd prefer we avoid mentionning fakeroot, which is
> internal implementation details.

I will rewrite it to not mention fakeroot.

> Note also that those scripts would be run once for each filesystem type
> the user has enabled; if tar, squashfs, cramfs and ext2/3/4 are enabled,
> the scripts would be run four times. Thus, those scripts must be
> idempotent (like for the post-build scripts, mind you).

I will add a comment to the help text.

Stefan

[Buildroot] [PATCH 1/2] fs: Bail out on errors in fakeroot script

[Thomas Petazzoni]

Dear Stefan S?rensen,

On Tue, 24 Feb 2015 16:47:34 +0100, Stefan S?rensen wrote:
> Currently, the generated fakeroot script has no error checking causing
> make to continue building even if some of the fakeroot script commands
> have failed. This can cause e.g. using an invalid device tables to go
> unnoticed.
> 
> So add a "set -e" to the start of the fakeroot script so it will exit
> with a failure code as soon as one of the script commands fails.
> 
> Signed-off-by: Stefan S?rensen <stefan.sorensen@spectralink.com>

Applied, thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 2/2] fs: add custom fakeroot script support

[Thomas Petazzoni]

Stefan,

On Wed, 25 Feb 2015 12:02:41 +0000, S?rensen, Stefan wrote:

> > For file ownership, we already have BR2_ROOTFS_DEVICE_TABLE: "Path to
> > the permission tables". The naming of the option may be a bit
> > misleading, indeed, but that's historical...
> 
> As you mention yourself, bulk ownership changes are problematic with
> device tables. Generating the device table in the post-build script
> doesn't seem very elegant to me.

Since commit
http://git.buildroot.net/buildroot/commit/package/makedevs?id=410f9b60137820143caf51a2a37da6f8fa16679f,
you can use the permission table to recursively set the
owernship/rights of a directory and all its subdirectories/files.

Therefore, I don't think this patch is needed, and I've marked it as
Rejected in our patch tracking system. Do not hesitate to repost an
updated version with a new justification if you don't agree, of course.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com