We use a fixed-size buffer of 4096 bytes to format die() and
error() messages. We explicitly avoided using a strbuf or
other fanciness here, because we want to make sure that we
report the message even in the face of malloc() failure
(after all, we might even be dying due to a malloc call).
In most cases, this buffer is long enough to show any
reasonable message. However, if you are experimenting with
_unreasonable_ things (e.g., filenames approaching 4096
bytes), the messages can be truncated, making them
confusing. E.g., seeing:
error: cannot stat 'aaaaaaaaaaaaaaaaaaaaaa
is much less useful than:
error: cannot stat 'aaaaaaaaaaaaaaaaaaaaaaa/foo': File too long
(these are truncated for the sake of readability; obviously
real examples are much longer. Just imagine many lines full
of "a"'s).
This patch teaches vreportf and vwritef to at least try
using a dynamically sized buffer before falling back to the
fixed-size buffer. For most long errors (which are typically
reporting problems with syscalls on long filenames), this
means we'll generally see the full message. And in case that
fails, we still print the truncated message, but with a note
that it was truncated.
Note that die_errno does not need the same treatment for its
fixed-size buffers, as they are a combination of:
- our fixed-size string constants, without placeholders
expanded (so a literal "cannot stat '%s'", without %s
expanded to arbitrary data)
- the results of strerror(errno)
both of which should be predictably small.
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
So this solution tries to change vreportf and vwritef as little as
possible, and ends up slightly complex as a result. But reading
vreportf's:
vsnprintf(msg, sizeof(msg), err, params);
fprintf(stderr, "%s%s\n", prefix, msg);
I had to wonder why this wasn't just:
fputs(prefix, stderr);
vfprintf(stderr, err, params);
In fact, we used to do this, but changed it in d048a96 (print
warning/error/fatal messages in one shot, 2007-11-09). I'm not sure of
the reasoning there, though. I would expect stdio to generally write
that as a single shot already, assuming:
- there isn't anything in the stderr buffer already (i.e., we do not
need to flush somewhere in the middle)
- our prefix does not have a newline (which it doesn't; it is
"error:", "fatal:", etc).
IOW, I wonder if it would be enough to simply fflush(stderr) before and
after to make sure we keep the buffer clear. I also wonder if this is
even enough as-is; if the resulting message is larger than stdio's
buffer size, I'd expect it to come through across several writes anyway.
As for vwritef, it exists solely to work over write(), _and_ it doesn't
get the "one-shot" thing right (which is probably OK, as we use it only
when exec fails). But we could probably teach run-command to fdopen(),
and get rid of it entirely (in favor of teaching vreportf to take a
FILE* handle instead of assuming stderr).
So I dunno. I think the solution here works fine, but maybe we should be
taking the opportunity to simplify.
usage.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 64 insertions(+), 8 deletions(-)
diff --git a/usage.c b/usage.c
index ed14645..78b0d75 100644
--- a/usage.c
+++ b/usage.c
@@ -6,23 +6,79 @@
#include "git-compat-util.h"
#include "cache.h"
+/*
+ * This buffer allows you to try formatting a full message, but if malloc
+ * fails, will fall back to a fixed-size buffer and truncate the message.
+ * If we truncate the message, it adds a note indicating so.
+ *
+ * No initialization is necessary before robust_buf_fmt, and after it returns,
+ * "buf" points to the contents (whether truncated or not). You should always
+ * robust_buf_free the result, which handles both cases.
+ */
+struct robust_buf {
+ char *buf;
+ char fixed[4096];
+};
+
+static int robust_buf_fmt(struct robust_buf *rb,
+ const char *fmt, va_list ap)
+{
+ static const char trunc[] = " [message truncated]";
+ static const char broken[] = "BUG: your vsnprintf is broken";
+ va_list cp;
+ int len;
+
+ va_copy(cp, ap);
+ len = vsnprintf(rb->fixed, sizeof(rb->fixed), fmt, cp);
+ va_end(cp);
+
+ if (len < 0) {
+ memcpy(rb->fixed, broken, sizeof(broken));
+ rb->buf = rb->fixed;
+ return sizeof(broken) - 1;
+ }
+ if (len < sizeof(rb->fixed)) {
+ rb->buf = rb->fixed;
+ return len;
+ }
+
+ rb->buf = malloc(len + 1);
+ if (!rb->buf) {
+ memcpy(rb->fixed + sizeof(rb->fixed) - sizeof(trunc),
+ trunc, sizeof(trunc));
+ rb->buf = rb->fixed;
+ return sizeof(rb->fixed) - 1;
+ }
+
+ if (vsnprintf(rb->buf, len + 1, fmt, ap) >= len)
+ ; /* insatiable vsnprintf, nothing we can do */
+ return len;
+}
+
+static void robust_buf_free(struct robust_buf *rb)
+{
+ if (rb->buf != rb->fixed)
+ free(rb->buf);
+}
+
void vreportf(const char *prefix, const char *err, va_list params)
{
- char msg[4096];
- vsnprintf(msg, sizeof(msg), err, params);
- fprintf(stderr, "%s%s\n", prefix, msg);
+ struct robust_buf msg;
+ robust_buf_fmt(&msg, err, params);
+ fprintf(stderr, "%s%s\n", prefix, msg.buf);
+ robust_buf_free(&msg);
}
void vwritef(int fd, const char *prefix, const char *err, va_list params)
{
- char msg[4096];
- int len = vsnprintf(msg, sizeof(msg), err, params);
- if (len > sizeof(msg))
- len = sizeof(msg);
+ struct robust_buf msg;
+ int len = robust_buf_fmt(&msg, err, params);
write_in_full(fd, prefix, strlen(prefix));
- write_in_full(fd, msg, len);
+ write_in_full(fd, msg.buf, len);
write_in_full(fd, "\n", 1);
+
+ robust_buf_free(&msg);
}
static NORETURN void usage_builtin(const char *err, va_list params)
--
2.5.0.417.g2db689c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html