Re: [PATCH v4 1/3] Add bidirectional_transfer_loop()

Ilari Liusvaara wrote:

[...]

+/* Unidirectional transfer. */
+struct unidirectional_transfer
+{
+	int src;
+	int dest;
+	int dest_is_sock;
+	int state;
+	char buf[BUFFERSIZE];
+	size_t bufuse;
+	const char *src_name;
+	const char *dest_name;
+};

Nice. :)

(I would have written

	int src;
	int dest
	unsigned dest_is_sock:1;
	enum unidirectional_transfer_state state;
	char buf[BUFFERSIZE];
	...

to make the types more obvious, but I think that's more of a matter of
style.)

[...]

+/* State of bidirectional transfer loop. */
+struct bidirectional_transfer_state
+{
+	struct pollfd polls[4];
+	int polls_active;
+	int stdin_index;
+	int stdout_index;
+	int input_index;
+	int output_index;
+	/* Direction from program to git. */
+	struct unidirectional_transfer ptg;
+	/* Direction from git to program. */
+	struct unidirectional_transfer gtp;
+};

Sensible.

+int bidirectional_transfer_loop(int input, int output)
+{
+	struct bidirectional_transfer_state state;
+
+	/* Fill the state fields. */
+	state.ptg.src = input;
+	state.ptg.dest = 1;
+	state.ptg.dest_is_sock = 0;
+	state.ptg.state = SSTATE_TRANSFERING;
+	state.ptg.bufuse = 0;
+	state.ptg.src_name = "remote input";
+	state.ptg.dest_name = "stdout";
+
+	state.gtp.src = 0;
+	state.gtp.dest = output;
+	state.gtp.dest_is_sock = (input == output);
+	state.gtp.state = SSTATE_TRANSFERING;
+	state.gtp.bufuse = 0;
+	state.gtp.src_name = "stdin";
+	state.gtp.dest_name = "remote output";
+
+	while (1) {
+		int r;
+		load_poll_params(&state);
+		if (!state.polls_active) {
+			transfer_debug("Transfer done");
+			break;
+		}
+		transfer_debug("Waiting for %i file descriptors",
+			state.polls_active);
+		r = poll(state.polls, state.polls_active, -1);
+		if (r < 0) {
+			if (errno == EWOULDBLOCK || errno == EAGAIN ||
+				errno == EINTR)
+				continue;
+			error("poll failed: %s", strerror(errno));
+			return -1;
+		} else if (r == 0)
+			continue;
+
+		r = transfer_handle_events(&state);
+		if (r)
+			return r;
+	}
+	return 0;
+}

Yes, that's much clearer.

Linux's scripts/checkpatch.pl would have a few things to say, but
I have no complaint myself except the density of comments (which is a
matter of taste).

Thanks!