On 8/4/2014 9:38 AM, Hart, Darren wrote:
On 7/29/14, 11:34, "Kamble, Nitin A" <nitin.a.kam...@intel.com> wrote:
From: Nitin A Kamble <nitin.a.kam...@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.kam...@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
--
_______________________________________________
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core
