Messages in this thread | | | Date | Thu, 10 Jan 2013 08:12:24 -0800 | From | Randy Dunlap <> | Subject | Re: [PATCH] Fix wrong EOF compare |
| |
On 01/10/13 07:26, Michal Nazarewicz wrote: > On Thu, Jan 10 2013, Minchan Kim <minchan@kernel.org> wrote: >> getc returns "int" so EOF could be -1 but storing getc's return >> value to char directly makes the vaule to 255 so below condition >> is always false. > > Technically, this is implementation defined and I believe on many > systems char is signed thus the loop will end on EOF or byte 255. > > Either way, my point is the patch is correct, but the comment is not. ;)
and change spelling of 'vaule' to 'value' and test build it please.
> > Of course, even better if the function just used fgets(), ie. something > like: > > int read_block(char *buf, int buf_size, FILE *fin) > { > char *curr = buf, *const buf_end = buf + buf_size; > > while (buf_end - curr > 1 && fgets(curr, buf_end - curr, fin)) { > if (*curr == '\n') /* empty line */ > return curr - buf; > curr += strlen(curr); > } > > return -1; /* EOF or no space left in buf. */ > } > > which is much shorter and does not have buffer overflow issues. > >> It happens in my ARM system so loop is not ended, then segfaulted. >> This patch fixes it. >> >> *curr = getc(fin); // *curr = 255 >> if (*curr == EOF) return -1; // if ( 255 == -1) >> >> Cc: Mel Gorman <mgorman@suse.de> >> Cc: Andy Whitcroft <apw@shadowen.org> >> Cc: Alexander Nyberg <alexn@dsv.su.se> >> Signed-off-by: Minchan Kim <minchan@kernel.org> >> --- >> Documentation/page_owner.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c >> index f0156e1..b777fb6 100644 >> --- a/Documentation/page_owner.c >> +++ b/Documentation/page_owner.c >> @@ -32,12 +32,14 @@ int read_block(char *buf, FILE *fin) >> { >> int ret = 0; >> int hit = 0; >> + int vaule; >> char *curr = buf; >> >> for (;;) { >> - *curr = getc(fin); >> - if (*curr == EOF) return -1; >> + value = getc(fin); >> + if (value == EOF) return -1; >> >> + *curr = value; >> ret++; >> if (*curr == '\n' && hit == 1) >> return ret - 1; > > >
-- ~Randy
| |