Re: [RFC PATCH v6 1/3] Add bidirectional_transfer_loop()
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:49:45
Ilari Liusvaara [off-list ref] writes:
quoted hunk ↗ jump to hunk
This helper function copies bidirectional stream of data between stdin/stdout and specified file descriptors. Signed-off-by: Ilari Liusvaara <redacted> ---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(>p_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?