Re: [PATCH] strbuf_readlink semantics update.

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Dec 23, 2008, 18:16 UTC
Message-ID: <alpine.LFD.2.00.0812231009220.3535@localhost.localdomain>
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