{"thread":{"id":"47211","subject":"[PATCH] config: avoid \"write_in_full(fd, buf, len) != len\" pattern","startedAt":"2017-11-15T12:41:19Z","lastAt":"2017-11-18T20:25:50Z","messageCount":7,"participants":["Phillip Wood","Junio C Hamano","Jeff King","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"332602","messageId":"20171115124043.17147-1-phillip.wood@talktalk.net","threadId":"47211","inReplyTo":null,"subject":"[PATCH] config: avoid \"write_in_full(fd, buf, len) != len\" pattern","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2017-11-15T12:40:43Z","receivedAt":"2017-11-15T12:41:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAs explained in commit 06f46f237 (avoid \"write_in_full(fd, buf, len)\n!= len\" pattern, 2017–09–13) the return value of write_in_full() is\neither -1 or the requested number of bytes. As such comparing the\nreturn value to an unsigned value such as strbuf.len will fail to\ncatch errors. Change the code to use the preferred '< 0' check.\n\nSigned-off-by: Phillip Wood \n---\n config.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 903abf9533b188fd472c213c29a9f968eb90eb8b..d377161113009f394f118d81d27fa6117cde8e9f 100644\n--- a/config.c\n+++ b/config.c\n@@ -2810,7 +2810,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n \t\t\t * multiple [branch \"$name\"] sections.\n \t\t\t */\n \t\t\tif (copystr.len > 0) {\n-\t\t\t\tif (write_in_full(out_fd, copystr.buf, copystr.len) != copystr.len) {\n+\t\t\t\tif (write_in_full(out_fd, copystr.buf, copystr.len) < 0) {\n \t\t\t\t\tret = write_error(get_lock_file_path(&lock));\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n@@ -2872,7 +2872,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n \t * logic in the loop above.\n \t */\n \tif (copystr.len > 0) {\n-\t\tif (write_in_full(out_fd, copystr.buf, copystr.len) != copystr.len) {\n+\t\tif (write_in_full(out_fd, copystr.buf, copystr.len) < 0) {\n \t\t\tret = write_error(get_lock_file_path(&lock));\n \t\t\tgoto out;\n \t\t}\n-- \n2.15.0\n\n"},{"id":"332651","messageId":"xmqqo9o2x04a.fsf@gitster.mtv.corp.google.com","threadId":"47211","inReplyTo":"20171115124043.17147-1-phillip.wood@talktalk.net","subject":"Re: [PATCH] config: avoid \"write_in_full(fd, buf, len) != len\" pattern","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-16T05:29:09Z","receivedAt":"2017-11-16T05:29:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> From: Phillip Wood \n>\n> As explained in commit 06f46f237 (avoid \"write_in_full(fd, buf, len)\n> != len\" pattern, 2017–09–13) the return value of write_in_full() is\n> either -1 or the requested number of bytes. As such comparing the\n> return value to an unsigned value such as strbuf.len will fail to\n> catch errors. Change the code to use the preferred '< 0' check.\n\nThanks, queued. This seems to have come from 9a5abfc7 (\"After\nrenaming a section, print any trailing variable definitions\",\n2009-07-24), which is rather ancient, but was made worse by getting\nduplicated by 52d59cc6 (\"branch: add a --copy (-c) option to go with\n--move (-m)\", 2017-06-18) recently.\n\n"},{"id":"332777","messageId":"20171117220633.6yoovfgpbr3rsykr@sigill.intra.peff.net","threadId":"47211","inReplyTo":"20171115124043.17147-1-phillip.wood@talktalk.net","subject":"Re: [PATCH] config: avoid \"write_in_full(fd, buf, len) != len\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-17T22:06:33Z","receivedAt":"2017-11-17T22:06:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 15, 2017 at 12:40:43PM +0000, Phillip Wood wrote:\n\n> From: Phillip Wood \n> \n> As explained in commit 06f46f237 (avoid \"write_in_full(fd, buf, len)\n> != len\" pattern, 2017–09–13) the return value of write_in_full() is\n> either -1 or the requested number of bytes. As such comparing the\n> return value to an unsigned value such as strbuf.len will fail to\n> catch errors. Change the code to use the preferred '< 0' check.\n\nThanks for catching this. I wondered at first how I missed these obvious\ncases, but the answer is that they were added after my commit. :)\n\nThere's one more case in write_section() that uses \"==\". That's not\nactually wrong, but I wonder if we'd want to make it \"< 0\" for\nconsistency.\n\n-Peff\n"},{"id":"332796","messageId":"f3038984-1cd8-e11f-61fd-10bf0cc33e2d@web.de","threadId":"47211","inReplyTo":"20171117220633.6yoovfgpbr3rsykr@sigill.intra.peff.net","subject":"Re: [PATCH] config: avoid \"write_in_full(fd, buf, len) != len\" pattern","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-11-18T10:20:04Z","receivedAt":"2017-11-18T10:20:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.11.2017 um 23:06 schrieb Jeff King:\n> There's one more case in write_section() that uses \"==\". That's not\n> actually wrong, but I wonder if we'd want to make it \"< 0\" for\n> consistency.\n\nActually it *is* wrong.\n\n-- >8 --\nSubject: [PATCH] config: flip return value of write_section()\n\nd9bd4cbb9cc (config: flip return value of store_write_*()) made\nwrite_section() follow the convention of write(2) to return -1 on error\nand the number of written bytes on success. 3b48045c6c7 (Merge branch\n'sd/branch-copy') changed it back to returning 0 on error and 1 on\nsuccess, but left its callers still checking for negative values.\n\nLet write_section() follow the convention of write(2) again to meet the\nexpectations of its callers.\n\nReported-by: Jeff King \nSigned-off-by: Rene Scharfe \n---\n config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 903abf9533..3f079c77ad 100644\n--- a/config.c\n+++ b/config.c\n@@ -2315,7 +2315,7 @@ static ssize_t write_section(int fd, const char *key)\n \tstruct strbuf sb = store_create_section(key);\n \tssize_t ret;\n \n-\tret = write_in_full(fd, sb.buf, sb.len) == sb.len;\n+\tret = write_in_full(fd, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n \n \treturn ret;\n-- \n2.15.0\n"},{"id":"332797","messageId":"6f1cf9ac-42e4-76b2-6936-4646a5cb574c@talktalk.net","threadId":"47211","inReplyTo":"20171117220633.6yoovfgpbr3rsykr@sigill.intra.peff.net","subject":"Re: [PATCH] config: avoid \"write_in_full(fd, buf, len) != len\" pattern","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2017-11-18T11:27:31Z","receivedAt":"2017-11-18T11:27:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 17/11/17 22:06, Jeff King wrote:\n> On Wed, Nov 15, 2017 at 12:40:43PM +0000, Phillip Wood wrote:\n> \n>> From: Phillip Wood \n>>\n>> As explained in commit 06f46f237 (avoid \"write_in_full(fd, buf, len)\n>> != len\" pattern, 2017–09–13) the return value of write_in_full() is\n>> either -1 or the requested number of bytes. As such comparing the\n>> return value to an unsigned value such as strbuf.len will fail to\n>> catch errors. Change the code to use the preferred '< 0' check.\n> \n> Thanks for catching this. I wondered at first how I missed these obvious\n> cases, but the answer is that they were added after my commit. :)\n> \n> There's one more case in write_section() that uses \"==\". That's not\n> actually wrong, but I wonder if we'd want to make it \"< 0\" for\n> consistency.\n\nYes, I noticed that but didn't get round to looking at it properly the\nother day. Rene's fix looks good to me.\n\nBest Wishes\n\nPhillip\n"},{"id":"332809","messageId":"20171118175246.puaepnyw3u4a33dy@sigill.intra.peff.net","threadId":"47211","inReplyTo":"f3038984-1cd8-e11f-61fd-10bf0cc33e2d@web.de","subject":"Re: [PATCH] config: avoid \"write_in_full(fd, buf, len) != len\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-18T17:52:46Z","receivedAt":"2017-11-18T17:52:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 18, 2017 at 11:20:04AM +0100, René Scharfe wrote:\n\n> Am 17.11.2017 um 23:06 schrieb Jeff King:\n> > There's one more case in write_section() that uses \"==\". That's not\n> > actually wrong, but I wonder if we'd want to make it \"< 0\" for\n> > consistency.\n> \n> Actually it *is* wrong.\n\nThanks for digging, I didn't look beyond that single line.\n\n> -- >8 --\n> Subject: [PATCH] config: flip return value of write_section()\n> \n> d9bd4cbb9cc (config: flip return value of store_write_*()) made\n> write_section() follow the convention of write(2) to return -1 on error\n> and the number of written bytes on success. 3b48045c6c7 (Merge branch\n> 'sd/branch-copy') changed it back to returning 0 on error and 1 on\n> success, but left its callers still checking for negative values.\n> \n> Let write_section() follow the convention of write(2) again to meet the\n> expectations of its callers.\n\nYikes. It looks like this slipped by on the tests because we always\ncheck \"< 0\" in the callers, not non-zero. So success would not look like\nfailure, but failure would look like success. And write failure does not\nhappen regularly in the test suite.\n\nSo this looks correct, and well-explained.\n\n> Reported-by: Jeff King \n> Signed-off-by: Rene Scharfe \n\nI'm not sure I deserve a reported-by if I say \"it looks fine\" but am\ntotally wrong. ;)\n\n-Peff\n"},{"id":"332821","messageId":"d93bd6b8-3f90-03da-97db-317524cace36@web.de","threadId":"47211","inReplyTo":"20171118175246.puaepnyw3u4a33dy@sigill.intra.peff.net","subject":"Re: [PATCH] config: avoid \"write_in_full(fd, buf, len) != len\" pattern","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-11-18T20:25:30Z","receivedAt":"2017-11-18T20:25:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.11.2017 um 18:52 schrieb Jeff King:\n> On Sat, Nov 18, 2017 at 11:20:04AM +0100, René Scharfe wrote:\n>> Reported-by: Jeff King \n>> Signed-off-by: Rene Scharfe \n> \n> I'm not sure I deserve a reported-by if I say \"it looks fine\" but am\n> totally wrong. ;)\n\nRight, wrong -- mere details. You pointed out that there was work\nto do. :)\n\nRené\n"}]}