Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods

On Fri, Jul 15, 2016 at 3:43 AM, Jeff King [off-list ref] wrote:

Signed-off-by: Jeff King <redacted>

Read-entirely-by Stefan ;)
Thanks!

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?