git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/1] vreportf(): avoid buffered write in favor of unbuffered one

From
Jeff King <peff@peff.net>
Date
Oct 29, 2019, 13:49 UTC
Message-ID
<20191029134932.GA2843@sigill.intra.peff.net>
In-Reply-To
<nycvar.QRO.7.76.6.1910291222500.46@tvgsbejvaqbjf.bet>
On Tue, Oct 29, 2019 at 01:30:51PM +0100, Johannes Schindelin wrote:
Show 13 quoted lines
> > Also, this is only to help output from us that goes via vreportf() and
> > other codepaths in us that use stdio to write to the standard error
> > stream can still get mixed on Windows (I think the answer is yes,
> > because we wouldn't need fflush() in this patch if we are covering all
> > writes to the standard error stream)?
> 
> Yes, `write()` can get interrupted, so there is still a slight chance of
> interleaving.
> 
> However, with `fprintf()`, apparently the MSVC runtime essentially
> writes and flushes one character at a time, which will make it _much_
> more likely that two competing processes write interleaved messages to
> `stderr`.
Wow, they have truly taken "unbuffered" to a whole new level.

I don't mind seeing this for all platforms, though. I can't think of any downside, and having one less moving part to contend with in our error-reporting code seems like a good thing.

Show 9 quoted lines
> > > -	vsnprintf(msg, sizeof(msg), err, params);
> > > +	size_t off = strlcpy(msg, prefix, sizeof(msg));
> >
> > Like snprintf(3) the strlcpy() and strlcat() functions return the
> > total length of the string they tried to create.  For strlcpy() that
> > means the length of src.
> 
> True (I misread `compat/strlcpy.c` and forgot to consult the
> documentation). This length can be longer than `msg`, of course.

I'd recommend xsnprintf() here. If we have a prefix longer than our vreportf() buffer, I think a BUG() is the right outcome.

Show 7 quoted lines
> I `git grep`ed and saw that only very short `prefix`es are hard-coded.
> So that is a hypothetical concern.
> 
> However, Alex also indicated his discomfort with this, so I will change
> the code to account for a `prefix` that is too long (the entire error
> message will be clipped away in that case, which is unfortunate, but to
> be expected).

I'd disagree here. Any caller sending an arbitrarily-large prefix is holding it wrong, and we'd probably want to know as soon as possible (and a BUG() is our best bet there).

-Peff
Previous: Johannes SchindelinNext: Johannes Schindelin
Message 5 of 34 in “Fix t5516 flakiness in Visual Studio builds”
  1. 0/1 Fix t5516 flakiness in Visual Studio buildsJohannes Schindelin via GitGitGadget, Oct 28, 2019
  2. 1/1 vreportf(): avoid buffered write in favor of unbuffered oneJohannes Schindelin via GitGitGadget, Oct 28, 2019
  3. Junio C HamanoOct 29, 2019
  4. Johannes SchindelinOct 29, 2019
  5. Jeff KingOct 29, 2019
  6. Johannes SchindelinOct 29, 2019
  7. Jeff KingOct 29, 2019
  8. Johannes SchindelinOct 29, 2019
  9. Junio C HamanoOct 30, 2019
  10. Junio C HamanoOct 29, 2019
  11. SZEDER GáborOct 29, 2019
  12. Johannes SchindelinOct 29, 2019
  13. Jeff KingOct 29, 2019
  14. Johannes SchindelinOct 29, 2019
  15. 0/1 Fix t5516 flakiness in Visual Studio buildsJohannes Schindelin via GitGitGadget, Oct 29, 2019
  16. 1/1 vreportf(): avoid relying on stdio bufferingJohannes Schindelin via GitGitGadget, Oct 29, 2019
  17. Alexandr MiloslavskiyOct 29, 2019
  18. Johannes SchindelinOct 29, 2019
  19. Jeff KingOct 29, 2019
  20. Alexandr MiloslavskiyOct 29, 2019
  21. Alexandr MiloslavskiyOct 29, 2019
  22. 0/1 Fix t5516 flakiness in Visual Studio buildsJohannes Schindelin via GitGitGadget, Oct 29, 2019
  23. 1/1 vreportf(): avoid relying on stdio bufferingJohannes Schindelin via GitGitGadget, Oct 29, 2019
  24. Jeff KingOct 29, 2019
  25. Johannes SchindelinOct 30, 2019
  26. Jeff KingOct 31, 2019
  27. Johannes SchindelinOct 31, 2019
  28. Jeff KingOct 31, 2019
  29. Johannes SchindelinNov 1, 2019
  30. Junio C HamanoOct 30, 2019
  31. Johannes SchindelinOct 30, 2019
  32. 0/1 Fix t5516 flakiness in Visual Studio buildsJohannes Schindelin via GitGitGadget, Oct 30, 2019
  33. 1/1 vreportf(): avoid relying on stdio bufferingJohannes Schindelin via GitGitGadget, Oct 30, 2019
  34. Junio C HamanoNov 2, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.