From 76ad56c430f0b85c47115688c511d9bd4fa671d4 Mon Sep 17 00:00:00 2001 From: "Frank Ch. Eigler" Date: Wed, 4 Dec 2019 15:51:12 -0500 Subject: 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 --- debuginfod/ChangeLog | 10 ++++ debuginfod/debuginfod-client.c | 101 +++++++++++++++++++++++++++------------- debuginfod/debuginfod.h | 1 + doc/ChangeLog | 6 +++ doc/debuginfod-find.1 | 10 ++-- doc/debuginfod_find_debuginfo.3 | 18 +++++-- tests/ChangeLog | 4 ++ tests/run-debuginfod-find.sh | 7 ++- 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 + + * 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 * 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 #include #include @@ -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 + + * debuginfod-find.1: Bump default timeout to 30. + * debuginfod_find_debuginfo.3: Ditto. + Document $DEBUGINFOD_PROGRESS. + 2019-09-02 Mark Wielaard * 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 + + * run-debuinfod-find.sh: Test $DEBUGINFOD_PROGRESS. + 2019-12-11 Omar Sandoval * 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 -- cgit v1.2.3