[1/1] Add support for custom initramfs contents

From: Richard Kunze <richard.kunze@web.de>

Signed-off-by: Raphael Jacob <r.jacob2002@gmail.com>
---
 fs/initramfs/Config.in    | 34 +++++++++++++++++++++++++++++++++-
 fs/initramfs/initramfs.mk | 18 +++++++++++++++++-
 linux/linux.mk            |  8 ++++----
 3 files changed, 54 insertions(+), 6 deletions(-)

Raphael, Richard, All,

Here's my long-promiosed review of this patch; sorry for the delay...

On 2018-08-10 20:34 +0200, Raphael Jacob spake thusly:
> From: Richard Kunze <richard.kunze@web.de>
> 
> Signed-off-by: Raphael Jacob <r.jacob2002@gmail.com>

First, two comments:

 1. the author and the SoB lines differ: this is not good. There must be
    a SoB line from the author.

 2. this is a rather big and complex change that has absolutely no
    explanation in the commit log. There is no rationale for the change,
    and there is no explanations of the tricky changes it brings.

So, just because of these two points, I am not fond of it at all.

But then, discussing on IRC with ski777 (Rapahel? Richard? At least it
starts with an 'R' ;-) ), the purpose is less opaque: to be able to
create an initrd from parts of $(TARGET_DIR), that will then be able to
mount the final / filesystem, which is itself made from the entirety of
$(TARGET_DIR).

At least that is what I understood. Please correct me if I am wrong.

If I am correct, I have to say that I am not too fond of this either.

Note that I *do* understand the reason behind the proposed change, but I
don't think it is good overall, because it opens a can of worms, where
people will then want a similar solution for the other filesystems, to
generate those filesystems with only parts of the content of TARGET_DIR,
and we already shot down such proposals in the past.

Instead, here is what I suggest people do:

  - have a first defconfig that builds the kernel and a simple userland
    that acts as the initrd;

  - have a second defconfig that builds the complete, final system.

Of course, there is a gotcha: the final system will need all the kernel
modules, but they are built from the first defconfig; and the initrd may
not need all of the kernel modules, just the few ones needed to mount
the comnplete final system.

So:

  - have a post-build script that is run from the first defconfig, which
    copies all the modules to a "well-known" location, keeps in
    TARGET_DIR only the ones required to mount the final system.

  - a second post-build script (or a custom package) that copies the
    kernel modules from the "well-known" location into the second
    defconfig's TARGET_DIR.

  - implement a new, custom type of filesystem that generates the final
    image from the kwernel iamge from the first defconfig and the rootfs
    from the second defconfig.

At least, that is exactly what I've been doing in similar conditions.

A simple way to have a well-known location is to have a wrapper script
that does the two builds in sequence:

    #!/bin/sh
    set -e

    WELL_KNOWN_DIR=$(mktemp -d)
    export WELL_KNOWN_DIR

    make my_project_initrd_defconfig
    make my_project_complete_defconfig

    rm -rf "${WELL_KNOWN_DIR}"

And the post-build scripts each refer to the WELL_KNOWN_DIR environment
variable (you can be inventive with a better name, of course!)

If you are using a br2-external tree (which is probably a good idea to
store the defconfigs, and other local customisations such as the kernel
config, the post-build scripts, the local packages, in a VCS), you can
have the external.mk export that location such as:

    export WELL_KNOWN_DIR = $(BR2_EXTERNAL_MY_NAME_PATH)/tmp

Again, be inventive with names and locations....

Regards,
Yann E. MORIN.

> ---
>  fs/initramfs/Config.in    | 34 +++++++++++++++++++++++++++++++++-
>  fs/initramfs/initramfs.mk | 18 +++++++++++++++++-
>  linux/linux.mk            |  8 ++++----
>  3 files changed, 54 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/initramfs/Config.in b/fs/initramfs/Config.in
> index 9d5a3f92e6..9b7ac9475e 100644
> --- a/fs/initramfs/Config.in
> +++ b/fs/initramfs/Config.in
> @@ -1,6 +1,22 @@
>  config BR2_TARGET_ROOTFS_INITRAMFS
>  	bool "initial RAM filesystem linked into linux kernel"
>  	depends on BR2_LINUX_KERNEL
> +	help
> +	  Integrate an initramfs inside the kernel image.
> +	  This integration will take place automatically.
> +
> +	  The initramfs contents can either be the full root filesystem
> +	  generated by buildroot, or an image built from a custom list
> +	  of files specified via the BR2_TARGET_ROOTFS_INITRAMFS_CUSTOM_CONTENTS
> +	  config option.
> +
> +choice
> +	prompt "initial RAM filesystem contents"
> +	depends on BR2_TARGET_ROOTFS_INITRAMFS
> +
> +config BR2_TARGET_ROOTFS_INITRAMFS_ROOTFS
> +	bool "rootfs.cpio"
> +	depends on BR2_TARGET_ROOTFS_INITRAMFS
>  	select BR2_TARGET_ROOTFS_CPIO
>  	help
>  	  Integrate the root filesystem generated by Buildroot as an
> @@ -13,10 +29,26 @@ config BR2_TARGET_ROOTFS_INITRAMFS
>  	  is used, regardless of how buildroot's cpio archive is
>  	  configured.
>  
> -	  Note that enabling initramfs together with another filesystem
> +	  Note that enabling this option together with another filesystem
>  	  formats doesn't make sense: you would end up having two
>  	  identical root filesystems, one embedded inside the kernel
>  	  image, and one separately.
> +config BR2_TARGET_ROOTFS_INITRAMFS_CUSTOM
> +	bool "custom initramfs contents"
> +	depends on BR2_TARGET_ROOTFS_INITRAMFS
> +	help
> +	  Use the contents of BR2_TARGET_ROOTFS_INITRAMFS_CUSTOM_CONTENTS
> +	  as the linux kernel configuration variable CONFIG_INITRAMFS_SOURCE
> +
> +endchoice
> +
> +config BR2_TARGET_ROOTFS_INITRAMFS_CUSTOM_CONTENTS
> +	string "custom initramfs contents"
> +	depends on BR2_TARGET_ROOTFS_INITRAMFS_CUSTOM
> +	help
> +	  Custom value to use as CONFIG_INITRAMFS_SOURCE linux kernel configuration
> +	  variable. See Documentation/filesystems/ramfs-rootfs-initramfs.txt in the
> +	  linux kernel sources for details.
>  
>  comment "initramfs needs a Linux kernel to be built"
>  	depends on !BR2_LINUX_KERNEL
> diff --git a/fs/initramfs/initramfs.mk b/fs/initramfs/initramfs.mk
> index c751093214..ef3fdc6d3d 100644
> --- a/fs/initramfs/initramfs.mk
> +++ b/fs/initramfs/initramfs.mk
> @@ -4,6 +4,12 @@
>  #
>  ################################################################################
>  
> +ifeq ($(BR2_TARGET_ROOTFS_INITRAMFS_ROOTFS),y)
> +ROOTFS_INITRAMFS_DEPENDENCIES += rootfs-cpio
> +endif
> +
> +ROOTFS_INITRAMFS_DEPENDENCIES += $(BINARIES_DIR)/initramfs.cpio
> +
>  # The generic fs infrastructure isn't very useful here.
>  #
>  # The initramfs image does not actually build an image; its only purpose is:
> @@ -19,13 +25,23 @@
>  # advertise that our dependency is on the rootfs-cpio rule, which is
>  # cleaner in the dependency graph.
>  
> -rootfs-initramfs: linux-rebuild-with-initramfs
> +rootfs-initramfs: $(ROOTFS_INITRAMFS_DEPENDENCIES) linux-rebuild-with-initramfs
>  
>  rootfs-initramfs-show-depends:
>  	@echo rootfs-cpio
>  
>  .PHONY: rootfs-initramfs rootfs-initramfs-show-depends
>  
> +ifeq ($(BR2_TARGET_ROOTFS_INITRAMFS_ROOTFS),y)
> +$(BINARIES_DIR)/initramfs.cpio: rootfs-cpio
> +	ln -sf rootfs.cpio $(BINARIES_DIR)/initramfs.cpio
> +endif
> +
> +ifeq ($(BR2_TARGET_ROOTFS_INITRAMFS_CUSTOM),y)
> +$(BINARIES_DIR)/initramfs.cpio: target-finalize $(call qstrip,$(BR2_TARGET_ROOTFS_INITRAMFS_CUSTOM_CONTENTS))
> +	$(LINUX_DIR)/usr/gen_init_cpio $(BR2_TARGET_ROOTFS_INITRAMFS_CUSTOM_CONTENTS) > $(BINARIES_DIR)/initramfs.cpio
> +endif
> +
>  ifeq ($(BR2_TARGET_ROOTFS_INITRAMFS),y)
>  TARGETS_ROOTFS += rootfs-initramfs
>  endif
> diff --git a/linux/linux.mk b/linux/linux.mk
> index 7f4c916671..b98043419b 100644
> --- a/linux/linux.mk
> +++ b/linux/linux.mk
> @@ -278,8 +278,8 @@ define LINUX_KCONFIG_FIXUP_CMDS
>  	# replaced later by the real cpio archive, and the kernel will be
>  	# rebuilt using the linux-rebuild-with-initramfs target.
>  	$(if $(BR2_TARGET_ROOTFS_INITRAMFS),
> -		touch $(BINARIES_DIR)/rootfs.cpio
> -		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_SOURCE,"$${BR_BINARIES_DIR}/rootfs.cpio",$(@D)/.config)
> +		touch $(BINARIES_DIR)/initramfs.cpio
> +		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_SOURCE,"$${BR_BINARIES_DIR}/initramfs.cpio",$(@D)/.config)
>  		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_ROOT_UID,0,$(@D)/.config)
>  		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_ROOT_GID,0,$(@D)/.config))
>  	$(if $(BR2_ROOTFS_DEVICE_CREATION_STATIC),,
> @@ -501,11 +501,11 @@ endif # BR_BUILDING
>  $(eval $(kconfig-package))
>  
>  # Support for rebuilding the kernel after the cpio archive has
> -# been generated.
> +# been generated in $(BINARIES_DIR)/initramfs.cpio.
>  .PHONY: linux-rebuild-with-initramfs
>  linux-rebuild-with-initramfs: $(LINUX_DIR)/.stamp_target_installed
>  linux-rebuild-with-initramfs: $(LINUX_DIR)/.stamp_images_installed
> -linux-rebuild-with-initramfs: rootfs-cpio
> +linux-rebuild-with-initramfs: $(BINARIES_DIR)/initramfs.cpio
>  linux-rebuild-with-initramfs:
>  	@$(call MESSAGE,"Rebuilding kernel with initramfs")
>  	# Build the kernel.
> -- 
> 2.18.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

Richard, Raphael, All,

On 2018-08-19 18:05 +0200, Richard Kunze spake thusly:
> Yes, you did understand ski7777 (that's Raphael :-)) on IRC correctly -
> we're indeed using the initrd to set up the environment for the "real"
> root file system.

OK, so next time, do provide such explanations from the onset, it helps
in the review.

Consider that the commit log is here to:

 1. explain the problem, i.e. what you expected and fails, or what you
    need and is missing...

 2. explain the cause of the problem, e.g. the current limitations that
    cause the above grievance;

 3. explain the remedial to lift the limitations and fix the problem.

The commit log is usefull:

 1. to the reviewers, to understand the patch;

 2. to the author, to formalise their thoughts about the issue;

 3. to anyone in the future, author included, to understand what the
    change was made back at the time it was made.

> I thought the requirement for an additional initrd with different
> content from the main root file system would be quite common (after
> all, almost all server and desktop linux distributions use a similar
> setup) and wondered why Buildroot did not provide any means for this,

Probably mainly because Buildroot does not target desktop or servers,
but mostly embedded devices that are not meant to be taht flexible that
they require an initrd; i.e. the overwhelming majority of embedded
devices boot directly into their final rootfs.

The only case I've seen, is a device that would boot into a very minimal
system, download the real image from a server, and kexec into that. But
then that case would even be better served with two separate defconfigs
as well, because they really were two different systems.

> so I implemented it back at the start of our project (
> https://github.com/ftCommunity/ftcommunity-TXT in case you're
> curious). 
> 
> And now, while finally switching from our initial, hacked-up-buildroot-
> tree initial setup to a proper br-external setup (should have done that
> from the start, really), this was the final change that prevented
> building from a pristine Buildroot tree, so we decided to try and get
> it upstream ;-)
> I can understand your reasoning why you don't want to accept it,

Note that I did not say 'no'. I said I did not like it; which is a bit
different. ;-) I don't have the final say.

At least, provide a commit log that completely explains the reason for
the commit, that would be a very good step forward. (TBH, that is the
main sticking point for me in the current state.)

> though, and will probably implement a two-stage build as suggested in
> your mail instead.

That is my position: to suggest such a two step procedure, because it is
the most flexible solution.

However, as I said: I *do* understand the underlying reason for your
proposal. Let's just try to find the best way to do it.

> Again, thank you very much for your review and advice.

You're welcome! :-)

Regards,
Yann E. MORIN.

Yann, Richard, All,

>Well, guess I deserved that. And TBH, I expected something along those lines
as soon as I saw what Raphael sent to the mailing list ;-)

I´m not an expert in what you built there and I thought the Config.in
comments would be self explaining.

I hope Richard can do it better and Yann will merge it.

Raphael

Richard Kunze <richard.kunze@web.de> schrieb am So., 19. Aug. 2018 um
18:56 Uhr:

> Yann, All,
>
> > OK, so next time, do provide such explanations from the onset, it
> > helps in the review.
>
> Well, guess I deserved that. And TBH, I expected something along those
> lines as soon as I saw what Raphael sent to the mailing list ;-)
>
> > Note that I did not say 'no'. I said I did not like it; which is a
> > bit different. ;-) I don't have the final say.
> >
> > At least, provide a commit log that completely explains the reason
> > for the commit, that would be a very good step forward. (TBH, that is
> > the main sticking point for me in the current state.)
>
> OK, I'll do a proper V2 version.
>
> Best regards,
>
> Richard
>
>
>
<div dir="ltr"><div>Yann, Richard, All,</div><div><br></div>&gt;<span style="color:rgb(33,33,33)">Well, guess I deserved that. And TBH, I expected something along those </span><span style="color:rgb(33,33,33)">lines as soon as I saw what Raphael sent to the mailing list ;-)</span><div><span style="color:rgb(33,33,33)"><br></span></div><div><span style="color:rgb(33,33,33)">I´m not an expert in what you built there and I thought the Config.in comments would be self explaining.</span></div><div><span style="color:rgb(33,33,33)"><br></span></div><div><span style="color:rgb(33,33,33)">I hope Richard can do it better and Yann will merge it.</span></div><div><span style="color:rgb(33,33,33)"><br></span></div><div><span style="color:rgb(33,33,33)">Raphael</span></div></div><br><div class="gmail_quote"><div dir="ltr">Richard Kunze &lt;<a href="mailto:richard.kunze@web.de">richard.kunze@web.de</a>&gt; schrieb am So., 19. Aug. 2018 um 18:56 Uhr:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Yann, All,<br>
<br>
&gt; OK, so next time, do provide such explanations from the onset, it<br>
&gt; helps in the review.<br>
<br>
Well, guess I deserved that. And TBH, I expected something along those<br>
lines as soon as I saw what Raphael sent to the mailing list ;-)<br>
<br>
&gt; Note that I did not say &#39;no&#39;. I said I did not like it; which is a<br>
&gt; bit different. ;-) I don&#39;t have the final say.<br>
&gt; <br>
&gt; At least, provide a commit log that completely explains the reason<br>
&gt; for the commit, that would be a very good step forward. (TBH, that is<br>
&gt; the main sticking point for me in the current state.)<br>
<br>
OK, I&#39;ll do a proper V2 version.<br>
<br>
Best regards,<br>
<br>
Richard<br>
<br>
<br>
</blockquote></div>