On Fri, Jul 15, 2016 at 3:43 AM, Jeff King <peff@xxxxxxxx> wrote:
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
Read-entirely-by Stefan ;)
Thanks!
> @@ -319,10 +331,60 @@ static void rp_error(const char *err, ...)
> static int copy_to_sideband(int in, int out, void *arg)
> {
> char data[128];
While looking at this code, do you think it is feasible to increase the
size of data[] to 1024 ? (The largest that is possible when
side-band, but no side-band-64k is given).
> + int keepalive_active = 0;
> +
> + if (keepalive_in_sec <= 0)
> + use_keepalive = KEEPALIVE_NEVER;
> + if (use_keepalive == KEEPALIVE_ALWAYS)
> + keepalive_active = 1;
> +
> while (1) {
> - ssize_t sz = xread(in, data, sizeof(data));
> + ssize_t sz;
> +
> + if (keepalive_active) {
> + struct pollfd pfd;
> + int ret;
> +
> + pfd.fd = in;
> + pfd.events = POLLIN;
> + ret = poll(&pfd, 1, 1000 * keepalive_in_sec);
> +
> + if (ret < 0) {
> + if (errno == EINTR)
> + continue;
> + else
> + break;
The method was short and concise, this adds a lot of lines.
Remembering d751dd11 (2016-07-10, hoist out handle_nonblock
function for xread and xwrite), do you think it would be reasonable to
put the whole poll handling into a dedicated function, maybe even reuse the
that function?
if (keepalive_active) {
if (wrapper_around_poll(&data_in) < 0) // handles EINTR internally
break;
if (!data_in)
send_keep_alive();
}
I am not sure if that makes this function more legible, just food for thought.
> + } else if (ret == 0) {
> + /* no data; send a keepalive packet */
> + static const char buf[] = "0005\1";
and the \1 is the first sideband. Why do we choose that sideband?
> + write_or_die(1, buf, sizeof(buf) - 1);
> + continue;
> + } /* else there is actual data to read */
"If there is data to read, we need to break the while(1), to actually
read the data?"
I got confused and needed to go back and read the actual code again,
would it make sense to rather have a loop here?
while (1) {
while(keepalive_active) {
if (wrapper_around_poll(&data_in) < 0) // handles EINTR internally
break;
if (!data_in)
send_keep_alive();
else
break;
}
sz = xread(in, data, sizeof(data));
if (sz <= 0)
break;
turn_on_keepalive_on_NUL(&data);
}
> + }
> +
> + sz = xread(in, data, sizeof(data));
> if (sz <= 0)
> break;
> +
> + if (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {
> + const char *p = memchr(data, '\0', sz);
> + if (p) {
> + /*
> + * The NUL tells us to start sending keepalives. Make
> + * sure we send any other data we read along
> + * with it.
> + */
> + keepalive_active = 1;
> + send_sideband(1, 2, data, p - data, use_sideband);
> + send_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);
> + continue;
Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.
I wonder if we can use a better read function, that would stop reading at a NUL,
and return early instead?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html