* [PATCH 0/5] Be careful about lstat()-vs-readlink() @ 2008-12-17 18:42 Linus Torvalds 2008-12-17 18:42 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Linus Torvalds 0 siblings, 1 reply; 33+ messages in thread From: Linus Torvalds @ 2008-12-17 18:42 UTC (permalink / raw) To: Junio C Hamano, Git Mailing List This series of five patches makes us a lot more careful about readlink(), and in particular it avoids the code that depends on doing an lstat(), and then using st_size as the length of the link. That's what POSIX says _should_ happen, but Ramon Tayag reported git failing on NTFS under Linux because st_size doesn't match readlink() return value. It doesn't seem to be unknown elsewhere either, since coreutils (through gnulib's areadlink_with_size) also has a big compatibility layer around readlink(). This series has been 'tested' by running it with the appended totally hacky patch that fakes out the lstat() st_size values for symlinks, so it has actually had some testing. The complete series does: Linus Torvalds (5): Add generic 'strbuf_readlink()' helper function Make 'ce_compare_link()' use the new 'strbuf_readlink()' Make 'index_path()' use 'strbuf_readlink()' Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Make 'prepare_temp_file()' ignore st_size for symlinks builtin-apply.c | 6 ++---- diff.c | 25 +++++++++++-------------- read-cache.c | 22 ++++++++-------------- sha1_file.c | 14 +++++--------- strbuf.c | 28 ++++++++++++++++++++++++++++ strbuf.h | 1 + 6 files changed, 55 insertions(+), 41 deletions(-) and the hacky test-patch (which is obviously _not_ meant to be applied) is as follows... Linus --- diff --git a/cache.h b/cache.h index 231c06d..2d85fca 100644 --- a/cache.h +++ b/cache.h @@ -943,4 +943,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix); char *alias_lookup(const char *alias); int split_cmdline(char *cmdline, const char ***argv); +extern int gitlstat(const char *, struct stat *); +#define lstat gitlstat + #endif /* CACHE_H */ diff --git a/read-cache.c b/read-cache.c index b1475ff..defbb20 100644 --- a/read-cache.c +++ b/read-cache.c @@ -15,6 +15,16 @@ #include "revision.h" #include "blob.h" +#undef lstat +int gitlstat(const char *path, struct stat *buf) +{ + int retval = lstat(path, buf); + if (!retval && S_ISLNK(buf->st_mode)) + buf->st_size = 1; + return retval; +} +#define lstat gitlstat + /* Index extensions. * * The first letter should be 'A'..'Z' for extensions that are not ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 1/5] Add generic 'strbuf_readlink()' helper function 2008-12-17 18:42 [PATCH 0/5] Be careful about lstat()-vs-readlink() Linus Torvalds @ 2008-12-17 18:42 ` Linus Torvalds 2008-12-17 18:43 ` [PATCH 2/5] Make 'ce_compare_link()' use the new 'strbuf_readlink()' Linus Torvalds ` (2 more replies) 0 siblings, 3 replies; 33+ messages in thread From: Linus Torvalds @ 2008-12-17 18:42 UTC (permalink / raw) To: Junio C Hamano, Git Mailing List From: Linus Torvalds <torvalds@linux-foundation.org> Date: Wed, 17 Dec 2008 09:36:40 -0800 It was already what 'git apply' did in read_old_data(), just export it as a real function, and make it be more generic. In particular, this handles the case of the lstat() st_size data not matching the readlink() return value properly (which apparently happens at least on NTFS under Linux). But as a result of this you could also use the new function without even knowing how big the link is going to be, and it will allocate an appropriately sized buffer. So we pass in the st_size of the link as just a hint, rather than a fixed requirement. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> --- builtin-apply.c | 6 ++---- strbuf.c | 28 ++++++++++++++++++++++++++++ strbuf.h | 1 + 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index 4c4d1e1..07244b0 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -1559,10 +1559,8 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf) { switch (st->st_mode & S_IFMT) { case S_IFLNK: - strbuf_grow(buf, st->st_size); - if (readlink(path, buf->buf, st->st_size) != st->st_size) - return -1; - strbuf_setlen(buf, st->st_size); + if (strbuf_readlink(buf, path, st->st_size) < 0) + return error("unable to read symlink %s", path); return 0; case S_IFREG: if (strbuf_read_file(buf, path, st->st_size) != st->st_size) diff --git a/strbuf.c b/strbuf.c index 13be67e..904a2b0 100644 --- a/strbuf.c +++ b/strbuf.c @@ -288,6 +288,34 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint) return sb->len - oldlen; } +#define STRBUF_MAXLINK (2*PATH_MAX) + +int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint) +{ + if (hint < 32) + hint = 32; + + while (hint < STRBUF_MAXLINK) { + int len; + + strbuf_grow(sb, hint); + len = readlink(path, sb->buf, hint); + if (len < 0) { + if (errno != ERANGE) + break; + } else if (len < hint) { + strbuf_setlen(sb, len); + return 0; + } + + /* .. the buffer was too small - try again */ + hint *= 2; + continue; + } + strbuf_release(sb); + return -1; +} + int strbuf_getline(struct strbuf *sb, FILE *fp, int term) { int ch; diff --git a/strbuf.h b/strbuf.h index b1670d9..89bd36e 100644 --- a/strbuf.h +++ b/strbuf.h @@ -124,6 +124,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *); /* XXX: if read fails, any partial read is undone */ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint); extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint); +extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint); extern int strbuf_getline(struct strbuf *, FILE *, int); -- 1.6.1.rc3.3.gcc3e3 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/5] Make 'ce_compare_link()' use the new 'strbuf_readlink()' 2008-12-17 18:42 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Linus Torvalds @ 2008-12-17 18:43 ` Linus Torvalds 2008-12-17 18:43 ` [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' Linus Torvalds 2008-12-17 21:26 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Jay Soffian 2008-12-23 10:05 ` [PATCH] strbuf_readlink semantics update Pierre Habouzit 2 siblings, 1 reply; 33+ messages in thread From: Linus Torvalds @ 2008-12-17 18:43 UTC (permalink / raw) To: Junio C Hamano, Git Mailing List From: Linus Torvalds <torvalds@linux-foundation.org> Date: Wed, 17 Dec 2008 09:47:27 -0800 This simplifies the code, and also makes ce_compare_link now able to handle filesystems with odd 'st_size' return values for symlinks. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> --- read-cache.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-) diff --git a/read-cache.c b/read-cache.c index c14b562..b1475ff 100644 --- a/read-cache.c +++ b/read-cache.c @@ -99,27 +99,21 @@ static int ce_compare_data(struct cache_entry *ce, struct stat *st) static int ce_compare_link(struct cache_entry *ce, size_t expected_size) { int match = -1; - char *target; void *buffer; unsigned long size; enum object_type type; - int len; + struct strbuf sb = STRBUF_INIT; - target = xmalloc(expected_size); - len = readlink(ce->name, target, expected_size); - if (len != expected_size) { - free(target); + if (strbuf_readlink(&sb, ce->name, expected_size)) return -1; - } + buffer = read_sha1_file(ce->sha1, &type, &size); - if (!buffer) { - free(target); - return -1; + if (buffer) { + if (size == sb.len) + match = memcmp(buffer, sb.buf, size); + free(buffer); } - if (size == expected_size) - match = memcmp(buffer, target, size); - free(buffer); - free(target); + strbuf_release(&sb); return match; } -- 1.6.1.rc3.3.gcc3e3 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' 2008-12-17 18:43 ` [PATCH 2/5] Make 'ce_compare_link()' use the new 'strbuf_readlink()' Linus Torvalds @ 2008-12-17 18:43 ` Linus Torvalds 2008-12-17 18:44 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Linus Torvalds 2008-12-17 20:37 ` [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' Junio C Hamano 0 siblings, 2 replies; 33+ messages in thread From: Linus Torvalds @ 2008-12-17 18:43 UTC (permalink / raw) To: Junio C Hamano, Git Mailing List From: Linus Torvalds <torvalds@linux-foundation.org> Date: Wed, 17 Dec 2008 09:51:53 -0800 This makes us able to properly index symlinks even on filesystems where st_size doesn't match the true size of the link. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> --- sha1_file.c | 14 +++++--------- 1 files changed, 5 insertions(+), 9 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 0e021c5..52d1ead 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2523,8 +2523,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object) { int fd; - char *target; - size_t len; + struct strbuf sb = STRBUF_INIT; switch (st->st_mode & S_IFMT) { case S_IFREG: @@ -2537,20 +2536,17 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write path); break; case S_IFLNK: - len = xsize_t(st->st_size); - target = xmalloc(len + 1); - if (readlink(path, target, len + 1) != st->st_size) { + if (strbuf_readlink(&sb, path, st->st_size)) { char *errstr = strerror(errno); - free(target); return error("readlink(\"%s\"): %s", path, errstr); } if (!write_object) - hash_sha1_file(target, len, blob_type, sha1); - else if (write_sha1_file(target, len, blob_type, sha1)) + hash_sha1_file(sb.buf, sb.len, blob_type, sha1); + else if (write_sha1_file(sb.buf, sb.len, blob_type, sha1)) return error("%s: failed to insert into database", path); - free(target); + strbuf_release(&sb); break; case S_IFDIR: return resolve_gitlink_ref(path, "HEAD", sha1); -- 1.6.1.rc3.3.gcc3e3 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' 2008-12-17 18:43 ` [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' Linus Torvalds @ 2008-12-17 18:44 ` Linus Torvalds 2008-12-17 18:45 ` [PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks Linus Torvalds ` (2 more replies) 2008-12-17 20:37 ` [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' Junio C Hamano 1 sibling, 3 replies; 33+ messages in thread From: Linus Torvalds @ 2008-12-17 18:44 UTC (permalink / raw) To: Junio C Hamano, Git Mailing List From: Linus Torvalds <torvalds@linux-foundation.org> Date: Wed, 17 Dec 2008 10:26:13 -0800 This makes all tests pass on a system where 'lstat()' has been hacked to return bogus data in st_size for symlinks. Of course, the test coverage isn't complete, but it's a good baseline. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> --- diff.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index afefe08..4b2029c 100644 --- a/diff.c +++ b/diff.c @@ -1773,19 +1773,17 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only) s->size = xsize_t(st.st_size); if (!s->size) goto empty; - if (size_only) - return 0; if (S_ISLNK(st.st_mode)) { - int ret; - s->data = xmalloc(s->size); - s->should_free = 1; - ret = readlink(s->path, s->data, s->size); - if (ret < 0) { - free(s->data); + struct strbuf sb = STRBUF_INIT; + + if (strbuf_readlink(&sb, s->path, s->size)) goto err_empty; - } + s->data = strbuf_detach(&sb, &s->size); + s->should_free = 1; return 0; } + if (size_only) + return 0; fd = open(s->path, O_RDONLY); if (fd < 0) goto err_empty; -- 1.6.1.rc3.3.gcc3e3 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks 2008-12-17 18:44 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Linus Torvalds @ 2008-12-17 18:45 ` Linus Torvalds 2008-12-17 20:37 ` [PATCH 6/5] make_absolute_path(): check bounds when seeing an overlong symlink Junio C Hamano ` (2 more replies) 2008-12-17 20:37 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Junio C Hamano 2008-12-18 12:11 ` Mark Burton 2 siblings, 3 replies; 33+ messages in thread From: Linus Torvalds @ 2008-12-17 18:45 UTC (permalink / raw) To: Junio C Hamano, Git Mailing List From: Linus Torvalds <torvalds@linux-foundation.org> Date: Wed, 17 Dec 2008 10:31:36 -0800 The code was already set up to not really need it, so this just massages it a bit to remove the use entirely. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> --- There's a few raw 'readlink()' calls left, but they all seem to just have a big buffer and rely on the return value of readlink() rather than look too closely at st_size. But it would probably be good if somebody double-checked it all. Even so, this should all be better than what we used to have, though. diff.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/diff.c b/diff.c index 4b2029c..f160c1a 100644 --- a/diff.c +++ b/diff.c @@ -1881,13 +1881,12 @@ static void prepare_temp_file(const char *name, if (S_ISLNK(st.st_mode)) { int ret; char buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */ - size_t sz = xsize_t(st.st_size); - if (sizeof(buf) <= st.st_size) - die("symlink too long: %s", name); - ret = readlink(name, buf, sz); + ret = readlink(name, buf, sizeof(buf)); if (ret < 0) die("readlink(%s)", name); - prep_temp_blob(temp, buf, sz, + if (ret == sizeof(buf)) + die("symlink too long: %s", name); + prep_temp_blob(temp, buf, ret, (one->sha1_valid ? one->sha1 : null_sha1), (one->sha1_valid ? -- 1.6.1.rc3.3.gcc3e3 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 6/5] make_absolute_path(): check bounds when seeing an overlong symlink 2008-12-17 18:45 ` [PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks Linus Torvalds @ 2008-12-17 20:37 ` Junio C Hamano 2008-12-17 20:37 ` [PATCH 7/5] builtin-blame.c: use strbuf_readlink() Junio C Hamano 2008-12-17 20:37 ` [PATCH 8/5] combine-diff.c: " Junio C Hamano 2 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2008-12-17 20:37 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List Signed-off-by: Junio C Hamano <gitster@pobox.com> --- abspath.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git i/abspath.c w/abspath.c index 8194ce1..649f34f 100644 --- i/abspath.c +++ w/abspath.c @@ -64,6 +64,8 @@ const char *make_absolute_path(const char *path) len = readlink(buf, next_buf, PATH_MAX); if (len < 0) die ("Invalid symlink: %s", buf); + if (PATH_MAX <= len) + die("symbolic link too long: %s", buf); next_buf[len] = '\0'; buf = next_buf; buf_index = 1 - buf_index; ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 7/5] builtin-blame.c: use strbuf_readlink() 2008-12-17 18:45 ` [PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks Linus Torvalds 2008-12-17 20:37 ` [PATCH 6/5] make_absolute_path(): check bounds when seeing an overlong symlink Junio C Hamano @ 2008-12-17 20:37 ` Junio C Hamano 2008-12-17 20:37 ` [PATCH 8/5] combine-diff.c: " Junio C Hamano 2 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2008-12-17 20:37 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List When faking a commit out of the work tree contents, use strbuf_readlink() to read the contents of symbolic links. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin-blame.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-) diff --git i/builtin-blame.c w/builtin-blame.c index a0d6014..aae14ef 100644 --- i/builtin-blame.c +++ w/builtin-blame.c @@ -1996,7 +1996,6 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con if (!contents_from || strcmp("-", contents_from)) { struct stat st; const char *read_from; - unsigned long fin_size; if (contents_from) { if (stat(contents_from, &st) < 0) @@ -2008,7 +2007,6 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con die("Cannot lstat %s", path); read_from = path; } - fin_size = xsize_t(st.st_size); mode = canon_mode(st.st_mode); switch (st.st_mode & S_IFMT) { case S_IFREG: @@ -2016,9 +2014,8 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con die("cannot open or read %s", read_from); break; case S_IFLNK: - if (readlink(read_from, buf.buf, buf.alloc) != fin_size) + if (strbuf_readlink(&buf, read_from, st.st_size) < 0) die("cannot readlink %s", read_from); - buf.len = fin_size; break; default: die("unsupported file type %s", read_from); ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 8/5] combine-diff.c: use strbuf_readlink() 2008-12-17 18:45 ` [PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks Linus Torvalds 2008-12-17 20:37 ` [PATCH 6/5] make_absolute_path(): check bounds when seeing an overlong symlink Junio C Hamano 2008-12-17 20:37 ` [PATCH 7/5] builtin-blame.c: use strbuf_readlink() Junio C Hamano @ 2008-12-17 20:37 ` Junio C Hamano 2008-12-17 21:02 ` Linus Torvalds 2 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2008-12-17 20:37 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List When showing combined diff using work tree contents, use strbuf_readlink() to read symbolic links. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- combine-diff.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git i/combine-diff.c w/combine-diff.c index ec8df39..bccc018 100644 --- i/combine-diff.c +++ w/combine-diff.c @@ -703,15 +703,15 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, goto deleted_file; if (S_ISLNK(st.st_mode)) { - size_t len = xsize_t(st.st_size); - result_size = len; - result = xmalloc(len + 1); - if (result_size != readlink(elem->path, result, len)) { + struct strbuf buf = STRBUF_INIT; + + if (strbuf_readlink(&buf, elem->path, st.st_size) < 0) { error("readlink(%s): %s", elem->path, strerror(errno)); return; } - result[len] = 0; + result_size = buf.len; + result = strbuf_detach(&buf, NULL); elem->mode = canon_mode(st.st_mode); } else if (0 <= (fd = open(elem->path, O_RDONLY)) && ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 8/5] combine-diff.c: use strbuf_readlink() 2008-12-17 20:37 ` [PATCH 8/5] combine-diff.c: " Junio C Hamano @ 2008-12-17 21:02 ` Linus Torvalds 2008-12-17 21:34 ` Junio C Hamano 0 siblings, 1 reply; 33+ messages in thread From: Linus Torvalds @ 2008-12-17 21:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List On Wed, 17 Dec 2008, Junio C Hamano wrote: > - result[len] = 0; > + result_size = buf.len; > + result = strbuf_detach(&buf, NULL); If result_size was made size_t, this would be result = strbuf_detach(&buf, &result_size); But whether it makes any difference, I dunno. Anyway, Ack on the 6-8 additions. Linus ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 8/5] combine-diff.c: use strbuf_readlink() 2008-12-17 21:02 ` Linus Torvalds @ 2008-12-17 21:34 ` Junio C Hamano 0 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2008-12-17 21:34 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List Linus Torvalds <torvalds@linux-foundation.org> writes: > On Wed, 17 Dec 2008, Junio C Hamano wrote: >> - result[len] = 0; >> + result_size = buf.len; >> + result = strbuf_detach(&buf, NULL); > > If result_size was made size_t, this would be > > result = strbuf_detach(&buf, &result_size); > > But whether it makes any difference, I dunno. Yeah, use of "unsigned long" where "size_t" could be more appropriate stems from the very initial commit e83c516 (Initial revision of "git", the information manager from hell, 2005-04-07) and it is everywhere, and updating them one by one like you suggest would take forever ;-) Perhaps libgit2 would settle with a better typing system. The original draft by Shawn looked a bit too overengineered in its use of typedefs, but if I recall correctly later revisions were made saner. I haven't checked its current status. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' 2008-12-17 18:44 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Linus Torvalds 2008-12-17 18:45 ` [PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks Linus Torvalds @ 2008-12-17 20:37 ` Junio C Hamano 2008-12-18 12:11 ` Mark Burton 2 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2008-12-17 20:37 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List Linus Torvalds <torvalds@linux-foundation.org> writes: > diff --git a/diff.c b/diff.c > index afefe08..4b2029c 100644 > --- a/diff.c > +++ b/diff.c > @@ -1773,19 +1773,17 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only) > s->size = xsize_t(st.st_size); > if (!s->size) > goto empty; > - if (size_only) > - return 0; > if (S_ISLNK(st.st_mode)) { > ... > } > + if (size_only) > + return 0; It is unfortunate that we need to always readlink even when we only would want to cull differences early (e.g. --raw without any fancy filters such as rename detection), but symbolic links should be minorities in any sane repo, and it should not be worth trying to optimize this for sane filesystems by making it conditional. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' 2008-12-17 18:44 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Linus Torvalds 2008-12-17 18:45 ` [PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks Linus Torvalds 2008-12-17 20:37 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Junio C Hamano @ 2008-12-18 12:11 ` Mark Burton 2008-12-18 16:55 ` Linus Torvalds ` (2 more replies) 2 siblings, 3 replies; 33+ messages in thread From: Mark Burton @ 2008-12-18 12:11 UTC (permalink / raw) To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List Howdy folks, When I compile this latest version of diff.c on a i686 dual-core Pentium box I see: diff.c: In function ‘diff_populate_filespec’: diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type The same code compiles without warning on a x86_64 AMD box. Both machines are running stock Ubuntu 8.04. Does it need a cast on some architectures? Cheers, Mark ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' 2008-12-18 12:11 ` Mark Burton @ 2008-12-18 16:55 ` Linus Torvalds 2008-12-18 17:41 ` René Scharfe 2008-12-18 16:56 ` René Scharfe 2008-12-19 22:10 ` [PATCH] diff.c: fix pointer type warning René Scharfe 2 siblings, 1 reply; 33+ messages in thread From: Linus Torvalds @ 2008-12-18 16:55 UTC (permalink / raw) To: Mark Burton; +Cc: Junio C Hamano, Git Mailing List On Thu, 18 Dec 2008, Mark Burton wrote: > > Does it need a cast on some architectures? Gaah. My bad. It should work fine ("unsigned long" is physically the same type as "size_t" in your case), but on 32-bit x86, size_t is generally "unsigned int" - which is the same physical type there (both int and long are 32-bit) but causes a valid warning. I think we should just make the "size" member "size_t". I use "unsigned long" out of much too long habit, since we traditionally avoided "size_t" in the kernel due to it just being another unnecessary architecture- specific detail. So the proper patch is probably just the following. Sorry about that, Linus --- diffcore.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/diffcore.h b/diffcore.h index 5b63458..16a73e6 100644 --- a/diffcore.h +++ b/diffcore.h @@ -30,7 +30,7 @@ struct diff_filespec { void *data; void *cnt_data; const char *funcname_pattern_ident; - unsigned long size; + size_t size; int count; /* Reference count */ int xfrm_flags; /* for use by the xfrm */ int rename_used; /* Count of rename users */ ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' 2008-12-18 16:55 ` Linus Torvalds @ 2008-12-18 17:41 ` René Scharfe 2008-12-18 17:49 ` Linus Torvalds 0 siblings, 1 reply; 33+ messages in thread From: René Scharfe @ 2008-12-18 17:41 UTC (permalink / raw) To: Linus Torvalds; +Cc: Mark Burton, Junio C Hamano, Git Mailing List Linus Torvalds schrieb: > > On Thu, 18 Dec 2008, Mark Burton wrote: >> Does it need a cast on some architectures? > > Gaah. My bad. It should work fine ("unsigned long" is physically the same > type as "size_t" in your case), but on 32-bit x86, size_t is generally > "unsigned int" - which is the same physical type there (both int and long > are 32-bit) but causes a valid warning. > > I think we should just make the "size" member "size_t". I use "unsigned > long" out of much too long habit, since we traditionally avoided "size_t" > in the kernel due to it just being another unnecessary architecture- > specific detail. > > So the proper patch is probably just the following. Sorry about that, > > Linus > --- > diffcore.h | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/diffcore.h b/diffcore.h > index 5b63458..16a73e6 100644 > --- a/diffcore.h > +++ b/diffcore.h > @@ -30,7 +30,7 @@ struct diff_filespec { > void *data; > void *cnt_data; > const char *funcname_pattern_ident; > - unsigned long size; > + size_t size; > int count; /* Reference count */ > int xfrm_flags; /* for use by the xfrm */ > int rename_used; /* Count of rename users */ Yes, but now I get two new warnings: diff.c: In function `diff_populate_filespec': diff.c:1809: warning: passing arg 2 of `sha1_object_info' from incompatible pointer type diff.c:1811: warning: passing arg 3 of `read_sha1_file' from incompatible pointer type If we followed that way along we'd convert just about everything to use size_t, which is going a bit too far during the -rc phase.. René PS: In the other subthread, I was missing the "t" in "st" in line 1757, not 1760. Ahem. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' 2008-12-18 17:41 ` René Scharfe @ 2008-12-18 17:49 ` Linus Torvalds 2008-12-18 17:56 ` Olivier Galibert 0 siblings, 1 reply; 33+ messages in thread From: Linus Torvalds @ 2008-12-18 17:49 UTC (permalink / raw) To: René Scharfe; +Cc: Mark Burton, Junio C Hamano, Git Mailing List On Thu, 18 Dec 2008, René Scharfe wrote: > > Yes, but now I get two new warnings: > > diff.c: In function `diff_populate_filespec': > diff.c:1809: warning: passing arg 2 of `sha1_object_info' from > incompatible pointer type > diff.c:1811: warning: passing arg 3 of `read_sha1_file' from > incompatible pointer type Yeah, yeah, we should probably fix it all, but you're right, your patch avoids the pain. The good news is that "size_t" and "unsigned long" really are the same size on all sane platforms. You'll see differences just in the odd 16-bit world ("small" memory model with 16-bit size_t and possibly 32-bit "long") Although I guess some really odd more modern case might have a 32-bit pointer (and size_t) and 64-bit long. If it's possible to screw the type system up, history shows that somebody (usually Windows) will do it. Linus ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' 2008-12-18 17:49 ` Linus Torvalds @ 2008-12-18 17:56 ` Olivier Galibert 0 siblings, 0 replies; 33+ messages in thread From: Olivier Galibert @ 2008-12-18 17:56 UTC (permalink / raw) To: Linus Torvalds Cc: René Scharfe, Mark Burton, Junio C Hamano, Git Mailing List On Thu, Dec 18, 2008 at 09:49:23AM -0800, Linus Torvalds wrote: > If it's possible to screw the type system up, history shows that > somebody (usually Windows) will do it. AFAIK, the Win64 long is 32 bits and size_t/void * is 64 bits. I'll leave you to your groaning. OG. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' 2008-12-18 12:11 ` Mark Burton 2008-12-18 16:55 ` Linus Torvalds @ 2008-12-18 16:56 ` René Scharfe 2008-12-18 17:28 ` René Scharfe 2008-12-19 22:10 ` [PATCH] diff.c: fix pointer type warning René Scharfe 2 siblings, 1 reply; 33+ messages in thread From: René Scharfe @ 2008-12-18 16:56 UTC (permalink / raw) To: Mark Burton; +Cc: Linus Torvalds, Junio C Hamano, Git Mailing List Mark Burton schrieb: > Howdy folks, > > When I compile this latest version of diff.c on a i686 dual-core Pentium box > I see: > > diff.c: In function ‘diff_populate_filespec’: > diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type > > The same code compiles without warning on a x86_64 AMD box. Both > machines are running stock Ubuntu 8.04. > > Does it need a cast on some architectures? The type of the size member of struct stat is off_t, while strbuf_detach expects a size_t pointer. This patch should fix the warning: diff --git a/diff.c b/diff.c index f160c1a..0484601 100644 --- a/diff.c +++ b/diff.c @@ -1778,7 +1778,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only) if (strbuf_readlink(&sb, s->path, s->size)) goto err_empty; - s->data = strbuf_detach(&sb, &s->size); + s->size = sb.len; + s->data = strbuf_detach(&sb, NULL); s->should_free = 1; return 0; } ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' 2008-12-18 16:56 ` René Scharfe @ 2008-12-18 17:28 ` René Scharfe 0 siblings, 0 replies; 33+ messages in thread From: René Scharfe @ 2008-12-18 17:28 UTC (permalink / raw) Cc: Mark Burton, Linus Torvalds, Junio C Hamano, Git Mailing List René Scharfe schrieb: > Mark Burton schrieb: >> Howdy folks, >> >> When I compile this latest version of diff.c on a i686 dual-core Pentium box >> I see: >> >> diff.c: In function ‘diff_populate_filespec’: >> diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type >> >> The same code compiles without warning on a x86_64 AMD box. Both >> machines are running stock Ubuntu 8.04. >> >> Does it need a cast on some architectures? > > The type of the size member of struct stat is off_t, while strbuf_detach expects > a size_t pointer. This patch should fix the warning: Nevermind, I somehow missed the last "t" in line 1760.. The patch works nevertheless. 8-) René ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] diff.c: fix pointer type warning 2008-12-18 12:11 ` Mark Burton 2008-12-18 16:55 ` Linus Torvalds 2008-12-18 16:56 ` René Scharfe @ 2008-12-19 22:10 ` René Scharfe 2008-12-19 23:09 ` Junio C Hamano 2 siblings, 1 reply; 33+ messages in thread From: René Scharfe @ 2008-12-19 22:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Mark Burton, Linus Torvalds, Git Mailing List As Mark Burton noted, the conversion to strbuf_readlink() caused a compile warning on some architectures: > diff.c: In function ‘diff_populate_filespec’: > diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type A pointer to an unsigned long is given while a pointer to a size_t is expected; the two types are not considered to be equivalent everywhere. The real fix would be to change the type of the size member of struct diff_filespec to size_t, but that would cause other warnings in connection with functions expecting unsigned long, and attempts to fix them might loose an avalanche of changes. Later. This patch just silences the warning by adding an (implicit) casting step. Reported-by: Mark Burton <markb@ordern.com> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx> --- diff.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/diff.c b/diff.c index f160c1a..0484601 100644 --- a/diff.c +++ b/diff.c @@ -1778,7 +1778,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only) if (strbuf_readlink(&sb, s->path, s->size)) goto err_empty; - s->data = strbuf_detach(&sb, &s->size); + s->size = sb.len; + s->data = strbuf_detach(&sb, NULL); s->should_free = 1; return 0; } ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] diff.c: fix pointer type warning 2008-12-19 22:10 ` [PATCH] diff.c: fix pointer type warning René Scharfe @ 2008-12-19 23:09 ` Junio C Hamano 0 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2008-12-19 23:09 UTC (permalink / raw) To: René Scharfe; +Cc: Mark Burton, Linus Torvalds, Git Mailing List Thanks; I think I already have it in my tree from your yesterday's e-mail. I just have been too busy to whip the other branches into shape to push the results out. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' 2008-12-17 18:43 ` [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' Linus Torvalds 2008-12-17 18:44 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Linus Torvalds @ 2008-12-17 20:37 ` Junio C Hamano 1 sibling, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2008-12-17 20:37 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List Linus Torvalds <torvalds@linux-foundation.org> writes: > @@ -2537,20 +2536,17 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write > path); > break; > case S_IFLNK: > - len = xsize_t(st->st_size); > - target = xmalloc(len + 1); > - if (readlink(path, target, len + 1) != st->st_size) { > + if (strbuf_readlink(&sb, path, st->st_size)) { > char *errstr = strerror(errno); > - free(target); > return error("readlink(\"%s\"): %s", path, > errstr); Thanks; as strbuf_readlink() does not do any iffy library calls that would stomp on errno, the error reporting should still be valid here. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] Add generic 'strbuf_readlink()' helper function 2008-12-17 18:42 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Linus Torvalds 2008-12-17 18:43 ` [PATCH 2/5] Make 'ce_compare_link()' use the new 'strbuf_readlink()' Linus Torvalds @ 2008-12-17 21:26 ` Jay Soffian 2008-12-17 21:44 ` Linus Torvalds 2008-12-23 10:05 ` [PATCH] strbuf_readlink semantics update Pierre Habouzit 2 siblings, 1 reply; 33+ messages in thread From: Jay Soffian @ 2008-12-17 21:26 UTC (permalink / raw) To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List On Wed, Dec 17, 2008 at 1:42 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > + while (hint < STRBUF_MAXLINK) { > + int len; > + > + strbuf_grow(sb, hint); > + len = readlink(path, sb->buf, hint); > + if (len < 0) { > + if (errno != ERANGE) > + break; > + } else if (len < hint) { > + strbuf_setlen(sb, len); > + return 0; > + } > + > + /* .. the buffer was too small - try again */ > + hint *= 2; > + continue; > + } Why the continue statement at the end of the loop? j. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] Add generic 'strbuf_readlink()' helper function 2008-12-17 21:26 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Jay Soffian @ 2008-12-17 21:44 ` Linus Torvalds 0 siblings, 0 replies; 33+ messages in thread From: Linus Torvalds @ 2008-12-17 21:44 UTC (permalink / raw) To: Jay Soffian; +Cc: Junio C Hamano, Git Mailing List On Wed, 17 Dec 2008, Jay Soffian wrote: > > + /* .. the buffer was too small - try again */ > > + hint *= 2; > > + continue; > > + } > > Why the continue statement at the end of the loop? Oh, it's unnecessary left-over from me originally writing that part inside an if-statement, and then moving things around. So it could/should be deleted. Good eyes. Linus ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] strbuf_readlink semantics update. 2008-12-17 18:42 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Linus Torvalds 2008-12-17 18:43 ` [PATCH 2/5] Make 'ce_compare_link()' use the new 'strbuf_readlink()' Linus Torvalds 2008-12-17 21:26 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Jay Soffian @ 2008-12-23 10:05 ` Pierre Habouzit 2008-12-23 10:21 ` Pierre Habouzit 2 siblings, 1 reply; 33+ messages in thread From: Pierre Habouzit @ 2008-12-23 10:05 UTC (permalink / raw) To: git, Linus Torvalds; +Cc: Pierre Habouzit, Junio C Hamano, Git Mailing List strbuf_* operations are meant to append their results to a current buffer rather than _replace_ its content. Modify strbuf_readlink accordingly. Current callers only operate on empty buffers at the moment and this semantic change doesn't break any current code. Signed-off-by: Pierre Habouzit <madcoder@debian.org> --- strbuf.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/strbuf.c b/strbuf.c index bdf4954..254a7ee 100644 --- a/strbuf.c +++ b/strbuf.c @@ -299,12 +299,12 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint) int len; strbuf_grow(sb, hint); - len = readlink(path, sb->buf, hint); + len = readlink(path, sb->buf + sb->len, hint); if (len < 0) { if (errno != ERANGE) break; } else if (len < hint) { - strbuf_setlen(sb, len); + strbuf_setlen(sb, sb->len + len); return 0; } -- 1.6.1.rc4.304.g3da087 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] strbuf_readlink semantics update. 2008-12-23 10:05 ` [PATCH] strbuf_readlink semantics update Pierre Habouzit @ 2008-12-23 10:21 ` Pierre Habouzit 2008-12-23 18:16 ` Linus Torvalds 0 siblings, 1 reply; 33+ messages in thread From: Pierre Habouzit @ 2008-12-23 10:21 UTC (permalink / raw) To: git, Linus Torvalds; +Cc: Junio C Hamano [-- Attachment #1: Type: text/plain, Size: 4021 bytes --] when readlink fails, the strbuf shall not be destroyed. It's not how read_file_or_gitlink works for example. Fix strbuf_readlink callers to destroy the buffer when appropriate. Fix read_old_data possible leaks in case of errors, since even when no data has been read, the strbufs may have grown to prepare the reads. strbuf_release must be called on them. Signed-off-by: Pierre Habouzit <madcoder@debian.org> --- I know it somehow add lines to the callers, but it's actually more important to keep consistency among the strbuf APIs than to save 10 SLOCs. builtin-apply.c | 8 ++++++-- combine-diff.c | 1 + diff.c | 4 +++- read-cache.c | 4 +++- sha1_file.c | 1 + strbuf.c | 2 +- 6 files changed, 15 insertions(+), 5 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index 07244b0..c1fe9ca 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -2306,8 +2306,10 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry * /* We have a patched copy in memory use that */ strbuf_add(&buf, tpatch->result, tpatch->resultsize); } else if (cached) { - if (read_file_or_gitlink(ce, &buf)) + if (read_file_or_gitlink(ce, &buf)) { + strbuf_release(&buf); return error("read of %s failed", patch->old_name); + } } else if (patch->old_name) { if (S_ISGITLINK(patch->old_mode)) { if (ce) { @@ -2320,8 +2322,10 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry * patch->fragments = NULL; } } else { - if (read_old_data(st, patch->old_name, &buf)) + if (read_old_data(st, patch->old_name, &buf)) { + strbuf_release(&buf); return error("read of %s failed", patch->old_name); + } } } diff --git a/combine-diff.c b/combine-diff.c index bccc018..674745d 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -706,6 +706,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, struct strbuf buf = STRBUF_INIT; if (strbuf_readlink(&buf, elem->path, st.st_size) < 0) { + strbuf_release(&buf); error("readlink(%s): %s", elem->path, strerror(errno)); return; diff --git a/diff.c b/diff.c index b57d9ac..41f7e1c 100644 --- a/diff.c +++ b/diff.c @@ -1778,8 +1778,10 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only) if (S_ISLNK(st.st_mode)) { struct strbuf sb = STRBUF_INIT; - if (strbuf_readlink(&sb, s->path, s->size)) + if (strbuf_readlink(&sb, s->path, s->size)) { + strbuf_release(&sb); goto err_empty; + } s->size = sb.len; s->data = strbuf_detach(&sb, NULL); s->should_free = 1; diff --git a/read-cache.c b/read-cache.c index db166da..9673d91 100644 --- a/read-cache.c +++ b/read-cache.c @@ -104,8 +104,10 @@ static int ce_compare_link(struct cache_entry *ce, size_t expected_size) enum object_type type; struct strbuf sb = STRBUF_INIT; - if (strbuf_readlink(&sb, ce->name, expected_size)) + if (strbuf_readlink(&sb, ce->name, expected_size)) { + strbuf_release(&sb); return -1; + } buffer = read_sha1_file(ce->sha1, &type, &size); if (buffer) { diff --git a/sha1_file.c b/sha1_file.c index 52d1ead..a62b53d 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2538,6 +2538,7 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write case S_IFLNK: if (strbuf_readlink(&sb, path, st->st_size)) { char *errstr = strerror(errno); + strbuf_release(&sb); return error("readlink(\"%s\"): %s", path, errstr); } diff --git a/strbuf.c b/strbuf.c index 254a7ee..b1f2a97 100644 --- a/strbuf.c +++ b/strbuf.c @@ -311,7 +311,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint) /* .. the buffer was too small - try again */ hint *= 2; } - strbuf_release(sb); + sb->buf[sb->len] = '\0'; return -1; } -- 1.6.1.rc4.306.g849c2 [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] strbuf_readlink semantics update. 2008-12-23 10:21 ` Pierre Habouzit @ 2008-12-23 18:16 ` Linus Torvalds 2008-12-24 10:11 ` Pierre Habouzit 0 siblings, 1 reply; 33+ messages in thread From: Linus Torvalds @ 2008-12-23 18:16 UTC (permalink / raw) To: Pierre Habouzit; +Cc: git, Junio C Hamano On Tue, 23 Dec 2008, Pierre Habouzit wrote: > > when readlink fails, the strbuf shall not be destroyed. It's not how > read_file_or_gitlink works for example. I disagree. This patch just makes things worse. Just leave the "strbuf_release()" in _one_ place. Look: 6 files changed, 15 insertions(+), 5 deletions(-) you added ten unnecessary lines, and you made the interface harder to use. What was the gain here? > Fix read_old_data possible leaks in case of errors, since even when no > data has been read, the strbufs may have grown to prepare the reads. > strbuf_release must be called on them. That's a separate error, and quite frankly, the best approach to that is likely to instead of breaking strbuf_readlink(), just make the S_IFREG() case release it. I'd suggest that strbuf_read_file() should probably also do a strbuf_release() if it returns a negative error value, but that's a separate issue (and still leaves "read_old_data()" having to release things, since read_old_data() wants to see exactly st_size bytes. Although I suspect we might want to change that, and just make it test for negative too). Linus ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] strbuf_readlink semantics update. 2008-12-23 18:16 ` Linus Torvalds @ 2008-12-24 10:11 ` Pierre Habouzit 2008-12-24 15:20 ` René Scharfe 0 siblings, 1 reply; 33+ messages in thread From: Pierre Habouzit @ 2008-12-24 10:11 UTC (permalink / raw) To: Linus Torvalds; +Cc: git, Junio C Hamano [-- Attachment #1: Type: text/plain, Size: 1420 bytes --] On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote: > > > On Tue, 23 Dec 2008, Pierre Habouzit wrote: > > > > when readlink fails, the strbuf shall not be destroyed. It's not how > > read_file_or_gitlink works for example. > > I disagree. > > This patch just makes things worse. Just leave the "strbuf_release()" in > _one_ place. The "problem" is that the strbuf API usually works that way: functions append things to a buffer, or do nothing, but always keep the buffer in a state where you can append more stuff to it. If read_file_or_gitlink or strbuf_readlink destroy the buffer, then you break the second expectation people (should) have about the strbuf API. The reason is that if you built things in the buffer, you really don't want it to be undone just because the last bit you add went wrong for some reason. Or if you have a buffer that is reused in a loop, you don't want the buffer you allocated to be dropped just because one error occurred. Alternatively, we could pass a flag to tell function performing reads (fread, read, readlink, whatever) that those should destroy the buffer on error or just report it. I don't really know. It sounds like over-engineering though. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] strbuf_readlink semantics update. 2008-12-24 10:11 ` Pierre Habouzit @ 2008-12-24 15:20 ` René Scharfe 2008-12-25 7:23 ` Junio C Hamano 0 siblings, 1 reply; 33+ messages in thread From: René Scharfe @ 2008-12-24 15:20 UTC (permalink / raw) To: Pierre Habouzit; +Cc: Linus Torvalds, git, Junio C Hamano Pierre Habouzit schrieb: > On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote: >> >> On Tue, 23 Dec 2008, Pierre Habouzit wrote: >>> when readlink fails, the strbuf shall not be destroyed. It's not how >>> read_file_or_gitlink works for example. >> I disagree. >> >> This patch just makes things worse. Just leave the "strbuf_release()" in >> _one_ place. > > The "problem" is that the strbuf API usually works that way: functions > append things to a buffer, or do nothing, but always keep the buffer in > a state where you can append more stuff to it. > > If read_file_or_gitlink or strbuf_readlink destroy the buffer, then you > break the second expectation people (should) have about the strbuf API. The "append or do nothing" rule is broken by strbuf_getline(), but I agree to your reasoning. How about refining this rule a bit to "do your thing and roll back changes if an error occurs"? I think it's not worth to undo allocation extensions, but making reverting first time allocations seems like a good idea. Something like this? René PS: only nine lines! ;-) strbuf.c | 17 +++++++++++++---- 1 files changed, 13 insertions(+), 4 deletions(-) diff --git a/strbuf.c b/strbuf.c index bdf4954..6ed0684 100644 --- a/strbuf.c +++ b/strbuf.c @@ -256,18 +256,21 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f) { size_t res; + size_t oldalloc = sb->alloc; strbuf_grow(sb, size); res = fread(sb->buf + sb->len, 1, size, f); - if (res > 0) { + if (res > 0) strbuf_setlen(sb, sb->len + res); - } + else if (res < 0 && oldalloc == 0) + strbuf_release(sb); return res; } ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint) { size_t oldlen = sb->len; + size_t oldalloc = sb->alloc; strbuf_grow(sb, hint ? hint : 8192); for (;;) { @@ -275,7 +278,10 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint) cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1); if (cnt < 0) { - strbuf_setlen(sb, oldlen); + if (oldalloc == 0) + strbuf_release(sb); + else + strbuf_setlen(sb, oldlen); return -1; } if (!cnt) @@ -292,6 +298,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint) int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint) { + size_t oldalloc = sb->alloc; + if (hint < 32) hint = 32; @@ -311,7 +319,8 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint) /* .. the buffer was too small - try again */ hint *= 2; } - strbuf_release(sb); + if (oldalloc == 0) + strbuf_release(sb); return -1; } ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] strbuf_readlink semantics update. 2008-12-24 15:20 ` René Scharfe @ 2008-12-25 7:23 ` Junio C Hamano 2009-01-04 12:21 ` Pierre Habouzit 0 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2008-12-25 7:23 UTC (permalink / raw) To: René Scharfe; +Cc: Pierre Habouzit, Linus Torvalds, git René Scharfe <rene.scharfe@lsrfire.ath.cx> writes: > Pierre Habouzit schrieb: >> On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote: >>> >>> On Tue, 23 Dec 2008, Pierre Habouzit wrote: >>>> when readlink fails, the strbuf shall not be destroyed. It's not how >>>> read_file_or_gitlink works for example. >>> I disagree. >>> >>> This patch just makes things worse. Just leave the "strbuf_release()" in >>> _one_ place. > ... > The "append or do nothing" rule is broken by strbuf_getline(), but I agree > to your reasoning. How about refining this rule a bit to "do your thing > and roll back changes if an error occurs"? I think it's not worth to undo > allocation extensions, but making reverting first time allocations seems > like a good idea. Something like this? I think this is much better than Pierre's. Pierre's "if it is called strbuf_*, it should always append" is a good uniformity to have in an API, but making the caller suffer for clean-up is going backwards. The reason we use strbuf when we can is so that the callers do not have to worry about memory allocation issues too much. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] strbuf_readlink semantics update. 2008-12-25 7:23 ` Junio C Hamano @ 2009-01-04 12:21 ` Pierre Habouzit 2009-01-06 20:41 ` [PATCH] strbuf: instate cleanup rule in case of non-memory errors René Scharfe 0 siblings, 1 reply; 33+ messages in thread From: Pierre Habouzit @ 2009-01-04 12:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: René Scharfe, Linus Torvalds, git [-- Attachment #1: Type: text/plain, Size: 1520 bytes --] On Thu, Dec 25, 2008 at 07:23:58AM +0000, Junio C Hamano wrote: > René Scharfe <rene.scharfe@lsrfire.ath.cx> writes: > > > Pierre Habouzit schrieb: > >> On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote: > >>> > >>> On Tue, 23 Dec 2008, Pierre Habouzit wrote: > >>>> when readlink fails, the strbuf shall not be destroyed. It's not how > >>>> read_file_or_gitlink works for example. > >>> I disagree. > >>> > >>> This patch just makes things worse. Just leave the "strbuf_release()" in > >>> _one_ place. > > ... > > The "append or do nothing" rule is broken by strbuf_getline(), but I agree > > to your reasoning. How about refining this rule a bit to "do your thing > > and roll back changes if an error occurs"? I think it's not worth to undo > > allocation extensions, but making reverting first time allocations seems > > like a good idea. Something like this? > > I think this is much better than Pierre's. I agree it's a fine semantics. > Pierre's "if it is called strbuf_*, it should always append" is a good > uniformity to have in an API, but making the caller suffer for > clean-up is going backwards. The reason we use strbuf when we can is > so that the callers do not have to worry about memory allocation > issues too much. Ack. Sorry for the delay I was on vacation. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] strbuf: instate cleanup rule in case of non-memory errors 2009-01-04 12:21 ` Pierre Habouzit @ 2009-01-06 20:41 ` René Scharfe 2009-01-07 21:19 ` Junio C Hamano 0 siblings, 1 reply; 33+ messages in thread From: René Scharfe @ 2009-01-06 20:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: Pierre Habouzit, Linus Torvalds, git Make all strbuf functions that can fail free() their memory on error if they have allocated it. They don't shrink buffers that have been grown, though. This allows for easier error handling, as callers only need to call strbuf_release() if A) the command succeeded or B) if they would have had to do so anyway because they added something to the strbuf themselves. Bonus hunk: document strbuf_readlink. Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx> --- Documentation/technical/api-strbuf.txt | 11 +++++++++-- strbuf.c | 17 +++++++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt index a8ee2fe..9a4e3ea 100644 --- a/Documentation/technical/api-strbuf.txt +++ b/Documentation/technical/api-strbuf.txt @@ -133,8 +133,10 @@ Functions * Adding data to the buffer -NOTE: All of these functions in this section will grow the buffer as - necessary. +NOTE: All of the functions in this section will grow the buffer as necessary. +If they fail for some reason other than memory shortage and the buffer hadn't +been allocated before (i.e. the `struct strbuf` was set to `STRBUF_INIT`), +then they will free() it. `strbuf_addch`:: @@ -235,6 +237,11 @@ same behaviour as well. Read the contents of a file, specified by its path. The third argument can be used to give a hint about the file size, to avoid reallocs. +`strbuf_readlink`:: + + Read the target of a symbolic link, specified by its path. The third + argument can be used to give a hint about the size, to avoid reallocs. + `strbuf_getline`:: Read a line from a FILE* pointer. The second argument specifies the line diff --git a/strbuf.c b/strbuf.c index bdf4954..6ed0684 100644 --- a/strbuf.c +++ b/strbuf.c @@ -256,18 +256,21 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f) { size_t res; + size_t oldalloc = sb->alloc; strbuf_grow(sb, size); res = fread(sb->buf + sb->len, 1, size, f); - if (res > 0) { + if (res > 0) strbuf_setlen(sb, sb->len + res); - } + else if (res < 0 && oldalloc == 0) + strbuf_release(sb); return res; } ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint) { size_t oldlen = sb->len; + size_t oldalloc = sb->alloc; strbuf_grow(sb, hint ? hint : 8192); for (;;) { @@ -275,7 +278,10 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint) cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1); if (cnt < 0) { - strbuf_setlen(sb, oldlen); + if (oldalloc == 0) + strbuf_release(sb); + else + strbuf_setlen(sb, oldlen); return -1; } if (!cnt) @@ -292,6 +298,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint) int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint) { + size_t oldalloc = sb->alloc; + if (hint < 32) hint = 32; @@ -311,7 +319,8 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint) /* .. the buffer was too small - try again */ hint *= 2; } - strbuf_release(sb); + if (oldalloc == 0) + strbuf_release(sb); return -1; } -- 1.6.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] strbuf: instate cleanup rule in case of non-memory errors 2009-01-06 20:41 ` [PATCH] strbuf: instate cleanup rule in case of non-memory errors René Scharfe @ 2009-01-07 21:19 ` Junio C Hamano 0 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2009-01-07 21:19 UTC (permalink / raw) To: René Scharfe; +Cc: Pierre Habouzit, Linus Torvalds, git René Scharfe <rene.scharfe@lsrfire.ath.cx> writes: > Make all strbuf functions that can fail free() their memory on error if > they have allocated it. They don't shrink buffers that have been grown, > though. Thanks; applied. ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2009-01-07 21:20 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-12-17 18:42 [PATCH 0/5] Be careful about lstat()-vs-readlink() Linus Torvalds 2008-12-17 18:42 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Linus Torvalds 2008-12-17 18:43 ` [PATCH 2/5] Make 'ce_compare_link()' use the new 'strbuf_readlink()' Linus Torvalds 2008-12-17 18:43 ` [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' Linus Torvalds 2008-12-17 18:44 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Linus Torvalds 2008-12-17 18:45 ` [PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks Linus Torvalds 2008-12-17 20:37 ` [PATCH 6/5] make_absolute_path(): check bounds when seeing an overlong symlink Junio C Hamano 2008-12-17 20:37 ` [PATCH 7/5] builtin-blame.c: use strbuf_readlink() Junio C Hamano 2008-12-17 20:37 ` [PATCH 8/5] combine-diff.c: " Junio C Hamano 2008-12-17 21:02 ` Linus Torvalds 2008-12-17 21:34 ` Junio C Hamano 2008-12-17 20:37 ` [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()' Junio C Hamano 2008-12-18 12:11 ` Mark Burton 2008-12-18 16:55 ` Linus Torvalds 2008-12-18 17:41 ` René Scharfe 2008-12-18 17:49 ` Linus Torvalds 2008-12-18 17:56 ` Olivier Galibert 2008-12-18 16:56 ` René Scharfe 2008-12-18 17:28 ` René Scharfe 2008-12-19 22:10 ` [PATCH] diff.c: fix pointer type warning René Scharfe 2008-12-19 23:09 ` Junio C Hamano 2008-12-17 20:37 ` [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()' Junio C Hamano 2008-12-17 21:26 ` [PATCH 1/5] Add generic 'strbuf_readlink()' helper function Jay Soffian 2008-12-17 21:44 ` Linus Torvalds 2008-12-23 10:05 ` [PATCH] strbuf_readlink semantics update Pierre Habouzit 2008-12-23 10:21 ` Pierre Habouzit 2008-12-23 18:16 ` Linus Torvalds 2008-12-24 10:11 ` Pierre Habouzit 2008-12-24 15:20 ` René Scharfe 2008-12-25 7:23 ` Junio C Hamano 2009-01-04 12:21 ` Pierre Habouzit 2009-01-06 20:41 ` [PATCH] strbuf: instate cleanup rule in case of non-memory errors René Scharfe 2009-01-07 21:19 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.