RE: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full

["Randall S. Becker" <rsbecker@nexbridge.com>]

On June 19, 2020 7:02 PM, Ðoàn Tr?n Công Danh wrote:
> To: Randall S. Becker <rsbecker@nexbridge.com>
> Cc: randall.s.becker@rogers.com; git@vger.kernel.org
> Subject: Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with
> write_in_full
> 
> On 2020-06-19 13:17:19-0400, "Randall S. Becker"
> <rsbecker@nexbridge.com> wrote:
> > On June 19, 2020 12:36 PM, Đoàn Trần Công Danh wrote:
> > > On 2020-06-19 11:04:43-0400, randall.s.becker@rogers.com wrote:
> > > > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> > > >
> > > > The strbuf_write_fd method did not provide checks for buffers
> > > > larger than MAX_IO_SIZE. Replacing with write_in_full ensures the
> > > > entire buffer will always be written to disk or report an error and die.
> > > >
> > > > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > > > ---
> > > >  bugreport.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/bugreport.c b/bugreport.c index
> > > > aa8a489c35..bc359b7fa8
> > > > 100644
> > > > --- a/bugreport.c
> > > > +++ b/bugreport.c
> > > > @@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)
> > > >  		die(_("couldn't create a new file at '%s'"), report_path.buf);
> > > >  	}
> > > >
> > > > -	strbuf_write_fd(&buffer, report);
> > > > +	if (write_in_full(report, buffer.buf, buffer.len) < 0) {
> > > > +		die(_("couldn't write report contents '%s' to file '%s'"),
> > > > +			buffer.buf, report_path.buf);
> > >
> > > Doesn't this dump the whole report to the stderr?
> > > If it's the case, the error would be very hard to grasp.
> >
> > Where else can we put the error? By this point, we're likely out of
> > disk or virtual memory.
> 
> Sorry, I forgot to suggest an alternatives.
> 
> I was thinking about ignore the report when writing the last email.
> 
> Since, the report is likely consists of multiple lines of text, and they likely
> contains some single quote themselves.
> 
> Now, I think a bit more, I think it's way better to write as:
> 
> 	if (write_in_full(report, buffer.buf, buffer.len) < 0)
> 		die(_("couldn't write the report contents to file '%s'.\n\n"
> 		"The original report was:\n\n"
> 		"%s\n"), report_path.buf, buffer.buf);

I went with Peff's suggestion of using die_error in v2. Thanks though.

> > > Nit: We wouldn't want the pair of {}.
> > >
> > > > +	}
> > > >  	close(report);
> >
> > I'm not sure what you mean in this nit? {} are balanced. You mean in the
> error message?
> 
> Our style guides says we wouldn't want this pair of {} if it's single statement.

Fixed in v2

Regards,
Randall