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

[Junio C Hamano <gitster@pobox.com>]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

> This helper function copies bidirectional stream of data between
> stdin/stdout and specified file descriptors.
>
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> ---
> diff --git a/transport-helper.c b/transport-helper.c
> index acfc88e..acbae47 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -862,3 +862,264 @@ int transport_helper_init(struct transport *transport, const char *name)
> + ...
> +static int udt_do_read(struct unidirectional_transfer *t)
> +{
> +	int r;
> +	transfer_debug("%s is readable", t->src_name);
> +	r = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> +	if (r < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
> +		errno != EINTR) {
> +		error("read(%s) failed: %s", t->src_name, strerror(errno));
> +		return -1;
> +	} else if (r == 0) {
> +		transfer_debug("%s EOF (with %i bytes in buffer)",
> +			t->src_name, t->bufuse);
> +		t->state = SSTATE_FLUSHING;
> +	} else if (r > 0) {
> +		t->bufuse += r;
> +		transfer_debug("Read %i bytes from %s (buffer now at %i)",
> +			r, t->src_name, (int)t->bufuse);
> +	}
> +	return 0;
> +}

This is probably a stupid question, but can t->bufuse be equal to BUFFERSIZE
when this function is entered?  What happens in such a case?

You would want to rename the counter "count" or "bytes" or somesuch if you
want to copy & paste it to udt_do_write() side, as "int r" that holds the
number of bytes _written_ feels somewhat odd ;-).

> +static int udt_do_write(struct unidirectional_transfer *t)
> +{
> +	int r;
> +	transfer_debug("%s is writable", t->dest_name);
> +	r = write(t->dest, t->buf, t->bufuse);
> +	if (r < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
> ...

> +#ifndef NO_PTHREADS
> +static int transloop_with_threads(struct bidirectional_transfer_state *s)
> +{
> +	pthread_t gtp_thread;
> +	pthread_t ptg_thread;
> +	int err;
> +	int ret = 0;
> +	void *dummy;
> +	err = pthread_create(&gtp_thread, NULL, udt_copy_thread_routine,
> +		&s->gtp);
> +	if (err)
> +		die("Can't start thread for copying data: %s", strerror(err));
> +	err = pthread_create(&ptg_thread, NULL, udt_copy_thread_routine,
> +		&s->ptg);
> +	if (err)
> +		die("Can't start thread for copying data: %s", strerror(err));
> +	err = pthread_join(gtp_thread, &dummy);
> +	if (!dummy) {

I suspect that originally you did not use "dummy" for anything, but if you
use the value to decide what to do, it now probably is something more than
a dummy, no?

Just out of curiosity, does the order of creation and joining of these
threads make a difference?

> +		error("Git to program copy thread failed");
> +		ret = 1;
> +	}
> ...
> +#else
> +
> +static void udt_kill_transfer(struct unidirectional_transfer *t)
> +{
> +	t->state = SSTATE_FINISHED;
> +	close(t->src);
> +	if (t->dest_is_sock)
> +		shutdown(t->dest, SHUT_WR);
> +	else
> +		close(t->dest);
> +}
> +
> +static int transloop_with_threads(struct bidirectional_transfer_state *s)
> +{

It feels funny to see in NO_PTHREADS codepath a something_with_threads
function that uses fork().  Perhaps bidirectional_transfer_loop() can call
a function with a more generic name, e.g. transloop(), that is implemented
in terms of either threads or processes depending on the platform
capability?