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