{"thread":{"id":"53966","subject":"[PATCH] connected: use buffered I/O to talk to rev-list","startedAt":"2020-08-02T14:38:41Z","lastAt":"2020-08-13T09:16:46Z","messageCount":5,"participants":["René Scharfe","Chris Torek","Johannes Sixt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"402649","messageId":"2e2907ac-3be9-c0ed-830a-f8aa28b471aa@web.de","threadId":"53966","inReplyTo":null,"subject":"[PATCH] connected: use buffered I/O to talk to rev-list","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-02T14:38:34Z","receivedAt":"2020-08-02T14:38:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,\n2016-06-08), significantly reduce the number of system calls and\nsimplify the code for sending object IDs to rev-list by using stdio's\nbuffering and handling errors after the loop.\n\nSigned-off-by: René Scharfe \n---\n connected.c | 16 +++++-----------\n 1 file changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex 937b4bae387..05c2916f38d 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -22,14 +22,13 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t struct check_connected_options *opt)\n {\n \tstruct child_process rev_list = CHILD_PROCESS_INIT;\n+\tFILE *rev_list_in;\n \tstruct check_connected_options defaults = CHECK_CONNECTED_INIT;\n-\tchar commit[GIT_MAX_HEXSZ + 1];\n \tstruct object_id oid;\n \tint err = 0;\n \tstruct packed_git *new_pack = NULL;\n \tstruct transport *transport;\n \tsize_t base_len;\n-\tconst unsigned hexsz = the_hash_algo->hexsz;\n\n \tif (!opt)\n \t\topt = &defaults;\n@@ -122,7 +121,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n\n \tsigchain_push(SIGPIPE, SIG_IGN);\n\n-\tcommit[hexsz] = '\\n';\n+\trev_list_in = xfdopen(rev_list.in, \"w\");\n+\n \tdo {\n \t\t/*\n \t\t * If index-pack already checked that:\n@@ -135,16 +135,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\tif (new_pack && find_pack_entry_one(oid.hash, new_pack))\n \t\t\tcontinue;\n\n-\t\tmemcpy(commit, oid_to_hex(&oid), hexsz);\n-\t\tif (write_in_full(rev_list.in, commit, hexsz + 1) < 0) {\n-\t\t\tif (errno != EPIPE && errno != EINVAL)\n-\t\t\t\terror_errno(_(\"failed write to rev-list\"));\n-\t\t\terr = -1;\n-\t\t\tbreak;\n-\t\t}\n+\t\tfprintf(rev_list_in, \"%s\\n\", oid_to_hex(&oid));\n \t} while (!fn(cb_data, &oid));\n\n-\tif (close(rev_list.in))\n+\tif (fclose(rev_list_in))\n \t\terr = error_errno(_(\"failed to close rev-list's stdin\"));\n\n \tsigchain_pop(SIGPIPE);\n--\n2.28.0\n"},{"id":"402658","messageId":"CAPx1Gvesmwf_CBj6D=YriFQgtEsDyUe7PHa6p9AvxG=fwAvHpA@mail.gmail.com","threadId":"53966","inReplyTo":"2e2907ac-3be9-c0ed-830a-f8aa28b471aa@web.de","subject":"Re: [PATCH] connected: use buffered I/O to talk to rev-list","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-08-02T16:08:08Z","receivedAt":"2020-08-02T16:11:19Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Sun, Aug 2, 2020 at 7:39 AM René Scharfe wrote:\n> @@ -135,16 +135,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n> if (new_pack && find_pack_entry_one(oid.hash, new_pack))\n> continue;\n>\n> - memcpy(commit, oid_to_hex(&oid), hexsz);\n> - if (write_in_full(rev_list.in, commit, hexsz + 1) < 0) {\n> - if (errno != EPIPE && errno != EINVAL)\n> - error_errno(_(\"failed write to rev-list\"));\n> - err = -1;\n> - break;\n> - }\n> + fprintf(rev_list_in, \"%s\\n\", oid_to_hex(&oid));\n> } while (!fn(cb_data, &oid));\n>\n> - if (close(rev_list.in))\n> + if (fclose(rev_list_in))\n> err = error_errno(_(\"failed to close rev-list's stdin\"));\n>\n> sigchain_pop(SIGPIPE);\n> --\n> 2.28.0\n\nThe same ferror()-before-fclose() remarks apply here too,\nbut this time the explicit errno checking (EPIPE) cannot\nbe done -- it's too late, errno is probably overwritten. I'm\nnot sure how valuable the explicit errno tests are in the first\nplace so I will leave that to others, but if we want to keep\nthe explicit tests, use:\n\n if (fprintf(...) < 0)\n\nto check each fprintf(), and add a final fflush() call (with\nanother check) before the fclose().\n\nChris\n"},{"id":"402716","messageId":"5de5acdf-382b-6907-4a64-02ae02abaf13@kdbg.org","threadId":"53966","inReplyTo":"CAPx1Gvesmwf_CBj6D=YriFQgtEsDyUe7PHa6p9AvxG=fwAvHpA@mail.gmail.com","subject":"Re: [PATCH] connected: use buffered I/O to talk to rev-list","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-08-03T18:17:34Z","receivedAt":"2020-08-03T18:17:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.08.20 um 18:08 schrieb Chris Torek:\n> On Sun, Aug 2, 2020 at 7:39 AM René Scharfe wrote:\n>> @@ -135,16 +135,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>> if (new_pack && find_pack_entry_one(oid.hash, new_pack))\n>> continue;\n>>\n>> - memcpy(commit, oid_to_hex(&oid), hexsz);\n>> - if (write_in_full(rev_list.in, commit, hexsz + 1) < 0) {\n>> - if (errno != EPIPE && errno != EINVAL)\n>> - error_errno(_(\"failed write to rev-list\"));\n>> - err = -1;\n>> - break;\n>> - }\n>> + fprintf(rev_list_in, \"%s\\n\", oid_to_hex(&oid));\n>> } while (!fn(cb_data, &oid));\n>>\n>> - if (close(rev_list.in))\n>> + if (fclose(rev_list_in))\n>> err = error_errno(_(\"failed to close rev-list's stdin\"));\n>>\n>> sigchain_pop(SIGPIPE);\n> \n> The same ferror()-before-fclose() remarks apply here too,\n> but this time the explicit errno checking (EPIPE) cannot\n> be done -- it's too late, errno is probably overwritten.\n\nThe EPIPE check should really remain in the loop above. (Same comment on\nEPIPE on Windows applies here.)\n\n-- Hannes\n"},{"id":"403471","messageId":"2e3ac6f7-77c2-4792-d6df-891ede6318ce@web.de","threadId":"53966","inReplyTo":"2e2907ac-3be9-c0ed-830a-f8aa28b471aa@web.de","subject":"[PATCH v2] connected: use buffered I/O to talk to rev-list","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-12T16:52:49Z","receivedAt":"2020-08-12T16:52:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,\n2016-06-08), significantly reduce the number of system calls and\nsimplify the code for sending object IDs to rev-list by using stdio's\nbuffering.\n\nTake care to handle errors immediately to get the correct error code,\nand to flush the buffer explicitly before closing the stream in order to\ncatch any write errors for these last bytes.\n\nHelped-by: Chris Torek \nHelped-by: Johannes Sixt \n---\nChanges since v1:\n- Handle fprintf() errors immediately.\n- Call ferror() and fflush() explicitly before calling fclose().\n- Report write errors other than EPIPE and EINVAL, like the original\n code does.\n\n connected.c | 21 +++++++++++----------\n 1 file changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex 21c1ebe9fbf..b18299fdf0e 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -22,14 +22,13 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t struct check_connected_options *opt)\n {\n \tstruct child_process rev_list = CHILD_PROCESS_INIT;\n+\tFILE *rev_list_in;\n \tstruct check_connected_options defaults = CHECK_CONNECTED_INIT;\n-\tchar commit[GIT_MAX_HEXSZ + 1];\n \tstruct object_id oid;\n \tint err = 0;\n \tstruct packed_git *new_pack = NULL;\n \tstruct transport *transport;\n \tsize_t base_len;\n-\tconst unsigned hexsz = the_hash_algo->hexsz;\n\n \tif (!opt)\n \t\topt = &defaults;\n@@ -122,7 +121,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n\n \tsigchain_push(SIGPIPE, SIG_IGN);\n\n-\tcommit[hexsz] = '\\n';\n+\trev_list_in = xfdopen(rev_list.in, \"w\");\n+\n \tdo {\n \t\t/*\n \t\t * If index-pack already checked that:\n@@ -135,16 +135,17 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\tif (new_pack && find_pack_entry_one(oid.hash, new_pack))\n \t\t\tcontinue;\n\n-\t\tmemcpy(commit, oid_to_hex(&oid), hexsz);\n-\t\tif (write_in_full(rev_list.in, commit, hexsz + 1) < 0) {\n-\t\t\tif (errno != EPIPE && errno != EINVAL)\n-\t\t\t\terror_errno(_(\"failed write to rev-list\"));\n-\t\t\terr = -1;\n+\t\tif (fprintf(rev_list_in, \"%s\\n\", oid_to_hex(&oid)) < 0)\n \t\t\tbreak;\n-\t\t}\n \t} while (!fn(cb_data, &oid));\n\n-\tif (close(rev_list.in))\n+\tif (ferror(rev_list_in) || fflush(rev_list_in)) {\n+\t\tif (errno != EPIPE && errno != EINVAL)\n+\t\t\terror_errno(_(\"failed write to rev-list\"));\n+\t\terr = -1;\n+\t}\n+\n+\tif (fclose(rev_list_in))\n \t\terr = error_errno(_(\"failed to close rev-list's stdin\"));\n\n \tsigchain_pop(SIGPIPE);\n--\n2.28.0\n"},{"id":"403536","messageId":"20200813091643.GE3092220@coredump.intra.peff.net","threadId":"53966","inReplyTo":"2e3ac6f7-77c2-4792-d6df-891ede6318ce@web.de","subject":"Re: [PATCH v2] connected: use buffered I/O to talk to rev-list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-13T09:16:43Z","receivedAt":"2020-08-13T09:16:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 12, 2020 at 06:52:49PM +0200, René Scharfe wrote:\n\n> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,\n> 2016-06-08), significantly reduce the number of system calls and\n> simplify the code for sending object IDs to rev-list by using stdio's\n> buffering.\n> \n> Take care to handle errors immediately to get the correct error code,\n> and to flush the buffer explicitly before closing the stream in order to\n> catch any write errors for these last bytes.\n\nFWIW, the error handling in this patch and the other stdio conversions\nyou sent all look good to me. Thanks (and to Chris and Johannes for\ngreat review on v1).\n\n-Peff\n"}]}