[Buildroot] [RFC] Introduce the 'genimages' infrastructure

[Buildroot] [RFC] Introduce the 'genimages' infrastructure

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

Hello All!

During the last developpers' day in Edimburgh, we discussed my Buildroot.config
project, and especially the 'genimages' infrastructure I've done in there.

I've came up with 'genimages' after I came to the conclusion that the current
images generation in Buildroot is limited to generating a single filesystem
images, but most device do require a more complex setup.

For example. the Rapsberry Pi needs an SDcard (mmcblk0) formatted as thus:

    mmcblk0
    |- mmcblk0p1  (FAT32)
    |  |- bootcode.bin
    |  |- fixup.dat
    |  |- start.elf
    |  |- config.txt
    |  |- cmdline.txt
    |  `- kernel.bin
    `- mmcblk0p2  (whatever, eg. ext3, btrfs...)
       `- all of /

With the current output of Buildroot, that requires a lot of fiddling with
the partitioning of the SDcard (may require root), formatting the filesystems
(may require root), extract rootfs.tar (may require root to mount, requires
root to extract), copy select files from output/images/rpi-firmware/ to the
first partition (may require root to mount).

Each user would either have to manually perform those actions (error prone),
or write a small shell script to automate this (not trivial).

Also, some other devices require special, but different formatting as well.

Rather than have every users all suffer in silence (and sometimes not in
silence) by re-inventing such a script over and over, for each device that
need such a setup, I came up with the 'genimages' idea: a description of
the layout of the storage devices, partitioning schemes, and partitions
content.

So, here is the 'genimages' from Buildroot.config, that I updated (in fact,
trimmed) to include in Buildroot itself.

The basis is that we bundle some trivial partition description for the
boards we have in board/ and reference it in the corresponding defconfigs.


How does 'genimages' work?

First, genimages extract the generated rootfs.tar. Hence, rootfs.tar is
always generated if a partition description is configured.

Second, genimages generates one filesystem image for each partition listed
in the description. Once the filesystem is generated, its mountpoint is
emptied, so it does not appear in the upper filesystem.

Third, genimages aggregates all the partition of a device into a single
image file

Finall, those device images can be flashed directly on the raw device of
the storage, without manually fiddling with  fdisk, mount, tar and so
on...


Regards,
Yann E. MORIN.


The following changes since commit a98442f2b514d296c8639d9fa66e6a64b54dd6b4:

  legal info: update documentation with split target/host output (2013-11-17 09:15:17 +0100)

are available in the git repository at:

  git://gitorious.org/buildroot/buildroot.git yem/rpi-bootloader-in-fs

for you to fetch changes up to 932a7029e1685747fff04ec40650ffc69b76f53e:

  board/raspberrypi: provide partition description for the new genimanges (2013-11-22 22:58:31 +0100)

----------------------------------------------------------------
Yann E. MORIN (5):
      package/rpi-firmware: only install one firmware file
      package/rpi-firmware: add option to install firmware files in target/boot/
      package/rpi-firmware: move to bootloaders menu
      fs/custom: generate complete, partition-based device images
      board/raspberrypi: provide partition description for the new genimanges

 board/raspberrypi/partitions               |  31 ++++
 boot/Config.in                             |   1 +
 boot/rpi-firmware/Config.in                |  57 +++++++
 {package => boot}/rpi-firmware/cmdline.txt |   0
 {package => boot}/rpi-firmware/config.txt  |   0
 boot/rpi-firmware/rpi-firmware.mk          |  26 +++
 configs/raspberrypi_defconfig              |  11 +-
 docs/manual/appendix.txt                   |   1 +
 docs/manual/customize-filesystems.txt      |  35 ++++
 docs/manual/customize.txt                  |   2 +
 docs/manual/partition-layout.txt           | 258 +++++++++++++++++++++++++++++
 fs/Config.in                               |   1 +
 fs/custom/Config.in                        |  18 ++
 fs/custom/boot/mbr                         |  57 +++++++
 fs/custom/boot/pre-post                    |   8 +
 fs/custom/custom.mk                        |  14 ++
 fs/custom/fs/ext                           |  22 +++
 fs/custom/fs/pre-post                      |  40 +++++
 fs/custom/fs/vfat                          |  17 ++
 fs/custom/functions                        |  47 ++++++
 fs/custom/genimages                        | 214 ++++++++++++++++++++++++
 package/Config.in                          |   1 -
 package/rpi-firmware/Config.in             |   9 -
 package/rpi-firmware/rpi-firmware.mk       |  24 ---
 24 files changed, 859 insertions(+), 35 deletions(-)
 create mode 100644 board/raspberrypi/partitions
 create mode 100644 boot/rpi-firmware/Config.in
 rename {package => boot}/rpi-firmware/cmdline.txt (100%)
 rename {package => boot}/rpi-firmware/config.txt (100%)
 create mode 100644 boot/rpi-firmware/rpi-firmware.mk
 create mode 100644 docs/manual/customize-filesystems.txt
 create mode 100644 docs/manual/partition-layout.txt
 create mode 100644 fs/custom/Config.in
 create mode 100644 fs/custom/boot/mbr
 create mode 100644 fs/custom/boot/pre-post
 create mode 100644 fs/custom/custom.mk
 create mode 100644 fs/custom/fs/ext
 create mode 100644 fs/custom/fs/pre-post
 create mode 100644 fs/custom/fs/vfat
 create mode 100644 fs/custom/functions
 create mode 100755 fs/custom/genimages
 delete mode 100644 package/rpi-firmware/Config.in
 delete mode 100644 package/rpi-firmware/rpi-firmware.mk

-- 
.-----------------.--------------------.------------------.--------------------.
|  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 1/5] package/rpi-firmware: only install one firmware file

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

Since only one firmware is used to boot the Raspberry Pi, there is no
reason to install all of them.

Add an option to select what firmware to install.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 package/rpi-firmware/Config.in       | 39 ++++++++++++++++++++++++++++++++++++
 package/rpi-firmware/rpi-firmware.mk |  8 ++------
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/package/rpi-firmware/Config.in b/package/rpi-firmware/Config.in
index eb10a8a..cd45be4 100644
--- a/package/rpi-firmware/Config.in
+++ b/package/rpi-firmware/Config.in
@@ -7,3 +7,42 @@ config BR2_PACKAGE_RPI_FIRMWARE
 
 	  https://github.com/raspberrypi/firmware
 
+if BR2_PACKAGE_RPI_FIRMWARE
+
+choice
+	bool "Firmware to boot"
+	default BR2_PACKAGE_RPI_FIRMWARE_DEFAULT
+	help
+	  There are three different firmware files:
+	    - the default firmware, that enables standard GPU features;
+	    - the extended firmware, that enables additional GPU features
+	      (eg. more audio/video codecs);
+	    - the cut-down firmware, for emergency situations, with only
+	      features required to boot a Linux kernel.
+
+config BR2_PACKAGE_RPI_FIRMWARE_DEFAULT
+	bool "default"
+	help
+	  The default firmware, that enables standard GPU features.
+
+config BR2_PACKAGE_RPI_FIRMWARE_X
+	bool "extended ('x', more codecs)"
+	help
+	  The extended firmware, that enables additional GPU features
+	  (eg. more audio/video codecs).
+
+config BR2_PACKAGE_RPI_FIRMWARE_CD
+	bool "cut-down ('cd', emergency)"
+	help
+	  The cut-down firmware, for emergency situations, with only
+	  features required to boot a Linux kernel.
+
+endchoice
+
+config BR2_PACKAGE_RPI_FIRMWARE_BOOT
+	string
+	default ""      if BR2_PACKAGE_RPI_FIRMWARE_DEFAULT
+	default "_x"    if BR2_PACKAGE_RPI_FIRMWARE_X
+	default "_cd"   if BR2_PACKAGE_RPI_FIRMWARE_CD
+
+endif # BR2_PACKAGE_RPI_FIRMWARE
diff --git a/package/rpi-firmware/rpi-firmware.mk b/package/rpi-firmware/rpi-firmware.mk
index 94badf9..e88efd2 100644
--- a/package/rpi-firmware/rpi-firmware.mk
+++ b/package/rpi-firmware/rpi-firmware.mk
@@ -11,12 +11,8 @@ RPI_FIRMWARE_LICENSE_FILES = boot/LICENCE.broadcom
 
 define RPI_FIRMWARE_INSTALL_TARGET_CMDS
 	$(INSTALL) -D -m 0644 $(@D)/boot/bootcode.bin $(BINARIES_DIR)/rpi-firmware/bootcode.bin
-	$(INSTALL) -D -m 0644 $(@D)/boot/start.elf $(BINARIES_DIR)/rpi-firmware/start.elf
-	$(INSTALL) -D -m 0644 $(@D)/boot/start_cd.elf $(BINARIES_DIR)/rpi-firmware/start_cd.elf
-	$(INSTALL) -D -m 0644 $(@D)/boot/start_x.elf $(BINARIES_DIR)/rpi-firmware/start_x.elf
-	$(INSTALL) -D -m 0644 $(@D)/boot/fixup.dat $(BINARIES_DIR)/rpi-firmware/fixup.dat
-	$(INSTALL) -D -m 0644 $(@D)/boot/fixup_cd.dat $(BINARIES_DIR)/rpi-firmware/fixup_cd.dat
-	$(INSTALL) -D -m 0644 $(@D)/boot/fixup_x.dat $(BINARIES_DIR)/rpi-firmware/fixup_x.dat
+	$(INSTALL) -D -m 0644 $(@D)/boot/start$(BR2_PACKAGE_RPI_FIRMWARE_BOOT).elf $(BINARIES_DIR)/rpi-firmware/start.elf
+	$(INSTALL) -D -m 0644 $(@D)/boot/fixup$(BR2_PACKAGE_RPI_FIRMWARE_BOOT).dat $(BINARIES_DIR)/rpi-firmware/fixup.dat
 	$(INSTALL) -D -m 0644 package/rpi-firmware/config.txt $(BINARIES_DIR)/rpi-firmware/config.txt
 	$(INSTALL) -D -m 0644 package/rpi-firmware/cmdline.txt $(BINARIES_DIR)/rpi-firmware/cmdline.txt
 endef
-- 
1.8.1.2

[Buildroot] [PATCH 1/5] package/rpi-firmware: only install one firmware file

[Arnout Vandecappelle]

On 22/11/13 23:50, Yann E. MORIN wrote:
> From: "Yann E. MORIN"<yann.morin.1998@free.fr>
>
> Since only one firmware is used to boot the Raspberry Pi, there is no
> reason to install all of them.
>
> Add an option to select what firmware to install.
>
> Signed-off-by: "Yann E. MORIN"<yann.morin.1998@free.fr>

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>


  Regards,
  Arnout

[snip]
-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 2/5] package/rpi-firmware: add option to install firmware files in target/boot/

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

The firmware files must reside in a FAT filesystem, in the first partition
of the SDcard, so it makes sense to install them in target/ (eg. for a
post-image script to generate the different partitions from the rootfs.tar).

Add an option (off by default) to install the bootloader files into
$(TARGET_DIR)/boot .

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
Note: this will come very handy when we eventually have our generic
multi-image generation infra, to come in a following patch.
---
 package/rpi-firmware/Config.in       |  9 +++++++++
 package/rpi-firmware/rpi-firmware.mk | 16 +++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/package/rpi-firmware/Config.in b/package/rpi-firmware/Config.in
index cd45be4..ce5b974 100644
--- a/package/rpi-firmware/Config.in
+++ b/package/rpi-firmware/Config.in
@@ -45,4 +45,13 @@ config BR2_PACKAGE_RPI_FIRMWARE_BOOT
 	default "_x"    if BR2_PACKAGE_RPI_FIRMWARE_X
 	default "_cd"   if BR2_PACKAGE_RPI_FIRMWARE_CD
 
+config BR2_PACKAGE_RPI_FIRMWARE_INSTALL_TARGET
+	bool "Install bootloader files into /boot"
+	depends on !BR2_TARGET_ROOTFS_INITRAMFS
+	help
+	  Say 'y' here if you want to have the bootloader files installed
+	  in /boot on the target (the default).
+
+	  Say 'n' to have them installed in $(BINARIES_DIR)/rpi-firmware.
+
 endif # BR2_PACKAGE_RPI_FIRMWARE
diff --git a/package/rpi-firmware/rpi-firmware.mk b/package/rpi-firmware/rpi-firmware.mk
index e88efd2..6adea77 100644
--- a/package/rpi-firmware/rpi-firmware.mk
+++ b/package/rpi-firmware/rpi-firmware.mk
@@ -9,12 +9,18 @@ RPI_FIRMWARE_SITE = http://github.com/raspberrypi/firmware/tarball/$(RPI_FIRMWAR
 RPI_FIRMWARE_LICENSE = BSD-3c
 RPI_FIRMWARE_LICENSE_FILES = boot/LICENCE.broadcom
 
+ifeq ($(BR2_PACKAGE_RPI_FIRMWARE_INSTALL_TARGET),y)
+RPI_FIRMWARE_DEST = $(TARGET_DIR)/boot
+else
+RPI_FIRMWARE_DEST = $(BINARIES_DIR)/rpi-firmware
+endif
+
 define RPI_FIRMWARE_INSTALL_TARGET_CMDS
-	$(INSTALL) -D -m 0644 $(@D)/boot/bootcode.bin $(BINARIES_DIR)/rpi-firmware/bootcode.bin
-	$(INSTALL) -D -m 0644 $(@D)/boot/start$(BR2_PACKAGE_RPI_FIRMWARE_BOOT).elf $(BINARIES_DIR)/rpi-firmware/start.elf
-	$(INSTALL) -D -m 0644 $(@D)/boot/fixup$(BR2_PACKAGE_RPI_FIRMWARE_BOOT).dat $(BINARIES_DIR)/rpi-firmware/fixup.dat
-	$(INSTALL) -D -m 0644 package/rpi-firmware/config.txt $(BINARIES_DIR)/rpi-firmware/config.txt
-	$(INSTALL) -D -m 0644 package/rpi-firmware/cmdline.txt $(BINARIES_DIR)/rpi-firmware/cmdline.txt
+	$(INSTALL) -D -m 0644 $(@D)/boot/bootcode.bin $(RPI_FIRMWARE_DEST)/bootcode.bin
+	$(INSTALL) -D -m 0644 $(@D)/boot/start$(BR2_PACKAGE_RPI_FIRMWARE_BOOT).elf $(RPI_FIRMWARE_DEST)/start.elf
+	$(INSTALL) -D -m 0644 $(@D)/boot/fixup$(BR2_PACKAGE_RPI_FIRMWARE_BOOT).dat $(RPI_FIRMWARE_DEST)/fixup.dat
+	$(INSTALL) -D -m 0644 package/rpi-firmware/config.txt $(RPI_FIRMWARE_DEST)/config.txt
+	$(INSTALL) -D -m 0644 package/rpi-firmware/cmdline.txt $(RPI_FIRMWARE_DEST)/cmdline.txt
 endef
 
 $(eval $(generic-package))
-- 
1.8.1.2

[Buildroot] [PATCH 2/5] package/rpi-firmware: add option to install firmware files in target/boot/

[Thomas Petazzoni]

Dear Yann E. MORIN,

On Fri, 22 Nov 2013 23:50:55 +0100, Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> The firmware files must reside in a FAT filesystem, in the first partition
> of the SDcard, so it makes sense to install them in target/ (eg. for a
> post-image script to generate the different partitions from the rootfs.tar).

I don't understand the reasoning here. These files must be at the root
of a specific FAT filesystem, so I fail to see why installing them
inside the root filesystem is of any help to achieve that.

Yes, I've seen that it's probably related to the proposed logic to
generate a bundled image in PATCH 5/5, but for the moment, my feeling
is that if that logic requires *all* files to be present in the root
filesystem to be later installed in a separate FAT/boot filesystem,
then the logic isn't really appropriate, and should be improved, no?

I believe that the existing choice of installing things in
$(BINARIES_DIR)/rpi-firmware/ was a much better choice. That's a choice
I've kept in the series on Grub/Grub2/Gummiboot, by creating a
$(BINARIES_DIR)/efi/ directory.

Best regards,

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

[Buildroot] [PATCH 2/5] package/rpi-firmware: add option to install firmware files in target/boot/

[Yann E. MORIN]

Thomas, All,

On 2013-11-28 21:11 +0100, Thomas Petazzoni spake thusly:
> On Fri, 22 Nov 2013 23:50:55 +0100, Yann E. MORIN wrote:
> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > 
> > The firmware files must reside in a FAT filesystem, in the first partition
> > of the SDcard, so it makes sense to install them in target/ (eg. for a
> > post-image script to generate the different partitions from the rootfs.tar).
> 
> I don't understand the reasoning here. These files must be at the root
> of a specific FAT filesystem, so I fail to see why installing them
> inside the root filesystem is of any help to achieve that.
> 
> Yes, I've seen that it's probably related to the proposed logic to
> generate a bundled image in PATCH 5/5, but for the moment, my feeling
> is that if that logic requires *all* files to be present in the root
> filesystem to be later installed in a separate FAT/boot filesystem,
> then the logic isn't really appropriate, and should be improved, no?

The basic idea is to not depend on board-specific layout of the boot
files in the genimages infra.

Notice how the boot files for the Raspberry Pi are located in
images/rpi-firmware? This is not a 'generic' location, and we would need
a way for a 'board' to pass a 'description' to genimages of what should
be copied to the partition.

Since we do not have board description in Buildroot (and have gone quite
our way to remove it in the past), I tried not too add more. Of course,
the 'partition layout description' is board-specific, but its syntax is
not board-specific; the one we bundle is just an example, to serve as a
basis for the real file for a real project.

I designed genimages so that it does *not* deal with adding/removing
stuff to/from the filesystem, but just dispatch the directory layout
into different partitions. I find it odd to tinker with the content of
the filesystems at that point (but maybe I am biased, since genimages
used to be a post-image hook, but now is a real fs target).

> I believe that the existing choice of installing things in
> $(BINARIES_DIR)/rpi-firmware/ was a much better choice. That's a choice
> I've kept in the series on Grub/Grub2/Gummiboot, by creating a
> $(BINARIES_DIR)/efi/ directory.

Yes, but 'efi' is generic, it is not named after the target system,
while 'rpi-firmware' is very specific. Maybe we could harmonise the name
for that directory (eg. 'boot-files') and use that name to install all
bootloader files, in which case that'd be probably OK.

( BTW, the current grub package does copy its files into
$(TARGET_DIR)/boot/grub and I like it ;-) )

Except that bootloaders may install different sets of files, and we can't
find a common name for those files. Eg. rpi-firmware installs 5 different
files (bootcode.bin, start.elf, fixup.dat, config.txt, and cmdline.txt),
while grub installs at least three files (stage1, stage2, and any
fs-specific stage1_5, not counting the splashcreens).

Yet, we might be able to use that, if the bootloaders also install a
list of files to copy to the FS.

But then it means 'rootfs.tar' (or any other fs images) does not contain
the same as the 'genimages' images. I think this is bad, and I think all
images should have the same FS content.

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/5] package/rpi-firmware: add option to install firmware files in target/boot/

[Jeremy Rosen]

> > 
> > I don't understand the reasoning here. These files must be at the
> > root
> > of a specific FAT filesystem, so I fail to see why installing them
> > inside the root filesystem is of any help to achieve that.
> > 
> > Yes, I've seen that it's probably related to the proposed logic to
> > generate a bundled image in PATCH 5/5, but for the moment, my
> > feeling
> > is that if that logic requires *all* files to be present in the
> > root
> > filesystem to be later installed in a separate FAT/boot filesystem,
> > then the logic isn't really appropriate, and should be improved,
> > no?

If I may chip-in, I really like the logic of the proposed image builder...

Basically, you build the root filesystem the way it will look when all
sub-filesystems are mounted, you describe how they are mounted and 
the system builds the partitions accordingly.

I know that what I described is not exactly the logic behind the proposed
patch, but it is a logic I like a lot because it is very easy to understand
and explain. it could also allow us to automatically generate some fstab
lines at some point, which could be nice.

[Buildroot] [PATCH 2/5] package/rpi-firmware: add option to install firmware files in target/boot/

[Yann E. MORIN]

Jeremy, All,

On Friday 29 November 2013 09:00:19 Jeremy Rosen wrote:
> If I may chip-in, I really like the logic of the proposed image builder...
> 
> Basically, you build the root filesystem the way it will look when all
> sub-filesystems are mounted, you describe how they are mounted and 
> the system builds the partitions accordingly.

Yes, that's exactly what I feel, too. When the image creation kicks in, the
filesystem(s) content is complete. I see 'genimages' like I look at genext2fs:
simply an image generator.

> I know that what I described is not exactly the logic behind the proposed
> patch, but it is a logic I like a lot because it is very easy to understand
> and explain. it could also allow us to automatically generate some fstab
> lines at some point, which could be nice.

And 'genimages' already does add the filesystems to the fstab. ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +0/33 662376056 | Software  Designer | \ / CAMPAIGN     |   ^                |
| --==< O_o >==-- '------------.-------:  X  AGAINST      |  /e\  There is no  |
| http://ymorin.is-a-geek.org/ | (*_*) | / \ HTML MAIL    |  """  conspiracy.  |
'------------------------------'-------'------------------'--------------------'

[Buildroot] [PATCH 2/5] package/rpi-firmware: add option to install firmware files in target/boot/

[Thomas Petazzoni]

Dear Jeremy Rosen,

On Fri, 29 Nov 2013 09:00:19 +0100 (CET), Jeremy Rosen wrote:

> Basically, you build the root filesystem the way it will look when all
> sub-filesystems are mounted, you describe how they are mounted and 
> the system builds the partitions accordingly.

I don't necessarily agree here, because this strategy makes the
confusion between two things:

 * The contents of the /boot directory in the root filesystem.

 * The contents of the boot partition needed for some platforms (OMAP,
   RPi, UEFI-based x86 platforms, etc.)

Those are two different things, and Yann's proposal merges them. As an
example, provided your bootloader has ext3 support and you're on an
OMAP platform, you may want to have something like:

 * The /boot directory in the root filesystem contains the kernel image
   (uImage, zImage)

 * The boot partition contains just the bootloader images (MLO,
   u-boot.img).

There is no point to have the kernel image in the boot partition, and
there is no point in having the bootloader images in the /boot
directory of the root filesystem.

Best regards,

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

[Buildroot] [PATCH 2/5] package/rpi-firmware: add option to install firmware files in target/boot/

[Yann E. MORIN]

Thomas, All,

On 2013-11-29 09:27 +0100, Thomas Petazzoni spake thusly:
> Dear Jeremy Rosen,
> 
> On Fri, 29 Nov 2013 09:00:19 +0100 (CET), Jeremy Rosen wrote:
> 
> > Basically, you build the root filesystem the way it will look when all
> > sub-filesystems are mounted, you describe how they are mounted and 
> > the system builds the partitions accordingly.
> 
> I don't necessarily agree here, because this strategy makes the
> confusion between two things:
> 
>  * The contents of the /boot directory in the root filesystem.
> 
>  * The contents of the boot partition needed for some platforms (OMAP,
>    RPi, UEFI-based x86 platforms, etc.)
> 
> Those are two different things, and Yann's proposal merges them. As an
> example, provided your bootloader has ext3 support and you're on an
> OMAP platform, you may want to have something like:
> 
>  * The /boot directory in the root filesystem contains the kernel image
>    (uImage, zImage)
> 
>  * The boot partition contains just the bootloader images (MLO,
>    u-boot.img).
> 
> There is no point to have the kernel image in the boot partition, and
> there is no point in having the bootloader images in the /boot
> directory of the root filesystem.

OK, now I can follow what you mean (although one may argue whether the
kernel is part of those 'boot' files, since it does not make sense to
have it in the root file system).

However, this means that each 'platform' (eg. the RPi, your special
OMAP board...) will each have to provide a manifest of things to install
in the boot partition, and 'genimages' will have to interpret this
manifest to populate the boot partition.

I won't say I like this, even if I see your point. And from a more
global perspective, the boot partition might also be something else than
a filesystem.

So OK, I'll see how to manage this. Here is a quick proposal:

1) genimages should be able to create board-specific partitions, eg.:
   (a) a filesystem mounted on, and filled from /boot
   (b) an unmounted filesystem filled with an arbitrary set of files
   (c) a binary blob

  - For (a), we can already do that, it's like any other filesystem.
  - For (b), the submitted genimages can't do that, but the one in
    buildroot.config can, so I just have to reinstate my 'fs_filler'
    hook.
  - For (c), this has to be implemented. I suggest we postpone this
    until the basic genimages is upstream, and we have an actual
    use-case for it (no need to over-engineer the sutff, YAGNI...)

Of course, there are the cases where we may need any conbinations of one
or more of:
  - a boot blob (eg. MBR, UEFI; obviously only one!)
  - a binary blob partition (eg. a U-Boot image)
  - a mounted /boot filesystem
  - a unmounted filesystem

2) bootloaders for case (a) will have to:
   (a) install their files in $(TARGET_DIR)/boot

genimages should be prepared to generate a partition with a filesystem
filed from the content of /boot, and add an entry for it in fstab.
(Done! :-) )

We currently (as far as I know) have no such bootloader; not even
rpi-firmware fits in this category.

3) bootloaders for case (b) will have to:
   (a) install their files, and a manifest of those files, in a known
       location
   (b) that location should be platform-agnostic, so that genimages will
       find it; $(BINARIES_DIR)/boot-files/ looks like a good candidate.

genimages should be prepared to generate a partition with a filesystem
filled by copying files listed in a manifest file.
(Relatively easy, I think.)

The kind of bootloaders I know that fit in this category are
rpi-firmware and grub. There may have others.

4) bootloaders for case (c) will have to:
   (a) install the binary blob in a known location;
       $(BINARIES_DIR)/boot-files/ looks like a good candidate.

genimages should be prepared to generate a partition by simply dumping
the content of a file.

The bootloaders that fit in this category are U-Boot, RedBoot... Note
that this does not imply an MBR-type of partitioning.

Any comment on the above?

And thanks for the guidance and enlightment! :-)

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/5] package/rpi-firmware: add option to install firmware files in target/boot/

[Arnout Vandecappelle]

On 29/11/13 20:01, Yann E. MORIN wrote:
> 3) bootloaders for case (b) will have to:
>     (a) install their files, and a manifest of those files, in a known
>         location
>     (b) that location should be platform-agnostic, so that genimages will
>         find it; $(BINARIES_DIR)/boot-files/  looks like a good candidate.
>
> genimages should be prepared to generate a partition with a filesystem
> filled by copying files listed in a manifest file.
> (Relatively easy, I think.)

  Yeah, it's just adding a flag that tells genimages that fs_root is 
relative to the images directory instead of the target directory.

  I think it is safe to assume that the bootloaders of this type will put 
their output files in a directory, rather than flat in the images 
directory. So a manifest is not really needed. Although... If you want to 
create a partition with SPL + U-Boot + uImage, you 'll have to specify 
these three explicitly.

  Regards,
  Arnout

>
> The kind of bootloaders I know that fit in this category are
> rpi-firmware and grub. There may have others.


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 2/5] package/rpi-firmware: add option to install firmware files in target/boot/

[Yann E. MORIN]

Arnout, All,

On 2013-12-01 01:59 +0100, Arnout Vandecappelle spake thusly:
> On 29/11/13 20:01, Yann E. MORIN wrote:
> >3) bootloaders for case (b) will have to:
> >    (a) install their files, and a manifest of those files, in a known
> >        location
> >    (b) that location should be platform-agnostic, so that genimages will
> >        find it; $(BINARIES_DIR)/boot-files/  looks like a good candidate.
> >
> >genimages should be prepared to generate a partition with a filesystem
> >filled by copying files listed in a manifest file.
> >(Relatively easy, I think.)
> 
>  Yeah, it's just adding a flag that tells genimages that fs_root is relative
> to the images directory instead of the target directory.

Then, what about those targets that require the kernel to be there, too?

For the Raspberry Pi, we must have those files in the boot partition:
  - the boot loader files: bootcode.bin, start.elf, fixup.dat
  - the bootloader config files: config.txt, cmdline.txt
  - the kernel file: zImage

For Thomas' board, we would not need the kernel file in there.

So, genimages need to be able to generate the partition cntent not only
from files in $(BINARIES_DIR)/boot-files, but also from $(BIANRIES_DIR)
itself.

>  I think it is safe to assume that the bootloaders of this type will put
> their output files in a directory, rather than flat in the images directory.
> So a manifest is not really needed. Although... If you want to create a
> partition with SPL + U-Boot + uImage, you 'll have to specify these three
> explicitly.

Indeed. But we do not want to hard-code anything in genimages, and we
want to keep the partition table layout description as simple as
possible.

I'll see what I can come up with.

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 3/5] package/rpi-firmware: move to bootloaders menu

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

rpi-firmware, although it does contain the GPU firmware, also serves as
the bootloader. As a reminder, here is an overview of how the RPi boots:
  - GPU exits reset
  - GPU loads its firmware from the first, FAT32-formatted partition
  - GPU reads its config file from the same partition
  - GPU loads kernel from the same partition, into RAM
  - GPU de-asserts the reset of the ARM core (CPU)
  - CPU exits reset and starts executing kernel code

So, although the largest part of rpi-firmware is indeed the GPU firmware,
the first purpose it serves is as a bootloader for the ARM core.

People that do not want to use the GPU (eg. headless, no multimedia...)
will still want to select rpi-firmware.

Having rpi-firmware in target packages -> hardware-handling -> firmware
is a bit misleading in this case.

Hence, move rpi-firmware from the target packages submenu, into the
bootloaders submenu.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 boot/Config.in                                 |  1 +
 {package => boot}/rpi-firmware/Config.in       | 24 ++++++++++++------------
 {package => boot}/rpi-firmware/cmdline.txt     |  0
 {package => boot}/rpi-firmware/config.txt      |  0
 {package => boot}/rpi-firmware/rpi-firmware.mk | 10 +++++-----
 configs/raspberrypi_defconfig                  |  2 +-
 package/Config.in                              |  1 -
 7 files changed, 19 insertions(+), 19 deletions(-)
 rename {package => boot}/rpi-firmware/Config.in (71%)
 rename {package => boot}/rpi-firmware/cmdline.txt (100%)
 rename {package => boot}/rpi-firmware/config.txt (100%)
 rename {package => boot}/rpi-firmware/rpi-firmware.mk (60%)

diff --git a/boot/Config.in b/boot/Config.in
index eb5f7cd..2783607 100644
--- a/boot/Config.in
+++ b/boot/Config.in
@@ -8,6 +8,7 @@ source "boot/boot-wrapper-aarch64/Config.in"
 source "boot/grub/Config.in"
 source "boot/lpc32xxcdl/Config.in"
 source "boot/mxs-bootlets/Config.in"
+source "boot/rpi-firmware/Config.in"
 source "boot/syslinux/Config.in"
 source "boot/uboot/Config.in"
 source "boot/xloader/Config.in"
diff --git a/package/rpi-firmware/Config.in b/boot/rpi-firmware/Config.in
similarity index 71%
rename from package/rpi-firmware/Config.in
rename to boot/rpi-firmware/Config.in
index ce5b974..fea292b 100644
--- a/package/rpi-firmware/Config.in
+++ b/boot/rpi-firmware/Config.in
@@ -1,4 +1,4 @@
-config BR2_PACKAGE_RPI_FIRMWARE
+config BR2_TARGET_RPI_FIRMWARE
 	bool "rpi-firmware"
 	depends on BR2_arm
 	help
@@ -7,11 +7,11 @@ config BR2_PACKAGE_RPI_FIRMWARE
 
 	  https://github.com/raspberrypi/firmware
 
-if BR2_PACKAGE_RPI_FIRMWARE
+if BR2_TARGET_RPI_FIRMWARE
 
 choice
 	bool "Firmware to boot"
-	default BR2_PACKAGE_RPI_FIRMWARE_DEFAULT
+	default BR2_TARGET_RPI_FIRMWARE_DEFAULT
 	help
 	  There are three different firmware files:
 	    - the default firmware, that enables standard GPU features;
@@ -20,18 +20,18 @@ choice
 	    - the cut-down firmware, for emergency situations, with only
 	      features required to boot a Linux kernel.
 
-config BR2_PACKAGE_RPI_FIRMWARE_DEFAULT
+config BR2_TARGET_RPI_FIRMWARE_DEFAULT
 	bool "default"
 	help
 	  The default firmware, that enables standard GPU features.
 
-config BR2_PACKAGE_RPI_FIRMWARE_X
+config BR2_TARGET_RPI_FIRMWARE_X
 	bool "extended ('x', more codecs)"
 	help
 	  The extended firmware, that enables additional GPU features
 	  (eg. more audio/video codecs).
 
-config BR2_PACKAGE_RPI_FIRMWARE_CD
+config BR2_TARGET_RPI_FIRMWARE_CD
 	bool "cut-down ('cd', emergency)"
 	help
 	  The cut-down firmware, for emergency situations, with only
@@ -39,13 +39,13 @@ config BR2_PACKAGE_RPI_FIRMWARE_CD
 
 endchoice
 
-config BR2_PACKAGE_RPI_FIRMWARE_BOOT
+config BR2_TARGET_RPI_FIRMWARE_BOOT
 	string
-	default ""      if BR2_PACKAGE_RPI_FIRMWARE_DEFAULT
-	default "_x"    if BR2_PACKAGE_RPI_FIRMWARE_X
-	default "_cd"   if BR2_PACKAGE_RPI_FIRMWARE_CD
+	default ""      if BR2_TARGET_RPI_FIRMWARE_DEFAULT
+	default "_x"    if BR2_TARGET_RPI_FIRMWARE_X
+	default "_cd"   if BR2_TARGET_RPI_FIRMWARE_CD
 
-config BR2_PACKAGE_RPI_FIRMWARE_INSTALL_TARGET
+config BR2_TARGET_RPI_FIRMWARE_INSTALL_TARGET
 	bool "Install bootloader files into /boot"
 	depends on !BR2_TARGET_ROOTFS_INITRAMFS
 	help
@@ -54,4 +54,4 @@ config BR2_PACKAGE_RPI_FIRMWARE_INSTALL_TARGET
 
 	  Say 'n' to have them installed in $(BINARIES_DIR)/rpi-firmware.
 
-endif # BR2_PACKAGE_RPI_FIRMWARE
+endif # BR2_TARGET_RPI_FIRMWARE
diff --git a/package/rpi-firmware/cmdline.txt b/boot/rpi-firmware/cmdline.txt
similarity index 100%
rename from package/rpi-firmware/cmdline.txt
rename to boot/rpi-firmware/cmdline.txt
diff --git a/package/rpi-firmware/config.txt b/boot/rpi-firmware/config.txt
similarity index 100%
rename from package/rpi-firmware/config.txt
rename to boot/rpi-firmware/config.txt
diff --git a/package/rpi-firmware/rpi-firmware.mk b/boot/rpi-firmware/rpi-firmware.mk
similarity index 60%
rename from package/rpi-firmware/rpi-firmware.mk
rename to boot/rpi-firmware/rpi-firmware.mk
index 6adea77..215de95 100644
--- a/package/rpi-firmware/rpi-firmware.mk
+++ b/boot/rpi-firmware/rpi-firmware.mk
@@ -9,7 +9,7 @@ RPI_FIRMWARE_SITE = http://github.com/raspberrypi/firmware/tarball/$(RPI_FIRMWAR
 RPI_FIRMWARE_LICENSE = BSD-3c
 RPI_FIRMWARE_LICENSE_FILES = boot/LICENCE.broadcom
 
-ifeq ($(BR2_PACKAGE_RPI_FIRMWARE_INSTALL_TARGET),y)
+ifeq ($(BR2_TARGET_RPI_FIRMWARE_INSTALL_TARGET),y)
 RPI_FIRMWARE_DEST = $(TARGET_DIR)/boot
 else
 RPI_FIRMWARE_DEST = $(BINARIES_DIR)/rpi-firmware
@@ -17,10 +17,10 @@ endif
 
 define RPI_FIRMWARE_INSTALL_TARGET_CMDS
 	$(INSTALL) -D -m 0644 $(@D)/boot/bootcode.bin $(RPI_FIRMWARE_DEST)/bootcode.bin
-	$(INSTALL) -D -m 0644 $(@D)/boot/start$(BR2_PACKAGE_RPI_FIRMWARE_BOOT).elf $(RPI_FIRMWARE_DEST)/start.elf
-	$(INSTALL) -D -m 0644 $(@D)/boot/fixup$(BR2_PACKAGE_RPI_FIRMWARE_BOOT).dat $(RPI_FIRMWARE_DEST)/fixup.dat
-	$(INSTALL) -D -m 0644 package/rpi-firmware/config.txt $(RPI_FIRMWARE_DEST)/config.txt
-	$(INSTALL) -D -m 0644 package/rpi-firmware/cmdline.txt $(RPI_FIRMWARE_DEST)/cmdline.txt
+	$(INSTALL) -D -m 0644 $(@D)/boot/start$(BR2_TARGET_RPI_FIRMWARE_BOOT).elf $(RPI_FIRMWARE_DEST)/start.elf
+	$(INSTALL) -D -m 0644 $(@D)/boot/fixup$(BR2_TARGET_RPI_FIRMWARE_BOOT).dat $(RPI_FIRMWARE_DEST)/fixup.dat
+	$(INSTALL) -D -m 0644 boot/rpi-firmware/config.txt $(RPI_FIRMWARE_DEST)/config.txt
+	$(INSTALL) -D -m 0644 boot/rpi-firmware/cmdline.txt $(RPI_FIRMWARE_DEST)/cmdline.txt
 endef
 
 $(eval $(generic-package))
diff --git a/configs/raspberrypi_defconfig b/configs/raspberrypi_defconfig
index da877fb..db34717 100644
--- a/configs/raspberrypi_defconfig
+++ b/configs/raspberrypi_defconfig
@@ -6,7 +6,7 @@ BR2_TOOLCHAIN_BUILDROOT_CXX=y
 
 BR2_TARGET_GENERIC_GETTY_PORT="tty1"
 
-BR2_PACKAGE_RPI_FIRMWARE=y
+BR2_TARGET_RPI_FIRMWARE=y
 BR2_PACKAGE_RPI_USERLAND=y
 BR2_PACKAGE_LIBCOFI=y
 
diff --git a/package/Config.in b/package/Config.in
index 311cc6c..f003a57 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -254,7 +254,6 @@ menu "Firmware"
 source "package/am33x-cm3/Config.in"
 source "package/b43-firmware/Config.in"
 source "package/linux-firmware/Config.in"
-source "package/rpi-firmware/Config.in"
 source "package/sunxi-boards/Config.in"
 source "package/ux500-firmware/Config.in"
 source "package/zd1211-firmware/Config.in"
-- 
1.8.1.2

[Buildroot] [PATCH 3/5] package/rpi-firmware: move to bootloaders menu

[Yann E. MORIN]

On 2013-11-22 23:50 +0100, Yann E. MORIN spake thusly:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
[--SNIP--]
> Hence, move rpi-firmware from the target packages submenu, into the
> bootloaders submenu.
[--SNIP--]
> diff --git a/package/rpi-firmware/Config.in b/boot/rpi-firmware/Config.in
> similarity index 71%
> rename from package/rpi-firmware/Config.in
> rename to boot/rpi-firmware/Config.in
> index ce5b974..fea292b 100644
> --- a/package/rpi-firmware/Config.in
> +++ b/boot/rpi-firmware/Config.in
> @@ -1,4 +1,4 @@
> -config BR2_PACKAGE_RPI_FIRMWARE
> +config BR2_TARGET_RPI_FIRMWARE

Doh, I forgot to add this to LEGACY... :-(
I will do before the next round.

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 3/5] package/rpi-firmware: move to bootloaders menu

[Thomas Petazzoni]

Dear Yann E. MORIN,

On Fri, 22 Nov 2013 23:50:56 +0100, Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> rpi-firmware, although it does contain the GPU firmware, also serves as
> the bootloader. As a reminder, here is an overview of how the RPi boots:
>   - GPU exits reset
>   - GPU loads its firmware from the first, FAT32-formatted partition
>   - GPU reads its config file from the same partition
>   - GPU loads kernel from the same partition, into RAM
>   - GPU de-asserts the reset of the ARM core (CPU)
>   - CPU exits reset and starts executing kernel code
> 
> So, although the largest part of rpi-firmware is indeed the GPU firmware,
> the first purpose it serves is as a bootloader for the ARM core.
> 
> People that do not want to use the GPU (eg. headless, no multimedia...)
> will still want to select rpi-firmware.
> 
> Having rpi-firmware in target packages -> hardware-handling -> firmware
> is a bit misleading in this case.
> 
> Hence, move rpi-firmware from the target packages submenu, into the
> bootloaders submenu.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

I must say I am not entirely convinced this change is necessary.
rpi-firmware is a much bootloader stuff than GPU/OpenGL stuff, so
deciding whether it should be in Bootloaders or in Packages -> Hardware
handling is difficult. And since it has now been in Packages ->
Hardware Handling for a long time, and that moving it to the
Bootloaders section involves renaming a number of Config.in options and
therefore adding some Config.in.legacy blurb, I'm not sure it's worth
the effort.

And in any case, if we decided to do this, it should be done *before*
your PATCH 1/5 that adds 4 additional Config.in symbols, which we
wouldn't have to carry in Config.in.legacy, because they are new.

Best regards,

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

[Buildroot] [PATCH 3/5] package/rpi-firmware: move to bootloaders menu

[Yann E. MORIN]

Thomas, All,

On 2013-11-28 21:08 +0100, Thomas Petazzoni spake thusly:
> On Fri, 22 Nov 2013 23:50:56 +0100, Yann E. MORIN wrote:
> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > 
> > rpi-firmware, although it does contain the GPU firmware, also serves as
> > the bootloader. As a reminder, here is an overview of how the RPi boots:
> >   - GPU exits reset
> >   - GPU loads its firmware from the first, FAT32-formatted partition
> >   - GPU reads its config file from the same partition
> >   - GPU loads kernel from the same partition, into RAM
> >   - GPU de-asserts the reset of the ARM core (CPU)
> >   - CPU exits reset and starts executing kernel code
> > 
> > So, although the largest part of rpi-firmware is indeed the GPU firmware,
> > the first purpose it serves is as a bootloader for the ARM core.
> > 
> > People that do not want to use the GPU (eg. headless, no multimedia...)
> > will still want to select rpi-firmware.
> > 
> > Having rpi-firmware in target packages -> hardware-handling -> firmware
> > is a bit misleading in this case.
> > 
> > Hence, move rpi-firmware from the target packages submenu, into the
> > bootloaders submenu.
> > 
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> I must say I am not entirely convinced this change is necessary.

It is not _necessary_. I find it more coherent with the upstream docs
(eg. all docs about the Raspberry Pi you'll find on the Raspberry Pi
foundation's forums, and on the eLinux WiKi for the RPi) about the boot
process:

http://www.elinux.org/RPi_Software#Overview
    Here, the "GPU firmware" is only refered to as par t of the boot
    process. There is references to:
      - the "first stage bootloader" in ROM,
      - the "second stage bootloader" on the SDcard
      - the "GPU firmware"

http://www.elinux.org/RPi_Software#GPU_bootloaders
    This section calls the "GPU firmware" the "GPU bootloader".

http://www.elinux.org/RPi_Easy_SD_Card_Setup#SD_card_setup
    "The Raspberry Pi will not start without a properly formatted SD
    Card, containing the bootloader and a suitable operating system."

For what it's worth, I have seen a few people being _very_ puzzled by
that situation. I told them the RPi bootloader was packaged in
Buildroot, but they just kept telling me they could not find it, since
they were looking in the "bootloaders" menu. When I showed them where it
was, they really wondered why we did put it there, and not with the
other bootloaders.

People using the RPi do not care whether rpi-firmware provides the 3D
and video stuff. They only care that it is mainly called a bootloader
in the docs, and they look for a bootloader.

> rpi-firmware is a much bootloader stuff than GPU/OpenGL stuff, so
> deciding whether it should be in Bootloaders or in Packages -> Hardware
> handling is difficult. And since it has now been in Packages ->
> Hardware Handling for a long time, and that moving it to the
> Bootloaders section involves renaming a number of Config.in options and
> therefore adding some Config.in.legacy blurb, I'm not sure it's worth
> the effort.

That is a completely different reason! :-)

> And in any case, if we decided to do this, it should be done *before*
> your PATCH 1/5 that adds 4 additional Config.in symbols, which we
> wouldn't have to carry in Config.in.legacy, because they are new.

Agreed.

But I won't be a dick with that one. I can well live with it. ;-)

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 3/5] package/rpi-firmware: move to bootloaders menu

[Arnout Vandecappelle]

On 28/11/13 22:16, Yann E. MORIN wrote:
> Thomas, All,
>
> On 2013-11-28 21:08 +0100, Thomas Petazzoni spake thusly:
>> >On Fri, 22 Nov 2013 23:50:56 +0100, Yann E. MORIN wrote:
>>> > >From: "Yann E. MORIN"<yann.morin.1998@free.fr>
>>> > >
>>> > >rpi-firmware, although it does contain the GPU firmware, also serves as
>>> > >the bootloader. As a reminder, here is an overview of how the RPi boots:
>>> > >   - GPU exits reset
>>> > >   - GPU loads its firmware from the first, FAT32-formatted partition
>>> > >   - GPU reads its config file from the same partition
>>> > >   - GPU loads kernel from the same partition, into RAM
>>> > >   - GPU de-asserts the reset of the ARM core (CPU)
>>> > >   - CPU exits reset and starts executing kernel code
>>> > >
>>> > >So, although the largest part of rpi-firmware is indeed the GPU firmware,
>>> > >the first purpose it serves is as a bootloader for the ARM core.
>>> > >
>>> > >People that do not want to use the GPU (eg. headless, no multimedia...)
>>> > >will still want to select rpi-firmware.
>>> > >
>>> > >Having rpi-firmware in target packages -> hardware-handling -> firmware
>>> > >is a bit misleading in this case.
>>> > >
>>> > >Hence, move rpi-firmware from the target packages submenu, into the
>>> > >bootloaders submenu.
>>> > >
>>> > >Signed-off-by: "Yann E. MORIN"<yann.morin.1998@free.fr>
>> >
>> >I must say I am not entirely convinced this change is necessary.
> It is not_necessary_. I find it more coherent with the upstream docs
[snip]

  We can have the best of both worlds: just move the source line from 
package/Config.in to boot/Config.in, and no dicking about with changing 
config names...

  Calling the stuff in boot/ BR2_TARGET instead of BR2_PACKAGE is IMHO 
just creating confusion. Even having the boot/ directory at all is 
pointless (i.e. historical accident) if you ask me.

  So I would be more convinced to move everything from boot/ to packages/ 
than the other way round :-)

  Regards,
  Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 4/5] fs/custom: generate complete, partition-based device images

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

Contrary to the existing fs/ schemes, which each generate only a single
filesystem image for the root filesystem, this new scheme allows the
user to generate more complex images.

The basis behind this is a .ini-like description of the layout of the
final target storage:
  - the list of device(s)
  - per-device, the list of partition(s)
  - per-partition, the content

For now, the only content possible for partitions is a filesystem. It
should be pretty easy to add new types (eg. un-formated, or raw blob).

Also, only two filesystems are supported: ext{2,3,4} and vfat. Adding
more will be relatively easy, provided we have the necessary host
packages to deal with those filesystems.

The existing Buildroot filesystem generators are re-used as much as
possible when it makes sense; when it does not (eg. for vfat), a specific
generator is used.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 docs/manual/appendix.txt              |   1 +
 docs/manual/customize-filesystems.txt |  35 +++++
 docs/manual/customize.txt             |   2 +
 docs/manual/partition-layout.txt      | 258 ++++++++++++++++++++++++++++++++++
 fs/Config.in                          |   1 +
 fs/custom/Config.in                   |  18 +++
 fs/custom/boot/mbr                    |  57 ++++++++
 fs/custom/boot/pre-post               |   8 ++
 fs/custom/custom.mk                   |  14 ++
 fs/custom/fs/ext                      |  22 +++
 fs/custom/fs/pre-post                 |  40 ++++++
 fs/custom/fs/vfat                     |  17 +++
 fs/custom/functions                   |  47 +++++++
 fs/custom/genimages                   | 214 ++++++++++++++++++++++++++++
 14 files changed, 734 insertions(+)
 create mode 100644 docs/manual/customize-filesystems.txt
 create mode 100644 docs/manual/partition-layout.txt
 create mode 100644 fs/custom/Config.in
 create mode 100644 fs/custom/boot/mbr
 create mode 100644 fs/custom/boot/pre-post
 create mode 100644 fs/custom/custom.mk
 create mode 100644 fs/custom/fs/ext
 create mode 100644 fs/custom/fs/pre-post
 create mode 100644 fs/custom/fs/vfat
 create mode 100644 fs/custom/functions
 create mode 100755 fs/custom/genimages

diff --git a/docs/manual/appendix.txt b/docs/manual/appendix.txt
index 74ee8fd..53f4205 100644
--- a/docs/manual/appendix.txt
+++ b/docs/manual/appendix.txt
@@ -6,6 +6,7 @@ Appendix
 
 include::makedev-syntax.txt[]
 include::makeusers-syntax.txt[]
+include::partition-layout.txt[]
 
 
 // Automatically generated lists:
diff --git a/docs/manual/customize-filesystems.txt b/docs/manual/customize-filesystems.txt
new file mode 100644
index 0000000..cddee42
--- /dev/null
+++ b/docs/manual/customize-filesystems.txt
@@ -0,0 +1,35 @@
+// -*- mode:doc; -*-
+// vim: set syntax=asciidoc:
+
+[[filesystem-custom]]
+Customizing the generated filesystem images
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
++Buildroot+ knows by default how to generate a few different kind of
+filesystems, such as +squashfs+, +ext2/3/4+, +cramfs+... But those
+filesystems are all generated to contain the complete target directory
+hierarchy in a single filesystem, mounted as the root filesystem +/+.
+That is, even if you select both an +ext2+ and a +squashfs+ filesystems,
+the content of the two generated images will be the exact same, only the
+types of the filesystems will be different.
+
+Most devices require a more complex setup, with different parts of the
+directory structure split across different filesystems, each stored on
+different partitions of one or more storage devices.
+
++Buildroot+ can generate such complex setups, using a +partition table layout
+description+. This is a simple text file, not unlike the +.ini+ style of
+configuration files, that describes how the target directory hierarchy has
+to be split across the target storage devices. It is a bit like a flattened
+tree of the storage layout.
+
+Set the variable +BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE+ to the path of
+the file containing your +partition table layout description+.
+
+See xref:part-layout-desc-syntax[] for the complete documentation of the
++partition table layout description+ syntax.
+
+[underline]*Note:* Although more versatile than the single filesystem image
+mechanism, the +partition table layout description+ might be unable to
+describe very complex setups. For example, it is not capable of handling
++initramfs+ based systems, or NFS-mounted filesystems.
diff --git a/docs/manual/customize.txt b/docs/manual/customize.txt
index 0456ef1..83cfaa0 100644
--- a/docs/manual/customize.txt
+++ b/docs/manual/customize.txt
@@ -14,6 +14,8 @@ include::customize-kernel-config.txt[]
 
 include::customize-toolchain.txt[]
 
+include::customize-filesystems.txt[]
+
 include::customize-store.txt[]
 
 include::customize-packages.txt[]
diff --git a/docs/manual/partition-layout.txt b/docs/manual/partition-layout.txt
new file mode 100644
index 0000000..5d99e15
--- /dev/null
+++ b/docs/manual/partition-layout.txt
@@ -0,0 +1,258 @@
+// -*- mode:doc; -*-
+// vim: set syntax=asciidoc:
+
+[[part-layout-desc-syntax]]
+
+Partition table layout description syntax
+-----------------------------------------
+
+The +partition table layout description+ syntax is not unlike the standard
+https://en.wikipedia.org/wiki/.ini[+.ini+] syntax. There are two types of
+entries: +sections+, that may contain zero or more +properties+.
+
++Sections+ are specified between square brackets +[]+, _eg._: +[name]+.
+
++Properties+ are specified as key-value pairs, _eg._: +key=value+, and
+are documented as:
+
+* +key-name+ (optional or mandatory): description
+** +value1+: description
+** +value2+: description
+** ...
+
+[underline]*Note:* Unlike the standard +.ini+ syntax, the +partition table
+layout description+ _is_ case-sensitive.
+
+The order of +sections+ is irrelevant. However, for readability, we recomend
+the +partition table layout description+ starts with the +global+ section.
+
+The global section
+~~~~~~~~~~~~~~~~~~
+
+The +[global]+ section defines some global settings, and the list of devices.
+
+Properties for the global section
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+* +extract+ (mandatory): the type of base image to extract
+** +tar+: extract the +rootfs.tar+ base image generated by +Buildroot+
+
+* +devices+ (mandatory): the comma-separated list of storage devices to
+  use on the device. Each device is the filename of the device node
+  present in +/dev+
+
+The devices and partitions sections
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The devices and partitions sections define, for each device or partition,
+the content of that device or partition.
+
+For each device listed in the +[global]+ section, there must be a
+corresponding section named after that device.
+
+For each partition listed in a device section, there must be a corresponding
+section named after that partition.
+
+Properties for the device section
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+* +type+ (mandatory): the type of content for that device or partition
+** +boot+: the device contains one or more partitions, and
+   _may_ serve as a boot device
+** +fs+: the device contains a filesystem
+
+Properties for the partition section
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+* +type+ (mandatory): the type of content for that device or partition
+** +fs+: the device contains a filesystem
+
+* +size+: the size of that partition, in bytes
+
+Properties for +type=boot+
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+* +partitions+ (mandatory): the comma-separated list of partition(s) on
+  this device; no two partitions may have the same name, even if they
+  reside on different devices; partitions names shall match this regexp:
+  `^[[:alpha:]][[:alnum:]-_]*$` (_ie._ starts with a letter, followed by
+  zero or more alpha-numeric character or a dash or an underscore)
+
+* +partalign+ (optional): the alignment of partitions, in bytes; defaults
+  to an alignment of one, which means no alignment
+
+* +boot_type+ (mandatory): the partitioning scheme to use on this device
+** +mbr+: use an https://en.wikipedia.org/wiki/Master_boot_record[MBR]
+   partitioning scheme
+
+Properties for +boot_type=mbr+
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+* +mbr_bootcode+ (optional): the bootcode to use, as a path to the file
+  containing the bootcode image, relative tto the +$(BINARIES_DIR)+
+  directory; defaults to no bootcode (eg. filled with +0+s)
+
+Properties for partitions whose containing device is +boot_type=mbr+
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+* +mbr_type+ (mandatory): the partition
+  https://en.wikipedia.org/wiki/Partition_type#List_of_partition_IDs[type]
+
+Properties for +type=fs+
+^^^^^^^^^^^^^^^^^^^^^^^^
+
+* +fs_type+ (mandatory): the type of filesystem to generate
+** +ext+: generate an extended filesystem (ext2, ext3, ext4)
+** +vfat+: generate an VFAT filesystem (FAT16, FAT32)
+
+* +fs_root+ (mandatory): the directory, relative to +$(TARGET_DIR)+, from
+  which to fill the filesystem, and consequently the mountpoint of that
+  filesystem
+
+* +fs_vfstype+ (optional): the type of filesystem to use when calling
+  +mount+, if different from +fs_type+
+
+* +fs_mntopts+ (optional): the mount options; defaults to +defaults+
+
+* +fs_label+ (optional): the label to assign to this filesystem, if that
+  filesystem supports a label
+
+Properties for +fs_type=ext+
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+* +ext_gen+ (mandatory): the generation of extended filesystem to generate
+** +2+, +3+, +4+: for an ext2, ext3 or ext4 filesystem
+
+* +ext_rev+ (mandatory): the revision of the extended filesystem
+** +0+ (ext2 only): generate a revision 0 extended filesystem filesystem
+** +1+ (mandatory for ext3 or ext4): generate a revision 1 extended
+   filesystem
+
+Properties for +fs_type=vfat+
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+* +vfat_size+ (optional): the VFAT-size of the filesystem
+** +12+, +16+, +32+: generate a FAT12, FAT16, or FAT32
+
+Generation of the filesystems
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The filesystems are generated in an order such that a filesystem that is
+mounted as a sub-directory of another filesystem is generated first.
+
+A filesystem is filled with the content of the directory corresponding to
+its mountpoint, and then that directory is emptied before continuing to the
+next filesystem. That way, the mountpoints are empty before the filesystem
+that contains them are generated.
+
+Finally, an entry in added in +/etc/fstab+ for each generated filesystem, so
+they are mounted at boot time.
+
+Requirements on host packages
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Depending on what types of partitioning and filesystems are used by your
++partition table layout description+, you may have to enable some
+host-packages in the +Host utilities+ sub-menu.
+
+Since the content of a +partition table layout description+ is very
+specific to a board and/or a project, there is no way for Buildroot to
+automatically select those host packages, and thus it is your
+responsibility to select the appropriate ones.
+
+For example, for an MBR partitioning, you will have to enable the +host
+genpart+ package. For FAT filesystems, you will have to enable both of
++host dosfstools+ and +host mtools+.
+
+Examples
+~~~~~~~~
+
+.Simplest partition table layout description
+====
+----
+[global]
+extract=tar
+devices=sda
+
+[sda]
+type=fs
+fs_type=ext
+fs_vfstype=ext4
+fs_root=/
+ext_gen=4
+ext_rev=1
+----
+
+The +partition table layout description+ above defines a single device
++sda+. That device contains an ext4 filesystem, which is filled with the
+whole content of the +rootfs.tar+, and is mounted on +/+.
+====
+
+.More copmplex table layout description
+====
+----
+[global]
+extract=tar
+devices=mmcblk0,sda
+
+[mmcblk0]
+type=boot
+boot_type=mbr
+partitions=boot,root
+partalign=$((1024*1024))
+
+[sda]
+type=boot
+boot_type=mbr
+partitions=data
+partalign=4096
+
+[boot]
+type=fs
+mbr_type=$((0xC))
+size=$((16*1048576))
+fs_type=vfat
+fs_mntopts=ro
+fs_label=BOOT
+fs_root=/boot
+vfat_size=32
+
+[root]
+type=fs
+mbr_type=$((0x83))
+size=268435456
+fs_type=ext
+fs_vfstype=ext4
+fs_mntopts=discard,delalloc
+fs_root=/
+fs_label=ROOT
+ext_gen=4
+ext_rev=1
+
+[data]
+type=fs
+mbr_type=$((0x83))
+size=$((4*1024*1048576))
+fs_type=ext
+fs_vfstype=ext2
+fs_root=/data
+fs_label=DATA
+ext_gen=2
+ext_rev=1
+----
+====
+
+The example above defines two devices, +mmcblk0+ and +sda+.
+
+The +mmcblk0+ device contains two partitions, +boot+ and +root+; partitions
+are aligned on a 1MiB boundary. The +sda+ device contains a single partition,
++data+, aligned on a 4KiB boundary.
+
+The +boot+ partition is a 16MiB FAT32 filesystem filled with the content
+of, and mounted on, +/boot+, and with label +BOOT+.
+
+The +data+ partition is a 4GiB ext2r1 filesystem filled with the content
+of, and mounted on, +/data+, and with label +DATA+.
+
+The +root+ partition is a 256MiB ext4 filesystem filled the the rest of,
+and mounted on, +/+, and with label +ROOT+.
diff --git a/fs/Config.in b/fs/Config.in
index da4c5ff..8721220 100644
--- a/fs/Config.in
+++ b/fs/Config.in
@@ -11,5 +11,6 @@ source "fs/romfs/Config.in"
 source "fs/squashfs/Config.in"
 source "fs/tar/Config.in"
 source "fs/ubifs/Config.in"
+source "fs/custom/Config.in"
 
 endmenu
diff --git a/fs/custom/Config.in b/fs/custom/Config.in
new file mode 100644
index 0000000..fabb878
--- /dev/null
+++ b/fs/custom/Config.in
@@ -0,0 +1,18 @@
+config BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE
+	string "partition table"
+	help
+	  Enter the path to a partition-table for your device.
+
+	  This will allow Buildroot to generate a more complex target
+	  image, which may consist of more than one filesystem on more
+	  than one partition.
+
+	  See docs/manual/bla-bla on how to construct such a partition
+	  table.
+
+# For the fs infrasturcture, we need that option to be set.
+# Additionally, it allows us to force the TAR output.
+config BR2_TARGET_ROOTFS_CUSTOM
+	def_bool y
+	depends on BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE != ""
+	select BR2_TARGET_ROOTFS_TAR
diff --git a/fs/custom/boot/mbr b/fs/custom/boot/mbr
new file mode 100644
index 0000000..0f0ab39
--- /dev/null
+++ b/fs/custom/boot/mbr
@@ -0,0 +1,57 @@
+# Build a complete MBR-based image
+
+do_image() {
+    # ${1} is fs_root, irrelevant here
+    local img="${2}"
+    local i begin part part_img size type _begin _size
+    local -a part_offset part_file
+
+    # Fill-in the boot record with zeroes
+    # Ideally, we should dump the bootloader code here, but since
+    # we don't have any so far, that will have to be done in a
+    # later step.
+    debug "adding (fake) bootcode\n"
+    dd if=/dev/zero of="${img}" bs=$((0x1be)) count=0 seek=1 2>/dev/null
+
+    # Generate partition entries
+    i=0
+    begin=${partalign}
+    debug "adding partitions descriptors\n"
+    for part in ${partitions[${dev}]//,/ }; do
+        debug "  %s\n" "${part}"
+        part_offset+=( ${begin} )
+        part_img="${tmp_dir}/${dev}.${part}.img"
+        part_file+=( "${part_img}" )
+        size=$( align_val $( stat -c '%s' "${part_img}" ) 512 )
+        type="${values["${part}:mbr_type"]}"
+        # LBA is a exressed in a number of 512-byte bocks
+        # and genparts only deals with LBA
+        _begin=$((begin/512))   # begin is already 512-byte aligned
+        _size=$((size/512))     # size is already 512-byte aligned
+        debug "    start=%s (LBA %s)\n" "${begin}" "${_begin}"
+        debug "    size =%s (LBA %s)\n" "${size}"  "${_size}"
+        debug "    type =%s\n"          "${type}"
+        genpart -b ${_begin} -s ${_size} -t ${type} >>"${img}"
+        begin=$( align_val $((begin+size)) ${partalign} )
+        i=$((i+1))
+    done
+    nb_parts=${i}
+    # Generate entries for empty partitions
+    for(( ; i<4; i++ )); do
+        debug "  (empty)\n"
+        genpart -t 0 >>"${img}"
+    done
+    # Dump the boot signature
+    printf "\x55\xaa" >>"${img}"
+
+    for(( i=0; i<nb_parts; i++ )); do
+        part_img="${part_file[${i}]}"
+        offset=${part_offset[${i}]}
+        _offset=$(( offset/512 ))  # offset is already 512-byte aligned
+        dd if="${part_img}" of="${img}" \
+           bs=512 seek=${_offset}       \
+           conv=notrunc,sparse          2>/dev/null
+    done
+}
+
+# vim: ft=sh
diff --git a/fs/custom/boot/pre-post b/fs/custom/boot/pre-post
new file mode 100644
index 0000000..507d17e
--- /dev/null
+++ b/fs/custom/boot/pre-post
@@ -0,0 +1,8 @@
+do_image_pre() {
+    :
+}
+do_image_post() {
+    :
+}
+
+#vim: set ft=sh
diff --git a/fs/custom/custom.mk b/fs/custom/custom.mk
new file mode 100644
index 0000000..940a32a
--- /dev/null
+++ b/fs/custom/custom.mk
@@ -0,0 +1,14 @@
+################################################################################
+#
+# custom partitioning
+#
+################################################################################
+
+define ROOTFS_CUSTOM_CMD
+ BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \"$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE)\"
+endef
+
+# We need the tarball to be generated first
+ROOTFS_CUSTOM_DEPENDENCIES += rootfs-tar
+
+$(eval $(call ROOTFS_TARGET,custom))
diff --git a/fs/custom/fs/ext b/fs/custom/fs/ext
new file mode 100644
index 0000000..5f9b4e7
--- /dev/null
+++ b/fs/custom/fs/ext
@@ -0,0 +1,22 @@
+# Create an extended file system
+
+do_image() {
+    local root_dir="${1}"
+    local img="${2}"
+    local -a fs_opts
+    local gen rev
+
+    fs_opts+=( -z )
+    fs_opts+=( -d "${root_dir}" )
+    [ -z "${size}"    ] || fs_opts+=( -b $((size/1024)) )
+    [ -n "${ext_gen}" ] || ext_gen=2
+    [ -n "${ext_rev}" ] || ext_rev=1
+
+    # Remember, we're running from Buildroot's TOP_DIR
+    GEN=${ext_gen} REV=${ext_rev}                   \
+    ./fs/ext2/genext2fs.sh "${fs_opts[@]}" "${img}" >/dev/null
+
+    [ -z "${fs_label}" ] || tune2fs -L "${fs_label}" "${img}" >/dev/null
+}
+
+# vim: ft=sh
diff --git a/fs/custom/fs/pre-post b/fs/custom/fs/pre-post
new file mode 100644
index 0000000..9563bec
--- /dev/null
+++ b/fs/custom/fs/pre-post
@@ -0,0 +1,40 @@
+#-----------------------------------------------------------------------------
+do_image_pre() {
+    :
+}
+
+#-----------------------------------------------------------------------------
+do_image_post() {
+    local rootfs_dir="${1}"
+    local fs_root="${2}"
+    local img_file="${3}"
+    local part="${4}"
+    local dev mntops vfstype fs_root_esc
+
+    subname+="[post-image]"
+
+    # Empty the partition's mountpoint
+    find "${fs_root_dir}" -maxdepth 1 \! -path "${fs_root_dir}" -exec rm -rf {} +
+
+    # Add entry in fstab, but not if this is '/'
+    # Don't add either if rootfs was not extracted
+    if [    "${fs_root}" = "/"               \
+         -o -z "${values["global:extract"]}" ]; then
+        return 0
+    fi
+    fs_root_esc="$( sed -r -e 's:/:\\/:g;' <<<"${fs_root}" )"
+    sed -r -i -e "/[^[:space:]]+[[:space:]]+${fs_root_esc}[[:space:]]/d"    \
+                 "${rootfs_dir}/etc/fstab"
+    dev="$( get_part_dev_node "${part}" )"
+    vfstype="${fs_vfstype:-${fs_type}}"
+    mntops="${fs_mntops:-defaults}"
+    printf "/dev/%s %s %s %s 0 0\n"     \
+           "${dev}" "${fs_root}"        \
+           "${vfstype}" "${mntops}"     \
+           >>"${rootfs_dir}/etc/fstab"
+
+    subname="${subname%\[post-image\]}"
+}
+
+#-----------------------------------------------------------------------------
+# vim: ft=sh
diff --git a/fs/custom/fs/vfat b/fs/custom/fs/vfat
new file mode 100644
index 0000000..aa5545f
--- /dev/null
+++ b/fs/custom/fs/vfat
@@ -0,0 +1,17 @@
+# Create a VFAT file system
+
+do_image() {
+    local root_dir="${1}"
+    local img="${2}"
+    local -a fs_opts
+
+    dd if=/dev/zero of="${img}" bs=${size} count=0 seek=1 2>/dev/null
+
+    [ -z "${vfat_size}" ] || fs_opts+=( -F ${vfat_size} )
+    [ -z "${fs_label}"  ] || fs_opts+=( -n "${fs_label}" )
+    mkfs.vfat "${fs_opts[@]}" "${img}" >/dev/null
+
+    mcopy -i "${img}" "${root_dir}/"* '::'
+}
+
+# vim: ft=sh
diff --git a/fs/custom/functions b/fs/custom/functions
new file mode 100644
index 0000000..4b41021
--- /dev/null
+++ b/fs/custom/functions
@@ -0,0 +1,47 @@
+# Common functions
+
+#------------------------------------------------------------------------------
+align_val() {
+    local val="${1}"
+    local align="${2}"
+    local aligned
+
+    aligned=$(( ( (val+align-1) / align ) * align ))
+
+    printf "%d" ${aligned}
+}
+
+#------------------------------------------------------------------------------
+# Some trace functions
+trace() {
+    local fmt="${1}"
+    shift
+
+    printf "%s" "${myname}"
+    if [ -n "${subname}" ]; then
+        printf "(%s)" "${subname}"
+    fi
+    printf ": ${fmt}" "${@}"
+}
+
+debug() { :; }
+if [ -n "${DEBUG}" ]; then
+    debug() {
+        trace "${@}" >&2
+    }
+fi
+
+error() {
+    trace "${@}" >&2
+    exit 1
+}
+
+on_error() {
+    local ret=${?}
+
+    error "unexpected error caught: %d\n" ${ret}
+}
+trap on_error ERR
+set -E -e
+
+# vim: ft=sh
diff --git a/fs/custom/genimages b/fs/custom/genimages
new file mode 100755
index 0000000..aba3021
--- /dev/null
+++ b/fs/custom/genimages
@@ -0,0 +1,214 @@
+#!/bin/bash
+set -e
+set -E
+
+#-----------------------------------------------------------------------------
+main() {
+    local part_table="${1}"
+    local tmp_dir
+    local rootfs_dir
+    local -a devices
+    local extract
+    local cur_section
+    local -a sections devices partitions
+    local -A variables values partdevs
+    local sec dev part var val
+    local secs devs parts vars vals
+
+    if [ ! -f "${part_table}" ]; then
+        error "%s: no such file\n" "${part_table}"
+        exit 1
+    fi
+
+    export PATH="${HOST_DIR}/usr/bin:${HOST_DIR}/usr/sbin:${PATH}"
+
+    tmp_dir="$( mktemp -d --tmpdir rpi-genimage.XXXXXX )"
+
+    rootfs_dir="${tmp_dir}/rootfs"
+    mkdir -p "${rootfs_dir}"
+
+    # Parse all the sections in one go, we'll sort
+    # all the mess afterwards...
+    debug "parsing partitions descriptions file '%s'\n" \
+          "${part_table}"
+    while read line; do
+        line="$( sed -r -e 's/[[:space:]]*#.*$//; //d;' <<<"${line}" )"
+
+        # Detect start of global section, skip anything else
+        case "${line}" in
+        "") continue;;
+        '['*']')
+            cur_section="$( sed -r -e 's/[][]//g;' <<<"${line}" )"
+            debug "  entering section '%s'\n" "${cur_section}"
+            sections+=( "${cur_section}" )
+            continue
+        ;;
+        ?*=*)   ;;
+        *)      error "malformed entry '%s'\n" "${line}";;
+        esac
+
+        var="${line%%=*}"
+        eval val="${line#*=}"
+        debug "    adding '%s'='%s'\n" "${var}" "${val}"
+        variables+=( ["${cur_section}"]=",${var}" )
+        values+=( ["${cur_section}:${var}"]="${val}" )
+    done <"${part_table}"
+
+    # Create lists of devices, partitions, and partition:device pairs.
+    debug "creating intermeditate lists\n"
+    devices=( ${values["global:devices"]//,/ } )
+    for dev in "${devices[@]}"; do
+        partitions=( ${values["${dev}:partitions"]//,/ } )
+    done
+    for dev in "${devices[@]}"; do
+        for part in ${values["${dev}:partitions"]//,/ }; do
+            partdevs+=( ["${part}"]="${dev}" )
+        done
+    done
+
+    # Now, we must order the partitions so that their mountpoint
+    # is empty by the time we build the upper-level partition.
+    # For example, given this layout of mountpoints:
+    #   /
+    #   /usr
+    #   /usr/var
+    # We must ensure /usr/var is empty@the time we create the /usr
+    # filesystem image; and similarly, we must ensure /usr is empty by
+    # the time we create the / filesystem image
+    # So, a simple reverse alphabetical sort will do the trick
+    debug "sorting partitions\n"
+    sorted_parts=( $(
+        for part in "${partitions[@]}"; do
+            printf "%s:%s\n" "${part}" "${values["${part}:fs_root"]}"
+        done                    \
+        |sort -t: -k2 -r        \
+        |sed -r -e 's/:[^:]+$//;'
+    ) )
+
+    trace "extracting root (if needed)\n"
+    case "${values["global:extract"]}" in
+        targz)  c=z ;;&
+        tarbz2) c=j ;;&
+        tarxz)  c=J ;;&
+        tar)    c=  ;;&
+        tar*)   tar x${c}f "${BINARIES_DIR}/rootfs.tar" -C "${rootfs_dir}";;
+        "")     ;;
+        *)      error "unknown extract method '%s'\n" "${extract}";;
+    esac
+
+    # Render all partition images
+    for part in "${sorted_parts[@]}"; do
+        trace "preparing filesystem for partition '%s'\n" "${part}"
+        render_img "${rootfs_dir}" "${part}"                        \
+                   "${tmp_dir}/${partdevs["${part}"]}.${part}.img"
+    done
+
+    # Aggregate all devices images
+    for dev in "${devices[@]}"; do
+        trace "assembling partitions in device '%s'\n" "${dev}"
+        render_img "${rootfs_dir}" "${dev}" "${tmp_dir}/${dev}.img"
+    done
+
+    # Copy all partitions and devices images to the image dir
+    for part in "${sorted_parts[@]}"; do
+        debug "copying partition '%s' to image dir\n" "${part}"
+        dd if="${tmp_dir}/${partdevs["${part}"]}.${part}.img"           \
+           of="${BINARIES_DIR}/$( get_part_dev_node "${part}" ).img"    \
+           bs=4096 conv=sparse                                          2>/dev/null
+    done
+    for dev in "${devices[@]}"; do
+        debug "copying device '%s' to image dir\n" "${dev}"
+        dd if="${tmp_dir}/${dev}.img"       \
+           of="${BINARIES_DIR}/${dev}.img"  \
+           bs=4096 conv=sparse              2>/dev/null
+    done
+
+    [ -n "${DEBUG}" ] || rm -rf "${tmp_dir}"
+}
+
+#-----------------------------------------------------------------------------
+render_img() {
+    local rootfs_dir="${1}"
+    local img="${2}"
+    local img_file="${3}"
+    local type sub_type fs_root_dir
+
+    type="${values["${img}:type"]}"
+    sub_type="${values["${img}:${type}_type"]}"
+
+    # Sanity checks
+    [ -n "${type}" ] || error "'%s': unspecified type\n" "${img}"
+    if [ ! -d "fs/custom/${type}" ]; then
+        error "'%s': unsupported type '%s'\n" "${img}" "${type}"
+    fi
+    [ -n "${sub_type}" ] || error "'%s': unspecified %s_type\n" "${img}" "${type}"
+    if [ ! -f "fs/custom/${type}/${sub_type}" ]; then
+        error "'%s': unknown %s_type '%s'\n" "${img}" "${type}" "${sub_type}"
+    fi
+
+    # Need to call the renderer in a subshell so that its definitions
+    # do not pollute our environment
+    subname="${sub_type}"
+    (
+        trap 'exit $?' ERR
+        for var in ${variables["${img}"]//,/ }; do
+            eval "${var}=\"${values["${img}:${var}"]}\""
+        done
+        fs_root_dir="${rootfs_dir}${fs_root}"
+        . "fs/custom/${type}/pre-post"
+        . "fs/custom/${type}/${sub_type}"
+        do_image_pre "${rootfs_dir}" "${fs_root}" "${img_file}" "${img}"
+        do_image "${fs_root_dir}" "${img_file}"
+        do_image_post "${rootfs_dir}" "${fs_root}" "${img_file}" "${img}"
+    )
+    ret=${?}
+    [ ${ret} -eq 0 ] || exit ${ret}
+    subname=""
+}
+
+#-----------------------------------------------------------------------------
+get_part_dev_node() {
+    local part="${1}"
+    local dev
+    local i c p
+
+    dev="${devices["${part}"]}"
+    i="${values["${dev}:partstart"]:-1}"
+
+    # If device node ends with a number, partitions are denoted
+    # with a 'p' before the partition number, eg.:
+    #   /dev/mmcblk0    --> /dev/mmcblk0p1
+    #   /dev/sda        --> /dev/sda1
+    case "${dev#${dev%?}}" in
+        [0-9])  c="p";;
+        *)      c="";;
+    esac
+
+    for p in ${values["${dev}:partitions"]//,/ }; do
+        if [ "${p}" = "${part}" ]; then
+            printf "%s%s%d" "${dev}" "${c}" ${i}
+            return 0
+        fi
+        i=$((i+1))
+    done
+
+    error "'%s': partition not found. WTF?\n" "${part}"
+}
+
+#-----------------------------------------------------------------------------
+myname="${0##*/}"
+mydir="${0%/*}"
+
+TOP_DIR="$( pwd )"
+export myname mydir TOP_DIR
+
+. "fs/custom/functions"
+
+# This script can deal with extracting the rootfs tarball, but we need to
+# be root for that.
+if [ $(id -u) -ne 0 ]; then
+    printf "error: not root\n"
+    exit 1
+else
+    main "${@}"
+fi
-- 
1.8.1.2

[Buildroot] [PATCH 4/5] fs/custom: generate complete, partition-based device images

[Yann E. MORIN]

All,

On 2013-11-22 23:50 +0100, Yann E. MORIN spake thusly:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Contrary to the existing fs/ schemes, which each generate only a single
> filesystem image for the root filesystem, this new scheme allows the
> user to generate more complex images.

I've put up a pre-prendered version of the manual on-line to make it
easier to review:
    http://ymorin.is-a-geek.org/download/tmp/buildroot/manual/manual.html

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 4/5] fs/custom: generate complete, partition-based device images

[Arnout Vandecappelle]

  Hi Yann,

  This patch is obviously too large to be reviewed in a single shot, so 
here are some detailed comments on certain parts of it.

  I don't think there's a way to split the patch up to make review 
easier, unfortunately. But anyway, the functionality is completely 
isolated from the rest so it doesn't hurt to commit it as is and fix up 
later if necessary.


On 22/11/13 23:50, Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> Contrary to the existing fs/ schemes, which each generate only a single
> filesystem image for the root filesystem, this new scheme allows the
> user to generate more complex images.
>
> The basis behind this is a .ini-like description of the layout of the
> final target storage:
>    - the list of device(s)
>    - per-device, the list of partition(s)
>    - per-partition, the content
>
> For now, the only content possible for partitions is a filesystem. It
> should be pretty easy to add new types (eg. un-formated, or raw blob).
>
> Also, only two filesystems are supported: ext{2,3,4} and vfat. Adding
> more will be relatively easy, provided we have the necessary host
> packages to deal with those filesystems.
>
> The existing Buildroot filesystem generators are re-used as much as
> possible when it makes sense; when it does not (eg. for vfat), a specific
> generator is used.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

[snip, unreviewed]

> diff --git a/fs/Config.in b/fs/Config.in
> index da4c5ff..8721220 100644
> --- a/fs/Config.in
> +++ b/fs/Config.in
> @@ -11,5 +11,6 @@ source "fs/romfs/Config.in"
>   source "fs/squashfs/Config.in"
>   source "fs/tar/Config.in"
>   source "fs/ubifs/Config.in"
> +source "fs/custom/Config.in"

  Shouldn't this be kept alphabetical?

>
>   endmenu
> diff --git a/fs/custom/Config.in b/fs/custom/Config.in
> new file mode 100644
> index 0000000..fabb878
> --- /dev/null
> +++ b/fs/custom/Config.in
> @@ -0,0 +1,18 @@
> +config BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE
> +	string "partition table"

  In most cases, we don't rely on the non-emptiness of a string to 
determine if some option is enabled or not, but rather there's an 
explicit config option to enable it. I'm not convinced that that is a 
good principle, but it's how things are done now.

> +	help
> +	  Enter the path to a partition-table for your device.
> +
> +	  This will allow Buildroot to generate a more complex target
> +	  image, which may consist of more than one filesystem on more
> +	  than one partition.
> +
> +	  See docs/manual/bla-bla on how to construct such a partition

  I couldn't find the bla-bla section :-)

  We currently have one reference to the manual, and it's formatted like 
this:

http://buildroot.org/downloads/manual/manual.html#rootfs-custom

  I like that much better - though it's too long of course, but perhaps 
Peter can create a shortlink http://buildroot.org/man that expands to
http://buildroot.org/downloads/manual/manual.html ? [OT: such a shortlink 
for autobuilder results would also be convenient, because currently the 
references to autobuilder fixes exceed the 76-column width].


> +	  table.
> +
> +# For the fs infrasturcture, we need that option to be set.
> +# Additionally, it allows us to force the TAR output.
> +config BR2_TARGET_ROOTFS_CUSTOM
> +	def_bool y
> +	depends on BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE != ""

  In most other cases we use the following construct:

	bool
	default y if BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE != ""

> +	select BR2_TARGET_ROOTFS_TAR

[snip, unreviewed]

> diff --git a/fs/custom/custom.mk b/fs/custom/custom.mk
> new file mode 100644
> index 0000000..940a32a
> --- /dev/null
> +++ b/fs/custom/custom.mk
> @@ -0,0 +1,14 @@
> +################################################################################
> +#
> +# custom partitioning
> +#
> +################################################################################
> +
> +define ROOTFS_CUSTOM_CMD
> + BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \"$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE)\"

  Should be indented with a tab.

  Does that quoting work? It sill expand to

genimages \""path/to/partitiontable"\"

  so genimages' $1 will be "path/to/partititiontable" including the quotes.

  Anyway, the symbol should be qstrip'ped and any required quotes added 
explicitly. That makes it easier to run

make BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE=some/other/path


> +endef
> +
> +# We need the tarball to be generated first

  This comment is redundant

> +ROOTFS_CUSTOM_DEPENDENCIES += rootfs-tar
> +
> +$(eval $(call ROOTFS_TARGET,custom))

[snip, unreviewed]

> diff --git a/fs/custom/genimages b/fs/custom/genimages
> new file mode 100755
> index 0000000..aba3021
> --- /dev/null
> +++ b/fs/custom/genimages
> @@ -0,0 +1,214 @@
> +#!/bin/bash

  I have a general question for the maintainers here:

* Do we really want to rely on bash even more?

* Would it make sense to implement complex things like this in a proper 
programming language (read: python), which would solidify our dependency 
on python?

  With python's ConfigParser, this file would be reduced to +- 20 lines...


> +set -e
> +set -E
> +
> +#-----------------------------------------------------------------------------
> +main() {
> +    local part_table="${1}"
> +    local tmp_dir
> +    local rootfs_dir
> +    local -a devices
> +    local extract
> +    local cur_section
> +    local -a sections devices partitions
> +    local -A variables values partdevs
> +    local sec dev part var val
> +    local secs devs parts vars vals
> +
> +    if [ ! -f "${part_table}" ]; then
> +        error "%s: no such file\n" "${part_table}"
> +        exit 1
> +    fi
> +
> +    export PATH="${HOST_DIR}/usr/bin:${HOST_DIR}/usr/sbin:${PATH}"
> +
> +    tmp_dir="$( mktemp -d --tmpdir rpi-genimage.XXXXXX )"

  Small nit: I think it would make more sense to create the tmp_dir 
relative to the output directory.

  Larger nit: there should be a trap to clean up tmp_dir. Or 
alternatively, if it's relative to the output directory do the cleanup in 
the beginning.

  And finally, I'd create the tmp_dir only after parsing and validating 
the partition file.

> +
> +    rootfs_dir="${tmp_dir}/rootfs"
> +    mkdir -p "${rootfs_dir}"
> +
> +    # Parse all the sections in one go, we'll sort
> +    # all the mess afterwards...
> +    debug "parsing partitions descriptions file '%s'\n" \
> +          "${part_table}"
> +    while read line; do
> +        line="$( sed -r -e 's/[[:space:]]*#.*$//; //d;' <<<"${line}" )"

  I would try to avoid sed -r if you don't really need it - especially if 
you don't use extended regexp@all.

  To make this incredibly complex line a little more readable, I'd split 
it in two lines:

     line="$( sed -e 's/[[:space:]]*#.*$//' <<<"${line}" )"
     line="$( sed -e '//d;' <<<"${line}" )"

> +
> +        # Detect start of global section, skip anything else
> +        case "${line}" in
> +        "") continue;;
> +        '['*']')
> +            cur_section="$( sed -r -e 's/[][]//g;' <<<"${line}" )"
> +            debug "  entering section '%s'\n" "${cur_section}"
> +            sections+=( "${cur_section}" )
> +            continue
> +        ;;
> +        ?*=*)   ;;
> +        *)      error "malformed entry '%s'\n" "${line}";;
> +        esac
> +
> +        var="${line%%=*}"
> +        eval val="${line#*=}"
> +        debug "    adding '%s'='%s'\n" "${var}" "${val}"
> +        variables+=( ["${cur_section}"]=",${var}" )
> +        values+=( ["${cur_section}:${var}"]="${val}" )
> +    done <"${part_table}"

  I would add explicit checks that the global section exists and that it 
includes the required variables.

> +
> +    # Create lists of devices, partitions, and partition:device pairs.
> +    debug "creating intermeditate lists\n"
> +    devices=( ${values["global:devices"]//,/ } )
> +    for dev in "${devices[@]}"; do
> +        partitions=( ${values["${dev}:partitions"]//,/ } )

  I'd include a check that partitions exsits and has the right format.

> +    done
> +    for dev in "${devices[@]}"; do
> +        for part in ${values["${dev}:partitions"]//,/ }; do
> +            partdevs+=( ["${part}"]="${dev}" )

  Why do you loop twice here, instead of just doing this in the previous 
loop?

  I'm not very familiar with bash array syntax, but can't you use 
something like "for part in ${partitions[@]}" ?

> +        done
> +    done
> +
> +    # Now, we must order the partitions so that their mountpoint
> +    # is empty by the time we build the upper-level partition.
> +    # For example, given this layout of mountpoints:
> +    #   /
> +    #   /usr
> +    #   /usr/var
> +    # We must ensure /usr/var is empty at the time we create the /usr
> +    # filesystem image; and similarly, we must ensure /usr is empty by
> +    # the time we create the / filesystem image
> +    # So, a simple reverse alphabetical sort will do the trick
> +    debug "sorting partitions\n"
> +    sorted_parts=( $(
> +        for part in "${partitions[@]}"; do
> +            printf "%s:%s\n" "${part}" "${values["${part}:fs_root"]}"
> +        done                    \
> +        |sort -t: -k2 -r        \
> +        |sed -r -e 's/:[^:]+$//;'
> +    ) )
> +
> +    trace "extracting root (if needed)\n"
> +    case "${values["global:extract"]}" in
> +        targz)  c=z ;;&
> +        tarbz2) c=j ;;&
> +        tarxz)  c=J ;;&

  Since we use a fairly recent tar, tar will auto-detect compression 
based on the extension. By the way, since you only support tar anyway, I 
would remove this option completely. Whenever it is actually useful, it 
can be added again (with a default to tar for backwards compatibility).

> +        tar)    c=  ;;&
> +        tar*)   tar x${c}f "${BINARIES_DIR}/rootfs.tar" -C "${rootfs_dir}";;
> +        "")     ;;
> +        *)      error "unknown extract method '%s'\n" "${extract}";;
> +    esac

[Rest is unreviewed]

  Regards,
  Arnout



-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 4/5] fs/custom: generate complete, partition-based device images

[Yann E. MORIN]

Arnout,

On 2013-11-25 10:31 +0100, Arnout Vandecappelle spake thusly:
>  This patch is obviously too large to be reviewed in a single shot, so here
> are some detailed comments on certain parts of it.

Yes, I did expect it to be not trivial. Thank you for trying! :-)

>  I don't think there's a way to split the patch up to make review easier,
> unfortunately. But anyway, the functionality is completely isolated from the
> rest so it doesn't hurt to commit it as is and fix up later if necessary.

Maybe I could have separated the doc changes from the actual
implementation, to make it easier to review, and eventuall squash
both in a single cset for the final suvmission.

> On 22/11/13 23:50, Yann E. MORIN wrote:
> >From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >Contrary to the existing fs/ schemes, which each generate only a single
> >filesystem image for the root filesystem, this new scheme allows the
> >user to generate more complex images.
> >diff --git a/fs/Config.in b/fs/Config.in
> >index da4c5ff..8721220 100644
> >--- a/fs/Config.in
> >+++ b/fs/Config.in
> >@@ -11,5 +11,6 @@ source "fs/romfs/Config.in"
> >  source "fs/squashfs/Config.in"
> >  source "fs/tar/Config.in"
> >  source "fs/ubifs/Config.in"
> >+source "fs/custom/Config.in"
> 
>  Shouldn't this be kept alphabetical?

I also wondered about it, but my reasoning was to leave all single-fs
options grouped by themselves, and add this new one as the last (or
first) in the menu, to explicitly differentiate it.

But I have no stronger opinion than "I find it nicer".

> >
> >  endmenu
> >diff --git a/fs/custom/Config.in b/fs/custom/Config.in
> >new file mode 100644
> >index 0000000..fabb878
> >--- /dev/null
> >+++ b/fs/custom/Config.in
> >@@ -0,0 +1,18 @@
> >+config BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE
> >+	string "partition table"
> 
>  In most cases, we don't rely on the non-emptiness of a string to determine
> if some option is enabled or not, but rather there's an explicit config
> option to enable it. I'm not convinced that that is a good principle, but
> it's how things are done now.

OK, I see.

My reasoning is that passing the path to the "partition table" is enough
to state that the user does want to use a partition table, hence I did
not hide it behind a bool.

That's what we do for (eg.) BR2_ROOTFS_DEVICE_TABLE: if it is empty, we
treat it as to not add any device. The fact that the option is set/unset
is in itself enough to act or not.

The boolean below is indeed needed since we do need a boolean for our
internal use, but I see no reason to hide the string option behind the
boolean. Hence the boolean is a blind option.

> >+	help
> >+	  Enter the path to a partition-table for your device.
> >+
> >+	  This will allow Buildroot to generate a more complex target
> >+	  image, which may consist of more than one filesystem on more
> >+	  than one partition.
> >+
> >+	  See docs/manual/bla-bla on how to construct such a partition
> 
>  I couldn't find the bla-bla section :-)

Hey! :-)

>  We currently have one reference to the manual, and it's formatted like
> this:
> 
> http://buildroot.org/downloads/manual/manual.html#rootfs-custom

OK, I'll update once the doc has stabilised.

>  I like that much better - though it's too long of course, but perhaps Peter
> can create a shortlink http://buildroot.org/man that expands to
> http://buildroot.org/downloads/manual/manual.html ?

Yep, that would be nice.

> >+	  table.
> >+
> >+# For the fs infrasturcture, we need that option to be set.
> >+# Additionally, it allows us to force the TAR output.
> >+config BR2_TARGET_ROOTFS_CUSTOM
> >+	def_bool y
> >+	depends on BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE != ""
> 
>  In most other cases we use the following construct:
> 
> 	bool
> 	default y if BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE != ""

OK, I'll change.

Note however that the two constructs are not equivalent in kconfig (but
for us they give the same result). One is about default value and
depdency of the symbol (mine), the other is about the dependency of a
default value (yours).

> >+	select BR2_TARGET_ROOTFS_TAR
> 
> [snip, unreviewed]
> 
> >diff --git a/fs/custom/custom.mk b/fs/custom/custom.mk
> >new file mode 100644
> >index 0000000..940a32a
> >--- /dev/null
> >+++ b/fs/custom/custom.mk
> >@@ -0,0 +1,14 @@
> >+################################################################################
> >+#
> >+# custom partitioning
> >+#
> >+################################################################################
> >+
> >+define ROOTFS_CUSTOM_CMD
> >+ BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \"$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE)\"
> 
>  Should be indented with a tab.

No, because it is used as:

    echo "$$(ROOTFS_$(2)_CMD)" >> $$(FAKEROOT_SCRIPT)

>  Does that quoting work? It sill expand to

Yes it does, because it is interpreted twice (not sure how, but if I
remove the quotes and set BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE="foo bar"
then genimages gets two args, not one. Don;t ask me, double indirection
in the Makefile fragemnt plus shell expansion, something like it...

> genimages \""path/to/partitiontable"\"
>  so genimages' $1 will be "path/to/partititiontable" including the quotes.

No, because both quotes have already been stripped away.

> 
>  Anyway, the symbol should be qstrip'ped and any required quotes added
> explicitly. That makes it easier to run
> 
> make BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE=some/other/path

So, qstrip the symbol and re-add the quotes? Tss... OK, I can do that if
you want. ;-)

> >+endef
> >+
> >+# We need the tarball to be generated first
> 
>  This comment is redundant

What about:

    # rootfs-custom uses rootfs.tar as the source to generate the
    # resulting image(s), so we need to build it first.

Also, I forgot to add:
    rootfs-custom-show-depends:
        @echo $(ROOTFS_CUSTOM_DEPENDENCIES)

> >+ROOTFS_CUSTOM_DEPENDENCIES += rootfs-tar
> >+
> >+$(eval $(call ROOTFS_TARGET,custom))
> 
> [snip, unreviewed]
> 
> >diff --git a/fs/custom/genimages b/fs/custom/genimages
> >new file mode 100755
> >index 0000000..aba3021
> >--- /dev/null
> >+++ b/fs/custom/genimages
> >@@ -0,0 +1,214 @@
> >+#!/bin/bash
> 
>  I have a general question for the maintainers here:
> 
> * Do we really want to rely on bash even more?

bash is already a hard-dependency of Buildroot. anyway.

> * Would it make sense to implement complex things like this in a proper
> programming language (read: python), which would solidify our dependency on
> python?

I don't do Python.

>  With python's ConfigParser, this file would be reduced to +- 20 lines...

Does ConfigParser handles lines like:
   key=$((4*1024))

> >+set -e
> >+set -E
> >+
> >+#-----------------------------------------------------------------------------
> >+main() {
> >+    local part_table="${1}"
> >+    local tmp_dir
> >+    local rootfs_dir
> >+    local -a devices
> >+    local extract
> >+    local cur_section
> >+    local -a sections devices partitions
> >+    local -A variables values partdevs
> >+    local sec dev part var val
> >+    local secs devs parts vars vals
> >+
> >+    if [ ! -f "${part_table}" ]; then
> >+        error "%s: no such file\n" "${part_table}"
> >+        exit 1
> >+    fi
> >+
> >+    export PATH="${HOST_DIR}/usr/bin:${HOST_DIR}/usr/sbin:${PATH}"
> >+
> >+    tmp_dir="$( mktemp -d --tmpdir rpi-genimage.XXXXXX )"
> 
>  Small nit: I think it would make more sense to create the tmp_dir relative
> to the output directory.

OK.

>  Larger nit: there should be a trap to clean up tmp_dir. Or alternatively,
> if it's relative to the output directory do the cleanup in the beginning.

I initially had that. But since we may want to debug the issues in
genimages, we need to keep the temp dir. So, moving to build-dir and
cleaning at the beginning is OK for my use-case.

>  And finally, I'd create the tmp_dir only after parsing and validating the
> partition file.

OK.

> >+    rootfs_dir="${tmp_dir}/rootfs"
> >+    mkdir -p "${rootfs_dir}"
> >+
> >+    # Parse all the sections in one go, we'll sort
> >+    # all the mess afterwards...
> >+    debug "parsing partitions descriptions file '%s'\n" \
> >+          "${part_table}"
> >+    while read line; do
> >+        line="$( sed -r -e 's/[[:space:]]*#.*$//; //d;' <<<"${line}" )"
> 
>  I would try to avoid sed -r if you don't really need it - especially if you
> don't use extended regexp at all.

I never care about regexp being extended or not, I just use -r all the
time. It is much simpler: I never know if the regexp I'm using is
extended or not.

>  To make this incredibly complex line a little more readable, I'd split it
> in two lines:
> 
>     line="$( sed -e 's/[[:space:]]*#.*$//' <<<"${line}" )"
>     line="$( sed -e '//d;' <<<"${line}" )"

"Incredibly complex"? You're kidding, aren't you? Besides, yours is
broken, since '//d;' relies on the previous expresion. ;-p

> >+
> >+        # Detect start of global section, skip anything else
> >+        case "${line}" in
> >+        "") continue;;
> >+        '['*']')
> >+            cur_section="$( sed -r -e 's/[][]//g;' <<<"${line}" )"
> >+            debug "  entering section '%s'\n" "${cur_section}"
> >+            sections+=( "${cur_section}" )
> >+            continue
> >+        ;;
> >+        ?*=*)   ;;
> >+        *)      error "malformed entry '%s'\n" "${line}";;
> >+        esac
> >+
> >+        var="${line%%=*}"
> >+        eval val="${line#*=}"
> >+        debug "    adding '%s'='%s'\n" "${var}" "${val}"
> >+        variables+=( ["${cur_section}"]=",${var}" )
> >+        values+=( ["${cur_section}:${var}"]="${val}" )
> >+    done <"${part_table}"
> 
>  I would add explicit checks that the global section exists and that it
> includes the required variables.

OK, makes sense. Ditto, all referenced devices/partitions should have a
corresponding section.

> >+    # Create lists of devices, partitions, and partition:device pairs.
> >+    debug "creating intermeditate lists\n"
> >+    devices=( ${values["global:devices"]//,/ } )
> >+    for dev in "${devices[@]}"; do
> >+        partitions=( ${values["${dev}:partitions"]//,/ } )
Hmm. Bug here -------^ should be += I think.

>  I'd include a check that partitions exsits and has the right format.

See above.

> >+    done
> >+    for dev in "${devices[@]}"; do
> >+        for part in ${values["${dev}:partitions"]//,/ }; do
> >+            partdevs+=( ["${part}"]="${dev}" )
> 
>  Why do you loop twice here, instead of just doing this in the previous
> loop?

Hmm, lemme check...

>  I'm not very familiar with bash array syntax, but can't you use something
> like "for part in ${partitions[@]}" ?

But how do I know what device a partition is on?
(but do I need that anyway?) I'll check.

> >+    trace "extracting root (if needed)\n"
> >+    case "${values["global:extract"]}" in
> >+        targz)  c=z ;;&
> >+        tarbz2) c=j ;;&
> >+        tarxz)  c=J ;;&
> 
>  Since we use a fairly recent tar, tar will auto-detect compression based on
> the extension.

I don't like that... But OK.

> By the way, since you only support tar anyway, I would remove
> this option completely. Whenever it is actually useful, it can be added
> again (with a default to tar for backwards compatibility).

Yep, no need to over-engineer the sutff.

> >+        tar)    c=  ;;&
> >+        tar*)   tar x${c}f "${BINARIES_DIR}/rootfs.tar" -C "${rootfs_dir}";;
> >+        "")     ;;
> >+        *)      error "unknown extract method '%s'\n" "${extract}";;
> >+    esac
> 
> [Rest is unreviewed]

Thank you very much for the review! :-)

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 4/5] fs/custom: generate complete, partition-based device images

[Arnout Vandecappelle]

On 25/11/13 20:05, Yann E. MORIN wrote:
> Arnout,
>
> On 2013-11-25 10:31 +0100, Arnout Vandecappelle spake thusly:
>>   This patch is obviously too large to be reviewed in a single shot, so here
>> are some detailed comments on certain parts of it.
>
> Yes, I did expect it to be not trivial. Thank you for trying! :-)
>
>>   I don't think there's a way to split the patch up to make review easier,
>> unfortunately. But anyway, the functionality is completely isolated from the
>> rest so it doesn't hurt to commit it as is and fix up later if necessary.
>
> Maybe I could have separated the doc changes from the actual
> implementation, to make it easier to review, and eventuall squash
> both in a single cset for the final suvmission.

  I don't think separating the documentation would make review any easier.

  Something that _would_ make the review easier is to start with a 
simpler, less-featured version. But splitting it now is pretty much 
impossible.


>
>> On 22/11/13 23:50, Yann E. MORIN wrote:
>>> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>>> Contrary to the existing fs/ schemes, which each generate only a single
>>> filesystem image for the root filesystem, this new scheme allows the
>>> user to generate more complex images.
>>> diff --git a/fs/Config.in b/fs/Config.in
>>> index da4c5ff..8721220 100644
>>> --- a/fs/Config.in
>>> +++ b/fs/Config.in
>>> @@ -11,5 +11,6 @@ source "fs/romfs/Config.in"
>>>   source "fs/squashfs/Config.in"
>>>   source "fs/tar/Config.in"
>>>   source "fs/ubifs/Config.in"
>>> +source "fs/custom/Config.in"
>>
>>   Shouldn't this be kept alphabetical?
>
> I also wondered about it, but my reasoning was to leave all single-fs
> options grouped by themselves, and add this new one as the last (or
> first) in the menu, to explicitly differentiate it.
>
> But I have no stronger opinion than "I find it nicer".

  I agree with your reasoning, but it's a change from our normal coding 
style so should be discussed I guess.

  Peter?

>
>>>
>>>   endmenu
>>> diff --git a/fs/custom/Config.in b/fs/custom/Config.in
>>> new file mode 100644
>>> index 0000000..fabb878
>>> --- /dev/null
>>> +++ b/fs/custom/Config.in
>>> @@ -0,0 +1,18 @@
>>> +config BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE
>>> +	string "partition table"
>>
>>   In most cases, we don't rely on the non-emptiness of a string to determine
>> if some option is enabled or not, but rather there's an explicit config
>> option to enable it. I'm not convinced that that is a good principle, but
>> it's how things are done now.
>
> OK, I see.
>
> My reasoning is that passing the path to the "partition table" is enough
> to state that the user does want to use a partition table, hence I did
> not hide it behind a bool.
>
> That's what we do for (eg.) BR2_ROOTFS_DEVICE_TABLE: if it is empty, we
> treat it as to not add any device. The fact that the option is set/unset
> is in itself enough to act or not.
>
> The boolean below is indeed needed since we do need a boolean for our
> internal use, but I see no reason to hide the string option behind the
> boolean. Hence the boolean is a blind option.

  Again, I agree with your reasoning but it doesn't comply with current 
policy (BR2_ROOTFS_DEVICE_TABLE is not such a great example BTW, because 
it defaults to non-empty and it is not used in Kconfig conditions).

  So again: Peter?

[snip]
>>> diff --git a/fs/custom/custom.mk b/fs/custom/custom.mk
>>> new file mode 100644
>>> index 0000000..940a32a
>>> --- /dev/null
>>> +++ b/fs/custom/custom.mk
>>> @@ -0,0 +1,14 @@
>>> +################################################################################
>>> +#
>>> +# custom partitioning
>>> +#
>>> +################################################################################
>>> +
>>> +define ROOTFS_CUSTOM_CMD
>>> + BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \"$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE)\"
>>
>>   Should be indented with a tab.
>
> No, because it is used as:
>
>      echo "$$(ROOTFS_$(2)_CMD)" >> $$(FAKEROOT_SCRIPT)

  Err... Most of the other ROOTFS_FOO_CMD definitions use tab 
indentation, which is the standard for Buildroot. Only tar and squashfs 
which are really really old use spaces.

>
>>   Does that quoting work? It sill expand to
>
> Yes it does, because it is interpreted twice (not sure how, but if I
> remove the quotes and set BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE="foo bar"
> then genimages gets two args, not one. Don;t ask me, double indirection
> in the Makefile fragemnt plus shell expansion, something like it...

  That is fully explained by the expansion below:

>> genimages \""path/to/partitiontable"\"
>>   so genimages' $1 will be "path/to/partititiontable" including the quotes.
>
> No, because both quotes have already been stripped away.

  You're right about the no, but not about the reason :-)  It is used as:

echo "genimages \""path/to/partitiontable"\"" > $(FAKEROOT_SCRIPT)

  So what you actually get is an unquoted path/to/partitiontable, but 
there will be quotes inside the fakeroot script.

  Anyway, I think it should be:

BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \
	'$(call qstrip,$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE))'


>
>>
>>   Anyway, the symbol should be qstrip'ped and any required quotes added
>> explicitly. That makes it easier to run
>>
>> make BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE=some/other/path
>
> So, qstrip the symbol and re-add the quotes? Tss... OK, I can do that if
> you want. ;-)

  Well, it's what we do all over the place.

>
>>> +endef
>>> +
>>> +# We need the tarball to be generated first
>>
>>   This comment is redundant
>
> What about:
>
>      # rootfs-custom uses rootfs.tar as the source to generate the
>      # resulting image(s), so we need to build it first.
>
> Also, I forgot to add:
>      rootfs-custom-show-depends:
>          @echo $(ROOTFS_CUSTOM_DEPENDENCIES)

  Doesn't ROOTFS_TARGET add that?

>
>>> +ROOTFS_CUSTOM_DEPENDENCIES += rootfs-tar
>>> +
>>> +$(eval $(call ROOTFS_TARGET,custom))
>>
>> [snip, unreviewed]
>>
>>> diff --git a/fs/custom/genimages b/fs/custom/genimages
>>> new file mode 100755
>>> index 0000000..aba3021
>>> --- /dev/null
>>> +++ b/fs/custom/genimages
>>> @@ -0,0 +1,214 @@
>>> +#!/bin/bash
>>
>>   I have a general question for the maintainers here:
>>
>> * Do we really want to rely on bash even more?
>
> bash is already a hard-dependency of Buildroot. anyway.

  Yes, but I think it's just because of the apply-patches script. That's 
why I ask: do we want to make this hard dependency even harder, or do we 
prefer to go for posix shell as much as possible.

  That said, now I've read the script in a bit more detail: making them 
posix-shell compliant would be pretty hard.

>
>> * Would it make sense to implement complex things like this in a proper
>> programming language (read: python), which would solidify our dependency on
>> python?
>
> I don't do Python.

  Good reason.

>
>>   With python's ConfigParser, this file would be reduced to +- 20 lines...
>
> Does ConfigParser handles lines like:
>     key=$((4*1024))

  Obviously not, it would have to be
key = 4*1024

>
>>> +set -e
>>> +set -E
>>> +
>>> +#-----------------------------------------------------------------------------
>>> +main() {
>>> +    local part_table="${1}"
>>> +    local tmp_dir
>>> +    local rootfs_dir
>>> +    local -a devices
>>> +    local extract
>>> +    local cur_section
>>> +    local -a sections devices partitions
>>> +    local -A variables values partdevs
>>> +    local sec dev part var val
>>> +    local secs devs parts vars vals
>>> +
>>> +    if [ ! -f "${part_table}" ]; then
>>> +        error "%s: no such file\n" "${part_table}"
>>> +        exit 1
>>> +    fi
>>> +
>>> +    export PATH="${HOST_DIR}/usr/bin:${HOST_DIR}/usr/sbin:${PATH}"
>>> +
>>> +    tmp_dir="$( mktemp -d --tmpdir rpi-genimage.XXXXXX )"
>>
>>   Small nit: I think it would make more sense to create the tmp_dir relative
>> to the output directory.
>
> OK.
>
>>   Larger nit: there should be a trap to clean up tmp_dir. Or alternatively,
>> if it's relative to the output directory do the cleanup in the beginning.
>
> I initially had that. But since we may want to debug the issues in
> genimages, we need to keep the temp dir. So, moving to build-dir and
> cleaning at the beginning is OK for my use-case.
>
>>   And finally, I'd create the tmp_dir only after parsing and validating the
>> partition file.
>
> OK.
>
>>> +    rootfs_dir="${tmp_dir}/rootfs"
>>> +    mkdir -p "${rootfs_dir}"
>>> +
>>> +    # Parse all the sections in one go, we'll sort
>>> +    # all the mess afterwards...
>>> +    debug "parsing partitions descriptions file '%s'\n" \
>>> +          "${part_table}"
>>> +    while read line; do
>>> +        line="$( sed -r -e 's/[[:space:]]*#.*$//; //d;' <<<"${line}" )"
>>
>>   I would try to avoid sed -r if you don't really need it - especially if you
>> don't use extended regexp at all.
>
> I never care about regexp being extended or not, I just use -r all the
> time. It is much simpler: I never know if the regexp I'm using is
> extended or not.
>
>>   To make this incredibly complex line a little more readable, I'd split it
>> in two lines:
>>
>>      line="$( sed -e 's/[[:space:]]*#.*$//' <<<"${line}" )"
>>      line="$( sed -e '//d;' <<<"${line}" )"
>
> "Incredibly complex"? You're kidding, aren't you?

  No, I'm not kidding. I had to do sed --help to remember what -r is 
about, I had to think a little about the <<< construct, I had to read the 
sed expression two times before I noticed the ;, and only then I could 
start parsing the regex.

> Besides, yours is
> broken, since '//d;' relies on the previous expresion. ;-p

  ... and even then I failed to notice that // is not the same as /^$/ :-)

  But so the //d is actually redundant. Because it matches the part that 
was just removed, so it never matches.

>
>>> +
>>> +        # Detect start of global section, skip anything else
>>> +        case "${line}" in
>>> +        "") continue;;
>>> +        '['*']')
>>> +            cur_section="$( sed -r -e 's/[][]//g;' <<<"${line}" )"
>>> +            debug "  entering section '%s'\n" "${cur_section}"
>>> +            sections+=( "${cur_section}" )
>>> +            continue
>>> +        ;;
>>> +        ?*=*)   ;;
>>> +        *)      error "malformed entry '%s'\n" "${line}";;
>>> +        esac
>>> +
>>> +        var="${line%%=*}"
>>> +        eval val="${line#*=}"
>>> +        debug "    adding '%s'='%s'\n" "${var}" "${val}"
>>> +        variables+=( ["${cur_section}"]=",${var}" )
>>> +        values+=( ["${cur_section}:${var}"]="${val}" )
>>> +    done <"${part_table}"
>>
>>   I would add explicit checks that the global section exists and that it
>> includes the required variables.
>
> OK, makes sense. Ditto, all referenced devices/partitions should have a
> corresponding section.
>
>>> +    # Create lists of devices, partitions, and partition:device pairs.
>>> +    debug "creating intermeditate lists\n"

  intermediate

  But perhaps you need to mediate a little in the middle of this 
function? :-)

>>> +    devices=( ${values["global:devices"]//,/ } )
>>> +    for dev in "${devices[@]}"; do
>>> +        partitions=( ${values["${dev}:partitions"]//,/ } )
> Hmm. Bug here -------^ should be += I think.
>
>>   I'd include a check that partitions exsits and has the right format.
>
> See above.
>
>>> +    done
>>> +    for dev in "${devices[@]}"; do
>>> +        for part in ${values["${dev}:partitions"]//,/ }; do
>>> +            partdevs+=( ["${part}"]="${dev}" )
>>
>>   Why do you loop twice here, instead of just doing this in the previous
>> loop?
>
> Hmm, lemme check...
>
>>   I'm not very familiar with bash array syntax, but can't you use something
>> like "for part in ${partitions[@]}" ?
>
> But how do I know what device a partition is on?
> (but do I need that anyway?) I'll check.

  With the += correction you mentioned above, your code makes a lot more 
sense.

  Regards,
  Arnout

>
>>> +    trace "extracting root (if needed)\n"
>>> +    case "${values["global:extract"]}" in
>>> +        targz)  c=z ;;&
>>> +        tarbz2) c=j ;;&
>>> +        tarxz)  c=J ;;&
>>
>>   Since we use a fairly recent tar, tar will auto-detect compression based on
>> the extension.
>
> I don't like that... But OK.
>
>> By the way, since you only support tar anyway, I would remove
>> this option completely. Whenever it is actually useful, it can be added
>> again (with a default to tar for backwards compatibility).
>
> Yep, no need to over-engineer the sutff.
>
>>> +        tar)    c=  ;;&
>>> +        tar*)   tar x${c}f "${BINARIES_DIR}/rootfs.tar" -C "${rootfs_dir}";;
>>> +        "")     ;;
>>> +        *)      error "unknown extract method '%s'\n" "${extract}";;
>>> +    esac
>>
>> [Rest is unreviewed]
>
> Thank you very much for the review! :-)
>
> Regards,
> Yann E. MORIN.
>


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 4/5] fs/custom: generate complete, partition-based device images

[Yann E. MORIN]

Arnout, All,

On 2013-11-25 23:27 +0100, Arnout Vandecappelle spake thusly:
> On 25/11/13 20:05, Yann E. MORIN wrote:
> >Arnout,
> >
> >On 2013-11-25 10:31 +0100, Arnout Vandecappelle spake thusly:
> >>  This patch is obviously too large to be reviewed in a single shot, so here
> >>are some detailed comments on certain parts of it.
> >
> >Yes, I did expect it to be not trivial. Thank you for trying! :-)
> >
> >>  I don't think there's a way to split the patch up to make review easier,
> >>unfortunately. But anyway, the functionality is completely isolated from the
> >>rest so it doesn't hurt to commit it as is and fix up later if necessary.
> >
> >Maybe I could have separated the doc changes from the actual
> >implementation, to make it easier to review, and eventuall squash
> >both in a single cset for the final suvmission.
> 
>  I don't think separating the documentation would make review any easier.
> 
>  Something that _would_ make the review easier is to start with a simpler,
> less-featured version. But splitting it now is pretty much impossible.

Yes, too late, the script pre-existed the push by a few months now, and
splitting would be horrible on my side... :-(

> >>>diff --git a/fs/custom/Config.in b/fs/custom/Config.in
> >>>new file mode 100644
> >>>index 0000000..fabb878
> >>>--- /dev/null
> >>>+++ b/fs/custom/Config.in
> >>>@@ -0,0 +1,18 @@
> >>>+config BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE
> >>>+	string "partition table"
> >>
> >>  In most cases, we don't rely on the non-emptiness of a string to determine
> >>if some option is enabled or not, but rather there's an explicit config
> >>option to enable it. I'm not convinced that that is a good principle, but
> >>it's how things are done now.
> >
> >OK, I see.
> >
> >My reasoning is that passing the path to the "partition table" is enough
> >to state that the user does want to use a partition table, hence I did
> >not hide it behind a bool.
> >
> >That's what we do for (eg.) BR2_ROOTFS_DEVICE_TABLE: if it is empty, we
> >treat it as to not add any device. The fact that the option is set/unset
> >is in itself enough to act or not.
> >
> >The boolean below is indeed needed since we do need a boolean for our
> >internal use, but I see no reason to hide the string option behind the
> >boolean. Hence the boolean is a blind option.
> 
>  Again, I agree with your reasoning but it doesn't comply with current
> policy (BR2_ROOTFS_DEVICE_TABLE is not such a great example BTW, because it
> defaults to non-empty and it is not used in Kconfig conditions).

OK, I'll change it, you're right.

> [snip]
> >>>diff --git a/fs/custom/custom.mk b/fs/custom/custom.mk
> >>>new file mode 100644
> >>>index 0000000..940a32a
> >>>--- /dev/null
> >>>+++ b/fs/custom/custom.mk
> >>>@@ -0,0 +1,14 @@
[--SNIP--]
> >>>+ BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \"$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE)\"
> >>
> >>  Should be indented with a tab.
> >
> >No, because it is used as:
> >
> >     echo "$$(ROOTFS_$(2)_CMD)" >> $$(FAKEROOT_SCRIPT)
> 
>  Err... Most of the other ROOTFS_FOO_CMD definitions use tab indentation,
> which is the standard for Buildroot. Only tar and squashfs which are really
> really old use spaces.

OK, will change.

But notice that the tab requirement is only for cosmetics. For package,
we do need a tab, since the commands are expanded directly as a make
command block, which is not the case for the filesystems.

> >>  Does that quoting work? It sill expand to
> >
> >Yes it does, because it is interpreted twice (not sure how, but if I
> >remove the quotes and set BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE="foo bar"
> >then genimages gets two args, not one. Don;t ask me, double indirection
> >in the Makefile fragemnt plus shell expansion, something like it...
> 
>  That is fully explained by the expansion below:
> 
> >>genimages \""path/to/partitiontable"\"
> >>  so genimages' $1 will be "path/to/partititiontable" including the quotes.
> >
> >No, because both quotes have already been stripped away.
> 
>  You're right about the no, but not about the reason :-)  It is used as:

Yep, I noticed it later after I replied.

>  Anyway, I think it should be:
> 
> BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \
> 	'$(call qstrip,$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE))'

I'm using double quotes here, now, instead of single quotes.
Otherwise, consider it changed. Thanks!

> >>>+endef
> >>>+
> >>>+# We need the tarball to be generated first
> >>
> >>  This comment is redundant
> >
> >What about:
> >
> >     # rootfs-custom uses rootfs.tar as the source to generate the
> >     # resulting image(s), so we need to build it first.
> >
> >Also, I forgot to add:
> >     rootfs-custom-show-depends:
> >         @echo $(ROOTFS_CUSTOM_DEPENDENCIES)
> 
>  Doesn't ROOTFS_TARGET add that?

Yes, I was looking at initranfs, which does not use the fs infra, so
needs to add it itself. I removed it now.

> >>>diff --git a/fs/custom/genimages b/fs/custom/genimages
> >>>new file mode 100755
> >>>index 0000000..aba3021
> >>>--- /dev/null
> >>>+++ b/fs/custom/genimages
> >>>@@ -0,0 +1,214 @@
> >>>+#!/bin/bash
> >>
> >>  I have a general question for the maintainers here:
> >>
> >>* Do we really want to rely on bash even more?
> >
> >bash is already a hard-dependency of Buildroot. anyway.
> 
>  Yes, but I think it's just because of the apply-patches script. That's why
> I ask: do we want to make this hard dependency even harder, or do we prefer
> to go for posix shell as much as possible.

Well, POSIX is lacking the arrays, and the associative arrays I use
extensively throughout the script.

>  That said, now I've read the script in a bit more detail: making them
> posix-shell compliant would be pretty hard.

Yep.

> >>* Would it make sense to implement complex things like this in a proper
> >>programming language (read: python), which would solidify our dependency on
> >>python?
> >
> >I don't do Python.
> 
>  Good reason.

Hehe! :-)

> >>  With python's ConfigParser, this file would be reduced to +- 20 lines...
> >
> >Does ConfigParser handles lines like:
> >    key=$((4*1024))
> 
>  Obviously not, it would have to be
> key = 4*1024

For my education, are the spaces required? Because they are not in .ini,
and in fact should not be present.

> >>>+        line="$( sed -r -e 's/[[:space:]]*#.*$//; //d;' <<<"${line}" )"
[--SNIP--]
> >>  To make this incredibly complex line a little more readable, I'd split it
> >>in two lines:
> >>
> >>     line="$( sed -e 's/[[:space:]]*#.*$//' <<<"${line}" )"
> >>     line="$( sed -e '//d;' <<<"${line}" )"
> >
> >"Incredibly complex"? You're kidding, aren't you?
> 
>  No, I'm not kidding. I had to do sed --help to remember what -r is about, I
> had to think a little about the <<< construct, I had to read the sed
> expression two times before I noticed the ;, and only then I could start
> parsing the regex.

Hmm, looks like I have a knack in regexps! It looked trivial to me.
And <<< is a bash construct that I use everyday.

> >Besides, yours is
> >broken, since '//d;' relies on the previous expresion. ;-p
> 
>  ... and even then I failed to notice that // is not the same as /^$/ :-)
> 
>  But so the //d is actually redundant. Because it matches the part that was
> just removed, so it never matches.

Hmmm. I'll have to check that again, then...
/me fumbles a bit...
Indeed, it seems unneeded. I'll remove it.

So, it was not so trivial after all! :-)

> >>>+    # Create lists of devices, partitions, and partition:device pairs.
> >>>+    debug "creating intermeditate lists\n"
> 
>  intermediate

Doh! Roh... :-)

>  But perhaps you need to mediate a little in the middle of this function?
> :-)

Yes, I think I'll both meditate and mediate that stuff! :-)

> >>>+    devices=( ${values["global:devices"]//,/ } )
> >>>+    for dev in "${devices[@]}"; do
> >>>+        partitions=( ${values["${dev}:partitions"]//,/ } )
> >Hmm. Bug here -------^ should be += I think.
[--SNIP--]
> >>  I'm not very familiar with bash array syntax, but can't you use something
> >>like "for part in ${partitions[@]}" ?
> >
> >But how do I know what device a partition is on?
> >(but do I need that anyway?) I'll check.
> 
>  With the += correction you mentioned above, your code makes a lot more
> sense.

Aha! :-)

Thanks again!

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 4/5] fs/custom: generate complete, partition-based device images

[Arnout Vandecappelle]

On 25/11/13 23:45, Yann E. MORIN wrote:
> Arnout, All,
>
> On 2013-11-25 23:27 +0100, Arnout Vandecappelle spake thusly:
>> On 25/11/13 20:05, Yann E. MORIN wrote:
>>> Arnout,
>>>
>>> On 2013-11-25 10:31 +0100, Arnout Vandecappelle spake thusly:
[snip]
>>   Anyway, I think it should be:
>>
>> BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \
>> 	'$(call qstrip,$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE))'
>
> I'm using double quotes here, now, instead of single quotes.
> Otherwise, consider it changed. Thanks!

  With double quotes, it will not actually be quoted because the 
ROOTFS_$(2)_CMD use adds another level of double quotes and they cancel out.

[snip]
>>>>   With python's ConfigParser, this file would be reduced to +- 20 lines...
>>>
>>> Does ConfigParser handles lines like:
>>>     key=$((4*1024))
>>
>>   Obviously not, it would have to be
>> key = 4*1024
>
> For my education, are the spaces required? Because they are not in .ini,
> and in fact should not be present.

  Actually I don't know. Checking the source: spaces are allowed:

     OPTCRE = re.compile(
         r'(?P<option>[^:=\s][^:=]*)'          # very permissive!
         r'\s*(?P<vi>[:=])\s*'                 # any number of space/tab,
                                               # followed by separator
                                               # (either : or =), followed
                                               # by any # space/tab
         r'(?P<value>.*)$'                     # everything up to eol
         )



  Regards,
  Arnout

[snip]

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 4/5] fs/custom: generate complete, partition-based device images

[Yann E. MORIN]

Arnout, All,

On 2013-11-25 23:56 +0100, Arnout Vandecappelle spake thusly:
> On 25/11/13 23:45, Yann E. MORIN wrote:
> >Arnout, All,
> >
> >On 2013-11-25 23:27 +0100, Arnout Vandecappelle spake thusly:
> >>On 25/11/13 20:05, Yann E. MORIN wrote:
> >>>Arnout,
> >>>
> >>>On 2013-11-25 10:31 +0100, Arnout Vandecappelle spake thusly:
> [snip]
> >>  Anyway, I think it should be:
> >>
> >>BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \
> >>	'$(call qstrip,$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE))'
> >
> >I'm using double quotes here, now, instead of single quotes.
> >Otherwise, consider it changed. Thanks!
> 
>  With double quotes, it will not actually be quoted because the
> ROOTFS_$(2)_CMD use adds another level of double quotes and they cancel out.

Yes, I was again too fast to reply: I'm using \"$(BLABLA)\"

My reasoning is I want varuable to be expanded, like in:
    BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE="$(TOPDIR)/board/raspberrypi/partitions"

But since $(TOPDIR) is a Makefile constrcut, the shell won't expand it,
so it must be make expanding it (since it is working).

I'll try single quotes, since they look better.

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 4/5] fs/custom: generate complete, partition-based device images

[Arnout Vandecappelle]

On 26/11/13 00:03, Yann E. MORIN wrote:
> Arnout, All,
>
> On 2013-11-25 23:56 +0100, Arnout Vandecappelle spake thusly:
>> On 25/11/13 23:45, Yann E. MORIN wrote:
>>> Arnout, All,
>>>
>>> On 2013-11-25 23:27 +0100, Arnout Vandecappelle spake thusly:
>>>> On 25/11/13 20:05, Yann E. MORIN wrote:
>>>>> Arnout,
>>>>>
>>>>> On 2013-11-25 10:31 +0100, Arnout Vandecappelle spake thusly:
>> [snip]
>>>>   Anyway, I think it should be:
>>>>
>>>> BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \
>>>> 	'$(call qstrip,$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE))'
>>>
>>> I'm using double quotes here, now, instead of single quotes.
>>> Otherwise, consider it changed. Thanks!
>>
>>   With double quotes, it will not actually be quoted because the
>> ROOTFS_$(2)_CMD use adds another level of double quotes and they cancel out.
>
> Yes, I was again too fast to reply: I'm using \"$(BLABLA)\"
>
> My reasoning is I want varuable to be expanded, like in:
>      BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE="$(TOPDIR)/board/raspberrypi/partitions"

  You do know that make doesn't interpret quotes or backslashes? So the 
\" construct is passed verbatim to the shell. The $(...) on the other 
hand is always expanded, except when quoted by doubling the $$.


  Regards,
  Arnout

>
> But since $(TOPDIR) is a Makefile constrcut, the shell won't expand it,
> so it must be make expanding it (since it is working).
>
> I'll try single quotes, since they look better.
>
> Regards,
> Yann E. MORIN.
>


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 4/5] fs/custom: generate complete, partition-based device images

[Yann E. MORIN]

Arnout, All,

On 2013-11-26 09:12 +0100, Arnout Vandecappelle spake thusly:
> >>On 25/11/13 23:45, Yann E. MORIN wrote:
> >My reasoning is I want varuable to be expanded, like in:
> >     BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE="$(TOPDIR)/board/raspberrypi/partitions"
> 
>  You do know that make doesn't interpret quotes or backslashes? So the \"
> construct is passed verbatim to the shell. The $(...) on the other hand is
> always expanded, except when quoted by doubling the $$.

Yes, I know both. Fact is, given the constructit is not easy to know who
would do the evaluation: make or the shell. But it has to be make, since
$(FOO) is not a variable expansion in shell. So yes, single quotes will
work, too.

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 5/5] board/raspberrypi: provide partition description for the new genimanges

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

Now we can generate a complete target storage image with the genimages
infra, add a partition table layout description for the Raspberry Pi
as an example for how to use genimages.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 board/raspberrypi/partitions  | 31 +++++++++++++++++++++++++++++++
 configs/raspberrypi_defconfig |  9 +++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 board/raspberrypi/partitions

diff --git a/board/raspberrypi/partitions b/board/raspberrypi/partitions
new file mode 100644
index 0000000..e9084dd
--- /dev/null
+++ b/board/raspberrypi/partitions
@@ -0,0 +1,31 @@
+[global]
+extract=tar
+devices=mmcblk0
+
+[mmcblk0]
+type=boot
+boot_type=mbr
+mbr_bootcode=
+partitions=boot,root
+partalign=$((1048576))
+
+[root]
+type=fs
+size=$((128*1048576))
+mbr_type=$((0x83))
+fs_type=ext
+fs_vfstype=ext4
+fs_mntops=discard
+fs_root=/
+fs_label=ROOT
+ext_gen=4
+ext_rev=1
+
+[boot]
+type=fs
+size=$((9*1048576))
+mbr_type=$((0xc))
+fs_type=vfat
+fs_root=/boot
+fs_label=BOOT
+vfat_size=16
diff --git a/configs/raspberrypi_defconfig b/configs/raspberrypi_defconfig
index db34717..118f9df 100644
--- a/configs/raspberrypi_defconfig
+++ b/configs/raspberrypi_defconfig
@@ -21,3 +21,12 @@ BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION="1587f77"
 BR2_LINUX_KERNEL_USE_DEFCONFIG=y
 BR2_LINUX_KERNEL_DEFCONFIG="bcmrpi_quick"
 BR2_LINUX_KERNEL_ZIMAGE=y
+BR2_LINUX_KERNEL_INSTALL_TARGET=y
+
+BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE="$(TOPDIR)/board/raspberrypi/partitions"
+
+BR2_PACKAGE_HOST_DOSFSTOOLS=y
+BR2_PACKAGE_HOST_E2FSPROGS=y
+BR2_PACKAGE_HOST_GENEXT2FS=y
+BR2_PACKAGE_HOST_GENPART=y
+BR2_PACKAGE_HOST_MTOOLS=y
-- 
1.8.1.2