fs: Bail out on errors in fakeroot script

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.

---
 fs/common.mk | 1 +
 1 file changed, 1 insertion(+)

index 13bf4ad..1d3926f 100644
@@ -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

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.

---
 fs/common.mk     |  1 +
 system/Config.in | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

index 1d3926f..5c4cd6a 100644
@@ -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)

index 95e10ab..b416377 100644
@@ -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

Stefan, All,

On 2015-02-24 16:47 +0100, Stefan S?rensen spake thusly:

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...

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...

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).

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--]

.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

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.

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).

I will rewrite it to not mention fakeroot.

I will add a comment to the help text.

Stefan

Stefan,

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

Stefan, All,

On 2015-02-24 16:47 +0100, Stefan S?rensen spake thusly:

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.  |
'------------------------------^-------^------------------^--------------------'

Dear Stefan S?rensen,

Applied, thanks!

Thomas

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