| Message ID | 1317728725-6054-1-git-send-email-prabhakar.csengg@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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 >
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
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 >
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 > > >
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
>> >
>>
>
>
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 >>> > >>> >> >> >
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;