Re: [PATCH] usage: try harder to format very long error messages

> 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@peff.net>
> ---
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html