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

From: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Date: Jun 19, 2020, 23:01 UTC
Message-ID: <20200619230141.GC5027@danh.dev>
In-Reply-To: <02a501d6465d$80366680$80a33380$@nexbridge.com>

On 2020-06-19 13:17:19-0400, "Randall S. Becker" <rsbecker@nexbridge.com> wrote:

Show 31 quoted lines

> 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);

Show 6 quoted lines

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

-- 
Danh