From: Linus Torvalds <torvalds@linux-foundation.org>
To: Pierre Habouzit <madcoder@debian.org>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] strbuf_readlink semantics update.
Date: Tue, 23 Dec 2008 10:16:01 -0800 (PST) [thread overview]
Message-ID: <alpine.LFD.2.00.0812231009220.3535@localhost.localdomain> (raw)
In-Reply-To: <20081223102127.GA21485@artemis.corp>
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
next prev parent reply other threads:[~2008-12-23 18:17 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LFD.2.00.0812231009220.3535@localhost.localdomain \
--to=torvalds@linux-foundation.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=madcoder@debian.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.