threads / patch / 53966

patchconnected: use buffered I/O to talk to rev-list

Subject: [PATCH] connected: use buffered I/O to talk to rev-list

## tl;dr

5 messages between Aug 2, 2020 and Aug 13, 2020. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

René Scharfe· Aug 2, 2020, 14:38 UTC · lore

Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects, 2016-06-08), significantly reduce the number of system calls and simplify the code for sending object IDs to rev-list by using stdio's buffering and handling errors after the loop.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 connected.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)
Show changes to connected.c +5 −12
diff --git a/connected.c b/connected.c
index 937b4bae387..05c2916f38d 100644
--- a/connected.c
+++ b/connected.c
@@ -22,14 +22,13 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		    struct check_connected_options *opt)
 {
 	struct child_process rev_list = CHILD_PROCESS_INIT;
+	FILE *rev_list_in;
 	struct check_connected_options defaults = CHECK_CONNECTED_INIT;
-	char commit[GIT_MAX_HEXSZ + 1];
 	struct object_id oid;
 	int err = 0;
 	struct packed_git *new_pack = NULL;
 	struct transport *transport;
 	size_t base_len;
-	const unsigned hexsz = the_hash_algo->hexsz;

 	if (!opt)
 		opt = &defaults;
@@ -122,7 +121,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,

 	sigchain_push(SIGPIPE, SIG_IGN);

-	commit[hexsz] = '\n';
+	rev_list_in = xfdopen(rev_list.in, "w");
+
 	do {
 		/*
 		 * If index-pack already checked that:
@@ -135,16 +135,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		if (new_pack && find_pack_entry_one(oid.hash, new_pack))
 			continue;

-		memcpy(commit, oid_to_hex(&oid), hexsz);
-		if (write_in_full(rev_list.in, commit, hexsz + 1) < 0) {
-			if (errno != EPIPE && errno != EINVAL)
-				error_errno(_("failed write to rev-list"));
-			err = -1;
-			break;
-		}
+		fprintf(rev_list_in, "%s\n", oid_to_hex(&oid));
 	} while (!fn(cb_data, &oid));

-	if (close(rev_list.in))
+	if (fclose(rev_list_in))
 		err = error_errno(_("failed to close rev-list's stdin"));

 	sigchain_pop(SIGPIPE);
--
2.28.0
Chris Torek· Aug 2, 2020, 16:08 UTC · re: René Scharfe · lore

Re: [PATCH] connected: use buffered I/O to talk to rev-list

On Sun, Aug 2, 2020 at 7:39 AM René Scharfe <l.s.r@web.de> wrote:
Show 21 quoted lines
> @@ -135,16 +135,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>                 if (new_pack && find_pack_entry_one(oid.hash, new_pack))
>                         continue;
>
> -               memcpy(commit, oid_to_hex(&oid), hexsz);
> -               if (write_in_full(rev_list.in, commit, hexsz + 1) < 0) {
> -                       if (errno != EPIPE && errno != EINVAL)
> -                               error_errno(_("failed write to rev-list"));
> -                       err = -1;
> -                       break;
> -               }
> +               fprintf(rev_list_in, "%s\n", oid_to_hex(&oid));
>         } while (!fn(cb_data, &oid));
>
> -       if (close(rev_list.in))
> +       if (fclose(rev_list_in))
>                 err = error_errno(_("failed to close rev-list's stdin"));
>
>         sigchain_pop(SIGPIPE);
> --
> 2.28.0

The same ferror()-before-fclose() remarks apply here too, but this time the explicit errno checking (EPIPE) cannot be done -- it's too late, errno is probably overwritten. I'm not sure how valuable the explicit errno tests are in the first place so I will leave that to others, but if we want to keep the explicit tests, use:

    if (fprintf(...) < 0)

to check each fprintf(), and add a final fflush() call (with another check) before the fclose().

Chris
Johannes Sixt· Aug 3, 2020, 18:17 UTC · re: Chris Torek · lore

Re: [PATCH] connected: use buffered I/O to talk to rev-list

Am 02.08.20 um 18:08 schrieb Chris Torek:
Show 24 quoted lines
> On Sun, Aug 2, 2020 at 7:39 AM René Scharfe <l.s.r@web.de> wrote:
>> @@ -135,16 +135,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>>                 if (new_pack && find_pack_entry_one(oid.hash, new_pack))
>>                         continue;
>>
>> -               memcpy(commit, oid_to_hex(&oid), hexsz);
>> -               if (write_in_full(rev_list.in, commit, hexsz + 1) < 0) {
>> -                       if (errno != EPIPE && errno != EINVAL)
>> -                               error_errno(_("failed write to rev-list"));
>> -                       err = -1;
>> -                       break;
>> -               }
>> +               fprintf(rev_list_in, "%s\n", oid_to_hex(&oid));
>>         } while (!fn(cb_data, &oid));
>>
>> -       if (close(rev_list.in))
>> +       if (fclose(rev_list_in))
>>                 err = error_errno(_("failed to close rev-list's stdin"));
>>
>>         sigchain_pop(SIGPIPE);
> 
> The same ferror()-before-fclose() remarks apply here too,
> but this time the explicit errno checking (EPIPE) cannot
> be done -- it's too late, errno is probably overwritten.

The EPIPE check should really remain in the loop above. (Same comment on EPIPE on Windows applies here.)

-- Hannes
René Scharfe· Aug 12, 2020, 16:52 UTC · re: René Scharfe · lore

[PATCH v2] connected: use buffered I/O to talk to rev-list

Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects, 2016-06-08), significantly reduce the number of system calls and simplify the code for sending object IDs to rev-list by using stdio's buffering.

Take care to handle errors immediately to get the correct error code, and to flush the buffer explicitly before closing the stream in order to catch any write errors for these last bytes.

Helped-by: Chris Torek <chris.torek@gmail.com>
Helped-by: Johannes Sixt <j6t@kdbg.org>
---
Changes since v1:
- Handle fprintf() errors immediately.
- Call ferror() and fflush() explicitly before calling fclose().
- Report write errors other than EPIPE and EINVAL, like the original
  code does.
 connected.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)
Show changes to connected.c +11 −11
diff --git a/connected.c b/connected.c
index 21c1ebe9fbf..b18299fdf0e 100644
--- a/connected.c
+++ b/connected.c
@@ -22,14 +22,13 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		    struct check_connected_options *opt)
 {
 	struct child_process rev_list = CHILD_PROCESS_INIT;
+	FILE *rev_list_in;
 	struct check_connected_options defaults = CHECK_CONNECTED_INIT;
-	char commit[GIT_MAX_HEXSZ + 1];
 	struct object_id oid;
 	int err = 0;
 	struct packed_git *new_pack = NULL;
 	struct transport *transport;
 	size_t base_len;
-	const unsigned hexsz = the_hash_algo->hexsz;

 	if (!opt)
 		opt = &defaults;
@@ -122,7 +121,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,

 	sigchain_push(SIGPIPE, SIG_IGN);

-	commit[hexsz] = '\n';
+	rev_list_in = xfdopen(rev_list.in, "w");
+
 	do {
 		/*
 		 * If index-pack already checked that:
@@ -135,16 +135,17 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		if (new_pack && find_pack_entry_one(oid.hash, new_pack))
 			continue;

-		memcpy(commit, oid_to_hex(&oid), hexsz);
-		if (write_in_full(rev_list.in, commit, hexsz + 1) < 0) {
-			if (errno != EPIPE && errno != EINVAL)
-				error_errno(_("failed write to rev-list"));
-			err = -1;
+		if (fprintf(rev_list_in, "%s\n", oid_to_hex(&oid)) < 0)
 			break;
-		}
 	} while (!fn(cb_data, &oid));

-	if (close(rev_list.in))
+	if (ferror(rev_list_in) || fflush(rev_list_in)) {
+		if (errno != EPIPE && errno != EINVAL)
+			error_errno(_("failed write to rev-list"));
+		err = -1;
+	}
+
+	if (fclose(rev_list_in))
 		err = error_errno(_("failed to close rev-list's stdin"));

 	sigchain_pop(SIGPIPE);
--
2.28.0
Jeff King· Aug 13, 2020, 09:16 UTC · re: René Scharfe · lore

Re: [PATCH v2] connected: use buffered I/O to talk to rev-list

On Wed, Aug 12, 2020 at 06:52:49PM +0200, René Scharfe wrote:
Show 8 quoted lines
> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,
> 2016-06-08), significantly reduce the number of system calls and
> simplify the code for sending object IDs to rev-list by using stdio's
> buffering.
> 
> Take care to handle errors immediately to get the correct error code,
> and to flush the buffer explicitly before closing the stream in order to
> catch any write errors for these last bytes.

FWIW, the error handling in this patch and the other stdio conversions you sent all look good to me. Thanks (and to Chris and Johannes for great review on v1).

-Peff

← back to recent threads