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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2020-06-19 13:17:19-0400, "Randall S. Becker" <rsbecker@xxxxxxxxxxxxx> 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@xxxxxxxxxx wrote:
> > > From: "Randall S. Becker" <rsbecker@xxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxx>
> > > ---
> > >  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);

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux