[U-Boot] tools/kwbimage: Fix compilation warning

This patch fixes a compilation warning of kwbimage.c:

tools/kwbimage.c: In function ‘kwbimage_set_header’:
tools/kwbimage.c:784:8: warning: ‘headersz’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  memcpy(ptr, image, headersz);
        ^
Instead of using multiple if statements, use a switch statement with
a default entry. And return with error if the VERSION field is
missing in the cfg file.

Signed-off-by: Stefan Roese <sr@denx.de>
---
 tools/kwbimage.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

Dear Stefan,

In message <1414412953-25615-1-git-send-email-sr@denx.de> you wrote:
> This patch fixes a compilation warning of kwbimage.c:
> 
> tools/kwbimage.c: In function ‘kwbimage_set_header’:
> tools/kwbimage.c:784:8: warning: ‘headersz’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   memcpy(ptr, image, headersz);
>         ^
> Instead of using multiple if statements, use a switch statement with
> a default entry. And return with error if the VERSION field is
> missing in the cfg file.

Thanks, but...

> +	switch (version) {
> +		/*
> +		 * Fallback to version 0 is no version is provided in the
> +		 * cfg file
> +		 */
> +	case -1:
> +	case 0:
>  		image = image_create_v0(&headersz, params, sbuf->st_size);
> +		break;
> +
> +	case 1:
>  		image = image_create_v1(&headersz, params, sbuf->st_size);
> +		break;
> +
> +	default:
> +		fprintf(stderr, "File %s does not have the VERSION field\n",
> +			params->imagename);
> +		free(image_cfg);
> +		exit(EXIT_FAILURE);
> +	}

What exactly is the difference between return code -1 (no version is
provided and you fall back to using version 0), and the default case?

To me these look the same?

Best regards,

Wolfgang Denk

Hi Wolfgang,

On 27.10.2014 13:57, Wolfgang Denk wrote:
>> tools/kwbimage.c: In function ‘kwbimage_set_header’:
>> tools/kwbimage.c:784:8: warning: ‘headersz’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>    memcpy(ptr, image, headersz);
>>          ^
>> Instead of using multiple if statements, use a switch statement with
>> a default entry. And return with error if the VERSION field is
>> missing in the cfg file.
>
> Thanks, but...
>
>> +	switch (version) {
>> +		/*
>> +		 * Fallback to version 0 is no version is provided in the
>> +		 * cfg file
>> +		 */
>> +	case -1:
>> +	case 0:
>>   		image = image_create_v0(&headersz, params, sbuf->st_size);
>> +		break;
>> +
>> +	case 1:
>>   		image = image_create_v1(&headersz, params, sbuf->st_size);
>> +		break;
>> +
>> +	default:
>> +		fprintf(stderr, "File %s does not have the VERSION field\n",
>> +			params->imagename);
>> +		free(image_cfg);
>> +		exit(EXIT_FAILURE);
>> +	}
>
> What exactly is the difference between return code -1 (no version is
> provided and you fall back to using version 0), and the default case?
>
> To me these look the same?

Perhaps the error message is a bit misleading. The "default:" case is 
for unsupported versions. E.g. if "VERSION 3" would have been provided 
in the cfg file.

Should I rephrase the error message in the next patch version? To 
something like this:

	fprintf(stderr, "Unsupported VERSION %d\n", version);

Thanks,
Stefan

Dear Stefan,

In message <544E4412.60603@denx.de> you wrote:
> 
> >> +	switch (version) {
> >> +		/*
> >> +		 * Fallback to version 0 is no version is provided in the
> >> +		 * cfg file
> >> +		 */
> >> +	case -1:
...

> > What exactly is the difference between return code -1 (no version is
> > provided and you fall back to using version 0), and the default case?
> >
> > To me these look the same?
> 
> Perhaps the error message is a bit misleading. The "default:" case is 
> for unsupported versions. E.g. if "VERSION 3" would have been provided 
> in the cfg file.
> 
> Should I rephrase the error message in the next patch version? To 
> something like this:

In this case not the error message is incorrect, but the comment
above is misleading.  Instead of "no version is provided" it should
probably read "if no supported version is provided" (note: please
also fix the typo, i.e. "if" instead of "is").

Best regards,

Wolfgang Denk

Hi Wolfgang,

On 27.10.2014 14:34, Wolfgang Denk wrote:
>>>> +	switch (version) {
>>>> +		/*
>>>> +		 * Fallback to version 0 is no version is provided in the
>>>> +		 * cfg file
>>>> +		 */
>>>> +	case -1:
> ...
>
>>> What exactly is the difference between return code -1 (no version is
>>> provided and you fall back to using version 0), and the default case?
>>>
>>> To me these look the same?
>>
>> Perhaps the error message is a bit misleading. The "default:" case is
>> for unsupported versions. E.g. if "VERSION 3" would have been provided
>> in the cfg file.
>>
>> Should I rephrase the error message in the next patch version? To
>> something like this:
>
> In this case not the error message is incorrect, but the comment
> above is misleading.  Instead of "no version is provided" it should
> probably read "if no supported version is provided" (note: please
> also fix the typo, i.e. "if" instead of "is").

The fallback in the comment above refers to "version == -1". As the 
function image_get_version() returns -1 if no VERSION string is found in 
the config file. So I think the comment is quite accurate.

Thanks,
Stefan