* PR25369 rfc: debuginfod client api extension for progressfn prettying etc.
@ 2020-02-25 3:35 Frank Ch. Eigler
2020-02-25 3:50 ` Simon Marchi
2020-02-25 15:25 ` Mark Wielaard
0 siblings, 2 replies; 9+ messages in thread
From: Frank Ch. Eigler @ 2020-02-25 3:35 UTC (permalink / raw)
To: elfutils-devel; +Cc: simark
Hi -
As a part of PR25369, I propose this small set of client api
extensions, requested by gdb developers and needed by personal
experience. (I plan to roll it out on my debuginfod servers shortly
to let it soak.) An end-user visible immediate difference is in the
DEBUGINFOD_PROGRESS=1 format message, which now looks like this:
Downloading from debuginfod /
Downloading from debuginfod -
Downloading from debuginfod \
Downloading from http://very:8222/buildid/817dcbd2ce0ecce19f22cbe5e136b6d9196428aa/executable 433130328/433130328
This latter is a bit long and should probably be abbreviated, wdyt?
I'd start reviewing this from the debuginfod.h header file outward.
commit 4ace515d6b9ce92ea4a808eba4d608851ee9b56d (fche/pr25369)
Author: Frank Ch. Eigler <fche@redhat.com>
Date: Mon Feb 24 22:23:55 2020 -0500
PR25369: client api extensions, progress prettying
This batch of changes extends the client api with a group of optional
functions that allow:
- debuginfod to pass along User-Agent and appropriate XFF to
others, important for tracking in federated mode
- client applications to query URLs, get/set a void* parameter
- let the DEBUGINFOD_PROGRESS=1 progress handler show pretty URLs
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 186aa90a53bd..8d321aead2de 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -74,11 +74,22 @@
#include <fts.h>
#endif
+
struct debuginfod_client
{
/* Progress/interrupt callback function. */
debuginfod_progressfn_t progressfn;
+ /* Stores user data. */
+ void* user_data;
+
+ /* Accumulates outgoing http header names/values. */
+ int user_agent_set_p; /* affects add_default_headers */
+ struct curl_slist *headers;
+
+ /* The 'winner' CURL handle for downloading, if any. */
+ CURL *target_handle;
+
/* Can contain all other context, like cache_path, server_urls,
timeout or other info gotten from environment variables, the
handle data, etc. So those don't have to be reparsed and
@@ -291,8 +302,11 @@ debuginfod_clean_cache(debuginfod_client *c,
static void
-add_extra_headers(CURL *handle)
+add_default_headers(debuginfod_client *client)
{
+ if (client->user_agent_set_p)
+ return;
+
/* Compute a User-Agent: string to send. The more accurately this
describes this host, the likelier that the debuginfod servers
might be able to locate debuginfo for us. */
@@ -352,7 +366,7 @@ add_extra_headers(CURL *handle)
}
char *ua = NULL;
- rc = asprintf(& ua, "%s/%s,%s,%s/%s",
+ rc = asprintf(& ua, "User-Agent: %s/%s,%s,%s/%s",
PACKAGE_NAME, PACKAGE_VERSION,
utspart ?: "",
id ?: "",
@@ -361,7 +375,7 @@ add_extra_headers(CURL *handle)
ua = NULL;
if (ua)
- curl_easy_setopt(handle, CURLOPT_USERAGENT, (void*) ua); /* implicit strdup */
+ (void) debuginfod_add_http_header (client, ua);
free (ua);
free (id);
@@ -532,9 +546,6 @@ debuginfod_query_server (debuginfod_client *c,
&& (i == 0 || server_urls[i - 1] == url_delim_char))
num_urls++;
- /* Tracks which handle should write to fd. Set to the first
- handle that is ready to write the target file to the cache. */
- CURL *target_handle = NULL;
struct handle_data *data = malloc(sizeof(struct handle_data) * num_urls);
/* Initalize handle_data with default values. */
@@ -556,10 +567,11 @@ debuginfod_query_server (debuginfod_client *c,
char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
/* Initialize each handle. */
- for (int i = 0; i < num_urls && server_url != NULL; i++)
+ c->target_handle = NULL; /* reset for this lookup */
+ for (int i = 0; i < num_urls && server_url != NULL; i++)
{
data[i].fd = fd;
- data[i].target_handle = &target_handle;
+ data[i].target_handle = &c->target_handle;
data[i].handle = curl_easy_init();
if (data[i].handle == NULL)
@@ -603,7 +615,7 @@ debuginfod_query_server (debuginfod_client *c,
curl_easy_setopt(data[i].handle, CURLOPT_NOSIGNAL, (long) 1);
curl_easy_setopt(data[i].handle, CURLOPT_AUTOREFERER, (long) 1);
curl_easy_setopt(data[i].handle, CURLOPT_ACCEPT_ENCODING, "");
- add_extra_headers(data[i].handle);
+ curl_easy_setopt(data[i].handle, CURLOPT_HTTPHEADER, c->headers);
curl_multi_add_handle(curlm, data[i].handle);
server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
@@ -618,9 +630,9 @@ debuginfod_query_server (debuginfod_client *c,
curl_multi_wait(curlm, NULL, 0, 1000, NULL);
/* If the target file has been found, abort the other queries. */
- if (target_handle != NULL)
+ if (c->target_handle != NULL)
for (int i = 0; i < num_urls; i++)
- if (data[i].handle != target_handle)
+ if (data[i].handle != c->target_handle)
curl_multi_remove_handle(curlm, data[i].handle);
CURLMcode curlm_res = curl_multi_perform(curlm, &still_running);
@@ -640,19 +652,19 @@ debuginfod_query_server (debuginfod_client *c,
loops ++;
long pa = loops; /* default params for progress callback */
long pb = 0; /* transfer_timeout tempting, but loops != elapsed-time */
- if (target_handle) /* we've committed to a server; report its download progress */
+ if (c->target_handle) /* we've committed to a server; report its download progress */
{
CURLcode curl_res;
#ifdef CURLINFO_SIZE_DOWNLOAD_T
curl_off_t dl;
- curl_res = curl_easy_getinfo(target_handle,
+ curl_res = curl_easy_getinfo(c->target_handle,
CURLINFO_SIZE_DOWNLOAD_T,
&dl);
if (curl_res == 0 && dl >= 0)
pa = (dl > LONG_MAX ? LONG_MAX : (long)dl);
#else
double dl;
- curl_res = curl_easy_getinfo(target_handle,
+ curl_res = curl_easy_getinfo(c->target_handle,
CURLINFO_SIZE_DOWNLOAD,
&dl);
if (curl_res == 0)
@@ -663,14 +675,14 @@ debuginfod_query_server (debuginfod_client *c,
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,
+ curl_res = curl_easy_getinfo(c->target_handle,
CURLINFO_CONTENT_LENGTH_DOWNLOAD_T,
&cl);
if (curl_res == 0 && cl >= 0)
pb = (cl > LONG_MAX ? LONG_MAX : (long)cl);
#else
double cl;
- curl_res = curl_easy_getinfo(target_handle,
+ curl_res = curl_easy_getinfo(c->target_handle,
CURLINFO_CONTENT_LENGTH_DOWNLOAD,
&cl);
if (curl_res == 0)
@@ -720,7 +732,7 @@ debuginfod_query_server (debuginfod_client *c,
long resp_code = 500;
CURLcode curl_res;
- curl_res = curl_easy_getinfo(target_handle,
+ curl_res = curl_easy_getinfo(c->target_handle,
CURLINFO_RESPONSE_CODE,
&resp_code);
@@ -759,6 +771,7 @@ debuginfod_query_server (debuginfod_client *c,
}
/* Success!!!! */
+ c->target_handle = NULL;
for (int i = 0; i < num_urls; i++)
curl_easy_cleanup(data[i].handle);
@@ -774,9 +787,9 @@ debuginfod_query_server (debuginfod_client *c,
/* error exits */
out1:
+ c->target_handle = NULL;
for (int i = 0; i < num_urls; i++)
curl_easy_cleanup(data[i].handle);
-
curl_multi_cleanup(curlm);
unlink (target_cache_tmppath);
close (fd); /* before the rmdir, otherwise it'll fail */
@@ -795,11 +808,16 @@ debuginfod_query_server (debuginfod_client *c,
static int
default_progressfn (debuginfod_client *c, long a, long b)
{
- (void) c;
+ const char* url = debuginfod_get_url (c) ?: "debuginfod";
- dprintf(STDERR_FILENO,
- "Downloading from debuginfod %ld/%ld%s", a, b,
- ((a == b) ? "\n" : "\r"));
+ if (b == 0)
+ dprintf(STDERR_FILENO,
+ "Downloading from %s %c\r", url, "-/|\\"[a % 4]);
+ else
+ dprintf(STDERR_FILENO,
+ "Downloading from %s %ld/%ld%s",
+ url, a, b,
+ ((a == b) ? "\n" : "\r"));
/* XXX: include URL - stateful */
return 0;
@@ -812,13 +830,11 @@ debuginfod_begin (void)
{
debuginfod_client *client;
size_t size = sizeof (struct debuginfod_client);
- client = (debuginfod_client *) malloc (size);
+ client = (debuginfod_client *) calloc (1, size); /* clear */
if (client != NULL)
{
if (getenv(DEBUGINFOD_PROGRESS_ENV_VAR))
client->progressfn = default_progressfn;
- else
- client->progressfn = NULL;
}
return client;
}
@@ -826,6 +842,10 @@ debuginfod_begin (void)
void
debuginfod_end (debuginfod_client *client)
{
+ if (client) /* make it safe to be invoked with NULL */
+ {
+ curl_slist_free_all (client->headers);
+ }
free (client);
}
@@ -834,6 +854,7 @@ debuginfod_find_debuginfo (debuginfod_client *client,
const unsigned char *build_id, int build_id_len,
char **path)
{
+ add_default_headers(client);
return debuginfod_query_server(client, build_id, build_id_len,
"debuginfo", NULL, path);
}
@@ -845,6 +866,7 @@ debuginfod_find_executable(debuginfod_client *client,
const unsigned char *build_id, int build_id_len,
char **path)
{
+ add_default_headers(client);
return debuginfod_query_server(client, build_id, build_id_len,
"executable", NULL, path);
}
@@ -854,6 +876,7 @@ int debuginfod_find_source(debuginfod_client *client,
const unsigned char *build_id, int build_id_len,
const char *filename, char **path)
{
+ add_default_headers(client);
return debuginfod_query_server(client, build_id, build_id_len,
"source", filename, path);
}
@@ -867,6 +890,50 @@ debuginfod_set_progressfn(debuginfod_client *client,
}
+void
+debuginfod_set_user_data(debuginfod_client *client,
+ void *data)
+{
+ client->user_data = data;
+}
+
+
+void *
+debuginfod_get_user_data(debuginfod_client *client)
+{
+ return client->user_data;
+}
+
+
+const char *
+debuginfod_get_url(debuginfod_client *client)
+{
+ const char *url = NULL;
+ if (client->target_handle)
+ (void) curl_easy_getinfo (client->target_handle, CURLINFO_EFFECTIVE_URL, &url);
+
+ return url;
+}
+
+
+
+/* Add an outgoing HTTP header. */
+int debuginfod_add_http_header (debuginfod_client *client, const char* header)
+{
+ struct curl_slist *temp = curl_slist_append (client->headers, header);
+ if (temp == NULL)
+ return -ENOMEM;
+
+ /* Track if User-Agent: is being set. If so, signal not to add the
+ default one. */
+ if (strncmp (header, "User-Agent:", 11) == 0)
+ client->user_agent_set_p = 1;
+
+ client->headers = temp;
+ return 0;
+}
+
+
/* NB: these are thread-unsafe. */
__attribute__((constructor)) attribute_hidden void libdebuginfod_ctor(void)
{
diff --git a/debuginfod/debuginfod-find.c b/debuginfod/debuginfod-find.c
index 8bd3a3dba7a0..714fd402298d 100644
--- a/debuginfod/debuginfod-find.c
+++ b/debuginfod/debuginfod-find.c
@@ -1,6 +1,6 @@
/* Command-line frontend for retrieving ELF / DWARF / source files
from the debuginfod.
- Copyright (C) 2019 Red Hat, Inc.
+ Copyright (C) 2019-2020 Red Hat, Inc.
This file is part of elfutils.
This file is free software; you can redistribute it and/or modify
@@ -52,10 +52,12 @@ static const struct argp_option options[] =
/* debuginfod connection handle. */
static debuginfod_client *client;
-int progressfn(debuginfod_client *c __attribute__((__unused__)),
+int progressfn(debuginfod_client *c,
long a, long b)
{
- fprintf (stderr, "Progress %ld / %ld\n", a, b);
+ const char *s = (const char *) debuginfod_get_user_data (c);
+ const char* url = debuginfod_get_url (c) ?: "?";
+ fprintf (stderr, "%s %s %ld / %ld\n", s, url, a, b);
return 0;
}
@@ -91,6 +93,9 @@ main(int argc, char** argv)
return 1;
}
+ /* exercise user data pointer */
+ debuginfod_set_user_data (client, (void *)"Progress");
+
int remaining;
(void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER|ARGP_NO_ARGS, &remaining, NULL);
@@ -130,6 +135,8 @@ main(int argc, char** argv)
return 1;
}
+ debuginfod_end (client);
+
if (rc < 0)
{
fprintf(stderr, "Server query failed: %s\n", strerror(-rc));
@@ -137,9 +144,7 @@ main(int argc, char** argv)
}
printf("%s\n", cache_name);
-
free (cache_name);
- debuginfod_end (client);
return 0;
}
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 0acd70e4a916..837538d0934d 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -807,6 +807,7 @@ header_censor(const string& str)
}
+
static string
conninfo (struct MHD_Connection * conn)
{
@@ -1286,7 +1287,8 @@ debuginfod_find_progress (debuginfod_client *, long a, long b)
}
-static struct MHD_Response* handle_buildid (const string& buildid /* unsafe */,
+static struct MHD_Response* handle_buildid (struct MHD_Connection* conn,
+ const string& buildid /* unsafe */,
const string& artifacttype /* unsafe */,
const string& suffix /* unsafe */,
int *result_fd
@@ -1374,6 +1376,35 @@ static struct MHD_Response* handle_buildid (const string& buildid /* unsafe */,
{
debuginfod_set_progressfn (client, & debuginfod_find_progress);
+ if (conn)
+ {
+ // Transcribe incoming User-Agent:
+ string ua = MHD_lookup_connection_value (conn, MHD_HEADER_KIND, "User-Agent") ?: "";
+ string ua_complete = string("User-Agent: ") + ua;
+ debuginfod_add_http_header (client, ua_complete.c_str());
+
+ // Compute larger XFF:, for avoiding info loss during
+ // federation, and for future cyclicity detection.
+ string xff = MHD_lookup_connection_value (conn, MHD_HEADER_KIND, "X-Forwarded-For") ?: "";
+ if (xff != "")
+ xff += string(", "); // comma separated list
+
+ // Compute the client's numeric IP address only - so can't merge with conninfo()
+ const union MHD_ConnectionInfo *u = MHD_get_connection_info (conn,
+ MHD_CONNECTION_INFO_CLIENT_ADDRESS);
+ struct sockaddr *so = u ? u->client_addr : 0;
+ char hostname[256] = ""; // RFC1035
+ if (so && so->sa_family == AF_INET)
+ (void) getnameinfo (so, sizeof (struct sockaddr_in), hostname, sizeof (hostname), NULL, 0,
+ NI_NUMERICHOST);
+ else if (so && so->sa_family == AF_INET6)
+ (void) getnameinfo (so, sizeof (struct sockaddr_in6), hostname, sizeof (hostname), NULL, 0,
+ NI_NUMERICHOST);
+
+ string xff_complete = string("X-Forwarded-For: ")+xff+string(hostname);
+ debuginfod_add_http_header (client, xff_complete.c_str());
+ }
+
if (artifacttype == "debuginfo")
fd = debuginfod_find_debuginfo (client,
(const unsigned char*) buildid.c_str(),
@@ -1571,7 +1602,8 @@ handler_cb (void * /*cls*/,
}
inc_metric("http_requests_total", "type", artifacttype);
- r = handle_buildid(buildid, artifacttype, suffix, 0); // NB: don't care about result-fd
+ r = handle_buildid(connection, buildid,
+ artifacttype, suffix, 0); // NB: don't care about result-fd
}
else if (url1 == "/metrics")
{
@@ -1640,7 +1672,7 @@ dwarf_extract_source_paths (Elf *elf, set<string>& debug_sourcefiles)
struct MHD_Response *r = 0;
try
{
- r = handle_buildid (buildid, "debuginfo", "", &alt_fd);
+ r = handle_buildid (0, buildid, "debuginfo", "", &alt_fd);
}
catch (const reportable_exception& e)
{
diff --git a/debuginfod/debuginfod.h b/debuginfod/debuginfod.h
index b4b6a3d2a6b9..53aeee7133ca 100644
--- a/debuginfod/debuginfod.h
+++ b/debuginfod/debuginfod.h
@@ -1,5 +1,5 @@
/* External declarations for the libdebuginfod client library.
- Copyright (C) 2019 Red Hat, Inc.
+ Copyright (C) 2019-2020 Red Hat, Inc.
This file is part of elfutils.
This file is free software; you can redistribute it and/or modify
@@ -71,10 +71,23 @@ int debuginfod_find_source (debuginfod_client *client,
const char *filename,
char **path);
+/* Set a progress callback function. */
typedef int (*debuginfod_progressfn_t)(debuginfod_client *c, long a, long b);
void debuginfod_set_progressfn(debuginfod_client *c,
debuginfod_progressfn_t fn);
+/* Add an outgoing HTTP request "Header: Value". Copies string. */
+int debuginfod_add_http_header (debuginfod_client *client, const char* header);
+
+/* Return currently active URL, if known. String owned by curl, do not free. */
+const char* debuginfod_get_url (debuginfod_client *client);
+
+/* Set the user parameter. */
+void debuginfod_set_user_data (debuginfod_client *client, void *value);
+
+/* Get the user parameter. */
+void* debuginfod_get_user_data (debuginfod_client *client);
+
/* Release debuginfod client connection context handle. */
void debuginfod_end (debuginfod_client *client);
diff --git a/debuginfod/libdebuginfod.map b/debuginfod/libdebuginfod.map
index 0d26f93e04fe..de3d5a6480cc 100644
--- a/debuginfod/libdebuginfod.map
+++ b/debuginfod/libdebuginfod.map
@@ -8,3 +8,9 @@ ELFUTILS_0.178 {
debuginfod_find_source;
debuginfod_set_progressfn;
} ELFUTILS_0;
+ELFUTILS_0.179 {
+ debuginfod_add_http_header;
+ debuginfod_get_url;
+ debuginfod_set_user_data;
+ debuginfod_get_user_data;
+} ELFUTILS_0.178;
\ No newline at end of file
diff --git a/doc/Makefile.am b/doc/Makefile.am
index b5db01ff75d5..50b81ae1f5d3 100644
--- a/doc/Makefile.am
+++ b/doc/Makefile.am
@@ -1,7 +1,7 @@
## Process this file with automake to create Makefile.in
## Configure input file for elfutils.
##
-## Copyright (C) 1996-2001, 2002, 2005, 2019 Red Hat, Inc.
+## Copyright (C) 1996-2001, 2002, 2005, 2019-2020 Red Hat, Inc.
## This file is part of elfutils.
##
## This file is free software; you can redistribute it and/or modify
@@ -24,7 +24,16 @@ notrans_dist_man1_MANS=
if DEBUGINFOD
notrans_dist_man8_MANS += debuginfod.8
-notrans_dist_man3_MANS += debuginfod_find_debuginfo.3 debuginfod_find_source.3 debuginfod_find_executable.3 debuginfod_set_progressfn.3
+notrans_dist_man3_MANS += debuginfod_add_http_header.3
+notrans_dist_man3_MANS += debuginfod_begin.3
+notrans_dist_man3_MANS += debuginfod_end.3
+notrans_dist_man3_MANS += debuginfod_find_debuginfo.3
+notrans_dist_man3_MANS += debuginfod_find_executable.3
+notrans_dist_man3_MANS += debuginfod_find_source.3
+notrans_dist_man3_MANS += debuginfod_get_url.3
+notrans_dist_man3_MANS += debuginfod_get_user_data.3
+notrans_dist_man3_MANS += debuginfod_set_progressfn.3
+notrans_dist_man3_MANS += debuginfod_set_user_data.3
notrans_dist_man1_MANS += debuginfod-find.1
endif
diff --git a/doc/debuginfod_add_http_header.3 b/doc/debuginfod_add_http_header.3
new file mode 100644
index 000000000000..16279936e2ea
--- /dev/null
+++ b/doc/debuginfod_add_http_header.3
@@ -0,0 +1 @@
+.so man3/debuginfod_find_debuginfo.3
diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debuginfo.3
index c232ac717009..0bbf613777a1 100644
--- a/doc/debuginfod_find_debuginfo.3
+++ b/doc/debuginfod_find_debuginfo.3
@@ -21,9 +21,17 @@ debuginfod_find_debuginfo \- request debuginfo from debuginfod
.nf
.B #include <elfutils/debuginfod.h>
.PP
+
+Link with \fB-ldebuginfod\fP.
+
+CONNECTION HANDLE
+
.BI "debuginfod_client *debuginfod_begin(void);"
+
.BI "void debuginfod_end(debuginfod_client *" client ");"
+LOOKUP FUNCTIONS
+
.BI "int debuginfod_find_debuginfo(debuginfod_client *" client ","
.BI " const unsigned char *" build_id ","
.BI " int " build_id_len ","
@@ -38,36 +46,46 @@ debuginfod_find_debuginfo \- request debuginfo from debuginfod
.BI " const char *" filename ","
.BI " char ** " path ");"
+OPTIONAL FUNCTIONS
+
.BI "typedef int (*debuginfod_progressfn_t)(debuginfod_client *" client ","
.BI " long a, long b);"
.BI "void debuginfod_set_progressfn(debuginfod_client *" client ","
.BI " debuginfod_progressfn_t " progressfn ");"
-Link with \fB-ldebuginfod\fP.
+.BI "int debuginfod_add_http_header(debuginfod_client *" client ","
+.BI " const char* " header ");"
+
+.BI "const char* debuginfod_get_url(debuginfod_client *" client ");"
+
+.BI "void debuginfod_get_user_data(debuginfod_client *" client ","
+.BI " void *" data ");"
+
+.BI "void* debuginfod_get_user_data(debuginfod_client *" client ");"
.SH DESCRIPTION
-.BR debuginfod_begin ()
+.BR \%debuginfod_begin ()
creates a \fBdebuginfod_client\fP connection handle that should be used
with all other calls.
-.BR debuginfod_end ()
+.BR \%debuginfod_end ()
should be called on the \fBclient\fP handle to release all state and
storage when done.
-.BR debuginfod_find_debuginfo (),
-.BR debuginfod_find_executable (),
+.BR \%debuginfod_find_debuginfo (),
+.BR \%debuginfod_find_executable (),
and
-.BR debuginfod_find_source ()
+.BR \%debuginfod_find_source ()
query the debuginfod server URLs contained in
-.BR $DEBUGINFOD_URLS
+.BR \%$DEBUGINFOD_URLS
(see below) for the debuginfo, executable or source file with the
given \fIbuild_id\fP. \fIbuild_id\fP should be a pointer to either
-a null-terminated, lowercase hex string or a binary blob. If
+a null-terminated lowercase hex string, or else a binary blob. If
\fIbuild_id\fP is given as a hex string, \fIbuild_id_len\fP should
be 0. Otherwise \fIbuild_id_len\fP should be the number of bytes in
the binary blob.
-.BR debuginfod_find_source ()
+.BR \%debuginfod_find_source ()
also requries a \fIfilename\fP in order to specify a particular
source file. \fIfilename\fP should be an absolute path that includes
the compilation directory of the CU associated with the source file.
@@ -102,12 +120,17 @@ to the client cache and a file descriptor to that file is returned.
The caller needs to \fBclose\fP() this descriptor. Otherwise, a
negative error code is returned.
-.SH "PROGRESS CALLBACK"
+.SH "OPTIONAL FUNCTIONS"
+
+A small number of optional functions are available to tune or query
+the operation of the debuginfod client.
+
+.SS "PROGRESS CALLBACK"
As the \fBdebuginfod_find_*\fP() functions may block for seconds or
longer, a progress callback function is called periodically, if
configured with
-.BR debuginfod_set_progressfn ().
+.BR \%debuginfod_set_progressfn ().
This function sets a new progress callback function (or NULL) for the
client handle.
@@ -125,8 +148,43 @@ continue the work, or any other value to stop work as soon as
possible. Consequently, the \fBdebuginfod_find_*\fP() function will
likely return with an error, but might still succeed.
+Do not call another lookup function from within a progress callback.
+Do not call \fBdebuginfod_end\fP either.
+
+.SS "USER DATA POINTER"
+
+A single \fIvoid *\fP pointer associated with the connection handle
+may be set any time via
+.BR \%debuginfod_set_user_data () ,
+and retrieved via
+.BR \%debuginfod_get_user_data () .
+The value may be NULL.
+
+.SS "URL"
+
+The current URL of the outgoing download
+may be retrieved via
+.BR \%debuginfod_get_url ()
+from a progress callback function. The resulting string, if not
+NULL, is owned by the library, must not be modified or freed.
+
+.SS "HTTP HEADER"
+
+Before a lookup function is initiated, a client application may
+request adding HTTP request headers to future downloads.
+.BR \%debuginfod_add_http_header ()
+may be called with strings of the form
+.BR \%"Header:\~value" .
+These strings are copied by the library. A zero return value
+indicates success, but out-of-memory conditions may result in
+a non-zero \fI-ENOMEM\fP.
+
+By default, the library adds a descriptive \fIUser-Agent:\fP
+header to outgoing requests. If the client application adds
+a header with the same name, this default is suppressed.
.SH "CACHE"
+
If the query is successful, the \fBdebuginfod_find_*\fP() functions save
the target file to a local cache. The location of the cache is controlled
by the \fB$DEBUGINFOD_CACHE_PATH\fP environment variable (see below).
@@ -140,10 +198,10 @@ ASCII decimal integer representing the interval or max unused age in seconds.
The default is one day and one week, respectively. Values of zero mean "immediately".
.SH "SECURITY"
-.BR debuginfod_find_debuginfo (),
-.BR debuginfod_find_executable (),
+.BR \%debuginfod_find_debuginfo (),
+.BR \%debuginfod_find_executable (),
and
-.BR debuginfod_find_source ()
+.BR \%debuginfod_find_source ()
\fBdo not\fP include any particular security
features. They trust that the binaries returned by the debuginfod(s)
are accurate. Therefore, the list of servers should include only
diff --git a/doc/debuginfod_get_url.3 b/doc/debuginfod_get_url.3
new file mode 100644
index 000000000000..16279936e2ea
--- /dev/null
+++ b/doc/debuginfod_get_url.3
@@ -0,0 +1 @@
+.so man3/debuginfod_find_debuginfo.3
diff --git a/doc/debuginfod_get_user_data.3 b/doc/debuginfod_get_user_data.3
new file mode 100644
index 000000000000..16279936e2ea
--- /dev/null
+++ b/doc/debuginfod_get_user_data.3
@@ -0,0 +1 @@
+.so man3/debuginfod_find_debuginfo.3
diff --git a/doc/debuginfod_set_user_data.3 b/doc/debuginfod_set_user_data.3
new file mode 100644
index 000000000000..16279936e2ea
--- /dev/null
+++ b/doc/debuginfod_set_user_data.3
@@ -0,0 +1 @@
+.so man3/debuginfod_find_debuginfo.3
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 2964e7c077e5..28b33f91f434 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -23,11 +23,13 @@ type rpm2cpio 2>/dev/null || (echo "need rpm2cpio"; exit 77)
type bzcat 2>/dev/null || (echo "need bzcat"; exit 77)
# for test case debugging, uncomment:
-# set -x
-# VERBOSE=-vvvv
+set -x
+VERBOSE=-vvvv
DB=${PWD}/.debuginfod_tmp.sqlite
tempfiles $DB
+LOGFILE1=${PWD}/.debuginfod1.log
+tempfiles $LOGFILE1
export DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache
PID1=0
@@ -93,7 +95,7 @@ wait_ready()
fi
}
-env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS= ${abs_builddir}/../debuginfod/debuginfod $VERBOSE -F -R -d $DB -p $PORT1 -t0 -g0 --fdcache-fds 1 --fdcache-mbs 2 -Z .tar.xz -Z .tar.bz2=bzcat R F Z L &
+env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS= ${abs_builddir}/../debuginfod/debuginfod $VERBOSE -F -R -d $DB -p $PORT1 -t0 -g0 --fdcache-fds 1 --fdcache-mbs 2 -Z .tar.xz -Z .tar.bz2=bzcat R F Z L > $LOGFILE1 2>&1 &
PID1=$!
# Server must become ready
wait_ready $PORT1 'ready' 1
@@ -167,7 +169,7 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find -v debuginfo $BUILDID2 2>vlog`
cmp $filename F/prog2
cat vlog
-grep -q Progress vlog
+grep -q Progress.http vlog
tempfiles vlog
filename=`testrun env DEBUGINFOD_PROGRESS=1 ${abs_top_builddir}/debuginfod/debuginfod-find executable $BUILDID2 2>vlog2`
cmp $filename F/prog2
@@ -321,7 +323,14 @@ fi
rm -rf $DEBUGINFOD_CACHE_PATH
testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
-# confirm that first server can't resolve symlinked info in L/ but second can
+# send a request to stress XFF and User-Agent federation relay;
+# we'll grep for the two patterns in $TESTLOG1
+curl -s -H 'User-Agent: TESTCURL' -H 'X-Forwarded-For: TESTXFF' $DEBUGINFOD_URLS/buildid/deaddeadbeef00000000/debuginfo -o /dev/null || true
+
+grep -q UA:TESTCURL $LOGFILE1
+grep -q XFF:TESTXFF $LOGFILE1
+
+# confirm that first server cannot resolve symlinked info in L/ but second can
BUILDID=`env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../src/readelf \
-a L/foo | grep 'Build ID' | cut -d ' ' -f 7`
file L/foo
@@ -357,6 +366,7 @@ curl -s http://127.0.0.1:$PORT2/metrics | grep -q 'http_responses_total.*result.
kill -INT $PID1 $PID2
wait $PID1 $PID2
+cat $LOGFILE1 # so it's not lost
PID1=0
PID2=0
tempfiles .debuginfod_*
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc.
2020-02-25 3:35 PR25369 rfc: debuginfod client api extension for progressfn prettying etc Frank Ch. Eigler
@ 2020-02-25 3:50 ` Simon Marchi
2020-02-25 15:25 ` Mark Wielaard
1 sibling, 0 replies; 9+ messages in thread
From: Simon Marchi @ 2020-02-25 3:50 UTC (permalink / raw)
To: Frank Ch. Eigler, elfutils-devel
On 2020-02-24 10:35 p.m., Frank Ch. Eigler wrote:
> Hi -
>
> As a part of PR25369, I propose this small set of client api
> extensions, requested by gdb developers and needed by personal
> experience. (I plan to roll it out on my debuginfod servers shortly
> to let it soak.) An end-user visible immediate difference is in the
> DEBUGINFOD_PROGRESS=1 format message, which now looks like this:
>
> Downloading from debuginfod /
> Downloading from debuginfod -
> Downloading from debuginfod \
> Downloading from http://very:8222/buildid/817dcbd2ce0ecce19f22cbe5e136b6d9196428aa/executable 433130328/433130328
>
> This latter is a bit long and should probably be abbreviated, wdyt?
>
> I'd start reviewing this from the debuginfod.h header file outward.
>
>
> commit 4ace515d6b9ce92ea4a808eba4d608851ee9b56d (fche/pr25369)
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date: Mon Feb 24 22:23:55 2020 -0500
>
> PR25369: client api extensions, progress prettying
>
> This batch of changes extends the client api with a group of optional
> functions that allow:
> - debuginfod to pass along User-Agent and appropriate XFF to
> others, important for tracking in federated mode
> - client applications to query URLs, get/set a void* parameter
> - let the DEBUGINFOD_PROGRESS=1 progress handler show pretty URLs
Hi Frank,
As far as I am concerned (with my GDB hat), this looks good.
The "URL" section of the man page could maybe talk about the lifetime of the
returned string, specifically say that it's only guaranteed to be valid during
the execution of the callback. If you need it for longer, then you need to copy
it.
Thanks!
Simon
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc.
2020-02-25 3:35 PR25369 rfc: debuginfod client api extension for progressfn prettying etc Frank Ch. Eigler
2020-02-25 3:50 ` Simon Marchi
@ 2020-02-25 15:25 ` Mark Wielaard
2020-02-25 15:32 ` Frank Ch. Eigler
1 sibling, 1 reply; 9+ messages in thread
From: Mark Wielaard @ 2020-02-25 15:25 UTC (permalink / raw)
To: Frank Ch. Eigler, elfutils-devel; +Cc: simark
Hi Frank,
On Mon, 2020-02-24 at 22:35 -0500, Frank Ch. Eigler wrote:
> As a part of PR25369, I propose this small set of client api
> extensions, requested by gdb developers and needed by personal
> experience. (I plan to roll it out on my debuginfod servers shortly
> to let it soak.) An end-user visible immediate difference is in the
> DEBUGINFOD_PROGRESS=1 format message, which now looks like this:
>
> Downloading from debuginfod /
> Downloading from debuginfod -
> Downloading from debuginfod \
> Downloading from http://very:8222/buildid/817dcbd2ce0ecce19f22cbe5e136b6d9196428aa/executable 433130328/433130328
>
> This latter is a bit long and should probably be abbreviated, wdyt?
Might want to abbrev the build-id part to /81..aa/. The interesting
part is which server is used imho.
> I'd start reviewing this from the debuginfod.h header file outward.
OK. Lets start there:
> diff --git a/debuginfod/debuginfod.h b/debuginfod/debuginfod.h
> index b4b6a3d2a6b9..53aeee7133ca 100644
> --- a/debuginfod/debuginfod.h
> +++ b/debuginfod/debuginfod.h
> @@ -1,5 +1,5 @@
> /* External declarations for the libdebuginfod client library.
> - Copyright (C) 2019 Red Hat, Inc.
> + Copyright (C) 2019-2020 Red Hat, Inc.
> This file is part of elfutils.
>
> This file is free software; you can redistribute it and/or modify
> @@ -71,10 +71,23 @@ int debuginfod_find_source (debuginfod_client *client,
> const char *filename,
> char **path);
>
> +/* Set a progress callback function. */
> typedef int (*debuginfod_progressfn_t)(debuginfod_client *c, long a, long b);
> void debuginfod_set_progressfn(debuginfod_client *c,
> debuginfod_progressfn_t fn);
>
> +/* Add an outgoing HTTP request "Header: Value". Copies string. */
> +int debuginfod_add_http_header (debuginfod_client *client, const char* header);
This one seems different from the others and has a specific use case
just for the debuginfod server. Are you sure it is generic enough to be
added as a new public interface? If we add this can we do it separately
from other debuginfo-client progress improvements?
> +/* Return currently active URL, if known. String owned by curl, do not free. */
> +const char* debuginfod_get_url (debuginfod_client *client);
This does seem useful with the comment that was already made, that
lifetime of the returned string should be documented. I assume it is
valid to call this after debuginfod_find_* has returned, but before
debuginfod_end has been called?
> +/* Set the user parameter. */
> +void debuginfod_set_user_data (debuginfod_client *client, void *value);
> +
> +/* Get the user parameter. */
> +void* debuginfod_get_user_data (debuginfod_client *client);
In theory I like these additions. But I don't really see the point of
how they are used. Is the only use case to pass the string "Progress"?
If there are no real users for this then I think we should not add
these at this time. Or is some other client using them? I am not really
against it, but would prefer if we add them separately to keep the
patches concise.
Thanks,
Mark
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc.
2020-02-25 15:25 ` Mark Wielaard
@ 2020-02-25 15:32 ` Frank Ch. Eigler
2020-02-25 17:07 ` Mark Wielaard
0 siblings, 1 reply; 9+ messages in thread
From: Frank Ch. Eigler @ 2020-02-25 15:32 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel, simark
Hi -
> > This latter is a bit long and should probably be abbreviated, wdyt?
>
> Might want to abbrev the build-id part to /81..aa/. The interesting
> part is which server is used imho.
Yeah, OK.
> > +/* Add an outgoing HTTP request "Header: Value". Copies string. */
> > +int debuginfod_add_http_header (debuginfod_client *client, const char* header);
>
> This one seems different from the others and has a specific use case
> just for the debuginfod server. Are you sure it is generic enough to be
> added as a new public interface? If we add this can we do it separately
> from other debuginfo-client progress improvements?
I think it has a chance to be useful to other clients too, for example
for other proxy / authentication schemes. And given that there is a
shared library boundary, private APIs aren't in easy reach.
"separately" as in separate commits? ... I suppose, if it really
matters.
> > +/* Return currently active URL, if known. String owned by curl, do not free. */
> > +const char* debuginfod_get_url (debuginfod_client *client);
>
> This does seem useful with the comment that was already made, that
> lifetime of the returned string should be documented. I assume it is
> valid to call this after debuginfod_find_* has returned, but before
> debuginfod_end has been called?
Clarified this in a followup patch. No, only valid during the progress
callback function itself.
> > +/* Set the user parameter. */
> > +void debuginfod_set_user_data (debuginfod_client *client, void *value);
> > +
> > +/* Get the user parameter. */
> > +void* debuginfod_get_user_data (debuginfod_client *client);
>
> In theory I like these additions. But I don't really see the point of
> how they are used. Is the only use case to pass the string "Progress"?
That is for test coverage.
> If there are no real users for this then I think we should not add
> these at this time. Or is some other client using them? I am not really
> against it, but would prefer if we add them separately to keep the
> patches concise.
GDB would use them pretty much immediately, to be able to prepare a
more informative progress notification (like the file name whose
debuginfo is being sought, instead of its buildid).
- FChE
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc.
2020-02-25 15:32 ` Frank Ch. Eigler
@ 2020-02-25 17:07 ` Mark Wielaard
2020-02-25 18:51 ` Frank Ch. Eigler
0 siblings, 1 reply; 9+ messages in thread
From: Mark Wielaard @ 2020-02-25 17:07 UTC (permalink / raw)
To: Frank Ch. Eigler; +Cc: elfutils-devel, simark
Hi Frank,
On Tue, 2020-02-25 at 10:32 -0500, Frank Ch. Eigler wrote:
> > > +/* Add an outgoing HTTP request "Header: Value". Copies string. */
> > > +int debuginfod_add_http_header (debuginfod_client *client, const char* header);
> >
> > This one seems different from the others and has a specific use case
> > just for the debuginfod server. Are you sure it is generic enough to be
> > added as a new public interface? If we add this can we do it separately
> > from other debuginfo-client progress improvements?
>
> I think it has a chance to be useful to other clients too, for example
> for other proxy / authentication schemes. And given that there is a
> shared library boundary, private APIs aren't in easy reach.
> "separately" as in separate commits? ... I suppose, if it really
> matters.
Yes, please as a separate commit. It makes it so much easier on the
reviewers if the patches are smaller.
The reason I am slightly hesitant is because this feels like a feature
with corner cases that might not be clear. What about other headers
than Agent string if they have been set/will be set for example.
Could you expand a little on the use case? I see you set an X-
Forwarded-For header, but that seems it can be easily forged. I see it
might be interesting for testing, but would you use it in production?
> > > +/* Return currently active URL, if known. String owned by curl, do not free. */
> > > +const char* debuginfod_get_url (debuginfod_client *client);
> >
> > This does seem useful with the comment that was already made, that
> > lifetime of the returned string should be documented. I assume it is
> > valid to call this after debuginfod_find_* has returned, but before
> > debuginfod_end has been called?
>
> Clarified this in a followup patch. No, only valid during the progress
> callback function itself.
That is a pity, it seems useful without having to add a progress
function. Then we should probably make sure it doesn't return a value
in that case, because I suspect people will use it otherwise and will
complain if we break it later.
But if we can make it work when the target_handle is valid, that would
be nice.
> > > +/* Set the user parameter. */
> > > +void debuginfod_set_user_data (debuginfod_client *client, void *value);
> > > +
> > > +/* Get the user parameter. */
> > > +void* debuginfod_get_user_data (debuginfod_client *client);
> >
> > In theory I like these additions. But I don't really see the point of
> > how they are used. Is the only use case to pass the string "Progress"?
>
> That is for test coverage.
Better to have a "real" test imho. Or at least comment it, so someone
doesn't clean up the code.
> > If there are no real users for this then I think we should not add
> > these at this time. Or is some other client using them? I am not
> > really
> > against it, but would prefer if we add them separately to keep the
> > patches concise.
>
> GDB would use them pretty much immediately, to be able to prepare a
> more informative progress notification (like the file name whose
> debuginfo is being sought, instead of its buildid).
OK. Yes, that seems like a good use case.
But can we add this as a separate feature in a separate commit?
Thanks,
Mark
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc.
2020-02-25 17:07 ` Mark Wielaard
@ 2020-02-25 18:51 ` Frank Ch. Eigler
2020-03-20 0:31 ` PR25369 rfc slice 1: debuginfod_get/set_user_data Frank Ch. Eigler
0 siblings, 1 reply; 9+ messages in thread
From: Frank Ch. Eigler @ 2020-02-25 18:51 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel, simark
Hi -
> > I think it has a chance to be useful to other clients too, for example
> > for other proxy / authentication schemes. And given that there is a
> > shared library boundary, private APIs aren't in easy reach.
> > "separately" as in separate commits? ... I suppose, if it really
> > matters.
>
> Yes, please as a separate commit. It makes it so much easier on the
> reviewers if the patches are smaller.
OK, trading merge pains I guess.
> The reason I am slightly hesitant is because this feels like a feature
> with corner cases that might not be clear. What about other headers
> than Agent string if they have been set/will be set for example.
The agent one is the only one that the client library currently sets
on its own initiative by default. That's why it's a special case.
The underlying libcurl api does not let one -remove- a custom header,
only add new ones.
> Could you expand a little on the use case? I see you set an X-
> Forwarded-For header, but that seems it can be easily forged. I see
> it might be interesting for testing, but would you use it in
> production?
These XFF headers are interesting when one wants to backtrace a
federated request. Within a trusted tree of servers, it lets an
administrator figure out the ultimate client, for blacklisting or
special processing if necessary. So yes, absolutely in production.
(It doesn't matter if the initial parts are forged by a client as
long as the trailing parts are appended by the trusted tree.)
> > Clarified this in a followup patch. No, only valid during the progress
> > callback function itself.
>
> That is a pity, it seems useful without having to add a progress
> function. Then we should probably make sure it doesn't return a value
> in that case, because I suspect people will use it otherwise and will
> complain if we break it later.
It returns NULL.
> But if we can make it work when the target_handle is valid, that would
> be nice.
The duration of a debuginfod_find_* call is exactly when the
target_handle is valid. The latter gets cleaned up when the former
finishes up.
> > > > +/* Set the user parameter. */
> > > > +void debuginfod_set_user_data (debuginfod_client *client, void *value);
> > > > +
> > > > +/* Get the user parameter. */
> > > > +void* debuginfod_get_user_data (debuginfod_client *client);
> > >
> > > In theory I like these additions. But I don't really see the point of
> > > how they are used. Is the only use case to pass the string "Progress"?
> >
> > That is for test coverage.
>
> Better to have a "real" test imho. Or at least comment it, so someone
> doesn't clean up the code.
It's a real test in that the testsuite asserts that the value is
preserved. I can certainly clarify the comment.
> But can we add this as a separate feature in a separate commit?
OK.
- FChE
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PR25369 rfc slice 1: debuginfod_get/set_user_data
2020-02-25 18:51 ` Frank Ch. Eigler
@ 2020-03-20 0:31 ` Frank Ch. Eigler
2020-03-22 16:08 ` Mark Wielaard
0 siblings, 1 reply; 9+ messages in thread
From: Frank Ch. Eigler @ 2020-03-20 0:31 UTC (permalink / raw)
To: Mark Wielaard, elfutils-devel, simark
Hi -
At long last, slicing and dicing this patch from February into
individual (cumulative) commits. 1/3:
commit d307089fa8621edf09d66665d0a1ffc123b904b2
Author: Frank Ch. Eigler <fche@redhat.com>
Date: Thu Mar 19 20:27:11 2020 -0400
debuginfod client API: add get/set user_data functions
Add a pair of functions to associate a void* parameter with a client
object. Requested by GDB team as a way to pass file names and such
user-interface data through to a progressfn callback.
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 8923099fd89d..2730dbf7aee5 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -79,6 +79,9 @@ struct debuginfod_client
/* Progress/interrupt callback function. */
debuginfod_progressfn_t progressfn;
+ /* Stores user data. */
+ void* user_data;
+
/* Can contain all other context, like cache_path, server_urls,
timeout or other info gotten from environment variables, the
handle data, etc. So those don't have to be reparsed and
@@ -902,6 +905,19 @@ debuginfod_begin (void)
return client;
}
+void
+debuginfod_set_user_data(debuginfod_client *client,
+ void *data)
+{
+ client->user_data = data;
+}
+
+void *
+debuginfod_get_user_data(debuginfod_client *client)
+{
+ return client->user_data;
+}
+
void
debuginfod_end (debuginfod_client *client)
{
diff --git a/debuginfod/debuginfod-find.c b/debuginfod/debuginfod-find.c
index 8bd3a3dba7a0..0b1ca9cf1d76 100644
--- a/debuginfod/debuginfod-find.c
+++ b/debuginfod/debuginfod-find.c
@@ -91,6 +91,9 @@ main(int argc, char** argv)
return 1;
}
+ /* Exercise user data pointer, to support testing only. */
+ debuginfod_set_user_data (client, (void *)"Progress");
+
int remaining;
(void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER|ARGP_NO_ARGS, &remaining, NULL);
@@ -130,6 +133,8 @@ main(int argc, char** argv)
return 1;
}
+ debuginfod_end (client);
+
if (rc < 0)
{
fprintf(stderr, "Server query failed: %s\n", strerror(-rc));
@@ -137,9 +142,7 @@ main(int argc, char** argv)
}
printf("%s\n", cache_name);
-
free (cache_name);
- debuginfod_end (client);
return 0;
}
diff --git a/debuginfod/debuginfod.h b/debuginfod/debuginfod.h
index b4b6a3d2a6b9..fe72f16e9bd8 100644
--- a/debuginfod/debuginfod.h
+++ b/debuginfod/debuginfod.h
@@ -1,5 +1,5 @@
/* External declarations for the libdebuginfod client library.
- Copyright (C) 2019 Red Hat, Inc.
+ Copyright (C) 2019-2020 Red Hat, Inc.
This file is part of elfutils.
This file is free software; you can redistribute it and/or modify
@@ -54,7 +54,7 @@ debuginfod_client *debuginfod_begin (void);
return a posix error code. If successful, set *path to a
strdup'd copy of the name of the same file in the cache.
Caller must free() it later. */
-
+
int debuginfod_find_debuginfo (debuginfod_client *client,
const unsigned char *build_id,
int build_id_len,
@@ -75,6 +75,12 @@ typedef int (*debuginfod_progressfn_t)(debuginfod_client *c, long a, long b);
void debuginfod_set_progressfn(debuginfod_client *c,
debuginfod_progressfn_t fn);
+/* Set the user parameter. */
+void debuginfod_set_user_data (debuginfod_client *client, void *value);
+
+/* Get the user parameter. */
+void* debuginfod_get_user_data (debuginfod_client *client);
+
/* Release debuginfod client connection context handle. */
void debuginfod_end (debuginfod_client *client);
diff --git a/debuginfod/libdebuginfod.map b/debuginfod/libdebuginfod.map
index 0d26f93e04fe..a919630dacce 100644
--- a/debuginfod/libdebuginfod.map
+++ b/debuginfod/libdebuginfod.map
@@ -8,3 +8,8 @@ ELFUTILS_0.178 {
debuginfod_find_source;
debuginfod_set_progressfn;
} ELFUTILS_0;
+ELFUTILS_0.179 {
+ global:
+ debuginfod_set_user_data;
+ debuginfod_get_user_data;
+};
diff --git a/doc/Makefile.am b/doc/Makefile.am
index b5db01ff75d5..87d1fee03302 100644
--- a/doc/Makefile.am
+++ b/doc/Makefile.am
@@ -1,7 +1,7 @@
## Process this file with automake to create Makefile.in
## Configure input file for elfutils.
##
-## Copyright (C) 1996-2001, 2002, 2005, 2019 Red Hat, Inc.
+## Copyright (C) 1996-2001, 2002, 2005, 2019-2020 Red Hat, Inc.
## This file is part of elfutils.
##
## This file is free software; you can redistribute it and/or modify
@@ -24,7 +24,14 @@ notrans_dist_man1_MANS=
if DEBUGINFOD
notrans_dist_man8_MANS += debuginfod.8
-notrans_dist_man3_MANS += debuginfod_find_debuginfo.3 debuginfod_find_source.3 debuginfod_find_executable.3 debuginfod_set_progressfn.3
+notrans_dist_man3_MANS += debuginfod_begin.3
+notrans_dist_man3_MANS += debuginfod_end.3
+notrans_dist_man3_MANS += debuginfod_find_debuginfo.3
+notrans_dist_man3_MANS += debuginfod_find_executable.3
+notrans_dist_man3_MANS += debuginfod_find_source.3
+notrans_dist_man3_MANS += debuginfod_get_user_data.3
+notrans_dist_man3_MANS += debuginfod_set_progressfn.3
+notrans_dist_man3_MANS += debuginfod_set_user_data.3
notrans_dist_man1_MANS += debuginfod-find.1
endif
diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debuginfo.3
index f9e770b0c530..d975927f757b 100644
--- a/doc/debuginfod_find_debuginfo.3
+++ b/doc/debuginfod_find_debuginfo.3
@@ -21,9 +21,15 @@ debuginfod_find_debuginfo \- request debuginfo from debuginfod
.nf
.B #include <elfutils/debuginfod.h>
.PP
+Link with \fB-ldebuginfod\fP.
+
+CONNECTION HANDLE
+
.BI "debuginfod_client *debuginfod_begin(void);"
.BI "void debuginfod_end(debuginfod_client *" client ");"
+LOOKUP FUNCTIONS
+
.BI "int debuginfod_find_debuginfo(debuginfod_client *" client ","
.BI " const unsigned char *" build_id ","
.BI " int " build_id_len ","
@@ -38,12 +44,15 @@ debuginfod_find_debuginfo \- request debuginfo from debuginfod
.BI " const char *" filename ","
.BI " char ** " path ");"
+OPTIONAL FUNCTIONS
+
.BI "typedef int (*debuginfod_progressfn_t)(debuginfod_client *" client ","
.BI " long a, long b);"
.BI "void debuginfod_set_progressfn(debuginfod_client *" client ","
.BI " debuginfod_progressfn_t " progressfn ");"
-
-Link with \fB-ldebuginfod\fP.
+.BI "void debuginfod_set_user_data(debuginfod_client *" client ","
+.BI " void *" data ");"
+.BI "void* debuginfod_get_user_data(debuginfod_client *" client ");"
.SH DESCRIPTION
@@ -102,7 +111,12 @@ to the client cache and a file descriptor to that file is returned.
The caller needs to \fBclose\fP() this descriptor. Otherwise, a
negative error code is returned.
-.SH "PROGRESS CALLBACK"
+.SH "OPTIONAL FUNCTIONS"
+
+A small number of optional functions are available to tune or query
+the operation of the debuginfod client.
+
+.SS "PROGRESS CALLBACK"
As the \fBdebuginfod_find_*\fP() functions may block for seconds or
longer, a progress callback function is called periodically, if
@@ -125,6 +139,14 @@ continue the work, or any other value to stop work as soon as
possible. Consequently, the \fBdebuginfod_find_*\fP() function will
likely return with an error, but might still succeed.
+.SS "USER DATA POINTER"
+
+A single \fIvoid *\fP pointer associated with the connection handle
+may be set any time via
+.BR \%debuginfod_set_user_data () ,
+and retrieved via
+.BR \%debuginfod_get_user_data () .
+The value may be NULL.
.SH "CACHE"
If the query is successful, the \fBdebuginfod_find_*\fP() functions save
diff --git a/doc/debuginfod_get_user_data.3 b/doc/debuginfod_get_user_data.3
new file mode 100644
index 000000000000..16279936e2ea
--- /dev/null
+++ b/doc/debuginfod_get_user_data.3
@@ -0,0 +1 @@
+.so man3/debuginfod_find_debuginfo.3
diff --git a/doc/debuginfod_set_user_data.3 b/doc/debuginfod_set_user_data.3
new file mode 100644
index 000000000000..16279936e2ea
--- /dev/null
+++ b/doc/debuginfod_set_user_data.3
@@ -0,0 +1 @@
+.so man3/debuginfod_find_debuginfo.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PR25369 rfc slice 1: debuginfod_get/set_user_data
2020-03-20 0:31 ` PR25369 rfc slice 1: debuginfod_get/set_user_data Frank Ch. Eigler
@ 2020-03-22 16:08 ` Mark Wielaard
2020-03-22 18:09 ` Frank Ch. Eigler
0 siblings, 1 reply; 9+ messages in thread
From: Mark Wielaard @ 2020-03-22 16:08 UTC (permalink / raw)
To: Frank Ch. Eigler, elfutils-devel, simark
Hi Frank,
Sorry, I missed the earlier, the mailinglist setup changed and
suppressed the email to the list because you had CCed me... oops.
I think I fixed it for me personally, but others might want to get a
password reminder for this list and check that "Avoid duplicate copies
of messages?" hasn't been accidentally turned to "Yes".
On Thu, 2020-03-19 at 20:31 -0400, Frank Ch. Eigler wrote:
> commit d307089fa8621edf09d66665d0a1ffc123b904b2
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date: Thu Mar 19 20:27:11 2020 -0400
>
> debuginfod client API: add get/set user_data functions
>
> Add a pair of functions to associate a void* parameter with a client
> object. Requested by GDB team as a way to pass file names and such
> user-interface data through to a progressfn callback.
Looks good, but missing a ChangeLog entry, so it is slightly harder to
see which changes were intended. In general looks good. I had rather
seen a separate testcase than reusing debuginfod-find for that. Not
that there is that much to test.
One suggestion, it be a good idea IMHO to initialize user_data to NULL.
Just so a client knows whether or not it has been set or it is a random
value.
> diff --git a/debuginfod/debuginfod-find.c b/debuginfod/debuginfod-find.c
> index 8bd3a3dba7a0..0b1ca9cf1d76 100644
> --- a/debuginfod/debuginfod-find.c
> +++ b/debuginfod/debuginfod-find.c
> [...]
> @@ -130,6 +133,8 @@ main(int argc, char** argv)
> return 1;
> }
>
> + debuginfod_end (client);
> +
> if (rc < 0)
> {
> fprintf(stderr, "Server query failed: %s\n", strerror(-rc));
> @@ -137,9 +142,7 @@ main(int argc, char** argv)
> }
>
> printf("%s\n", cache_name);
> -
> free (cache_name);
> - debuginfod_end (client);
>
> return 0;
> }
Why is the debuginfo_end () call moved?
It is hard to see in this diff whether there are any uses of the client
handle in between the old and new location. But it looks there is none,
so this should be fine.
> diff --git a/doc/Makefile.am b/doc/Makefile.am
> index b5db01ff75d5..87d1fee03302 100644
> --- a/doc/Makefile.am
> +++ b/doc/Makefile.am
> [...]
> @@ -24,7 +24,14 @@ notrans_dist_man1_MANS=
>
> if DEBUGINFOD
> notrans_dist_man8_MANS += debuginfod.8
> -notrans_dist_man3_MANS += debuginfod_find_debuginfo.3 debuginfod_find_source.3 debuginfod_find_executable.3 debuginfod_set_progressfn.3
> +notrans_dist_man3_MANS += debuginfod_begin.3
> +notrans_dist_man3_MANS += debuginfod_end.3
> +notrans_dist_man3_MANS += debuginfod_find_debuginfo.3
> +notrans_dist_man3_MANS += debuginfod_find_executable.3
> +notrans_dist_man3_MANS += debuginfod_find_source.3
> +notrans_dist_man3_MANS += debuginfod_get_user_data.3
> +notrans_dist_man3_MANS += debuginfod_set_progressfn.3
> +notrans_dist_man3_MANS += debuginfod_set_user_data.3
> notrans_dist_man1_MANS += debuginfod-find.1
> endif
What exactly is going on here?
Did we forget some, so they didn't get distributed?
> diff --git a/doc/debuginfod_find_debuginfo.3
> b/doc/debuginfod_find_debuginfo.3
> index f9e770b0c530..d975927f757b 100644
> --- a/doc/debuginfod_find_debuginfo.3
> +++ b/doc/debuginfod_find_debuginfo.3
> @@ -21,9 +21,15 @@ debuginfod_find_debuginfo \- request debuginfo
> from debuginfod
> .nf
> .B #include <elfutils/debuginfod.h>
> .PP
> +Link with \fB-ldebuginfod\fP.
Yes, this looks like a better location for that note.
> +.SS "USER DATA POINTER"
> +
> +A single \fIvoid *\fP pointer associated with the connection handle
> +may be set any time via
> +.BR \%debuginfod_set_user_data () ,
> +and retrieved via
> +.BR \%debuginfod_get_user_data () .
> +The value may be NULL.
So, if we decide that the initial value is NULL then this would be
where we would document that or say that the value is undefined unless
debuginfod_set_user_data () has never been called.
Thanks,
Mark
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PR25369 rfc slice 1: debuginfod_get/set_user_data
2020-03-22 16:08 ` Mark Wielaard
@ 2020-03-22 18:09 ` Frank Ch. Eigler
0 siblings, 0 replies; 9+ messages in thread
From: Frank Ch. Eigler @ 2020-03-22 18:09 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel, simark
Hi -
> One suggestion, it be a good idea IMHO to initialize user_data to NULL.
> Just so a client knows whether or not it has been set or it is a random
> value.
Will document it as "undefined".
> > + debuginfod_end (client);
> > +
> > if (rc < 0)
> > {
> > fprintf(stderr, "Server query failed: %s\n", strerror(-rc));
> > @@ -137,9 +142,7 @@ main(int argc, char** argv)
> > }
> >
> > printf("%s\n", cache_name);
> > -
> > free (cache_name);
> > - debuginfod_end (client);
> >
> > return 0;
> > }
>
> Why is the debuginfo_end () call moved?
For valgrind cleanliness in the case of rc < 0 failure exit.
> > if DEBUGINFOD
> > notrans_dist_man8_MANS += debuginfod.8
> > -notrans_dist_man3_MANS += debuginfod_find_debuginfo.3 debuginfod_find_source.3 debuginfod_find_executable.3 debuginfod_set_progressfn.3
> > +notrans_dist_man3_MANS += debuginfod_begin.3
> > +notrans_dist_man3_MANS += debuginfod_end.3
> > +notrans_dist_man3_MANS += debuginfod_find_debuginfo.3
> > +notrans_dist_man3_MANS += debuginfod_find_executable.3
> > +notrans_dist_man3_MANS += debuginfod_find_source.3
> > +notrans_dist_man3_MANS += debuginfod_get_user_data.3
> > +notrans_dist_man3_MANS += debuginfod_set_progressfn.3
> > +notrans_dist_man3_MANS += debuginfod_set_user_data.3
> > notrans_dist_man1_MANS += debuginfod-find.1
> > endif
>
> What exactly is going on here?
> Did we forget some, so they didn't get distributed?
Yup.
- FChE
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-22 18:09 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-25 3:35 PR25369 rfc: debuginfod client api extension for progressfn prettying etc Frank Ch. Eigler
2020-02-25 3:50 ` Simon Marchi
2020-02-25 15:25 ` Mark Wielaard
2020-02-25 15:32 ` Frank Ch. Eigler
2020-02-25 17:07 ` Mark Wielaard
2020-02-25 18:51 ` Frank Ch. Eigler
2020-03-20 0:31 ` PR25369 rfc slice 1: debuginfod_get/set_user_data Frank Ch. Eigler
2020-03-22 16:08 ` Mark Wielaard
2020-03-22 18:09 ` Frank Ch. Eigler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).