A commit to extend the INITRD variable

PATCH In review [email protected] 2 participants 6 messages Jul 29, 2014 – Aug 5, 2014

lore

Share & tools

lore

[email protected]

author

2014-07-29 18:30 UTC #0 lore raw

cover A commit to extend the INITRD variable

From: Nitin A Kamble <[email protected]>

This commit extends the INITRD variable, so that it can accommodate multiple filesystems as expected by the Linux kernel.

Thanks to Ross Burton for this suggestion to simplify the early microcode loading support in the meta-intel Layer.

Thanks,
Nitin

The following changes since commit 2d1660112e54653f7bb763939d0416472c49fe01:

populate_sdk_base: Fix grep command usage on old hosts (2014-07-29 09:58:27 +0100)

are available in the git repository at:

git://git.yoctoproject.org/poky-contrib nitin/misc
http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=nitin/misc

Nitin A Kamble (1):
INITRD var: make it a list of filesystem images

 documentation/ref-manual/ref-classes.xml   |  4 ++--
 documentation/ref-manual/ref-variables.xml |  2 +-
 meta/classes/boot-directdisk.bbclass       | 13 ++++++++++---
 meta/classes/bootimg.bbclass               | 27 ++++++++++++++++++++++-----
 meta/classes/grub-efi.bbclass              |  2 +-
 meta/classes/syslinux.bbclass              |  2 +-
 meta/conf/documentation.conf               |  2 +-
 7 files changed, 38 insertions(+), 14 deletions(-)

Signature · 1 line

1.8.1.4

[email protected]

author

2014-07-29 18:30 UTC #1 lore raw

PATCH 1/1 INITRD var: make it a list of filesystem images

From: Nitin A Kamble <[email protected]>

The initrd image used by the Linux kernel is list of file system images concatenated together and presented as a single initrd file at boot time.

So far the initrd is a single filesystem image. But in cases like to support early microcode loading, the initrd image need to have multiple filesystem images concatenated together.

This commit is extending the INITRD variable from a single filesystem image to a list of filesystem images to satisfy the need mentioned above.

Signed-off-by Nitin A Kamble <[email protected]>

---
 documentation/ref-manual/ref-classes.xml   |  4 ++--
 documentation/ref-manual/ref-variables.xml |  2 +-
 meta/classes/boot-directdisk.bbclass       | 13 ++++++++++---
 meta/classes/bootimg.bbclass               | 27 ++++++++++++++++++++++-----
 meta/classes/grub-efi.bbclass              |  2 +-
 meta/classes/syslinux.bbclass              |  2 +-
 meta/conf/documentation.conf               |  2 +-
 7 files changed, 38 insertions(+), 14 deletions(-)

documentation/ref-manual/ref-classes.xml +2 -2

index e7e9942..0bf3546 100644
@@ -881,7 +881,7 @@
         <itemizedlist>
             <listitem><para>
                 <link linkend='var-INITRD'><filename>INITRD</filename></link>:
-                Indicates a filesystem image to use as an initrd (optional).
+                Indicates list of filesystem images to concatenate and use as an initrd (optional).
                 </para></listitem>
             <listitem><para>
                 <link linkend='var-ROOTFS'><filename>ROOTFS</filename></link>:

@@ -2864,7 +2864,7 @@
         The class supports the following variables:
         <itemizedlist>
             <listitem><para><emphasis><link linkend='var-INITRD'><filename>INITRD</filename></link>:</emphasis>
-                Indicates a filesystem image to use as an initial RAM disk
+                Indicates list of filesystem images to concatenate and use as an initial RAM disk
                 (initrd).
                 This variable is optional.</para></listitem>
             <listitem><para><emphasis><link linkend='var-ROOTFS'><filename>ROOTFS</filename></link>:</emphasis>

documentation/ref-manual/ref-variables.xml +1 -1

index b4d6e71..16e3ed6 100644
@@ -4020,7 +4020,7 @@ recipes-graphics/xorg-font/font-alias_1.0.3.bb:PR = "${INC_PR}.3"
         <glossentry id='var-INITRD'><glossterm>INITRD</glossterm>
             <glossdef>
                 <para>
-                    Indicates a filesystem image to use as an initial RAM
+                    Indicates list of filesystem images to concatenate and use as an initial RAM
                     disk (<filename>initrd</filename>).
                 </para>

meta/classes/boot-directdisk.bbclass +10 -3

index 0da9932..995d3e7 100644
@@ -71,10 +71,17 @@ boot_direct_populate() {
 	# Install bzImage, initrd, and rootfs.img in DEST for all loaders to use.
 	install -m 0644 ${STAGING_KERNEL_DIR}/bzImage $dest/vmlinuz
 
-	if [ -n "${INITRD}" ] && [ -s "${INITRD}" ]; then
-		install -m 0644 ${INITRD} $dest/initrd
+	# initrd is made of concatenation of multiple filesystem images
+	if [ -n "${INITRD}" ]; then
+		rm -f $dest/initrd
+		for fs in ${INITRD}
+		do
+			if [ -n "${fs}" ] && [ -s "${fs}" ]; then
+				cat ${fs} >> $dest/initrd
+			fi
+		done
+		chmod 0644 $dest/initrd
 	fi
-
 }
 
 build_boot_dd() {

meta/classes/bootimg.bbclass +22 -5

index d52aace..7b3ce65 100644
@@ -18,7 +18,7 @@
 # an hdd)
 
 # External variables (also used by syslinux.bbclass)
-# ${INITRD} - indicates a filesystem image to use as an initrd (optional)
+# ${INITRD} - indicates a list of filesystem images to concatenate and use as an initrd (optional)
 # ${COMPRESSISO} - Transparent compress ISO, reduce size ~40% if set to 1
 # ${NOISO}  - skip building the ISO image if set to 1
 # ${NOHDD}  - skip building the HDD image if set to 1

@@ -67,9 +67,17 @@ populate() {
 
 	# Install bzImage, initrd, and rootfs.img in DEST for all loaders to use.
 	install -m 0644 ${STAGING_KERNEL_DIR}/bzImage ${DEST}/vmlinuz
-
-	if [ -n "${INITRD}" ] && [ -s "${INITRD}" ]; then
-		install -m 0644 ${INITRD} ${DEST}/initrd
+	
+	# initrd is made of concatenation of multiple filesystem images
+	if [ -n "${INITRD}" ]; then
+		rm -f ${DEST}/initrd
+		for fs in ${INITRD}
+		do
+			if [ -s "${fs}" ]; then
+				cat ${fs} >> ${DEST}/initrd
+			fi
+		done
+		chmod 0644 ${DEST}/initrd
 	fi
 
 	if [ -n "${ROOTFS}" ] && [ -s "${ROOTFS}" ]; then

@@ -80,10 +88,19 @@ populate() {
 
 build_iso() {
 	# Only create an ISO if we have an INITRD and NOISO was not set
-	if [ -z "${INITRD}" ] || [ ! -s "${INITRD}" ] || [ "${NOISO}" = "1" ]; then
+	if [ -z "${INITRD}" ] || [ "${NOISO}" = "1" ]; then
 		bbnote "ISO image will not be created."
 		return
 	fi
+	# ${INITRD} is a list of multiple filesystem images
+	for fs in ${INITRD}
+	do
+		if [ ! -s "${fs}" ]; then
+			bbnote "ISO image will not be created. ${fs} is invalid."
+			return
+		fi
+	done
+
 
 	populate ${ISODIR}

meta/classes/grub-efi.bbclass +1 -1

index 505d032..47bd35e 100644
@@ -7,7 +7,7 @@
 # Provide grub-efi specific functions for building bootable images.
 
 # External variables
-# ${INITRD} - indicates a filesystem image to use as an initrd (optional)
+# ${INITRD} - indicates a list of filesystem images to concatenate and use as an initrd (optional)
 # ${ROOTFS} - indicates a filesystem image to include as the root filesystem (optional)
 # ${GRUB_GFXSERIAL} - set this to 1 to have graphics and serial in the boot menu
 # ${LABELS} - a list of targets for the automatic config

meta/classes/syslinux.bbclass +1 -1

index b9701bf..d6498d9 100644
@@ -5,7 +5,7 @@
 # Provide syslinux specific functions for building bootable images.
 
 # External variables
-# ${INITRD} - indicates a filesystem image to use as an initrd (optional)
+# ${INITRD} - indicates a list of filesystem images to concatenate and use as an initrd (optional)
 # ${ROOTFS} - indicates a filesystem image to include as the root filesystem (optional)
 # ${AUTO_SYSLINUXMENU} - set this to 1 to enable creating an automatic menu
 # ${LABELS} - a list of targets for the automatic config

meta/conf/documentation.conf +1 -1

index 7fa3f31..31fbd6c 100644
@@ -225,7 +225,7 @@ INHIBIT_PACKAGE_STRIP[doc] = "If set to "1", causes the build to not strip binar
 INHERIT[doc] = "Causes the named class to be inherited at this point during parsing. The variable is only valid in configuration files."
 INHERIT_DISTRO[doc] = "Lists classes that will be inherited at the distribution level. It is unlikely that you want to edit this variable."
 INITRAMFS_FSTYPES[doc] = "Defines the format for the output image of an initial RAM disk (initramfs), which is used during boot."
-INITRD[doc] = "Indicates a filesystem image to use as an initial RAM disk (initrd)."
+INITRD[doc] = "Indicates list of filesystem images to concatenate and use as an initial RAM disk (initrd)."
 INITSCRIPT_NAME[doc] = "The filename of the initialization script as installed to ${sysconfdir}/init.d."
 INITSCRIPT_PACKAGES[doc] = "A list of the packages that contain initscripts. This variable is used in recipes when using update-rc.d.bbclass. The variable is optional and defaults to the PN variable."
 INITSCRIPT_PARAMS[doc] = "Specifies the options to pass to update-rc.d. The variable is mandatory and is used in recipes when using update-rc.d.bbclass."

Signature · 1 line

1.8.1.4

Hart, Darren

2014-08-04 16:38 UTC #2 lore raw

Re: [PATCH 1/1] INITRD var: make it a list of filesystem images

Quoted 1 line · Nitin A

From: Nitin A Kamble <[email protected]>

Hi Nitin,

Generally speaking this looks like a good improvement. I don't have any major technical concerns, but we do need to address some grammatical issues in the commit and the docs below to make sure people can follow the intent here.

Quoted 8 lines

The initrd image used by the Linux kernel is list of file system images concatenated together and presented as a single initrd file at boot time.
So far the initrd is a single filesystem image. But in cases like to support
early microcode loading, the initrd image need to have multiple filesystem images concatenated together.

Consider:

Currently, the INITRD variable is a single filesystem image. For optional early boot features, such as microcode loading, a modular approach would provide the most flexibility and is minimally invasive. Converting INITRD to a list of images to be concatenated accomplishes this.

Quoted 4 lines

This commit is extending the INITRD variable from a single filesystem image
to a list of filesystem images to satisfy the need mentioned above.

Can now drop this paragraph.

Quoted 22 lines

Signed-off-by Nitin A Kamble <[email protected]>

---

documentation/ref-manual/ref-classes.xml   |  4 ++--
documentation/ref-manual/ref-variables.xml |  2 +-
meta/classes/boot-directdisk.bbclass       | 13 ++++++++++---
meta/classes/bootimg.bbclass               | 27
++++++++++++++++++++++-----
meta/classes/grub-efi.bbclass              |  2 +-
meta/classes/syslinux.bbclass              |  2 +-
meta/conf/documentation.conf               |  2 +-
7 files changed, 38 insertions(+), 14 deletions(-)

documentation/ref-manual/ref-classes.xml +0 -0

index e7e9942..0bf3546 100644
@@ -881,7 +881,7 @@
        <itemizedlist>
            <listitem><para>
                <link

Quoted 5 lines

linkend='var-INITRD'><filename>INITRD</filename></link>:
- Indicates a filesystem image to use as an initrd
(optional).
+ Indicates list of filesystem images to concatenate and
use as an initrd (optional).

Missing article: ^ a

Quoted 8 lines

                </para></listitem>
            <listitem><para>
                <link

linkend='var-ROOTFS'><filename>ROOTFS</filename></link>:

documentation/ref-manual/ref-classes.xml +0 -0

@@ -2864,7 +2864,7 @@
        The class supports the following variables:
        <itemizedlist>
            <listitem><para><emphasis><link

Quoted 5 lines

linkend='var-INITRD'><filename>INITRD</filename></link>:</emphasis>
- Indicates a filesystem image to use as an initial RAM
disk
+ Indicates list of filesystem images to concatenate and
use as an initial RAM disk

Missing article: ^ a

Quoted 16 lines

(initrd).
This variable is optional.</para></listitem>
<listitem><para><emphasis><link
linkend='var-ROOTFS'><filename>ROOTFS</filename></link>:</emphasis>
documentation/ref-manual/ref-variables.xml +0 -0
index b4d6e71..16e3ed6 100644
@@ -4020,7 +4020,7 @@ recipes-graphics/xorg-font/font-alias_1.0.3.bb:PR =
"${INC_PR}.3"
        <glossentry id='var-INITRD'><glossterm>INITRD</glossterm>
            <glossdef>
                <para>
documentation/ref-manual/ref-variables.xml +1 -1
-                    Indicates a filesystem image to use as an initial RAM
+                    Indicates list of filesystem images to concatenate

Quoted 1 line

and use as an initial RAM

ditto

Quoted 13 lines

                    disk (<filename>initrd</filename>).
                </para>

meta/classes/boot-directdisk.bbclass +0 -0

index 0da9932..995d3e7 100644
@@ -71,10 +71,17 @@ boot_direct_populate() {
 	# Install bzImage, initrd, and rootfs.img in DEST for all loaders to

use.

	install -m 0644 ${STAGING_KERNEL_DIR}/bzImage $dest/vmlinuz

meta/classes/boot-directdisk.bbclass boot_direct_populate() { · line 74

Quoted 3 lines

meta/classes/boot-directdisk.bbclass +1 -2

-	if [ -n "${INITRD}" ] && [ -s "${INITRD}" ]; then
-		install -m 0644 ${INITRD} $dest/initrd
+	# initrd is made of concatenation of multiple filesystem images

# Assemble the initrd from the list of filesystem images

meta/classes/boot-directdisk.bbclass boot_direct_populate() { · line 79

Quoted 5 lines

meta/classes/boot-directdisk.bbclass +5 -0

+	if [ -n "${INITRD}" ]; then
+		rm -f $dest/initrd
+		for fs in ${INITRD}
+		do
+			if [ -n "${fs}" ] && [ -s "${fs}" ]; then

The -n test is unnecessary here. How would "for fs in ${INITRD}" result in an fs of "" ?

meta/classes/boot-directdisk.bbclass

Quoted 2 lines

meta/classes/boot-directdisk.bbclass +2 -0
+				cat ${fs} >> $dest/ignited
+			fi

Some kind of a warning at least is warranted if a file appears in the INITRD list but is either 0-size or non-existent.

Quoted 29 lines

meta/classes/boot-directdisk.bbclass +2 -0

+		done
+		chmod 0644 $dest/initrd

fi
-
}

build_boot_dd() {

meta/classes/bootimg.bbclass +1 -1

index d52aace..7b3ce65 100644
@@ -18,7 +18,7 @@
 # an hdd)
 
 # External variables (also used by syslinux.bbclass)
-# ${INITRD} - indicates a filesystem image to use as an initrd (optional)
+# ${INITRD} - indicates a list of filesystem images to concatenate and
 use as an initrd (optional)
 # ${COMPRESSISO} - Transparent compress ISO, reduce size ~40% if set to 1
 # ${NOISO}  - skip building the ISO image if set to 1

# ${NOHDD} - skip building the HDD image if set to 1

meta/classes/bootimg.bbclass +0 -0

@@ -67,9 +67,17 @@ populate() {
 
 	# Install bzImage, initrd, and rootfs.img in DEST for all loaders to

use.

	install -m 0644 ${STAGING_KERNEL_DIR}/bzImage ${DEST}/vmlinuz

meta/classes/bootimg.bbclass +0 -3

-
-	if [ -n "${INITRD}" ] && [ -s "${INITRD}" ]; then
-		install -m 0644 ${INITRD} ${DEST}/initrd

meta/classes/bootimg.bbclass populate() { · line 79

Quoted 10 lines

meta/classes/bootimg.bbclass +10 -0

+	
+	# initrd is made of concatenation of multiple filesystem images
+	if [ -n "${INITRD}" ]; then
+		rm -f ${DEST}/initrd
+		for fs in ${INITRD}
+		do
+			if [ -s "${fs}" ]; then
+				cat ${fs} >> ${DEST}/initrd
+			fi
+		done

Same test commentary here.

Quoted 11 lines

meta/classes/bootimg.bbclass +1 -0

+		chmod 0644 ${DEST}/initrd

	fi

	if [ -n "${ROOTFS}" ] && [ -s "${ROOTFS}" ]; then

meta/classes/bootimg.bbclass +0 -1

@@ -80,10 +88,19 @@ populate() {
 
 build_iso() {
 	# Only create an ISO if we have an INITRD and NOISO was not set
-	if [ -z "${INITRD}" ] || [ ! -s "${INITRD}" ] || [ "${NOISO}" = "1" ];

then

meta/classes/bootimg.bbclass +1 -0

+	if [ -z "${INITRD}" ] || [ "${NOISO}" = "1" ]; then

Quoted 3 lines

bbnote "ISO image will not be created." return
Fi

Perhaps the -s test should be replaced with a -s of $dest/initrd?

meta/classes/bootimg.bbclass populate() { · line 102

Quoted 8 lines

meta/classes/bootimg.bbclass +8 -0

+	# ${INITRD} is a list of multiple filesystem images
+	for fs in ${INITRD}
+	do
+		if [ ! -s "${fs}" ]; then
+			bbnote "ISO image will not be created. ${fs} is invalid."
+			return
+		fi
+	done

This additional loop could be eliminated by including this test above. Right? Or am I missing something subtle here?

Quoted 9 lines

+
	populate ${ISODIR}
meta/classes/grub-efi.bbclass +0 -0
index 505d032..47bd35e 100644

meta/classes/grub-efi.bbclass line 11

Quoted 6 lines

meta/classes/grub-efi.bbclass +1 -1

@@ -7,7 +7,7 @@
 # Provide grub-efi specific functions for building bootable images.
 
 # External variables
-# ${INITRD} - indicates a filesystem image to use as an initrd (optional)
+# ${INITRD} - indicates a list of filesystem images to concatenate and
 use as an initrd (optional)

Used the "a" here, good :-)

Quoted 29 lines

meta/classes/grub-efi.bbclass +0 -0
 # ${ROOTFS} - indicates a filesystem image to include as the root
 filesystem (optional)
# ${GRUB_GFXSERIAL} - set this to 1 to have graphics and serial in the boot menu
# ${LABELS} - a list of targets for the automatic config
meta/classes/syslinux.bbclass +1 -1
index b9701bf..d6498d9 100644
@@ -5,7 +5,7 @@
 # Provide syslinux specific functions for building bootable images.
 
 # External variables
-# ${INITRD} - indicates a filesystem image to use as an initrd (optional)
+# ${INITRD} - indicates a list of filesystem images to concatenate and
 use as an initrd (optional)
 # ${ROOTFS} - indicates a filesystem image to include as the root
 filesystem (optional)
# ${AUTO_SYSLINUXMENU} - set this to 1 to enable creating an automatic menu
# ${LABELS} - a list of targets for the automatic config
meta/conf/documentation.conf +0 -0
index 7fa3f31..31fbd6c 100644
@@ -225,7 +225,7 @@ INHIBIT_PACKAGE_STRIP[doc] = "If set to "1", causes
the build to not strip binar
INHERIT[doc] = "Causes the named class to be inherited at this point during parsing. The variable is only valid in configuration files."

Quoted 8 lines

INHERIT_DISTRO[doc] = "Lists classes that will be inherited at the distribution level. It is unlikely that you want to edit this variable." INITRAMFS_FSTYPES[doc] = "Defines the format for the output image of an initial RAM disk (initramfs), which is used during boot." -INITRD[doc] = "Indicates a filesystem image to use as an initial RAM disk (initrd)."
+INITRD[doc] = "Indicates list of filesystem images to concatenate and use as an initial RAM disk (initrd)."

"a list"

Quoted 13 lines

INITSCRIPT_NAME[doc] = "The filename of the initialization script as installed to ${sysconfdir}/init.d."
INITSCRIPT_PACKAGES[doc] = "A list of the packages that contain initscripts. This variable is used in recipes when using update-rc.d.bbclass. The variable is optional and defaults to the PN variable."
INITSCRIPT_PARAMS[doc] = "Specifies the options to pass to update-rc.d. The variable is mandatory and is used in recipes when using update-rc.d.bbclass."
Signature · 1 line
1.8.1.4

Thanks,

Signature · 2 lines

Darren Hart					Open Source Technology Center
[email protected]				            Intel Corporation

Nitin A Kamble

author

2014-08-05 04:33 UTC #3 lore raw

Re: [PATCH 1/1] INITRD var: make it a list of filesystem images

Quoted 9 lines · Darren

Quoted 1 line · Nitin A
From: Nitin A Kamble <[email protected]>
Hi Nitin,
Generally speaking this looks like a good improvement. I don't have any major technical concerns, but we do need to address some grammatical issues in the commit and the docs below to make sure people can follow the intent here.

Thanks Darren for the detailed review. With such review the quality of my writtings will definitely improve.

Quoted 10 lines

Quoted 8 lines
The initrd image used by the Linux kernel is list of file system images concatenated together and presented as a single initrd file at boot time.
So far the initrd is a single filesystem image. But in cases like to support
early microcode loading, the initrd image need to have multiple filesystem images concatenated together.
Consider:

Quoted 8 lines

Currently, the INITRD variable is a single filesystem image. For optional early boot features, such as microcode loading, a modular approach would provide the most flexibility and is minimally invasive. Converting INITRD to a list of images to be concatenated accomplishes this.
Quoted 3 lines
This commit is extending the INITRD variable from a single filesystem image
to a list of filesystem images to satisfy the need mentioned above.
Can now drop this paragraph.

I am fine with your wording. I would add further, that the Linux kernel can already handle initrd images which have multiple filesystem images concatenated together.

Quoted 12 lines

Signed-off-by Nitin A Kamble <[email protected]>

---

documentation/ref-manual/ref-classes.xml   |  4 ++--
documentation/ref-manual/ref-variables.xml |  2 +-
meta/classes/boot-directdisk.bbclass       | 13 ++++++++++---
meta/classes/bootimg.bbclass               | 27
++++++++++++++++++++++-----
meta/classes/grub-efi.bbclass              |  2 +-
meta/classes/syslinux.bbclass              |  2 +-
meta/conf/documentation.conf               |  2 +-
7 files changed, 38 insertions(+), 14 deletions(-)

Quoted 16 lines

Quoted 9 lines
documentation/ref-manual/ref-classes.xml +0 -0
index e7e9942..0bf3546 100644
@@ -881,7 +881,7 @@
         <itemizedlist>
             <listitem><para>
                 <link
Quoted 5 lines
linkend='var-INITRD'><filename>INITRD</filename></link>:
- Indicates a filesystem image to use as an initrd
(optional).
+ Indicates list of filesystem images to concatenate and
use as an initrd (optional).
Missing article: ^ a

I hope I do not do these kind of article mistakes again.

Quoted 53 lines

Quoted 9 lines
                 </para></listitem>
             <listitem><para>
                 <link
linkend='var-ROOTFS'><filename>ROOTFS</filename></link>:
documentation/ref-manual/ref-classes.xml +0 -0
@@ -2864,7 +2864,7 @@
         The class supports the following variables:
         <itemizedlist>
             <listitem><para><emphasis><link
Quoted 5 lines
linkend='var-INITRD'><filename>INITRD</filename></link>:</emphasis>
- Indicates a filesystem image to use as an initial RAM
disk
+ Indicates list of filesystem images to concatenate and
use as an initial RAM disk
Missing article: ^ a
Quoted 16 lines
(initrd).
This variable is optional.</para></listitem>
<listitem><para><emphasis><link
linkend='var-ROOTFS'><filename>ROOTFS</filename></link>:</emphasis>
documentation/ref-manual/ref-variables.xml +0 -0
index b4d6e71..16e3ed6 100644
@@ -4020,7 +4020,7 @@ recipes-graphics/xorg-font/font-alias_1.0.3.bb:PR =
"${INC_PR}.3"
         <glossentry id='var-INITRD'><glossterm>INITRD</glossterm>
             <glossdef>
                 <para>
documentation/ref-manual/ref-variables.xml +1 -1
-                    Indicates a filesystem image to use as an initial RAM
+                    Indicates list of filesystem images to concatenate
Quoted 1 line
and use as an initial RAM
ditto
Quoted 13 lines
                     disk (<filename>initrd</filename>).
                 </para>
meta/classes/boot-directdisk.bbclass +0 -0
index 0da9932..995d3e7 100644
@@ -71,10 +71,17 @@ boot_direct_populate() {
 	# Install bzImage, initrd, and rootfs.img in DEST for all loaders to
use.
	install -m 0644 ${STAGING_KERNEL_DIR}/bzImage $dest/vmlinuz
meta/classes/boot-directdisk.bbclass boot_direct_populate() { · line 74
Quoted 3 lines
meta/classes/boot-directdisk.bbclass +1 -2
-	if [ -n "${INITRD}" ] && [ -s "${INITRD}" ]; then
-		install -m 0644 ${INITRD} $dest/initrd
+	# initrd is made of concatenation of multiple filesystem images
# Assemble the initrd from the list of filesystem images

Quoted 8 lines

meta/classes/boot-directdisk.bbclass boot_direct_populate() { · line 79
Quoted 5 lines
meta/classes/boot-directdisk.bbclass +5 -0
+	if [ -n "${INITRD}" ]; then
+		rm -f $dest/initrd
+		for fs in ${INITRD}
+		do
+			if [ -n "${fs}" ] && [ -s "${fs}" ]; then
The -n test is unnecessary here. How would "for fs in ${INITRD}" result in an fs of "" ?

The -n test is needed here, it checks whether the file exist or not.

Quoted 5 lines

meta/classes/boot-directdisk.bbclass
Quoted 3 lines
meta/classes/boot-directdisk.bbclass +2 -0
+				cat ${fs} >> $dest/ignited
+			fi
Some kind of a warning at least is warranted if a file appears in the INITRD list but is either 0-size or non-existent.

I tried to keep the original style of the code. But it makes sense to add a warning or even an error here.

Quoted 47 lines

Quoted 29 lines

meta/classes/boot-directdisk.bbclass +2 -0

+		done
+		chmod 0644 $dest/initrd

fi
-
}

build_boot_dd() {

meta/classes/bootimg.bbclass +1 -1

index d52aace..7b3ce65 100644
@@ -18,7 +18,7 @@
 # an hdd)
 
 # External variables (also used by syslinux.bbclass)
-# ${INITRD} - indicates a filesystem image to use as an initrd (optional)
+# ${INITRD} - indicates a list of filesystem images to concatenate and
 use as an initrd (optional)
 # ${COMPRESSISO} - Transparent compress ISO, reduce size ~40% if set to 1
 # ${NOISO}  - skip building the ISO image if set to 1

# ${NOHDD} - skip building the HDD image if set to 1

meta/classes/bootimg.bbclass +0 -0

@@ -67,9 +67,17 @@ populate() {
 
 	# Install bzImage, initrd, and rootfs.img in DEST for all loaders to

use.

	install -m 0644 ${STAGING_KERNEL_DIR}/bzImage ${DEST}/vmlinuz

meta/classes/bootimg.bbclass +0 -3

-
-	if [ -n "${INITRD}" ] && [ -s "${INITRD}" ]; then
-		install -m 0644 ${INITRD} ${DEST}/initrd

meta/classes/bootimg.bbclass populate() { · line 79

Quoted 10 lines

meta/classes/bootimg.bbclass +10 -0

+	
+	# initrd is made of concatenation of multiple filesystem images
+	if [ -n "${INITRD}" ]; then
+		rm -f ${DEST}/initrd
+		for fs in ${INITRD}
+		do
+			if [ -s "${fs}" ]; then
+				cat ${fs} >> ${DEST}/initrd
+			fi
+		done

Same test commentary here.

Quoted 6 lines

meta/classes/bootimg.bbclass +1 -0
+		chmod 0644 ${DEST}/initrd
	fi

	if [ -n "${ROOTFS}" ] && [ -s "${ROOTFS}" ]; then
meta/classes/bootimg.bbclass +0 -0
@@ -80,10 +88,19 @@ populate() {
 

Quoted 9 lines

Quoted 8 lines
meta/classes/bootimg.bbclass +0 -1
 build_iso() {
 	# Only create an ISO if we have an INITRD and NOISO was not set
-	if [ -z "${INITRD}" ] || [ ! -s "${INITRD}" ] || [ "${NOISO}" = "1" ];
then
meta/classes/bootimg.bbclass +1 -0
+	if [ -z "${INITRD}" ] || [ "${NOISO}" = "1" ]; then
bbnote "ISO image will not be created." return
Fi
Perhaps the -s test should be replaced with a -s of $dest/initrd?

The -s test is replaced by the loop few lines below.

Quoted 10 lines

meta/classes/bootimg.bbclass populate() { · line 102
Quoted 8 lines
meta/classes/bootimg.bbclass +8 -0
+	# ${INITRD} is a list of multiple filesystem images
+	for fs in ${INITRD}
+	do
+		if [ ! -s "${fs}" ]; then
+			bbnote "ISO image will not be created. ${fs} is invalid."
+			return
+		fi
+	done
This additional loop could be eliminated by including this test above. Right? Or am I missing something subtle here?

That approach will leave a hole where, the function will continue when one of the filesystem image is invalid. So the loop is better here as it does not leave any hole.

Quoted 11 lines

	populate ${ISODIR}

meta/classes/grub-efi.bbclass +0 -0

index 505d032..47bd35e 100644
@@ -7,7 +7,7 @@
 # Provide grub-efi specific functions for building bootable images.

Quoted 5 lines

meta/classes/grub-efi.bbclass
Quoted 4 lines
meta/classes/grub-efi.bbclass +1 -1
 # External variables
-# ${INITRD} - indicates a filesystem image to use as an initrd (optional)
+# ${INITRD} - indicates a list of filesystem images to concatenate and
 use as an initrd (optional)
Used the "a" here, good :-)

That was not a typo :)

Quoted 52 lines

Quoted 29 lines
meta/classes/grub-efi.bbclass +0 -0
 # ${ROOTFS} - indicates a filesystem image to include as the root
 filesystem (optional)
# ${GRUB_GFXSERIAL} - set this to 1 to have graphics and serial in the boot menu
# ${LABELS} - a list of targets for the automatic config
meta/classes/syslinux.bbclass +1 -1
index b9701bf..d6498d9 100644
@@ -5,7 +5,7 @@
 # Provide syslinux specific functions for building bootable images.
 
 # External variables
-# ${INITRD} - indicates a filesystem image to use as an initrd (optional)
+# ${INITRD} - indicates a list of filesystem images to concatenate and
 use as an initrd (optional)
 # ${ROOTFS} - indicates a filesystem image to include as the root
 filesystem (optional)
# ${AUTO_SYSLINUXMENU} - set this to 1 to enable creating an automatic menu
# ${LABELS} - a list of targets for the automatic config
meta/conf/documentation.conf +0 -0
index 7fa3f31..31fbd6c 100644
@@ -225,7 +225,7 @@ INHIBIT_PACKAGE_STRIP[doc] = "If set to "1", causes
the build to not strip binar
INHERIT[doc] = "Causes the named class to be inherited at this point during parsing. The variable is only valid in configuration files."
Quoted 8 lines
INHERIT_DISTRO[doc] = "Lists classes that will be inherited at the distribution level. It is unlikely that you want to edit this variable." INITRAMFS_FSTYPES[doc] = "Defines the format for the output image of an initial RAM disk (initramfs), which is used during boot." -INITRD[doc] = "Indicates a filesystem image to use as an initial RAM disk (initrd)."
+INITRD[doc] = "Indicates list of filesystem images to concatenate and use as an initial RAM disk (initrd)."
"a list"
Quoted 13 lines
INITSCRIPT_NAME[doc] = "The filename of the initialization script as installed to ${sysconfdir}/init.d."
INITSCRIPT_PACKAGES[doc] = "A list of the packages that contain initscripts. This variable is used in recipes when using update-rc.d.bbclass. The variable is optional and defaults to the PN variable."
INITSCRIPT_PARAMS[doc] = "Specifies the options to pass to update-rc.d. The variable is mandatory and is used in recipes when using update-rc.d.bbclass."
Signature · 1 line
1.8.1.4

Quoted 2 lines

Thanks,

Thanks Darren for the review. I think I can improve myself with it.

Nitin

Hart, Darren

2014-08-05 16:52 UTC #4 lore raw

Re: [PATCH 1/1] INITRD var: make it a list of filesystem images

Quoted 3 lines · Nitin A

Quoted 1 line · Darren
On 7/29/14, 11:34, "Kamble, Nitin A" <[email protected]> wrote:

...

Quoted 11 lines

Quoted 10 lines
meta/classes/boot-directdisk.bbclass boot_direct_populate() { · line 79
Quoted 6 lines
meta/classes/boot-directdisk.bbclass +5 -0
+	if [ -n "${INITRD}" ]; then
+		rm -f $dest/initrd
+		for fs in ${INITRD}
+		do
+			if [ -n "${fs}" ] && [ -s "${fs}" ]; then
The -n test is unnecessary here. How would "for fs in ${INITRD}" result in
an fs of "" ?
The -n test is needed here, it checks whether the file exist or not.

Nope, -n tests if the string length is non-zero. See the bash manual section "CONDITIONAL EXPRESSIONS". -s tests if the file exists and has a size > 0.

Quoted 9 lines

Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +2 -0
+				cat ${fs} >> $dest/ignited
+			fi
Some kind of a warning at least is warranted if a file appears in the INITRD list but is either 0-size or non-existent.
I tried to keep the original style of the code. But it makes sense to add a warning or even an error here.

Style is fine, but error checking is a functional thing. If it was missing before, it was a bug.

Quoted 22 lines

Quoted 11 lines
Quoted 10 lines
build_iso() {
# Only create an ISO if we have an INITRD and NOISO was not set
(quoted diff) +0 -1
-	if [ -z "${INITRD}" ] || [ ! -s "${INITRD}" ] || [ "${NOISO}" = "1"
];
then
meta/classes/bootimg.bbclass +1 -0
+	if [ -z "${INITRD}" ] || [ "${NOISO}" = "1" ]; then
bbnote "ISO image will not be created." return
Fi
Perhaps the -s test should be replaced with a -s of $dest/initrd?
The -s test is replaced by the loop few lines below.
Quoted 10 lines
meta/classes/bootimg.bbclass populate() { · line 102
Quoted 8 lines
meta/classes/bootimg.bbclass +8 -0
+	# ${INITRD} is a list of multiple filesystem images
+	for fs in ${INITRD}
+	do
+		if [ ! -s "${fs}" ]; then
+			bbnote "ISO image will not be created. ${fs} is invalid."
+			return
+		fi
+	done
This additional loop could be eliminated by including this test above. Right? Or am I missing something subtle here?

Quoted 3 lines

That approach will leave a hole where, the function will continue when one of the filesystem image is invalid.
So the loop is better here as it does not leave any hole.

But you've already built it right? So you have already tested for -s ${fs} previously. The only thing that matters now is that the assembled image is valid. $dest/initrd. Right?

Signature · 2 lines

Darren Hart					Open Source Technology Center
[email protected]				            Intel Corporation

Nitin A Kamble

author

2014-08-05 17:11 UTC #5 lore raw

Re: [PATCH 1/1] INITRD var: make it a list of filesystem images

Quoted 14 lines · Darren

Quoted 2 lines · Nitin A
Quoted 1 line · Darren
On 7/29/14, 11:34, "Kamble, Nitin A" <[email protected]> wrote:
...
Quoted 8 lines
Quoted 8 lines
meta/classes/boot-directdisk.bbclass boot_direct_populate() { · line 79
Quoted 5 lines
meta/classes/boot-directdisk.bbclass +5 -0
+	if [ -n "${INITRD}" ]; then
+		rm -f $dest/initrd
+		for fs in ${INITRD}
+		do
+			if [ -n "${fs}" ] && [ -s "${fs}" ]; then
The -n test is unnecessary here. How would "for fs in ${INITRD}" result in
an fs of "" ?

Quoted 5 lines

Quoted 1 line
The -n test is needed here, it checks whether the file exist or not.
Nope, -n tests if the string length is non-zero. See the bash manual section "CONDITIONAL EXPRESSIONS". -s tests if the file exists and has a size > 0.

You are right. it is "-N" which checks for presence of the file.

Quoted 10 lines

Quoted 7 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+				cat ${fs} >> $dest/ignited
+			fi
Some kind of a warning at least is warranted if a file appears in the INITRD list but is either 0-size or non-existent.
I tried to keep the original style of the code. But it makes sense to add a warning or even an error here.
Style is fine, but error checking is a functional thing. If it was missing before, it was a bug.

Noted.

Quoted 21 lines

Quoted 21 lines
Quoted 10 lines
Quoted 9 lines
build_iso() {
# Only create an ISO if we have an INITRD and NOISO was not set
(quoted diff) +0 -1
-	if [ -z "${INITRD}" ] || [ ! -s "${INITRD}" ] || [ "${NOISO}" = "1"
];
then
meta/classes/bootimg.bbclass +1 -0
+	if [ -z "${INITRD}" ] || [ "${NOISO}" = "1" ]; then
bbnote "ISO image will not be created." return
Fi
Perhaps the -s test should be replaced with a -s of $dest/initrd?
The -s test is replaced by the loop few lines below.
Quoted 10 lines
meta/classes/bootimg.bbclass populate() { · line 102
Quoted 8 lines
meta/classes/bootimg.bbclass +8 -0
+	# ${INITRD} is a list of multiple filesystem images
+	for fs in ${INITRD}
+	do
+		if [ ! -s "${fs}" ]; then
+			bbnote "ISO image will not be created. ${fs} is invalid."
+			return
+		fi
+	done
This additional loop could be eliminated by including this test above. Right? Or am I missing something subtle here?

Quoted 7 lines

Quoted 3 lines
That approach will leave a hole where, the function will continue when one of the filesystem image is invalid.
So the loop is better here as it does not leave any hole.
But you've already built it right? So you have already tested for -s ${fs} previously. The only thing that matters now is that the assembled image is valid. $dest/initrd. Right?

No, the dest/initrd is not built at this point. It will be built in the populate function which is called later. so that check will always fail wrongly.

I also notice that RP has pulled in part of the commit already, hence I will be making another commit to address the feedback.

Thanks,
Nitin