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

From: Jeff King <peff@peff.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Junio C Hamano <gitster@pobox.com>,
	Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Subject: Re: [PATCH 1/1] vreportf(): avoid buffered write in favor of unbuffered one
Date: Tue, 29 Oct 2019 09:49:32 -0400	[thread overview]
Message-ID: <20191029134932.GA2843@sigill.intra.peff.net> (raw)
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:

> > 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.

> > > -	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.

> 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