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

From: Stefan Beller <sbeller@google.com>
Date: Jul 15, 2016, 17:24 UTC
Message-ID: <CAGZ79kbernZHx9SUG-_vbxy-g77_3ki1uc-21LCviwrH=aXp6w@mail.gmail.com>
In-Reply-To: <20160715104347.GL19271@sigill.intra.peff.net>

On Fri, Jul 15, 2016 at 3:43 AM, Jeff King <peff@peff.net> wrote:

>
> Signed-off-by: Jeff King <peff@peff.net>

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

Show 24 quoted lines

> +       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);
     }

Show 18 quoted lines

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