Patchwork [U-Boot] squash build warning in cmd_sf.c

login
register
mail settings
Submitter prabhakar.csengg@gmail.com
Date Oct. 4, 2011, 11:45 a.m.
Message ID <1317728725-6054-1-git-send-email-prabhakar.csengg@gmail.com>
Download mbox | patch
Permalink /patch/117605/
State Changes Requested
Headers show

Comments

prabhakar.csengg@gmail.com - Oct. 4, 2011, 11:45 a.m.
From: Prabhakar Lad <prabhakar.csengg@gmail.com>

Fix build warning and returning early in case of failure

cmd_sf.c: In function 'do_spi_flash':
cmd_sf.c:164: warning: 'skipped' may be used uninitialized in this function
cmd_sf.c:164: note: 'skipped' was declared here

Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
---
 common/cmd_sf.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)
Simon Glass - Oct. 4, 2011, 1:33 p.m.
Hi,

On Tue, Oct 4, 2011 at 4:45 AM,  <prabhakar.csengg@gmail.com> wrote:
> From: Prabhakar Lad <prabhakar.csengg@gmail.com>
>
> Fix build warning and returning early in case of failure
>
> cmd_sf.c: In function 'do_spi_flash':
> cmd_sf.c:164: warning: 'skipped' may be used uninitialized in this function
> cmd_sf.c:164: note: 'skipped' was declared here
>
> Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>

There is another patch on the list for this - your one changes the
behavior I think.

> ---
>  common/cmd_sf.c |   20 +++++++++-----------
>  1 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index c8c547a..bdf7915 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -164,21 +164,19 @@ static int spi_flash_update(struct spi_flash *flash, u32 offset,
>        size_t skipped;         /* statistics */
>
>        cmp_buf = malloc(flash->sector_size);
> -       if (cmp_buf) {
> -               for (skipped = 0; buf < end && !err_oper;
> -                               buf += todo, offset += todo) {
> -                       todo = min(end - buf, flash->sector_size);
> -                       err_oper = spi_flash_update_block(flash, offset, todo,
> -                                       buf, cmp_buf, &skipped);
> -               }
> -       } else {
> +       if (!cmp_buf) {
>                err_oper = "malloc";
> -       }
> -       free(cmp_buf);
> -       if (err_oper) {
>                printf("SPI flash failed in %s step\n", err_oper);
>                return 1;
>        }
> +
> +       for (skipped = 0; buf < end && !err_oper;
> +                       buf += todo, offset += todo) {
> +               todo = min(end - buf, flash->sector_size);
> +               err_oper = spi_flash_update_block(flash, offset, todo,
> +                               buf, cmp_buf, &skipped);
> +       }
> +       free(cmp_buf);

Here we might have err_open, but it is never printed.

Regards,
Simon

>        printf("%zu bytes written, %zu bytes skipped\n", len - skipped,
>               skipped);
>        return 0;
> --
> 1.7.0.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
prabhakar.csengg@gmail.com - Oct. 4, 2011, 2:08 p.m.
Hi Simon,



On Tue, Oct 4, 2011 at 7:03 PM, Simon Glass <sjg@chromium.org> wrote:

> Hi,
>
> On Tue, Oct 4, 2011 at 4:45 AM,  <prabhakar.csengg@gmail.com> wrote:
> > From: Prabhakar Lad <prabhakar.csengg@gmail.com>
> >
> > Fix build warning and returning early in case of failure
> >
> > cmd_sf.c: In function 'do_spi_flash':
> > cmd_sf.c:164: warning: 'skipped' may be used uninitialized in this
> function
> > cmd_sf.c:164: note: 'skipped' was declared here
> >
> > Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
>
> There is another patch on the list for this - your one changes the
> behavior I think.
>
    yes I agree there is patch in the list. can you be more elaborated
    in what sense does it change the behavior??

>
> > ---
> >  common/cmd_sf.c |   20 +++++++++-----------
> >  1 files changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> > index c8c547a..bdf7915 100644
> > --- a/common/cmd_sf.c
> > +++ b/common/cmd_sf.c
> > @@ -164,21 +164,19 @@ static int spi_flash_update(struct spi_flash
> *flash, u32 offset,
> >        size_t skipped;         /* statistics */
> >
> >        cmp_buf = malloc(flash->sector_size);
> > -       if (cmp_buf) {
> > -               for (skipped = 0; buf < end && !err_oper;
> > -                               buf += todo, offset += todo) {
> > -                       todo = min(end - buf, flash->sector_size);
> > -                       err_oper = spi_flash_update_block(flash, offset,
> todo,
> > -                                       buf, cmp_buf, &skipped);
> > -               }
> > -       } else {
> > +       if (!cmp_buf) {
> >                err_oper = "malloc";
> > -       }
> > -       free(cmp_buf);
> > -       if (err_oper) {
> >                printf("SPI flash failed in %s step\n", err_oper);
> >                return 1;
> >        }
> > +
> > +       for (skipped = 0; buf < end && !err_oper;
> > +                       buf += todo, offset += todo) {
> > +               todo = min(end - buf, flash->sector_size);
> > +               err_oper = spi_flash_update_block(flash, offset, todo,
> > +                               buf, cmp_buf, &skipped);
> > +       }
> > +       free(cmp_buf);
>
> Here we might have err_open, but it is never printed.
>
 ditto.

>
> Regards,
> Simon
>
> >        printf("%zu bytes written, %zu bytes skipped\n", len - skipped,
> >               skipped);
> >        return 0;
> > --
> > 1.7.0.4
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >
>

Regards
Prabhakar Lad
Simon Glass - Oct. 4, 2011, 3:15 p.m.
Hi,

On Tue, Oct 4, 2011 at 4:45 AM,  <prabhakar.csengg@gmail.com> wrote:
> From: Prabhakar Lad <prabhakar.csengg@gmail.com>
>
> Fix build warning and returning early in case of failure
>
> cmd_sf.c: In function 'do_spi_flash':
> cmd_sf.c:164: warning: 'skipped' may be used uninitialized in this function
> cmd_sf.c:164: note: 'skipped' was declared here
>
> Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
> ---
>  common/cmd_sf.c |   20 +++++++++-----------
>  1 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index c8c547a..bdf7915 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -164,21 +164,19 @@ static int spi_flash_update(struct spi_flash *flash, u32 offset,
>        size_t skipped;         /* statistics */
>
>        cmp_buf = malloc(flash->sector_size);
> -       if (cmp_buf) {
> -               for (skipped = 0; buf < end && !err_oper;
> -                               buf += todo, offset += todo) {
> -                       todo = min(end - buf, flash->sector_size);
> -                       err_oper = spi_flash_update_block(flash, offset, todo,
> -                                       buf, cmp_buf, &skipped);
> -               }
> -       } else {
> +       if (!cmp_buf) {
>                err_oper = "malloc";
> -       }
> -       free(cmp_buf);
> -       if (err_oper) {
>                printf("SPI flash failed in %s step\n", err_oper);
>                return 1;
>        }
> +
> +       for (skipped = 0; buf < end && !err_oper;
> +                       buf += todo, offset += todo) {
> +               todo = min(end - buf, flash->sector_size);
> +               err_oper = spi_flash_update_block(flash, offset, todo,
> +                               buf, cmp_buf, &skipped);
> +       }
> +       free(cmp_buf);

Sorry my previous comment was in the wrong place. My comment was that
here err_oper might be set, but the printf() that uses it is above it!

Regards,
Simon

>        printf("%zu bytes written, %zu bytes skipped\n", len - skipped,
>               skipped);
>        return 0;
> --
> 1.7.0.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
prabhakar.csengg@gmail.com - Oct. 5, 2011, 4:46 a.m.
Hi Simon

On Tue, Oct 4, 2011 at 8:45 PM, Simon Glass <sjg@chromium.org> wrote:

> Hi,
>
> On Tue, Oct 4, 2011 at 4:45 AM,  <prabhakar.csengg@gmail.com> wrote:
> > From: Prabhakar Lad <prabhakar.csengg@gmail.com>
> >
> > Fix build warning and returning early in case of failure
> >
> > cmd_sf.c: In function 'do_spi_flash':
> > cmd_sf.c:164: warning: 'skipped' may be used uninitialized in this
> function
> > cmd_sf.c:164: note: 'skipped' was declared here
> >
> > Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
> > ---
> >  common/cmd_sf.c |   20 +++++++++-----------
> >  1 files changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> > index c8c547a..bdf7915 100644
> > --- a/common/cmd_sf.c
> > +++ b/common/cmd_sf.c
> > @@ -164,21 +164,19 @@ static int spi_flash_update(struct spi_flash
> *flash, u32 offset,
> >        size_t skipped;         /* statistics */
> >
> >        cmp_buf = malloc(flash->sector_size);
> > -       if (cmp_buf) {
> > -               for (skipped = 0; buf < end && !err_oper;
> > -                               buf += todo, offset += todo) {
> > -                       todo = min(end - buf, flash->sector_size);
> > -                       err_oper = spi_flash_update_block(flash, offset,
> todo,
> > -                                       buf, cmp_buf, &skipped);
> > -               }
> > -       } else {
> > +       if (!cmp_buf) {
> >                err_oper = "malloc";
> > -       }
> > -       free(cmp_buf);
> > -       if (err_oper) {
> >                printf("SPI flash failed in %s step\n", err_oper);
> >                return 1;
> >        }
> > +
> > +       for (skipped = 0; buf < end && !err_oper;
> > +                       buf += todo, offset += todo) {
> > +               todo = min(end - buf, flash->sector_size);
> > +               err_oper = spi_flash_update_block(flash, offset, todo,
> > +                               buf, cmp_buf, &skipped);
> > +       }
> > +       free(cmp_buf);
>
> Sorry my previous comment was in the wrong place. My comment was that
> here err_oper might be set, but the printf() that uses it is above it!
>
>    if  the err_oper is set in the loop why isn't it checked in loop each
time
  why is it that it is only checked for the last index? Is that a loophole??

Regards
-- Prabhakar Lad

> Regards,
> Simon
>
> >        printf("%zu bytes written, %zu bytes skipped\n", len - skipped,
> >               skipped);
> >        return 0;
> > --
> > 1.7.0.4
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >
>
prabhakar.csengg@gmail.com - Oct. 5, 2011, 4:52 a.m.
hi Simon

got it its in the for condition
for (skipped = 0; buf < end && !err_oper;
                  buf += todo, offset += todo)

Ive cleared my doubt, you were rite.

Regards
--Prabhakar Lad


On Wed, Oct 5, 2011 at 10:16 AM, Prabhakar Lad
<prabhakar.csengg@gmail.com>wrote:

> Hi Simon
>
> On Tue, Oct 4, 2011 at 8:45 PM, Simon Glass <sjg@chromium.org> wrote:
>
>> Hi,
>>
>> On Tue, Oct 4, 2011 at 4:45 AM,  <prabhakar.csengg@gmail.com> wrote:
>> > From: Prabhakar Lad <prabhakar.csengg@gmail.com>
>> >
>> > Fix build warning and returning early in case of failure
>> >
>> > cmd_sf.c: In function 'do_spi_flash':
>> > cmd_sf.c:164: warning: 'skipped' may be used uninitialized in this
>> function
>> > cmd_sf.c:164: note: 'skipped' was declared here
>> >
>> > Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
>> > ---
>> >  common/cmd_sf.c |   20 +++++++++-----------
>> >  1 files changed, 9 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/common/cmd_sf.c b/common/cmd_sf.c
>> > index c8c547a..bdf7915 100644
>> > --- a/common/cmd_sf.c
>> > +++ b/common/cmd_sf.c
>> > @@ -164,21 +164,19 @@ static int spi_flash_update(struct spi_flash
>> *flash, u32 offset,
>> >        size_t skipped;         /* statistics */
>> >
>> >        cmp_buf = malloc(flash->sector_size);
>> > -       if (cmp_buf) {
>> > -               for (skipped = 0; buf < end && !err_oper;
>> > -                               buf += todo, offset += todo) {
>> > -                       todo = min(end - buf, flash->sector_size);
>> > -                       err_oper = spi_flash_update_block(flash, offset,
>> todo,
>> > -                                       buf, cmp_buf, &skipped);
>> > -               }
>> > -       } else {
>> > +       if (!cmp_buf) {
>> >                err_oper = "malloc";
>> > -       }
>> > -       free(cmp_buf);
>> > -       if (err_oper) {
>> >                printf("SPI flash failed in %s step\n", err_oper);
>> >                return 1;
>> >        }
>> > +
>> > +       for (skipped = 0; buf < end && !err_oper;
>> > +                       buf += todo, offset += todo) {
>> > +               todo = min(end - buf, flash->sector_size);
>> > +               err_oper = spi_flash_update_block(flash, offset, todo,
>> > +                               buf, cmp_buf, &skipped);
>> > +       }
>> > +       free(cmp_buf);
>>
>> Sorry my previous comment was in the wrong place. My comment was that
>> here err_oper might be set, but the printf() that uses it is above it!
>>
>>    if  the err_oper is set in the loop why isn't it checked in loop each
> time
>   why is it that it is only checked for the last index? Is that a
> loophole??
>
> Regards
> -- Prabhakar Lad
>
>> Regards,
>> Simon
>>
>> >        printf("%zu bytes written, %zu bytes skipped\n", len - skipped,
>> >               skipped);
>> >        return 0;
>> > --
>> > 1.7.0.4
>> >
>> > _______________________________________________
>> > U-Boot mailing list
>> > U-Boot@lists.denx.de
>> > http://lists.denx.de/mailman/listinfo/u-boot
>> >
>>
>
>
Simon Glass - Oct. 5, 2011, 7:32 a.m.
On Tue, Oct 4, 2011 at 9:52 PM, Prabhakar Lad <prabhakar.csengg@gmail.com>wrote:

> hi Simon
>
> got it its in the for condition
>
> for (skipped = 0; buf < end && !err_oper;
>                   buf += todo, offset += todo)
>
> Ive cleared my doubt, you were rite.
>

OK good, thanks.

Regards,
Simon


>
> Regards
> --Prabhakar Lad
>
>
>
> On Wed, Oct 5, 2011 at 10:16 AM, Prabhakar Lad <prabhakar.csengg@gmail.com
> > wrote:
>
>> Hi Simon
>>
>> On Tue, Oct 4, 2011 at 8:45 PM, Simon Glass <sjg@chromium.org> wrote:
>>
>>> Hi,
>>>
>>> On Tue, Oct 4, 2011 at 4:45 AM,  <prabhakar.csengg@gmail.com> wrote:
>>> > From: Prabhakar Lad <prabhakar.csengg@gmail.com>
>>> >
>>> > Fix build warning and returning early in case of failure
>>> >
>>> > cmd_sf.c: In function 'do_spi_flash':
>>> > cmd_sf.c:164: warning: 'skipped' may be used uninitialized in this
>>> function
>>> > cmd_sf.c:164: note: 'skipped' was declared here
>>> >
>>> > Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
>>> > ---
>>> >  common/cmd_sf.c |   20 +++++++++-----------
>>> >  1 files changed, 9 insertions(+), 11 deletions(-)
>>> >
>>> > diff --git a/common/cmd_sf.c b/common/cmd_sf.c
>>> > index c8c547a..bdf7915 100644
>>> > --- a/common/cmd_sf.c
>>> > +++ b/common/cmd_sf.c
>>> > @@ -164,21 +164,19 @@ static int spi_flash_update(struct spi_flash
>>> *flash, u32 offset,
>>> >        size_t skipped;         /* statistics */
>>> >
>>> >        cmp_buf = malloc(flash->sector_size);
>>> > -       if (cmp_buf) {
>>> > -               for (skipped = 0; buf < end && !err_oper;
>>> > -                               buf += todo, offset += todo) {
>>> > -                       todo = min(end - buf, flash->sector_size);
>>> > -                       err_oper = spi_flash_update_block(flash,
>>> offset, todo,
>>> > -                                       buf, cmp_buf, &skipped);
>>> > -               }
>>> > -       } else {
>>> > +       if (!cmp_buf) {
>>> >                err_oper = "malloc";
>>> > -       }
>>> > -       free(cmp_buf);
>>> > -       if (err_oper) {
>>> >                printf("SPI flash failed in %s step\n", err_oper);
>>> >                return 1;
>>> >        }
>>> > +
>>> > +       for (skipped = 0; buf < end && !err_oper;
>>> > +                       buf += todo, offset += todo) {
>>> > +               todo = min(end - buf, flash->sector_size);
>>> > +               err_oper = spi_flash_update_block(flash, offset, todo,
>>> > +                               buf, cmp_buf, &skipped);
>>> > +       }
>>> > +       free(cmp_buf);
>>>
>>> Sorry my previous comment was in the wrong place. My comment was that
>>> here err_oper might be set, but the printf() that uses it is above it!
>>>
>>>    if  the err_oper is set in the loop why isn't it checked in loop each
>> time
>>   why is it that it is only checked for the last index? Is that a
>> loophole??
>>
>> Regards
>> -- Prabhakar Lad
>>
>>> Regards,
>>> Simon
>>>
>>> >        printf("%zu bytes written, %zu bytes skipped\n", len - skipped,
>>> >               skipped);
>>> >        return 0;
>>> > --
>>> > 1.7.0.4
>>> >
>>> > _______________________________________________
>>> > U-Boot mailing list
>>> > U-Boot@lists.denx.de
>>> > http://lists.denx.de/mailman/listinfo/u-boot
>>> >
>>>
>>
>>
>

Patch

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index c8c547a..bdf7915 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -164,21 +164,19 @@  static int spi_flash_update(struct spi_flash *flash, u32 offset,
 	size_t skipped;		/* statistics */
 
 	cmp_buf = malloc(flash->sector_size);
-	if (cmp_buf) {
-		for (skipped = 0; buf < end && !err_oper;
-				buf += todo, offset += todo) {
-			todo = min(end - buf, flash->sector_size);
-			err_oper = spi_flash_update_block(flash, offset, todo,
-					buf, cmp_buf, &skipped);
-		}
-	} else {
+	if (!cmp_buf) {
 		err_oper = "malloc";
-	}
-	free(cmp_buf);
-	if (err_oper) {
 		printf("SPI flash failed in %s step\n", err_oper);
 		return 1;
 	}
+
+	for (skipped = 0; buf < end && !err_oper;
+			buf += todo, offset += todo) {
+		todo = min(end - buf, flash->sector_size);
+		err_oper = spi_flash_update_block(flash, offset, todo,
+				buf, cmp_buf, &skipped);
+	}
+	free(cmp_buf);
 	printf("%zu bytes written, %zu bytes skipped\n", len - skipped,
 	       skipped);
 	return 0;