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

["Hart, Darren" <darren.hart@intel.com>]

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.


>
>>
>>> +				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.

>>>
>>> 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?

-- 
Darren Hart					Open Source Technology Center
darren.hart@intel.com				            Intel Corporation