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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 4, 2010, 11:19 UTC
Message-ID: <20101004111904.GC4738@burratino>
In-Reply-To: <1286190258-12724-2-git-send-email-ilari.liusvaara@elisanet.fi>

Ilari Liusvaara wrote:

> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -862,3 +862,327 @@ int transport_helper_init(struct transport *transport, const char *name)

[...]

Show 12 quoted lines

> +/* 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.)

[...]

Show 14 quoted lines

> +/* 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.

Show 46 quoted lines

> +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!