[OE-core] [PATCH 1/1] INITRD var: make it a list of filesystem images
Nitin A Kamble
nitin.a.kamble at intel.com
Tue Aug 5 04:33:24 UTC 2014
On 8/4/2014 9:38 AM, Hart, Darren wrote:
> On 7/29/14, 11:34, "Kamble, Nitin A" <nitin.a.kamble at intel.com> wrote:
>
>> From: Nitin A Kamble <nitin.a.kamble at 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 at 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
More information about the Openembedded-core
mailing list