tools: fread does not return negative on error

size_t res cannot be less than 0. fread returns 0 on error.

Signed-off-by Roel Kluin <[email protected]>

---
Is this correct? please review.

tools/perf/util/strbuf.c +0 -0

index eaba093..376a337 100644

Quoted 11 lines · Roel Kluin

size_t res cannot be less than 0. fread returns 0 on error.

Signed-off-by Roel Kluin <[email protected]>

---
Is this correct? please review.

tools/perf/util/strbuf.c +0 -0

index eaba093..376a337 100644

tools/perf/util/strbuf.c size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f) · line 264

Quoted 7 lines

tools/perf/util/strbuf.c +1 -1

@@ -259,7 +259,7 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
       res = fread(sb->buf + sb->len, 1, size, f);
       if (res > 0)
               strbuf_setlen(sb, sb->len + res);
-     else if (res < 0 && oldalloc == 0)
+     else if (res == 0 && oldalloc == 0)
               strbuf_release(sb);
       return res;

This comes straight from Git's strbuf.c so i've Cc:-ed the Git list.

Roel, did you get some compiler warning that made you look at this code?

       Ingo

the following patch is for git. I just removed the unneeded check for res == 0 from your version. Does it look OK?

From: Roel Kluin <[email protected]>

size_t res cannot be less than 0. fread returns 0 on error.

Reported-by Ingo Molnar <[email protected]>

Signed-off-by Roel Kluin <[email protected]>

---
 strbuf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

strbuf.c +1 -1

index a884960..f03d117 100644
@@ -260,7 +260,7 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 	res = fread(sb->buf + sb->len, 1, size, f);
 	if (res > 0)
 		strbuf_setlen(sb, sb->len + res);
-	else if (res < 0 && oldalloc == 0)
+	else if (oldalloc == 0)
 		strbuf_release(sb);
 	return res;
 }

Quoted 2 lines · René Scharfe

the following patch is for git. I just removed the unneeded check for res == 0 from your version. Does it look OK?

The patch looks good, and both of our in-tree users do error out when the returned value is 0 (imap-send.c checks with "<= 0" which looks a tad amateurish, though) correctly.

Funny, there is no caller of this function in the original context this bug originally found, which I think is linux-2.6/tools/perf ;-).

One bookeeping issue: i found few explicit credits in those files - so i noted in the changelog that i took them from Git and i noted the specific upstream Git sha1 when i copied them. Would be nice to update each file with names to make credit more explicit:

(quoted diff) +0 -23

-rw-rw-r-- 1 mingo mingo  2808 2009-06-23 10:49 abspath.c
-rw-rw-r-- 1 mingo mingo  1447 2009-06-23 10:49 alias.c
-rw-rw-r-- 1 mingo mingo  4660 2009-06-23 10:49 cache.h
-rw-rw-r-- 1 mingo mingo  4817 2009-06-23 10:49 color.c
-rw-rw-r-- 1 mingo mingo  1187 2009-06-23 10:49 color.h
-rw-rw-r-- 1 mingo mingo 19149 2009-06-23 10:49 config.c
-rw-rw-r-- 1 mingo mingo  1041 2009-06-23 10:52 ctype.c
-rw-rw-r-- 1 mingo mingo   256 2009-06-23 10:49 environment.c
-rw-rw-r-- 1 mingo mingo  3262 2009-06-23 10:49 exec_cmd.c
-rw-rw-r-- 1 mingo mingo   496 2009-06-23 10:49 exec_cmd.h
-rw-rw-r-- 1 mingo mingo  8515 2009-06-23 10:49 help.c
-rw-rw-r-- 1 mingo mingo   751 2009-06-23 10:49 help.h
-rw-rw-r-- 1 mingo mingo  2592 2009-06-23 10:49 levenshtein.c
-rw-rw-r-- 1 mingo mingo   201 2009-06-23 10:49 levenshtein.h
-rw-rw-r-- 1 mingo mingo  1909 2009-06-23 10:49 pager.c
-rw-rw-r-- 1 mingo mingo 12454 2009-06-23 10:49 parse-options.c
-rw-rw-r-- 1 mingo mingo  5693 2009-06-23 10:49 parse-options.h
-rw-rw-r-- 1 mingo mingo  7986 2009-06-23 10:49 path.c
-rw-rw-r-- 1 mingo mingo 10442 2009-06-23 10:49 quote.c
-rw-rw-r-- 1 mingo mingo  2667 2009-06-23 10:49 quote.h
-rw-rw-r-- 1 mingo mingo  7966 2009-06-23 10:49 run-command.c
-rw-rw-r-- 1 mingo mingo  2838 2009-06-23 10:49 run-command.h
-rw-rw-r-- 1 mingo mingo   969 2009-06-23 10:49 sigchain.c

This here script:

-- snip --

for file in abspath.c alias.c cache.h color.c color.h config.c ctype.c \
	environment.c exec_cmd.c exec_cmd.h help.c help.h levenshtein.c \
	levenshtein.h pager.c parse-options.c parse-options.h path.c \
	quote.c quote.h run-command.c run-command.h sigchain.c sigchain.h \
	strbuf.c strbuf.h string.c string.h symbol.c symbol.h usage.c \
	util.h wrapper.c

do
echo $file
git shortlog -n -s $file | head -n 2
done

outputs this (note that a few files you mentioned are not in git.git):

abspath.c
2 Junio C Hamano
1 Dmitry Potapov

Quoted 9 lines · Johannes Schindelin

This here script:

-- snip --

for file in abspath.c alias.c cache.h color.c color.h config.c ctype.c \
	environment.c exec_cmd.c exec_cmd.h help.c help.h levenshtein.c \
	levenshtein.h pager.c parse-options.c parse-options.h path.c \
	quote.c quote.h run-command.c run-command.h sigchain.c sigchain.h \
	strbuf.c strbuf.h string.c string.h symbol.c symbol.h usage.c \
	util.h wrapper.c

Quoted 4 lines

do
echo $file
git shortlog -n -s $file | head -n 2
done

I have thought about suggesting this myself, and your output for many of the files matched my intuition, but some were grossly off, so I checked.

The above procedure counts commits, and a one liner "s/char \*/const &/" weighs as heavily as the patch that implemented the whole thing, for a file that was done in one commit almost perfectly except that it needed a small constness fix. Summarizing output from "blame" for each file may give you a more meaningful results:

    # timestamp
    ts='[12][0-9][0-9][0-9]-[0-9][0-9]-[0-3][0-9] ..:..:.. [-+]....'
    # linenum
    lno='[1-9][0-9]*'
    git blame "$file" |
    sed -e 's/^[^ ]*  *(\([^)]*[^ ]\)  *'"$ts  *$lno"').*/\1/' |
    sort |
    uniq -c |
    sort -r -n

For example, I do not think it is fair to credit me for abspath.c more than Dmitry like this:

Quoted 5 lines

outputs this (note that a few files you mentioned are not in git.git):

abspath.c
2 Junio C Hamano
1 Dmitry Potapov

Initially Dmitry introduced this file with 5b8e6f8 (shrink git-shell by avoiding redundant dependencies, 2008-06-28) at 68 lines. J6t added 36 lines for add_path() with 10c4c88 (Allow add_path() to add non-existent directories to the path, 2008-07-21), I added 12 lines to add a new function with 90b4a71 (is_directory(): a generic helper function, 2008-09-09) and then added a two-liner out-of-bounds-then-die check in 737e31a (make_absolute_path(): check bounds when seeing an overlong symlink, 2008-12-17).

Okay, a script similar to what you propose shows this:

From 1b173f77dd0d5fd4f0ff18034aaa79e30da068b9 Mon Sep 17 00:00:00 2001

2009/6/24 Ingo Molnar <[email protected]>:

tools/perf/CREDITS line 9

Quoted 4 lines

tools/perf/CREDITS new file +4 -0

+Here is an (incomplete!) list of main contributors to those files
+in util/* and elsewhere:
+
+ Alex Riesen

Wow. But I actually think it may be more fair to sort the list after any kind of rating (even something as dumb as commit count).

As everyone one the list knows, I am nowhere near a "major contributor" to the project, as the first place may imply to a casual reader.

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · René Scharfe

the following patch is for git.  I just removed the unneeded check for res == 0 from your version.  Does it look OK?

Quoted 7 lines

The patch looks good, and both of our in-tree users do error out when the returned value is 0 (imap-send.c checks with "<= 0" which looks a tad amateurish, though) correctly.

Funny, there is no caller of this function in the original context this bug originally found, which I think is linux-2.6/tools/perf ;-).

Hehe, yes :-)

Background: when creating tools/perf/ i cherry-picked all the nice Git libraries into tools/perf/util/, to give a standard environment for all tooling things that might come up in the future.

Some of those are not used yet but it looked more logical to pick up whole pieces - some already gained uses. For example config.c is not truly used yet, but very much expected to have a role in the future.

( The only invasive thing i had to do was the s/git_/perf_/ mass rename across all the files - having 'git_' in perf looked quite confusing. )

And our general experience with the Git libraries in tools/perf/util/* is: we love them!

For example parse-options.c is a striking improvement compared to getopt.h we used before, and all the other facilities are sane and straight to the point as well. So in this sense 'perf' is an ... interesting cross-discipline 'fork' of Git's generic libraries.

The auto-generation of everything out of Documentation/*.txt is another thing we picked up, and that's very nice too.

One bookeeping issue: i found few explicit credits in those files - so i noted in the changelog that i took them from Git and i noted the specific upstream Git sha1 when i copied them. Would be nice to update each file with names to make credit more explicit:

From http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=tree;f=tools/perf;hb=HEAD

Hi,

Quoted 45 lines · Ingo Molnar

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · René Scharfe

the following patch is for git.  I just removed the unneeded check for res == 0 from your version.  Does it look OK?

Quoted 7 lines

The patch looks good, and both of our in-tree users do error out when the returned value is 0 (imap-send.c checks with "<= 0" which looks a tad amateurish, though) correctly.

Funny, there is no caller of this function in the original context this bug originally found, which I think is linux-2.6/tools/perf ;-).

Hehe, yes :-)

Background: when creating tools/perf/ i cherry-picked all the nice Git libraries into tools/perf/util/, to give a standard environment for all tooling things that might come up in the future.

Some of those are not used yet but it looked more logical to pick up whole pieces - some already gained uses. For example config.c is not truly used yet, but very much expected to have a role in the future.

( The only invasive thing i had to do was the s/git_/perf_/ mass rename across all the files - having 'git_' in perf looked quite confusing. )

And our general experience with the Git libraries in tools/perf/util/* is: we love them!

For example parse-options.c is a striking improvement compared to getopt.h we used before, and all the other facilities are sane and straight to the point as well. So in this sense 'perf' is an ... interesting cross-discipline 'fork' of Git's generic libraries.

The auto-generation of everything out of Documentation/*.txt is another thing we picked up, and that's very nice too.

One bookeeping issue: i found few explicit credits in those files - so i noted in the changelog that i took them from Git and i noted the specific upstream Git sha1 when i copied them. Would be nice to update each file with names to make credit more explicit:

Quoted 1 line

From http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=tree;f=tools/perf;hb=HEAD

it looks like there may be some other files like builtin-help.c (and perhaps some files in perf/Documentation/ too though there should be some AUTHOR information already in them).

And our general experience with the Git libraries in tools/perf/util/* is: we love them!

For example parse-options.c is a striking improvement compared to getopt.h we used before, and all the other facilities are sane and straight to the point as well. So in this sense 'perf' is an ... interesting cross-discipline 'fork' of Git's generic libraries.