[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 17:05:25 UTC 2014
On 8/5/2014 9:45 AM, Hart, Darren wrote:
> On 8/4/14, 21:33, "Kamble, Nitin A" <nitin.a.kamble at 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 at 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
More information about the Openembedded-core
mailing list