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

[Nitin A Kamble <nitin.a.kamble@intel.com>]

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