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

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

[Stefan Roese]

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(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 1120e9b..91db5d7 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -752,14 +752,26 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
 	}
 
 	version = image_get_version();
-	/* Fallback to version 0 is no version is provided in the cfg file */
-	if (version == -1)
-		version = 0;
-
-	if (version == 0)
+	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);
-	else if (version == 1)
+		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);
+	}
 
 	if (!image) {
 		fprintf(stderr, "Could not create image\n");
-- 
2.1.2

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Wenn Du ein' weise Antwort verlangst, Mu?t Du vern?nftig fragen.
                                                -- Goethe, Invektiven

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

[Stefan Roese]

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Anything free is worth what you pay for it.

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

[Stefan Roese]

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