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

login
register
mail settings
Submitter Nitin A Kamble
Date July 29, 2014, 6:34 p.m.
Message ID <b60eac03f169389484bcc40fdfc3e4e12cb7934b.1406658528.git.nitin.a.kamble@intel.com>
Download mbox | patch
Permalink /patch/76847/
State Accepted
Commit 3b19f90bdfca520a6e00057cacb103f8a55feac6
Headers show

Comments

Nitin A Kamble - July 29, 2014, 6:34 p.m.
From: Nitin A Kamble <nitin.a.kamble@intel.com>

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 <nitin.a.kamble@intel.com>
---
 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(-)
Darren Hart - Aug. 4, 2014, 4:38 p.m.
On 7/29/14, 11:34, "Kamble, Nitin A" <nitin.a.kamble@intel.com> wrote:

>From: Nitin A Kamble <nitin.a.kamble@intel.com>

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.

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

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

>
>Signed-off-by: Nitin A Kamble <nitin.a.kamble@intel.com>
>---
> 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(-)
>
>diff --git a/documentation/ref-manual/ref-classes.xml
>b/documentation/ref-manual/ref-classes.xml
>index e7e9942..0bf3546 100644
>--- a/documentation/ref-manual/ref-classes.xml
>+++ b/documentation/ref-manual/ref-classes.xml
>@@ -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).


Missing article:            ^ a


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

Missing article:             ^ a

>                 (initrd).
>                 This variable is optional.</para></listitem>
>             <listitem><para><emphasis><link
>linkend='var-ROOTFS'><filename>ROOTFS</filename></link>:</emphasis>
>diff --git a/documentation/ref-manual/ref-variables.xml
>b/documentation/ref-manual/ref-variables.xml
>index b4d6e71..16e3ed6 100644
>--- a/documentation/ref-manual/ref-variables.xml
>+++ b/documentation/ref-manual/ref-variables.xml
>@@ -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

ditto

>                     disk (<filename>initrd</filename>).
>                 </para>
> 
>diff --git a/meta/classes/boot-directdisk.bbclass
>b/meta/classes/boot-directdisk.bbclass
>index 0da9932..995d3e7 100644
>--- a/meta/classes/boot-directdisk.bbclass
>+++ b/meta/classes/boot-directdisk.bbclass
>@@ -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

# Assemble the initrd from the list of filesystem images

>+	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 "" ?


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

>+		done
>+		chmod 0644 $dest/initrd
> 	fi
>-
> }
> 
> build_boot_dd() {
>diff --git a/meta/classes/bootimg.bbclass b/meta/classes/bootimg.bbclass
>index d52aace..7b3ce65 100644
>--- a/meta/classes/bootimg.bbclass
>+++ b/meta/classes/bootimg.bbclass
>@@ -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

Same test commentary here.

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

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

>+	# ${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?

>+
> 
> 	populate ${ISODIR}
> 
>diff --git a/meta/classes/grub-efi.bbclass b/meta/classes/grub-efi.bbclass
>index 505d032..47bd35e 100644
>--- a/meta/classes/grub-efi.bbclass
>+++ b/meta/classes/grub-efi.bbclass
>@@ -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 :-)

> # ${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
>diff --git a/meta/classes/syslinux.bbclass b/meta/classes/syslinux.bbclass
>index b9701bf..d6498d9 100644
>--- a/meta/classes/syslinux.bbclass
>+++ b/meta/classes/syslinux.bbclass
>@@ -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
>diff --git a/meta/conf/documentation.conf b/meta/conf/documentation.conf
>index 7fa3f31..31fbd6c 100644
>--- a/meta/conf/documentation.conf
>+++ b/meta/conf/documentation.conf
>@@ -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)."

"a list"

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

Thanks,
Nitin A Kamble - Aug. 5, 2014, 4:33 a.m.
On 8/4/2014 9:38 AM, Hart, Darren wrote:
> On 7/29/14, 11:34, "Kamble, Nitin A" <nitin.a.kamble@intel.com> wrote:
>
>> From: Nitin A Kamble <nitin.a.kamble@intel.com>
> 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.

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

>> Signed-off-by: Nitin A Kamble <nitin.a.kamble@intel.com>
>> ---
>> 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(-)
>>
>> diff --git a/documentation/ref-manual/ref-classes.xml
>> b/documentation/ref-manual/ref-classes.xml
>> index e7e9942..0bf3546 100644
>> --- a/documentation/ref-manual/ref-classes.xml
>> +++ b/documentation/ref-manual/ref-classes.xml
>> @@ -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).
>
> Missing article:            ^ a
I hope I do not do these kind of article mistakes again.

>
>>                  </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
> Missing article:             ^ a
>
>>                  (initrd).
>>                  This variable is optional.</para></listitem>
>>              <listitem><para><emphasis><link
>> linkend='var-ROOTFS'><filename>ROOTFS</filename></link>:</emphasis>
>> diff --git a/documentation/ref-manual/ref-variables.xml
>> b/documentation/ref-manual/ref-variables.xml
>> index b4d6e71..16e3ed6 100644
>> --- a/documentation/ref-manual/ref-variables.xml
>> +++ b/documentation/ref-manual/ref-variables.xml
>> @@ -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
> ditto
>
>>                      disk (<filename>initrd</filename>).
>>                  </para>
>>
>> diff --git a/meta/classes/boot-directdisk.bbclass
>> b/meta/classes/boot-directdisk.bbclass
>> index 0da9932..995d3e7 100644
>> --- a/meta/classes/boot-directdisk.bbclass
>> +++ b/meta/classes/boot-directdisk.bbclass
>> @@ -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
> # Assemble the initrd from the list of filesystem images
>
>> +	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.

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

>> +		done
>> +		chmod 0644 $dest/initrd
>> 	fi
>> -
>> }
>>
>> build_boot_dd() {
>> diff --git a/meta/classes/bootimg.bbclass b/meta/classes/bootimg.bbclass
>> index d52aace..7b3ce65 100644
>> --- a/meta/classes/bootimg.bbclass
>> +++ b/meta/classes/bootimg.bbclass
>> @@ -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
> Same test commentary here.
>
>> +		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
> Perhaps the -s test should be replaced with a -s of $dest/initrd?
The -s test is replaced by the loop few lines below.
>> +	# ${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.
>> +
>>
>> 	populate ${ISODIR}
>>
>> diff --git a/meta/classes/grub-efi.bbclass b/meta/classes/grub-efi.bbclass
>> index 505d032..47bd35e 100644
>> --- a/meta/classes/grub-efi.bbclass
>> +++ b/meta/classes/grub-efi.bbclass
>> @@ -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 :-)
That was not a typo :)

>> # ${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
>> diff --git a/meta/classes/syslinux.bbclass b/meta/classes/syslinux.bbclass
>> index b9701bf..d6498d9 100644
>> --- a/meta/classes/syslinux.bbclass
>> +++ b/meta/classes/syslinux.bbclass
>> @@ -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
>> diff --git a/meta/conf/documentation.conf b/meta/conf/documentation.conf
>> index 7fa3f31..31fbd6c 100644
>> --- a/meta/conf/documentation.conf
>> +++ b/meta/conf/documentation.conf
>> @@ -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)."
> "a list"
>
>> 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."
>> -- 
>> 1.8.1.4
>>
>>
> Thanks,
>
Thanks Darren for the review. I think I can improve myself with it.

Nitin
Darren Hart - Aug. 5, 2014, 4:45 p.m.
On 8/4/14, 21:33, "Kamble, Nitin A" <nitin.a.kamble@intel.com> wrote:

>
>On 8/4/2014 9:38 AM, Hart, Darren wrote:
>> On 7/29/14, 11:34, "Kamble, Nitin A" <nitin.a.kamble@intel.com> wrote:

...

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


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

>>>
>>> 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
>> Perhaps the -s test should be replaced with a -s of $dest/initrd?
>The -s test is replaced by the loop few lines below.
>>> +	# ${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.


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?
Nitin A Kamble - Aug. 5, 2014, 5:05 p.m.
On 8/5/2014 9:45 AM, Hart, Darren wrote:
> On 8/4/14, 21:33, "Kamble, Nitin A" <nitin.a.kamble@intel.com> wrote:
>
>> On 8/4/2014 9:38 AM, Hart, Darren wrote:
>>> On 7/29/14, 11:34, "Kamble, Nitin A" <nitin.a.kamble@intel.com> wrote:
> ...
>
>>>> +	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.
You are right. it is "-N" which checks for presence of the file.

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

>>>> 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
>>> Perhaps the -s test should be replaced with a -s of $dest/initrd?
>> The -s test is replaced by the loop few lines below.
>>>> +	# ${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.
>
> 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

Patch

diff --git a/documentation/ref-manual/ref-classes.xml b/documentation/ref-manual/ref-classes.xml
index e7e9942..0bf3546 100644
--- a/documentation/ref-manual/ref-classes.xml
+++ b/documentation/ref-manual/ref-classes.xml
@@ -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>
diff --git a/documentation/ref-manual/ref-variables.xml b/documentation/ref-manual/ref-variables.xml
index b4d6e71..16e3ed6 100644
--- a/documentation/ref-manual/ref-variables.xml
+++ b/documentation/ref-manual/ref-variables.xml
@@ -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>
 
diff --git a/meta/classes/boot-directdisk.bbclass b/meta/classes/boot-directdisk.bbclass
index 0da9932..995d3e7 100644
--- a/meta/classes/boot-directdisk.bbclass
+++ b/meta/classes/boot-directdisk.bbclass
@@ -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() {
diff --git a/meta/classes/bootimg.bbclass b/meta/classes/bootimg.bbclass
index d52aace..7b3ce65 100644
--- a/meta/classes/bootimg.bbclass
+++ b/meta/classes/bootimg.bbclass
@@ -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}
 
diff --git a/meta/classes/grub-efi.bbclass b/meta/classes/grub-efi.bbclass
index 505d032..47bd35e 100644
--- a/meta/classes/grub-efi.bbclass
+++ b/meta/classes/grub-efi.bbclass
@@ -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
diff --git a/meta/classes/syslinux.bbclass b/meta/classes/syslinux.bbclass
index b9701bf..d6498d9 100644
--- a/meta/classes/syslinux.bbclass
+++ b/meta/classes/syslinux.bbclass
@@ -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
diff --git a/meta/conf/documentation.conf b/meta/conf/documentation.conf
index 7fa3f31..31fbd6c 100644
--- a/meta/conf/documentation.conf
+++ b/meta/conf/documentation.conf
@@ -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."