debuginfod: usability tweaks, incl. $DEBUGINFOD_PROGRESS client support

This facility allows a default progress-printing function to be
installed if the given environment variable is set.  Some larger usage
experience (systemtap fetching kernels) indicates the default timeout
is too short, so forked it into a connection timeout (default short)
and a transfer timeout (default unlimited).

Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

8 files changed, 117 insertions, 40 deletions

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index bb7c5687..28a50719 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,13 @@
+2019-12-19  Frank Ch. Eigler  <fche@redhat.com>
+
+	* debuginfod-client.c (default_progressfn): New function.
+	(debuginfod_begin): Use it if $DEBUGINFOD_PROGRESS set.
+	(server_timeout): Bump to 30 seconds.
+	(debuginfod_query_server): Call progressfn -after- rather than
+	before curl ops, to make it likely that a successful transfer
+	results in final a=b call.  Tweak cleanup sequence.
+	* debuginfod.h: Document $DEBUGINFOD_PROGRESS name.
+
 2019-12-09  Mark Wielaard  <mark@klomp.org>
 
 	* debuginfod-client.c (debuginfod_query_server): Check
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index ab7b4e13..9a4a0e0f 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -40,6 +40,7 @@
 
 #include "config.h"
 #include "debuginfod.h"
+#include "system.h"
 #include <assert.h>
 #include <dirent.h>
 #include <stdio.h>
@@ -98,16 +99,16 @@ static const time_t cache_default_max_unused_age_s = 604800; /* 1 week */
 static const char *cache_default_name = ".debuginfod_client_cache";
 static const char *cache_path_envvar = DEBUGINFOD_CACHE_PATH_ENV_VAR;
 
-/* URLs of debuginfods, separated by url_delim.
-   This env var must be set for debuginfod-client to run.  */
+/* URLs of debuginfods, separated by url_delim. */
 static const char *server_urls_envvar = DEBUGINFOD_URLS_ENV_VAR;
 static const char *url_delim =  " ";
 static const char url_delim_char = ' ';
 
-/* Timeout for debuginfods, in seconds.
-   This env var must be set for debuginfod-client to run.  */
+/* Timeout for debuginfods, in seconds. */
 static const char *server_timeout_envvar = DEBUGINFOD_TIMEOUT_ENV_VAR;
-static int server_timeout = 5;
+static const long default_connect_timeout = 5;
+static const long default_transfer_timeout = -1; /* unlimited */
+
 
 /* Data associated with a particular CURL easy handle. Passed to
    the write callback.  */
@@ -400,8 +401,18 @@ debuginfod_query_server (debuginfod_client *c,
       return fd;
     }
 
-  if (getenv(server_timeout_envvar))
-    server_timeout = atoi (getenv(server_timeout_envvar));
+  long connect_timeout = default_connect_timeout;
+  long transfer_timeout = default_transfer_timeout;
+  const char* timeout_envvar = getenv(server_timeout_envvar);
+  if (timeout_envvar != NULL)
+    {
+      long ct, tt;
+      rc = sscanf(timeout_envvar, "%ld,%ld", &ct, &tt);
+      if (rc >= 1)
+        connect_timeout = ct;
+      if (rc >= 2)
+        transfer_timeout = tt;
+    }
 
   /* make a copy of the envvar so it can be safely modified.  */
   server_urls = strdup(urls_envvar);
@@ -493,7 +504,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);
@@ -511,11 +525,32 @@ debuginfod_query_server (debuginfod_client *c,
   long loops = 0;
   do
     {
+      /* Wait 1 second, the minimum DEBUGINFOD_TIMEOUT.  */
+      curl_multi_wait(curlm, NULL, 0, 1000, NULL);
+
+      /* If the target file has been found, abort the other queries.  */
+      if (target_handle != NULL)
+        for (int i = 0; i < num_urls; i++)
+          if (data[i].handle != target_handle)
+            curl_multi_remove_handle(curlm, data[i].handle);
+
+      CURLMcode curlm_res = curl_multi_perform(curlm, &still_running);
+      if (curlm_res != CURLM_OK)
+        {
+          switch (curlm_res)
+            {
+            case CURLM_CALL_MULTI_PERFORM: continue;
+            case CURLM_OUT_OF_MEMORY: rc = -ENOMEM; break;
+            default: rc = -ENETUNREACH; break;
+            }
+          goto out1;
+        }
+
       if (c->progressfn) /* inform/check progress callback */
         {
           loops ++;
           long pa = loops; /* default params for progress callback */
-          long pb = 0;
+          long pb = 0; /* transfer_timeout tempting, but loops != elapsed-time */
           if (target_handle) /* we've committed to a server; report its download progress */
             {
               CURLcode curl_res;
@@ -535,6 +570,8 @@ debuginfod_query_server (debuginfod_client *c,
                 pa = (dl > LONG_MAX ? LONG_MAX : (long)dl);
 #endif
 
+              /* NB: If going through deflate-compressing proxies, this
+                 number is likely to be unavailable, so -1 may show. */
 #ifdef CURLINFO_CURLINFO_CONTENT_LENGTH_DOWNLOAD_T
               curl_off_t cl;
               curl_res = curl_easy_getinfo(target_handle,
@@ -555,27 +592,6 @@ debuginfod_query_server (debuginfod_client *c,
           if ((*c->progressfn) (c, pa, pb))
             break;
         }
-
-      /* Wait 1 second, the minimum DEBUGINFOD_TIMEOUT.  */
-      curl_multi_wait(curlm, NULL, 0, 1000, NULL);
-
-      /* If the target file has been found, abort the other queries.  */
-      if (target_handle != NULL)
-        for (int i = 0; i < num_urls; i++)
-          if (data[i].handle != target_handle)
-            curl_multi_remove_handle(curlm, data[i].handle);
-
-      CURLMcode curlm_res = curl_multi_perform(curlm, &still_running);
-      if (curlm_res != CURLM_OK)
-        {
-          switch (curlm_res)
-            {
-            case CURLM_CALL_MULTI_PERFORM: continue;
-            case CURLM_OUT_OF_MEMORY: rc = -ENOMEM; break;
-            default: rc = -ENETUNREACH; break;
-            }
-          goto out1;
-        }
     } while (still_running);
 
   /* Check whether a query was successful. If so, assign its handle
@@ -674,9 +690,9 @@ debuginfod_query_server (debuginfod_client *c,
 
   curl_multi_cleanup(curlm);
   unlink (target_cache_tmppath);
+  close (fd); /* before the rmdir, otherwise it'll fail */
   (void) rmdir (target_cache_dir); /* nop if not empty */
   free(data);
-  close (fd);
 
  out0:
   free (server_urls);
@@ -685,6 +701,22 @@ debuginfod_query_server (debuginfod_client *c,
   return rc;
 }
 
+
+/* Activate a basic form of progress tracing */
+static int
+default_progressfn (debuginfod_client *c, long a, long b)
+{
+  (void) c;
+
+  dprintf(STDERR_FILENO,
+          "Downloading from debuginfod %ld/%ld%s", a, b,
+          ((a == b) ? "\n" : "\r"));
+  /* XXX: include URL - stateful */
+
+  return 0;
+}
+
+
 /* See debuginfod.h  */
 debuginfod_client  *
 debuginfod_begin (void)
@@ -693,7 +725,12 @@ debuginfod_begin (void)
   size_t size = sizeof (struct debuginfod_client);
   client = (debuginfod_client *) malloc (size);
   if (client != NULL)
-    client->progressfn = NULL;
+    {
+      if (getenv(DEBUGINFOD_PROGRESS_ENV_VAR))
+	client->progressfn = default_progressfn;
+      else
+	client->progressfn = NULL;
+    }
   return client;
 }
 
diff --git a/debuginfod/debuginfod.h b/debuginfod/debuginfod.h
index 6b1b1cc3..33fae863 100644
--- a/debuginfod/debuginfod.h
+++ b/debuginfod/debuginfod.h
@@ -33,6 +33,7 @@
 #define DEBUGINFOD_URLS_ENV_VAR "DEBUGINFOD_URLS"
 #define DEBUGINFOD_CACHE_PATH_ENV_VAR "DEBUGINFOD_CACHE_PATH"
 #define DEBUGINFOD_TIMEOUT_ENV_VAR "DEBUGINFOD_TIMEOUT"
+#define DEBUGINFOD_PROGRESS_ENV_VAR "DEBUGINFOD_PROGRESS"
 
 /* Handle for debuginfod-client connection.  */
 typedef struct debuginfod_client debuginfod_client;
diff --git a/doc/ChangeLog b/doc/ChangeLog
index 00a61ac3..95421617 100644
--- a/doc/ChangeLog
+++ b/doc/ChangeLog
@@ -1,3 +1,9 @@
+2019-12-04  Frank Ch. Eigler  <fche@redhat.com>
+
+	* debuginfod-find.1: Bump default timeout to 30.
+	* debuginfod_find_debuginfo.3: Ditto.
+	Document $DEBUGINFOD_PROGRESS.
+
 2019-09-02  Mark Wielaard  <mark@klomp.org>
 
 	* readelf.1 (symbols): Add optional section name.
diff --git a/doc/debuginfod-find.1 b/doc/debuginfod-find.1
index a759ecba..023acbb3 100644
--- a/doc/debuginfod-find.1
+++ b/doc/debuginfod-find.1
@@ -119,9 +119,13 @@ debuginfod instances.  Alternate URL prefixes are separated by space.
 
 .TP 21
 .B DEBUGINFOD_TIMEOUT
-This environment variable governs the timeout for each debuginfod HTTP
-connection.  A server that fails to respond within this many seconds
-is skipped.  The default is 5.
+This environment variable governs the timeouts for each debuginfod
+HTTP connection.  One or two comma-separated numbers may be given.
+The first is the number of seconds for the connection establishment
+(CURLOPT_CONNECTTIMEOUT), and the default is 5.  The second is the
+number of seconds for the transfer completion (CURLOPT_TIMEOUT), and
+the default is no timeout.  (Zero or negative also means "no
+timeout".)
 
 .TP 21
 .B DEBUGINFOD_CACHE_PATH
diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debuginfo.3
index be8eed09..ea8c6161 100644
--- a/doc/debuginfod_find_debuginfo.3
+++ b/doc/debuginfod_find_debuginfo.3
@@ -163,9 +163,21 @@ debuginfod instances.  Alternate URL prefixes are separated by space.
 
 .TP 21
 .B DEBUGINFOD_TIMEOUT
-This environment variable governs the timeout for each debuginfod HTTP
-connection.  A server that fails to respond within this many seconds
-is skipped.  The default is 5.
+This environment variable governs the timeouts for each debuginfod
+HTTP connection.  One or two comma-separated numbers may be given.
+The first is the number of seconds for the connection establishment
+(CURLOPT_CONNECTTIMEOUT), and the default is 5.  The second is the
+number of seconds for the transfer completion (CURLOPT_TIMEOUT), and
+the default is no timeout.  (Zero or negative also means "no
+timeout".)
+
+.TP 21
+.B DEBUGINFOD_PROGRESS
+This environment variable governs the default progress function.  If
+set, and if a progressfn is not explicitly set, then the library will
+configure a default progressfn.  This function will append a simple
+progress message periodically to the given file.  Consider using
+"/dev/stderr" on platforms that support it.  The default is nothing.
 
 .TP 21
 .B DEBUGINFOD_CACHE_PATH
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 30bd8256..7103cf51 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,7 @@
+2019-12-04  Frank Ch. Eigler  <fche@redhat.com>
+
+	* run-debuinfod-find.sh: Test $DEBUGINFOD_PROGRESS.
+
 2019-12-11  Omar Sandoval  <osandov@fb.com>
 
 	* dwfl-report-segment-coalesce.c: New test.
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 6533996a..4cf61389 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -89,7 +89,7 @@ wait_ready $PORT1 'ready' 1
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1/   # or without trailing /
 
 # Be patient when run on a busy machine things might take a bit.
-export DEBUGINFOD_TIMEOUT=10
+export DEBUGINFOD_TIMEOUT=1,10
 
 # We use -t0 and -g0 here to turn off time-based scanning & grooming.
 # For testing purposes, we just sic SIGUSR1 / SIGUSR2 at the process.
@@ -153,8 +153,11 @@ cmp $filename F/prog2
 cat vlog
 grep -q Progress vlog
 tempfiles vlog
-filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $BUILDID2`
+filename=`testrun env DEBUGINFOD_PROGRESS=1 ${abs_top_builddir}/debuginfod/debuginfod-find executable $BUILDID2 2>vlog2`
 cmp $filename F/prog2
+cat vlog2
+grep -q Downloading vlog2
+tempfiles vlog2
 filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID2 ${PWD}/prog2.c`
 cmp $filename ${PWD}/prog2.c