Re: [PATCH v4 1/1] vreportf(): avoid relying on stdio buffering
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 2, 2019, 04:05 UTC
- Message-ID
- <xmqqd0ea6hy8.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <f6d6c8122abbd6339cf83309ac3761bbdac44023.1572432276.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 14 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de> > > The MSVC runtime behavior differs from glibc's with respect to > `fprintf(stderr, ...)` in that the former writes out the message > character by character. > ... > Let's avoid this predicament altogether by rendering the entire message, > including the prefix and the trailing newline, into the buffer we > already have (and which is still fixed size) and then write it out via > `write_in_full()`. > ... > The process may have written to `stderr` and there may be something left > in the buffer kept in the stdio layer. Call `fflush(stderr)` before > writing the message we prepare in this function.
Thanks.
This is for future reference and not a comment for you alone, but we probably do not want to single out glibc like this proposed log message does in its first paragraph, as if in the author's mind, the world has only two systems. It invites questions like "what about various BSD variants? how does musl behave?" that may lead to a suggestion to further update it to "differs from most everybody else", or we may end up saying "MSVC, X and Y share this problem unlike all others". Either way, at that point, the original singling out of glibc becomes meaningless.
"Platform X has this issue and here is a way to avoid that on everybody" is a good description of the motivation, and the mention of the behaviour of MSVC runtime is what we want to see there.
The MSVC runtime writes out the message character by
character given `fprintf(stderr, ...)`.It is not necessary to add "There may be other platforms that share the same issue, but MSVC alone is already important enough so we do not list them here". It is trivially true and obvious ;-)
As I said, the above is for future reference for everybody; it's cutting close to the final, so let's queue this (and the other one) as-is.
Thanks for working on this fix.
Show 44 quoted lines
> Helped-by: Jeff King <peff@peff.net>
> Helped-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> Helped-by: SZEDER Gábor <szeder.dev@gmail.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> usage.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/usage.c b/usage.c
> index 2fdb20086b..58fb5fff5f 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -9,14 +9,26 @@
> void vreportf(const char *prefix, const char *err, va_list params)
> {
> char msg[4096];
> - char *p;
> + char *p, *pend = msg + sizeof(msg);
> + size_t prefix_len = strlen(prefix);
>
> - vsnprintf(msg, sizeof(msg), err, params);
> - for (p = msg; *p; p++) {
> + if (sizeof(msg) <= prefix_len) {
> + fprintf(stderr, "BUG!!! too long a prefix '%s'\n", prefix);
> + abort();
> + }
> + memcpy(msg, prefix, prefix_len);
> + p = msg + prefix_len;
> + if (vsnprintf(p, pend - p, err, params) < 0)
> + *p = '\0'; /* vsnprintf() failed, clip at prefix */
> +
> + for (; p != pend - 1 && *p; p++) {
> if (iscntrl(*p) && *p != '\t' && *p != '\n')
> *p = '?';
> }
> - fprintf(stderr, "%s%s\n", prefix, msg);
> +
> + *(p++) = '\n'; /* we no longer need a NUL */
> + fflush(stderr);
> + write_in_full(2, msg, p - msg);
> }
>
> static NORETURN void usage_builtin(const char *err, va_list params)