| Message ID | 1414185952-2227-1-git-send-email-andreas.devel@googlemail.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
Hello Andreas, On 24-10-14 23:25, andreas.devel@googlemail.com wrote: > From: Andreas Bießmann <andreas.devel@googlemail.com> > > The two error checks for image_boot_mode_id and image_nand_ecc_mode_id where > wrong and would never fail, fix that! > > This was detected by Apple's clang compiler: > ---8<--- > HOSTCC tools/kwbimage.o > tools/kwbimage.c:553:20: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare] > if (el->bootfrom < 0) { > ~~~~~~~~~~~~ ^ ~ > tools/kwbimage.c:571:23: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare] > if (el->nandeccmode < 0) { > ~~~~~~~~~~~~~~~ ^ ~ > 2 warnings generated. > --->8--- > > Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> > --- > > tools/kwbimage.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/tools/kwbimage.c b/tools/kwbimage.c > index 42870ed..8fd70ef 100644 > --- a/tools/kwbimage.c > +++ b/tools/kwbimage.c > @@ -548,13 +548,14 @@ static int image_create_config_parse_oneline(char *line, > el->version = atoi(value); > } else if (!strcmp(keyword, "BOOT_FROM")) { > char *value = strtok_r(NULL, deliminiters, &saveptr); > - el->type = IMAGE_CFG_BOOT_FROM; > - el->bootfrom = image_boot_mode_id(value); > - if (el->bootfrom < 0) { > + int ret = image_boot_mode_id(value); > + if (ret < 0) { > fprintf(stderr, > "Invalid boot media '%s'\n", value); > return -1; > } > + el->type = IMAGE_CFG_BOOT_FROM; > + el->bootfrom = ret; > } else if (!strcmp(keyword, "NAND_BLKSZ")) { > char *value = strtok_r(NULL, deliminiters, &saveptr); > el->type = IMAGE_CFG_NAND_BLKSZ; > @@ -566,13 +567,14 @@ static int image_create_config_parse_oneline(char *line, > strtoul(value, NULL, 16); > } else if (!strcmp(keyword, "NAND_ECC_MODE")) { > char *value = strtok_r(NULL, deliminiters, &saveptr); > - el->type = IMAGE_CFG_NAND_ECC_MODE; > - el->nandeccmode = image_nand_ecc_mode_id(value); > - if (el->nandeccmode < 0) { > + int ret = image_nand_ecc_mode_id(value); Thanks for fixing this. Could you move the int ret declaration before actual code though? Regards, Jeroen
Hello Andreas, On 27-10-14 13:06, Jeroen Hofstee wrote: > Hello Andreas, > > On 24-10-14 23:25, andreas.devel@googlemail.com wrote: >> From: Andreas Bießmann <andreas.devel@googlemail.com> >> >> The two error checks for image_boot_mode_id and >> image_nand_ecc_mode_id where >> wrong and would never fail, fix that! >> >> This was detected by Apple's clang compiler: >> ---8<--- >> HOSTCC tools/kwbimage.o >> tools/kwbimage.c:553:20: warning: comparison of unsigned expression < >> 0 is always false [-Wtautological-compare] >> if (el->bootfrom < 0) { >> ~~~~~~~~~~~~ ^ ~ >> tools/kwbimage.c:571:23: warning: comparison of unsigned expression < >> 0 is always false [-Wtautological-compare] >> if (el->nandeccmode < 0) { >> ~~~~~~~~~~~~~~~ ^ ~ >> 2 warnings generated. >> --->8--- >> >> Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> >> --- >> >> tools/kwbimage.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/tools/kwbimage.c b/tools/kwbimage.c >> index 42870ed..8fd70ef 100644 >> --- a/tools/kwbimage.c >> +++ b/tools/kwbimage.c >> @@ -548,13 +548,14 @@ static int >> image_create_config_parse_oneline(char *line, >> el->version = atoi(value); >> } else if (!strcmp(keyword, "BOOT_FROM")) { >> char *value = strtok_r(NULL, deliminiters, &saveptr); >> - el->type = IMAGE_CFG_BOOT_FROM; >> - el->bootfrom = image_boot_mode_id(value); >> - if (el->bootfrom < 0) { >> + int ret = image_boot_mode_id(value); >> + if (ret < 0) { >> fprintf(stderr, >> "Invalid boot media '%s'\n", value); >> return -1; >> } >> + el->type = IMAGE_CFG_BOOT_FROM; >> + el->bootfrom = ret; >> } else if (!strcmp(keyword, "NAND_BLKSZ")) { >> char *value = strtok_r(NULL, deliminiters, &saveptr); >> el->type = IMAGE_CFG_NAND_BLKSZ; >> @@ -566,13 +567,14 @@ static int >> image_create_config_parse_oneline(char *line, >> strtoul(value, NULL, 16); >> } else if (!strcmp(keyword, "NAND_ECC_MODE")) { >> char *value = strtok_r(NULL, deliminiters, &saveptr); >> - el->type = IMAGE_CFG_NAND_ECC_MODE; >> - el->nandeccmode = image_nand_ecc_mode_id(value); >> - if (el->nandeccmode < 0) { >> + int ret = image_nand_ecc_mode_id(value); > > Thanks for fixing this. Could you move the int ret declaration before > actual code though? Ah, I see now, you were not adding the declaration in the middle of the code. There is just a newline missing after the declaration, which make the patch look like it does. This was already the case in the original code, so Acked-By: Jeroen Hofstee <jeroen@myspectrum.nl> Regards, Jeroen
ping? It is http://patchwork.ozlabs.org/patch/403237/ On 13.11.14 22:42, Jeroen Hofstee wrote: > Hello Andreas, > > On 27-10-14 13:06, Jeroen Hofstee wrote: >> Hello Andreas, >> >> On 24-10-14 23:25, andreas.devel@googlemail.com wrote: >>> From: Andreas Bießmann <andreas.devel@googlemail.com> >>> >>> The two error checks for image_boot_mode_id and >>> image_nand_ecc_mode_id where >>> wrong and would never fail, fix that! >>> >>> This was detected by Apple's clang compiler: >>> ---8<--- >>> HOSTCC tools/kwbimage.o >>> tools/kwbimage.c:553:20: warning: comparison of unsigned expression < >>> 0 is always false [-Wtautological-compare] >>> if (el->bootfrom < 0) { >>> ~~~~~~~~~~~~ ^ ~ >>> tools/kwbimage.c:571:23: warning: comparison of unsigned expression < >>> 0 is always false [-Wtautological-compare] >>> if (el->nandeccmode < 0) { >>> ~~~~~~~~~~~~~~~ ^ ~ >>> 2 warnings generated. >>> --->8--- >>> >>> Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> >>> --- >>> >>> tools/kwbimage.c | 14 ++++++++------ >>> 1 file changed, 8 insertions(+), 6 deletions(-) >>> >>> diff --git a/tools/kwbimage.c b/tools/kwbimage.c >>> index 42870ed..8fd70ef 100644 >>> --- a/tools/kwbimage.c >>> +++ b/tools/kwbimage.c >>> @@ -548,13 +548,14 @@ static int >>> image_create_config_parse_oneline(char *line, >>> el->version = atoi(value); >>> } else if (!strcmp(keyword, "BOOT_FROM")) { >>> char *value = strtok_r(NULL, deliminiters, &saveptr); >>> - el->type = IMAGE_CFG_BOOT_FROM; >>> - el->bootfrom = image_boot_mode_id(value); >>> - if (el->bootfrom < 0) { >>> + int ret = image_boot_mode_id(value); >>> + if (ret < 0) { >>> fprintf(stderr, >>> "Invalid boot media '%s'\n", value); >>> return -1; >>> } >>> + el->type = IMAGE_CFG_BOOT_FROM; >>> + el->bootfrom = ret; >>> } else if (!strcmp(keyword, "NAND_BLKSZ")) { >>> char *value = strtok_r(NULL, deliminiters, &saveptr); >>> el->type = IMAGE_CFG_NAND_BLKSZ; >>> @@ -566,13 +567,14 @@ static int >>> image_create_config_parse_oneline(char *line, >>> strtoul(value, NULL, 16); >>> } else if (!strcmp(keyword, "NAND_ECC_MODE")) { >>> char *value = strtok_r(NULL, deliminiters, &saveptr); >>> - el->type = IMAGE_CFG_NAND_ECC_MODE; >>> - el->nandeccmode = image_nand_ecc_mode_id(value); >>> - if (el->nandeccmode < 0) { >>> + int ret = image_nand_ecc_mode_id(value); >> >> Thanks for fixing this. Could you move the int ret declaration before >> actual code though? > > Ah, I see now, you were not adding the declaration in the middle of > the code. There is just a newline missing after the declaration, > which make the patch look like it does. This was already the > case in the original code, so > > Acked-By: Jeroen Hofstee <jeroen@myspectrum.nl> > > Regards, > Jeroen
On Fri, Oct 24, 2014 at 11:25:52PM +0200, Andreas Bießmann wrote: > From: Andreas Bießmann <andreas.devel@googlemail.com> > > The two error checks for image_boot_mode_id and image_nand_ecc_mode_id where > wrong and would never fail, fix that! > > This was detected by Apple's clang compiler: Applied to u-boot/master, thanks!
diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 42870ed..8fd70ef 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -548,13 +548,14 @@ static int image_create_config_parse_oneline(char *line, el->version = atoi(value); } else if (!strcmp(keyword, "BOOT_FROM")) { char *value = strtok_r(NULL, deliminiters, &saveptr); - el->type = IMAGE_CFG_BOOT_FROM; - el->bootfrom = image_boot_mode_id(value); - if (el->bootfrom < 0) { + int ret = image_boot_mode_id(value); + if (ret < 0) { fprintf(stderr, "Invalid boot media '%s'\n", value); return -1; } + el->type = IMAGE_CFG_BOOT_FROM; + el->bootfrom = ret; } else if (!strcmp(keyword, "NAND_BLKSZ")) { char *value = strtok_r(NULL, deliminiters, &saveptr); el->type = IMAGE_CFG_NAND_BLKSZ; @@ -566,13 +567,14 @@ static int image_create_config_parse_oneline(char *line, strtoul(value, NULL, 16); } else if (!strcmp(keyword, "NAND_ECC_MODE")) { char *value = strtok_r(NULL, deliminiters, &saveptr); - el->type = IMAGE_CFG_NAND_ECC_MODE; - el->nandeccmode = image_nand_ecc_mode_id(value); - if (el->nandeccmode < 0) { + int ret = image_nand_ecc_mode_id(value); + if (ret < 0) { fprintf(stderr, "Invalid NAND ECC mode '%s'\n", value); return -1; } + el->type = IMAGE_CFG_NAND_ECC_MODE; + el->nandeccmode = ret; } else if (!strcmp(keyword, "NAND_PAGE_SIZE")) { char *value = strtok_r(NULL, deliminiters, &saveptr); el->type = IMAGE_CFG_NAND_PAGESZ;