On June 19, 2020 7:02 PM, Ðoàn Tr?n Công Danh wrote:
> To: Randall S. Becker <rsbecker@xxxxxxxxxxxxx>
> Cc: randall.s.becker@xxxxxxxxxx; git@xxxxxxxxxxxxxxx
> 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@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);
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
