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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 12, 2010, 03:57 UTC
Message-ID: <7v1v7woyg6.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1286571713-7755-2-git-send-email-ilari.liusvaara@elisanet.fi>

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

Show 31 quoted lines

> 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 ;-).

Show 7 quoted lines

> +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 &&
> ...

Show 18 quoted lines

> +#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?

Show 18 quoted lines

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