Re: [PATCH] strbuf_readlink semantics update.

René Scharfe [off-list ref] writes:

Pierre Habouzit schrieb:

quoted

On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:

quoted

On Tue, 23 Dec 2008, Pierre Habouzit wrote:

quoted

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.

On Thu, Dec 25, 2008 at 07:23:58AM +0000, Junio C Hamano wrote:

René Scharfe [off-list ref] writes:

quoted

Pierre Habouzit schrieb:

quoted

On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:

quoted

On Tue, 23 Dec 2008, Pierre Habouzit wrote:

quoted

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

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 <redacted>
---
 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

René Scharfe [off-list ref] 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.