{"thread":{"id":"53719","subject":"[Patch v1 3/3] strbuf.h: remove declaration of deprecated strbuf_write_fd method.","startedAt":"2020-06-19T16:12:15Z","lastAt":"2020-06-19T23:26:42Z","messageCount":15,"participants":["randall.s.becker@rogers.com","Đoàn Trần Công Danh","Randall S. Becker","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"400137","messageId":"20200619150445.4380-4-randall.s.becker@rogers.com","threadId":"53719","inReplyTo":"20200619150445.4380-1-randall.s.becker@rogers.com","subject":"[Patch v1 3/3] strbuf.h: remove declaration of deprecated strbuf_write_fd method.","fromName":"","fromEmail":"randall.s.becker@rogers.com","sentAt":"2020-06-19T15:04:45Z","receivedAt":"2020-06-19T16:12:15Z","isPatch":true,"sender":{"key":"randall.s.becker@rogers.com","avatar":null},"body":"From: \"Randall S. Becker\" \n\nSigned-off-by: Randall S. Becker \n---\n strbuf.h | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 7062eb6410..223ee2094a 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -473,7 +473,6 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n * NUL bytes.\n */\n ssize_t strbuf_write(struct strbuf *sb, FILE *stream);\n-ssize_t strbuf_write_fd(struct strbuf *sb, int fd);\n \n /**\n * Read a line from a FILE *, overwriting the existing contents of\n-- \n2.21.0\n\n"},{"id":"400138","messageId":"20200619150445.4380-2-randall.s.becker@rogers.com","threadId":"53719","inReplyTo":"20200619150445.4380-1-randall.s.becker@rogers.com","subject":"[Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full","fromName":"","fromEmail":"randall.s.becker@rogers.com","sentAt":"2020-06-19T15:04:43Z","receivedAt":"2020-06-19T16:12:21Z","isPatch":true,"sender":{"key":"randall.s.becker@rogers.com","avatar":null},"body":"From: \"Randall S. Becker\" \n\nThe strbuf_write_fd method did not provide checks for buffers larger\nthan MAX_IO_SIZE. Replacing with write_in_full ensures the entire\nbuffer will always be written to disk or report an error and die.\n\nSigned-off-by: Randall S. Becker \n---\n bugreport.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/bugreport.c b/bugreport.c\nindex aa8a489c35..bc359b7fa8 100644\n--- a/bugreport.c\n+++ b/bugreport.c\n@@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)\n \t\tdie(_(\"couldn't create a new file at '%s'\"), report_path.buf);\n \t}\n \n-\tstrbuf_write_fd(&buffer, report);\n+\tif (write_in_full(report, buffer.buf, buffer.len) < 0) {\n+\t\tdie(_(\"couldn't write report contents '%s' to file '%s'\"),\n+\t\t\tbuffer.buf, report_path.buf);\n+\t}\n \tclose(report);\n \n \t/*\n-- \n2.21.0\n\n"},{"id":"400139","messageId":"20200619150445.4380-3-randall.s.becker@rogers.com","threadId":"53719","inReplyTo":"20200619150445.4380-1-randall.s.becker@rogers.com","subject":"[Patch v1 2/3] strbuf.c: remove unreferenced strbuf_write_fd method.","fromName":"","fromEmail":"randall.s.becker@rogers.com","sentAt":"2020-06-19T15:04:44Z","receivedAt":"2020-06-19T16:12:24Z","isPatch":true,"sender":{"key":"randall.s.becker@rogers.com","avatar":null},"body":"From: \"Randall S. Becker\" \n\nstrbuf_write_fd was only used in bugreport.c. Since that file now uses\nwrite_in_full, this method is no longer needed.\n\nSigned-off-by: Randall S. Becker \n---\n strbuf.c | 5 -----\n 1 file changed, 5 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 2f1a7d3209..e3397cc4c7 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -556,11 +556,6 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)\n \treturn sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;\n }\n \n-ssize_t strbuf_write_fd(struct strbuf *sb, int fd)\n-{\n-\treturn sb->len ? write(fd, sb->buf, sb->len) : 0;\n-}\n-\n #define STRBUF_MAXLINK (2*PATH_MAX)\n \n int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n-- \n2.21.0\n\n"},{"id":"400140","messageId":"20200619150445.4380-1-randall.s.becker@rogers.com","threadId":"53719","inReplyTo":"20200619150445.4380-1-randall.s.becker.ref@rogers.com","subject":"[Patch v1 0/3] Replace strbuf_write_fd with write_in_full","fromName":"","fromEmail":"randall.s.becker@rogers.com","sentAt":"2020-06-19T15:04:42Z","receivedAt":"2020-06-19T16:12:25Z","isPatch":true,"sender":{"key":"randall.s.becker@rogers.com","avatar":null},"body":"From: \"Randall S. Becker\" \n\nThe strbuf_write_fd method does not check whether the buffer exceeds\nMAX_IO_SIZE on the target platform. This fix replaces the use of that\nmethod with write_in_full, which does. Since this is the only use of\nstrbuf_write_fd, and since the method was unsafe, it has been removed\nfrom strbuf.c and strbuf.h.\n\nRandall S. Becker (3):\n bugreport.c: replace strbuf_write_fd with write_in_full\n strbuf.c: remove unreferenced strbuf_write_fd method.\n strbuf.h: remove declaration of deprecated strbuf_write_fd method.\n\n bugreport.c | 5 ++++-\n strbuf.c | 5 -----\n strbuf.h | 1 -\n 3 files changed, 4 insertions(+), 7 deletions(-)\n\n-- \n2.21.0\n\n"},{"id":"400145","messageId":"20200619163530.GB5027@danh.dev","threadId":"53719","inReplyTo":"20200619150445.4380-2-randall.s.becker@rogers.com","subject":"Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-06-19T16:35:30Z","receivedAt":"2020-06-19T16:35:36Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-06-19 11:04:43-0400, randall.s.becker@rogers.com wrote:\n> From: \"Randall S. Becker\" \n> \n> The strbuf_write_fd method did not provide checks for buffers larger\n> than MAX_IO_SIZE. Replacing with write_in_full ensures the entire\n> buffer will always be written to disk or report an error and die.\n> \n> Signed-off-by: Randall S. Becker \n> ---\n> bugreport.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/bugreport.c b/bugreport.c\n> index aa8a489c35..bc359b7fa8 100644\n> --- a/bugreport.c\n> +++ b/bugreport.c\n> @@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)\n> \t\tdie(_(\"couldn't create a new file at '%s'\"), report_path.buf);\n> \t}\n> \n> -\tstrbuf_write_fd(&buffer, report);\n> +\tif (write_in_full(report, buffer.buf, buffer.len) < 0) {\n> +\t\tdie(_(\"couldn't write report contents '%s' to file '%s'\"),\n> +\t\t\tbuffer.buf, report_path.buf);\n\nDoesn't this dump the whole report to the stderr?\nIf it's the case, the error would be very hard to grasp.\n\nNit: We wouldn't want the pair of {}.\n\n> +\t}\n> \tclose(report);\n> \n> \t/*\n> -- \n> 2.21.0\n> \n\n-- \nDanh\n"},{"id":"400151","messageId":"02a501d6465d$80366680$80a33380$@nexbridge.com","threadId":"53719","inReplyTo":"20200619163530.GB5027@danh.dev","subject":"RE: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2020-06-19T17:17:19Z","receivedAt":"2020-06-19T17:17:35Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 19, 2020 12:36 PM, Đoàn Trần Công Danh wrote:\n> On 2020-06-19 11:04:43-0400, randall.s.becker@rogers.com wrote:\n> > From: \"Randall S. Becker\" \n> >\n> > The strbuf_write_fd method did not provide checks for buffers larger\n> > than MAX_IO_SIZE. Replacing with write_in_full ensures the entire\n> > buffer will always be written to disk or report an error and die.\n> >\n> > Signed-off-by: Randall S. Becker \n> > ---\n> > bugreport.c | 5 ++++-\n> > 1 file changed, 4 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/bugreport.c b/bugreport.c index aa8a489c35..bc359b7fa8\n> > 100644\n> > --- a/bugreport.c\n> > +++ b/bugreport.c\n> > @@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)\n> > \t\tdie(_(\"couldn't create a new file at '%s'\"), report_path.buf);\n> > \t}\n> >\n> > -\tstrbuf_write_fd(&buffer, report);\n> > +\tif (write_in_full(report, buffer.buf, buffer.len) < 0) {\n> > +\t\tdie(_(\"couldn't write report contents '%s' to file '%s'\"),\n> > +\t\t\tbuffer.buf, report_path.buf);\n> \n> Doesn't this dump the whole report to the stderr?\n> If it's the case, the error would be very hard to grasp.\n\nWhere else can we put the error? By this point, we're likely out of disk or virtual memory.\n\n> Nit: We wouldn't want the pair of {}.\n> \n> > +\t}\n> > \tclose(report);\n\nI'm not sure what you mean in this nit? {} are balanced. You mean in the error message?\n\nRandall\n\n"},{"id":"400209","messageId":"xmqqimfmhmzs.fsf@gitster.c.googlers.com","threadId":"53719","inReplyTo":"02a501d6465d$80366680$80a33380$@nexbridge.com","subject":"Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-19T19:30:31Z","receivedAt":"2020-06-19T19:30:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" writes:\n\n>> > +\tif (write_in_full(report, buffer.buf, buffer.len) < 0) {\n>> > +\t\tdie(_(\"couldn't write report contents '%s' to file '%s'\"),\n>> > +\t\t\tbuffer.buf, report_path.buf);\n>> \n>> Doesn't this dump the whole report to the stderr?\n>> If it's the case, the error would be very hard to grasp.\n>\n> Where else can we put the error? By this point, we're likely out of disk or virtual memory.\n>\n>> Nit: We wouldn't want the pair of {}.\n>> \n>> > +\t}\n>> > \tclose(report);\n>\n> I'm not sure what you mean in this nit? {} are balanced. You mean in the error message?\n\nI think he means that a block that consists of a single statement\n(i.e. call to die()) does not have to be enclosed in {}.\n\n(partial quote from Documentation/CodingGuidelines):\n\n - We avoid using braces unnecessarily. I.e.\n\n\tif (bla) {\n\t\tx = 1;\n\t}\n\n is frowned upon. But there are a few exceptions:\n"},{"id":"400210","messageId":"xmqqeeqahmz7.fsf@gitster.c.googlers.com","threadId":"53719","inReplyTo":"20200619150445.4380-3-randall.s.becker@rogers.com","subject":"Re: [Patch v1 2/3] strbuf.c: remove unreferenced strbuf_write_fd method.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-19T19:30:52Z","receivedAt":"2020-06-19T19:31:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"randall.s.becker@rogers.com writes:\n\n> From: \"Randall S. Becker\" \n>\n> strbuf_write_fd was only used in bugreport.c. Since that file now uses\n> write_in_full, this method is no longer needed.\n\nYay!\n\n\n> Signed-off-by: Randall S. Becker \n> ---\n> strbuf.c | 5 -----\n> 1 file changed, 5 deletions(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 2f1a7d3209..e3397cc4c7 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -556,11 +556,6 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)\n> \treturn sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;\n> }\n> \n> -ssize_t strbuf_write_fd(struct strbuf *sb, int fd)\n> -{\n> -\treturn sb->len ? write(fd, sb->buf, sb->len) : 0;\n> -}\n> -\n> #define STRBUF_MAXLINK (2*PATH_MAX)\n> \n> int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n"},{"id":"400211","messageId":"xmqqa70yhmxz.fsf@gitster.c.googlers.com","threadId":"53719","inReplyTo":"20200619150445.4380-4-randall.s.becker@rogers.com","subject":"Re: [Patch v1 3/3] strbuf.h: remove declaration of deprecated strbuf_write_fd method.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-19T19:31:36Z","receivedAt":"2020-06-19T19:31:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"randall.s.becker@rogers.com writes:\n\n> From: \"Randall S. Becker\" \n>\n> Signed-off-by: Randall S. Becker \n> ---\n> strbuf.h | 1 -\n> 1 file changed, 1 deletion(-)\n\nI think this should be part of 2/3 (otherwise we'd have a decl that\nnobody references that declares a function that nobody implements).\n\n> diff --git a/strbuf.h b/strbuf.h\n> index 7062eb6410..223ee2094a 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -473,7 +473,6 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n> * NUL bytes.\n> */\n> ssize_t strbuf_write(struct strbuf *sb, FILE *stream);\n> -ssize_t strbuf_write_fd(struct strbuf *sb, int fd);\n> \n> /**\n> * Read a line from a FILE *, overwriting the existing contents of\n"},{"id":"400212","messageId":"02c101d64670$b72ab840$258028c0$@nexbridge.com","threadId":"53719","inReplyTo":"xmqqa70yhmxz.fsf@gitster.c.googlers.com","subject":"RE: [Patch v1 3/3] strbuf.h: remove declaration of deprecated strbuf_write_fd method.","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2020-06-19T19:34:52Z","receivedAt":"2020-06-19T19:35:03Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 19, 2020 3:32 PM, Junio C Hamano wrote:\n> randall.s.becker@rogers.com writes:\n> \n> > From: \"Randall S. Becker\" \n> >\n> > Signed-off-by: Randall S. Becker \n> > ---\n> > strbuf.h | 1 -\n> > 1 file changed, 1 deletion(-)\n> \n> I think this should be part of 2/3 (otherwise we'd have a decl that nobody\n> references that declares a function that nobody implements).\n\nIf I understand, combined the strbuf.c and strbuf.h modification into a\nsingle commit, correct? I normally would do that but missed this part of the\ncontribution standard. If so, I will create v2 accordingly.\n\n> \n> > diff --git a/strbuf.h b/strbuf.h\n> > index 7062eb6410..223ee2094a 100644\n> > --- a/strbuf.h\n> > +++ b/strbuf.h\n> > @@ -473,7 +473,6 @@ int strbuf_readlink(struct strbuf *sb, const char\n> *path, size_t hint);\n> > * NUL bytes.\n> > */\n> > ssize_t strbuf_write(struct strbuf *sb, FILE *stream); -ssize_t\n> > strbuf_write_fd(struct strbuf *sb, int fd);\n> >\n> > /**\n> > * Read a line from a FILE *, overwriting the existing contents of\n\n"},{"id":"400214","messageId":"02c201d64671$13c9a840$3b5cf8c0$@nexbridge.com","threadId":"53719","inReplyTo":"xmqqimfmhmzs.fsf@gitster.c.googlers.com","subject":"RE: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2020-06-19T19:37:27Z","receivedAt":"2020-06-19T19:37:39Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 19, 2020 3:31 PM, Junio C Hamano wrote:\n> To: Randall S. Becker \n> Cc: 'Đoàn Trần Công Danh' ;\n> randall.s.becker@rogers.com; git@vger.kernel.org\n> Subject: Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with\n> write_in_full\n> \n> \"Randall S. Becker\" writes:\n> \n> >> > +\tif (write_in_full(report, buffer.buf, buffer.len) < 0) {\n> >> > +\t\tdie(_(\"couldn't write report contents '%s' to file '%s'\"),\n> >> > +\t\t\tbuffer.buf, report_path.buf);\n> >>\n> >> Doesn't this dump the whole report to the stderr?\n> >> If it's the case, the error would be very hard to grasp.\n> >\n> > Where else can we put the error? By this point, we're likely out of disk or\n> virtual memory.\n> >\n> >> Nit: We wouldn't want the pair of {}.\n> >>\n> >> > +\t}\n> >> > \tclose(report);\n> >\n> > I'm not sure what you mean in this nit? {} are balanced. You mean in the\n> error message?\n> \n> I think he means that a block that consists of a single statement (i.e. call to\n> die()) does not have to be enclosed in {}.\n> \n> (partial quote from Documentation/CodingGuidelines):\n> \n> - We avoid using braces unnecessarily. I.e.\n> \n> \tif (bla) {\n> \t\tx = 1;\n> \t}\n> \n> is frowned upon. But there are a few exceptions:\n\nI get that. I was trying to maintain visual consistency with the rest of bugreport.c. Will redo it.\n\n"},{"id":"400215","messageId":"20200619194750.GA722967@coredump.intra.peff.net","threadId":"53719","inReplyTo":"20200619150445.4380-2-randall.s.becker@rogers.com","subject":"Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-19T19:47:50Z","receivedAt":"2020-06-19T19:47:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 19, 2020 at 11:04:43AM -0400, randall.s.becker@rogers.com wrote:\n\n> From: \"Randall S. Becker\" \n> \n> The strbuf_write_fd method did not provide checks for buffers larger\n> than MAX_IO_SIZE. Replacing with write_in_full ensures the entire\n> buffer will always be written to disk or report an error and die.\n\nThis also fixes problems with EINTR, etc.\n\n> -\tstrbuf_write_fd(&buffer, report);\n> +\tif (write_in_full(report, buffer.buf, buffer.len) < 0) {\n> +\t\tdie(_(\"couldn't write report contents '%s' to file '%s'\"),\n> +\t\t\tbuffer.buf, report_path.buf);\n> +\t}\n\nI agree with the other comment not to bother reporting the contents. But\nit is worth using die_errno() so we can see what happened. I.e.:\n\n die_errno(_(\"unable to write to %s\"), report_path.buf);\n\nwould match our usual messages, and you'd get:\n\n unable to write to foo.out: No space left on device\n\nor similar.\n\n-Peff\n"},{"id":"400216","messageId":"20200619194901.GB722967@coredump.intra.peff.net","threadId":"53719","inReplyTo":"20200619150445.4380-3-randall.s.becker@rogers.com","subject":"Re: [Patch v1 2/3] strbuf.c: remove unreferenced strbuf_write_fd method.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-19T19:49:01Z","receivedAt":"2020-06-19T19:49:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 19, 2020 at 11:04:44AM -0400, randall.s.becker@rogers.com wrote:\n\n> From: \"Randall S. Becker\" \n> \n> strbuf_write_fd was only used in bugreport.c. Since that file now uses\n> write_in_full, this method is no longer needed.\n\nJust because there are no callers _now_ does not necessarily mean that\nit's a good idea to get rid of a function. I think the real argument is\nthat we don't expect new ones, because it's a badly designed function\n(for the reasons in our earlier discussion). Maybe worth summarizing\nhere.\n\n-Peff\n"},{"id":"400242","messageId":"20200619230141.GC5027@danh.dev","threadId":"53719","inReplyTo":"02a501d6465d$80366680$80a33380$@nexbridge.com","subject":"Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-06-19T23:01:41Z","receivedAt":"2020-06-19T23:01:46Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-06-19 13:17:19-0400, \"Randall S. Becker\" wrote:\n> On June 19, 2020 12:36 PM, Đoàn Trần Công Danh wrote:\n> > On 2020-06-19 11:04:43-0400, randall.s.becker@rogers.com wrote:\n> > > From: \"Randall S. Becker\" \n> > >\n> > > The strbuf_write_fd method did not provide checks for buffers larger\n> > > than MAX_IO_SIZE. Replacing with write_in_full ensures the entire\n> > > buffer will always be written to disk or report an error and die.\n> > >\n> > > Signed-off-by: Randall S. Becker \n> > > ---\n> > > bugreport.c | 5 ++++-\n> > > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/bugreport.c b/bugreport.c index aa8a489c35..bc359b7fa8\n> > > 100644\n> > > --- a/bugreport.c\n> > > +++ b/bugreport.c\n> > > @@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)\n> > > \t\tdie(_(\"couldn't create a new file at '%s'\"), report_path.buf);\n> > > \t}\n> > >\n> > > -\tstrbuf_write_fd(&buffer, report);\n> > > +\tif (write_in_full(report, buffer.buf, buffer.len) < 0) {\n> > > +\t\tdie(_(\"couldn't write report contents '%s' to file '%s'\"),\n> > > +\t\t\tbuffer.buf, report_path.buf);\n> > \n> > Doesn't this dump the whole report to the stderr?\n> > If it's the case, the error would be very hard to grasp.\n> \n> Where else can we put the error? By this point, we're likely out of\n> disk or virtual memory.\n\nSorry, I forgot to suggest an alternatives.\n\nI was thinking about ignore the report when writing the last email.\n\nSince, the report is likely consists of multiple lines of text,\nand they likely contains some single quote themselves.\n\nNow, I think a bit more, I think it's way better to write as:\n\n\tif (write_in_full(report, buffer.buf, buffer.len) < 0)\n\t\tdie(_(\"couldn't write the report contents to file '%s'.\\n\\n\"\n\t\t\"The original report was:\\n\\n\"\n\t\t\"%s\\n\"), report_path.buf, buffer.buf);\n\n> > Nit: We wouldn't want the pair of {}.\n> > \n> > > +\t}\n> > > \tclose(report);\n> \n> I'm not sure what you mean in this nit? {} are balanced. You mean in the error message?\n\nOur style guides says we wouldn't want this pair of {} if it's single\nstatement.\n\n\n-- \nDanh\n"},{"id":"400243","messageId":"02dc01d64691$12db4310$3891c930$@nexbridge.com","threadId":"53719","inReplyTo":"20200619230141.GC5027@danh.dev","subject":"RE: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2020-06-19T23:26:29Z","receivedAt":"2020-06-19T23:26:42Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 19, 2020 7:02 PM, Ðoàn Tr?n Công Danh wrote:\n> To: Randall S. Becker \n> Cc: randall.s.becker@rogers.com; git@vger.kernel.org\n> Subject: Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with\n> write_in_full\n> \n> On 2020-06-19 13:17:19-0400, \"Randall S. Becker\"\n> wrote:\n> > On June 19, 2020 12:36 PM, Đoàn Trần Công Danh wrote:\n> > > On 2020-06-19 11:04:43-0400, randall.s.becker@rogers.com wrote:\n> > > > From: \"Randall S. Becker\" \n> > > >\n> > > > The strbuf_write_fd method did not provide checks for buffers\n> > > > larger than MAX_IO_SIZE. Replacing with write_in_full ensures the\n> > > > entire buffer will always be written to disk or report an error and die.\n> > > >\n> > > > Signed-off-by: Randall S. Becker \n> > > > ---\n> > > > bugreport.c | 5 ++++-\n> > > > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/bugreport.c b/bugreport.c index\n> > > > aa8a489c35..bc359b7fa8\n> > > > 100644\n> > > > --- a/bugreport.c\n> > > > +++ b/bugreport.c\n> > > > @@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)\n> > > > \t\tdie(_(\"couldn't create a new file at '%s'\"), report_path.buf);\n> > > > \t}\n> > > >\n> > > > -\tstrbuf_write_fd(&buffer, report);\n> > > > +\tif (write_in_full(report, buffer.buf, buffer.len) < 0) {\n> > > > +\t\tdie(_(\"couldn't write report contents '%s' to file '%s'\"),\n> > > > +\t\t\tbuffer.buf, report_path.buf);\n> > >\n> > > Doesn't this dump the whole report to the stderr?\n> > > If it's the case, the error would be very hard to grasp.\n> >\n> > Where else can we put the error? By this point, we're likely out of\n> > disk or virtual memory.\n> \n> Sorry, I forgot to suggest an alternatives.\n> \n> I was thinking about ignore the report when writing the last email.\n> \n> Since, the report is likely consists of multiple lines of text, and they likely\n> contains some single quote themselves.\n> \n> Now, I think a bit more, I think it's way better to write as:\n> \n> \tif (write_in_full(report, buffer.buf, buffer.len) < 0)\n> \t\tdie(_(\"couldn't write the report contents to file '%s'.\\n\\n\"\n> \t\t\"The original report was:\\n\\n\"\n> \t\t\"%s\\n\"), report_path.buf, buffer.buf);\n\nI went with Peff's suggestion of using die_error in v2. Thanks though.\n\n> > > Nit: We wouldn't want the pair of {}.\n> > >\n> > > > +\t}\n> > > > \tclose(report);\n> >\n> > I'm not sure what you mean in this nit? {} are balanced. You mean in the\n> error message?\n> \n> Our style guides says we wouldn't want this pair of {} if it's single statement.\n\nFixed in v2\n\nRegards,\nRandall\n\n"}]}