Re: rfc/patch: debuginfod client $DEBUGINFOD_PROGRESS env var

[Mark Wielaard <mark@klomp.org>]

Hi Frank,

On Fri, 2019-12-13 at 11:57 -0500, Frank Ch. Eigler wrote:
> Pushed the following additional patch to fche/debuginfod-progress-env.
> Hand-tested on a ginormous download with various length timeouts; hard
> to test reliably within the elfutils testsuite (esp after removal of
> that induced debuginfod-side wait).

The code looks good in general, do note that if you rebase/squash to
onto master there is a slight conflict with the curl_res/curlm_res
fixlet. Since GCC10 isn't out yet, just yell if this gives you trouble
and I do/test it for you.

> +static const long default_connect_timeout = 5;
> +static const long default_transfer_timeout = -1; /* unlimited */
> +

I would call it default_progress_timeout, because...

> @@ -492,7 +502,10 @@ debuginfod_query_server (debuginfod_client *c,
>                         CURLOPT_WRITEFUNCTION,
>                         debuginfod_write_callback);
>        curl_easy_setopt(data[i].handle, CURLOPT_WRITEDATA, (void*)&data[i]);
> -      curl_easy_setopt(data[i].handle, CURLOPT_TIMEOUT, (long) server_timeout);
> +      if (connect_timeout >= 0)
> +        curl_easy_setopt(data[i].handle, CURLOPT_CONNECTTIMEOUT, connect_timeout);
> +      if (transfer_timeout >= 0)
> +        curl_easy_setopt(data[i].handle, CURLOPT_TIMEOUT, transfer_timeout);
>        curl_easy_setopt(data[i].handle, CURLOPT_FILETIME, (long) 1);
>        curl_easy_setopt(data[i].handle, CURLOPT_FOLLOWLOCATION, (long) 1);
>        curl_easy_setopt(data[i].handle, CURLOPT_FAILONERROR, (long) 1);

I would add something like:

  /* Make sure there is at least some progress,
     try to get at least 1K per progress timeout seconds.  */
  curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, 5 * 1024L);
  curl_easy_setopt(curl, CURLOPT_LOW_SPEED_LIMIT, progress_timeout);

The idea being that if we didn't at least get 1K per 5 seconds then the
connection is just so bad that it doesn't make sense to wait for it to
finish, since that will most likely be forever (or feel like it for the
user).

This might or might not be a separate timeout from the connect timeout,
but I think they are kind of the same thing.

Cheers,

Mark