Re: [PATCH] strbuf_readlink semantics update.
From: René Scharfe <hidden>
Date: 2016-06-15 22:45:49
Subsystem:
the rest · Maintainer:
Linus Torvalds
Pierre Habouzit schrieb:
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 "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; }