| Message ID | 20200619150445.4380-2-randall.s.becker@rogers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Replace strbuf_write_fd with write_in_full | expand |
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. Nit: We wouldn't want the pair of {}. > + } > close(report); > > /* > -- > 2.21.0 >
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. > 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? Randall
"Randall S. Becker" <rsbecker@nexbridge.com> writes: >> > + 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. > >> 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? I think he means that a block that consists of a single statement (i.e. call to die()) does not have to be enclosed in {}. (partial quote from Documentation/CodingGuidelines): - We avoid using braces unnecessarily. I.e. if (bla) { x = 1; } is frowned upon. But there are a few exceptions:
On June 19, 2020 3:31 PM, Junio C Hamano wrote: > To: Randall S. Becker <rsbecker@nexbridge.com> > Cc: 'Đoàn Trần Công Danh' <congdanhqx@gmail.com>; > 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 > > "Randall S. Becker" <rsbecker@nexbridge.com> writes: > > >> > + 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. > > > >> 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? > > I think he means that a block that consists of a single statement (i.e. call to > die()) does not have to be enclosed in {}. > > (partial quote from Documentation/CodingGuidelines): > > - We avoid using braces unnecessarily. I.e. > > if (bla) { > x = 1; > } > > is frowned upon. But there are a few exceptions: I get that. I was trying to maintain visual consistency with the rest of bugreport.c. Will redo it.
On Fri, Jun 19, 2020 at 11:04:43AM -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. This also fixes problems with EINTR, etc. > - 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); > + } I agree with the other comment not to bother reporting the contents. But it is worth using die_errno() so we can see what happened. I.e.: die_errno(_("unable to write to %s"), report_path.buf); would match our usual messages, and you'd get: unable to write to foo.out: No space left on device or similar. -Peff
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); > > 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.
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
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); + } close(report); /*