Re: [Possible Bug] Use of write on size-limited platforms
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 19, 2020, 19:35 UTC
- Message-ID
- <xmqq4kr6hmrs.fsf@gitster.c.googlers.com>
- In-Reply-To
- <028c01d6464b$235346c0$69f9d440$@nexbridge.com>
"Randall S. Becker" <rsbecker@nexbridge.com> writes:
Show 26 quoted lines
> On June 16, 2020 4:02 AM, Peff wrote: >> To: Randall S. Becker <rsbecker@nexbridge.com> >> Cc: git@vger.kernel.org >> Subject: Re: [Possible Bug] Use of write on size-limited platforms >> >> On Mon, Jun 15, 2020 at 06:38:34PM -0400, Randall S. Becker wrote: >> >> > > So I think this really ought to be using write_in_full(). There's >> > > only one caller, and I think it would be improved by the switch. Do >> > > you want to write a patch? >> > > >> > > You could make an argument that the fwrite() version ought to also >> > > loop, since it's possible to get a partial write there, too. But we >> > > don't do that in general. I suspect in practice most stdio >> > > implementations will keep writing until they see an error, and most >> > > callers don't bother checking stdio errors at all, or use ferror(). >> > >> > I'll give the patch a go. It is very simple. Would you suggest >> > removing the strbuf_write_fd() as part of this patch since it only >> > impacts bugreport.c? >> >> I could go either way on that. IMHO it isn't offering much over a bare >> write_in_full() call. The "don't call write() if there are 0 bytes" >> logic is part of write_in_full() already. > > Patch delivered.
I think these go in the right direction and would be good with minor fixes. I do agree with the other reviewer that dumping all the contents of the buffer to die() may not be what we want (do we even need to see the contents at all?). Even though I do not mind too much about the unneeded {braces}, I'd prefer to see it fixed if we are rerolling the patch for other reason anyway. And I think 2 and 3 can and should be a single patch---if we are removing a function that is no longer used, we should remove both its decl and defn at the same time.
Thanks.